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..dfba0ff 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -1,5 +1,69 @@ # 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, 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 + 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. +- **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 %. + ## 0.5.0 **SAFETY-02 — the sign-disagreement watchdog.** The add-on now watches whether @@ -23,7 +87,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..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" @@ -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: @@ -352,25 +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 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. - 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) @@ -600,9 +618,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 +691,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..afaf2a4 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,71 @@ 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) + + +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. @@ -147,11 +279,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 +315,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 +335,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..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) @@ -150,6 +152,101 @@ 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): + """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, loop_is_following(r, 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, 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) + 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 +334,155 @@ 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) + +# ⚠️ 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)}")