From 4e66583a52cbb855b1466bb82ae22e31b0543533 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 15:52:09 +0200 Subject: [PATCH] SAFETY-03 review fixes: both clamp stages always run, and two checks that could not fail 1. BEHAVIOURAL, and a regression this PR introduced. clamp_to_rating() returned early from the max_w stage, so the device bound was applied only when the rating bound had NOT fired - the bigger the overshoot, the less the guard did. At max_w 5000 against a +-4000 device, +9999 emitted +5000 W, outside the device range, while +4500 was correctly held to +4000. It undid the pre-PR guarantee "never send a value the device will reject outright" and undercut this module's own case for max_w-not-min(), which rests on that stage catching a validation that did not run. Both stages now always run; the result names both bounds when both bite (max_w+device). Asserted over a 492-point sweep, because the defect was invisible to any single-value check sitting inside one stage. 2. The hysteresis check issued range(CLEAR_CYCLES - 1) clean cycles, which at CLEAR_CYCLES = 1 issues zero, passes, and prints "0 clean cycles do NOT clear it" - green for the chatter setting the constant exists to prevent. Now: ONE clean cycle must not clear the alarm, a literal that does not move with the constant, and the de-assert count is measured by looping until it clears. 3. The re-assert check stimulated at REASSERT_S + 9, green for any value including 3600 - the hour of silence the module promises not to produce. Now stated in wall-clock terms: a loop pinned across the 900 s capacity-tariff quarter must speak at least 5 times and on no more than 20% of cycles. Pinned in both directions. Red confirmed for each: the restored early return (3 checks), CLEAR_CYCLES = 1 (1), REASSERT_S = 3600 (1) and REASSERT_S = 0 (3). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC --- goodwe_controller/CHANGELOG.md | 28 ++++++++++ goodwe_controller/app/clamp.py | 29 +++++++--- goodwe_controller/test_clamp.py | 96 ++++++++++++++++++++++++++++----- 3 files changed, 132 insertions(+), 21 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 2d307f2..bb736d6 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -55,6 +55,34 @@ out-of-range value. Measured: because every comparison against `nan` is False, a naive `max(-limit, min(limit, nan))` keeps its *first* argument and turns a `nan` into a command **at the full rating**, then reports it as a clamp engagement. +**Review fixes (PR #12).** Three, and the first was a behavioural regression +this PR introduced: `clamp_to_rating()` **returned early** from the `max_w` +stage, so the device bound - the "applied second, defence in depth" half - was +reached only when the rating bound had NOT fired. The bigger the overshoot, the +less the guard did. At `max_w` 5000 against a +-4000 device, +9999 emitted +**+5000 W, outside the device range**, while a mere +4500 was correctly held to ++4000. That undid the pre-PR guarantee "never send a value the device will +reject outright" and undercut this module's own case for clamping to `max_w` +rather than a runtime `min()`, which rests entirely on that stage catching a +validation that did not run. Both stages now always run, the result names both +bounds when both bite (`max_w+device`), and the property is asserted over a +492-point sweep rather than on witness values - the defect was invisible to any +single-value check that happened to sit inside one stage. No reachable +production path was found for it (two validation gates and the arbiter bound sit +in front), but latent is not harmless. + +Two harness defects, both of the kind where a check cannot fail for the value it +forbids: the hysteresis check issued `range(CLEAR_CYCLES - 1)` clean cycles, +which at `CLEAR_CYCLES = 1` issues **zero** and passes while printing +"0 clean cycles do NOT clear it" - green for the chatter setting the constant +exists to prevent. And the re-assert check stimulated at `REASSERT_S + 9`, green +for **any** value including 3600, i.e. the hour of silence the module promises +not to produce. Both are now written in wall-clock terms that do not move when +the constant does: **one** clean cycle must not clear the alarm, and a loop +pinned across the 900 s capacity-tariff quarter must speak at least 5 times and +on no more than 20 % of cycles. `CLEAR_CYCLES = 1`, `REASSERT_S = 3600` and +`REASSERT_S = 0` each go red. + **`test_clamp.py`** (new, assert-based, no framework) covers the clamp arithmetic and its boundaries - exactly +-`max_w` is *not* an engagement - the alarm's edge/re-assert/hysteresis cadence, the 10.3 runaway replayed through the diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index 5a9e2af..7b9acba 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -161,9 +161,9 @@ class ClampResult: def describe(self) -> str: return (f"{REASON_CODE}: the computed setpoint {self.requested_w:+.0f} W " - f"exceeds the {self.limit_w:.0f} W rating and was clamped to " - f"{self.applied_w:+.0f} W by the {self.bound} bound - something " - f"upstream is winding") + f"was clamped to {self.applied_w:+.0f} W by the {self.bound} " + f"bound (rating {self.limit_w:.0f} W) - something upstream is " + f"winding") def is_commandable(value) -> bool: @@ -204,19 +204,32 @@ def clamp_to_rating(requested_w, limit_w, dev_min=None, dev_max=None) -> ClampRe requested = float(requested_w) limit = abs(float(limit_w)) + # ⚠️ BOTH STAGES ALWAYS RUN. THIS USED TO `return` FROM THE FIRST ONE, AND + # THAT WAS A REGRESSION AGAINST THE CODE IT REPLACED. `write_setpoint()` + # previously clamped to the device range unconditionally, under the comment + # "never send a value the device will reject outright"; short-circuiting here + # meant the device bound was applied only when the rating bound had NOT + # fired, i.e. THE BIGGER THE OVERSHOOT THE LESS THE GUARD DID. Measured on + # the shipped function at max_w 5000 against a device of +-4000: + # + # +9999 -> +5000 bound='max_w' <- outside the device range + # +4500 -> +4000 bound='device' + # + # It also undercut this module's own argument for clamping to `max_w` rather + # than a runtime `min()`, which rests entirely on this stage catching the + # case where the startup validation did not run. applied = max(-limit, min(limit, requested)) - if applied != requested: - return ClampResult(requested, applied, limit, True, "max_w") + bound = "max_w" if applied != requested else "" # Second stage. Arithmetically dead after a successful startup validation - # and therefore worth alarming on precisely BECAUSE it fired. if dev_min is not None and dev_max is not None: bounded = max(float(dev_min), min(float(dev_max), applied)) if bounded != applied: - return ClampResult(requested, bounded, limit, True, "device") - applied = bounded + bound = "device" if not bound else "max_w+device" + applied = bounded - return ClampResult(requested, applied, limit, False, "") + return ClampResult(requested, applied, limit, applied != requested, bound) class ClampWatchdog: diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 0b1fb88..b9e3d0c 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -122,6 +122,36 @@ r = clamp_to_rating(3000.0, 5000.0, dev_min=-5000.0, dev_max=5000.0) check("...and is a no-op once the startup validation holds", r.applied_w == 3000.0 and not r.engaged) +# ⚠️ REGRESSION, REVIEW-FOUND. THE FIRST STAGE USED TO `return` AND THE DEVICE +# STAGE WAS THEN NEVER REACHED, so the harder the overshoot the less the guard +# did: at max_w 5000 against a +-4000 device, +9999 emitted +5000 - OUTSIDE THE +# DEVICE RANGE - while a mere +4500 was correctly held to +4000. That inverts +# the guard and it was a regression against the pre-PR `write_setpoint()`, whose +# comment read "never send a value the device will reject outright". +# +# The property is asserted over a sweep, not on the two witness values, because +# the defect was invisible to every single-value check that happened to sit +# inside one stage. +DEV_LO, DEV_HI = -4000.0, 4000.0 +escapes = [(m, clamp_to_rating(m, 5000.0, DEV_LO, DEV_HI).applied_w) + for m in range(-15000, 15001, 61) + for a in [clamp_to_rating(m, 5000.0, DEV_LO, DEV_HI).applied_w] + if not (DEV_LO <= a <= DEV_HI)] +big = clamp_to_rating(9999.0, 5000.0, DEV_LO, DEV_HI) +small = clamp_to_rating(4500.0, 5000.0, DEV_LO, DEV_HI) +print(f" max_w 5000 vs a device of {DEV_LO:+.0f}..{DEV_HI:+.0f}: " + f"+9999 -> {big.applied_w:+.0f} W ({big.bound}), " + f"+4500 -> {small.applied_w:+.0f} W ({small.bound})") +check(f"BOTH stages always run - {big.requested_w:+.0f} W emits " + f"{big.applied_w:+.0f} W, inside the device's {DEV_HI:+.0f} W, and names " + f"both bounds ({big.bound!r})", + big.applied_w == DEV_HI and big.bound == "max_w+device") +check(f"...and it holds over the whole range, so no overshoot escapes the " + f"device ({len(escapes)} escapes in {len(range(-15000, 15001, 61))})", + not escapes) +check("...a big overshoot is not clamped LESS than a small one", + abs(big.applied_w) <= abs(small.applied_w) + 0.0) + print(" non-finite input is loss of input, not an out-of-range value") for bad in (None, float("nan"), float("inf"), float("-inf"), "abc", object()): check(f"is_commandable({bad!r:.30}) is False", is_commandable(bad) is False) @@ -156,22 +186,62 @@ check(f"but every one of them is counted: engagements={wd.engagements}", check(f"and it is one episode, not ten: episodes={wd.episodes}", wd.episodes == 1) -# Re-assert: still engaged, REASSERT_S later. -check("it re-asserts on the slow cadence rather than going quiet", - wd.note(True, "still winding", T0 + timedelta(seconds=REASSERT_S + 9)) is True) +# --- THE RE-ASSERT CADENCE, PINNED IN BOTH DIRECTIONS ------------------------ +# ⚠️ REVIEW-FOUND. This used to read `wd.note(True, ..., T0 + REASSERT_S + 9)`, +# which is green for ANY value of the constant - REASSERT_S = 3600 passed, and an +# hour of silence is precisely what the module promises not to do. A check whose +# stimulus is written in terms of the constant it is grading cannot grade it. +# +# The requirements are stated in wall-clock seconds and derived from what the +# alarm is FOR, so they do not move when the constant does: +# +# floor - a loop pinned on the rail must speak at least MIN_PER_QUARTER times +# inside the 900 s quarter that bills the capacity tariff. That window +# is the same one freeze.py and DOCS.md reason about. +# ceiling - it must not speak on more than MAX_FRACTION of cycles, or it is +# the per-cycle flood the split exists to remove. +QUARTER_S = 900.0 +MIN_PER_QUARTER = 5 +MAX_FRACTION = 0.20 -# Hysteresis: a command dithering across the rail must not chatter the alarm. -before = wd.active -partial = [wd.note(False, "", T0 + timedelta(seconds=100 + i)) - for i in range(CLEAR_CYCLES - 1)] -check(f"{CLEAR_CYCLES - 1} clean cycles do NOT clear it " - f"(active={wd.active}, was {before})", wd.active is True) -wd.note(False, "", T0 + timedelta(seconds=200)) -check(f"the {CLEAR_CYCLES}th does (active={wd.active})", wd.active is False) +pinned = ClampWatchdog() +alarms = sum(1 for i in range(int(QUARTER_S)) + if pinned.note(True, "pinned", T0 + timedelta(seconds=i))) +print(f" pinned on the rail for {QUARTER_S:.0f} s at 1 s cycles: " + f"{pinned.engagements} engagements, {alarms} alarm(s) " + f"({alarms / QUARTER_S:.1%} of cycles)") +check(f"a loop pinned for a whole billing quarter speaks at least " + f"{MIN_PER_QUARTER} times (it spoke {alarms})", + alarms >= MIN_PER_QUARTER) +check(f"...and on no more than {MAX_FRACTION:.0%} of cycles (it was " + f"{alarms / QUARTER_S:.1%})", alarms <= MAX_FRACTION * QUARTER_S) +check("...while every single cycle is still counted", + pinned.engagements == int(QUARTER_S)) + +# --- THE HYSTERESIS, PINNED AGAINST THE CHATTER DIRECTION -------------------- +# ⚠️ REVIEW-FOUND. This used to issue `range(CLEAR_CYCLES - 1)` clean cycles, +# which at CLEAR_CYCLES = 1 issues ZERO of them and passes while printing +# "0 clean cycles do NOT clear it". Green for every value >= 1 - including 1, +# which IS the chatter the constant exists to prevent. The stimulus is a literal +# now: ONE clean cycle must not clear the alarm, whatever the constant says. +wd2 = ClampWatchdog() +wd2.note(True, "winding", T0) +wd2.note(False, "", T0 + timedelta(seconds=1)) +check(f"ONE clean cycle does NOT clear the alarm (active={wd2.active}) - " + f"a command dithering across the rail must not chatter", + wd2.active is True) +survived = 1 +while wd2.active and survived < 50: + wd2.note(False, "", T0 + timedelta(seconds=1 + survived)) + survived += 1 +print(f" it took {survived} consecutive clean cycles to de-assert " + f"(CLEAR_CYCLES = {CLEAR_CYCLES})") +check(f"...and it does de-assert, after {survived} of them, rather than " + f"latching", wd2.active is False and survived == CLEAR_CYCLES) check("...and the engagement count survives the clear, so the evidence does", - wd.engagements == 11) + wd2.engagements == 1) check("a clean cycle on an idle watchdog says nothing", - wd.note(False, "", T0 + timedelta(seconds=300)) is False) + wd2.note(False, "", T0 + timedelta(seconds=300)) is False) # ==============================================================================