From cc4e88490b4440874b7f867517e8849992e26260 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 22:33:15 +0200 Subject: [PATCH 1/2] SAFETY-02 review: a latch that a restart clears is not a latch BLOCKER. `auto` was persisted and `sign_wd.tripped` was not, so reconstructing the Controller against the same store - which is what a container restart does - brought it back ARMED, into the same fault, unattended: tripped=False, banner back to NOT READY, and it resumed commanding -1000 then -2000 W before re-tripping 180 s later. "Restart the add-on" is the first thing an operator looking at a FAILSAFE banner and a 0 W inverter will try, and under a supervisor that restarts a crashed add-on it is a loop of three-minute wrong-direction bursts. DOCS.md's "it does not clear itself" was false across a restart. `sign_tripped` and `sign_reason` are Store keys now, beside the maintenance state and for the reason store.py's own docstring gives. Written the moment the trip happens - reason first, flag second, so a crash between the two can never persist a latch with no explanation - restored in Controller.__init__, cleared only by the operator's start. The regression is covered: reverting the restore fails seven checks in test_watchdog.py. Review follow-ups, no behaviour change: - The arming gate is tested against the REAL arbiter. It is the decision the ticket turns on and it was asserted in three comment blocks and verified by nothing - every case passed `following` in as a literal. Twelve claim configurations now derive it from resolve(), including the redirect case `winner == "loop"` alone would have got wrong and the `lo` limit that raises magnitude without flipping sign. - The MQTT `status: "failsafe"` value is tested. The check that claimed to cover it restated the line above it and never touched the payload. - The worked example described a config that does not ship: at the defaults maintenance_charge_w 2500 against max_w 2000 is an arbiter contradiction and 0 W, not a -2500 W charge. Rewritten at 1500 W. The contradiction is a real pre-existing bug - a default cheap-window maintenance charge silently never happens - filed separately and deliberately not fixed here. - Forward hazard recorded at the gate: `winner == "loop"` disarms the watchdog under any higher-priority SET, and a peak-shaving strategy at P_PEAK IS grid-following in sign, so it would blind this for every peak event. - disagrees() is public so the replay harness stops carrying its own copy of the predicate it grades. - held_s takes the clock instead of reading one. - The two limits are now each stated in both places rather than one each. - The no-false-positive argument is tied to control.py's freeze permitting the command to cross zero; under the shrink-only encoding control.py records as rejected, this would false-trip on a load step. - Measured figures corrected: 93.4 % / 4212 of 17280, per-cluster 0.3/0.9/10.5/0.0, and the near-circularity of those figures is now stated rather than leaned on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- HANDOVER.md | 24 +++- goodwe_controller/CHANGELOG.md | 49 +++++++- goodwe_controller/DOCS.md | 21 +++- goodwe_controller/app/main.py | 79 ++++++++++--- goodwe_controller/app/store.py | 25 ++++- goodwe_controller/app/watchdog.py | 142 ++++++++++++++++++----- goodwe_controller/config.yaml | 4 +- goodwe_controller/test_watchdog.py | 175 ++++++++++++++++++++++++++++- 8 files changed, 458 insertions(+), 61 deletions(-) diff --git a/HANDOVER.md b/HANDOVER.md index 2e6bca1..5e242d9 100644 --- a/HANDOVER.md +++ b/HANDOVER.md @@ -57,11 +57,25 @@ python3 test_control.py && python3 test_maintenance.py && python3 test_arbiter.p && python3 test_loop.py ``` -`test_watchdog.py` covers SAFETY-02's logic and its trip path. Its headline -validation is not here at all: `sim/replay_sign.py` in the infra repo drives the -same module over the two captured RS485 days and must fire on all four of the -2026-08-20 episodes and stay silent on 2026-08-23. That needs both checkouts — -`GOODWE_ADDON_REPO=/path/to/goodwe-addon python3 sim/replay_sign.py`. +`test_watchdog.py` covers SAFETY-02's logic, its arming gate (derived from the +real arbiter, not passed in), its trip path, and the fact that the latch survives +an add-on restart. Its headline validation is not here at all: `sim/replay_sign.py` +in the infra repo drives the same module over the two captured RS485 days and must +fire on all four of the 2026-08-20 episodes and stay silent on 2026-08-23. That +needs both checkouts: + +```bash +GOODWE_ADDON_REPO=/path/to/goodwe-addon python3 sim/replay_sign.py +GOODWE_ADDON_REPO=/path/to/goodwe-addon python3 sim/replay_sign.py --stub +``` + +The second run checks the grader still *discriminates* rather than merely still +passing — a watchdog that never trips and one that always trips must both be +rejected. + +⚠️ **If you are testing the failsafe by hand, note that stopping and starting the +add-on will not clear a trip.** That is the point of it; use the *Start / stop +control* button twice, or delete `sign_tripped` from `/data/state.json`. ⚠️ `test_loop.py` takes about **95 seconds** (measured 93 s) and that is not a bug. It drives the real control loop, which sleeps 1 s a tick, eight times over, to prove a diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index c3203cf..628c811 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -1,5 +1,52 @@ # Changelog +## 0.5.1 + +**SAFETY-02's latch now survives an add-on restart, which is the difference +between a latch and a pause.** `auto` was persisted and `sign_wd.tripped` was +not, so reconstructing the `Controller` against the same store — exactly what a +container restart does — brought it back **armed, into the same fault, with no +operator involved**: `tripped=False`, banner back to `NOT READY`, and it resumed +commanding (measured: −1000 W, −2000 W) before re-tripping 180 s later. Under a +Supervisor that restarts a crashed add-on that is a loop of three-minute +wrong-direction bursts — and "restart the add-on" is the first thing an operator +looking at a `FAILSAFE` banner and a 0 W inverter will try. + +`sign_tripped` and `sign_reason` are now `Store` keys beside the maintenance +state, written the moment the trip happens (reason first, flag second, so a crash +between the two can never persist a latch with no explanation) and restored in +`Controller.__init__`. Cleared only by the operator's start. `DOCS.md` said "it +does not clear itself"; that was false across a restart and now is not. + +Also in this release, all of it review follow-up rather than new behaviour: + +- **The arming gate is tested against the real arbiter.** It was the decision the + whole ticket turns on and it was asserted in three comment blocks and verified + by nothing — every case passed `following` in as a literal. `test_watchdog.py` + now derives it from `resolve()` across twelve claim configurations, including + the redirect case `winner == "loop"` alone would have got wrong and the `lo` + limit that raises magnitude without flipping sign. +- **The MQTT `status: "failsafe"` value is tested.** The check that claimed to + cover it restated the assertion three lines above it and never touched the + payload. +- **A forward hazard written down where it will be read.** `winner == "loop"` + disarms the watchdog under any higher-priority SET claim. Today only + maintenance and the safety stops emit one and neither follows the meter; a + peak-shaving strategy at `P_PEAK` **does**, and would blind the watchdog for + every peak event. Noted at the gate itself, with what has to change. +- **The worked example described a configuration that does not ship.** At the + defaults `maintenance_charge_w` (2500) exceeds `max_w` (2000), so that claim is + an arbiter *contradiction* resolving to 0 W, not a −2500 W charge. The example + is now written at 1500 W. (The contradiction is a real pre-existing bug — + a default cheap-window maintenance charge silently never happens — filed + separately, deliberately not fixed here.) +- **`disagrees()` is public**, so `sim/replay_sign.py` stops carrying its own copy + of the predicate it grades. +- `held_s` takes the clock instead of reading one, restoring the pure-function + property the module advertises. +- Measured figures corrected throughout: 93.4 % / 4212 of 17280 (was 92.9 % / + 4127 of 16349), per-cluster 0.3 / 0.9 / 10.5 / 0.0 %. + ## 0.5.0 **SAFETY-02 — the sign-disagreement watchdog.** The add-on now watches whether @@ -23,7 +70,7 @@ module over the committed RS485 captures of both 2026-08-20 (the four episodes) and 2026-08-23 (healthy): **18 trips on the fault day, every one inside a window `NOTES.md` §4.7 accounts for, and 0 on the healthy day.** The four episodes do share one signature rather than merely resembling each other — sign agreement -runs 0.3 / 0.9 / 10.5 / 0.0 % inside them against 92.9 % on the healthy day. +runs 0.3 / 0.9 / 10.5 / 0.0 % inside them against 93.4 % on the healthy day. Three decisions worth knowing about: diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 1717980..f6b53db 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -294,11 +294,20 @@ there. The banner reads `FAILSAFE`, the commissioning list says which two numbers disagreed and for how long, and the `Controller status` entity reports `failsafe`. -**It does not clear itself.** Pressing *Start / stop control* twice clears it. -That is deliberate: commanding 0 W removes the command side of the comparison, -so the disagreement always vanishes the instant the trip works, and a watchdog -that took that as proof the fault was over would hand the inverter straight back -to the loop that was driving the wrong way. +**It does not clear itself, and restarting the add-on does not clear it either.** +Pressing *Start / stop control* twice — stop, then start — is the only thing that +does. That is deliberate on both counts: + +- Commanding 0 W removes the command side of the comparison, so the disagreement + always vanishes the instant the trip works. A watchdog that took that as proof + the fault was over would hand the inverter straight back to the loop that was + driving the wrong way, three minutes later, forever. +- The trip is written to `/data/state.json` and restored on startup. Restarting + the add-on is the first thing most people try when they see a red banner, and + a latch that a restart clears would bring the controller back **armed, into the + same fault, unattended** — and under a Supervisor that restarts a crashed add-on + automatically, would do so in a loop. After a restart the banner, the 0 W and + the recorded reason are all still there. **What it is for.** This is a loop-integrity check: it asks whether the loop's output direction still matches the input state it was regulating. It catches a @@ -326,7 +335,7 @@ must not be mistaken for a fault. Both numbers are measured rather than chosen, and are exposed only because the ticket requires defaults rather than constants. On the captured healthy day sign -agreement is 92.9 %; on the four captured fault episodes it is 0.3 %, 0.9 %, +agreement is 93.4 %; on the four captured fault clusters it is 0.3 %, 0.9 %, 10.5 % and 0.0 %. There is no reason to move either value without a capture that says so. diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index f34a9de..825b527 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -105,10 +105,27 @@ class Controller: ) # SAFETY-02. One instance, one actuator, one cadence - see watchdog.py. - self.sign_wd = SignWatchdog(SignWatchdogConfig( - min_w=float(opts.get("sign_disagree_w", 300)), - hold_s=float(opts.get("sign_disagree_s", 180)), - )) + # + # ⚠️ RESTORED FROM THE STORE, and that is the difference between a latch + # and a pause. A trip held only in memory is cleared by the one action an + # operator staring at a FAILSAFE banner is most likely to take - restart + # the add-on - and the controller then comes back armed, into the fault, + # with nobody involved. Restored here, the banner and the 0 W survive the + # restart and only `auto_toggle` clears them. + self.sign_wd = SignWatchdog( + SignWatchdogConfig( + min_w=float(opts.get("sign_disagree_w", 300)), + hold_s=float(opts.get("sign_disagree_s", 180)), + ), + tripped=bool(store.data.get("sign_tripped", False)), + reason=str(store.data.get("sign_reason", "") or ""), + ) + if self.sign_wd.tripped: + _LOG.error("SAFETY-02 was TRIPPED before this restart and stays " + "tripped: %s. The inverter is held at 0 W. Restarting the " + "add-on does not clear this - start control from the UI " + "once you know why the command opposed the meter.", + self.sign_wd.reason or "(reason not recorded)") # P1 ingestion (TEL-01). `meter_source: off` keeps the original # single-entity meter_entity path, so an existing install is unchanged @@ -243,9 +260,18 @@ class Controller: standing = self.last_write if self.last_write is not None else self.target if self.sign_wd.evaluate(standing, self.grid, self.following, now) \ and not was_tripped: + # ⚠️ REASON FIRST, FLAG SECOND, and each `set` writes the file. The + # ordering is the whole atomicity story: a crash between the two + # persists a reason nothing reads, which is harmless, where the other + # order persists a latch with no explanation for the operator who has + # to decide whether to clear it. + self.store.set("sign_reason", self.sign_wd.reason) + self.store.set("sign_tripped", True) self.log_event("FAILSAFE " + self.sign_wd.reason) _LOG.error("SAFETY-02 TRIPPED - %s. Commanding 0 W and holding it " - "until control is started again.", self.sign_wd.reason) + "until control is started again. This survives an add-on " + "restart; restarting will not clear it.", + self.sign_wd.reason) stale_after = float(self.o.get("stale_input_s", 15)) if not self.inputs_ok and not self.sign_wd.tripped: @@ -358,15 +384,33 @@ class Controller: # # ⚠️ "THE LOOP WON" IS NOT ENOUGH, and the difference is a whole class of # false trip. A LIMIT claim can survive the loop winning and still - # replace its number: maintenance's charge-only shaping is - # `hi = -charge_w`, so the loop can ask for +500 W of discharge, win the - # arbitration, and have the emitted value come out at -2500 W of grid - # charge. Winner says "loop"; the wire carries maintenance's intent. So - # the test is on the NUMBER, not on the name: the loop is following only - # while the emitted value points the way the loop asked and is no larger - # than it asked - i.e. limits reduced it, they did not redirect it. - # Zero on either side falls out as not-following, which costs nothing: - # a 0 W command cannot be a wrong-direction command. + # replace its number: maintenance's cheap-window shaping is + # `hi = -maintenance_charge_w`, so with `maintenance_charge_w: 1500` the + # loop can ask for +500 W of discharge, win the arbitration, and have the + # emitted value come out at -1500 W of grid charge. Winner says "loop"; + # the wire carries maintenance's intent. So the test is on the NUMBER, + # not on the name: the loop is following only while the emitted value + # points the way the loop asked and is no larger than it asked - i.e. + # limits reduced it, they did not redirect it. Zero on either side falls + # out as not-following, which costs nothing: a 0 W command cannot be a + # wrong-direction command. The `abs(...) <= abs(asked)` half is + # load-bearing on its own: a `lo` limit can raise the magnitude without + # flipping the sign, and that is not the loop following either. + # + # (1500 W because at the SHIPPED defaults - maintenance_charge_w 2500 vs + # max_w 2000 - that claim is `lo=-2000 > hi=-2500`, which the arbiter + # calls a contradiction and resolves to 0 W. Filed separately; the + # redirect hazard here is real at any charge rate inside max_w.) + # + # ⚠️ FORWARD HAZARD, AND IT IS NOT HYPOTHETICAL - the claim block above + # already anticipates it. `winner == "loop"` disarms this watchdog under + # ANY higher-priority SET claim. Today only maintenance and the safety + # stops emit one, and neither is grid-following, so the gate costs + # nothing. A peak-shaving strategy claiming SET at P_PEAK **is** + # grid-following in sign, and the moment it lands this watchdog goes + # blind for the duration of every peak event. Whoever adds it must widen + # the gate to "the winning strategy follows the meter" rather than + # "the winner is the loop", not just add another name to a comparison. asked = self.loop_w self.following = (resolution.winner == "loop" and asked is not None and resolution.target_w * asked > 0 @@ -600,9 +644,10 @@ class Controller: "text": "SIGN-DISAGREEMENT FAILSAFE - " + self.sign_wd.reason + ". The inverter is held at 0 W. Find out why the " "command opposed the meter before starting control " - "again; starting clears this."}) + "again; starting clears this. Restarting the " + "add-on does NOT clear it."}) else: - held = self.sign_wd.held_s + held = self.sign_wd.held_s(datetime.now(timezone.utc)) out.append({"ok": True, "warn": False, "text": f"sign watchdog: armed (trips after " f"{self.sign_wd.cfg.hold_s:.0f} s opposed, both over " @@ -672,6 +717,8 @@ class Controller: # An operator pressing start has, at minimum, looked at the # banner that says why. self.sign_wd.reset() + self.store.set("sign_tripped", False) + self.store.set("sign_reason", "") self.log_event("sign-disagreement failsafe cleared by operator") if not self.auto: self.target = 0.0 diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 91ac8a2..0d87959 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -22,11 +22,30 @@ _LOG = logging.getLogger("goodwe.store") # `auto_start` option only when the key is absent - if a default supplied False, # the fallback could never fire and auto_start would silently do nothing on # every fresh install. +# +# ⚠️ `sign_tripped` IS in here, and the difference from `auto` is the whole +# reason both decisions are written down. `auto` has an option to fall back to +# (`auto_start`), so a stored default would mask a fresh install. A latched +# failsafe has no such fallback: absent means "never tripped", which is exactly +# what a default of False says. +# +# It is persisted for the reason this module's docstring gives for maintenance +# state, applied to something with a much shorter fuse. SAFETY-02's latch is the +# only thing standing between a controller that has been caught driving the wrong +# way and its own restart, and "restart the add-on" is the FIRST thing an operator +# looking at a FAILSAFE banner will try. Held only in memory it came back armed, +# into the fault, unattended - measured: after reconstructing Controller against +# the same store, `tripped` was False, the banner read NOT READY, and it resumed +# commanding -1000, -2000 W before re-tripping 180 s later. Under a supervisor +# that restarts a crashed add-on that is a loop, three minutes of wrong-direction +# power at a time, forever. DEFAULTS = { "phase": "idle", "phase_started": None, "last_completed": None, "last_start_attempt": None, + "sign_tripped": False, + "sign_reason": "", } @@ -42,8 +61,10 @@ class Store: loaded = json.load(fh) if isinstance(loaded, dict): self.data = {**DEFAULTS, **loaded} - _LOG.info("state restored: phase=%s last_completed=%s", - self.data.get("phase"), self.data.get("last_completed")) + _LOG.info("state restored: phase=%s last_completed=%s " + "sign_tripped=%s", + self.data.get("phase"), self.data.get("last_completed"), + self.data.get("sign_tripped")) except FileNotFoundError: _LOG.info("no saved state, starting fresh") except (json.JSONDecodeError, OSError) as err: diff --git a/goodwe_controller/app/watchdog.py b/goodwe_controller/app/watchdog.py index 5a1d8d9..f0162f4 100644 --- a/goodwe_controller/app/watchdog.py +++ b/goodwe_controller/app/watchdog.py @@ -18,15 +18,36 @@ module over both of them. Every number in this file was measured on those two days, not chosen: day samples gated in sign agreement - 2026-08-23 4 127 / 16 349 92.9 % (healthy) - 2026-08-20 6 892 / 15 598 55.6 % (the whole fault day) - episode A 15:26-16:43 0.3 % - episode B 17:11-18:28 0.8 % - episode C 20:01-20:31 11.0 % - episode D 20:55-21:11 0.0 % + 2026-08-23 4 212 / 17 280 93.4 % (healthy) + 2026-08-20 7 182 / 17 280 56.4 % (the whole fault day) + cluster A 15:26-16:43 0.3 % + cluster B 17:11-18:28 0.9 % + cluster C 20:01-20:31 10.5 % + cluster D 20:55-21:11 0.0 % -That is the signature, and it is the SAME signature in all four episodes: not a -noisier version of healthy, a near-total inversion. +⚠️ THOSE AGREEMENT FIGURES ARE THE WEAKEST EVIDENCE ON THIS PAGE, and an earlier +version of this comment leaned on them as though they were the strongest. The +clusters are found by looking for sustained opposed samples, so measuring "how +opposed is this cluster" over them is close to a tautology - and "the healthy day +contains no such cluster" is the same sentence as "the healthy day produces no +trip", not independent corroboration of it. + +What actually carries the argument is two things neither the detector nor the +clustering assumes: + + 1. The RUN-LENGTH DISTRIBUTION. The longest unbroken opposed run on the healthy + day is 30 s. On the fault day there are 18 runs over 180 s and the longest is + 1750 s. Nothing about that gap was chosen - it is what the two days measure. + 2. The clusters land where a human independently wrote down onsets. NOTES.md + §4.7 records 15:01, 16:56, 19:57 and 20:23 from watching it happen, before + any of this existed, and cluster D coincides with the separately documented + inverter-off runaway of §4.7 item 2. + +Be precise about (2), because the convenient overstatement is available and wrong: +the clustering does NOT reproduce four episodes. It yields THREE clusters over the +four documented onsets - C spans both 19:57 and 20:23 - plus a fourth for §4.7 +item 2, which is not one of the four. Every trip does fall inside a documented +window, which is the claim worth making. ------------------------------------------------------------------------------ WHY IT LIVES HERE AND NOT IN THE FIRMWARE OR THE WITNESS @@ -68,21 +89,31 @@ Four gates, and each one exists to exclude a specific thing that is NOT a fault: 1. `following`. ⚠️ THE GATE THAT IS EASIEST TO FORGET AND MOST EXPENSIVE TO OMIT. Sign agreement is a property of a GRID-FOLLOWING loop. It is not expected of a strategy that deliberately commands against the meter - - maintenance is exactly that. A maintenance grid charge of 2500 W into a house - drawing 500 W puts +3000 W on the meter against a -2500 W command: opposed, + maintenance is exactly that. A maintenance grid charge of 1500 W into a house + drawing 500 W puts +2000 W on the meter against a -1500 W command: opposed, both far over any sane threshold, and sustained for HOURS. Armed unconditionally this watchdog would trip on every single maintenance cycle, which is not a tuning problem, it is the wrong question being asked. So the caller says whether the grid-following loop actually won the arbitration this cycle, and the timer only accumulates when it did. + ⚠️ 1500 W, NOT THE DEFAULT 2500 W, AND THE REASON IS NOT COSMETIC. At the + shipped defaults `maintenance_charge_w` is 2500 and `max_w` is 2000, so the + cheap-window claim is `hi = -2500` against a supervised `lo = -2000`: lo > hi, + which arbiter.py resolves as a CONTRADICTION and 0 W, not as a -2500 W charge. + The example above is written at 1500 W because that is a combination the + schema and the arbiter both actually produce. (The contradiction at the + defaults is a real pre-existing bug - a default maintenance charge silently + never happens in a cheap window - but it is maintenance's, not this module's, + and it is filed separately. Do not "fix" it here.) + 2. The strict `< 0` product. "Opposed" and "neither side is zero" in one expression. 3. The 300 W magnitude gate ON BOTH SIDES. Near zero the sign is noise, and near zero is where a healthy loop LIVES - the deadband holds the command still while the meter wanders either side of `target_grid_w`. On the healthy day - only 4 127 of 16 349 samples (25 %) are even gated in; the other 75 % are the + only 4 212 of 17 280 samples (24 %) are even gated in; the other 76 % are the converged band where sign carries no information. This is also why the deadband cannot produce a false trip: inside it the meter is by definition under 300 W. @@ -109,6 +140,33 @@ a stuck reading of +800 W makes the loop command discharge and keep commanding discharge. Command and meter converge on AGREEING. Freezes are SAFETY-12's and SAFETY-01's problem, and this watchdog neither helps nor hinders there. +⚠️ ALL OF THAT DEPENDS ON ONE PROPERTY OF THE CONTROL LAW, so it is an invariant +and not a happy accident: the saturation freeze in control.py must keep letting +the command FALL, CROSS ZERO AND REVERSE - "may not wind further in the direction +it is already pushing", not "only corrections that shrink |i_w|". control.py +records the shrink-only encoding as tried and rejected because it pins the command +at its last value indefinitely; a pinned command against a meter that has moved on +is precisely a sustained sign disagreement, so under that encoding this watchdog +WOULD false-trip on a plain load step. If anyone ever revisits that freeze, this +module's no-false-positive argument is downstream of the answer. + +------------------------------------------------------------------------------ +WHAT IT DOES NOT CATCH + +An inverter that ACTS opposite to what it was told. Tell it to discharge 2000 W +and have it charge instead, and the meter moves FURTHER the way it was already +going - so the command and the meter agree, all the way into the rail. This +watchdog compares the loop's output direction against the input state it was +regulating; it is blind to the plant lying about which way it went. That is the +saturation freeze's question (|command - the battery's actual power| > 500 W, +NOTES.md §4.7 row 4), and the two checks are complementary rather than +overlapping. Neither substitutes for the other. + +Nor does it see a sign inversion introduced DOWNSTREAM of this add-on - in the +firmware, on the wire, or in a future driver. Our view stays self-consistent and +this stays silent. That is a wire-versus-intent problem and it belongs to the +witness, which is the only vantage point that sees both. + ------------------------------------------------------------------------------ STATE, TRIP AND LATCH @@ -123,6 +181,15 @@ hand the actuator straight back to the loop that was driving the wrong way - three minutes later, forever. `tripped` therefore stays true until an operator `reset()`s it by starting control again, which is a deliberate human act after looking at why. + +⚠️ AND THE LATCH HAS TO OUTLIVE THE PROCESS. Held only in memory it is not a +latch at all - a container restart clears it and the add-on comes back armed, +into the same fault, unattended. Worse than merely losing state: "restart the +add-on" is the first thing an operator confronted with a FAILSAFE banner and a +0 W inverter will try, and under a supervisor that restarts a crashed add-on it +is a loop of three-minute wrong-direction bursts. `tripped` and `reason` are +therefore constructor arguments, restored from `Store` in `Controller.__init__` +and written back the moment the trip happens. See store.py. """ from dataclasses import dataclass @@ -137,6 +204,23 @@ class SignWatchdogConfig: hold_s: float = 180.0 +def disagrees(commanded_w, grid_w, following: bool, + cfg: SignWatchdogConfig) -> bool: + """The predicate, and the ONLY place it is written down. + + ⚠️ Public because `sim/replay_sign.py` needs it too - to describe episodes, to + measure agreement, to sweep the clock-skew guard - and it had reimplemented it + inline. Two copies of a safety predicate is one copy plus a future + disagreement about what the check means, and the copy that drifts is always + the one in the harness that grades it. + """ + return (following + and commanded_w is not None and grid_w is not None + and commanded_w * grid_w < 0 + and abs(commanded_w) > cfg.min_w + and abs(grid_w) > cfg.min_w) + + class SignWatchdog: """One instance per actuator, evaluated once per control cycle. @@ -147,11 +231,16 @@ class SignWatchdog: arbiter applies to the actuator, applied to the detector's own state. """ - def __init__(self, cfg: SignWatchdogConfig | None = None): + def __init__(self, cfg: SignWatchdogConfig | None = None, *, + tripped: bool = False, reason: str = ""): self.cfg = cfg or SignWatchdogConfig() self.disagree_since: datetime | None = None - self.tripped = False - self.reason = "" + # ⚠️ RESTORED FROM THE STORE, NOT DEFAULTED. A latch that lives only in + # memory is not a latch: a container restart clears it, and the add-on + # comes back armed, into the same fault, with no operator involved. See + # store.py's note on `sign_tripped` for the measured behaviour. + self.tripped = bool(tripped) + self.reason = reason if self.tripped else "" # -- the whole check, and it is a pure function of its arguments ---------- def evaluate(self, commanded_w, grid_w, following: bool, @@ -178,15 +267,7 @@ class SignWatchdog: return True cfg = self.cfg - disagree = ( - following - and commanded_w is not None and grid_w is not None - and commanded_w * grid_w < 0 - and abs(commanded_w) > cfg.min_w - and abs(grid_w) > cfg.min_w - ) - - if not disagree: + if not disagrees(commanded_w, grid_w, following, cfg): self.disagree_since = None return False @@ -206,13 +287,18 @@ class SignWatchdog: ) return True - @property - def held_s(self) -> float: - """How long the current disagreement has been standing, for the UI.""" + def held_s(self, now: datetime) -> float: + """How long the current disagreement has been standing, for the UI. + + ⚠️ Takes the clock rather than reading one. It was a property calling + `datetime.now()`, which quietly broke the property this module's header + advertises - every other observable here is a pure function of what it + was handed - and an unclocked function is one nothing can test, which is + exactly what happened to it. + """ if self.disagree_since is None: return 0.0 - return (datetime.now(self.disagree_since.tzinfo) - - self.disagree_since).total_seconds() + return (now - self.disagree_since).total_seconds() def reset(self) -> None: self.disagree_since = None diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index 5ff30a0..d9fc3d1 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -1,5 +1,5 @@ name: GoodWe RS485 Controller -version: "0.5.0" +version: "0.5.1" slug: goodwe_controller description: >- Drives a GoodWe ES/BP battery inverter over RS485 by emulating its smart @@ -101,7 +101,7 @@ options: # Both defaults are measured, not chosen. On the captured healthy day # (2026-08-23) the longest legitimate opposed run - a load step the loop has # not caught up with - is 30 s, and sign agreement over the gated samples is - # 92.9 %. On the captured fault day (2026-08-20) the four episodes run 0.0-11 % + # 93.4 %. On the captured fault day (2026-08-20) the clusters run 0.0-10.5 % # agreement with opposed runs up to 1752 s. 300 W / 180 s sits in the middle of # a very wide gap: at 60 s the healthy day still trips nothing, at 300 s the # fault day still trips 13 times. There is no reason to move either number diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index a55b944..3f92c87 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -150,6 +150,83 @@ p = validate_startup_config({**base, "max_w": 2000, "sign_disagree_w": 1999}, -5000, 5000) check("one watt under the ceiling is reachable, so accepted", p == []) +print() +print("the arming gate, derived from the REAL arbiter") +# ⚠️ THIS IS THE DECISION THE WHOLE TICKET TURNS ON, and until now it was +# asserted in three comment blocks and verified by nothing: every other case in +# this file passes `following` in as a literal. What has to hold is that the +# expression in main.py - loop won AND the emitted value points the way the loop +# asked AND is no larger - is never True for a resolution the loop did not +# actually get. So run real claims through the real resolve() and derive it. +from app.arbiter import ( # noqa: E402 + P_LOOP, P_MAINT_SHAPE, P_MAINTENANCE, P_PEAK, P_SAFETY, Claim, resolve, +) + + +def following_from(claims, asked): + """Exactly main.py's expression. Keep the two in step.""" + r = resolve(claims) + return r, (r.winner == "loop" and asked is not None + and r.target_w * asked > 0 + and abs(r.target_w) <= abs(asked)) + + +MAXW = Claim.limit("supervised", P_SAFETY, -2000, 2000, "max_w") +DEV = Claim.limit("device", P_SAFETY, -5000, 5000, "rating") + +# name, claims, what the loop asked, expected `following` +TABLE = [ + ("the loop alone, unopposed", + [Claim.set("loop", P_LOOP, 900)], 900, True), + ("the loop bound by max_w - reduced, not redirected", + [Claim.set("loop", P_LOOP, 4000), MAXW], 4000, True), + ("charging, bound by max_w on the other side", + [Claim.set("loop", P_LOOP, -4000), MAXW], -4000, True), + ("maintenance owns the setpoint outright", + [Claim.set("loop", P_LOOP, 900), + Claim.set("maintenance", P_MAINTENANCE, -2500, "charge"), MAXW], 900, False), + ("maintenance charge-only clamps the loop to 0 W", + [Claim.set("loop", P_LOOP, 900), + Claim.limit("maintenance", P_MAINT_SHAPE, hi=0.0, reason="charge-only"), + MAXW], 900, False), + # ⚠️ THE CASE `winner == \"loop\"` ALONE WOULD HAVE GOT WRONG. 1500 W, not + # the default 2500 W: at max_w 2000 a -2500 hi against a -2000 lo is an + # arbiter contradiction, not a charge - see main.py. + ("cheap-window charge REDIRECTS the loop while it still 'wins'", + [Claim.set("loop", P_LOOP, 500), + Claim.limit("maintenance", P_MAINT_SHAPE, hi=-1500, reason="cheap-window"), + MAXW], 500, False), + ("...and the contradiction at the shipped defaults is not following either", + [Claim.set("loop", P_LOOP, 500), + Claim.limit("maintenance", P_MAINT_SHAPE, hi=-2500, reason="cheap-window"), + MAXW], 500, False), + ("a lo limit RAISES the magnitude without flipping the sign", + [Claim.set("loop", P_LOOP, 200), + Claim.limit("floor", P_SAFETY, lo=800), MAXW], 200, False), + ("a safety stop wins outright", + [Claim.set("loop", P_LOOP, 900), + Claim.set("safety", P_SAFETY, 0.0, "stopped"), MAXW], 900, False), + ("no claims at all - the arbiter's own failsafe", + [MAXW, DEV], None, False), + ("the loop asked for zero", + [Claim.set("loop", P_LOOP, 0.0), MAXW], 0.0, False), + # The forward hazard, asserted as it stands TODAY rather than as it should + # be. When peak shaving lands, this expectation is what has to change. + ("peak shaving at P_PEAK disarms the gate (see main.py's forward hazard)", + [Claim.set("loop", P_LOOP, 900), + Claim.set("peak", P_PEAK, 1800, "shaving"), MAXW], 900, False), +] +for name, claims, asked, expect in TABLE: + r, got = following_from(claims, asked) + check(f"{name} -> following={expect}", got is expect) + if got is not expect: + print(f" resolution was {r.explain()!r}") + +# And the gate is never True when the loop did not win, whatever else is going on. +check("no configuration above arms the gate without the loop winning", + all(not following_from(c, a)[1] + for _, c, a, _ in TABLE if following_from(c, a)[0].winner != "loop")) + print() print("end to end: the trip is COMMAND ZERO, not alarm-only") # ⚠️ THE FAULT INJECTED HERE IS A NEGATIVE GAIN, and that is not a contrivance - @@ -237,10 +314,106 @@ check("the status page says so in words", any("SIGN-DISAGREEMENT FAILSAFE" in c["text"] for c in ctrl.checks())) check("the banner leads with it", "FAILSAFE" in ctrl.status_payload()["banner"]) -check("MQTT reports the failsafe state", ctrl.sign_wd.tripped) check("it stays at 0 W on later cycles too", hass.writes[-1] == 0.0 and hass.writes.count(0.0) >= 1) +# ⚠️ THIS USED TO BE `check("MQTT reports the failsafe state", ctrl.sign_wd.tripped)` +# which restates the assertion three lines above and never touches the payload. +# The `status: "failsafe"` value was shipped untested. Capture what publish() +# actually hands the broker. +published = {} +ctrl.mqtt = type("P", (), {"enabled": True, + "publish": lambda _s, v: published.update(v), + "close": lambda _s: None})() +ctrl.publish() +check("MQTT's controller-status sensor reports 'failsafe'", + published.get("status") == "failsafe") +ctrl.sign_wd.reset() +ctrl.publish() +check("...and goes back to 'running' once cleared", + published.get("status") == "running") + + +print() +print("the latch survives a restart - it is the only thing between a tripped") +print("controller and the first thing an operator will try") +# ⚠️ THE BLOCKER THIS FILE MISSED FIRST TIME ROUND. `auto` is persisted and +# `tripped` was not, so reconstructing a Controller against the same Store - +# which is exactly what a container restart does - came back armed, into the +# fault, with no operator involved: tripped=False, banner NOT READY, and it +# resumed commanding -1000 then -2000 W before re-tripping 180 s later. + + +class _MemStore: + """A Store that keeps its dict across "restarts", like the real file does.""" + + def __init__(self): + self.data = {"phase": "idle", "sign_tripped": False, "sign_reason": ""} + + def set(self, k, v): + self.data[k] = v + + def get_time(self, *a): + return None + + +async def _restart(): + shared = _MemStore() + hass1 = _Hass() + opts = {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": 2000, "gain": -0.6, + "sign_disagree_w": 300, "sign_disagree_s": 1, "auto_start": True} + a = Controller(opts, hass1, shared, _P(), stop=asyncio.Event()) + a.auto = True + a.dev_min, a.dev_max = -5000.0, 5000.0 + for _ in range(12): + await a.read_inputs() + await a.cycle() + await asyncio.sleep(0.25) + + # The restart. Same options, same store, brand new Controller and Hass. + hass2 = _Hass() + b = Controller(opts, hass2, shared, _P(), stop=asyncio.Event()) + b.auto = True + b.dev_min, b.dev_max = -5000.0, 5000.0 + for _ in range(6): + await b.read_inputs() + await b.cycle() + await asyncio.sleep(0.05) + return a, b, shared, hass2 + + +pre, post, store, hass2 = asyncio.run(_restart()) +check("it tripped before the restart", pre.sign_wd.tripped) +check("the trip reached the store", store.data["sign_tripped"] is True) +check("...with the reason beside it, so the operator is told why", + "SIGN_DISAGREEMENT" in store.data["sign_reason"]) +check("the new Controller comes back TRIPPED", post.sign_wd.tripped) +check("...and says why, restored rather than re-detected", + "SIGN_DISAGREEMENT" in post.sign_wd.reason) +check("...commands 0 W from its very first cycle", post.target == 0.0) +check("...and never commanded anything else after the restart", + hass2.writes != [] and set(hass2.writes) == {0.0}) +check("the banner still leads with the failsafe", + "FAILSAFE" in post.status_payload()["banner"]) +# ⚠️ TWO PRESSES, and the first one must NOT clear it. `auto` is True while +# tripped, so the first press stops control; only the transition back to True +# clears. That is the documented behaviour and it is worth an assertion, because +# a one-press clear would be reachable by an operator who never read the banner. +asyncio.run(post.handle_action("auto_toggle")) # -> stopped +check("pressing stop does not clear the failsafe", + post.sign_wd.tripped and store.data["sign_tripped"] is True) +asyncio.run(post.handle_action("auto_toggle")) # -> started +check("pressing start DOES clear it, and clears the store with it", + not post.sign_wd.tripped and store.data["sign_tripped"] is False + and store.data["sign_reason"] == "") +# One more restart from the cleared store, to prove the clear persisted too. +fresh = Controller({"setpoint_entity": "number.sp", "meter_source": "off"}, + _Hass(), store, _P(), stop=asyncio.Event()) +check("...so the next restart comes back armed and untripped, as it should", + not fresh.sign_wd.tripped) + print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") -- 2.54.0 From e40eb0768d9bfd24aa3de01bbf8b4f23190b95a4 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 22:59:46 +0200 Subject: [PATCH 2/2] SAFETY-02: the P_PEAK assertion was pinning the test's copy of the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding 2, and it is the same duplication `disagrees()` just removed, so it gets the same fix rather than a follow-up card. `following_from()` in test_watchdog.py was a TRANSCRIPTION of the gate in main.py, carrying "keep the two in step" as an instruction - which is exactly the arrangement that does not keep two things in step. Measured: editing the shipped gate alone to `winner in ("loop", "peak")` left every suite green while the watchdog went blind for the duration of every peak event. The row whose whole job is to force whoever lands peak shaving to come and change an expectation did not do that. The expression is now `watchdog.loop_is_following()`, called by main.py and by the test. One definition, two callers, same reasoning as `disagrees()`: the copy that drifts is always the one in the harness doing the grading. ⚠️ AND EXTRACTING IT WAS NOT SUFFICIENT - the drift proof still came out green after the extraction, which is why it was worth running rather than assuming. The realistic P_PEAK row has peak shaving asking for MORE than the loop, so the magnitude term rejects it whatever `winner` says and masks the term under test. A second row with peak asking for LESS makes `winner` the only deciding term. With that row, widening the shipped gate fails two checks; without it, none. Also enforcing the write ordering that was documented and not asserted: swapping the two `store.set` calls passed everything. A store that fails the second write now proves the property the ordering exists for - a crash mid-trip leaves a reason with no latch, never a latch with no explanation. Swapping them now turns that check red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/CHANGELOG.md | 27 ++++++++-- goodwe_controller/app/main.py | 48 ++++------------- goodwe_controller/app/watchdog.py | 48 +++++++++++++++++ goodwe_controller/test_watchdog.py | 83 +++++++++++++++++++++++++++--- 4 files changed, 157 insertions(+), 49 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 628c811..dfba0ff 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -29,11 +29,23 @@ Also in this release, all of it review follow-up rather than new behaviour: - **The MQTT `status: "failsafe"` value is tested.** The check that claimed to cover it restated the assertion three lines above it and never touched the payload. -- **A forward hazard written down where it will be read.** `winner == "loop"` - disarms the watchdog under any higher-priority SET claim. Today only - maintenance and the safety stops emit one and neither follows the meter; a - peak-shaving strategy at `P_PEAK` **does**, and would blind the watchdog for - every peak event. Noted at the gate itself, with what has to change. +- **A forward hazard written down where it will be read, and made to bite.** + `winner == "loop"` disarms the watchdog under any higher-priority SET claim. + Today only maintenance and the safety stops emit one and neither follows the + meter; a peak-shaving strategy at `P_PEAK` **does**, and would blind the + watchdog for every peak event. + + The gate is now `watchdog.loop_is_following()`, called by both `main.py` and + the test, for the same reason `disagrees()` was extracted: the test held a + *transcription* of the shipped expression, so editing the shipped gate alone to + `winner in ("loop", "peak")` left every suite green while the watchdog went + blind. Two copies, and the one that drifts is the one doing the grading. + + ⚠️ Extracting it was not sufficient on its own. The realistic P_PEAK row — + peak shaving asking for *more* than the loop — still could not detect the + widened gate, because the magnitude term rejects it regardless and masks the + winner term. A second row with peak asking for *less* isolates `winner` as the + only deciding term, and that is the one that turns red. - **The worked example described a configuration that does not ship.** At the defaults `maintenance_charge_w` (2500) exceeds `max_w` (2000), so that claim is an arbiter *contradiction* resolving to 0 W, not a −2500 W charge. The example @@ -44,6 +56,11 @@ Also in this release, all of it review follow-up rather than new behaviour: of the predicate it grades. - `held_s` takes the clock instead of reading one, restoring the pure-function property the module advertises. +- **The `sign_reason`-before-`sign_tripped` write ordering is enforced, not just + documented.** Swapping the two lines used to pass every check. A store that + fails the second write now proves the property the ordering exists for: what + survives a crash mid-trip is a reason with no latch, never a latch with no + explanation. - Measured figures corrected throughout: 93.4 % / 4212 of 17280 (was 92.9 % / 4127 of 16349), per-cluster 0.3 / 0.9 / 10.5 / 0.0 %. diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 825b527..a44eb43 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -41,7 +41,7 @@ from .maintenance import IDLE, MaintConfig, Maintenance from .mqtt import MqttPublisher from .p1 import P1Ingest, build_source, is_enabled from .validate import ConfigRefused, refuse, validate_startup_config -from .watchdog import SignWatchdog, SignWatchdogConfig +from .watchdog import SignWatchdog, SignWatchdogConfig, loop_is_following from . import web OPTIONS_PATH = "/data/options.json" @@ -378,43 +378,17 @@ class Controller: # --- SAFETY-02's arming gate ------------------------------------------ # Was the number about to go on the wire chosen by the grid-following # loop, or by a strategy that commands against the meter on purpose? - # Only the former is expected to agree in sign with P1. See gate 1 in - # watchdog.py - without this, every maintenance grid charge trips the - # failsafe within three minutes of starting. + # Only the former is expected to agree in sign with P1 - without this, + # every maintenance grid charge trips the failsafe within three minutes. # - # ⚠️ "THE LOOP WON" IS NOT ENOUGH, and the difference is a whole class of - # false trip. A LIMIT claim can survive the loop winning and still - # replace its number: maintenance's cheap-window shaping is - # `hi = -maintenance_charge_w`, so with `maintenance_charge_w: 1500` the - # loop can ask for +500 W of discharge, win the arbitration, and have the - # emitted value come out at -1500 W of grid charge. Winner says "loop"; - # the wire carries maintenance's intent. So the test is on the NUMBER, - # not on the name: the loop is following only while the emitted value - # points the way the loop asked and is no larger than it asked - i.e. - # limits reduced it, they did not redirect it. Zero on either side falls - # out as not-following, which costs nothing: a 0 W command cannot be a - # wrong-direction command. The `abs(...) <= abs(asked)` half is - # load-bearing on its own: a `lo` limit can raise the magnitude without - # flipping the sign, and that is not the loop following either. - # - # (1500 W because at the SHIPPED defaults - maintenance_charge_w 2500 vs - # max_w 2000 - that claim is `lo=-2000 > hi=-2500`, which the arbiter - # calls a contradiction and resolves to 0 W. Filed separately; the - # redirect hazard here is real at any charge rate inside max_w.) - # - # ⚠️ FORWARD HAZARD, AND IT IS NOT HYPOTHETICAL - the claim block above - # already anticipates it. `winner == "loop"` disarms this watchdog under - # ANY higher-priority SET claim. Today only maintenance and the safety - # stops emit one, and neither is grid-following, so the gate costs - # nothing. A peak-shaving strategy claiming SET at P_PEAK **is** - # grid-following in sign, and the moment it lands this watchdog goes - # blind for the duration of every peak event. Whoever adds it must widen - # the gate to "the winning strategy follows the meter" rather than - # "the winner is the loop", not just add another name to a comparison. - asked = self.loop_w - self.following = (resolution.winner == "loop" and asked is not None - and resolution.target_w * asked > 0 - and abs(resolution.target_w) <= abs(asked)) + # ⚠️ THE EXPRESSION LIVES IN watchdog.py, NOT HERE, and that is not + # tidiness. It was inline, test_watchdog.py held a transcription of it, + # and the P_PEAK case meant to force whoever lands peak shaving to come + # and change an expectation did not: editing this line alone to + # `winner in ("loop", "peak")` left every suite green while the watchdog + # went blind for every peak event. Read loop_is_following() before + # touching the gate - the forward hazard and the reasoning are there. + self.following = loop_is_following(resolution, self.loop_w) self.target = resolution.target_w self.reason = resolution.explain() await self.write_setpoint(self.target) diff --git a/goodwe_controller/app/watchdog.py b/goodwe_controller/app/watchdog.py index f0162f4..afaf2a4 100644 --- a/goodwe_controller/app/watchdog.py +++ b/goodwe_controller/app/watchdog.py @@ -221,6 +221,54 @@ def disagrees(commanded_w, grid_w, following: bool, and abs(grid_w) > cfg.min_w) +def loop_is_following(resolution, loop_asked) -> bool: + """Did the grid-following loop actually choose the number going on the wire? + + Gate 1 of the predicate above, and it lives here rather than inline in + `cycle()` for the reason `disagrees()` does: the test suite has to be able to + assert on THE SHIPPED EXPRESSION, not on a transcription of it that drifts + apart from it in silence. + + ⚠️ IT WAS A TRANSCRIPTION, AND THAT WAS MEASURED, NOT FEARED. The P_PEAK case + in test_watchdog.py exists to make whoever lands peak shaving come here and + change an expectation. With the expression copied into the test it did not do + that: editing the shipped gate alone to `winner in ("loop", "peak")` left every + suite green while the watchdog went blind for the whole of every peak event. + One definition, two callers, and that test now bites. + + `resolution` the arbiter's Resolution. Duck-typed on `.winner` and + `.target_w` so this module still imports nothing from arbiter. + `loop_asked` what the grid-following loop claimed this cycle, or None if it + did not claim at all. + + "The loop won" is NOT the question, and the difference is a whole class of + false trip. A LIMIT claim can survive the loop winning and still replace its + number: maintenance's cheap-window shaping is `hi = -maintenance_charge_w`, so + at 1500 W the loop can ask for +500 W of discharge, win the arbitration, and + have the emitted value come out at -1500 W of grid charge. Winner says "loop"; + the wire carries maintenance's intent. + + So the test is on the NUMBER: the loop is following only while the emitted + value points the way the loop asked AND is no larger than it asked - limits + reduced it, they did not redirect it. The magnitude half is load-bearing on + its own, because a `lo` limit can raise magnitude without flipping sign, and + that is not the loop following either. Zero on either side falls out as + not-following, which costs nothing: a 0 W command cannot be a + wrong-direction command. + + ⚠️ FORWARD HAZARD, and the claim block in cycle() already anticipates it. + `winner == "loop"` disarms the watchdog under ANY higher-priority SET claim. + Today only maintenance and the safety stops emit one and neither follows the + meter, so the gate costs nothing. A peak-shaving strategy claiming SET at + P_PEAK **is** grid-following in sign, and the moment it lands this goes blind + for the duration of every peak event. Widening it means asking "does the + winning strategy follow the meter", not adding another name to a comparison. + """ + return (resolution.winner == "loop" and loop_asked is not None + and resolution.target_w * loop_asked > 0 + and abs(resolution.target_w) <= abs(loop_asked)) + + class SignWatchdog: """One instance per actuator, evaluated once per control cycle. diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index 3f92c87..5cb701a 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -13,7 +13,9 @@ table-driven half: boundaries, timers, the latch, and the arming gate. import sys from datetime import datetime, timedelta, timezone -from app.watchdog import SignWatchdog, SignWatchdogConfig +from app.watchdog import ( + SignWatchdog, SignWatchdogConfig, loop_is_following, +) fails = [] T0 = datetime(2026, 8, 20, 15, 0, 0, tzinfo=timezone.utc) @@ -164,11 +166,18 @@ from app.arbiter import ( # noqa: E402 def following_from(claims, asked): - """Exactly main.py's expression. Keep the two in step.""" + """THE SHIPPED gate, not a copy of it. + + ⚠️ This used to transcribe main.py's expression with a comment saying + "keep the two in step", which is exactly the arrangement that does not keep + two things in step. Measured: editing the shipped gate alone to + `winner in ("loop", "peak")` left this suite - including the P_PEAK row below, + whose entire job is to catch that edit - completely green, while the watchdog + went blind for the duration of every peak event. Import it or the P_PEAK + assertion is decoration. + """ r = resolve(claims) - return r, (r.winner == "loop" and asked is not None - and r.target_w * asked > 0 - and abs(r.target_w) <= abs(asked)) + return r, loop_is_following(r, asked) MAXW = Claim.limit("supervised", P_SAFETY, -2000, 2000, "max_w") @@ -211,10 +220,21 @@ TABLE = [ ("the loop asked for zero", [Claim.set("loop", P_LOOP, 0.0), MAXW], 0.0, False), # The forward hazard, asserted as it stands TODAY rather than as it should - # be. When peak shaving lands, this expectation is what has to change. - ("peak shaving at P_PEAK disarms the gate (see main.py's forward hazard)", + # be. When peak shaving lands, these expectations are what have to change. + ("peak shaving at P_PEAK disarms the gate (loop_is_following's forward hazard)", [Claim.set("loop", P_LOOP, 900), Claim.set("peak", P_PEAK, 1800, "shaving"), MAXW], 900, False), + # ⚠️ AND THE ROW ABOVE CANNOT DETECT A WIDENED GATE ON ITS OWN, which was + # found by trying it: with the shipped gate edited to + # `winner in ("loop", "peak")` that case STILL comes out False, because peak + # shaving asked for MORE than the loop and the magnitude term rejects it + # anyway. The winner term is masked. This row isolates it - a peak claim + # smaller than the loop's ask and pointing the same way, so `winner` is the + # only term that can decide - and it is what actually turns red if the gate + # is widened without anyone revisiting the hazard. + ("...and with peak BELOW the loop's ask, only the winner term separates them", + [Claim.set("loop", P_LOOP, 1800), + Claim.set("peak", P_PEAK, 900, "shaving"), MAXW], 1800, False), ] for name, claims, asked, expect in TABLE: r, got = following_from(claims, asked) @@ -414,6 +434,55 @@ fresh = Controller({"setpoint_entity": "number.sp", "meter_source": "off"}, check("...so the next restart comes back armed and untripped, as it should", not fresh.sign_wd.tripped) +# ⚠️ THE WRITE ORDERING IS AN INVARIANT, SO IT GETS AN ASSERTION. main.py writes +# `sign_reason` and THEN `sign_tripped`, each one its own `save()`, so that a +# process that dies between the two leaves a reason nothing reads - harmless - +# rather than a latch with no explanation for the operator who has to decide +# whether to clear it. Documented reasoning is not enforcement: swapping those two +# lines used to pass every check in this file. +# +# The death is modelled as the second write raising. In the field it is a power +# cut or a container kill (Store.save() swallows OSError rather than raising), so +# what is asserted here is the PROPERTY - what survives on disk - not the +# mechanism. + + +class _CrashOnFlag(_MemStore): + def set(self, k, v): + if k == "sign_tripped" and v: + raise RuntimeError("power cut, mid-trip") + super().set(k, v) + + +async def _crash_mid_write(): + store = _CrashOnFlag() + hass = _Hass() + opts = {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": 2000, "gain": -0.6, + "sign_disagree_w": 300, "sign_disagree_s": 1, "auto_start": True} + c = Controller(opts, hass, store, _P(), stop=asyncio.Event()) + c.auto = True + c.dev_min, c.dev_max = -5000.0, 5000.0 + died = False + for _ in range(12): + try: + await c.read_inputs() + await c.cycle() + except RuntimeError: + died = True + break + await asyncio.sleep(0.25) + return store, died + + +crashed_store, died = asyncio.run(_crash_mid_write()) +check("the second write is the one that sets the flag", died) +check("a crash between the two writes leaves the REASON on disk", + "SIGN_DISAGREEMENT" in crashed_store.data.get("sign_reason", "")) +check("...and never a latch with nothing to explain it", + not crashed_store.data.get("sign_tripped")) + print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") -- 2.54.0