From e941bf7f62f69fd93811f8bdf1ac7481bdece170 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Wed, 2 Sep 2026 14:58:54 +0200 Subject: [PATCH] DEV-02: zero every unit's target when the operator stops, not just the site's A hazard THIS CARD CREATED, found by re-reading the operator paths rather than by a failing check - so a check now exists for it. Before DEV-02 the heartbeat re-emitted `self.target`, so the stop button zeroing that one scalar was enough: the next refresh carried the zero. The heartbeat now re-emits `inv.target` - it has to, or it would put the SITE number on every inverter, which is the ARCH-01 broadcast fault arriving once every `heartbeat_s` on a path with no allocator in it. So a stop that zeroed only `self.target` left each unit's LAST SHARE standing. No cycle has run yet at that moment, so nothing else would have cleared it, and the very next heartbeat - up to `heartbeat_s` later - would put it back on the wire. The operator presses stop, watches the inverter go to zero, and sees it resume seconds later. `command_site_zero()` moves both together and carries the reason. It is one method rather than two lines at three call sites precisely because the two lines have to stay together: shutdown(), the stop button and maint_abort all mean the same thing, and 0 W is the one value that may legitimately be written site-wide. test_inverter.py part 6 presses stop with 995/995 standing and then runs the SHIPPED `send_heartbeats()` before any cycle, which is the racing refresh. The mutation that zeroes the site target alone takes it red. 19 files green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/app/main.py | 38 ++++++++++++++++++++------- goodwe_controller/test_inverter.py | 41 +++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index e817a53..6be824b 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -2181,14 +2181,32 @@ class Controller: """Deterministic wind-down. Do not skip this.""" self.stopping = True _LOG.info("shutting down - commanding 0 W on every inverter") - # 0 W is the one value that means the same thing to every inverter, so - # the site-wide form of the write is legitimate here - see - # write_setpoint(). Each unit's target is zeroed too, so a heartbeat - # racing this cannot re-emit the old share. + await self.command_site_zero() + self.mqtt.close() + + async def command_site_zero(self) -> None: + """Every inverter to 0 W, and every inverter's TARGET to 0 W with it. + + ⚠️ THE SECOND HALF IS NOT BOOKKEEPING, AND IT IS A HAZARD DEV-02 + CREATED. Before this card the heartbeat re-emitted `self.target`, so + zeroing that one scalar was enough: the stop button set it and the next + refresh carried the zero. The heartbeat now re-emits `inv.target` - it + has to, or it would broadcast the site number - so a stop that zeroed + only `self.target` would leave each unit's LAST SHARE standing, and the + very next heartbeat (up to `heartbeat_s` later, and before any cycle has + run) would put it back on the wire. The operator would have pressed + stop, watched the inverter go to zero, and seen it resume seconds later. + Both must move together, which is why this is one method with a reason + and not two lines repeated at three call sites. + + ⚠️ AND 0 W IS THE ONE VALUE THAT MAY BE WRITTEN SITE-WIDE. It means the + same thing to every inverter; a non-zero site-wide write is the ARCH-01 + broadcast fault, and `write_setpoint()` refuses it. + """ + self.target = 0.0 for inv in self.inverters: inv.target = 0.0 await self.write_setpoint(0.0, force=True) - self.mqtt.close() # -- UI ------------------------------------------------------------------ def checks(self, when: datetime | None = None) -> list: @@ -2572,8 +2590,11 @@ class Controller: self.log_event(f"sign-disagreement failsafe cleared by " f"operator on {inv.name}") if not self.auto: - self.target = 0.0 - await self.write_setpoint(0.0, force=True) + # ⚠️ THE WHOLE SITE, TARGETS INCLUDED - see command_site_zero(). + # Zeroing `self.target` alone would leave each unit's last share + # standing for its next heartbeat to re-emit, which an operator + # would read as the stop button not working. + await self.command_site_zero() return {"auto": self.auto} if what == "maint_start": for msg in self.maint.force_start(now): @@ -2582,8 +2603,7 @@ class Controller: if what == "maint_abort": for msg in self.maint.abort(now, "operator"): self.log_event(msg) - self.target = 0.0 - await self.write_setpoint(0.0, force=True) + await self.command_site_zero() return {"phase": self.maint.phase} return None diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py index 5f187cb..41e5493 100644 --- a/goodwe_controller/test_inverter.py +++ b/goodwe_controller/test_inverter.py @@ -954,6 +954,41 @@ check("...and the shipped budget is restored after the experiments, so nothing " app_main.WRITE_BUDGET_S == WRITE_BUDGET_S) +# ⚠️ AND THE OPERATOR'S STOP HAS TO MOVE THE SAME TWO THINGS. This is a hazard +# THIS CARD CREATED and it is worth stating plainly: before DEV-02 the heartbeat +# re-emitted `self.target`, so the stop button zeroing that one scalar was +# enough. It now re-emits `inv.target` - it must, or it would broadcast the site +# number - so a stop that zeroed only the site target would leave each unit's +# last share standing for the very next refresh to put back on the wire, seconds +# after the operator watched the inverter go to zero. +async def _stop_then_beat(): + store = MemStore() + hass = Hass() + c, _ = built({"auto_start": True}, store=store, hass=hass, + second=inverter_b(store)) + c.auto = True + c.target = 1990.0 + c.inverters[0].target, c.inverters[1].target = 995.0, 995.0 + await c.handle_action("auto_toggle") # -> stopped + stopped_at = list(hass.writes) + # A heartbeat racing the press, before any cycle has run. + await c.send_heartbeats(1000.0, 10.0) + return c, hass, stopped_at + + +_c6s, _h6s, _stop_writes = run(_stop_then_beat()) +print(f" stop pressed with shares standing at 995/995: wrote {_stop_writes}, " + f"then the next heartbeat wrote {_h6s.writes[len(_stop_writes):]}") +check("the stop button commands 0 W to EVERY inverter", + sorted(_stop_writes) == [("number.sp_a", 0.0), ("number.sp_b", 0.0)]) +check("⚠️ AND THE NEXT HEARTBEAT DOES NOT PUT THE OLD SHARES BACK. Each unit's " + "own target was zeroed with the write, so the refresh that proves this " + "add-on is alive carries the zero rather than resurrecting the command " + "the operator just stopped", + all(v == 0.0 for _e, v in _h6s.writes) + and all(i.target == 0.0 for i in _c6s.inverters)) + + # ============================================================================== print() print("7. AC 7: live_units() answers per-instance liveness honestly") @@ -1145,6 +1180,10 @@ MUTATIONS = [ "test_inverter.py part 4: 'the pack floor is the EMPTIEST pack'"), ("`device`/`supervised` filed without `inv` - B's rating on A", "test_peakguard.py 10g: the static routing pin, at the line"), + ("the stop button zeroing `self.target` but not each `inv.target`", + "test_inverter.py part 6: 'the next heartbeat does NOT put the old shares " + "back' - a hazard THIS CARD created, because the heartbeat re-emits a " + "share now and not the site number"), ] # ⚠️ TWO OF THESE SURVIVED THE FIRST TIME AND THE CHECKS WERE CHANGED, NOT THE # TABLE. The serialised dispatch and the shared `last_beat` both passed while @@ -1159,7 +1198,7 @@ for what, caught in MUTATIONS: check(f"all {len(MUTATIONS)} defeats of this card are caught by a named check " f"above. A guard whose checks survive its own defeat is not a guard, and " f"this repository has three separate records of exactly that", - len(MUTATIONS) == 18) + len(MUTATIONS) == 19) print()