Files
goodwe-addon/goodwe_controller/app/watchdog.py
T
glenn schrooyenandClaude Opus 5 d5b1ef19b3
checks / unit (pull_request) Successful in 3m44s
CFG-03: give maintenance option defaults one source of truth
Option defaults for the maintenance cycle were declared in three places that
could disagree, and MAINT-04 proved they already did. config.yaml shipped
`maintenance_charge_w: 2000`; main.py carried TWO `opts.get(..., 2500)`
fallbacks; MaintConfig's dataclass carried a third `2500.0`. So an options dict
with the key ABSENT was ACCEPTED by validate.py's rule 6 - which read 2000 -
and then built by main.py at 2500, against a 2000 W ceiling:

    options dict with maintenance_charge_w ABSENT, max_w=2000
      validate_startup_config  ->  []  (accepted, no problems)
      MaintConfig().charge_w   ->  2500.0
      arbiter.resolve()        ->  0.0 W, contradiction=True

That is the bug rule 6 exists to refuse, passing rule 6. It is unreachable on a
Supervisor install, which always supplies every key from config.yaml, and fully
reachable from sim/inject.py and from the test suite - which is where anyone
debugging maintenance actually works.

MAINT-04 shipped a TRIPWIRE and said so: test_validate.py pinned all three
literals and asserted the consequence through the real arbiter, so the
divergence could not silently widen or be half-closed. Its PASSING CONDITION
WAS THE BUG. Both it and validate.py's comment said to delete each other when
the divergence was closed. It is closed, so both are deleted.

THE DIVERGENCE WAS WIDER THAN ONE KEY. `maintenance_discharge_w` had the
identical three-way split (config.yaml 2000, MaintConfig 2500) and the card
named only the charge side. Closing one and leaving its twin would have
satisfied the numbers and left three sources of truth one line down, which is
the state that produced the bug. Both are closed, and so is the whole family:
all eight `maintenance_*` keys go through the same mechanism.

WHAT THE SINGLE SOURCE IS. config.yaml's `options:` block - it is what the
Supervisor reads and what the operator edits, so it wins by definition. There
is now exactly one in-code copy, `maintenance.MAINT_OPTION_DEFAULTS`, which is
TARIFF-02's `PRICE_OPTION_DEFAULTS` and SAFETY-24's `PEAK_OPTION_DEFAULTS`
pattern applied to the family that named the card - validate.py's rule 8 comment
already called that "CFG-03 read as a rule rather than as a complaint", so this
makes the rule hold where it was first broken rather than inventing a fourth
mechanism for one option family.

A MIRROR RATHER THAN A RUNTIME READ, and the reason is mechanical: the
Dockerfile copies `app/` and `run.sh` into the image and NOT config.yaml. The
running add-on cannot open the file it is configured by, so there is nothing to
read at 03:00 on a client's Pi, and a YAML-subset parser that could fail at
import is a new way for the add-on not to start. What makes the copy safe is
that test_maintenance.py pins every entry by READING the shipped file - drift is
a CI failure, not a comment.

EVERY OTHER SITE NOW DERIVES FROM IT OR HAS CEASED TO EXIST:

  * MaintConfig's eight dataclass defaults are indexed out of the mirror. No
    numeric literal survives in the class that held the third opinion.
  * `MaintConfig.from_options()` is the ONLY place in the add-on that reads a
    `maintenance_*` option. main.py's eight inline `opts.get(key, <literal>)`
    calls are gone: moving the read next to the mirror means the fallback cannot
    be anything but the mirror, because there is no literal in sight of it to
    disagree with. It uses `key in opts` rather than truthiness, so a shipped
    `false` or `0` is honoured instead of being quietly replaced by the default
    - this card's own bug in a different costume.
  * main.py's SECOND fallback site - the cheap-window charge floor at the
    P_MAINT_SHAPE claim - reads `self.maint.cfg.charge_w` instead of re-opening
    the options dict with its own literal. The Controller had already parsed
    this once in __init__; asking the object that owns it cannot drift from it,
    and the rate the shaping asks for can no longer be a different number from
    the rate the state machine drains at on the same install.
  * validate.py's rule 6 and its `maintenance_discharge_w` warning take their
    fallbacks from the mirror.

WHERE THE RANGE LIVES, STATED AND CHECKED. config.yaml's `schema:` block carries
`maintenance_charge_w: int(500,5000)` - a fourth declaration site of a different
kind. It is a BOUND, not a default, so it is not this bug; but leaving the
question open is how a fifth site arrives. The decision recorded in config.yaml
and in maintenance.py is: bounds live in the schema block ONLY, because the
Supervisor is what enforces them and the Supervisor reads only that file, so a
Python copy of `int(500,5000)` would be a place to disagree and nothing else.
test_maintenance.py checks both halves of that: every mirrored key must have a
schema entry - without one the Supervisor drops the option and the add-on
silently runs on the mirror instead of the operator's value - and every shipped
default must lie INSIDE its own range, since a default the schema rejects is a
config that cannot be re-entered once it is edited. What the schema grammar
cannot express is a relation BETWEEN keys (`maintenance_charge_w` against
`max_w`), which is exactly why rule 6 is written in Python and why it does not
belong in the mirror either.

