diff --git a/STYLE.md b/STYLE.md index f5ea0d5..7bbb891 100644 --- a/STYLE.md +++ b/STYLE.md @@ -8,7 +8,7 @@ This applies to everything a home owner or installer reads: notifications, phone - **Details**: the last line links to the add-on documentation and names the section with the rest: `Details: add-on documentation, section "" under Notifications.` Background, figures, reason codes and history go there or into the log, never into the body. - **Status row**: at most 15 words. **Banner**: at most 12 words. **Refusal**: at most 30 words, naming the setting by its label. **Option name**: at most 6 words, not counting the group prefix. **Option description**: at most 30 words. Where a value comes from belongs in DOCS.md. - **Log line** (WARNING/ERROR): one line, at most 25 words. The reasoning goes on a DEBUG line. Log lines stay in English and are not translated. -- **Sentences**: one idea each, active voice, present tense, with nothing nested in brackets. These translate well into Dutch and French. +- **Sentences**: one idea each, active voice, present tense, with nothing nested in brackets and no semicolons (a semicolon is a second sentence in disguise). These translate well into Dutch and French. ## 2. Numbers Use a number only if it helps the reader act, for example the peak limit, or what was asked and what was sent in a fault report. Write it the way `app/messages.py` does: `5 000 W`, `20 %`, `10 minutes`, `14:05`. diff --git a/goodwe_controller/DECISIONS/UX-08-short-notifications.md b/goodwe_controller/DECISIONS/UX-08-short-notifications.md index ce6648f..488a082 100644 --- a/goodwe_controller/DECISIONS/UX-08-short-notifications.md +++ b/goodwe_controller/DECISIONS/UX-08-short-notifications.md @@ -5,10 +5,10 @@ The owner decided on 2026-09-27: a title of at most about 5 everyday words and a **Unchanged:** every `notification_id`, every trigger and cadence (edges, SAFETY-34 re-announce, per-unit ids), every log line and every status row. The status banner and rows are phase 2b, refusals and option texts 2c, logs 2d; `ux08_allowlist.txt` lists them until then. ## Where a choice was made -1. **Severity of "not delivering" (SAFETY-42).** The notification is an *alarm*, so it may use 3 sentences. Its log line is ERROR and the alarm is latched. The page banner stays amber, as SAFETY-42 decided, because control keeps running. The three checks stay in their original order. +1. **Severity of "not delivering" (SAFETY-42).** The notification is an *alarm*, so it may use 3 sentences. Its log line is ERROR and the alarm is latched. The page banner stays amber, as SAFETY-42 decided, because control keeps running; the two answer different questions (is control running / must a person act) and HA shows no colour on a notification. The three checks stay in their original order. Clearing it takes stop then start, and the body says so. 2. **Figures left the bodies** of the frozen-meter card (the rail and the command), the circulating-power card (the watts and the time window) and the peak cards (the projection). None of them is needed to act, and all of them are still in the reason text that the log line and the status row carry. `test_freeze.py` and `test_watchdog.py` now pin that. The frozen-meter card can therefore no longer quote a wrong rail (SAFETY-20), because it quotes none. 3. **The over-rating alarm keeps its numbers** (asked, sent, and which limit cut it), because they are what a fault report needs. `ClampWatchdog.facts` carries them under the same keep-the-bug rule as `reason`, so the words and the numbers always describe the same tick, with asked before sent (SAFETY-40, `test_clamp.py` part 6). -4. **SAFETY-25 stays in the peak-reserve alarm** as its last sentence ("the reserve only limits peak shaving … down to its cut-out level"). The alarm now gives each branch its own action: at the reserve, lower it; reserve not set, set one; battery level unread, check the sensor. Before, every branch said "lower peak_soc_floor". +4. **SAFETY-25 stays in the peak-reserve alarm** as its last sentence ("the reserve only limits peak shaving … down to its cut-out level"). The alarm now gives each branch its own action: at the reserve, lower it; battery level unread, check the sensor; floor unknown, set the cut-out level. That last branch is `soc_floor is None`, which happens when an inverter's cut-out level is missing, not the peak reserve (that option always has a value). Before, every branch said "lower peak_soc_floor". The title, "Battery stopped limiting the peak", is true on all three branches. 5. **SAFETY-32 at a roster of one.** The body no longer ends in a separate "WHICH PACK" paragraph. The pack's name is part of the first sentence ("The battery of inverter-b …"), and at a roster of one that is the only difference. 6. **Dropped as background, now in DOCS:** "charging stays blocked" (peak reserve), "why no other guard fired" (circulating power, not delivering), and the saturation-freeze distinction (over-rating). "The meter cannot see this" stays in the circulating-power card, as the owner asked. -7. **The Details link** opens the add-on's documentation page (`/hassio/addon/<slug>/documentation`, with the slug taken from the container hostname) and names the section, because Home Assistant does not render heading anchors there. Outside an add-on container it names the section without a link. +7. **The Details link** opens the add-on's documentation page (`/hassio/addon/<slug>/documentation`, with the slug taken from the container hostname) and names the section. A `#heading` anchor was not used because nobody has checked that Home Assistant's documentation tab renders heading ids; if it does, adding one is a one-line change. Outside an add-on container it names the section without a link. diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 5c9cda5..6260c62 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -2816,7 +2816,7 @@ texts live in `app/messages.py`. Every notification keeps a fixed | Battery is not keeping up | warning | `goodwe_integrator_bound` | | Power request too high | alarm | `goodwe_output_clamp` | | Load too high for battery | alarm | `goodwe_peak_saturated` | -| Battery at its peak reserve | alarm | `goodwe_peak_soc_floor` | +| Battery stopped limiting the peak | alarm | `goodwe_peak_soc_floor` | | Inverters working against each other | alarm | `goodwe_circulating_power` | | *name* is not delivering | alarm | `goodwe_not_delivering_<name>` | | Meter reading may be stuck | alarm | `goodwe_meter_frozen` | @@ -2854,11 +2854,17 @@ than the inverter. Switching off load in the next few minutes is the only thing that still changes the bill. The projection and the watts are in the add-on log and on the status page. See *The capacity-peak guard*. -### Battery at its peak reserve +### Battery stopped limiting the peak + +Two words in this notification are settings. The **peak reserve** is +*Peak guard will not discharge below (%)* (`peak_soc_floor`). The +**cut-out level** is *Battery cut-out level (%)* (`bms_soc_floor`), where the +battery's own system stops discharging. The peak guard wanted to discharge to hold the 15-minute peak, but it may not go below the peak reserve (`peak_soc_floor`, SAFETY-24). It also stops when the -battery level cannot be read, or when no reserve is set. So this 15-minute +battery level cannot be read, or when an inverter's cut-out level is not set, +because the reserve is graded against it. So this 15-minute period will go over the peak limit and set or raise the month's billed peak. Charging from the grid stays blocked meanwhile. On a site with several inverters the notification names the battery at its limit (SAFETY-32): each battery is @@ -2901,8 +2907,9 @@ to the missing unit's rating. Check, in this order: (`bms_soc_floor`) says? If so, that setting does not describe this battery. 3. Is anything else writing to it? -The alarm is latched and survives restarts; start control on the add-on page -once you know why. See *A unit that stops DELIVERING is a third fault again*. +The alarm is latched and survives restarts. Control is still running, so to +clear it, press *Start / stop control* twice (stop, then start) once you know +why. See *A unit that stops DELIVERING is a third fault again*. ### Meter reading may be stuck diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 85115f4..0a10050 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -3917,7 +3917,7 @@ class Controller: await self.hass.call("persistent_notification", "create", messages.peak_reserve( g.ceiling_w, soc_unread=g.soc is None, - reserve_unset=(g.soc is not None + floor_unknown=(g.soc is not None and g.soc_floor is None), packs=(_blocked if len(self.inverters) > 1 else ())).data()) diff --git a/goodwe_controller/app/messages.py b/goodwe_controller/app/messages.py index 38501c9..b1682d7 100644 --- a/goodwe_controller/app/messages.py +++ b/goodwe_controller/app/messages.py @@ -83,7 +83,7 @@ class Notice: T_LOOP_BOUND = "Battery is not keeping up" T_OUTPUT_CLAMP = "Power request too high" T_PEAK_SATURATED = "Load too high for battery" -T_PEAK_RESERVE = "Battery at its peak reserve" +T_PEAK_RESERVE = "Battery stopped limiting the peak" T_CIRCULATING = "Inverters working against each other" T_NOT_DELIVERING = "{unit} is not delivering" S_NOT_DELIVERING = "An inverter is not delivering" @@ -109,8 +109,8 @@ LIMIT_WORDS = { def loop_bound() -> Notice: """SAFETY-40: the integrator bound. Normal in short bursts; not a fault.""" return Notice(ID_LOOP_BOUND, WARNING, T_LOOP_BOUND, - "The battery is not following the controller; short moments " - "are normal, repeated ones need a check.") + "The battery cannot give what the controller asks, which is " + "normal for a moment but needs a check if it keeps happening.") @dataclass(frozen=True) @@ -137,12 +137,13 @@ def output_clamp(clamps) -> Notice: c = clamps[0] who = f"For {c.unit}, the" if c.unit else "The" others = [x.unit for x in clamps[1:] if x.unit] - also = (f"; {names(others)} {'was' if len(others) == 1 else 'were'} cut too" - if others else "") + running = (f"{names(others)} {'was' if len(others) == 1 else 'were'} cut " + f"too, and control is still running." if others + else "Control is still running.") return Notice(ID_OUTPUT_CLAMP, ALARM, T_OUTPUT_CLAMP, f"{who} controller asked for {watts(c.asked_w)}, above " f"{LIMIT_WORDS.get(c.limit, LIMIT_WORDS['rating'])}, and sent " - f"{watts(c.sent_w)} instead{also}. Control is still running. " + f"{watts(c.sent_w)} instead. {running} " f"This is a software fault, so please report it to your " f"installer.") @@ -157,20 +158,24 @@ def peak_saturated(ceiling_w) -> Notice: f"the inverter.") -def peak_reserve(ceiling_w, *, soc_unread: bool, reserve_unset: bool, +def peak_reserve(ceiling_w, *, soc_unread: bool, floor_unknown: bool, packs=()) -> Notice: """SAFETY-24/25/32: the peak guard may not discharge any further. The last sentence is SAFETY-25 and must stay: the reserve binds the peak guard only, and normal home use goes on discharging below it. `packs` names the batteries at their limit, only on a site with several. + `floor_unknown`: the floor the guard grades against is None, which at a + roster means some inverter's cut-out level is missing (the peak reserve + itself always has a value) - so the action names the cut-out level. """ if soc_unread: why = "The battery level cannot be read" act = "Switch off large loads, and check the battery level sensor." - elif reserve_unset: - why = "No peak reserve is set" - act = "Switch off large loads, and set a peak reserve in the settings." + elif floor_unknown: + why = "The cut-out level of a battery is not known" + act = ("Switch off large loads, and set each battery's cut-out level " + "in the settings.") else: why = (f"The battery of {names(packs)} is at its peak reserve" if packs else "The battery is at its peak reserve") @@ -179,7 +184,7 @@ def peak_reserve(ceiling_w, *, soc_unread: bool, reserve_unset: bool, return Notice(ID_PEAK_RESERVE, ALARM, T_PEAK_RESERVE, f"{why}, so this 15-minute period will go over your " f"{watts(ceiling_w)} peak limit and raise this month's peak. " - f"{act} The reserve only limits peak shaving: normal home use " + f"{act} The reserve only applies to keeping the peak down: normal home use " f"can still discharge the battery below it, down to its " f"cut-out level.") @@ -194,10 +199,10 @@ def circulating(discharging=(), charging=()) -> Notice: what = ("One inverter was charging from another, and your meter " "cannot see this.") return Notice(ID_CIRCULATING, ALARM, T_CIRCULATING, - f"{what} Control is stopped and every inverter is set to " - f"0 W; if the grid power does not change now, switch the " - f"inverters off at the isolator. A restart does not clear " - f"this: start control on the add-on page once you know why.") + f"{what} Every inverter is set to 0 W: if the grid power does " + f"not change now, switch the inverters off at the isolator. " + f"Control stays stopped, even after a restart, until you " + f"start it on the add-on page once you know why.") def not_delivering(unit: str, slug: str) -> Notice: @@ -205,11 +210,11 @@ def not_delivering(unit: str, slug: str) -> Notice: return Notice( ID_NOT_DELIVERING.format(slug=slug), ALARM, T_NOT_DELIVERING.format(unit=unit), - f"{unit} gives less power than it is asked for, so the site is short; " - f"nothing was changed and control keeps running. Check that {unit} is " + f"{unit} gives less power than it is asked for, but nothing was " + f"changed and control keeps running. Check that {unit} is " f"on and connected, that its battery is not empty or cold, and that " - f"nothing else controls it. A restart does not clear this: start " - f"control on the add-on page once you know why.", + f"nothing else controls it. A restart does not clear this: once you " + f"know why, stop and start control on the add-on page.", section=S_NOT_DELIVERING) @@ -218,9 +223,9 @@ def meter_frozen(unchanged_s) -> Notice: return Notice(ID_METER_FROZEN, ALARM, T_METER_FROZEN, f"The meter has shown the same value for " f"{duration(unchanged_s)} while the controller asks the " - f"battery for full power. Control is still running and still follows this " - f"reading. Check the meter; if it is stuck, stop control on " - f"the add-on page.") + f"battery for full power. Control is still running and still " + f"follows this reading. Check the meter, and if it is stuck, " + f"stop control on the add-on page.") def samples() -> list: @@ -232,11 +237,11 @@ def samples() -> list: output_clamp([Clamp("inverter-a", -3000, -2500, "device"), Clamp("inverter-b", 6000, 4600, "max_w+device")]), peak_saturated(5000), - peak_reserve(5000, soc_unread=False, reserve_unset=False), - peak_reserve(5000, soc_unread=False, reserve_unset=False, + peak_reserve(5000, soc_unread=False, floor_unknown=False), + peak_reserve(5000, soc_unread=False, floor_unknown=False, packs=["inverter-b"]), - peak_reserve(5000, soc_unread=True, reserve_unset=False), - peak_reserve(5000, soc_unread=False, reserve_unset=True), + peak_reserve(5000, soc_unread=True, floor_unknown=False), + peak_reserve(5000, soc_unread=False, floor_unknown=True), circulating(["inverter-a"], ["inverter-b"]), circulating(), not_delivering("inverter-b", "inverter_b"), diff --git a/goodwe_controller/test_messages.py b/goodwe_controller/test_messages.py index 4918ea8..46582bc 100644 --- a/goodwe_controller/test_messages.py +++ b/goodwe_controller/test_messages.py @@ -112,6 +112,8 @@ def body_problems(body, severity): out.append(f"{severity} body over 1 sentence ({n_s})") if n_w > 25: out.append(f"{severity} body over 25 words ({n_w})") + if ";" in body: + out.append("semicolon: split it into sentences or drop a clause") return out + text_problems(body, keys=False, entities=False) @@ -297,7 +299,9 @@ def key_of(kind, text): ALLOW_FILE = HERE / "ux08_allowlist.txt" -# The allowlist may never grow: lower this number when you delete lines. +# The allowlist may never grow. The test demands the list be exactly this long, +# so deleting lines means lowering this number in the same change, and adding +# one means raising it where a reviewer sees it. ALLOWLIST_MAX = 200 @@ -340,8 +344,9 @@ for k in stale: f"delete its line from ux08_allowlist.txt") check(f"no allowlist entry is stale, so the list only shrinks " f"({len(stale)} stale)", not stale) -check(f"the allowlist has not grown ({len(allow)} entries, cap {ALLOWLIST_MAX})", - len(allow) <= ALLOWLIST_MAX) +check(f"the allowlist is exactly its cap ({len(allow)} entries, cap " + f"{ALLOWLIST_MAX}): lower the cap when you delete lines", + len(allow) == ALLOWLIST_MAX) print(f"CI: UX-08 allowlist {len(allow)} entries " + ", ".join(f"{k} {sum(1 for x in allow if x.startswith(k + ' '))}" for k in LIMITS)) @@ -366,6 +371,8 @@ check("a six-word title is refused", check("a GoodWe prefix is refused", bool(title_problems("GoodWe: meter stuck"))) check("a second sentence in a warning is refused", bool(body_problems("The battery lags. Check it.", messages.WARNING))) +check("a semicolon run-on is refused, in an alarm too", + bool(body_problems("It stopped; check it.", messages.ALARM))) check("a fourth sentence in an alarm is refused", bool(body_problems("One. Two. Three. Four.", messages.ALARM))) check("an internal name is refused", bool(body_problems( diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index 483fd67..1884bbd 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -2528,7 +2528,7 @@ _body25 = hass25.last_message("goodwe_peak_soc_floor") def _says_scope(body): """SAFETY-25 in messages.peak_reserve()'s words: the reserve binds the peak guard only, and home use goes on discharging below it to the cut-out.""" - return ("The reserve only limits peak shaving" in body + return ("The reserve only applies to keeping the peak down" in body and "normal home use can still discharge the battery below it" in body and "down to its cut-out level" in body) diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index 98558d1..77ca082 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -1112,8 +1112,8 @@ def _says_three(m): """UX-08: the three things only this alarm can say, in its plain words (messages.circulating): the meter cannot see it, control is stopped at 0 W, and if the grid does not move a person at the isolator is next.""" - return ("meter cannot see this" in m and "Control is stopped" in m - and "every inverter is set to 0 W" in m and "isolator" in m + return ("meter cannot see this" in m and "Control stays stopped" in m + and "Every inverter is set to 0 W" in m and "isolator" in m and "if the grid power does not change" in m)