admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

SAFETY-02 review: a latch that a restart clears is not a latch

Browse Source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
This commit is contained in:
glenn schrooyen
2026-08-28 22:33:15 +02:00
co-authored by Claude Opus 5
parent 2513ba1f12
commit cc4e88490b
8 changed files with 458 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/app/main.py
+63 -16
View File
@@ -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
goodwe_controller/app/store.py
+23 -2
View File
@@ -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:
goodwe_controller/app/watchdog.py
+114 -28
View File
@@ -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