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()