THE REPLACEMENT CHECKS, AND HOW THEY WERE VERIFIED. test_validate.py's new
section exercises the same four consumers the defect ran through, the same way:
the validator ACCEPTS a dict with every maintenance key missing; a REAL
Controller built from that same dict holds 2000 W (through main.py, not through
a regex over its source); `MaintConfig()` answers 2000 W with no dict at all;
and the real arbiter emits `-2000 W, limited by supervised(max_w)` instead of
`0 W, contradiction=True`. test_maintenance.py pins the mirror to config.yaml in
both directions - equal values, and full coverage of every `maintenance_*` key
the file ships, so a ninth key cannot arrive unmirrored - and asserts the
consumers move as a RELATION rather than against numbers, reading MaintConfig's
defaults back out of `dataclasses.fields()`.

Each new check was verified by BREAKING the thing it watches and observing red,
this project's rule:

  1. config.yaml -> 1500, mirror left at 2000:  test_maintenance 1 FAILED,
     test_validate 4 FAILED.
  2. config.yaml AND the mirror -> 1500: green again, and every consumer moved
     together - MaintConfig() 1500 W, the Controller 1500 W, and the arbiter
     `-1500 W, limited by maintenance(cheap-window charge)` where it had said
     `-2000 W, limited by supervised(max_w)`. That is AC4: change the single
     source and watch the consumers follow.
  3. `charge_w: float = 2500.0` retyped into the dataclass: 2 FAILED in each of
     the two files.
  4. `opts.get("maintenance_charge_w", 2500)` restored in main.py: 1 FAILED,
     naming the file and the literal it found.
  5. `maintenance_hold_min` deleted from config.yaml's options block: 3 FAILED.

Full suite 17/17 in goodwe-addon-ci, twice, once under parallel load.

Stale prose that asserted the now-closed divergence as current is corrected in
app/watchdog.py, test_watchdog.py and test_peakguard.py - watchdog.py's example
still uses 1500 W, but because the point of the example is a charge strictly
inside the ceiling, not because 2500 is what ships.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 06:16:54 +02:00

357 lines
19 KiB
Python

