From 55a6d5dcdb01eb50614f0271d0656df12e14172f Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sat, 26 Sep 2026 23:08:44 +0200 Subject: [PATCH] SAFETY-40 review: integrator-bound WARNING gets its own notification id A later anti-windup notice could replace a write-path BUG SIGNAL notice under the shared goodwe_output_clamp id (escalation ignores the budget). The WARNING now uses goodwe_integrator_bound and points to DOCS; the cadence sentence in DOCS is corrected. Tests pin both ids (red on fd14569). Co-Authored-By: Claude Opus 5.5 --- ...AFETY-40-the-integrator-bound-is-not-a-bug.md | 6 ++++-- goodwe_controller/DOCS.md | 14 ++++++++------ goodwe_controller/app/main.py | 9 ++++++--- goodwe_controller/test_clamp.py | 16 ++++++++++++---- goodwe_controller/test_freeze.py | 5 +++-- 5 files changed, 33 insertions(+), 17 deletions(-) diff --git a/goodwe_controller/DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md b/goodwe_controller/DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md index 2a41f95..521c806 100644 --- a/goodwe_controller/DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md +++ b/goodwe_controller/DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md @@ -2,7 +2,7 @@ **Decision.** `i_clamped` (the integrator reaching `integrator_max_w`, i.e. anti-windup) keeps alarming through the same `ClampWatchdog`: it is counted, -notified on the same id and cadence, and escalates on a sustained run +notified on the same cadence, and escalates on a sustained run (SAFETY-41). It gets its own text, a `WARNING` and an amber status row. Only `clamped` (a command over `max_w`) and write-path clamps keep the BUG SIGNAL, the `ERROR` and the red row. @@ -16,7 +16,9 @@ say less." **Why a bug has its own budget.** A genuine over-rating must not wait up to `REASSERT_S` behind anti-windup notices. `ClampWatchdog` tracks a separate `last_bug_alarm`, and a bug's text is never overwritten by later anti-windup -text within an episode. With only bug engagements, the cadence is the same as +text within an episode. Across episodes the WARNING uses its own +notification id (`goodwe_integrator_bound`), so it cannot replace a BUG SIGNAL +notice still on screen (review of #130). With only bug engagements, the cadence is the same as before (`test_clamp.py` part 9). **Numbers.** They come from the `Decision` of the clamping tick (`i_bound_w`, diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index ecdd5cc..a4a7faf 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -1641,10 +1641,10 @@ other guard's entries out of it. **There is nothing to configure**, deliberately. The bound is `max_w`, which you already set; giving the guard a limit of its own would be a second number that can disagree with the first, which is exactly the failure the startup validation -exists to prevent. The alarm's cadence - at most one notification per 150 s -(an episode that has not yet notified does so once it has engaged 3 cycles in -a row), cleared after 3 clean -cycles - is fixed for the same reason `RAIL_FRAC` is: it describes the +exists to prevent. The alarm's cadence - normally at most one notification per +150 s (an episode that has not yet notified does so once it has engaged 3 +cycles in a row, and an over-rating has its own 150 s budget), cleared after 3 +clean cycles - is fixed for the same reason `RAIL_FRAC` is: it describes the notification bus, not the plant. WARNING: **on a default install the engagement you will see is the integrator's, @@ -1657,10 +1657,12 @@ every standard configuration. **The integrator bound is not a bug, and it is reported as what it is.** It reads *"the loop's integrator reached its +5000 W anti-windup bound; the loop's command this cycle was +2000 W"*, titled *"the control loop is at its integrator -bound"*, logged as a `WARNING`, with an amber (not red) status row. Brief +bound"*, logged as a `WARNING`, with an amber (not red) status row and its own +notification (`goodwe_integrator_bound`), so it never replaces a BUG SIGNAL +notice still on screen. Brief engagements after a large load or solar step are normal. If it stays engaged, check that the battery is following its commands. Only a command over the rating -carries the BUG SIGNAL text, and it never waits behind integrator-bound notices. +carries the BUG SIGNAL text, and it has its own notification budget. Why: [`DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md`](DECISIONS/SAFETY-40-the-integrator-bound-is-not-a-bug.md). ### The frozen-meter alarm diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index a60afb5..e8160d4 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -3762,14 +3762,17 @@ class Controller: return if not self.clamp_wd.bug: # SAFETY-40: the integrator bound is anti-windup, not a bug - a - # WARNING and a plainly titled notice, same id and cadence. + # WARNING and a plainly titled notice, same cadence. Its OWN id, so + # it can never replace a BUG SIGNAL notice still on screen. _LOG.warning("SAFETY-03 - %s Engagements so far: %d.", self.clamp_wd.reason, self.clamp_wd.engagements) await self.hass.call("persistent_notification", "create", { - "notification_id": "goodwe_output_clamp", + "notification_id": "goodwe_integrator_bound", "title": "GoodWe: the control loop is at its integrator bound", "message": (f"{self.clamp_wd.reason}\n\nNo command exceeded " - f"the rating and the controller is STILL RUNNING."), + f"the rating and the controller is STILL RUNNING. " + f"See the add-on documentation, 'The winding-loop " + f"alarm'."), }) return _LOG.error( diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index be9df87..d6dcdf6 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -102,6 +102,9 @@ T0 = datetime(2026, 8, 30, 9, 0, 0, tzinfo=timezone.utc) # which is also the device rating, so this is the bound the alarm is written at. LIVE_MAX_W = 5000.0 CLAMP_ID = "goodwe_output_clamp" +# SAFETY-40: the integrator-bound WARNING has its own id, so it can never +# replace a BUG SIGNAL notice still on screen. +BOUND_ID = "goodwe_integrator_bound" def check(name, cond): @@ -461,7 +464,7 @@ def wired(grid, cycles=40, batt=0.0, track=True, **over): def clamp_notes(hass): return [d for dom, svc, d in hass.calls if (dom, svc) == ("persistent_notification", "create") - and d.get("notification_id") == CLAMP_ID] + and d.get("notification_id") in (CLAMP_ID, BOUND_ID)] # THE 10.3 RUNAWAY, replayed. "With the inverter switched off entirely, an @@ -500,8 +503,9 @@ print(f" the loop clamped on {ctl_r.clamp_wd.engagements} of {60} cycles, " f"notification(s)") check("AC 3: and it ALARMED - silently clipping this is the defect", ctl_r.clamp_wd.engagements > 0 and len(notes_r) >= 1) -check("...with its own notification_id, not shared with any other guard", - all(d["notification_id"] == CLAMP_ID for d in notes_r)) +check("...with its own notification_id, not shared with any other guard - " + "on a default install the integrator bound's (SAFETY-40)", + all(d["notification_id"] == BOUND_ID for d in notes_r)) check("...one episode, re-asserted rather than one per cycle", ctl_r.clamp_wd.episodes == 1 and len(notes_r) < ctl_r.clamp_wd.engagements) # SAFETY-40: on a default install this runaway is the INTEGRATOR bound, so it @@ -1971,6 +1975,9 @@ notes_i = clamp_notes(hass_i) check("it still NOTIFIES (not suppressed), under a title that claims no bug", notes_i and all("integrator bound" in n["title"] and "exceeded" not in n["title"] for n in notes_i)) +check("...under its OWN notification_id, so it cannot replace a BUG SIGNAL " + "notice still on screen", + notes_i and all(n["notification_id"] == BOUND_ID for n in notes_i)) check("...logged as a WARNING, not an ERROR", any(x.levelno == logging.WARNING and "anti-windup" in x.getMessage() for x in log_i) @@ -2003,7 +2010,8 @@ check("...and the 14 768 W rationale is still in its ERROR", for x in log_o)) check("...under the 'exceeded the rating' title, with a red status row", clamp_notes(hass_o) - and all("exceeded the rating" in n["title"] for n in clamp_notes(hass_o)) + and all("exceeded the rating" in n["title"] + and n["notification_id"] == CLAMP_ID for n in clamp_notes(hass_o)) and any(not c["warn"] and "OUTPUT CLAMP ENGAGED" in c["text"] for c in red_o)) diff --git a/goodwe_controller/test_freeze.py b/goodwe_controller/test_freeze.py index a5e0f00..661113d 100644 --- a/goodwe_controller/test_freeze.py +++ b/goodwe_controller/test_freeze.py @@ -835,7 +835,8 @@ frozen_rows = [c for c in ctl_f.checks() if "METER MAY BE FROZEN" in c["text"]] print(f" frozen at +4000 W: command ran to {ctl_f.target:.0f} W, " f"{len(notes)} notification(s), {len(frozen_rows)} red status row(s)") # SURFACE-WIDE, deliberately, and the second guard has now landed. SAFETY-03 -# ships `goodwe_output_clamp`, and this scenario - a runaway loop winding into +# ships `goodwe_output_clamp` (and, for the integrator bound on a default +# install, `goodwe_integrator_bound` - SAFETY-40), and this scenario - a runaway loop winding into # the rail - raises it legitimately: the loop asks for more than the rating and # is clamped. So the answer to "is this scenario still meant to raise exactly # one alarm" is NO, it raises exactly two, and BOTH ARE NAMED here rather than @@ -846,7 +847,7 @@ _ids = sorted(d.get("notification_id") for d in all_notes) print(f" the whole notification surface this run: {_ids}") check("the whole notification surface carried exactly two alarms this run - " "SAFETY-18's and SAFETY-03's, both named", - _ids == ["goodwe_meter_frozen", "goodwe_output_clamp"]) + _ids == ["goodwe_integrator_bound", "goodwe_meter_frozen"]) # THIS GUARD'S, keyed on the id. check("...and the freeze detector is the one that raised it", len(notes) == 1) # ⚠️ Selected by TEXT, not by id - see notes_by_text_of(). One of the two