diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 4b05c9c..71f1b27 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,41 @@ ## Unreleased +**`check_output_clamp()` could be moved above `write_setpoint()` in `cycle()` +with test_clamp.py, test_freeze.py and test_loop.py all staying GREEN — the +guard's write-path evidence, `self.clamp_last`, had nothing watching it.** +Measured before the fix, on the move itself: all three suites green. The guard +has two evidence sources and only one of them was covered. `self.loop_clamped` +is set from the `Decision` well above the write, so it survives the move — and +on the §10.3 runaway replay it is the source of every engagement the alarm +reports (0 write-path engagements against 2 loop-side ones), which is precisely +what made the move nearly invisible. `self.clamp_last` is assigned by +`write_setpoint()` and by nothing else (`main.py`), is `None` until it has run, +and is what `engaged_write` reads; moved above the write, that consumer goes +quietly false and a DEVICE-bound clamp — `max_w` above the inverter's own +rating, SAFETY-20's install — stops being reported by anything. +test_clamp.py gains part 6, and it grades the CONSEQUENCE rather than the +position: a spy subclassed from the shipped `ClampWatchdog` records what +`self.clamp_last` held each time the guard consulted it, over a slew-limited +ramp where every cycle's command is a different number, and asserts the +evidence is always the write of THAT cycle. Hooking `clamp_wd.note()` rather +than the guard's name is what keeps a rename legitimate; the same assertions +are re-run through a controller whose guard is renamed and whose write lives in +a helper, and were verified against `main.py` itself — renaming the method +throughout and moving both calls into a new `_emit_and_guard()` helper keeps +part 6 green, while swapping the two calls turns it red on a line that names +`clamp_last`. AC 3 needed a stimulus of its own: through the real `cycle()` the +write-path half can never engage, because the arbiter files the device range +and `max_w` as P_SAFETY LIMIT claims and `resolution.target_w` is already +inside both bounds before `write_setpoint()` re-applies them — that duplication +is deliberate defence in depth, and part 6 now measures the 0 engagements +rather than leaving it as a claim in a comment. So the write path is driven +directly at `max_w` 5000 against a ±2500 W device: the wire carries 2500 W, +every loop-side flag stays clean, and the alarm fires on `clamp_last` alone — +with the two calls swapped, the same stimulus clamps the same value and the +alarm is silent. `main.py` gains one comment sentence pointing at the check, +mirroring the one SAFETY-18's block already carries for test_freeze.py part 5. + **`max_w` and the `LIVE_MAX_W` literal beside it were unpinned in test_freeze.py and test_tariff.py, the same gap TEST-06 closed one key over for `meter_freeze_s` (TEST-10).** Measured before the fix: `max_w: 2000 -> 4500` diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 28775cc..1478dce 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -791,6 +791,12 @@ class Controller: # `self.target`, which is assigned further up - see check_output_clamp() # and check_meter_frozen() for each. Two guards, two orderings, adjacent # lines: do not "tidy" them into one rule. + # + # Moving it BELOW the other guards is harmless - nothing between here and + # the end of cycle() writes `self.clamp_last`. Moving it above the write + # is not, and test_clamp.py part 6 fails on exactly that: it grades the + # evidence, not the line number, so a rename or a move into a helper that + # still runs after the write keeps it green. (TEST-11.) await self.check_output_clamp() # --- SAFETY-18, and its boundary is the ASSIGNMENT -------------------- diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index b8c9958..c0dfc99 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -4,7 +4,8 @@ No framework, asserts only, same house style as test_freeze.py. -Five parts, and parts 3 and 4 are the ones that carry the acceptance criteria: +Six parts, and parts 3 and 4 are the ones that carry SAFETY-03's own +acceptance criteria; part 6 is TEST-11's: 1. THE CLAMP ARITHMETIC, on the shipped `clamp_to_rating()` - boundaries, symmetry, the device second stage, and the non-finite cases. Table driven. @@ -23,6 +24,13 @@ Five parts, and parts 3 and 4 are the ones that carry the acceptance criteria: 5. NO PATH BYPASSES IT. write_setpoint() is the only route to the wire, so it is called directly with an out-of-range value the way the heartbeat and the operator's buttons call it. + 6. TEST-11. THE GUARD READS THE WRITE THAT JUST HAPPENED. `self.clamp_last` + is assigned by `write_setpoint()` and by nothing else, so a + `check_output_clamp()` moved above the write keeps its loop-side evidence + and silently loses its write-side half. Graded as a CONSEQUENCE - what the + watchdog could see when it was consulted - so that a rename or a helper + does not trip it, and with the write-path half exercised on a stimulus of + its own, because parts 3-5 never produce one through cycle(). ------------------------------------------------------------------------------ WHY PART 4 ALSO ASSERTS ON `Decision`'s BOOLEANS AND NOT ON `reason` @@ -550,6 +558,240 @@ check("an in-range value is untouched", hass_d.writes[3] == 1500.0) check("every emitted frame satisfies |emitted| <= the rating", all(abs(w) <= 2000.0 for w in hass_d.writes)) +# ============================================================================== +print() +print("6. TEST-11: the guard's evidence is THIS cycle's write, not the last one") + +# WHY THIS PART EXISTS. `check_output_clamp()` sits one line below +# `await self.write_setpoint(self.target)` in cycle(), and moving it above the +# write left this file, test_freeze.py and test_loop.py ALL GREEN. That is not a +# missing assertion about line numbers - it is a missing assertion about +# EVIDENCE. The guard has two sources: +# +# * `self.loop_clamped`, set from the Decision well ABOVE the write. Still +# seen wherever the call is put, which is why the move is nearly invisible: +# on the §10.3 replay above, every engagement the alarm reports is this one. +# * `self.clamp_last`, which ONLY `write_setpoint()` assigns (main.py) and +# which is None until it has run. This half is what loses its evidence. +# +# So the check below does not pin the call's POSITION IN THE SOURCE, and must +# not: the comment at that line warns against tidying SAFETY-03's ordering and +# SAFETY-18's into one rule, and a check a rename breaks is worse than no check. +# It pins the CONSEQUENCE - that when the guard is consulted, `self.clamp_last` +# is the result of the write of THIS cycle - and part 6b runs the identical +# assertions through a deliberately refactored controller to prove it. + + +class _SpyWatchdog(ClampWatchdog): + """The shipped watchdog, plus a record of what the guard could SEE. + + ⚠️ IT HOOKS THE CONSUMER, NOT THE GUARD'S NAME. Wrapping + `check_output_clamp` by name would make a rename - AC 2's legitimate + refactor - look identical to the defect, and this ticket is explicitly not + allowed to buy its check with a nuisance. `clamp_wd.note()` is the one call + the guard exists to make, so it is hooked instead: it is reached from the + guard under any name, from any helper, as long as that helper runs after the + write. + """ + + def __init__(self): + super().__init__() + self.owner = None + self.seen = [] # clamp_last.requested_w at each consultation + self.seen_write = [] # was the WRITE-path half engaged, per cycle + + def note(self, engaged, reason, now): + cl = self.owner.clamp_last + self.seen.append(None if cl is None else cl.requested_w) + self.seen_write.append(bool(cl and cl.engaged)) + return super().note(engaged, reason, now) + + +def probed(cls=Controller, grid=9000.0, cycles=5, **over): + """`wired()`, with the watchdog replaced by the spy and the per-cycle + command returned. A RAMP is used on purpose: the slew limiter makes every + cycle's command a different number, which is what makes "this cycle's + evidence" and "last cycle's evidence" distinguishable at all.""" + async def go(): + # `track=True` - the plant follows, so the saturation freeze never + # engages and the slew limiter keeps the command MOVING; against a + # dead plant the freeze pins it flat after three cycles and the ramp + # stops being a ramp. + hass = _Hass(grid, track=True) + c = cls(dict( + {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": LIVE_MAX_W, "gain": 0.6, + "slew_w": 1000, "deadband_w": 15, "target_grid_w": 0, + "step_w": 10, "heartbeat_s": 10, "stale_input_s": 15}, + **over), hass, _Store(), _Pub(), stop=asyncio.Event()) + c.auto = True + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W + c.clamp_wd = _SpyWatchdog() + c.clamp_wd.owner = c + asked = [] + for _ in range(cycles): + await c.read_inputs() + await c.cycle() + asked.append(c.target) + return hass, c, asked + return asyncio.run(go()) + + +def evidence_checks(label, asked, wd): + """The same four claims, run against the shipped controller (6a) and against + a refactored one (6b). Sharing them is the point: if a rename or a helper + changed the answer, this function would say so in the same words.""" + seen = wd.seen + print(f" {label}: asked " + f"{[f'{w:+.0f}' for w in asked]}") + print(f" {label}: the guard saw " + f"{['None' if s is None else f'{s:+.0f}' for s in seen]}") + check(f"{label}: PRECONDITION - the command MOVES on every cycle, so this " + f"cycle's evidence and last cycle's are different numbers", + len(set(asked)) == len(asked) and len(asked) >= 4) + check(f"{label}: the guard was consulted exactly once per cycle", + len(seen) == len(asked)) + check(f"{label}: AC 1 - on EVERY cycle self.clamp_last held the result of " + f"the write THAT CYCLE. write_setpoint() is the only line that " + f"assigns self.clamp_last, so a check_output_clamp() moved above the " + f"write grades the PREVIOUS cycle's write instead", + seen == asked) + check(f"{label}: ...and on the FIRST cycle self.clamp_last was already set " + f"- before the run's first write it is None, i.e. the write-path half " + f"of the guard has no evidence at all", + bool(seen) and seen[0] is not None) + + +hass_e, ctl_e, asked_e = probed() +evidence_checks("shipped", asked_e, ctl_e.clamp_wd) + +# ⚠️ AND HERE IS WHY AC 3 NEEDS A STIMULUS OF ITS OWN, MEASURED RATHER THAN +# ASSERTED. Through the real cycle() the write-path half never engages: the +# arbiter files the device range and max_w as P_SAFETY LIMIT claims, so +# `resolution.target_w` is already inside both bounds before write_setpoint() +# re-applies them (main.py calls that duplication defence in depth, and it is). +# Every engagement the alarm reports on this path is therefore the loop-side +# one - which is exactly what makes the move nearly invisible, and exactly why +# 6c below drives the write path directly instead of hoping cycle() will. +check(f"MEASURED: the write-path half engaged 0 times in {len(asked_e)} real " + f"cycles ({sum(ctl_e.clamp_wd.seen_write)}), so no cycle()-driven check " + f"can be watching it", not any(ctl_e.clamp_wd.seen_write)) + + +class _RefactoredController(Controller): + """AC 2, as a fixture rather than as a promise: the two refactors a P3 tidy + would actually make, applied at once. + + * the guard is RENAMED to `_clamp_guard()`, the old name left as the thin + delegate such a rename leaves behind; + * the write MOVES INTO A HELPER, `_emit_setpoint()`. + + Neither changes when the guard runs relative to the write, so every check in + `evidence_checks()` must stay green. If one of them goes red here, the check + is grading the text of main.py and has to be thrown away. + """ + + async def _emit_setpoint(self, value, *, force=False): + await Controller.write_setpoint(self, value, force=force) + + async def write_setpoint(self, value, *, force=False): + await self._emit_setpoint(value, force=force) + + async def _clamp_guard(self): + await Controller.check_output_clamp(self) + + async def check_output_clamp(self): + await self._clamp_guard() + + +# ⚠️ VERIFIED AGAINST main.py ITSELF, NOT ONLY AGAINST THIS SUBCLASS. Renaming +# `check_output_clamp` throughout main.py AND moving both the write and the guard +# into a new `_emit_and_guard()` helper called from cycle() leaves every check in +# this part green; moving the guard above the write turns the two AC 1 lines +# below red. The one thing a repo-wide rename does cost is the direct call in 6c, +# which is a mechanical find/replace and fails as an AttributeError naming the +# new method - not as an assertion claiming the ordering is wrong, which is the +# false signal this part was written to avoid. +hass_f, ctl_f, asked_f = probed(cls=_RefactoredController) +evidence_checks("renamed + helper", asked_f, ctl_f.clamp_wd) +check("AC 2: the refactored controller behaves identically to the shipped one " + "- same commands, same evidence, so the check above is not pinning " + "either name", + asked_f == asked_e and ctl_f.clamp_wd.seen == ctl_e.clamp_wd.seen) + + +# --------------------------------------------------------------------------- # +# 6c. AC 3 - the WRITE-PATH half, exercised +# --------------------------------------------------------------------------- # +# The stimulus is SAFETY-20's shape and that is not a coincidence: `max_w` 5000 +# on an inverter that reports +-2500 is the one configuration where the value +# put on the wire DIFFERS from the value the loop asked for while every loop-side +# flag stays clean. That difference is the whole content of `clamp_last`, and a +# DEVICE-bound engagement is the only kind `loop_clamped` cannot also see. +# +# It is driven through `write_setpoint()` and the guard directly rather than +# through cycle(), because the measurement above says cycle() cannot produce it +# today. That is stated as a limitation, not hidden: what is being graded here is +# the guard's own contract - given a write that clamped, alarm - and the ordering +# in cycle() is what 6a grades. + + +async def _write_path(moved=False): + """`moved` runs the two calls in the order the ticket asks about.""" + hass = _Hass(0.0) + c = Controller({"setpoint_entity": "number.sp", "meter_source": "off", + "max_w": LIVE_MAX_W}, + hass, _Store(), _Pub(), stop=asyncio.Event()) + c.auto = True + # The inverter is SMALLER than the configured ceiling - SAFETY-20's install. + c.dev_min, c.dev_max = -2500.0, 2500.0 + c.target = 3000.0 + if moved: + await c.check_output_clamp() + await c.write_setpoint(c.target) + else: + await c.write_setpoint(c.target) + await c.check_output_clamp() + return hass, c + + +hass_w, ctl_w = asyncio.run(_write_path()) +notes_w = clamp_notes(hass_w) +print(f" max_w {LIVE_MAX_W:.0f} on a +-2500 W device, asked for " + f"{ctl_w.target:+.0f} W: the device saw {hass_w.writes[-1]:+.0f} W, " + f"bound={ctl_w.clamp_last.bound!r}, loop_clamped={ctl_w.loop_clamped[0]}") +check("PRECONDITION: the write really did clamp, on the DEVICE bound", + ctl_w.clamp_last is not None and ctl_w.clamp_last.engaged + and ctl_w.clamp_last.bound == "device" + and hass_w.writes[-1] == 2500.0) +check("...and the loop-side half is SILENT here, so self.clamp_last is the " + "only evidence that this happened at all", + ctl_w.loop_clamped == (False, "")) +check("AC 3: the guard alarmed on write-path evidence alone", + ctl_w.clamp_wd.engagements == 1 and len(notes_w) == 1) +check("...and the operator is told WHICH bound bit, so a device-rating clamp " + "is not reported as a max_w one", + bool(notes_w) and "device bound" in notes_w[0]["message"]) +check("...and the action log carries the row, written by write_setpoint()", + any(REASON_CODE in e and "device bound" in e for e in ctl_w.events)) + +# The same stimulus, the same clamp, the two calls swapped. This is the ticket +# in one pair of runs. +hass_m, ctl_m = asyncio.run(_write_path(moved=True)) +print(f" the same stimulus with the guard consulted BEFORE the write: " + f"{ctl_m.clamp_wd.engagements} engagement(s), " + f"{len(clamp_notes(hass_m))} notification(s), and the clamp still " + f"happened (engaged={ctl_m.clamp_last.engaged})") +check("THE DEFECT, DEMONSTRATED: consulted before the write the guard is " + "SILENT - self.clamp_last is still None at that point - while the wire " + "carries a clamped value either way", + not clamp_notes(hass_m) and ctl_m.clamp_wd.engagements == 0 + and ctl_m.clamp_last.engaged and hass_m.writes[-1] == 2500.0) +check("...and main.py:856's consumer is the line that goes quietly false", + bool(ctl_w.clamp_last and ctl_w.clamp_last.engaged)) + + print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}")