"""SAFETY-02 - the sign-disagreement watchdog.
WHAT IT CATCHES, in one sentence: a grid-following loop that is pushing power in
the wrong direction, which every other guard in this system is blind to.
The stale-input watchdog (SAFETY-01) asks "is the meter still talking?". The
saturation freeze asks "can the inverter follow?". The output clamp asks "is the
number too big?". A loop can pass all three and still be charging the battery
out of the grid while the house imports - the meter is fresh, the inverter is
tracking beautifully, and the magnitude is inside the rating. The only thing
that separates that from healthy operation is the SIGN.
⚠️ THIS IS NOT A HYPOTHETICAL. On 2026-08-20 the vendor controller (Illusmart)
did exactly this four times in one day (NOTES.md §4.7). Both captures of that
bus are committed - `sim/scenarios/bus-2026-08-20.log.gz` and the healthy
`bus-2026-08-23.log.gz` in the infra repo - and `sim/replay_sign.py` drives this
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 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 %
⚠️ 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
Three vantage points see three different things. The firmware knows what it
WROTE, the witness Pi knows what was ON THE WIRE and whether the inverter ACKed
it, and the add-on knows what it INTENDED and holds the authoritative meter.
This check is not command-versus-wire; it is command-versus-physics. It needs
the emitted setpoint and the P1 net figure in the same place, and it needs the
authority to command zero when they fight:
- the witness is read-only by charter and cannot command anything, so it could
raise the alarm but never take the trip action the AC requires;
- the firmware has the wire but not the meter. §5.1 makes P1 the only
authoritative grid figure and the board only ever receives its AGE
(`sensor.goodwe_p1_sample_age`), never its value. Putting the check there
would mean shipping the meter to the ESP32 for no other reason, and it would
be C++ per install rather than one generic guard;
- the add-on has both halves already, plus an arbiter that can outrank every
strategy with a P_SAFETY claim.
The cost of standing here is real and worth stating: this watchdog compares what
we MEANT to put on the wire against the meter. If the firmware or the driver
inverts the sign on the way out, the add-on's own view still agrees with itself
and this check stays silent. That gap is a wire-versus-intent problem and it
belongs to the witness, not here.
------------------------------------------------------------------------------
WHAT "DISAGREEMENT" MEANS, PRECISELY
disagree = following
and commanded_w * grid_w < 0
and abs(commanded_w) > min_w
and abs(grid_w) > min_w
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 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 2500 W, AND THE REASON IS NOT COSMETIC. The example has to be
a charge the arbiter actually produces. When this was written the shipped
`maintenance_charge_w` was 2500 against a `max_w` of 2000, so the cheap-window
claim was `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 - a
default maintenance charge that silently never happened. MAINT-04 refused that
pair in validate.py and moved the shipped rate to 2000; CFG-03 gave the
default one source of truth so main.py and MaintConfig could not go on
answering 2500 to a dict that had never said so. The number here stays 1500
because the point of the example is a charge STRICTLY INSIDE the ceiling, not
one that happens to sit exactly on it.
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 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.
4. The 180 s continuity requirement. This is what excludes the legitimate
transient. A load step reverses the meter's sign instantly while the command
is still where the last cycle left it; slew limiting then needs a few cycles
to cross over. Measured on the healthy day, the LONGEST such crossing is 30 s
- and that is with a vendor controller far cruder than ours. 180 s is six
times that. It is not a fitted threshold: at 60 s the healthy day still
produces zero trips, and at 300 s the fault day still produces 13.
⚠️ A BATTERY THAT REFUSES A COMMAND CANNOT TRIP THIS, and that is structural
rather than lucky. At SoC 10 % in Standby, a 1350 W discharge command into a
1350 W import is DECLINED: the battery does nothing, so the meter stays at
+1350 W and the command stays at +1350 W. Same sign. Agreement. A refusal leaves
the meter where it was; only the inverter ACTING moves it, and acting in the
commanded direction moves it toward zero, not through it. The failure mode this
check keys on is the opposite of a refusal - it is the inverter obediently doing
a wrong thing.
⚠️ A FROZEN METER cannot trip it either, for the same reason read the other way:
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
One timestamp, not a sample counter. The cycle rate is not fixed - a meter
arrival, or `cycle_floor_s` elapsing, whichever comes first - so N samples is a
different wall-clock window on every path, and the AC is written in minutes.
The latch is not optional and not a nicety. Commanding 0 W removes the command
side of the comparison (|0| is not > 300), so the disagreement disappears the
instant the trip works. Un-latched, this watchdog would trip, clear itself, and
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
from datetime import datetime
@dataclass(frozen=True)
class SignWatchdogConfig:
"""Defaults, not constants - config.yaml carries both (SAFETY-02 AC 5)."""
min_w: float = 300.0
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.
⚠️ NOT SHARED ACROSS CADENCES. `disagree_since` is a wall-clock start time,
so driving one instance from two loops at two rates would have each one
resetting the other's window. If a fast on-device path ever wants this
check, it gets its own instance - the same exactly-one-writer discipline the
arbiter applies to the actuator, applied to the detector's own state.
"""
def __init__(self, cfg: SignWatchdogConfig | None = None, *,
tripped: bool = False, reason: str = ""):
self.cfg = cfg or SignWatchdogConfig()
self.disagree_since: datetime | None = None
# ⚠️ 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,
now: datetime) -> bool:
"""Advance the detector one cycle. Returns True while tripped.
`commanded_w` the EMITTED setpoint - what is standing on the wire now,
after deadband, slew, quantisation and the arbiter. Not
the optimizer's raw wish: the wire carries the emitted
value, so anything else can disagree with the meter for a
reason that is not a fault.
`grid_w` net P1, + = importing. None when the meter is stale or
missing, which CLEARS the timer rather than accumulating
- a frozen figure held against a moving command would
otherwise manufacture a disagreement in about three
minutes, on top of SAFETY-01 having already commanded
zero for the same cause.
`following` did the grid-following loop actually win this cycle? See
gate 1 in the module docstring.
`now` the caller's clock, so this stays testable and replayable.
"""
if self.tripped:
# Latched. Do not re-evaluate: the trip removed the evidence.
return True
cfg = self.cfg
if not disagrees(commanded_w, grid_w, following, cfg):
self.disagree_since = None
return False
if self.disagree_since is None:
self.disagree_since = now
return False
held = (now - self.disagree_since).total_seconds()
if held < cfg.hold_s:
return False
self.tripped = True
self.reason = (
f"SIGN_DISAGREEMENT: commanded {commanded_w:+.0f} W against a meter "
f"reading {grid_w:+.0f} W for {held:.0f} s continuously "
f"(limits: both over {cfg.min_w:.0f} W, held over {cfg.hold_s:.0f} s)"
)
return True
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 (now - self.disagree_since).total_seconds()
def reset(self) -> None:
self.disagree_since = None
self.tripped = False
self.reason = ""