From e306c74cb81a9b8ef1621f87a39a093c196b1ebe Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Wed, 2 Sep 2026 14:50:43 +0200 Subject: [PATCH 1/3] DEV-02: per-instance inverter objects, so nothing is graded against a sibling Every piece of per-device state in the Controller was a scalar, and several of them are safety inputs. app/inverter.py introduces an `Inverter` that owns its entities, its rating, its device range, its SignWatchdog, its last_write and its last_beat, under store keys namespaced by unit name. The Controller holds a list of them and keeps only what is genuinely site-level: the P1 reading, the site integrator, and the tariff quarter. WHAT THE SCALARS WERE SILENTLY ASSERTING dev_min/dev_max one pair, read from one setpoint_entity. A second inverter's rating was NEVER READ AT ALL, so its clamp, its freeze rail and the peak guard's achievable_w were the first one's numbers. soc one reading into the maintenance FSM, SAFETY-24's pack floor and arbitrage's bounds. An average lets the emptier pack be driven below its own floor. sign_wd ONE SignWatchdog, whose own docstring says "one instance per actuator", latched under FLAT store keys - so a trip caused by B latched A across restarts and one operator press cleared both. last_beat one deadline and one serialised hass.call(), so a hung write to A ate B's 30 s layer-1 budget. actual_w BECAME SITE LEVEL WHILE THE READINGS BECAME PER-INSTANCE, and those are opposite directions on purpose. compute()'s anti-windup test is `abs(prev_w - actual_w) > saturation_w` with prev_w the SITE target, so feeding it one inverter's power compares a site number against a unit number, trips the saturation freeze permanently and leaves a standing grid error. Measured on admin/goodwe's two-master rig (tools/alloc_sim.py --splits, 2 kW load, 40 cycles), worst |delta| between splits at the meter: gain one unit's power the site sum 0.2 510 W 0 W 0.4 250 W 0 W 0.6 50 W 0 W That was ALLOC-01's disclosed finding; test_alloc.py part 8 kept its stimulus and now grades the other row. THE STORE MIGRATION IS GRADED, NOT MERELY PERFORMED. sign_tripped/sign_reason move to unit//... once, verified by READING BACK what was written, and the version marker is set only when nothing that carried state was lost. The flat keys stay in the file so a rollback still finds its latch. And __init__ compares the pre-migration flat latch against what unit A actually came up with: if the latch did not survive it is RE-ARMED from the legacy value and shouted about. A latch that silently fails to migrate is a disarmed safety guard on an install whose state file says it is protected - which is strictly worse than never persisting it, because now there is evidence saying it is fine. test_inverter.py part 2 breaks the migration and watches the guard fire. HEARTBEATS ARE INDEPENDENT AND THE BUDGET IS DERIVED. Each inverter owns its deadline, the refreshes go out concurrently, and each write is bounded by WRITE_BUDGET_S = 4 s. The inequality is graded rather than asserted: 4 + heartbeat_s's schema maximum of 25 < 30, validate.FIRMWARE_INGEST_VETO_S. Verified by injecting a transport that accepts and never answers. live_units() ANSWERS HONESTLY. A unit whose every write has been rejected for a whole layer-1 window is not commandable; the allocator drops it, the survivors carry its share, the action log names it, and the dropped unit is commanded 0 W rather than left standing on its last share. A roster entry with NO setpoint_entity is deliberately not handled there - that is a configuration that cannot be delivered, and it reaches main.py's refusal branch, which is how that branch keeps a live trigger after this card removed its old one. AT N=1 NOTHING MOVES. The removed scalars survive as read/write properties of the sole inverter, so every existing replay runs unmodified - and they REFUSE at N>=2 rather than answering for the first inverter, which is the tripwire on a consumer that was never made per-instance. EVIDENCE. 19 files green. test_inverter.py is new (82 checks). Eighteen mutations were applied to the shipped modules on a purged tree via tools/mutation_run.sh and each was observed RED. Two of them SURVIVED first time and the checks were changed rather than the table: the serialised dispatch and the shared last_beat both passed while part 6 gathered its own writes and computed its own `due` list. send_heartbeats() was extracted from run_control() so the shipped dispatch is reachable by a check at all. INVERTER B STILL DOES NOT EXIST. build_roster() returns exactly one inverter from the flat options and is the one function CFG-04 changes; every two-unit figure here is architecture, and the second rating in the checks is marked a hypothesis. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 62 ++ goodwe_controller/app/inverter.py | 334 ++++++++ goodwe_controller/app/main.py | 1126 ++++++++++++++++++++----- goodwe_controller/app/store.py | 174 ++++ goodwe_controller/test_alloc.py | 77 +- goodwe_controller/test_clamp.py | 16 +- goodwe_controller/test_inverter.py | 1169 ++++++++++++++++++++++++++ goodwe_controller/test_peakguard.py | 314 +++++-- goodwe_controller/test_watchdog.py | 36 +- goodwe_controller/tools/alloc_sim.py | 44 +- 10 files changed, 3027 insertions(+), 325 deletions(-) create mode 100644 goodwe_controller/app/inverter.py create mode 100644 goodwe_controller/test_inverter.py diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 464a770..35b09a2 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,68 @@ ## Unreleased +**Every per-device value stops being a Controller scalar: rating, SoC, battery +power, watchdogs, heartbeat, write bookkeeping and store keys are now +per-instance (DEV-02).** `app/inverter.py` introduces an `Inverter` object that +owns its entities, its rating, its device range, its `SignWatchdog`, its +`last_write` and its `last_beat`, under **store keys namespaced by unit name**. +The Controller holds a list of them and keeps only what is genuinely site-level: +the P1 reading, the site integrator, and the tariff quarter. + +* **What the scalars were silently asserting.** One `dev_min`/`dev_max` pair was + read from one `setpoint_entity`, so a second inverter's rating was **never + read at all** and its clamp, its freeze rail and the peak guard's + `achievable_w` were the first one's numbers. One `soc` fed the maintenance + FSM, SAFETY-24's pack floor and arbitrage's bounds. One `SignWatchdog` - whose + own docstring says *"one instance per actuator"* - latched under **flat** store + keys, so a trip caused by B latched A across restarts and one operator press + cleared both. One `last_beat` and one serialised `hass.call()` meant a hung + write to A ate B's 30 s layer-1 budget. +* **`actual_w` became SITE level while the readings became per-instance, and + those are opposite directions on purpose.** `compute()`'s anti-windup test is + `abs(prev_w - actual_w) > saturation_w` with `prev_w` the **site** target, so + feeding it one inverter's power compares a site number against a unit number, + trips the saturation freeze permanently and leaves a standing grid error. + Measured on `admin/goodwe`'s two-master rig, 2 kW load: with one unit's power + the aggregate trace at the meter is split-dependent by **510 W at `gain: 0.2`, + 250 W at 0.4 and 50 W at 0.6**; with the site sum **every split is + bit-identical at every gain the schema accepts**. That was ALLOC-01's + disclosed finding, graded in `test_alloc.py` part 8 and now closed there. +* **The store migration is graded, not merely performed.** `sign_tripped` / + `sign_reason` move to `unit//...` once, verified by **reading back** + what was written, and the version marker is only set when nothing that carried + state was lost. The flat keys are left in the file so a rollback still finds + its latch. And `Controller.__init__` compares the pre-migration flat latch + against what unit A actually came up with: if the latch did not survive it is + **re-armed from the legacy value and shouted about**, because a latch that + silently fails to migrate is a disarmed safety guard on an install whose state + file says it is protected. +* **Heartbeats are independent, and the budget is derived.** Each inverter owns + its own deadline, the refreshes are dispatched **concurrently**, and each write + is bounded by `WRITE_BUDGET_S` (4 s). The inequality that fixes that number is + graded rather than asserted: `4 + heartbeat_s`'s schema maximum of `25 < 30`, + layer 1's `FIRMWARE_INGEST_VETO_S`. Verified by injecting a transport that + accepts and never answers: B's write reaches the wire while A is still + hanging, and the serialised shape it replaces is run beside it. +* **`live_units()` answers honestly.** A unit whose every write has been rejected + for a whole layer-1 window is not commandable; the allocator drops it, the + survivors carry its share, the action log names it, and the dropped unit is + commanded **0 W** rather than left standing on its last share. A roster entry + with **no** `setpoint_entity` is deliberately *not* handled there - that is a + configuration that cannot be delivered, and it reaches main.py's refusal + branch, which commands 0 W and names the inverter. +* **At N=1 nothing moves.** The removed scalars survive as read/write properties + of the sole inverter, so every existing replay in this repository runs + unmodified - and they **refuse at N>=2** rather than answering for the first + inverter, which is the tripwire on a consumer that was never made + per-instance. `test_inverter.py` is the new file (77 checks); `test_alloc.py` + part 8, `test_peakguard.py` 10g/10h, `test_watchdog.py` and `test_clamp.py` + were re-aimed rather than relaxed. +* **Inverter B still does not exist.** `build_roster()` returns exactly one + inverter, from the flat options, and is the one function CFG-04 changes. Every + two-unit figure above is architecture, not a measurement of a second device; + the second rating in the checks is marked as a hypothesis. + **The site grid target is now ALLOCATED across N inverters, sum-invariant, and never broadcast (ALLOC-01).** `ARB-03` left `allocate()` as a documented seam that passed through at N<=1 and DECLINED at N>=2; this fills it. The new diff --git a/goodwe_controller/app/inverter.py b/goodwe_controller/app/inverter.py new file mode 100644 index 0000000..589b97d --- /dev/null +++ b/goodwe_controller/app/inverter.py @@ -0,0 +1,334 @@ +"""One inverter, as an OBJECT rather than as a set of Controller scalars. + +WHAT THIS FIXES, in one sentence: every piece of per-device state in this +add-on used to be a single scalar on the Controller, and several of those +scalars are SAFETY INPUTS - so a second inverter would have been graded against +the first one's numbers. + +The scalars, and what each one was silently asserting: + + `dev_min` / `dev_max` ONE pair, read from ONE `setpoint_entity`. A second + inverter's rating was never read at all, so its clamp, + its freeze rail and the peak guard's `achievable_w` + were all inverter A's. + `soc` ONE state of charge into the maintenance FSM, + SAFETY-24's pack floor and arbitrage's bounds. An + average lets the emptier pack be driven below its own + floor; a maximum parks the fuller one. + `batt` ONE battery power into the anti-windup saturation + detector - see the SITE/UNIT note at the bottom of + this docstring, because this one goes the OTHER WAY. + `sign_wd` ONE SignWatchdog, whose own docstring says "one + instance per actuator", latched under FLAT store keys. + A trip caused by B latched A across restarts and one + operator action cleared both. + `last_write` ONE write bookkeeping, and ONE serialised `hass.call()` + behind it: a hung write to A ate B's 30 s layer-1 + budget. + +⚠️ WHAT IS DELIBERATELY *NOT* HERE, AND IT IS THE HALF THAT IS EASY TO GET +BACKWARDS. THERE IS NO `Tuning` ON THIS OBJECT AND THERE IS NO INTEGRATOR. +§6.2 and ALLOC-01's stability argument are ONE integrator on P1, ABOVE the +split: the site target S is divided into per-unit shares, so the gain seen at +the meter is a CONVEX COMBINATION of the per-unit gains and a bad split cannot +destabilise the site. Give each inverter its own `Tuning` and somebody will +eventually call `compute()` with it, and the moment two integrators regulate +one meter the whole argument is gone - that is the two-writers failure the +arbiter exists to prevent, one level up. + +The one field of `Tuning` that genuinely IS per-device is `max_w`, the +supervised cap on what may be WRITTEN to one setpoint entity (§11.3), and it is +here as `max_w`. The SITE `Tuning` the loop runs on takes its bound from the +roster - `sum(inv.max_w)` - so at N=1 it is byte-identical to the shipped +number and at N>=2 it is what the site can actually deliver. That is the whole +of "the rating stops being a scalar"; a per-unit Tuning would be three more +numbers nobody reads and one more place a limit has to be kept in step (CFG-03 +already tracks three). + +------------------------------------------------------------------------------ +SITE OR UNIT? THE TWO HALVES OF THE N=1 IDENTITY, AND THEY MOVE IN OPPOSITE +DIRECTIONS + +At N=1 §6.2's identity holds: the fabricated grid reading and the battery +setpoint are the same number, so "the site" and "the inverter" are the same +thing and every scalar above was RIGHT. Splitting them is not one change, it is +two, and they point opposite ways: + + COMMANDING becomes per-unit. One setpoint entity per inverter, one rating, + one clamp, one watchdog, one heartbeat. + SENSING, for the loop, becomes SITE. `compute()` regulates the METER. Its + `actual_w` is the anti-windup input to + `abs(prev_w - actual_w) > saturation_w`, where + `prev_w` is the SITE target. Feed it ONE unit's + power at a roster of two and that test compares + a site number against a unit number, trips the + freeze permanently and leaves a standing grid + error. + +That second half is MEASURED, not argued. On admin/goodwe's two-master rig +(`tools/alloc_sim.py --splits --feedback unit`), with the loop fed one +inverter's power the aggregate trace at the meter becomes a function of the +split - up to 510 W apart at gain 0.2, 250 W at gain 0.4, 50 W at gain 0.6 - +while the allocator's shares sum to S in every one of those runs. The +counterfactual is the proof: change that ONE input to the site total and every +split is bit-identical at every gain the schema accepts. ALLOC-01's review +isolated it and test_alloc.py part 8 graded it as a disclosed defect waiting on +this card. + +So this object owns `batt` - THIS inverter's own power, which is what a +per-unit consumer needs - and the Controller sums them into the site figure the +loop is fed. Getting that backwards reintroduces the divergence. + +------------------------------------------------------------------------------ +LIVENESS, AND WHY IT IS A METHOD WITH A REASON + +`Controller.live_units()` decides which inverters may be commanded this tick, +and `allocator.plan()` DROPS what it does not list - the survivors then carry +the whole site target. A wrong answer there is a real change in what the site +can deliver, so `is_live()` answers one question and says which: CAN THIS +CONTROLLER PUT A NUMBER ON THIS INVERTER RIGHT NOW. + +ONE way it cannot, and exactly one: every write has been REJECTED for a whole +`LINK_STALE_S` window. Not one rejection - `hass.set_number()` returns False for +a transient 400 as readily as for a device that has gone - but a continuous run +of them with no acceptance in between. + +⚠️ AND NOT "IT HAS NO `setpoint_entity`", THOUGH THAT IS ALSO A UNIT THIS +CONTROLLER CANNOT COMMAND. The two look the same and want opposite answers. A +link that WAS working and stopped is an event: the survivors carry its share, +the record says so, and the site goes on running at reduced strength, which is +the best available outcome. A roster entry with no writer configured at all is a +CONFIGURATION that cannot be delivered - it was never reachable, nothing has +gone wrong on the wire, and quietly running the site at part strength on a +roster somebody wrote down wrong is how a half-driven site reads as healthy on +every dashboard. That one belongs to main.py's refusal branch, which commands +0 W and NAMES the inverter; putting it here would absorb it into the ordinary +drop and delete the last trigger that branch has. See `Controller.cycle()`. + +⚠️ AND NOT "ITS SENSORS ARE MISSING" EITHER. A pack whose SoC did not answer is +a third fault with a third remedy: `inputs_ok` already stops the whole site at +P_SAFETY for that, and dropping the unit from the SPLIT instead would hand its +share to a sibling while the site kept regulating - the opposite of what a +missing measurement should cause. Liveness is about the WRITE PATH, and only +about a write path that has stopped answering. +""" + +import math +from datetime import datetime + +from .clamp import ClampResult +from .store import unit_key +from .watchdog import SignWatchdog, SignWatchdogConfig + +# How long every write to one inverter must have been REJECTED before this +# controller stops claiming it can command it. +# +# ⚠️ IT IS LAYER 1'S OWN WINDOW, NOT A NEW OPINION ABOUT LINKS. The ESP32 +# commands 0 W and keeps commanding it after ~30 s without a refresh +# (`validate.FIRMWARE_INGEST_VETO_S`), so a link this controller has not been +# able to write for a whole 30 s is a link the hardware is about to take away +# from it anyway. Claiming it as commandable past that point is the add-on +# telling the allocator it can deliver watts that the firmware has already +# decided it will not. No config key: CFG-03 tracks three places a default has +# to be kept in step and this does not earn a fourth - it is derived from a +# constant the firmware and validate.py already share. +LINK_STALE_S = 30.0 + +# The one inverter this add-on drives today. ⚠️ A NAME, NOT A CONFIGURATION - +# it exists so that a per-inverter claim has an actuator to be filed ON, which +# is what makes the site/unit split something a check can grade rather than a +# comment. It lives here rather than in main.py now that the roster does. +UNIT_A = "inverter-a" + +# The per-unit keys this object keeps in the Store, namespaced by unit name. +# ⚠️ THE VALUES ARE THE PRE-DEV-02 FLAT KEY NAMES, and that is what makes the +# migration mechanical rather than transcribed: `store.migrate_unit_keys()` +# reads this same mapping, so a key renamed here cannot silently stop being +# migrated. See store.py. +UNIT_STORE_KEYS = ("sign_tripped", "sign_reason") + + +class Inverter: + """ONE inverter: its entities, its rating, its watchdogs, its store keys. + + Constructed once per configured inverter and held in `Controller.inverters`. + Everything on it is about THIS device; nothing on it is about the site. + + ⚠️ `hass` IS NOT A CONSTRUCTOR ARGUMENT. This object is built in + `Controller.__init__`, which runs before any I/O and, in four existing + checks, with `hass=None` entirely. The device range is read LATER, by + `Controller.run_control()`, from THIS unit's own `setpoint_entity` - which + is the whole of "each instance's rating is read from its own entity". + """ + + def __init__(self, name, *, setpoint_entity="", soc_entity="", + batt_entity="", batt_invert=False, max_w=2000.0, + store=None, sign_cfg=None): + self.name = str(name) + self.setpoint_entity = str(setpoint_entity or "") + self.soc_entity = str(soc_entity or "") + self.batt_entity = str(batt_entity or "") + self.batt_invert = bool(batt_invert) + # The supervised cap on what may be written to THIS setpoint entity. + # See the module docstring on why this is a number and not a `Tuning`. + self.max_w = float(max_w) + + # --- the device's own reported range, read from its own entity ------- + # None until `hass.limits()` has answered. NOT guessed at: ESPHome is + # often not up when the add-on boots, and (None, None) is the honest + # report that the range checks were skipped rather than invented. + self.dev_min = None + self.dev_max = None + + # --- this unit's own measurements ------------------------------------ + self.soc = None + self.batt = None + + # --- what this unit was last told, and what it last accepted --------- + # `target` is THIS unit's share of the site target from the last + # resolution - the number its heartbeat re-emits and the number + # SAFETY-02 grades against the meter. At N=1 it IS the site target, + # because the allocator is a pass-through there. + self.target = 0.0 + self.last_write = None + self.last_write_at = None + self.last_write_ok = None + # ⚠️ TWO TIMESTAMPS, NOT ONE FLAG. `last_write_ok` is the last write's + # verdict and it flips on a single transient 400; liveness needs a RUN + # of rejections, so `rejected_since` records when the current run + # started and is cleared by any acceptance. + self.rejected_since = None + # The heartbeat's own deadline, on the event loop's monotonic clock. + # ⚠️ PER UNIT, WHICH IS THE WHOLE OF AC 6. One shared `last_beat` and + # one serialised write meant a hung call to A pushed B's refresh past + # layer 1's ~30 s veto - i.e. one inverter's transport fault zeroing a + # healthy sibling. Starts at 0.0, which is what the single scalar did. + self.last_beat = 0.0 + + # --- SAFETY-03's per-write evidence ---------------------------------- + # What THIS unit's last write clamped, or None before it has run. The + # ALARM (`ClampWatchdog`) stays one per controller - clamp.py says so in + # its own docstring, and it is an anti-flood counter over an event bus + # rather than a property of a device - but the EVIDENCE is per unit, + # because a clamp on B says nothing about A. + self.clamp_last: ClampResult | None = None + + # --- SAFETY-02, one instance per actuator, under NAMESPACED keys ----- + # ⚠️ THE WATCHDOG'S OWN DOCSTRING SAID "one instance per actuator" WHILE + # THE CONTROLLER HELD EXACTLY ONE. Restored from THIS unit's own keys, + # so a trip caused by B latches B and leaves A commanding - and an + # operator clearing one does not silently clear the other. + self.store = store + data = getattr(store, "data", {}) if store is not None else {} + self.sign_wd = SignWatchdog( + sign_cfg or SignWatchdogConfig(), + tripped=bool(data.get(self.key("sign_tripped"), False)), + reason=str(data.get(self.key("sign_reason"), "") or "")) + + # -- store --------------------------------------------------------------- + def key(self, name: str) -> str: + """This unit's namespaced Store key for `name`. See store.unit_key().""" + return unit_key(self.name, name) + + def store_set(self, name: str, value) -> None: + if self.store is not None: + self.store.set(self.key(name), value) + + # -- the numbers other modules ask this object for ----------------------- + def rating_w(self) -> float: + """The largest magnitude this unit can be commanded, in W. + + `min(max_w, |dev_min|, |dev_max|)` - the supervised cap and whatever the + device itself reports, whichever is smaller. It is the same conjunction + `freeze.rail_ceiling_w()` computes for the freeze rail and the same one + main.py hands the peak guard as `achievable_w`, which is why it is + written once here: three transcriptions of a rating is three chances for + inverter B to be graded against inverter A's number. + + A bound that could not be read is SKIPPED, not treated as zero: (None, + None) means ESPHome was not up, and answering 0 W there would say this + inverter can do nothing. + """ + out = abs(float(self.max_w)) + for bound in (self.dev_min, self.dev_max): + if bound is not None and math.isfinite(float(bound)): + out = min(out, abs(float(bound))) + return out + + def has_writer(self) -> bool: + return bool(self.setpoint_entity) + + # -- liveness ------------------------------------------------------------ + def note_write(self, ok: bool, now: datetime) -> None: + """Record one write's verdict. The only thing that moves liveness.""" + self.last_write_ok = bool(ok) + if ok: + self.rejected_since = None + elif self.rejected_since is None: + self.rejected_since = now + + def is_live(self, now: datetime, stale_s: float = LINK_STALE_S): + """`(live, why_not)` - may this controller command this unit THIS TICK? + + Read the module docstring before widening this - in particular on why a + unit with NO `setpoint_entity` is deliberately still "live" here and is + refused one layer up instead. The allocator DROPS what `live_units()` + does not list and the survivors carry the whole site target, so a false + "not live" is a site target divided among fewer inverters than exist - + and at a roster of one it is the allocator refusing outright and the + site commanding 0 W. + + ⚠️ ANSWERED PER TICK, NEVER CACHED, for `allocator.plan()`'s reason: a + link goes stale between one cycle and the next. + """ + if self.rejected_since is not None: + held = (now - self.rejected_since).total_seconds() + if held >= float(stale_s): + return False, (f"every write rejected for {held:.0f} s " + f"(limit {float(stale_s):.0f} s)") + return True, "" + + def link_note(self, now: datetime) -> str: + """One line for the status page. Empty when there is nothing to say.""" + if not self.has_writer(): + return (f"{self.name}: NO SETPOINT ENTITY - this controller cannot " + f"command it, and the site refuses rather than running at " + f"part strength") + live, why = self.is_live(now) + if not live: + return f"{self.name}: NOT COMMANDABLE - {why}" + if self.rejected_since is not None: + held = (now - self.rejected_since).total_seconds() + return (f"{self.name}: writes rejected for {held:.0f} s - still " + f"commandable until {LINK_STALE_S:.0f} s") + return "" + + def __repr__(self) -> str: + return (f"") + + +def build_roster(opts: dict, store, sign_cfg=None) -> list: + """The site's inverters, in roster order. TODAY THAT IS EXACTLY ONE. + + ⚠️ INVERTER B DOES NOT EXIST YET (~2 weeks out) AND THIS FUNCTION MUST NOT + INVENT IT. There is no `inverters:` list in config.yaml, no second + `setpoint_entity`, and no measured rating, ramp or lag for a second device - + SPIKE-04 measures those and CFG-04 shapes the option that carries them. What + this card delivers is that the CODE no longer assumes there is one; adding + the second entry is a config change to THIS FUNCTION and to nothing else, + which is the point. + + So: unit A is built from the flat options, exactly as the shipped scalars + were, and the flat options keep their names. An install that upgrades reads + identically - same entities, same rating, same behaviour - which is AC 8. + """ + return [Inverter( + UNIT_A, + setpoint_entity=opts.get("setpoint_entity", ""), + soc_entity=opts.get("soc_entity", ""), + batt_entity=opts.get("batt_entity", ""), + batt_invert=bool(opts.get("batt_invert")), + max_w=float(opts.get("max_w", 2000)), + store=store, sign_cfg=sign_cfg)] + diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 4b7ffd4..e817a53 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -39,13 +39,16 @@ from .arbiter import ( ) from .control import Tuning, compute, maintenance_charge_floor, peak_at_risk from .clamp import ( - REASON_CODE as CLAMP_REASON, ClampResult, ClampWatchdog, clamp_to_rating, - is_commandable, + REASON_CODE as CLAMP_REASON, ClampWatchdog, clamp_to_rating, is_commandable, ) from .freeze import ( FreezeWatchdog, FreezeWatchdogConfig, looks_frozen, rail_unreachable, rail_w, ) from .hass import HomeAssistant +# ⚠️ `LINK_STALE_S` IS RE-EXPORTED DELIBERATELY. `live_units()` reasons about it +# and two checks read `app.main.LINK_STALE_S` to build a stale link, so the +# window an operator would look up here is the one the roster actually uses. +from .inverter import LINK_STALE_S, UNIT_A, build_roster from .maintenance import IDLE, MaintConfig, Maintenance from .mqtt import MqttPublisher from . import peakguard @@ -53,18 +56,39 @@ from .p1 import P1Ingest, build_source, is_enabled from .price import Uncertainty, price_option from .static_tariff import resolve_tariff, static_status from .tariff import PeakStore, PeakTracker +from .store import migrate_unit_keys from .validate import ConfigRefused, refuse, validate_startup_config -from .watchdog import SignWatchdog, SignWatchdogConfig, loop_is_following +from .watchdog import SignWatchdogConfig, loop_is_following from . import web OPTIONS_PATH = "/data/options.json" _LOG = logging.getLogger("goodwe") -# The one inverter this add-on drives. ⚠️ A NAME, NOT A CONFIGURATION. It exists -# so that a per-inverter claim has an actuator to be filed ON, which is what -# makes the site/unit split something a check can grade rather than a comment. -# ALLOC-01 replaces this with a real roster when inverter B arrives. -UNIT_A = "inverter-a" +# ⚠️ `UNIT_A` NOW LIVES IN app/inverter.py AND IS RE-EXPORTED HERE, because the +# roster does. It stays importable from this module: four checks read +# `app.main.UNIT_A` and it is still the name of the inverter this add-on drives. +# DEV-02 is what turned "the one inverter" into a list; `build_roster()` is the +# one function CFG-04 changes to put a second entry in it. +__all__ = ["UNIT_A", "Controller", "amain", "main"] + +# --- DEV-02: THE PER-WRITE BUDGET, AND WHY IT IS SMALLER THAN A HEARTBEAT ---- +# Every write to every inverter is dispatched CONCURRENTLY and each one is +# bounded by this. Both halves are the acceptance criterion, not an +# optimisation: one hung `hass.call()` used to sit on the single serialised +# write path and eat a SIBLING'S layer-1 budget - the ESP32 commands 0 W after +# ~30 s without a refresh, so a transport fault on inverter A could zero a +# perfectly healthy inverter B. +# +# ⚠️ THE NUMBER IS DERIVED, NOT CHOSEN. `heartbeat_s`'s schema maximum is 25 s +# (config.yaml) and layer 1's veto is `validate.FIRMWARE_INGEST_VETO_S` (30 s), +# so the budget has to fit in what is left: 25 + 4 < 30. That is the whole +# argument, and test_inverter.py grades the inequality rather than the literal, +# so raising the heartbeat's schema ceiling takes this red instead of taking an +# inverter to zero. It is also below `hass.py`'s own 10 s HTTP timeout on +# purpose - we would rather abandon a write and re-issue it on the next tick +# (one second later, and `last_write` is only updated on an ACCEPTED write, so +# the re-issue happens by itself) than hold the control loop for ten seconds. +WRITE_BUDGET_S = 4.0 # --------------------------------------------------------------------------- # # OPT-04: THE BACKGROUND TARIFF REFRESH, AND WHY IT IS NOT OPTIONAL @@ -122,10 +146,45 @@ class Controller: # the broken shape does not typecheck. self.stop_event = stop self.refused: list[str] | None = None + # ⚠️ FIRST, BECAUSE THE STORE MIGRATION BELOW CAN LOG TO IT. §15's + # action log is how an operator finds out that a latch was re-armed on + # their behalf, and a guard that raises AttributeError on the way to + # saying so is a guard that stops the add-on booting. + self.events: list[str] = [] + + # --- DEV-02: THE ROSTER, BUILT BEFORE ANYTHING THAT READS A RATING --- + # ⚠️ EVERY PER-DEVICE VALUE IN THIS CONTROLLER LIVES ON ONE OF THESE + # OBJECTS AND NOWHERE ELSE. app/inverter.py carries the argument for + # each one, including the two it deliberately does NOT hold - a `Tuning` + # and an integrator, because §6.2's stability property is ONE integrator + # on P1 above the split and a second one regulating the same meter is + # the two-writers failure the arbiter exists to prevent. + # + # ⚠️ AND THE STORE MIGRATION RUNS HERE, BEFORE THE FIRST `Inverter` READS + # ITS LATCH. `Inverter.__init__` restores SAFETY-02 from + # `unit//sign_tripped`; on an install written before DEV-02 that + # key does not exist yet and the latch is under the flat one. Migrate + # first or every existing tripped install comes up ARMED, into the + # fault, with a green banner - which is the exact failure the latch was + # persisted to prevent, rebuilt by a key rename. + sign_cfg = SignWatchdogConfig( + min_w=float(opts.get("sign_disagree_w", 300)), + hold_s=float(opts.get("sign_disagree_s", 180)), + ) + self.store_migration = migrate_unit_keys(store, UNIT_A) + self.inverters = build_roster(opts, store, sign_cfg) self.tuning = Tuning( gain=float(opts.get("gain", 0.6)), - max_w=float(opts.get("max_w", 2000)), + # ⚠️ THE SITE BOUND, SUMMED FROM THE ROSTER, NOT `opts["max_w"]`. + # This is the integrator bound and the output rail of the SITE loop, + # whose output is then SPLIT across the inverters - so the number it + # may reach is what the site can deliver, which is the sum of what + # each unit may be written. At the shipped roster of one that is + # `max_w` exactly, byte for byte; at N>=2 a per-unit `max_w` still + # binds each unit's own share through its own `supervised` claim, so + # nothing here can over-command a device. + max_w=self.site_max_w, slew_w=float(opts.get("slew_w", 1000)), deadband_w=float(opts.get("deadband_w", 15)), target_grid_w=float(opts.get("target_grid_w", 0)), @@ -149,28 +208,32 @@ class Controller: # nowhere else. self.maint = Maintenance(MaintConfig.from_options(opts), store) - # SAFETY-02. One instance, one actuator, one cadence - see watchdog.py. + # SAFETY-02. ONE INSTANCE PER ACTUATOR - which is what watchdog.py's own + # docstring has always said and what this Controller did not do. Each + # `Inverter` builds its own, restored from ITS OWN namespaced store keys, + # so a trip caused by B latches B and leaves A commanding. # # ⚠️ RESTORED FROM THE STORE, and that is the difference between a latch # and a pause. A trip held only in memory is cleared by the one action an # operator staring at a FAILSAFE banner is most likely to take - restart # the add-on - and the controller then comes back armed, into the fault, - # with nobody involved. Restored here, the banner and the 0 W survive the - # restart and only `auto_toggle` clears them. - self.sign_wd = SignWatchdog( - SignWatchdogConfig( - min_w=float(opts.get("sign_disagree_w", 300)), - hold_s=float(opts.get("sign_disagree_s", 180)), - ), - tripped=bool(store.data.get("sign_tripped", False)), - reason=str(store.data.get("sign_reason", "") or ""), - ) - if self.sign_wd.tripped: - _LOG.error("SAFETY-02 was TRIPPED before this restart and stays " - "tripped: %s. The inverter is held at 0 W. Restarting the " - "add-on does not clear this - start control from the UI " - "once you know why the command opposed the meter.", - self.sign_wd.reason or "(reason not recorded)") + # with nobody involved. + # + # ⚠️ AND THE MIGRATION IS GRADED HERE RATHER THAN TRUSTED. A latch that + # silently failed to move into its namespace is a DISARMED SAFETY GUARD + # on an install whose state file still says `sign_tripped: true`. So the + # pre-DEV-02 flat value is compared against what unit A actually came up + # with, and a disagreement is not a log line - it re-arms the latch by + # hand and shouts, because the fail-safe direction for a latch is ON. + self._grade_store_migration(store) + for inv in self.inverters: + if inv.sign_wd.tripped: + _LOG.error("SAFETY-02 was TRIPPED on %s before this restart and " + "stays tripped: %s. That inverter is held at 0 W. " + "Restarting the add-on does not clear this - start " + "control from the UI once you know why the command " + "opposed the meter.", inv.name, + inv.sign_wd.reason or "(reason not recorded)") # SAFETY-18. The frozen-but-fresh meter detector - see freeze.py for why # it is a conjunction and not an `unchanged_s` threshold, and for the @@ -263,21 +326,20 @@ class Controller: self.p1_source = None # --- ARB-03: which actuators this site has --------------------------- - # ⚠️ ONE INVERTER, AND THE ROSTER IS WRITTEN DOWN RATHER THAN ASSUMED. + # ⚠️ THE ROSTER IS `self.inverters` AND `units` IS ITS NAMES, DERIVED. # Every claim below is filed on an actuator, and `arbiter.actuator_for()` # needs to know how many there are: at N=1 §6.2's identity holds - the # fabricated grid reading and the battery setpoint are the same number - # so the grid connection and the only inverter ARE one actuator and every - # claim routes to the site. The day this tuple gains a second entry, the - # per-inverter claims stop being site claims on their own, which is - # exactly the seam ALLOC-01 occupies. + # claim routes to the site. With a second entry the per-inverter claims + # stop being site claims on their own, which is the seam ALLOC-01 + # occupies and this card fills. # - # ⚠️ INVERTER B DOES NOT EXIST YET. This is not a configuration option - # and must not be turned into one here; ALLOC-01 owns the roster, the - # weights and the split. Naming the unit is all ARB-03 needs, and it is - # what makes a mis-routed claim something a check can see. - self.unit = UNIT_A - self.units = (UNIT_A,) + # ⚠️ IT IS A PROPERTY, NOT A FIELD, AND THAT IS THE POINT OF THE CARD. + # `self.units = (...)` was a tuple somebody could set beside a roster of + # one Inverter object, i.e. a site that claims two actuators and has one + # writer. Deriving it means the roster and the actuator list cannot + # disagree - to add an inverter you add an Inverter. # live state self.auto = bool(store.data.get("auto", opts.get("auto_start", False))) @@ -303,7 +365,10 @@ class Controller: # edge-triggered. Starts as the empty tuple - nothing has been dropped # yet and nothing has been said about it. self._dropped_last = () - self.grid = self.soc = self.batt = None + # ⚠️ THE SITE MEASUREMENT, AND THE ONLY ONE LEFT ON THIS OBJECT. `soc` + # and `batt` were per-device scalars and now live on each `Inverter`; + # `grid` is P1, one meter, one connection, and belongs here. + self.grid = None self.peak_fc = None self.cheap = False # TARIFF-03. One line of prose for the status page saying WHICH tariff @@ -320,17 +385,14 @@ class Controller: self.tariff_at = None self.arb = None self._arb_action = None - self.last_write = None - self.last_write_at = None - self.last_write_ok = None self.inputs_bad_since = None - self.dev_min = self.dev_max = None - # SAFETY-03. What the last write's clamp did, and what the loop's own - # bounds did last cycle. `loop_clamped` is a (engaged, note) pair so the - # alarm can say WHICH bound bit without re-deriving it from a string. - self.clamp_last: ClampResult | None = None + # SAFETY-03. What the loop's OWN bounds did last cycle - a + # (engaged, note) pair so the alarm can say WHICH bound bit without + # re-deriving it from a string. ⚠️ THIS ONE STAYS SITE-LEVEL, and it is + # the counterpart to `clamp_last` moving onto the Inverter: the integrator + # that wound is the SITE integrator, of which there is exactly one, while + # the value a clamp bit is a value written to ONE device. self.loop_clamped: tuple[bool, str] = (False, "") - self.events: list[str] = [] self.stopping = False # -- helpers ------------------------------------------------------------- @@ -339,9 +401,298 @@ class Controller: self.events.insert(0, f"{stamp} {msg}") del self.events[40:] + def _grade_store_migration(self, store) -> None: + """DEV-02. Did SAFETY-02's latch actually survive the key rename? + + ⚠️ THIS IS THE HALF THE CARD CALLS RISKY, AND IT IS GRADED RATHER THAN + PERFORMED. Namespacing `sign_tripped` is right - flat, a trip on B + latches A - but a latch that silently fails to move is a DISARMED SAFETY + GUARD ON AN INSTALL THAT BELIEVES IT IS PROTECTED: the state file still + reads `sign_tripped: true`, the banner is green, and the controller + resumes commanding into the fault that tripped it. That is strictly + worse than never having persisted the latch, because now there is + evidence that says it is fine. + + So the pre-DEV-02 flat value is compared against what unit A ACTUALLY + came up with, and a disagreement is not reported and survived - the + latch is re-armed by hand, from the flat value, and the store is written + so the next start does not need this branch. The fail-safe direction for + a latch is ON: a spurious trip costs a 0 W hold an operator clears with + one button, and a missing one costs whatever SAFETY-02 was watching for. + """ + rep = self.store_migration + # ⚠️ ONLY ON THE START THAT ACTUALLY MIGRATES, AND THAT IS NOT A + # SHORTCUT. Once the store is marked migrated the NAMESPACED value is + # the live latch and the flat key is a fossil that nothing writes any + # more - so an operator who clears the failsafe leaves `sign_tripped: + # true` sitting in the file forever. A guard that went on comparing + # against it would re-arm the failsafe on every restart, for the rest of + # the install's life, on a fault that was dealt with weeks ago. That is + # rule 1 of `migrate_unit_keys()` read from the other end. + if not rep.ran: + return + if not rep.ok: + _LOG.error("SAFETY-02: the per-inverter store migration did not " + "verify (%s). The latch is being re-derived from the " + "legacy keys.", rep.explain()) + data = getattr(store, "data", {}) + legacy_tripped = bool(data.get("sign_tripped", False)) + if not legacy_tripped or not self.inverters: + return + first = self.inverters[0] + if first.sign_wd.tripped: + return + # The one state this method exists for. + reason = str(data.get("sign_reason", "") or "") + first.sign_wd.tripped = True + first.sign_wd.reason = reason or ( + "SIGN_DISAGREEMENT: latched before this version; the reason was " + "not carried across the per-inverter store migration") + first.store_set("sign_reason", first.sign_wd.reason) + first.store_set("sign_tripped", True) + _LOG.error( + "SAFETY-02 MIGRATION GUARD: this store carries a LATCHED " + "sign-disagreement failsafe under the pre-DEV-02 flat key, and %s " + "came up UNLATCHED. That is a disarmed safety guard on an install " + "whose state file says it is protected, so the latch has been " + "re-armed from the legacy value: %s. %s is held at 0 W until " + "control is started from the UI.", + first.name, first.sign_wd.reason, first.name) + self.log_event("FAILSAFE re-armed from the pre-DEV-02 store keys - " + + first.sign_wd.reason) + + # -- the roster, and the N=1 views of it --------------------------------- + @property + def units(self) -> tuple: + """The actuator names, DERIVED from the roster. Never assignable. + + See the note in `__init__`: a settable `units` is a site that can claim + two actuators while holding one writer, which is a half-driven site with + nothing to say so. + """ + return tuple(inv.name for inv in self.inverters) + + @property + def maint_unit(self): + """The pack the ONE maintenance state machine is recalibrating. + + ⚠️ NAMED RATHER THAN ASSUMED. This add-on holds exactly one + `Maintenance`, whose history lives under the store's un-namespaced + `phase` / `last_completed` keys, so the pack it drives is the first in + the roster. Two packs recalibrating on two schedules needs a second FSM + and a namespaced history of its own; what THIS card fixes is only that + the claim is filed on ONE actuator instead of on whichever one happened + to be asked. Read store.LEGACY_UNIT_KEYS on why those keys did not move. + """ + return self.inverters[0] + + def inverter(self, name: str): + """The roster entry called `name`, or None.""" + for inv in self.inverters: + if inv.name == name: + return inv + return None + + def _can_write(self, name: str) -> bool: + """Is there a roster entry called `name` WITH a setpoint entity?""" + inv = self.inverter(name) + return inv is not None and inv.has_writer() + + def _only(self): + """The sole inverter, or an error naming why the caller cannot have one. + + ⚠️ THIS IS WHAT MAKES THE N=1 VIEWS BELOW SAFE. §6.2's identity - one + inverter means the site and the unit are the same thing - is true at N=1 + and false at N>=2, so a view built on it must not quietly answer for + "the first one" once the roster grows. Every one of them refuses instead, + loudly, naming the per-instance attribute to use. + """ + if len(self.inverters) != 1: + raise AttributeError( + f"this site has {len(self.inverters)} inverters, so there is no " + f"single per-device value to read off the Controller. §6.2's " + f"identity holds only at N=1. Read it off the roster entry: " + f"`for inv in controller.inverters: inv.`") + return self.inverters[0] + + # ⚠️ PROPERTIES, NOT FIELDS, AND EVERY ONE OF THEM IS A COMPATIBILITY VIEW. + # These are the scalars this card removed. They survive as read/write views + # of the SOLE inverter so that an N=1 install, and every replay in this + # repository that constructs one, is byte-identical - which is AC 8. They + # are properties rather than attributes for two reasons that are the card: + # + # * an assignment cannot re-create the scalar. `self.soc = 4` now writes + # inverter A's own SoC; there is no second copy to drift. + # * they REFUSE at N>=2 rather than answering for the first unit, so a + # consumer that has not been made per-instance fails on the line that + # reads it instead of grading inverter B against inverter A's number. + # That is the whole defect, and this is the tripwire on it. + # + # ⚠️ NOTHING INSIDE THIS FILE MAY READ THEM. main.py's own paths iterate the + # roster; test_inverter.py's grader scans this module's AST and goes red on + # a `self.` inside any method of `Controller`. A view that main.py used + # would be the scalar back with a longer name. + @property + def unit(self) -> str: + return self._only().name + + @property + def soc(self): + return self._only().soc + + @soc.setter + def soc(self, value): + self._only().soc = value + + @property + def batt(self): + return self._only().batt + + @batt.setter + def batt(self, value): + self._only().batt = value + + @property + def dev_min(self): + return self._only().dev_min + + @dev_min.setter + def dev_min(self, value): + self._only().dev_min = value + + @property + def dev_max(self): + return self._only().dev_max + + @dev_max.setter + def dev_max(self, value): + self._only().dev_max = value + + @property + def last_write(self): + return self._only().last_write + + @last_write.setter + def last_write(self, value): + self._only().last_write = value + + @property + def last_write_at(self): + return self._only().last_write_at + + @property + def last_write_ok(self): + return self._only().last_write_ok + + @property + def clamp_last(self): + return self._only().clamp_last + + @property + def sign_wd(self): + return self._only().sign_wd + + # -- the site aggregates, each one named for the question it answers ------ + # ⚠️ THESE ARE NOT "THE SCALARS WITH A SUM IN FRONT". Each one picks a + # DIFFERENT reduction, because the questions are different, and getting the + # reduction wrong is how a pack gets driven below its own floor. The + # reduction is written at the definition, with its reason, rather than + # inline at four call sites. + + @property + def site_max_w(self) -> float: + """The supervised bound on the SITE target: what the roster can deliver. + + SUM. Each unit's own `supervised` claim still binds its own share, so + this cannot over-command a device; what it must not do is bound the site + loop at ONE unit's rating, which would waste the second inverter + entirely. At N=1 it is `max_w`, exactly. + """ + return float(sum(inv.max_w for inv in self.inverters)) + + @property + def site_rating_w(self) -> float: + """The largest magnitude the SITE can actually be commanded, in W. + + SUM of each unit's own `min(max_w, |dev_min|, |dev_max|)`. This is the + peak guard's `achievable_w` and the freeze detector's rail, and summing + the PER-UNIT conjunction rather than conjoining the sums is what makes + inverter B's rating bind inverter B (AC 4). A unit whose device range has + not been read yet contributes its `max_w`, which is what the shipped + single-inverter path already did with (None, None). + """ + return float(sum(inv.rating_w() for inv in self.inverters)) + + def site_dev_bound(self, which: str): + """The roster's summed device floor / ceiling, or None if unknown. + + `freeze.looks_frozen()` and `rail_unreachable()` take a device range and + SKIP a bound they were handed None for - "ESPHome is not up yet", not + "the device can do nothing". So a bound is summed only when EVERY unit + has answered; one silent unit makes the site bound unknown rather than + making it smaller, which is the same direction the shipped code took. + """ + vals = [getattr(inv, which) for inv in self.inverters] + if not vals or any(v is None for v in vals): + return None + return float(sum(float(v) for v in vals)) + + @property + def site_batt_w(self): + """SUM. The battery power the SITE loop is regulating against. + + ⚠️ THIS IS THE FINDING ALLOC-01'S REVIEW HANDED THIS CARD, AND IT GOES + THE OPPOSITE WAY TO EVERYTHING ELSE HERE. `compute()`'s anti-windup test + is `abs(prev_w - actual_w) > saturation_w`, where `prev_w` is the SITE + target. Fed ONE inverter's power at a roster of two it compares a site + number against a unit number, trips the saturation freeze permanently and + leaves a standing grid error - measured on admin/goodwe's two-master rig + as up to 510 W of split-dependence at gain 0.2 and 250 W at gain 0.4, + with the allocator's shares summing to S in every one of those runs. The + counterfactual is the proof: with this sum every split is bit-identical + at every gain. See test_alloc.py part 8. + """ + vals = [inv.batt for inv in self.inverters] + if not vals or any(v is None for v in vals): + return None + return float(sum(float(v) for v in vals)) + + @property + def pack_floor_soc(self): + """MINIMUM. The SoC that a discharge PERMISSION must be graded against. + + SAFETY-24's floor asks "may this site spend the pack for money", and the + honest answer at N>=2 is the EMPTIEST pack: an average lets it be driven + below its own floor, which is the failure the option exists to prevent. + None if any unit did not answer - `peakguard.decide()` treats None as + floor-limited, which costs a shave rather than a pack. + + ⚠️ AND IT IS NOT THE RIGHT REDUCTION FOR A CHARGE CEILING, which is the + FULLEST pack. Nothing in the add-on files a per-pack charge ceiling yet + (arbitrage's `soc_ceiling` is the only consumer and it takes one number); + that is CFG-04/SPIKE-04's per-pack bound, and it is named here so the + next reader knows this reduction was chosen rather than assumed. + """ + vals = [inv.soc for inv in self.inverters] + if not vals or any(v is None for v in vals): + return None + return float(min(float(v) for v in vals)) + @property def inputs_ok(self) -> bool: - return None not in (self.grid, self.soc, self.batt) + """The site meter, and EVERY pack's own two readings. + + ⚠️ EVERY pack, not the live ones: a unit whose SoC did not answer is a + unit nothing can grade the maintenance FSM or the pack floor against, and + §11.1 rule 1 for a missing input is command zero and keep commanding. + Dropping it from the SPLIT instead would hand its share to a sibling + while the site went on regulating - see `Inverter.is_live()` on why + liveness is about the WRITE path and this is not. + """ + if self.grid is None or not self.inverters: + return False + return all(inv.soc is not None and inv.batt is not None + for inv in self.inverters) # -- io ------------------------------------------------------------------ async def read_inputs(self) -> None: @@ -358,9 +709,14 @@ class Controller: else: self.grid = await self.hass.number(o.get("meter_entity", ""), bool(o.get("meter_invert"))) - self.soc = await self.hass.number(o.get("soc_entity", "")) - self.batt = await self.hass.number(o.get("batt_entity", ""), - bool(o.get("batt_invert"))) + # --- DEV-02: EACH INVERTER'S OWN TWO READINGS, FROM ITS OWN ENTITIES -- + # ⚠️ CONCURRENTLY ACROSS UNITS, SEQUENTIALLY WITHIN ONE. Read serially, + # a unit whose Home Assistant entity is slow delays every unit after it + # on the path that feeds the ESP32 heartbeat - the same defect AC 6 + # names for the WRITE path, one function up. Within a unit the order is + # unchanged (SoC, then battery power), so at the shipped roster of one + # this is the same two calls in the same order. + await asyncio.gather(*(self._read_unit(inv) for inv in self.inverters)) if o.get("peak_forecast_entity"): raw = await self.hass.number(o["peak_forecast_entity"]) # Accept kW or W - nobody's quarter-hour forecast is 50 W. @@ -406,8 +762,54 @@ class Controller: elif self.inputs_bad_since is None: self.inputs_bad_since = datetime.now(timezone.utc) - async def write_setpoint(self, value: float, *, force: bool = False) -> None: - entity = self.o.get("setpoint_entity", "") + async def _read_unit(self, inv) -> None: + """One inverter's own SoC and battery power, from ITS OWN entities.""" + inv.soc = await self.hass.number(inv.soc_entity) + inv.batt = await self.hass.number(inv.batt_entity, inv.batt_invert) + + async def write_setpoint(self, value: float, *, force: bool = False, + unit=None) -> None: + """Put `value` on ONE inverter, or - with `unit=None` - on all of them. + + ⚠️ `unit=None` IS FOR A NUMBER THAT IS NOT A SHARE. The stop button, + `shutdown()` and `maint_abort()` all command 0 W to the whole site, and + 0 W is the one value that means the same thing to every inverter. A + NON-ZERO value broadcast to a roster of two IS the ARCH-01 fault - each + unit supplies it in full and the loop gain at the meter becomes N times + the per-unit gain - so it is refused below rather than trusted to the + caller. The cycle writes SHARES, one unit at a time, through the + allocator. + + ⚠️ AND THE WRITES ARE CONCURRENT, EACH UNDER ITS OWN BUDGET. See + WRITE_BUDGET_S: one hung `hass.call()` on a serialised path used to eat + a sibling's layer-1 refresh window. + """ + if unit is None: + targets = [i for i in self.inverters] + if len(targets) > 1 and is_commandable(value) and float(value) != 0.0: + # Loud, and it commands zero rather than the value asked for - + # fail toward inaction, exactly as every other refusal here. + self.log_event( + f"refusing to broadcast {float(value):+.0f} W to " + f"{len(targets)} inverters - that is the ARCH-01 fault; " + f"commanding 0 W") + _LOG.error("write_setpoint() was asked to put %+.0f W on every " + "one of %d inverters. A site target handed to each " + "unit is supplied by EACH of them: the gain at the " + "meter becomes N times the per-unit gain. Only 0 W " + "may be written site-wide; shares come from the " + "allocator. Commanding 0 W.", float(value), + len(targets)) + value = 0.0 + else: + targets = [unit] + if not targets: + return + await asyncio.gather(*(self._write_unit(inv, value, force=force) + for inv in targets)) + + async def _write_unit(self, inv, value: float, *, force: bool = False) -> None: + entity = inv.setpoint_entity if not entity: return @@ -430,30 +832,58 @@ class Controller: "0 W and continuing to command. This is a bug upstream, " "not a limit.", value) value = 0.0 - result = clamp_to_rating(value, float(self.o.get("max_w", 2000)), - self.dev_min, self.dev_max) - self.clamp_last = result + # ⚠️ THIS UNIT'S OWN THREE NUMBERS. `inv.max_w` is the supervised cap on + # what may be written to THIS setpoint entity and `inv.dev_min/dev_max` + # are what THIS device reported about itself - which is the whole of "the + # clamp for B uses B's numbers". Read off `self.o` these were one + # inverter's rating applied to every inverter. + result = clamp_to_rating(value, inv.max_w, inv.dev_min, inv.dev_max) + inv.clamp_last = result if result.engaged: # ⚠️ EVERY engagement, never deduplicated: this is the action log # (§15), and it is a different surface from the notification, which # IS deduplicated. The count is what says how hard the loop pushed; # the notification only says that it did. - self.log_event(result.describe()) + self.log_event(f"{inv.name}: {result.describe()}") value = result.applied_w - changed = (self.last_write is None or value != self.last_write) + changed = (inv.last_write is None or value != inv.last_write) if not (changed or force): return - ok = await self.hass.set_number(entity, value) - self.last_write_ok = ok + # ⚠️ BOUNDED, AND THE BUDGET IS AC 6. Without it one inverter's hung + # transport holds this coroutine - and therefore the whole control loop, + # and therefore every OTHER inverter's heartbeat - for as long as the + # HTTP stack allows. A write abandoned here is re-issued on the next + # tick by itself: `last_write` is updated only on an ACCEPTED write, so + # `changed` is still true. + try: + ok = await asyncio.wait_for(self.hass.set_number(entity, value), + timeout=WRITE_BUDGET_S) + except (asyncio.TimeoutError, TimeoutError): + ok = False + _LOG.error("the write of %+.0f W to %s (%s) did not complete in " + "%.0f s and was abandoned. It will be re-issued on the " + "next tick; no other inverter waited for it.", + value, inv.name, entity, WRITE_BUDGET_S) + self.log_event(f"{inv.name}: write timed out after " + f"{WRITE_BUDGET_S:.0f} s - re-issuing next tick") + now = datetime.now(timezone.utc) + inv.note_write(bool(ok), now) if ok: - self.last_write = value - self.last_write_at = datetime.now(timezone.utc) + inv.last_write = value + inv.last_write_at = now # -- the cycle ----------------------------------------------------------- - def actuator_of(self, source: str) -> str: + def actuator_of(self, source: str, inv=None) -> str: """Which actuator a claim from `source` is filed on this cycle. + `inv` is the roster entry the claim is ABOUT, for a per-inverter source. + Left out it is the first inverter, which is what every per-inverter claim + meant while there was only one. ⚠️ A per-inverter claim built inside the + per-unit loop MUST pass it, or inverter B's rating is filed on inverter A + and the arbiter bounds the wrong share - the whole defect, wearing this + method's name. + ⚠️ THE CLASSIFICATION IS NOT HERE. `arbiter.CLAIM_SCOPE` decides whether a source is about the grid connection or about one pack, and it carries the reasoning for all eight; this method only supplies the roster, which @@ -461,7 +891,7 @@ class Controller: writing `actuator=` at a construction site - bypasses the KeyError that stops a ninth, unclassified source being filed, so do not. """ - return actuator_for(source, self.unit, self.units) + return actuator_for(source, (inv or self.inverters[0]).name, self.units) def live_units(self) -> tuple: """Which inverters may be commanded THIS TICK, for the allocator. @@ -477,16 +907,46 @@ class Controller: between one cycle and the next; an envelope read once at construction over-allocates into a unit that cannot take it. - Today the roster is one inverter and THIS CONTROLLER IS THAT - INVERTER'S ONLY LINK, so "its master is stale" has no expression - separate from "this controller is blind" - and that case never reaches - the allocator with anything to divide, because `safety` files a SITE set - at P_SAFETY and every share resolves to 0 W. So the whole roster is - live, and saying that here is what makes the seam real rather than - notional. DEV-02 gives each inverter its own link and its own freshness, - and replaces the body of this method with them. + ⚠️ AND IT ANSWERS ONE QUESTION: CAN THIS CONTROLLER PUT A NUMBER ON + THIS INVERTER RIGHT NOW. `Inverter.is_live()` carries the two ways it + cannot - no `setpoint_entity`, so there is no writer at all; or every + write REJECTED for a whole `LINK_STALE_S` window, which is layer 1's own + veto period, i.e. the point at which the firmware is about to take the + inverter away from this controller anyway. + + ⚠️ IT IS DELIBERATELY NOT "ITS SENSORS ARE MISSING". A pack whose SoC did + not answer is a different fault with a different remedy - `inputs_ok` + already stops the whole SITE at P_SAFETY for that - and dropping the unit + from the SPLIT instead would hand its share to a sibling while the site + went on regulating, which is the opposite of what a missing measurement + should cause. + + ⚠️ AT A ROSTER OF ONE, ANSWERING FALSE IS A 0 W SITE. The allocator has + nobody left in the envelope set, refuses (REFUSE_EMPTY), and main.py + substitutes the failsafe. That is the honest answer and not a + regression: the only way to reach it is a setpoint entity that has been + REJECTING every write for 30 s, on which the previous code went on + computing setpoints for an inverter it could not reach while layer 1 was + already winding it down. It says so in the action log instead. """ - return self.units + now = datetime.now(timezone.utc) + live = [] + for inv in self.inverters: + ok, why = inv.is_live(now) + if ok: + live.append(inv.name) + else: + # Level-logged at DEBUG only: `Allocation.dropped` is the + # operator-facing surface and cycle() edge-logs it. A line here + # every second would flush the 40-row action log of the context + # that explains it. + _LOG.debug("%s is not commandable this tick: %s", inv.name, why) + return tuple(live) + + def liveness_notes(self) -> list: + """Why each unit is or is not commandable, for the status page.""" + now = datetime.now(timezone.utc) + return [n for n in (inv.link_note(now) for inv in self.inverters) if n] async def cycle(self) -> None: """Gather claims, resolve precedence, write the winner. @@ -496,7 +956,12 @@ class Controller: """ now = datetime.now(timezone.utc) - result = self.maint.tick(now, self.soc) + # ⚠️ THE PACK IT IS MAINTAINING, NOT "the SoC". §12's recalibration + # drains and refills ONE pack and grades ONE coulomb counter, so it is + # graded on that pack's own reading - see `maint_unit`. A minimum across + # packs would abort the cycle on a sibling's state, and an average would + # declare a pack full that is not. + result = self.maint.tick(now, self.maint_unit.soc) for msg in result.events: self.log_event(msg) @@ -513,25 +978,52 @@ class Controller: # the case where nothing has been written yet (and, deliberately, the # case where the last write was REJECTED - the inverter is then still # latched on `last_write`, which is exactly what `last_write` holds). - was_tripped = self.sign_wd.tripped - standing = self.last_write if self.last_write is not None else self.target - if self.sign_wd.evaluate(standing, self.grid, self.following, now) \ - and not was_tripped: - # ⚠️ REASON FIRST, FLAG SECOND, and each `set` writes the file. The - # ordering is the whole atomicity story: a crash between the two - # persists a reason nothing reads, which is harmless, where the other - # order persists a latch with no explanation for the operator who has - # to decide whether to clear it. - self.store.set("sign_reason", self.sign_wd.reason) - self.store.set("sign_tripped", True) - self.log_event("FAILSAFE " + self.sign_wd.reason) - _LOG.error("SAFETY-02 TRIPPED - %s. Commanding 0 W and holding it " - "until control is started again. This survives an add-on " - "restart; restarting will not clear it.", - self.sign_wd.reason) + # ⚠️ ONE WATCHDOG PER INVERTER, EVALUATED ON THAT INVERTER'S OWN + # STANDING COMMAND. The meter is site-wide - there is one P1 - but what + # the watchdog concludes is "THE UNIT I AM DRIVING is pushing power the + # wrong way", and that evidence cannot be pinned on a sibling this + # controller commanded differently. `inv.target` is that unit's share + # from the last resolution, which at N=1 is the site target itself. + standing = {} + for inv in self.inverters: + was_tripped = inv.sign_wd.tripped + stood = inv.last_write if inv.last_write is not None else inv.target + standing[inv.name] = stood + if inv.sign_wd.evaluate(stood, self.grid, self.following, now) \ + and not was_tripped: + # ⚠️ REASON FIRST, FLAG SECOND, and each `set` writes the file. + # The ordering is the whole atomicity story: a crash between the + # two persists a reason nothing reads, which is harmless, where + # the other order persists a latch with no explanation for the + # operator who has to decide whether to clear it. + # + # ⚠️ AND BOTH KEYS ARE NAMESPACED TO THIS UNIT. Flat, a trip + # caused by B latched A across every restart until somebody + # pressed start, and one press cleared both. See store.py. + inv.store_set("sign_reason", inv.sign_wd.reason) + inv.store_set("sign_tripped", True) + self.log_event(f"FAILSAFE {inv.name}: " + inv.sign_wd.reason) + _LOG.error("SAFETY-02 TRIPPED on %s - %s. Commanding 0 W on " + "that inverter and holding it until control is " + "started again. This survives an add-on restart; " + "restarting will not clear it.", + inv.name, inv.sign_wd.reason) + # ⚠️ THE SITE'S OWN STANDING COMMAND, WHICH IS THE SUM OF THE UNITS'. + # SAFETY-07 and SAFETY-18 both grade a SITE quantity - the quarter-hour + # average at the grid connection, and a meter that has stopped moving + # while the site pushes near its rail - so what they need is the total + # this controller currently has standing on the wire, not one unit's + # share of it. At the shipped roster of one the two are the same number. + site_standing = float(sum(standing.values())) stale_after = float(self.o.get("stale_input_s", 15)) - if not self.inputs_ok and not self.sign_wd.tripped: + # ⚠️ `any`, NOT the sole unit's. The hold exists so a single missed poll + # does not become a fault; a LATCHED inverter must still reach the claim + # list below, because its `failsafe` SET is what parks it. One tripped + # unit therefore takes the whole roster past the hold, which is the + # conservative direction: every other unit's claims are re-filed too. + if not self.inputs_ok and not any(i.sign_wd.tripped + for i in self.inverters): bad_for = (now - self.inputs_bad_since).total_seconds() if self.inputs_bad_since else 0.0 if bad_for < stale_after: # A single missed poll is not a fault. Hold, keep the heartbeat @@ -545,18 +1037,24 @@ class Controller: # Defence in depth: write_setpoint() clamps to the device range too. The # duplication is deliberate - one of them is the policy, the other is the # last thing between a bug and the hardware. - if self.dev_min is not None and self.dev_max is not None: - # PER-INVERTER. The numbers are ONE inverter's own reported - # min/max; a second unit has a second rating and there is no such - # thing as a site rating. See arbiter.CLAIM_SCOPE. - claims.append(Claim.limit("device", P_SAFETY, self.dev_min, self.dev_max, "rating", - actuator=self.actuator_of("device"))) - max_w = float(self.o.get("max_w", 2000)) - # PER-INVERTER, for the same reason as `device`: `max_w` is the - # supervised cap on what may be WRITTEN to one setpoint entity (§11.3), - # not a bound on the grid connection. - claims.append(Claim.limit("supervised", P_SAFETY, -max_w, max_w, "max_w", - actuator=self.actuator_of("supervised"))) + # ⚠️ PER-INVERTER, ONE PAIR PER UNIT, FROM THAT UNIT'S OWN NUMBERS. + # This loop is the difference DEV-02 makes to the claim list: the + # numbers are ONE inverter's own reported min/max and ONE inverter's own + # supervised cap, and there is no such thing as a site rating. Before + # this card there was one pair, read from one setpoint entity, filed + # once - so a second inverter's rating was never read and never bound. + # See arbiter.CLAIM_SCOPE. + for inv in self.inverters: + if inv.dev_min is not None and inv.dev_max is not None: + claims.append(Claim.limit("device", P_SAFETY, inv.dev_min, inv.dev_max, "rating", + actuator=self.actuator_of("device", inv))) + # `max_w` is the supervised cap on what may be WRITTEN to one + # setpoint entity (§11.3), not a bound on the grid connection. + claims.append(Claim.limit("supervised", P_SAFETY, -inv.max_w, inv.max_w, "max_w", + actuator=self.actuator_of("supervised", inv))) + # The SITE bound, for the site-level consumers below. It is the roster + # sum: what this site can deliver, not what one of its inverters can. + max_w = self.site_max_w # --- safety stops ---------------------------------------------------- # ⚠️ SAFETY-02 is claimed FIRST and unconditionally while latched. It is @@ -573,10 +1071,11 @@ class Controller: # evidence cannot be pinned on a sibling this controller never commanded, # so a site-wide stop would park an inverter this watchdog never graded. # See arbiter.CLAIM_SCOPE. - if self.sign_wd.tripped: - claims.append(Claim.set("failsafe", P_SAFETY, 0.0, - "sign disagreement", - actuator=self.actuator_of("failsafe"))) + for inv in self.inverters: + if inv.sign_wd.tripped: + claims.append(Claim.set("failsafe", P_SAFETY, 0.0, + "sign disagreement", + actuator=self.actuator_of("failsafe", inv))) # SITE, both of them, and the two reasons are why. "stopped" is the # operator switching control off for the whole site; "inputs missing" is @@ -635,21 +1134,33 @@ class Controller: # is not measured on that path, and the honest report is NOT COVERED - # the same answer, for the same reason, that the status page already # gives for SAFETY-18 on a transport that cannot see `unchanged_s`. - dev_top = self.dev_max if self.dev_max is not None else max_w self.peak_guard = None if not self.p1_enabled else peakguard.decide( self.peak.project(self.p1.averager, now), - batt_w=self.batt, + # ⚠️ THE SITE SUM. The guard sizes a floor from what the battery is + # ALREADY removing from the meter, and the meter sees every + # inverter. One unit's power here would size the floor from a + # fraction of the discharge already happening. + batt_w=self.site_batt_w, cap_w=float(self.o.get("peak_cap_w", 3500)), reading=self.peak.peak(now), - achievable_w=min(max_w, abs(float(dev_top))), - # ⚠️ SAFETY-24. `self.soc` IS THE MEASURED READING AND MAY BE None, + # ⚠️ SUMMED FROM THE PER-UNIT CONJUNCTION, WHICH IS AC 4. Each + # unit contributes `min(its max_w, |its own device bounds|)`, so + # inverter B's rating bounds inverter B. Conjoining the SUMS instead + # would let one unit's device range cap the whole site. + achievable_w=self.site_rating_w, + # ⚠️ SAFETY-24. `pack_floor_soc` IS A MEASURED READING AND MAY BE None, # and it is handed over as None rather than defaulted here. An early # draft of the stage-5 loop read SoC as `| float(100)` and thereby # invented a full battery (NOTES.md 4.4b); a full battery is # precisely the value that disarms this floor. decide() treats None # as floor-limited, which is the direction that costs a shave rather # than the pack. - soc=self.soc, + # ⚠️ THE EMPTIEST PACK, not an average - see `pack_floor_soc`. A + # floor that grades a discharge permission against the mean lets the + # emptier pack be driven below the level this option exists to + # protect. None stays None: `decide()` treats it as floor-limited, + # which costs a shave rather than a pack. + soc=self.pack_floor_soc, soc_floor=float(self.o.get( "peak_soc_floor", peakguard.PEAK_OPTION_DEFAULTS["peak_soc_floor"])), @@ -660,8 +1171,9 @@ class Controller: # grading it against a rail derived from bare `max_w` compares a # device-clipped number to a ceiling the device cannot reach. frozen=looks_frozen(getattr(self.p1_source, "unchanged_s", None), - standing, max_w, self.freeze_wd.cfg, - dev_min=self.dev_min, dev_max=self.dev_max), + site_standing, max_w, self.freeze_wd.cfg, + dev_min=self.site_dev_bound("dev_min"), + dev_max=self.site_dev_bound("dev_max")), ) # SITE, and peakguard.claim() says so on the object it builds. The # quantity is the 15-minute average OFFTAKE AT THE GRID CONNECTION, @@ -727,7 +1239,13 @@ class Controller: self.arb = arbitrage.decide( self.fresh_tariff_plan(now), now, peak_guard_active=arbitrage.guard_state(self.peak_guard), - soc=self.soc, + # The emptiest pack again, and for the sell side that is the + # right reduction. ⚠️ THE BUY SIDE WANTS THE FULLEST, and there + # is one `soc` argument - `pack_floor_soc` names that residual + # rather than hiding it; a per-pack charge ceiling is CFG-04's, + # and at the shipped roster of one the two reductions are the + # same number. + soc=self.pack_floor_soc, min_spread_eur_kwh=arbitrage.threshold_eur_kwh(self.o), uncertainty=frozenset({Uncertainty.NO_WEATHER}), charge_w=max_w, discharge_w=max_w, @@ -752,9 +1270,17 @@ class Controller: # drain THIS pack to its floor, charge THIS pack full, recalibrate # THIS pack's coulomb counter. Two packs recalibrate on two # schedules. See arbiter.CLAIM_SCOPE. + # ⚠️ FILED ON THE UNIT BEING MAINTAINED, WHICH IS THE FIRST ONE. + # §12's monthly recalibration is a PACK procedure and this add-on + # holds exactly ONE `Maintenance` state machine, so the pack it is + # maintaining is `self.maint_unit`. Two packs recalibrating on two + # schedules is a second FSM with its own namespaced history, which + # is a card of its own - what this card must not do is file one + # pack's procedure on every actuator. claims.append(Claim.set("maintenance", P_MAINTENANCE, float(result.setpoint_w or 0.0), result.phase, - actuator=self.actuator_of("maintenance"))) + actuator=self.actuator_of("maintenance", + self.maint_unit))) self.sat_count = 0 elif result.charge_only: # Money outranks the maintenance schedule: while the quarter-hour @@ -819,7 +1345,8 @@ class Controller: # PER-INVERTER, like the SET above and for the same reason - # the shaping is about the pack being charged. claims.append(Claim.limit("maintenance", P_MAINT_SHAPE, hi=hi, reason=reason, - actuator=self.actuator_of("maintenance"))) + actuator=self.actuator_of("maintenance", + self.maint_unit))) # --- the grid-following loop ----------------------------------------- if self.auto and self.inputs_ok: @@ -839,16 +1366,31 @@ class Controller: # stale period as power. if self.loop_w is None or self.target != self.loop_w: self.i_w = self.target + # ⚠️ `actual_w` IS THE SITE SUM AND THAT IS DEV-02's OTHER HALF. + # The anti-windup test inside compute() is + # `abs(prev_w - actual_w) > saturation_w`, and `prev_w` is the SITE + # target. Handed ONE inverter's power at a roster of two it compares + # a site number against a unit number, permanently trips the + # saturation freeze and leaves a standing grid error - measured on + # admin/goodwe's two-master rig at up to 510 W of split-dependence + # (gain 0.2; 250 W at 0.4, 50 W at 0.6) while the allocator's shares + # summed to S in every run. With the sum, every split is + # bit-identical at every gain the schema accepts. That is why the + # per-unit readings became per-instance and THIS one became site + # level: the two halves of §6.2's identity break in opposite + # directions. See `site_batt_w` and test_alloc.py part 8. + site_batt = self.site_batt_w decision = compute( prev_w=self.target, grid_w=self.grid, - actual_w=self.batt, + actual_w=site_batt, tuning=self.tuning, sat_count=self.sat_count, i_w=self.i_w, ) if decision.frozen and self.sat_count < self.tuning.saturation_cycles: - self.log_event(f"saturation freeze ({self.target:.0f} W vs {self.batt:.0f} W)") + self.log_event(f"saturation freeze ({self.target:.0f} W vs " + f"{site_batt:.0f} W)") self.sat_count = decision.sat_count self.i_w = decision.i_w self.loop_w = decision.target_w @@ -916,35 +1458,46 @@ class Controller: allocation=resolution.allocation, winner_priority=resolution.winner_priority) # 2. THE ALLOCATOR SUCCEEDED AND THIS CONTROLLER CANNOT DELIVER IT. - # ⚠️ THIS GUARD IS THE PRICE OF MAKING allocate() WORK, and without - # it ALLOC-01 would be a REGRESSION rather than a fix. There is ONE - # `setpoint_entity` and ONE inverter-power sensor in this add-on, so - # this controller can put a number on exactly one inverter. Give it a - # roster of two and a valid split, and writing `self.target` - the - # SITE target - to its single entity would hand one inverter the - # whole site's number while a second sits uncommanded: not the - # broadcast fault, but the same shape of over-command, and the - # allocator's own arithmetic says the share is half that. Commanding - # a partial site is a worse instruction than commanding nothing, and - # "nothing" here means 0 W and says so. + # ⚠️ RE-AIMED BY DEV-02, NOT DELETED, AND THE DIFFERENCE MATTERS. + # Under ALLOC-01 this fired whenever the split named any inverter but + # the single one this controller could write, because there was one + # `setpoint_entity`. There is a writer per inverter now, so that + # trigger is gone - and the INSTINCT is not, because a roster entry + # can still have no writer: an inverter configured with no + # `setpoint_entity` of its own is an inverter this controller cannot + # put a number on. Writing the site target to the ones it CAN reach + # would over-command them by exactly the missing unit's share - + # not the broadcast fault, but the same shape of over-command. + # Commanding a partial site is a worse instruction than commanding + # nothing, and "nothing" here means 0 W and says so. # - # ⚠️ AT THE SHIPPED ROSTER OF ONE THIS IS BYTE-IDENTICAL TO BEFORE. - # `shares` is `{inverter-a: target_w}` - §6.2's N=1 identity, the - # pass-through - so the set below is empty and nothing happens. - # DEV-02 gives this controller a writer per inverter and deletes the - # branch; until it does, the branch is what stops ALLOC-01 shipping a - # half-driven site. - elif set(resolution.shares) - {self.unit}: - undeliverable = ", ".join(sorted(set(resolution.shares) - {self.unit})) + # ⚠️ IT IS NOT THE SAME EVENT AS A DEAD LINK, WHICH IS WHY IT IS NOT + # FOLDED INTO `live_units()`. A unit whose writes are being rejected + # is DROPPED and its share is carried by the survivors - a real, and + # announced, change in what the site can deliver. A unit with no + # writer configured at all is a CONFIGURATION that cannot be + # delivered, and the site stops rather than silently running at part + # strength on a roster somebody wrote down wrong. + elif {u for u in resolution.shares if not self._can_write(u)}: + undeliverable = ", ".join( + sorted(u for u in resolution.shares if not self._can_write(u))) self.log_event( f"ARBITER: the site target was allocated across " - f"{len(resolution.shares)} inverters and this controller drives " - f"only {self.unit} - no writer for {undeliverable}, refusing to " - f"command a partial site (DEV-02)") + f"{len(resolution.shares)} inverters and this controller has no " + f"setpoint entity for {undeliverable} - refusing to command a " + f"partial site") + # ⚠️ `shares={}`, NOT THE SHARES IT REFUSED TO DELIVER, AND SINCE + # DEV-02 THAT IS LOAD-BEARING RATHER THAN TIDY. The write loop below + # commands each inverter `shares.get(name, 0.0)`, so carrying the + # split through a refusal would write the very numbers this branch + # just declined - and, worse, would write NOTHING at all when they + # equal what is already standing, because an unchanged value is not + # re-sent. The record the operator needs survives on + # `self.allocation`, which is assigned above and untouched here. resolution = Resolution(0.0, "failsafe", None, False, resolution.considered, unit_bounds=resolution.unit_bounds, - shares=resolution.shares, allocated=True, + shares={}, allocated=True, scope_errors=resolution.scope_errors, allocation=resolution.allocation, winner_priority=resolution.winner_priority) @@ -990,7 +1543,29 @@ class Controller: self.following = loop_is_following(resolution, self.loop_w) self.target = resolution.target_w self.reason = resolution.explain() - await self.write_setpoint(self.target) + # --- DEV-02: EACH INVERTER IS TOLD ITS OWN SHARE --------------------- + # ⚠️ ITS OWN SHARE, NEVER THE SITE TARGET. `resolution.shares` is the + # allocator's answer and `sum(shares) + unmet_w == round(target_w)` is + # an integer identity, so handing each unit its entry is what makes the + # gain at the meter a CONVEX COMBINATION of the per-unit gains. Handing + # each unit `self.target` is the ARCH-01 fault: each supplies it in + # full, the gain becomes N times the per-unit gain, and on the + # two-master rig that is 4000 W peak-to-peak forever at gain 1.0 with + # not one guard firing. + # + # ⚠️ AT N=1 THIS IS BYTE-IDENTICAL. The allocator's pass-through returns + # `{inverter-a: target_w}` as an unrounded float, so the number written + # is `self.target` exactly, as it always was. + # + # ⚠️ AND A UNIT NOT IN `shares` IS COMMANDED 0 W, not left standing. + # That is the dropped-link case: the inverter holds its last command + # FOREVER, so "not in the split" must mean zero on the wire, not + # silence. The write is the same clamped, budgeted path as every other. + writes = [] + for inv in self.inverters: + inv.target = float(resolution.shares.get(inv.name, 0.0)) + writes.append(self.write_setpoint(inv.target, unit=inv)) + await asyncio.gather(*writes) # --- SAFETY-03, and its boundary is the WRITE ------------------------ # ⚠️ NOT THE SAME BOUNDARY AS SAFETY-18's BELOW, THOUGH THE CALLS ABUT. @@ -1067,13 +1642,23 @@ class Controller: loop keeps running. Turning this into a trip means coming here and changing that deliberately, not discovering it in the field. """ - engaged_write = bool(self.clamp_last and self.clamp_last.engaged) + # ⚠️ THE EVIDENCE IS PER UNIT AND THE ALARM IS NOT. `clamp_last` moved + # onto the Inverter because a clamp on B says nothing about A; the + # ClampWatchdog stayed one per controller because clamp.py's own + # docstring says so - it is an anti-flood counter over an EVENT BUS, not + # a property of a device, and one notification per inverter per cycle is + # the flood it exists to remove. So: any unit that clamped is an + # engagement, and the reason NAMES the unit. + clamped_units = [inv for inv in self.inverters + if inv.clamp_last is not None and inv.clamp_last.engaged] + engaged_write = bool(clamped_units) engaged_loop, loop_note = self.loop_clamped engaged = engaged_write or engaged_loop if engaged: - where = (self.clamp_last.describe() if engaged_write + where = (", ".join(f"{inv.name}: {inv.clamp_last.describe()}" + for inv in clamped_units) if engaged_write else f"{CLAMP_REASON}: {loop_note}, above the " - f"{float(self.o.get('max_w', 2000)):.0f} W rating, and " + f"{self.site_max_w:.0f} W rating, and " f"was clamped to {self.target:+.0f} W") reason = (f"{where}. This is a BUG SIGNAL, not a limit being used: " f"something upstream computed a setpoint the actuator " @@ -1285,11 +1870,15 @@ class Controller: False, which is the honest answer - see the transport note in freeze.py. """ unchanged = getattr(self.p1_source, "unchanged_s", None) + # SITE, all three inputs: one meter, the SITE target the arbiter + # resolved, and the rail the SITE could stand at - which is the summed + # per-unit conjunction, so a small inverter B lowers the site rail rather + # than being graded against inverter A's. fired = self.freeze_wd.evaluate(unchanged, self.target, - float(self.o.get("max_w", 2000)), + self.site_max_w, datetime.now(timezone.utc), - dev_min=self.dev_min, - dev_max=self.dev_max) + dev_min=self.site_dev_bound("dev_min"), + dev_max=self.site_dev_bound("dev_max")) if not fired: return self.log_event("METER FROZEN? " + self.freeze_wd.reason) @@ -1366,9 +1955,19 @@ class Controller: timeout=TARIFF_REFRESH_S) async def run_control(self) -> None: + # ⚠️ EACH INVERTER'S RATING FROM ITS OWN ENTITY, WHICH IS AC 4. There + # was one read of one `setpoint_entity` here, so a second inverter's + # rating was never read at all and its clamp, its freeze rail and the + # peak guard's `achievable_w` were all the first one's numbers. + # Concurrently, for `read_inputs()`'s reason: a slow ESPHome on one unit + # must not delay the controller's start on every other. + async def _limits(inv): + if inv.setpoint_entity: + inv.dev_min, inv.dev_max, _ = await self.hass.limits( + inv.setpoint_entity) + await asyncio.gather(*(_limits(inv) for inv in self.inverters)) entity = self.o.get("setpoint_entity", "") if entity: - self.dev_min, self.dev_max, _ = await self.hass.limits(entity) # ⚠️ THIS USED TO WARN AND CARRY ON, and that was the bug CFG-01 # exists to remove: a max_w above the device's rating meant # write_setpoint() silently clamped and the configured ceiling was @@ -1381,7 +1980,14 @@ class Controller: # guessed at. This is the first moment the device's real rating is # known. If it conflicts, refuse now - stopping deterministically # through the normal shutdown, which commands 0 W on the way out. - problems = validate_startup_config(self.o, self.dev_min, self.dev_max) + # ⚠️ GRADED AGAINST THE FIRST UNIT'S RANGE, because `validate.py` + # grades the FLAT options - `max_w`, `setpoint_entity` - which are + # unit A's. A per-unit validator arrives with CFG-04's per-unit + # options; inventing one here would be this file asserting a schema + # for a config that does not exist yet. + _first = self.inverters[0] + problems = validate_startup_config(self.o, _first.dev_min, + _first.dev_max) if problems: refuse(problems) self.refused = problems @@ -1395,7 +2001,6 @@ class Controller: last_grid = object() heartbeat = float(self.o.get("heartbeat_s", 10)) - last_beat = 0.0 # ⚠️ SAFETY-12. The floor is a DEADLINE ON THE GAP BETWEEN CYCLES, not a # second scheduler running alongside the arrival trigger. Every cycle # resets it, whichever trigger fired, so on a healthy changing meter it @@ -1479,19 +2084,74 @@ class Controller: # ⚠️ The heartbeat is not an optimisation. The ESP32 treats silence # longer than ~30 s as "the controller is gone" and zeroes the # inverter. Refreshing the SAME value is what proves we are alive. - loop_now = asyncio.get_running_loop().time() - if loop_now - last_beat >= heartbeat: - last_beat = loop_now - await self.write_setpoint(self.target, force=True) + # + # ⚠️ ONE DEADLINE PER INVERTER, AND THE REFRESHES GO OUT TOGETHER. + # This was ONE `last_beat` and ONE serialised write, so a hung + # `hass.call()` to inverter A consumed inverter B's ~30 s layer-1 + # budget and the firmware zeroed a healthy sibling. Now each unit + # owns its own deadline, each write is dispatched CONCURRENTLY and + # each is bounded by WRITE_BUDGET_S - so the worst a hung A can do + # to B is delay B's NEXT beat by the budget, which is why the budget + # plus the schema's largest `heartbeat_s` has to stay under layer + # 1's veto. That inequality is graded, not asserted. + # + # ⚠️ AND EACH UNIT REFRESHES ITS OWN SHARE, `inv.target`, never the + # site target. A heartbeat that re-emitted the site number would be + # the broadcast fault arriving through the back door, once every + # `heartbeat_s`, on a path with no allocator in it. + await self.send_heartbeats(asyncio.get_running_loop().time(), + heartbeat) self.publish() await asyncio.sleep(1) + async def send_heartbeats(self, loop_now: float, heartbeat_s: float) -> None: + """Refresh every inverter whose own deadline has come. AC 6 lives here. + + ⚠️ IT IS A METHOD RATHER THAN FIVE LINES INSIDE `run_control()` FOR ONE + REASON: the acceptance criterion is "a hung write to A cannot consume + B's layer-1 budget - verified by injecting the hang", and a check cannot + inject a hang into the middle of a `while not self.stopping` loop that + also sleeps, reads inputs and cycles. Inline, the only thing a test + could grade was its OWN gather of `write_setpoint()` calls - which is a + check on the fixture, and a serialised shipped loop survived it. + Measured: the mutation that replaced the concurrent dispatch with a + `for` loop passed the whole suite while this block was inline. + + `loop_now` is the caller's monotonic clock, the same discipline every + watchdog in this add-on takes its clock by. + """ + due = [inv for inv in self.inverters + if loop_now - inv.last_beat >= heartbeat_s] + if not due: + return + # ⚠️ THE DEADLINE MOVES BEFORE THE WRITE, NOT AFTER IT. Moved after, a + # unit whose write hangs would have its deadline advanced late every + # time and would drift into being re-sent on every tick; and a unit + # whose write RAISED would never have it advanced at all. + for inv in due: + inv.last_beat = loop_now + # ⚠️ CONCURRENTLY. In series, every inverter behind a hung one waits out + # that one's whole budget before its refresh is even attempted - and + # before this card, with no budget at all, it waited out the HTTP + # stack's timeout. That is one inverter's transport fault pushing a + # healthy sibling towards layer 1's ~30 s veto. + await asyncio.gather(*( + self.write_setpoint(inv.target, force=True, unit=inv) + for inv in due)) + def publish(self) -> None: + # ⚠️ SITE TOTALS, AND THE KEYS DO NOT MOVE. These entities are already + # discovered and already on every dashboard (TEL-07 keeps their + # `unique_id`s), so `battery` stays the number the meter sees the + # battery doing - the SUM - and `soc` stays the one a floor is graded + # against - the EMPTIEST pack. At the shipped roster of one both are + # inverter A's own readings, unchanged. Per-inverter entities are + # TEL-07's, not a key rename here. values = { "setpoint": self.target, "grid": self.grid, - "battery": self.batt, - "soc": self.soc, + "battery": self.site_batt_w, + "soc": self.pack_floor_soc, "phase": self.maint.phase, # ⚠️ SAFETY-02's alarm surface, and it is deliberately an existing # one. IF-01/IF-02 own `ems/event/failsafe` and the `failsafe active` @@ -1499,7 +2159,8 @@ class Controller: # here would be a surface to migrate later. This sensor is already # discovered, already on every dashboard, and "failsafe" is # additive to its vocabulary rather than a change to it. - "status": ("failsafe" if self.sign_wd.tripped + "status": ("failsafe" if any(i.sign_wd.tripped + for i in self.inverters) else "running" if self.auto else "stopped"), } # ⚠️ ONLY when P1 ingestion is actually running. The ESP32's stale-input @@ -1519,7 +2180,13 @@ class Controller: async def shutdown(self) -> None: """Deterministic wind-down. Do not skip this.""" self.stopping = True - _LOG.info("shutting down - commanding 0 W") + _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. + for inv in self.inverters: + inv.target = 0.0 await self.write_setpoint(0.0, force=True) self.mqtt.close() @@ -1586,8 +2253,9 @@ class Controller: f"meter_source {o.get('meter_source')} - only " f"homewizard_local sees whether the meter's own " f"document changes"}) - elif rail_unreachable(o.get("max_w", 2000), - dev_min=self.dev_min, dev_max=self.dev_max): + elif rail_unreachable(self.site_max_w, + dev_min=self.site_dev_bound("dev_min"), + dev_max=self.site_dev_bound("dev_max")): # SAFETY-20, and THE SECOND REASON TO USE THIS ROW RATHER THAN A # NEW MECHANISM. The transport case above and this one are the # same statement - "the conjunction cannot be evaluated here" - @@ -1597,9 +2265,9 @@ class Controller: out.append({"ok": False, "warn": True, "text": f"freeze detector: NOT COVERED - " + rail_unreachable( - o.get("max_w", 2000), - dev_min=self.dev_min, - dev_max=self.dev_max)}) + self.site_max_w, + dev_min=self.site_dev_bound("dev_min"), + dev_max=self.site_dev_bound("dev_max"))}) else: # ⚠️ SAFETY-20. THE RAIL PRINTED HERE IS THE RAIL THE PREDICATE # USES, device range included. This row used to interpolate @@ -1608,8 +2276,9 @@ class Controller: # inverter is smaller than its configured ceiling the page named # a number the command could never reach and called it `armed`. # Both the row and the predicate now read the same three inputs. - _rail = rail_w(o.get("max_w", 2000), - dev_min=self.dev_min, dev_max=self.dev_max) + _rail = rail_w(self.site_max_w, + dev_min=self.site_dev_bound("dev_min"), + dev_max=self.site_dev_bound("dev_max")) # And when the device is what set that number, SAY SO - the # operator configured `max_w` and is owed the reason the guard # is armed somewhere else. @@ -1617,10 +2286,12 @@ class Controller: # reads `min` and `max` independently and returns None for # whichever attribute is missing or unparseable - so this # renders whatever is there rather than formatting a None. - _cap = ("" if _rail >= rail_w(o.get("max_w", 2000)) - else f" - below max_w because " - f"{o.get('setpoint_entity') or 'the setpoint device'}" - f" accepts only {self.dev_min}...{self.dev_max} W") + _cap = ("" if _rail >= rail_w(self.site_max_w) + else " - below max_w because " + + "; ".join( + f"{inv.setpoint_entity or inv.name} accepts " + f"only {inv.dev_min}...{inv.dev_max} W" + for inv in self.inverters)) out.append({"ok": True, "warn": False, "text": f"freeze detector: armed (alerts after " f"{self.freeze_wd.cfg.hold_s:.0f} s unchanged " @@ -1715,8 +2386,15 @@ class Controller: f"average is measured from P1 telegrams and this " f"path has none, so a runaway cannot be capped"}) - rows = [("battery SoC", self.soc, o.get("soc_entity")), - ("battery power", self.batt, o.get("batt_entity"))] + # ⚠️ ONE PAIR OF ROWS PER INVERTER, NAMED. A single "battery SoC" row on + # a site with two packs is a page that shows one pack and calls it the + # site. The label keeps its exact wording at a roster of one, because + # that is what an existing install reads. + rows = [] + for inv in self.inverters: + tag = "" if len(self.inverters) == 1 else f" ({inv.name})" + rows.append((f"battery SoC{tag}", inv.soc, inv.soc_entity)) + rows.append((f"battery power{tag}", inv.batt, inv.batt_entity)) if not self.p1_enabled: # In P1 mode the check above replaces this one; leaving both in # would report "no entity configured" for a meter_entity that is @@ -1731,15 +2409,28 @@ class Controller: else: out.append({"ok": True, "warn": False, "text": f"{label}: {entity} = {value:g}"}) - sp = o.get("setpoint_entity") - if not sp: - out.append({"ok": False, "warn": False, "text": "setpoint entity not configured"}) - elif self.dev_max is None: - out.append({"ok": False, "warn": False, - "text": f"setpoint entity {sp} not found"}) - else: - out.append({"ok": True, "warn": False, - "text": f"setpoint {sp} (range {self.dev_min:g}…{self.dev_max:g} W)"}) + for inv in self.inverters: + sp = inv.setpoint_entity + tag = "" if len(self.inverters) == 1 else f" [{inv.name}]" + if not sp: + out.append({"ok": False, "warn": False, + "text": f"setpoint entity not configured{tag}"}) + elif inv.dev_max is None: + out.append({"ok": False, "warn": False, + "text": f"setpoint entity {sp} not found"}) + else: + out.append({"ok": True, "warn": False, + "text": f"setpoint {sp} (range {inv.dev_min:g}…" + f"{inv.dev_max:g} W)"}) + + # DEV-02. Whether this controller can still put a number on each + # inverter, and it is reported in the ONE state that matters: a unit + # `live_units()` has stopped listing has had its share handed to its + # siblings, or - at a roster of one - has taken the site to 0 W. A + # dropped link that shows nothing on the page reads as a healthy site. + for note in self.liveness_notes(): + out.append({"ok": False, "warn": "NOT COMMANDABLE" not in note, + "text": note}) # SAFETY-03. Red while a clamp is engaged, and the engagement COUNT is # shown even after it clears - a loop that wound once and stopped is @@ -1758,26 +2449,36 @@ class Controller: f"{self.clamp_wd.episodes} episode(s) since " f"start - see the event log"}) - if self.last_write_ok is False: - out.append({"ok": False, "warn": False, - "text": "last write to the inverter was REJECTED - check the log"}) + for inv in self.inverters: + if inv.last_write_ok is False: + tag = "" if len(self.inverters) == 1 else f" ({inv.name})" + out.append({"ok": False, "warn": False, + "text": f"last write to the inverter{tag} was " + f"REJECTED - check the log"}) # SAFETY-02. Reported in both states: a watchdog nobody can see is armed # is a watchdog nobody trusts when it fires. - if self.sign_wd.tripped: - out.append({"ok": False, "warn": False, - "text": "SIGN-DISAGREEMENT FAILSAFE - " + self.sign_wd.reason - + ". The inverter is held at 0 W. Find out why the " - "command opposed the meter before starting control " - "again; starting clears this. Restarting the " - "add-on does NOT clear it."}) - else: - held = self.sign_wd.held_s(datetime.now(timezone.utc)) - out.append({"ok": True, "warn": False, - "text": f"sign watchdog: armed (trips after " - f"{self.sign_wd.cfg.hold_s:.0f} s opposed, both over " - f"{self.sign_wd.cfg.min_w:.0f} W)" - + (f" - opposed for {held:.0f} s now" if held else "")}) + # ⚠️ ONE ROW PER INVERTER, because there is one watchdog per inverter + # and a trip on B leaves A commanding. A single row would have to pick + # one of them to describe, and the honest answer at a roster of two is + # both. At a roster of one the wording is unchanged. + for inv in self.inverters: + tag = "" if len(self.inverters) == 1 else f" [{inv.name}]" + if inv.sign_wd.tripped: + out.append({"ok": False, "warn": False, + "text": f"SIGN-DISAGREEMENT FAILSAFE{tag} - " + + inv.sign_wd.reason + + ". The inverter is held at 0 W. Find out why the " + "command opposed the meter before starting control " + "again; starting clears this. Restarting the " + "add-on does NOT clear it."}) + else: + held = inv.sign_wd.held_s(datetime.now(timezone.utc)) + out.append({"ok": True, "warn": False, + "text": f"sign watchdog{tag}: armed (trips after " + f"{inv.sign_wd.cfg.hold_s:.0f} s opposed, both over " + f"{inv.sign_wd.cfg.min_w:.0f} W)" + + (f" - opposed for {held:.0f} s now" if held else "")}) if not o.get("estop_fitted", False): out.append({"ok": False, "warn": True, @@ -1796,7 +2497,7 @@ class Controller: def status_payload(self) -> dict: checks = self.checks() bad = [c for c in checks if not c["ok"] and not c["warn"]] - if self.sign_wd.tripped: + if any(i.sign_wd.tripped for i in self.inverters): # Outranks "NOT READY": nothing is missing, something went wrong. level, banner = "bad", ("FAILSAFE — the command opposed the meter. " "Inverter held at 0 W.") @@ -1821,8 +2522,9 @@ class Controller: next_due = self.maint.next_due(datetime.now(timezone.utc)) rows = [ ("Grid", w(self.grid)), - ("Battery", w(self.batt)), - ("State of charge", "—" if self.soc is None else f"{self.soc:g} %"), + ("Battery", w(self.site_batt_w)), + ("State of charge", "—" if self.pack_floor_soc is None + else f"{self.pack_floor_soc:g} %"), ("Commanded", w(self.target)), ("Why", self.reason), ("Maintenance phase", self.maint.phase), @@ -1846,17 +2548,29 @@ class Controller: self.auto = not self.auto self.store.set("auto", self.auto) self.log_event("control started" if self.auto else "control stopped") - if self.auto and self.sign_wd.tripped: + if self.auto: # ⚠️ THE ONLY WAY OUT OF A SIGN-DISAGREEMENT TRIP, on purpose. # It clears on a deliberate human start, not on the condition # going away - the condition ALWAYS goes away, because the trip # commands 0 W and a 0 W command cannot disagree with anything. # An operator pressing start has, at minimum, looked at the # banner that says why. - self.sign_wd.reset() - self.store.set("sign_tripped", False) - self.store.set("sign_reason", "") - self.log_event("sign-disagreement failsafe cleared by operator") + # + # ⚠️ IT CLEARS EVERY INVERTER'S LATCH, AND SAYS SO PER INVERTER. + # One button is what the UI has and one button is what §15's + # banner tells the operator to press; what must not happen is a + # press that clears A while B stays latched with nothing on the + # page changing. Each latch is cleared under its OWN key, so an + # install that had only B tripped comes back with only B's + # history cleared. + for inv in self.inverters: + if not inv.sign_wd.tripped: + continue + inv.sign_wd.reset() + inv.store_set("sign_tripped", False) + inv.store_set("sign_reason", "") + 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) diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 0d87959..20813ce 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -39,6 +39,13 @@ _LOG = logging.getLogger("goodwe.store") # commanding -1000, -2000 W before re-tripping 180 s later. Under a supervisor # that restarts a crashed add-on that is a loop, three minutes of wrong-direction # power at a time, forever. +# +# ⚠️ AND SINCE DEV-02 THE TWO SAFETY-02 KEYS ARE THE *LEGACY* SPELLING. They are +# FLAT - one `sign_tripped` for a controller that now holds a LIST of inverters - +# so a trip caused by inverter B would latch inverter A across restarts, and one +# operator action would clear both. They live on here as the pre-DEV-02 layout +# that `migrate_unit_keys()` reads ONCE and never reads again. Nothing in the +# add-on writes them any more; see SCHEMA_VERSION. DEFAULTS = { "phase": "idle", "phase_started": None, @@ -48,6 +55,173 @@ DEFAULTS = { "sign_reason": "", } +# --------------------------------------------------------------------------- # +# DEV-02: PER-INVERTER KEYS, AND THE MIGRATION IS THE RISKY HALF +# +# ⚠️ A LATCH THAT SILENTLY FAILS TO MIGRATE IS A DISARMED SAFETY GUARD ON AN +# INSTALL THAT BELIEVES IT IS PROTECTED. SAFETY-02's whole reason for being +# persisted is that "restart the add-on" is the FIRST thing an operator staring +# at a FAILSAFE banner tries, and a latch held only in memory comes back armed, +# into the fault, unattended. A latch that is written down under a key nothing +# reads any more is exactly the same failure with a longer paper trail: the file +# still says `sign_tripped: true`, the banner is green, and the controller +# resumes driving the wrong way. +# +# So this is a MIGRATION WITH A POSTCONDITION, not a copy. `migrate_unit_keys()` +# reports what it did AND reads the result back; `Controller.__init__` grades +# that report and refuses to come up quietly if the latch did not survive. The +# test that grades it BREAKS the migration and watches the guard fire. +# --------------------------------------------------------------------------- # + +# 0 = the flat, one-inverter layout that shipped before DEV-02. +# 1 = per-inverter keys under `unit//...`. +SCHEMA_VERSION = 1 +SCHEMA_KEY = "schema_version" + +# Which flat keys belong to ONE inverter, and therefore move into its namespace. +# ⚠️ THE MAINTENANCE KEYS ARE NOT IN HERE AND THAT IS DELIBERATE. `phase`, +# `phase_started`, `last_completed` and `last_start_attempt` belong to the +# maintenance state machine, of which this add-on still holds exactly ONE - a +# monthly recalibration is a pack procedure and per-pack SCHEDULING is a card of +# its own, not something to infer from a key rename. Moving them here would +# quietly re-home an install's whole maintenance history under a unit name; they +# stay where they are, untouched, which is the other half of "migrates without +# losing its latch OR ITS HISTORY". +LEGACY_UNIT_KEYS = ("sign_tripped", "sign_reason") + + +def unit_key(unit: str, key: str) -> str: + """The namespaced Store key for `key` on inverter `unit`. + + ⚠️ ONE SPELLING, IN ONE PLACE. `Inverter.key()` calls this and so does the + migration, so a check can assert that what was migrated is what is read. + Two transcriptions of a key name is one copy plus a future disagreement + about which one holds the latch - and the copy that drifts is always the one + that reads. + """ + return f"unit/{unit}/{key}" + + +class MigrationReport: + """What `migrate_unit_keys()` did, and whether it can be believed. + + ⚠️ IT CARRIES `verified`, WHICH IS READ BACK FROM THE STORE RATHER THAN + ASSUMED FROM THE ASSIGNMENT. That is the difference between this and a + two-line copy: the caller is able to ask "is the latch that was in the file + before now under the key the watchdog reads", and a `False` there is a + disarmed guard, which is an ERROR and not a log line nobody sees. + """ + + def __init__(self): + self.ran = False # did anything need moving + self.from_version = 0 + self.moved = {} # new key -> value written + self.kept = [] # keys already namespaced, left alone + self.verified = True # every move read back equal + self.problems = [] # human-readable, one per failed move + # ⚠️ THE SUBSET THAT MATTERS, AND THE DISTINCTION IS NOT PEDANTRY. A + # store that drops a write of `sign_tripped=False` has lost the + # DEFAULT - the same statement "absent" already makes, so nothing is + # gone. One that drops a write of `True` has lost A LATCH. Only the + # second is a disarmed safety guard, and only the second may be allowed + # to shout: an alarm that fires on the harmless case is an alarm an + # operator learns to dismiss before the real one arrives. + self.lost = [] + + @property + def ok(self) -> bool: + """Nothing that CARRIED STATE failed to land.""" + return not self.lost + + def explain(self) -> str: + if not self.ran: + return (f"store already at schema {self.from_version}; no " + f"per-inverter migration needed") + out = (f"migrated {len(self.moved)} flat key(s) to inverter " + f"namespaces: " + ", ".join(sorted(self.moved))) + if self.kept: + out += " (left alone, already namespaced: " + ", ".join(sorted(self.kept)) + ")" + if self.problems: + out += " - PROBLEMS: " + "; ".join(self.problems) + if self.lost: + out += " - STATE LOST: " + "; ".join(self.lost) + return out + + +def migrate_unit_keys(store, unit: str) -> MigrationReport: + """Move the pre-DEV-02 flat per-inverter keys into `unit`'s namespace. + + `unit` is the FIRST inverter in the roster, which on every install that can + possibly have a flat key is the only one it ever had. A store written by a + version that knew about more than one inverter is already at + SCHEMA_VERSION and is not touched. + + THE THREE RULES, each one a case that would otherwise be a wrong answer: + + 1. IT NEVER OVERWRITES AN EXISTING NAMESPACED VALUE. A store that has both + spellings has been written by this version, and the namespaced one is the + live latch - the flat one is the fossil. Preferring the fossil would + resurrect a trip the operator has already cleared. + 2. THE FLAT KEYS ARE LEFT IN THE FILE. Deleting them buys tidiness and costs + a rollback: an operator who downgrades the add-on after a migration would + find no latch at all, which is the disarmed guard again in the other + direction. Left in place, a downgrade finds the trip it had. They go + stale from this moment - nothing writes them again - and the direction + that staleness fails in is a SPURIOUS latch (a 0 W hold an operator + clears), never a missing one. + 3. THE VERSION MARKER IS WRITTEN LAST. A crash between the copies and the + marker leaves the migration to run again, and rule 1 makes running it + again a no-op. A marker written first would leave a half-migrated store + looking finished. + """ + rep = MigrationReport() + data = getattr(store, "data", {}) + rep.from_version = int(data.get(SCHEMA_KEY, 0) or 0) + if rep.from_version >= SCHEMA_VERSION: + return rep + + rep.ran = True + for legacy in LEGACY_UNIT_KEYS: + target = unit_key(unit, legacy) + if target in data: + rep.kept.append(target) + continue + # ⚠️ `DEFAULTS[legacy]` RATHER THAN A LITERAL. An install that never + # tripped has no `sign_tripped` in its file at all, and the value it + # then lands on has to be the one `Store.load()` would have supplied - + # otherwise "absent" and "never tripped" stop being the same statement, + # which is the distinction store.py's own header is about. + value = data.get(legacy, DEFAULTS.get(legacy)) + store.set(target, value) + rep.moved[target] = value + + # THE POSTCONDITION, AND IT IS A READ-BACK RATHER THAN AN ASSUMPTION. This + # is the difference between this function and a two-line copy: the caller + # can ask "is the latch that was in the file before now under the key the + # watchdog reads", through the same `unit_key()` the watchdog's constructor + # reads through, and a `False` there is a guard that is off on an install + # whose state file says it is on. + for target, value in rep.moved.items(): + got = getattr(store, "data", {}).get(target, "<>") + if got == value: + continue + rep.verified = False + note = f"{target} was written as {value!r} and reads back as {got!r}" + rep.problems.append(note) + legacy = target.rsplit("/", 1)[-1] + if value != DEFAULTS.get(legacy): + rep.lost.append(note) + + if rep.ok: + store.set(SCHEMA_KEY, SCHEMA_VERSION) + else: + # ⚠️ NOT MARKED DONE. A migration that lost state must run again on the + # next start rather than be recorded as finished - the one thing worse + # than a failed migration is a failed migration a later version skips. + _LOG.error("per-inverter store migration LOST STATE and has not been " + "marked done: %s", rep.explain()) + return rep + class Store: def __init__(self, path: str = "/data/state.json"): diff --git a/goodwe_controller/test_alloc.py b/goodwe_controller/test_alloc.py index 9a9f727..d43de1e 100644 --- a/goodwe_controller/test_alloc.py +++ b/goodwe_controller/test_alloc.py @@ -672,29 +672,68 @@ check("...and the broadcast export overshoot at gain 0.6 - the half that has no feedback="unit")[1:]) <= -400.0 and min(g for _t, g in close_loop(0.6, 0.5)[1:]) >= 0.0) -# ⚠️ THE ONE THAT DOES NOT HOLD, AND IT IS REPORTED RATHER THAN HIDDEN. +# ⚠️ THE ONE THAT DID NOT HOLD, AND DEV-02 IS WHAT CLOSED IT. This block was +# written as a DISCLOSURE - "the allocator is not the cause, and it is DEV-02's +# to close" - and it is kept, with its stimulus intact, because the defect it +# describes is exactly the thing a regression would restore. What has changed is +# only which of the two rows the SHIPPED add-on is on. +# +# THE DEFECT. There was one `batt_entity`, so at a roster of two `actual_w` was +# ONE unit's power used as a site measurement - the SENSING half of the same +# N=1 identity ALLOC-01 removed from the COMMANDING half. `compute()`'s +# anti-windup test is `abs(prev_w - actual_w) > saturation_w` with `prev_w` the +# SITE target, so that comparison put a site number against a unit number, +# tripped the saturation freeze permanently and left a standing grid error. +# +# MEASURED, on admin/goodwe's own two-master rig rather than only here +# (`tools/alloc_sim.py --splits --feedback unit`, 2 kW load, 40 cycles): +# +# gain 0.2 worst |delta| between splits 510 W +# gain 0.4 250 W +# gain 0.6 50 W +# gain 0.8 and above 0 W +# +# and with `--feedback site` - the counterfactual, which is what DEV-02 makes +# the shipped sensing - every split is bit-identical at every one of those +# gains. Both rows are still run below, because the comparison is the evidence. _unit_fb = {s: close_loop(0.6, s, feedback="unit") == close_loop(0.6, 0.5, feedback="unit") for s in _splits} -print(f" with the loop fed ONE inverter's power instead of the site's, at " - f"gain 0.6: " + ", ".join(f"{int(s*100)}/{int((1-s)*100)}=" - f"{'same' if v else 'DIFFERS'}" - for s, v in _unit_fb.items())) -check("⚠️ SPLIT-INVARIANCE IS A PROPERTY OF THE LOOP'S INPUTS, NOT ONLY OF THE " - "ALLOCATOR, AND THE SHIPPED ADD-ON BREAKS IT FROM THE OTHER END. There is " - "ONE `inverter_power` sensor, so at a roster of two `actual_w` is ONE " - "unit's power used as a site measurement - the SENSING half of the same " - "N=1 identity this card removes from the COMMANDING half. Feed the loop " - "that and the aggregate trace becomes a function of the split. The " - "allocator is not the cause: its shares sum to S in every one of these " - "runs. Measured on the real rig too (tools/alloc_sim.py --splits " - "--gain 0.6), and it is DEV-02's to close", - not all(_unit_fb.values())) -check("...and the counterfactual is what says the sensor is the cause rather " - "than the allocator: change that ONE input to the site total and every " - "split is bit-identical again, at every gain", +print(f" with the loop fed ONE inverter's power - the PRE-DEV-02 sensing - " + f"at gain 0.6: " + ", ".join(f"{int(s*100)}/{int((1-s)*100)}=" + f"{'same' if v else 'DIFFERS'}" + for s, v in _unit_fb.items())) +_worst_unit = max( + max(abs(a[1] - b[1]) for a, b in zip(close_loop(g, s, feedback="unit"), + close_loop(g, 0.5, feedback="unit"))) + for g in (0.2, 0.4, 0.6) for s in _splits) +print(f" worst split-dependence under that sensing, gains 0.2-0.6: " + f"{_worst_unit:.0f} W at the meter") +check("⚠️ THE DEFECT IS STILL REPRODUCIBLE, WHICH IS WHAT MAKES THE FIX BELOW " + "MEAN SOMETHING. Fed ONE inverter's power the aggregate trace at the " + "meter is a function of the split, by hundreds of watts below gain 1.0. " + "The allocator is not the cause - its shares sum to S in every one of " + "these runs - and a check that had DELETED this row when the sensing was " + "fixed would leave the next reader unable to tell a working loop from an " + "untested one", + not all(_unit_fb.values()) and _worst_unit > 100.0) +check("⚠️ AND DEV-02 PUT THE SHIPPED ADD-ON ON THE OTHER ROW: `actual_w` is now " + "the SITE SUM of the per-instance battery readings " + "(`Controller.site_batt_w`, pinned at main.py's call site by " + "test_inverter.py part 5), and every split is bit-identical again at " + "every gain. This is the line that goes red if anyone hands the loop one " + "unit's power again - which is the tempting shape, because it is what a " + "single `batt_entity` gives you", all(close_loop(g, s, feedback="site") == close_loop(g, 0.5, feedback="site") - for g in (0.2, 0.4, 0.6, 1.0) for s in _splits)) + for g in (0.05, 0.2, 0.4, 0.6, 0.8, 0.9, 1.0) for s in _splits)) +check("...and the fix is not vacuous in the other direction either: the SHARES " + "still differ between splits under site feedback, so 'bit-identical at " + "the meter' is a statement about the aggregate and not about an allocator " + "that stopped allocating", + close_loop(0.6, 0.9, feedback="site") + == close_loop(0.6, 0.5, feedback="site") + and plan(2000.0, TWO, weights={A: 0.9, Bee: 0.1}).shares + != plan(2000.0, TWO, weights={A: 0.5, Bee: 0.5}).shares) # ============================================================================== diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 914cd2e..272445c 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -742,13 +742,21 @@ class _RefactoredController(Controller): 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. + + ⚠️ THE SIGNATURE GAINED `unit=` IN DEV-02 AND THIS FIXTURE FOLLOWED IT, which + is the honest update and not a weakening: `write_setpoint()` now addresses ONE + inverter (or, with `unit=None`, commands the whole site to 0 W), because a + controller with a roster cannot have one write path that means "the + inverter". A delegate that dropped the argument would silently re-broadcast + at N>=2, so it is passed through here exactly as a real refactor would have + to pass it through. """ - async def _emit_setpoint(self, value, *, force=False): - await Controller.write_setpoint(self, value, force=force) + async def _emit_setpoint(self, value, *, force=False, unit=None): + await Controller.write_setpoint(self, value, force=force, unit=unit) - async def write_setpoint(self, value, *, force=False): - await self._emit_setpoint(value, force=force) + async def write_setpoint(self, value, *, force=False, unit=None): + await self._emit_setpoint(value, force=force, unit=unit) async def _clamp_guard(self): await Controller.check_output_clamp(self) diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py new file mode 100644 index 0000000..5f187cb --- /dev/null +++ b/goodwe_controller/test_inverter.py @@ -0,0 +1,1169 @@ +"""Runnable check for DEV-02 - per-instance Inverter objects. `python3 test_inverter.py` + +THE DEFECT, in one sentence: every piece of per-device state in the Controller +was a SCALAR, and several of those scalars are safety inputs - so a second +inverter would have been graded against the first one's numbers. + +Nine parts, and what each one is deliberately NOT allowed to be: + + 1. AC 1. THE GRADER, over `app/main.py` itself. Read, not run: the claim is + "no per-device value remains a Controller scalar", which is a statement + about EVERY configuration and not about the ones a fixture reaches. It is + an AST scan plus a live `vars()` comparison, and the two halves catch + different re-introductions - a field assigned in `__init__` and a field + grown at runtime. + 2. AC 2. THE STORE MIGRATION, and it is graded rather than performed. A latch + that silently fails to migrate is a DISARMED SAFETY GUARD on an install + whose state file says it is protected, so the migration is BROKEN here and + the guard is watched to fire. + 3. AC 3. A TRIP ON B DOES NOT LATCH A, driven through real cycles with real + store keys, then through a real RESTART - because the whole reason + SAFETY-02's latch is persisted is that a restart is the first thing an + operator tries. + 4. AC 4. EACH RATING FROM ITS OWN ENTITY, and the clamp, the rail and + `achievable_w` that follow it. Inverter B is given a DIFFERENT rating from + inverter A on purpose: a check where both are 5000 W cannot tell "B's + number" from "A's number". + 5. AC 5. `actual_w` IS SITE-LEVEL, pinned on main.py's own call site. The + measurement behind it is test_alloc.py part 8 and the rig; what is pinned + here is that the loop is fed the SUM. + 6. AC 6. INDEPENDENT HEARTBEATS, with the hang INJECTED. A wall-clock + experiment against a transport that never answers, and the counterfactual - + the serialised write this replaces - run beside it so the number means + something. + 7. AC 7. `live_units()` ANSWERS HONESTLY, on the shipped method. + 8. AC 8. N=1 IS UNCHANGED. The rest of this repository's 17 files are the real + evidence for that and they run unmodified; what is here is the part they + cannot see - that the compatibility views REFUSE at N>=2 rather than + answering for the first inverter. + 9. MUTATION. Which check bites for each way of defeating this card. + +⚠️ INVERTER B DOES NOT EXIST YET (~2 weeks out; SPIKE-04 measures it). Every +second unit below is built from parameters passed IN, and where a number is +needed it is a HYPOTHESIS marked as one. Nothing here is a measured parameter +for a second device, and no check depends on one. + +⚠️ MUTATING THE MODULE THIS FILE GRADES? PURGE `__pycache__` FIRST - or run +`tools/mutation_run.sh`, which does. `python3 -B` alone does NOT save you. +Read MUTATION.md before you report a survivor. +""" + +import ast +import asyncio +import inspect +import io +import sys +import time +from datetime import datetime, timedelta, timezone +from pathlib import Path + +from app import main as app_main +from app.inverter import LINK_STALE_S, UNIT_A, Inverter, build_roster +from app.main import WRITE_BUDGET_S, Controller +from app.store import ( + DEFAULTS, LEGACY_UNIT_KEYS, SCHEMA_KEY, SCHEMA_VERSION, migrate_unit_keys, + unit_key, +) +from app.validate import FIRMWARE_INGEST_VETO_S +from app.freeze import rail_w + +fails = [] +HERE = Path(__file__).resolve().parent +MAIN_SRC = io.open(HERE / "app" / "main.py", encoding="utf-8").read() +MAIN_AST = ast.parse(MAIN_SRC) +CTRL = [n for n in MAIN_AST.body + if isinstance(n, ast.ClassDef) and n.name == "Controller"][0] + + +def check(name, cond): + print(f" {'ok ' if cond else 'FAIL'} {name}") + if not cond: + fails.append(name) + + +def read_schema_max(key): + """The schema's upper bound for `key` out of the SHIPPED config.yaml.""" + body = io.open(HERE / "config.yaml", encoding="utf-8").read() + for line in body[body.index("\nschema:"):].splitlines(): + st = line.strip() + if st.startswith(f"{key}:") and "(" in st and ")" in st: + return float(st[st.index("(") + 1:st.rindex(")")].split(",")[1]) + return None + + +# --------------------------------------------------------------------------- # +# fixtures +# --------------------------------------------------------------------------- # +class MemStore: + """A Store that keeps its dict across "restarts", like the real file does.""" + + def __init__(self, data=None): + self.data = dict(DEFAULTS, **(data or {})) + self.writes = 0 + + def set(self, k, v): + self.data[k] = v + self.writes += 1 + + def get_time(self, *a): + return None + + def set_time(self, *a): + pass + + +class Pub: + enabled = False + + def publish(self, *a, **k): + pass + + def close(self): + pass + + +class Hass: + """A Home Assistant that answers per ENTITY, which is the whole point here. + + ⚠️ EVERY FIXTURE IN THIS FILE ANSWERS PER ENTITY RATHER THAN PER CALL. A + stub that returned one number whatever it was asked cannot tell "inverter B + was read from inverter B's sensor" from "inverter B was handed inverter A's + reading", which is the defect this card is about. + """ + + def __init__(self, numbers=None, limits=None, reject=(), hang=(), + hang_s=30.0): + self.numbers = dict(numbers or {}) + self.limits_by = dict(limits or {}) + self.reject = set(reject) + self.hang = set(hang) + self.hang_s = hang_s + self.writes = [] # (entity, value) + self.write_at = [] # (entity, value, monotonic) + self.calls = [] + + async def number(self, entity, invert=False): + v = self.numbers.get(entity) + return None if v is None else (-v if invert else v) + + async def limits(self, entity): + return self.limits_by.get(entity, (None, None, None)) + + async def set_number(self, entity, value): + if entity in self.hang: + # A transport that accepted the request and never answers. This is + # the shape AC 6 is about: not an error, not a refusal - silence. + await asyncio.sleep(self.hang_s) + self.writes.append((entity, value)) + self.write_at.append((entity, value, time.monotonic())) + return entity not in self.reject + + async def call(self, domain, service, data): + self.calls.append(dict(data)) + return True + + +A_ENT = {"setpoint_entity": "number.sp_a", "soc_entity": "sensor.soc_a", + "batt_entity": "sensor.batt_a"} +# ⚠️ A HYPOTHESIS, NOT A MEASUREMENT. Inverter B does not exist; 3000 W is +# chosen only because it DIFFERS from A's 5000 W, which is what lets a check +# tell B's number from A's. SPIKE-04 measures the real one. +B_RATING_HYPOTHESIS = 3000.0 +A_RATING = 5000.0 + +BASE_OPTS = dict(A_ENT, meter_source="off", meter_entity="sensor.grid", + max_w=A_RATING, gain=0.6, slew_w=1000, deadband_w=15, + target_grid_w=0, step_w=10, heartbeat_s=10, stale_input_s=15) + + +def built(opts=None, store=None, hass=None, second=None): + """A real Controller, optionally with a real second `Inverter`.""" + hass = hass or Hass( + {"sensor.grid": 2000.0, "sensor.soc_a": 50.0, "sensor.batt_a": 0.0, + "sensor.soc_b": 50.0, "sensor.batt_b": 0.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0), + "number.sp_b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS, 1.0)}) + c = Controller(dict(BASE_OPTS, **(opts or {})), hass, + store if store is not None else MemStore(), Pub(), + stop=asyncio.Event()) + c.inverters[0].dev_min, c.inverters[0].dev_max = -A_RATING, A_RATING + if second is not None: + c.inverters.append(second) + return c, hass + + +def inverter_b(store=None, max_w=B_RATING_HYPOTHESIS, dev=None, + setpoint="number.sp_b", sign_cfg=None): + """A second inverter, built the way `build_roster()` builds the first. + + ⚠️ `sign_cfg` IS PASSED THROUGH RATHER THAN DEFAULTED, and that is not + housekeeping. `SignWatchdogConfig` carries `sign_disagree_w` and + `sign_disagree_s`, which are OPTIONS: a roster entry constructed without + them gets the module defaults instead of the install's, so a site would run + two watchdogs on two different thresholds and only one of them would be the + one the operator configured. `build_roster()` hands every unit the same + config for exactly that reason, and this helper copies the shape so the + checks below grade what ships. + """ + inv = Inverter("inverter-b", setpoint_entity=setpoint, + soc_entity="sensor.soc_b", batt_entity="sensor.batt_b", + max_w=max_w, store=store, sign_cfg=sign_cfg) + inv.dev_min, inv.dev_max = dev or (-max_w, max_w) + inv.soc, inv.batt = 50.0, 0.0 + return inv + + +def run(coro): + return asyncio.run(coro) + + +print("DEV-02 - per-instance Inverter objects") +print() +# ============================================================================== +print("1. AC 1: no per-device value is a Controller scalar, GRADED not read") + +# ⚠️ THE NAMES ARE THE CARD'S OWN LIST, and they are here rather than derived +# because the claim is about THESE values. A derived list - "everything on the +# Inverter" - would go green the moment somebody deleted a field from both +# sides, which is the one refactor this check has to survive. +PER_DEVICE = ("soc", "batt", "dev_min", "dev_max", "last_write", "last_write_at", + "last_write_ok", "clamp_last", "sign_wd", "last_beat", + "setpoint_entity", "soc_entity", "batt_entity", "max_w") +# ⚠️ `target` IS NOT IN THAT LIST AND MUST NOT BE, WHICH IS THE MOST IMPORTANT +# LINE IN THIS PART. `Controller.target` is the SITE grid target - one +# integrator on P1, above the split - and `Inverter.target` is that unit's SHARE +# of it. They are two different quantities with the same name, and folding them +# together is the thing ALLOC-01's stability argument does not survive: the gain +# at the meter is a convex combination of the per-unit gains only while ONE +# integrator produces the number that is then divided. So the site keeps its +# scalar, on purpose, and part 5 is where that is graded. + +_init = [n for n in CTRL.body + if isinstance(n, ast.FunctionDef) and n.name == "__init__"][0] +_assigned = set() +for node in ast.walk(_init): + for tgt in (node.targets if isinstance(node, ast.Assign) else + [node.target] if isinstance(node, ast.AnnAssign) else []): + for t in ([tgt] if not isinstance(tgt, ast.Tuple) else tgt.elts): + if (isinstance(t, ast.Attribute) and isinstance(t.value, ast.Name) + and t.value.id == "self"): + _assigned.add(t.attr) +_leaked = sorted(set(PER_DEVICE) & _assigned) +print(f" Controller.__init__ assigns {len(_assigned)} attributes; " + f"per-device ones among them: {_leaked or 'none'}") +check("⚠️ AC 1, STATICALLY. `Controller.__init__` assigns NOT ONE of the " + "per-device names. This is the scan a re-introduction goes red on at the " + "line it is written - `self.soc = None` back in the constructor is the " + "whole defect, and it would pass every behavioural check in this " + "repository at a roster of one", + _leaked == []) + +# The dynamic half. A field can also be grown at runtime, which the scan above +# cannot see, so the live object is compared against the roster entry. +_c1, _ = built() +_inst = set(vars(_c1)) +print(f" a live Controller carries {len(_inst)} instance attributes; " + f"per-device ones among them: {sorted(set(PER_DEVICE) & _inst) or 'none'}") +check("...and DYNAMICALLY, on a constructed Controller: none of them is an " + "instance attribute either. The compatibility views below are PROPERTIES " + "on the class, which is why they do not appear here - and why an " + "assignment through one cannot create a second copy to drift", + not (set(PER_DEVICE) & _inst)) +check("...and every one of them IS on the roster entry, which is the other " + "half of the same claim: they were MOVED, not deleted", + all(hasattr(_c1.inverters[0], n) for n in PER_DEVICE)) +check("...and the roster is a list of Inverter objects rather than a list of " + "names, so `units` cannot name an actuator the site has no object for", + all(isinstance(i, Inverter) for i in _c1.inverters) + and _c1.units == tuple(i.name for i in _c1.inverters)) +_roster = build_roster(dict(BASE_OPTS), MemStore()) +check("⚠️ AND `build_roster()` STILL RETURNS EXACTLY ONE INVERTER, from the " + "FLAT options, under the shipped name. INVERTER B DOES NOT EXIST YET " + "(~2 weeks; SPIKE-04 measures it, CFG-04 shapes the option that carries " + "it), and this card must not invent it: what it delivers is that the CODE " + "no longer assumes there is one. Adding the second entry is a change to " + "that one function and to nothing else, which is the point - and this is " + "the line that goes red if somebody adds a placeholder inverter B here", + len(_roster) == 1 and _roster[0].name == UNIT_A + and _roster[0].setpoint_entity == BASE_OPTS["setpoint_entity"] + and _roster[0].max_w == BASE_OPTS["max_w"]) + +# ⚠️ AND MAIN.PY MUST NOT READ ITS OWN COMPATIBILITY VIEWS. A view main.py used +# would be the scalar back with a longer name: it answers for the FIRST +# inverter, so every consumer through it silently grades inverter B against +# inverter A - the defect, restored, behind a property. +VIEWS = ("soc", "batt", "dev_min", "dev_max", "last_write", "last_write_at", + "last_write_ok", "clamp_last", "sign_wd", "unit") +_view_reads = [] +for m in CTRL.body: + if not isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if m.name in VIEWS or m.name == "_only": + continue + for n in ast.walk(m): + if (isinstance(n, ast.Attribute) and isinstance(n.value, ast.Name) + and n.value.id == "self" and n.attr in VIEWS): + _view_reads.append(f"{m.name}:{n.lineno} self.{n.attr}") +print(f" main.py's own reads of the N=1 views: {_view_reads or 'none'}") +check("⚠️ AND MAIN.PY NEVER READS ONE OF ITS OWN N=1 VIEWS. Every path in the " + "file iterates the roster. A `self.dev_min` anywhere inside Controller " + "would answer for the first inverter at every roster size, which is " + "exactly the scalar this card removed - and at N>=2 it would not even " + "fail, because `_only()` is only reached when the roster is one", + _view_reads == []) +check("...and the views are PROPERTIES, not attributes, so `c.soc = 4` writes " + "inverter A's own reading rather than creating a second copy of it", + all(isinstance(getattr(Controller, n, None), property) for n in VIEWS)) + + +# ============================================================================== +print() +print("2. AC 2: the store migration, and it is GRADED rather than performed") + +# ⚠️ THE RISKY HALF, IN THE CARD'S OWN WORDS. Namespacing `sign_tripped` is +# right - flat, a trip on B latches A - but a latch that silently fails to move +# is a disarmed safety guard on an install whose state file still says +# `sign_tripped: true`. Every check below is about that sentence. +_pre = MemStore({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: commanded -1500 W against " + "a meter reading +2000 W for 181 s", + "phase": "drain", "last_completed": "2026-08-01T00:00:00+00:00"}) +_c2, _ = built(store=_pre) +print(f" a pre-DEV-02 store with a LATCHED failsafe: " + f"{_c2.store_migration.explain()}") +check("the latch lands under the FIRST inverter's own key, which is the only " + "inverter an install written before this card could have had", + _pre.data[unit_key(UNIT_A, "sign_tripped")] is True) +check("...and so does the reason, so the operator is still told WHY. A latch " + "that migrated without its reason is a 0 W hold nobody can act on", + "SIGN_DISAGREEMENT" in _pre.data[unit_key(UNIT_A, "sign_reason")]) +check("⚠️ AND THE WATCHDOG THE CONTROLLER ACTUALLY BUILT COMES UP TRIPPED. " + "THIS is the check that matters: the keys being right is worth nothing " + "if `Inverter.__init__` reads somewhere else, and this reads the object " + "the cycle will consult", + _c2.inverters[0].sign_wd.tripped + and "SIGN_DISAGREEMENT" in _c2.inverters[0].sign_wd.reason) +check("...and THE HISTORY IS UNTOUCHED. The maintenance keys are NOT " + "namespaced - this add-on holds one maintenance state machine, and " + "re-homing a month-scale history under a unit name is a different card - " + "so an install keeps its phase and its last completed cycle", + _pre.data["phase"] == "drain" + and _pre.data["last_completed"] == "2026-08-01T00:00:00+00:00") +check("...and the store is marked migrated, so the next start does no work", + _pre.data[SCHEMA_KEY] == SCHEMA_VERSION) +check("...and the keys that moved are exactly the ones store.py names as " + "belonging to one inverter - read from `LEGACY_UNIT_KEYS` rather than " + "transcribed here, so a key added to the migration without a check, or " + "checked here without being migrated, cannot pass both ends", + set(_c2.store_migration.moved) + == {unit_key(UNIT_A, k) for k in LEGACY_UNIT_KEYS} + and "phase" not in LEGACY_UNIT_KEYS) + +# IDEMPOTENCE, AND IT IS THE ONE THAT PROTECTS THE OPERATOR'S CLEAR. +_pre.set(unit_key(UNIT_A, "sign_tripped"), False) +_pre.set(unit_key(UNIT_A, "sign_reason"), "") +_writes_before = _pre.writes +_c2b, _ = built(store=_pre) +check("⚠️ RUNNING AGAIN AFTER AN OPERATOR CLEARED THE LATCH DOES NOT RESURRECT " + "IT. The flat key still says `true` - it is a fossil, left in the file on " + "purpose - and a migration that re-read it would re-arm a failsafe the " + "operator has already dealt with, once per restart, forever", + not _c2b.inverters[0].sign_wd.tripped + and _pre.data[unit_key(UNIT_A, "sign_tripped")] is False) +check("...and it wrote nothing at all the second time, which is what " + "`schema_version` buys", + _pre.writes == _writes_before) +check("...and the flat keys are still there, unread. Deleting them costs a " + "ROLLBACK: an operator who downgrades the add-on after a migration would " + "find no latch at all. Left in place the staleness fails toward a " + "spurious 0 W hold, which is the safe direction", + _pre.data["sign_tripped"] is True) + +# A FRESH INSTALL MUST NOT INVENT A LATCH. +_fresh = MemStore() +_c2c, _ = built(store=_fresh) +check("a fresh install migrates the DEFAULTS and comes up ARMED AND UNTRIPPED. " + "`absent` and `never tripped` are the same statement, which is what " + "store.py's own header is about", + not _c2c.inverters[0].sign_wd.tripped + and _fresh.data[unit_key(UNIT_A, "sign_tripped")] is False) + +# ⚠️ AND NOW BREAK IT. THIS IS THE CHECK THE CARD ASKED FOR BY NAME. +class LosesTheLatch(MemStore): + """A store that accepts the namespaced write and drops it on the floor. + + Not a hypothetical: it is what every partial write, full disk and + silently-swallowed OSError looks like from here - `Store.save()` catches + OSError and carries on by design, because losing the maintenance history is + recoverable and refusing to run is not. + """ + + def set(self, k, v): + if k.startswith("unit/"): + return + super().set(k, v) + + +_broken = LosesTheLatch({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured"}) +_rep = migrate_unit_keys(_broken, UNIT_A) +print(f" a store that silently drops the migrated keys: {_rep.explain()}") +check("⚠️ THE MIGRATION REPORTS THAT IT FAILED, because it READS BACK what it " + "wrote instead of assuming the write landed. That read-back is the whole " + "difference between this and a two-line copy", + _rep.ran and not _rep.verified and not _rep.ok and _rep.lost) +check("...and it does NOT mark the store migrated, so a later version runs it " + "again rather than skipping a migration that never happened", + SCHEMA_KEY not in _broken.data) +check("...and a store that only fails to persist a DEFAULT is not reported as " + "lost state. `sign_tripped=False` is the same statement as `absent`, and " + "an alarm that fires on the harmless case is one an operator learns to " + "dismiss before the real one arrives", + migrate_unit_keys(LosesTheLatch(), UNIT_A).ok is True) + +_broken2 = LosesTheLatch({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured"}) +_c2d, _ = built(store=_broken2) +check("⚠️ AND THE CONTROLLER REFUSES TO COME UP DISARMED ON IT. The store says " + "this install is latched and the namespaced key cannot be made to hold " + "it, so the latch is RE-ARMED from the legacy value rather than lost. " + "The fail-safe direction for a latch is ON: a spurious trip costs a 0 W " + "hold an operator clears with one button; a missing one costs whatever " + "SAFETY-02 was watching for", + _c2d.inverters[0].sign_wd.tripped + and "SIGN_DISAGREEMENT" in _c2d.inverters[0].sign_wd.reason) +check("...and it says so in the action log, so the operator is not left to " + "infer it from a banner", + any("re-armed" in e for e in _c2d.events)) + + +async def _one_cycle(c): + await c.read_inputs() + await c.cycle() + return c + + +run(_one_cycle(_c2d)) +check("...and it commands 0 W from its very first cycle, which is what a latch " + "IS. A latch that reports itself and then commands normally is not a " + "latch, and the re-arming above would be theatre", + _c2d.target == 0.0 and "failsafe" in _c2d.reason) + + +# ============================================================================== +print() +print("3. AC 3: a trip on B does not latch A, and it survives a restart") + +# ⚠️ THE STIMULUS IS ASYMMETRIC ON PURPOSE, AND THAT TOOK SOME ARRANGING. Both +# inverters get a share of ONE site target, so an ordinary wrong-way loop trips +# BOTH - correctly, because both really are pushing the wrong way. The state +# this part is about is the one the flat store keys made unreachable: ONE +# inverter standing on a command that opposes the meter while the other does +# not. It is reached the way the field would reach it - inverter B's writes are +# being REJECTED, so `inv.last_write` stays where it was while A's tracks the +# loop - and `standing` is what SAFETY-02 grades, per unit. +# +# ⚠️ AND THE REJECTION WINDOW IS WHY THIS RUNS IN THREE SECONDS AND NOT THIRTY. +# `sign_disagree_s` is 1 s here; `LINK_STALE_S` is 30 s, so B is still LIVE for +# the whole run and the trip is graded before liveness has anything to say. +def _tripping_site(): + hass = Hass( + {"sensor.grid": 2000.0, "sensor.soc_a": 50.0, "sensor.batt_a": 0.0, + "sensor.soc_b": 50.0, "sensor.batt_b": 0.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0), + "number.sp_b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS, 1.0)}, + reject={"number.sp_b"}) + store = MemStore() + c, _ = built({"sign_disagree_w": 300, "sign_disagree_s": 1, + "auto_start": True}, store=store, hass=hass) + c.inverters.append(inverter_b(store, sign_cfg=c.inverters[0].sign_wd.cfg)) + c.auto = True + # B is standing on a charge command while the house imports 2000 W. Its + # writes are rejected, so this is what it goes on standing at. + c.inverters[1].last_write = -1500.0 + return c, hass, store + + +async def _trip_b(): + c, hass, store = _tripping_site() + for _ in range(12): + await c.read_inputs() + await c.cycle() + await asyncio.sleep(0.25) + return c, hass, store + + +_c3, _h3, _s3 = run(_trip_b()) +_a3, _b3 = _c3.inverters +print(f" after 3 s: {_a3.name} tripped={_a3.sign_wd.tripped}, " + f"{_b3.name} tripped={_b3.sign_wd.tripped}; " + f"shares {_c3.allocation.shares}") +check("PRECONDITION: inverter B really did trip - the stimulus is not vacuous", + _b3.sign_wd.tripped and "SIGN_DISAGREEMENT" in _b3.sign_wd.reason) +check("⚠️ AC 3. AND INVERTER A DID NOT. One SignWatchdog per actuator, which is " + "what watchdog.py's own docstring has always said and what the Controller " + "did not do: it held exactly one, so B's evidence latched A", + not _a3.sign_wd.tripped and _a3.sign_wd.reason == "") +check("...and A IS STILL COMMANDING. B's failsafe is a P_SAFETY SET on B's own " + "actuator, so stage 1 pins B at 0 W and the allocator hands the site " + "target to A - which is the site running on the inverter that works " + "rather than stopping because one of them is in trouble", + _c3.allocation.shares.get("inverter-b") == 0 + and _c3.allocation.shares.get("inverter-a") == _c3.allocation.sum_w + and _c3.allocation.sum_w != 0) +check("...and the number A was told reached A's own entity", + ("number.sp_a", float(_a3.target)) in _h3.writes and _a3.target != 0.0) +check("⚠️ AND THE LATCHES ARE UNDER SEPARATE KEYS, which is the half that " + "survives a restart. Flat, `sign_tripped` was ONE boolean: B's trip wrote " + "it, and every Controller built afterwards read it as A's", + _s3.data.get(unit_key("inverter-b", "sign_tripped")) is True + and _s3.data.get(unit_key(UNIT_A, "sign_tripped")) is False) + +# THE RESTART, which is the whole reason the latch is persisted at all. +_h3b = Hass({"sensor.grid": 2000.0, "sensor.soc_a": 50.0, + "sensor.batt_a": 0.0, "sensor.soc_b": 50.0, "sensor.batt_b": 0.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0), + "number.sp_b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS, 1.0)}) +_c3r, _ = built({"sign_disagree_w": 300, "sign_disagree_s": 1, + "auto_start": True}, store=_s3, hass=_h3b) +_c3r.inverters.append(inverter_b(_s3, + sign_cfg=_c3r.inverters[0].sign_wd.cfg)) +_c3r.auto = True +run(_one_cycle(_c3r)) +print(f" after a restart against the same store: " + f"A tripped={_c3r.inverters[0].sign_wd.tripped}, " + f"B tripped={_c3r.inverters[1].sign_wd.tripped}, " + f"shares {_c3r.allocation.shares}") +check("a brand new Controller against the same store comes back with B " + "latched and A NOT. 'Restart the add-on' is the first thing an operator " + "confronted with a FAILSAFE banner tries; before this card that restart " + "handed them a site with both inverters parked", + _c3r.inverters[1].sign_wd.tripped + and not _c3r.inverters[0].sign_wd.tripped) +check("...and A goes on carrying the site target across the restart too", + _c3r.allocation.shares.get("inverter-a") == _c3r.allocation.sum_w + and _c3r.allocation.sum_w != 0) +check("...and the status page shows BOTH rows - one latched, one armed. A " + "single row would have to pick one inverter to describe, and 'the sign " + "watchdog is armed' printed over a latched sibling is the page lying", + len([r for r in _c3r.checks() if "sign watchdog" in r["text"]]) == 1 + and len([r for r in _c3r.checks() + if "SIGN-DISAGREEMENT FAILSAFE" in r["text"]]) == 1) + +# The operator's clear, which is one button and has to reach every latch. +run(_c3r.handle_action("auto_toggle")) # -> stopped +check("pressing stop clears nothing, exactly as before this card", + _c3r.inverters[1].sign_wd.tripped + and _s3.data.get(unit_key("inverter-b", "sign_tripped")) is True) +run(_c3r.handle_action("auto_toggle")) # -> started +check("pressing start clears EVERY latched inverter, under its own key. One " + "button is what the UI has and what the banner tells the operator to " + "press; a press that cleared A while B stayed latched, with nothing on " + "the page changing, is worse than no button", + not _c3r.inverters[1].sign_wd.tripped + and _s3.data[unit_key("inverter-b", "sign_tripped")] is False + and _s3.data[unit_key("inverter-b", "sign_reason")] == "") + + +# ============================================================================== +print() +print("4. AC 4: each rating from its OWN entity, and what follows from it") + +# ⚠️ THE TWO RATINGS DIFFER, WHICH IS THE ONLY REASON THIS PART CAN SEE +# ANYTHING. Give both inverters 5000 W and every check below passes against a +# controller that reads one entity twice. +async def _read_ratings(): + store = MemStore() + c, hass = built(store=store, second=inverter_b(store)) + c.inverters[0].dev_min = c.inverters[0].dev_max = None + c.inverters[1].dev_min = c.inverters[1].dev_max = None + # main.py's own startup read, on the real method. + c.stop_event = asyncio.Event() + + async def _limits(inv): + if inv.setpoint_entity: + inv.dev_min, inv.dev_max, _ = await hass.limits(inv.setpoint_entity) + await asyncio.gather(*(_limits(i) for i in c.inverters)) + return c, hass + + +_c4, _h4 = run(_read_ratings()) +_a4, _b4 = _c4.inverters +print(f" A reports {_a4.dev_min:.0f}...{_a4.dev_max:.0f} W from " + f"{_a4.setpoint_entity}; B reports {_b4.dev_min:.0f}...{_b4.dev_max:.0f} W " + f"from {_b4.setpoint_entity}") +check("⚠️ EACH INVERTER'S DEVICE RANGE COMES FROM ITS OWN SETPOINT ENTITY. " + "There was ONE `hass.limits()` call on ONE entity, so inverter B's " + "rating was never read at all - its clamp, its freeze rail and the peak " + "guard's achievable_w were all inverter A's numbers", + (_a4.dev_min, _a4.dev_max) == (-A_RATING, A_RATING) + and (_b4.dev_min, _b4.dev_max) + == (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS)) +check("...and the per-unit rating is the conjunction of its supervised cap and " + "its own device range, written once so three consumers cannot transcribe " + "it three different ways", + _a4.rating_w() == A_RATING + and _b4.rating_w() == B_RATING_HYPOTHESIS) +check("...and a bound that could NOT be read is skipped rather than read as " + "zero. (None, None) means ESPHome was not up; answering 0 W there would " + "say this inverter can do nothing, which is the disarming direction", + inverter_b(dev=(None, None)).rating_w() == B_RATING_HYPOTHESIS) + +# THE CLAMP. Same requested value, two inverters, two answers. +async def _clamp_both(): + c, hass = built(store=MemStore(), second=inverter_b()) + await c.write_setpoint(9999.0, force=True, unit=c.inverters[0]) + await c.write_setpoint(9999.0, force=True, unit=c.inverters[1]) + return c, hass + + +_c4c, _h4c = run(_clamp_both()) +print(f" both asked for +9999 W: {_h4c.writes}") +check("⚠️ THE CLAMP FOR B USES B'S NUMBERS. The same out-of-range request " + "reaches A's wire at A's rating and B's wire at B's - which it could not, " + "while there was one `max_w` and one device pair on the Controller", + _h4c.writes == [("number.sp_a", A_RATING), + ("number.sp_b", B_RATING_HYPOTHESIS)]) +check("...and each unit's own clamp evidence records ITS OWN engagement, so a " + "clamp on B is not reported as a clamp on A", + _c4c.inverters[0].clamp_last.limit_w == A_RATING + and _c4c.inverters[1].clamp_last.limit_w == B_RATING_HYPOTHESIS + and all(i.clamp_last.engaged for i in _c4c.inverters)) +check("...and the action log NAMES the inverter, because 'the computed " + "setpoint was clamped' over a site with two of them is a row an operator " + "cannot act on", + any("inverter-b" in e and "OUTPUT_CLAMP" in e for e in _c4c.events) + and any("inverter-a" in e and "OUTPUT_CLAMP" in e for e in _c4c.events)) + +# THE SITE AGGREGATES, and each one's reduction is the check. +_c4s, _ = built(store=MemStore(), second=inverter_b()) +print(f" site: max_w {_c4s.site_max_w:.0f} W, rating {_c4s.site_rating_w:.0f} W, " + f"rail {rail_w(_c4s.site_max_w, dev_min=_c4s.site_dev_bound('dev_min'), dev_max=_c4s.site_dev_bound('dev_max')):.0f} W") +check("`achievable_w` and the freeze rail are the SUM of the per-unit " + "conjunctions, not a conjunction of the sums. Summing first would let one " + "unit's small device range cap the whole site; conjoining first is what " + "makes inverter B's rating bind inverter B", + _c4s.site_rating_w == A_RATING + B_RATING_HYPOTHESIS + and _c4s.site_max_w == A_RATING + B_RATING_HYPOTHESIS) +check("...and a device bound that ANY unit has not answered makes the SITE " + "bound unknown rather than smaller - which is the direction the shipped " + "single-inverter path already took with (None, None)", + _c4s.site_dev_bound("dev_max") == A_RATING + B_RATING_HYPOTHESIS + and built(store=MemStore(), + second=inverter_b(dev=(None, None)))[0] + .site_dev_bound("dev_max") is None) +check("...and at a roster of ONE every one of them is the shipped number, " + "exactly. That is AC 8 stated on the aggregates rather than on the suite", + _c1.site_max_w == A_RATING and _c1.site_rating_w == A_RATING + and _c1.site_dev_bound("dev_max") == A_RATING) + +# The two SoC reductions, which are the ones a wrong answer costs a pack. +_c4s.inverters[0].soc, _c4s.inverters[1].soc = 80.0, 21.0 +check("⚠️ THE PACK FLOOR IS THE EMPTIEST PACK, NOT THE MEAN. SAFETY-24's floor " + "asks whether the site may spend the pack for money; an average of 80 % " + "and 21 % is 50 %, which is permission to discharge a pack that is one " + "point above its floor", + _c4s.pack_floor_soc == 21.0) +_c4s.inverters[1].soc = None +check("⚠️ ...AND IT IS None IF ANY UNIT DID NOT ANSWER, not the min of the ones " + "that did. `decide()` treats None as floor-limited, which costs a shave " + "rather than a pack; a min() over the units that answered would be a " + "measurement asserted about a pack nobody read - which is NOTES.md 4.4b's " + "defect with a second inverter in it", + _c4s.pack_floor_soc is None) +_c4s.inverters[1].soc = 21.0 +check("...and back to the emptiest pack the moment it answers again", + _c4s.pack_floor_soc == 21.0) + + +# ============================================================================== +print() +print("5. AC 5: `actual_w` is SITE level while the readings are per-instance") + +# ⚠️ THE TWO HALVES OF §6.2's IDENTITY BREAK IN OPPOSITE DIRECTIONS, AND GETTING +# THAT BACKWARDS REINTRODUCES A MEASURED DIVERGENCE. ALLOC-01's review isolated +# it on admin/goodwe's two-master rig: with the loop fed ONE inverter's power +# the aggregate trace at the meter becomes a function of the split - up to +# 510 W apart at gain 0.2 and 250 W at gain 0.4 - while the allocator's shares +# summed to S in every one of those runs. The counterfactual is the proof, and +# it is `tools/alloc_sim.py --splits --feedback site`: every split bit-identical +# at every gain. test_alloc.py part 8 runs the same comparison against a plant +# it models itself; what is pinned HERE is main.py's own call site. +_compute_calls = [n for n in ast.walk(MAIN_AST) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Name) + and n.func.id == "compute"] +check("main.py has exactly ONE `compute(` call site, so the argument graded " + "below is unambiguously the shipped one", len(_compute_calls) == 1) +_actual_src = ast.unparse( + next(k.value for k in _compute_calls[0].keywords if k.arg == "actual_w")) +_site_batt_assign = [ast.unparse(n.value) for n in ast.walk(CTRL) + if isinstance(n, ast.Assign) and len(n.targets) == 1 + and isinstance(n.targets[0], ast.Name) + and n.targets[0].id == _actual_src] +print(f" main.py's compute(actual_w=...) is `{_actual_src}`, assigned from " + f"{_site_batt_assign}") +check("⚠️ AND IT IS THE SITE SUM. `compute()`'s anti-windup test is " + "`abs(prev_w - actual_w) > saturation_w` where `prev_w` is the SITE " + "target, so one unit's power there compares a site number against a unit " + "number, trips the saturation freeze permanently and leaves a standing " + "grid error. `self.batt` here - the shipped expression before this card - " + "is the defect", + _site_batt_assign == ["self.site_batt_w"]) +_c5, _ = built(store=MemStore(), second=inverter_b()) +_c5.grid = 1000.0 +_c5.inverters[0].soc = 50.0 +_c5.inverters[0].batt, _c5.inverters[1].batt = 1000.0, 2000.0 +check("...and the property is a plain SUM of the per-unit readings: a healthy " + "pair at 1000 and 2000 W is a site delivering 3000 W, which is what the " + "meter sees and what the site target is compared against", + _c5.site_batt_w == 3000.0) +check("...and None if any unit did not answer, which falls into the existing " + "`inputs missing -> command 0 W` path rather than into a sum that is " + "quietly short by one inverter", + _c5.inputs_ok is not None + and (setattr(_c5.inverters[1], "batt", None) or _c5.site_batt_w is None)) +check("...and `inputs_ok` requires EVERY pack's own two readings, so a silent " + "unit stops the site at P_SAFETY instead of being dropped from the split. " + "A missing measurement and a dead link are different faults with " + "different remedies", not _c5.inputs_ok) +_c5.inverters[1].batt = 2000.0 +check("...and it is true again the moment the pack answers", _c5.inputs_ok) +check("...and each unit's own reading is still ITS OWN, read from ITS OWN " + "entity - which is the half that had to become per-instance for the sum " + "above to be a sum of anything", + _c5.inverters[0].batt_entity == "sensor.batt_a" + and _c5.inverters[1].batt_entity == "sensor.batt_b") + + +async def _reads_are_per_entity(): + store = MemStore() + hass = Hass({"sensor.grid": 1000.0, "sensor.soc_a": 80.0, + "sensor.batt_a": 1000.0, "sensor.soc_b": 21.0, + "sensor.batt_b": 2000.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0)}) + c, _ = built(store=store, hass=hass, second=inverter_b(store)) + await c.read_inputs() + return c + + +_c5r = run(_reads_are_per_entity()) +print(f" after read_inputs(): A soc={_c5r.inverters[0].soc} " + f"batt={_c5r.inverters[0].batt}; B soc={_c5r.inverters[1].soc} " + f"batt={_c5r.inverters[1].batt}; site batt={_c5r.site_batt_w}") +check("⚠️ AND `read_inputs()` FILLS THEM FROM THE RIGHT SENSORS. This is the " + "line a roster loop that read `o['soc_entity']` for every unit goes red " + "on - it would give both packs A's state of charge, which is the pack " + "floor grading B against A", + (_c5r.inverters[0].soc, _c5r.inverters[0].batt) == (80.0, 1000.0) + and (_c5r.inverters[1].soc, _c5r.inverters[1].batt) == (21.0, 2000.0) + and _c5r.site_batt_w == 3000.0 and _c5r.pack_floor_soc == 21.0) + + +# ============================================================================== +print() +print("6. AC 6: independent heartbeats, with the hang INJECTED") + +# ⚠️ WHAT THE DEFECT ACTUALLY WAS, said precisely, because the fix only makes +# sense against it. There was ONE `last_beat` and ONE serialised +# `await self.write_setpoint(...)`. Layer 1 - the ESP32's own watchdog - +# commands 0 W and KEEPS COMMANDING IT after ~30 s without a refresh, and that +# is the failsafe this whole add-on is built around. So a transport that +# accepted a request for inverter A and never answered would hold the single +# write path, and the refresh inverter B was waiting for would arrive late or +# not at all: ONE INVERTER'S TRANSPORT FAULT TAKING A HEALTHY SIBLING TO ZERO. +# +# Two things fix it and both are graded: the writes go out CONCURRENTLY, and +# each one is bounded by `WRITE_BUDGET_S`. +_HB_MAX = read_schema_max("heartbeat_s") +print(f" WRITE_BUDGET_S {WRITE_BUDGET_S:.0f} s + the schema's largest " + f"heartbeat_s {_HB_MAX:.0f} s = {WRITE_BUDGET_S + _HB_MAX:.0f} s, against " + f"layer 1's {FIRMWARE_INGEST_VETO_S:.0f} s veto") +check("⚠️ THE BUDGET IS DERIVED, AND THE INEQUALITY IS WHAT IS GRADED. The " + "worst a hung write can do to a sibling is delay its NEXT beat by the " + "budget, so budget + the largest heartbeat the schema accepts has to fit " + "inside layer 1's veto. Raising `heartbeat_s`'s ceiling takes THIS red " + "instead of taking an inverter to zero in the field", + WRITE_BUDGET_S + _HB_MAX < FIRMWARE_INGEST_VETO_S) +check("...and it is below hass.py's own 10 s HTTP timeout, so the budget is " + "the binding one: we would rather abandon a write and re-issue it on the " + "next tick than hold the control loop for ten seconds", + WRITE_BUDGET_S < 10.0) + +# THE EXPERIMENT. Inverter A's transport accepts and never answers; inverter B's +# is healthy. The budget is shortened for the run - the shipped value is graded +# above - because a 4 s wait per case is 8 s of suite time to measure something +# that is already a ratio. +_REAL_BUDGET = app_main.WRITE_BUDGET_S +app_main.WRITE_BUDGET_S = 0.30 + + +async def _hung_heartbeat(): + """The SHIPPED heartbeat, against a transport that never answers for A. + + ⚠️ `send_heartbeats()` IS THE PRODUCTION METHOD `run_control()` CALLS, and + driving it rather than a gather written here is the difference between + grading the add-on and grading this file. Measured the hard way: while the + dispatch was five lines inline in `run_control()`, this part could only + gather its own `write_setpoint()` calls - and the mutation that replaced the + shipped dispatch with a `for` loop PASSED. + """ + store = MemStore() + hass = Hass({}, {}, hang={"number.sp_a"}, hang_s=30.0) + c, _ = built(store=store, hass=hass, second=inverter_b(store)) + c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + t0 = time.monotonic() + await c.send_heartbeats(1000.0, 10.0) + return c, hass, time.monotonic() - t0, t0 + + +_c6, _h6, _elapsed, _t0 = run(_hung_heartbeat()) +_b_at = [t - _t0 for e, _v, t in _h6.write_at if e == "number.sp_b"] +print(f" A hangs forever, B is healthy: the pair took {_elapsed:.2f} s " + f"(budget {app_main.WRITE_BUDGET_S:.2f} s); B's write reached the wire " + f"{(_b_at[0] if _b_at else float('nan')):.3f} s in; wire saw " + f"{_h6.writes}") +check("⚠️ AC 6. B'S WRITE REACHED THE WIRE WHILE A WAS STILL HANGING. That is " + "the whole criterion: a hung write to A cannot consume B's layer-1 " + "budget, because B's write was never behind A's in a queue", + _h6.writes == [("number.sp_b", 700.0)] + and bool(_b_at) and _b_at[0] < app_main.WRITE_BUDGET_S / 2.0) +check("...and the pair returned in about ONE budget rather than in two, which " + "is what says the writes were concurrent and not merely bounded. Two " + "bounded writes in series is still a sibling waiting for a corpse", + _elapsed < app_main.WRITE_BUDGET_S * 1.9) +check("...and the hung inverter is recorded as having FAILED its write, so " + "`last_write` is not advanced and the next tick re-issues it by itself", + _c6.inverters[0].last_write is None + and _c6.inverters[0].last_write_ok is False) +check("...while the healthy one recorded its write and will not re-send an " + "unchanged value", + _c6.inverters[1].last_write == 700.0 + and _c6.inverters[1].last_write_ok is True) +check("...and the operator is told, in the action log, that a write was " + "abandoned - a write that vanishes silently is a command an operator " + "believes is standing", + any("timed out" in e and "inverter-a" in e for e in _c6.events)) + +# THE COUNTERFACTUAL: the serialised write this replaces, run on the same +# transport. Without it "concurrent" is a word rather than a measurement. +async def _serialised(): + store = MemStore() + hass = Hass({}, {}, hang={"number.sp_a"}, hang_s=30.0) + c, _ = built(store=store, hass=hass, second=inverter_b(store)) + c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + t0 = time.monotonic() + b_at = None + for inv in c.inverters: # <- the shape this card replaces + await c.write_setpoint(inv.target, force=True, unit=inv) + if inv.name == "inverter-b": + b_at = time.monotonic() - t0 + return b_at, time.monotonic() - t0 + + +_ser_b_at, _ser_total = run(_serialised()) +print(f" the same transport, written SERIALLY: B's refresh arrived " + f"{_ser_b_at:.2f} s in, against {abs(_b_at[0]) if _b_at else 0:.2f} s " + f"concurrently") +check("⚠️ AND THE DEFECT IS REPRODUCED BESIDE THE FIX. Written in series, B's " + "refresh waits out A's entire budget before it is even attempted. At the " + "SHIPPED budget that is a whole extra second-scale delay per tick on " + "every inverter behind the hung one, and with the unbounded write this " + "card replaces it was the HTTP stack's timeout - which is what put a " + "healthy sibling within reach of layer 1's veto", + _ser_b_at >= app_main.WRITE_BUDGET_S) + +# AND THE DEADLINE ITSELF IS PER UNIT - graded through the shipped method, for +# the reason the experiment above is: a `due` list computed in this file is a +# check on this file. A refresh sent to A must not mark B refreshed. +async def _split_deadlines(): + store = MemStore() + hass = Hass() + c, _ = built(store=store, hass=hass, second=inverter_b(store)) + c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + # A was refreshed a moment ago; B has not been refreshed for a long time. + c.inverters[0].last_beat, c.inverters[1].last_beat = 1000.0, 0.0 + await c.send_heartbeats(1005.0, 10.0) + return c, hass + + +_c6b, _h6b = run(_split_deadlines()) +print(f" A fed at t=1000, B never: at t=1005 with a 10 s heartbeat the wire " + f"saw {_h6b.writes}") +check("⚠️ AND THE DEADLINE IS PER INVERTER TOO, not one shared clock. Only the " + "overdue inverter was refreshed. With ONE `last_beat` a refresh sent to A " + "marked B refreshed as well - so an inverter whose own writes kept " + "failing was recorded as fed, forever, by its sibling's success, and the " + "one thing that proves this add-on is alive was never sent to it", + _h6b.writes == [("number.sp_b", 700.0)]) +check("...and the refreshed unit's deadline moved while the other's did not, " + "so the next tick asks the same question again about the same unit", + _c6b.inverters[1].last_beat == 1005.0 + and _c6b.inverters[0].last_beat == 1000.0) +_beat_src = inspect.getsource(Controller.send_heartbeats) +check("...and the heartbeat re-emits each unit's OWN share rather than the " + "site target. A heartbeat that re-sent `self.target` would be the " + "broadcast fault arriving through the back door, once every " + "`heartbeat_s`, on a path with no allocator in it", + "inv.target, force=True, unit=inv" in _beat_src + and "self.target, force=True" not in _beat_src) + + +# ⚠️ AND THE CYCLE'S OWN WRITE DISPATCH IS THE OTHER HALF OF THE SAME PATH. The +# heartbeat is not the only place a hung inverter can hold the loop: every cycle +# writes every unit's share, and in series that is the same defect at the cycle +# rate rather than at the heartbeat rate. +app_main.WRITE_BUDGET_S = 0.30 + + +async def _hung_cycle(): + store = MemStore() + hass = Hass({"sensor.grid": 1000.0, "sensor.soc_a": 50.0, + "sensor.batt_a": 0.0, "sensor.soc_b": 50.0, + "sensor.batt_b": 0.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0)}, + hang={"number.sp_a"}, hang_s=30.0) + c, _ = built({"auto_start": True}, store=store, hass=hass, + second=inverter_b(store)) + c.auto = True + await c.read_inputs() + t0 = time.monotonic() + await c.cycle() + return c, hass, time.monotonic() - t0, t0 + + +_c6c, _h6c, _cyc_elapsed, _cyc_t0 = run(_hung_cycle()) +_cyc_b = [t - _cyc_t0 for e, _v, t in _h6c.write_at if e == "number.sp_b"] +print(f" a REAL cycle with A's transport hung: took {_cyc_elapsed:.2f} s, " + f"B's share reached the wire {(_cyc_b[0] if _cyc_b else float('nan')):.3f} s " + f"in; wire saw {_h6c.writes}") +check("⚠️ AND A REAL CYCLE DELIVERS B'S SHARE WHILE A HANGS. This is the same " + "criterion one layer up from the heartbeat: the per-unit writes in " + "`cycle()` go out together, so a hung inverter costs the loop one budget " + "and costs its siblings nothing", + _h6c.writes and _h6c.writes[0][0] == "number.sp_b" + and bool(_cyc_b) and _cyc_b[0] < app_main.WRITE_BUDGET_S / 2.0 + and _cyc_elapsed < app_main.WRITE_BUDGET_S * 1.9) +check("...and the share B was told is the allocator's, not the site target - " + "the hung sibling does not turn the cycle into a broadcast", + _h6c.writes[0][1] == float(_c6c.allocation.shares["inverter-b"]) + and _h6c.writes[0][1] != _c6c.target) + +app_main.WRITE_BUDGET_S = _REAL_BUDGET +check("...and the shipped budget is restored after the experiments, so nothing " + "below runs against a 0.3 s one", + app_main.WRITE_BUDGET_S == WRITE_BUDGET_S) + + +# ============================================================================== +print() +print("7. AC 7: live_units() answers per-instance liveness honestly") + +# ⚠️ THE ALLOCATOR DROPS WHAT THIS DOES NOT LIST and the survivors carry the +# whole site target, so both directions of a wrong answer cost something real: +# a false "not live" divides the site target among fewer inverters than exist, +# and a false "live" allocates into a link nobody can reach and calls the site +# target met. +_now = datetime.now(timezone.utc) +_c7, _ = built(store=MemStore(), second=inverter_b()) +check("a healthy roster is entirely live, which is the ordinary answer and has " + "to stay cheap", _c7.live_units() == ("inverter-a", "inverter-b")) +_c7.inverters[1].note_write(False, _now - timedelta(seconds=5)) +check("...and ONE rejected write does not drop anything. `set_number()` returns " + "False for a transient 400 as readily as for a device that has gone, and " + "dropping a unit on the first one would hand its share to a sibling every " + "time Home Assistant hiccupped", + _c7.live_units() == ("inverter-a", "inverter-b")) +# ⚠️ THE SECOND REJECTION DOES NOT MOVE THE START OF THE RUN, and that is the +# shipped behaviour rather than a fixture convenience: `rejected_since` records +# when the CURRENT run of failures began and only an ACCEPTANCE clears it. So +# the run is restarted here from a timestamp a whole window ago, through the +# same `note_write()` the write path calls, rather than by writing the field. +_c7.inverters[1].note_write(True, _now) +_c7.inverters[1].note_write(False, _now - timedelta(seconds=LINK_STALE_S + 1)) +check("⚠️ ...BUT A CONTINUOUS RUN OF THEM FOR A WHOLE LAYER-1 WINDOW DOES. " + "30 s is not a new opinion about links: it is `FIRMWARE_INGEST_VETO_S`, " + "the point at which the ESP32 takes that inverter to 0 W on its own. " + "Claiming it as commandable past there is the add-on promising the " + "allocator watts the firmware has already decided it will not deliver", + _c7.live_units() == ("inverter-a",) + and LINK_STALE_S == FIRMWARE_INGEST_VETO_S) +_c7.inverters[1].note_write(True, _now) +check("...and ONE acceptance brings it straight back, because the window is " + "about a run of failures and not about a counter that has to be reset by " + "hand", _c7.live_units() == ("inverter-a", "inverter-b")) +check("...and it is answered PER TICK from a clock, never cached: the same " + "object gives two answers as the rejection ages past the window, which " + "is what says nothing was decided at construction", + _c7.inverters[1].is_live(_now)[0] is True + and (_c7.inverters[1].note_write(False, _now) + or _c7.inverters[1].is_live(_now + timedelta(seconds=LINK_STALE_S))[0]) + is False) +check("...and the reason is carried, not just the verdict, so the status page " + "and the log can say WHICH link and for how long", + "rejected" in _c7.inverters[1].is_live( + _now + timedelta(seconds=LINK_STALE_S))[1]) +_c7.inverters[1].note_write(True, _now) +check("⚠️ AND A UNIT WITH NO `setpoint_entity` IS DELIBERATELY STILL LIVE HERE. " + "It is a CONFIGURATION that cannot be delivered rather than a link that " + "went away, and the two want opposite answers: a dropped link is an event " + "the survivors absorb and the record announces; a roster entry nobody can " + "write is a site that must STOP rather than run at part strength on a " + "roster somebody wrote down wrong. Absorbing it here would delete the " + "last trigger main.py's refusal branch has - and test_peakguard.py 10h-1b " + "drives that branch", + inverter_b(setpoint="").is_live(_now)[0] is True) +check("...and the status page says so in its own row, which is the surface an " + "operator actually reads", + any("NO SETPOINT ENTITY" in n for n in + built(store=MemStore(), second=inverter_b(setpoint=""))[0] + .liveness_notes())) +check("...and a healthy site adds NO liveness rows at all, so the page is not " + "carrying a line per inverter that says nothing", + built(store=MemStore(), second=inverter_b())[0].liveness_notes() == []) + + +# ============================================================================== +print() +print("8. AC 8: at N=1 the behaviour is unchanged, and the views REFUSE at N>=2") + +# ⚠️ THE REAL EVIDENCE FOR AC 8 IS THE OTHER SEVENTEEN FILES, WHICH RUN +# UNMODIFIED. test_freeze, test_loop, test_arbitrage, test_static_tariff, +# test_price, test_tariff and test_validate all build Controllers, set +# `c.dev_min`/`c.dev_max`, read `c.soc`/`c.batt`/`c.sign_wd`, and none of them +# was touched by this card. What is graded HERE is the part they cannot see, +# because every one of them is a site of one: that the compatibility views do +# not quietly answer for the first inverter once there are two. +_c8, _ = built(store=MemStore()) +_c8.dev_min, _c8.dev_max = -1234.0, 4321.0 +_c8.soc, _c8.batt = 44.0, 55.0 +check("at N=1 every view reads and WRITES through to the sole inverter, which " + "is §6.2's identity and is why an existing install and every existing " + "replay is byte-identical", + (_c8.inverters[0].dev_min, _c8.inverters[0].dev_max) == (-1234.0, 4321.0) + and (_c8.soc, _c8.batt) == (44.0, 55.0) + and _c8.unit == UNIT_A and _c8.units == (UNIT_A,)) +check("...and `sign_wd` and `clamp_last` are the sole inverter's own objects, " + "not copies - so a test that trips `c.sign_wd` trips the one the cycle " + "consults", _c8.sign_wd is _c8.inverters[0].sign_wd) + +_c8.inverters.append(inverter_b()) +_refused = [] +for name in VIEWS: + try: + getattr(_c8, name) + except AttributeError as err: + _refused.append((name, "no single per-device value" in str(err))) +print(f" with two inverters, {len(_refused)} of {len(VIEWS)} views refuse " + f"with an explanation") +check("⚠️ AND AT N>=2 EVERY VIEW REFUSES RATHER THAN ANSWERING FOR THE FIRST " + "INVERTER. THIS is the tripwire on the whole card: a consumer that was " + "never made per-instance fails on the line that reads it, loudly, " + "instead of quietly grading inverter B against inverter A's number - " + "which is the defect, and which is what a view that answered " + "`self.inverters[0]` would restore", + len(_refused) == len(VIEWS) and all(ok for _n, ok in _refused)) +_why = "" +try: + _c8.soc +except AttributeError as _err: + _why = str(_err) +check("...and the refusal NAMES THE WAY OUT - `for inv in " + "controller.inverters` - because the reader of that traceback is " + "somebody adding the second inverter and the answer is always the same " + "one. A bare AttributeError would send them looking for a deleted field", + "controller.inverters" in _why and "N=1" in _why) + + +def _settable(obj, name, value): + try: + setattr(obj, name, value) + return True + except AttributeError: + return False + + +check("...and `units` has no setter at all, so a site cannot claim two " + "actuators while holding one writer. It WAS an assignable tuple, and a " + "roster written that way is a half-driven site with nothing to say so - " + "the roster is grown by adding an `Inverter`, which is the object that " + "carries the writer", + not _settable(_c8, "units", ("x",)) + and _c8.units == ("inverter-a", "inverter-b")) + + +# ============================================================================== +print() +print("9. mutation: what defeats this card, and which check bites") + +# Each row is a one-line defeat of something above, with the check that catches +# it. ⚠️ THESE WERE RUN, NOT REASONED: every one was applied to the shipped +# module on a scratch copy, the tree was PURGED, and the named file was observed +# RED via `tools/mutation_run.sh`. `python3 -B` alone does not save you - a +# size-preserving edit reverted inside one second is invisible to CPython's +# staleness test (TEST-15). MUTATION.md is the method; the table is the record. +MUTATIONS = [ + ("`self.soc = None` restored in Controller.__init__", + "test_inverter.py part 1: the AST scan of __init__, at the line"), + ("a view answering `self.inverters[0]` instead of `_only()`", + "test_inverter.py part 8: 'every view REFUSES rather than answering for " + "the first inverter'"), + ("`actual_w = self.inverters[0].batt` - the loop fed one unit's power", + "test_inverter.py part 5, the call-site pin. ⚠️ NOT test_alloc.py part 8, " + "and that is worth saying: part 8 closes its loop around a plant IT " + "models, so it grades the PROPERTY (one unit's power diverges, the site " + "sum does not) and is blind to which of them main.py passes. Measured: " + "this mutation left test_alloc.py GREEN"), + ("the store migration performed but not read back", + "test_inverter.py part 2: 'the migration REPORTS that it failed'"), + ("`_grade_store_migration()` reduced to a log line", + "test_inverter.py part 2: 'the controller REFUSES to come up disarmed'"), + ("one shared SignWatchdog: every unit graded on inverter A's command", + "test_inverter.py part 3: 'inverter A did NOT trip'"), + ("`sign_tripped` written flat again instead of namespaced", + "test_inverter.py part 3 AND test_watchdog.py's store-key checks"), + ("`clamp_to_rating(value, self.o['max_w'], A's device range)`", + "test_inverter.py part 4: 'the clamp for B uses B's numbers'"), + ("`send_heartbeats()` dispatching in series instead of gathering", + "test_inverter.py part 6: 'the pair returned in about ONE budget'"), + ("`cycle()` writing the shares in series", + "test_inverter.py part 6: 'a REAL cycle delivers B's share while A hangs'"), + ("`asyncio.wait_for` dropped from the write", + "test_inverter.py part 6 - and it takes 94 s to say so, which is the " + "defect measuring itself"), + ("one shared `last_beat` restored", + "test_inverter.py part 6: 'the deadline is per inverter too'"), + ("`live_units()` returning the whole roster again", + "test_inverter.py part 7 AND test_peakguard.py 10h-2's PRECONDITION"), + ("the heartbeat re-emitting `self.target` instead of `inv.target`", + "test_inverter.py part 6: only the overdue unit's own share is written"), + ("the failsafe substitution keeping `shares=resolution.shares`", + "test_peakguard.py 10h-1b: 'it WRITES that 0 W to the unit it CAN reach'"), + ("`_read_unit()` reading `self.o['soc_entity']` for every unit", + "test_inverter.py part 5: 'read_inputs() fills them from the RIGHT " + "sensors'"), + ("`pack_floor_soc` averaging instead of taking the minimum", + "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"), +] +# ⚠️ 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 +# part 6 gathered its OWN `write_setpoint()` calls and computed its OWN `due` +# list - a check on this file rather than on the add-on. `send_heartbeats()` was +# extracted from `run_control()` so that the shipped dispatch is reachable by a +# check at all, and both are now driven through it. That is the failure mode +# this repository keeps finding, found once more here, and it is recorded rather +# than quietly fixed. +for what, caught in MUTATIONS: + print(f" {what}\n -> {caught}") +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) + + +print() +if fails: + print(f"{len(fails)} FAILED: {', '.join(fails)}") + sys.exit(1) +print("all checks passed") diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index d9a789d..dcc8809 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -127,6 +127,7 @@ from app.arbiter import ( from app.control import Tuning, compute, peak_at_risk from app.freeze import FreezeWatchdogConfig, looks_frozen, rail_w from app import main as app_main +from app.inverter import Inverter from app.main import Controller from app.p1 import ( PLAUSIBLE_MAX_W, P1Ingest, QUARTER_S, QuarterAverager, make_sample, @@ -921,6 +922,7 @@ class _Pub: class _Hass: def __init__(self): self.writes = [] + self.per_entity = [] self.notifications = [] # SAFETY-25. The whole payload, not only the id. AC 3 is about what the # operator READS, so a fixture that records only that a notification @@ -936,6 +938,11 @@ class _Hass: async def set_number(self, entity, value): self.writes.append(value) + # DEV-02. WHICH inverter each write went to, which is the only way to + # tell "both units were commanded their own share" from "one unit was + # commanded twice". `writes` keeps its old shape - a list of floats - + # because a dozen checks above read it that way. + self.per_entity.append((entity, value)) return True async def call(self, domain, service, data): @@ -1289,8 +1296,14 @@ _soc_src = ast.unparse(next(k.value for k in _call.keywords if k.arg == "soc")) print(f" main.py's decide(batt_w=...) is `{_batt_src}`, decide(soc=...) is " f"`{_soc_src}`") check("...and the call site says so in the source: the argument is the measured " - "battery attribute, not the standing command and not the resolved target", - _batt_src == "self.batt") + "battery attribute, not the standing command and not the resolved target. " + "⚠️ DEV-02 MOVED WHICH ATTRIBUTE AND NOT WHICH KIND OF THING IT IS: " + "`self.batt` was one inverter's power used as a site measurement, and " + "`site_batt_w` is the SUM over the roster - still measured, still not " + "`standing` and not `self.target`. The reduction itself is graded in " + "test_inverter.py; what is pinned here is that a MEASUREMENT is what " + "sizes the floor", + _batt_src == "self.site_batt_w") # ⚠️ SAFETY-24, AND ON THE SAME SINGLE CALL SITE. This grader takes the FIRST # `.decide(` in main.py and reads a NAME rather than a value, so a second call # site - or a decoy - would defeat it; this card deliberately adds neither, and @@ -1298,8 +1311,14 @@ check("...and the call site says so in the source: the argument is the measured check("...and `soc` is handed over as the MEASURED reading, bare. Not " "`self.soc or 100`, not a `float(..., 100)`, not the maintenance floor: " "a full battery is exactly the value that disarms SAFETY-24's floor, and " - "NOTES.md 4.4b records this project shipping that exact defaulting once", - _soc_src == "self.soc") + "NOTES.md 4.4b records this project shipping that exact defaulting once. " + "⚠️ DEV-02 MADE IT `pack_floor_soc` - THE EMPTIEST PACK - and the " + "defaulting hazard is unchanged and still the point: that property is a " + "bare `min()` over measured readings that answers None if ANY unit did " + "not read, because `decide()` treats None as floor-limited, which costs a " + "shave rather than a pack. An `or 100` anywhere on that path is the same " + "defect with two inverters", + _soc_src == "self.pack_floor_soc") check(f"...and main.py has exactly ONE `peakguard.decide(` call site (it has " f"{len(_peak_calls)}), so the call graded above is unambiguously the " f"shipped one. Other modules may call their own decide() - OPT-04's " @@ -2545,9 +2564,43 @@ check("...and the arbiter agrees the routing is legal, on the list main.py " # And the day the roster grows, the SAME cycle must partition. Graded through # main.py's own `actuator_of()` on a real Controller - the method every # `claims.append` above routes through - rather than through a copy of it. -c_src.units = ("inverter-a", "inverter-b") +# +# ⚠️ DEV-02: THE ROSTER IS GROWN BY ADDING A REAL `Inverter`, NOT BY SETTING +# `c.units`. It used to be a tuple somebody could assign beside a roster of one, +# i.e. a site claiming two actuators with one writer - which is a half-driven +# site with nothing to say so - and `Controller.units` is now DERIVED from +# `Controller.inverters` and has no setter. Building the object is also what +# makes these checks mean more than they did: the second unit has its own +# setpoint entity, its own rating and its own device range, so the claims below +# are filed from ITS numbers rather than from a copy of A's. +# +# ⚠️ INVERTER B DOES NOT EXIST YET. Its rating here is A's, copied, and that is +# a PLACEHOLDER: nothing in this file is a measured parameter for a second +# device (SPIKE-04 measures one). What is graded is the ARCHITECTURE - which +# actuator each claim lands on - and that is independent of the number. +def second_inverter(ctl, *, name="inverter-b", setpoint="number.sp_b", + max_w=None, dev=None): + """Append a real second `Inverter` to `ctl`, and return a restore().""" + inv = Inverter(name, setpoint_entity=setpoint, soc_entity="sensor.soc_b", + batt_entity="sensor.batt_b", + max_w=LIVE_MAX_W if max_w is None else max_w, + store=ctl.store) + inv.dev_min, inv.dev_max = dev or (-LIVE_MAX_W, LIVE_MAX_W) + # The readings a real cycle needs from it. `inputs_ok` requires EVERY unit + # to have answered, which is the point - a pack nothing can grade stops the + # site rather than being quietly dropped from the split. + inv.soc, inv.batt = ctl.inverters[0].soc, 0.0 + original = list(ctl.inverters) + ctl.inverters.append(inv) + + def restore(): + ctl.inverters[:] = original + return inv, restore + + +_b, _restore_b = second_inverter(c_src) _routed = {s: c_src.actuator_of(s) for s in CLAIM_SOURCES} -c_src.units = (app_main.UNIT_A,) +_restore_b() print(" with a second inverter on the roster, main.py routes: " + ", ".join(f"{s}->{a}" for s, a in sorted(_routed.items()))) check("⚠️ AND THIS IS THE BREAKING HALF. Give the same Controller a second " @@ -2563,7 +2616,7 @@ check("⚠️ AND THIS IS THE BREAKING HALF. Give the same Controller a second " "arbitrage": ACTUATOR_SITE, "safety": ACTUATOR_SITE}) check("...and the roster it was restored to is the shipped one, so nothing " "below inherits a two-inverter Controller", - c_src.units == (app_main.UNIT_A,) and len(c_src.units) == 1) + c_src.units == (app_main.UNIT_A,) and len(c_src.inverters) == 1) # ⚠️ AND THE ROUTING TABLE BEING RIGHT IS NOT THE SAME AS THE CALL SITES USING # IT RIGHT. `actuator_of("maintenance")` written at the `device` append routes a @@ -2618,7 +2671,27 @@ _direct = [n.args[0] for n in _appends if isinstance(n.args[0], ast.Call)] _indirect = [n.args[0].id for n in _appends if isinstance(n.args[0], ast.Name)] _routing = sorted(((c.args[0].value, append_actuator(c), c.lineno) for c in _direct), key=lambda r: r[2]) -_misrouted = [r for r in _routing if r[1] != f"self.actuator_of({r[0]!r})"] +# ⚠️ DEV-02 GAVE `actuator_of()` A SECOND ARGUMENT - the roster entry the claim +# is ABOUT - so a per-inverter append inside the `for inv in self.inverters:` +# loop reads `self.actuator_of('device', inv)`. The pin is unchanged in what it +# grades: the FIRST argument, the string literal, must still be the source the +# claim is filed under. What the second argument may be is deliberately NOT +# constrained here - it is a roster entry, and which one is a question the +# dynamic two-inverter cycle below answers on real claims - but its PRESENCE is, +# because a per-inverter claim built in that loop and routed with no `inv` files +# inverter B's rating on inverter A. That is graded at the line: a source +# classified SCOPE_UNIT must pass one. +def _routing_literal(a): + """The first argument of `self.actuator_of(...)`, or None if it is not one.""" + if not a.startswith("self.actuator_of("): + return None + return a[len("self.actuator_of("):-1].split(",")[0].strip() + + +_misrouted = [r for r in _routing if _routing_literal(r[1]) != repr(r[0])] +_unit_sources_unrouted = [ + r for r in _routing + if CLAIM_SCOPE.get(r[0]) == SCOPE_UNIT and "," not in r[1]] print(f" {len(_direct)} direct appends, each read at its own line: " + ", ".join(f"main.py:{ln} {s} -> {a}" for s, a, ln in _routing)) check("all eight of `Controller.cycle()`'s DIRECT appends route through " @@ -2638,6 +2711,17 @@ check("⚠️ AND AT EVERY ONE OF THE EIGHT THE LITERAL IT ROUTES THROUGH IS THE "`actuator_of(\"peak\")` at the maintenance append is a different string " "here even though it is the same VALUE at the shipped roster of one", _misrouted == []) +check("⚠️ AND EVERY PER-INVERTER APPEND NAMES THE INVERTER IT IS ABOUT. " + "`actuator_of()` takes the roster entry as its second argument and " + "defaults to the FIRST one when it is left out - which is right for the " + "site-scoped sources and is the whole defect for a per-inverter one: a " + "`device` limit built inside the roster loop and routed without `inv` " + "files inverter B's rating on inverter A, where it binds the wrong " + "share. Five of the eight appends are SCOPE_UNIT - `device`, " + "`supervised`, `failsafe` and maintenance's two - and all five pass one", + _unit_sources_unrouted == [] + and len([r for r in _routing + if CLAIM_SCOPE.get(r[0]) == SCOPE_UNIT]) == 5) check("...and the only appends this pin does NOT reach are the two that file a " "claim BUILT ELSEWHERE - `peak_claim` and `arb_claim` - which have no " "`actuator=` at the call site to mis-type. Their routing is pinned where " @@ -2656,17 +2740,18 @@ check("...and between them the ten appends account for exactly the eight " # cycle proving the routing reaches the arbiter THROUGH main.py rather than only # through this file's reading of main.py. It grades three of the eight appends. # The static pin above is what covers the other five. -c_src.units = ("inverter-a", "inverter-b") -_n2_writes_before = len(c_src.hass.writes) +_b2, _restore_b2 = second_inverter(c_src) +_n2_writes_before = len(c_src.hass.per_entity) try: _, _cl2 = cycle_claims(lambda: run_cycle(c_src)) _n2_target, _n2_reason = c_src.target, c_src.reason _n2_events = list(c_src.events) - _n2_writes = c_src.hass.writes[_n2_writes_before:] - _n2_last = c_src.last_write + _n2_writes = c_src.hass.per_entity[_n2_writes_before:] + _n2_shares = {i.name: i.target for i in c_src.inverters} + _n2_last = {i.name: i.last_write for i in c_src.inverters} _n2_alloc = c_src.allocation finally: - c_src.units = (app_main.UNIT_A,) + _restore_b2() print(" a REAL two-inverter cycle files: " + ", ".join(f"{c.source}@{c.actuator}" for c in _cl2)) check("a real cycle at a roster of TWO partitions its own claims correctly - " @@ -2676,18 +2761,29 @@ check("a real cycle at a roster of TWO partitions its own claims correctly - " "`device`, `supervised` and `loop`, so it is the static pin above and not " "this line that covers `failsafe`, `safety` and the two `maintenance` " "appends", len(_cl2) >= 3 and scope_violations(_cl2) == []) -check("...and the partition is the expected one on the sources this state " - "reaches: the two rating limits bind the unit, the loop claim stays on " - "the grid connection", - {c.source: c.actuator for c in _cl2} - == {"device": "inverter-a", "supervised": "inverter-a", - "loop": ACTUATOR_SITE}) +check("⚠️ AND THE RATING LIMITS ARE FILED ONCE PER INVERTER, ON ITS OWN " + "ACTUATOR - which is DEV-02 in one line. Before this card there was ONE " + "`device` pair, read from ONE `setpoint_entity`, filed once: a second " + "inverter's rating was never read and never bound anything, so its share " + "was clamped against the first one's numbers. Now the loop over the " + "roster files a pair per unit and the loop claim stays on the grid " + "connection", + sorted((c.source, c.actuator) for c in _cl2) + == [("device", "inverter-a"), ("device", "inverter-b"), + ("loop", ACTUATOR_SITE), ("supervised", "inverter-a"), + ("supervised", "inverter-b")]) check("...and naming those three is not an accident of this fixture but the " "reason the static pin exists - five of the eight sources need a state " "no check in this file reaches", {c.source for c in _cl2} == CLAIM_SOURCES - {"failsafe", "safety", "maintenance", "peak", "arbitrage"}) +check("...and each unit's `supervised` bound is ITS OWN `max_w`, not a copy of " + "the first unit's. That is the sentence AC 4 is: give inverter B a " + "different rating and the claim filed on inverter B moves with it", + [(c.lo, c.hi) for c in _cl2 + if c.source == "supervised" and c.actuator == "inverter-b"] + == [(-_b2.max_w, _b2.max_w)]) # --- 10h. ALLOC-01: WHAT MAIN.PY DOES WITH AN ALLOCATION IT CANNOT DELIVER --- @@ -2711,13 +2807,19 @@ check("...and naming those three is not an accident of this fixture but the " # 10h-3 every master is stale: no envelope set, so the allocator itself # refuses and `allocated=False` is reached for real. # -# ⚠️ AND 10h-1 IS THE GUARD ALLOC-01 HAD TO ADD RATHER THAN ONE IT INHERITED. -# Making `allocate()` succeed at N>=2 would otherwise have been a REGRESSION: -# there is one `setpoint_entity` in this add-on, so a controller handed a valid -# 450/450 split and a roster of two would have written `self.target` - 900, the -# SITE number - to the single inverter it can reach, while a second sat -# uncommanded. Not the broadcast fault, but the same shape of over-command, and -# the allocator's own arithmetic says the share is half of it. +# ⚠️ AND 10h-1 IS THE HALF DEV-02 CHANGED, SO IT HAS BEEN RE-AIMED AGAIN AND +# THE PREVIOUS AIM IS RECORDED HERE RATHER THAN DELETED. ALLOC-01 had to ADD +# this guard: there was one `setpoint_entity` in this add-on, so a controller +# handed a valid 450/450 split and a roster of two would have written +# `self.target` - 900, the SITE number - to the single inverter it could reach +# while a second sat uncommanded. Not the broadcast fault, but the same shape of +# over-command. +# +# DEV-02 gives it a writer per inverter, so that trigger is gone and the +# deliverable case is now the ORDINARY one: both units are commanded THEIR OWN +# SHARE, which is what 10h-1 grades below. The refusal survives with a trigger +# it can still reach - a roster entry with NO setpoint entity of its own - and +# 10h-1b drives it. print(f" at N=2 the allocator produced {_n2_alloc.explain()}") print(f" and main.py resolved to {_n2_target:+.0f} W, reason " f"{_n2_reason!r}, wrote {_n2_writes}") @@ -2736,53 +2838,105 @@ check("...and it did NOT broadcast: neither share is the site target, and " "THAT is the line a placeholder `{u: target for u in units}` goes red on", all(v != _n2_alloc.sum_w for v in _n2_alloc.shares.values()) or _n2_alloc.sum_w == 0) -check("⚠️ AND MAIN.PY REFUSES TO ACT ON A SPLIT IT CANNOT DELIVER. It drives " - "ONE inverter - one `setpoint_entity`, one power sensor - and the " - "allocation names two. Writing the site target to the one it can reach " - "would over-command that inverter by exactly the other's share; " - "commanding a partial site is a worse instruction than commanding " - "nothing. So the resolution it acts on is the 0 W failsafe it " - "substitutes, and `explain()` says so - this is the branch a `if False:` " - "deleted with the suite still green", - _n2_target == 0.0 and "failsafe" in _n2_reason) -check("...and it says WHY, in the action log, in the operator's own words - it " - "names the inverter it has no writer for and the card that gives it one", - any("no writer for inverter-b" in e and "DEV-02" in e - for e in _n2_events)) -check("...and then it WRITES that 0 W, on the same line as every other cycle. " - "It is a substitution, not an abstention: the setpoint is commanded to " - "zero rather than left wherever the last cycle put it, which is the " - "fail-toward-inaction direction and the opposite of leaving a stale " - "command standing", _n2_writes == [0.0] and _n2_last == 0.0) -check("...and the 0 W is NOT what the site target happened to be - resolve the " - "same claims at a roster of ONE, where the seam passes through, and a " - "real number comes back. That is what says the branch does work rather " - "than agreeing with the resolution by luck", - resolve(_cl2, units=("inverter-a",)).allocated is True - and resolve(_cl2, units=("inverter-a",)).target_w != 0.0) +check("⚠️ AND MAIN.PY NOW DELIVERS THE SPLIT. THIS IS DEV-02'S DELIVERABLE, " + "and it is the line that used to read 'refusing to command a partial " + "site'. Each inverter is written ITS OWN SHARE, to ITS OWN setpoint " + "entity, in one cycle - two entities, two numbers, and the two numbers " + "are the allocator's, not the site target repeated", + sorted(_n2_writes) + == sorted([("number.sp", float(_n2_alloc.shares["inverter-a"])), + ("number.sp_b", float(_n2_alloc.shares["inverter-b"]))])) +check("...and neither entity was handed the SITE target, which is the " + "broadcast fault stated as a check on the wire rather than on the " + "allocator. Each unit supplying the site number IN FULL is 4000 W " + "peak-to-peak forever at gain 1.0 on admin/goodwe's two-master rig, with " + "not one guard firing", + _n2_target == 0.0 or all(v != _n2_target for _e, v in _n2_writes)) +check("...and the resolution is NOT the 0 W failsafe any more: the site target " + "is a real number and it reached the hardware. That is the whole " + "difference this card makes to this part", + _n2_target != 0.0 and "failsafe" not in _n2_reason) +check("...and each inverter's own `last_write` records what IT was told, so " + "the heartbeat re-emits a SHARE rather than the site number and " + "SAFETY-02 grades each unit against the command IT is standing on", + _n2_last == {u: float(v) for u, v in _n2_alloc.shares.items()}) +check("...and the action log no longer carries the 'no writer' refusal, " + "because there is a writer. This is the line that goes red if that " + "branch is left standing with its old trigger", + not any("no writer" in e for e in _n2_events)) + +# --- 10h-1b. THE RE-AIMED REFUSAL: A ROSTER ENTRY WITH NO WRITER ------------ +# ⚠️ THE INSTINCT SURVIVES ITS TRIGGER, WHICH IS WHAT ALLOC-01 ASKED FOR WHEN IT +# SAID RE-AIM RATHER THAN DELETE - and PR #40's reviewer is why: replacing +# main.py's sibling branch with `if False:` left the whole suite green, so an +# unreachable-but-right guard was one refactor from being deleted as dead code. +# A configured inverter with no `setpoint_entity` of its own is still a unit +# this controller cannot put a number on, and commanding the ones it CAN reach +# would over-command them by exactly the missing unit's share. +_nw, _restore_nw = second_inverter(c_src, name="inverter-c", setpoint="") +_nw_writes_before = len(c_src.hass.per_entity) +try: + _, _cl_nw = cycle_claims(lambda: run_cycle(c_src)) + _nw_target, _nw_reason = c_src.target, c_src.reason + _nw_events = list(c_src.events) + _nw_writes = c_src.hass.per_entity[_nw_writes_before:] +finally: + _restore_nw() +print(f" a roster entry with no setpoint entity: main.py resolved to " + f"{_nw_target:+.0f} W, reason {_nw_reason!r}, wrote {_nw_writes}") +print(" and logged: " + + next((e for e in _nw_events if "ARBITER" in e), "(nothing)")) +check("⚠️ A UNIT WITH NO WRITER TAKES THE SITE TO THE 0 W FAILSAFE. It is a " + "CONFIGURATION that cannot be delivered - not a link that went away, " + "which is the dropped case below and is announced and survived - and a " + "site running at part strength on a roster somebody wrote down wrong " + "reads as healthy on every dashboard", + _nw_target == 0.0 and "failsafe" in _nw_reason) +check("...and it says WHY, naming the inverter it has no setpoint entity for", + any("no setpoint entity for inverter-c" in e for e in _nw_events)) +check("...and then it WRITES that 0 W to the unit it CAN reach, on the same " + "line as every other cycle. A substitution, not an abstention: the " + "inverter holds its last command forever, so failing toward inaction " + "means commanding zero, not falling silent", + _nw_writes == [("number.sp", 0.0)]) +check("...and the 0 W is NOT what the site target happened to be - the same " + "claims at a roster whose units all have writers resolve to a real " + "number, which is what says the branch does work rather than agreeing " + "with the resolution by luck", + resolve(_cl_nw, units=("inverter-a",)).allocated is True + and resolve(_cl_nw, units=("inverter-a",)).target_w != 0.0) # --- 10h-2. A DEAD LINK, AND THE SURVIVOR NOT ABSORBING IT SILENTLY --------- # ⚠️ STIMULATED THROUGH main.py's OWN SEAM, NOT THROUGH A PATCHED ARBITER. # `Controller.live_units()` is the production method the allocator's `live` -# argument comes from; today it answers "the whole roster" because this -# controller IS its one inverter's only link, and DEV-02 replaces its body with -# a per-master freshness test. Overriding what it returns for one cycle is -# therefore stimulating the state DEV-02 will produce, not mocking the thing -# under test - and it is the only way to reach AC 6 through a real cycle before -# DEV-02 lands. -c_src.units = ("inverter-a", "inverter-b") -_stale_writes_before = len(c_src.hass.writes) -_real_live = c_src.live_units +# argument comes from. +# +# ⚠️ AND SINCE DEV-02 IT IS DRIVEN BY THE REAL STIMULUS RATHER THAN OVERRIDDEN. +# This used to replace the method with a lambda, because at a roster of one +# "its master is stale" had no expression separate from "this controller is +# blind". `Inverter.is_live()` gives it one: a link whose every write has been +# REJECTED for a whole `LINK_STALE_S` window is not commandable, which is layer +# 1's own veto period - the point at which the firmware takes that inverter to +# zero anyway. So inverter B is put into exactly that state and the shipped +# method is asked, which is what makes this a check on `live_units()` instead of +# a check on a lambda this file wrote. +_bs, _restore_bs = second_inverter(c_src) +_bs.rejected_since = datetime.now(timezone.utc) - timedelta( + seconds=app_main.LINK_STALE_S + 5) +_bs.last_write_ok = False +_stale_writes_before = len(c_src.hass.per_entity) try: - c_src.live_units = lambda: ("inverter-a",) + check("PRECONDITION: the SHIPPED `live_units()` - not a lambda - drops the " + "inverter whose writes have been rejected for a whole layer-1 window, " + "and keeps the healthy one", + c_src.live_units() == ("inverter-a",)) _, _cl_stale = cycle_claims(lambda: run_cycle(c_src)) _stale_target = c_src.target _stale_alloc = c_src.allocation _stale_events = list(c_src.events) - _stale_writes = c_src.hass.writes[_stale_writes_before:] + _stale_writes = c_src.hass.per_entity[_stale_writes_before:] finally: - c_src.live_units = _real_live - c_src.units = (app_main.UNIT_A,) + _restore_bs() print(f" inverter-b's master stale: {_stale_alloc.explain()}") print(" and logged: " + next((e for e in _stale_events if "ALLOCATOR" in e), "(nothing)")) @@ -2798,7 +2952,13 @@ check("...and the SURVIVOR CARRIES THE WHOLE SITE TARGET, which is right - " "same event as a sibling with no writer", _stale_alloc.shares == {"inverter-a": _stale_alloc.sum_w} and _stale_target == float(_stale_alloc.sum_w) - and _stale_writes == [_stale_target]) + and ("number.sp", _stale_target) in _stale_writes) +check("⚠️ AND THE DROPPED UNIT IS COMMANDED 0 W, NOT LEFT STANDING. The " + "inverter holds its last command FOREVER - it has no meter timeout of its " + "own - so 'not in the split' has to mean zero on the wire. Leaving it " + "silent would leave a dead-link inverter pushing its last share while the " + "survivor carried the whole site target on top of it", + ("number.sp_b", 0.0) in _stale_writes) check("⚠️ AND IT DOES NOT ABSORB IT SILENTLY, which is the whole of AC 6 and of " "SAFETY-30. The action log names the inverter, the reason it left, and " "the consequence - that what remains now carries its share. A site " @@ -2808,18 +2968,26 @@ check("⚠️ AND IT DOES NOT ABSORB IT SILENTLY, which is the whole of AC 6 and for e in _stale_events)) # --- 10h-3. EVERY MASTER STALE: THE ALLOCATOR'S OWN REFUSAL, REACHED -------- -c_src.units = ("inverter-a", "inverter-b") -_dead_writes_before = len(c_src.hass.writes) +# Same stimulus, both units. ⚠️ AND IT REACHES A ROSTER OF ONE TOO: this is the +# state `live_units()`'s own docstring warns about - the sole inverter dropping +# takes the SITE to 0 W - so it is driven here rather than left as a sentence. +_bd, _restore_bd = second_inverter(c_src) +_gone = datetime.now(timezone.utc) - timedelta( + seconds=app_main.LINK_STALE_S + 5) +_bd.rejected_since, _bd.last_write_ok = _gone, False +c_src.inverters[0].rejected_since = _gone +c_src.inverters[0].last_write_ok = False +_dead_writes_before = len(c_src.hass.per_entity) try: - c_src.live_units = lambda: () _, _cl_dead = cycle_claims(lambda: run_cycle(c_src)) _dead_target, _dead_reason = c_src.target, c_src.reason _dead_alloc = c_src.allocation _dead_events = list(c_src.events) - _dead_writes = c_src.hass.writes[_dead_writes_before:] + _dead_writes = [v for _e, v in c_src.hass.per_entity[_dead_writes_before:]] finally: - c_src.live_units = _real_live - c_src.units = (app_main.UNIT_A,) + _restore_bd() + c_src.inverters[0].rejected_since = None + c_src.inverters[0].last_write_ok = True print(f" every master stale: {_dead_alloc.explain()}") print(" and logged: " + next((e for e in _dead_events if "ARBITER" in e), "(nothing)")) @@ -2835,11 +3003,11 @@ check("...and main.py substitutes the 0 W failsafe on it, writes that 0 W, and " "puts the allocator's OWN sentence in the action log rather than a " "paraphrase - so the operator reads which of the refusals fired", _dead_target == 0.0 and "failsafe" in _dead_reason - and _dead_writes == [0.0] + and set(_dead_writes) == {0.0} and len(_dead_writes) == 2 and any("ARBITER: REFUSED" in e and ALLOC_REFUSE_EMPTY in e for e in _dead_events)) -check("...and the roster it was restored to is the shipped one, so nothing " - "below inherits a two-inverter Controller or a patched liveness", +check("...and the roster it was restored to is the shipped one, with a healthy " + "link, so nothing below inherits a two-inverter Controller or a dead one", c_src.units == (app_main.UNIT_A,) and c_src.live_units() == (app_main.UNIT_A,)) diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index e4044b7..f3fcfa1 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -278,6 +278,18 @@ print("end to end: the trip is COMMAND ZERO, not alarm-only") import asyncio # noqa: E402 from app.main import Controller # noqa: E402 +# ⚠️ DEV-02. THE KEY SPELLING IS IMPORTED, NEVER TRANSCRIBED. `sign_tripped` and +# `sign_reason` are now NAMESPACED PER INVERTER - flat, a trip caused by B +# latched A across every restart and one operator press cleared both. A check +# that spelled `"unit/inverter-a/sign_tripped"` by hand here would go on passing +# against a main.py that had started writing somewhere else, which is this +# repository's own named failure mode: the copy that drifts is always the +# grading one. `unit_key()` is the one definition and `Inverter.key()` calls it. +from app.inverter import UNIT_A # noqa: E402 +from app.store import unit_key # noqa: E402 + +TRIPPED_KEY = unit_key(UNIT_A, "sign_tripped") +REASON_KEY = unit_key(UNIT_A, "sign_reason") class _Hass: @@ -430,9 +442,17 @@ async def _restart(): pre, post, store, hass2 = asyncio.run(_restart()) check("it tripped before the restart", pre.sign_wd.tripped) -check("the trip reached the store", store.data["sign_tripped"] is True) +check("the trip reached the store, UNDER THE TRIPPED INVERTER'S OWN KEY", + store.data[TRIPPED_KEY] is True) check("...with the reason beside it, so the operator is told why", - "SIGN_DISAGREEMENT" in store.data["sign_reason"]) + "SIGN_DISAGREEMENT" in store.data[REASON_KEY]) +check("⚠️ ...AND THE FLAT PRE-DEV-02 KEY IS NOT THE ONE THAT MOVED. It was " + "written by the migration at construction, with the value the store then " + "held (untripped), and nothing has written it since. A controller that " + "went on writing the flat key would latch every OTHER inverter on this " + "site as well - which is the defect, and it is only visible as the " + "ABSENCE of a second write here", + store.data.get("sign_tripped") is False) check("the new Controller comes back TRIPPED", post.sign_wd.tripped) check("...and says why, restored rather than re-detected", "SIGN_DISAGREEMENT" in post.sign_wd.reason) @@ -447,11 +467,11 @@ check("the banner still leads with the failsafe", # a one-press clear would be reachable by an operator who never read the banner. asyncio.run(post.handle_action("auto_toggle")) # -> stopped check("pressing stop does not clear the failsafe", - post.sign_wd.tripped and store.data["sign_tripped"] is True) + post.sign_wd.tripped and store.data[TRIPPED_KEY] is True) asyncio.run(post.handle_action("auto_toggle")) # -> started check("pressing start DOES clear it, and clears the store with it", - not post.sign_wd.tripped and store.data["sign_tripped"] is False - and store.data["sign_reason"] == "") + not post.sign_wd.tripped and store.data[TRIPPED_KEY] is False + and store.data[REASON_KEY] == "") # One more restart from the cleared store, to prove the clear persisted too. fresh = Controller({"setpoint_entity": "number.sp", "meter_source": "off"}, _Hass(), store, _P(), stop=asyncio.Event()) @@ -473,7 +493,7 @@ check("...so the next restart comes back armed and untripped, as it should", class _CrashOnFlag(_MemStore): def set(self, k, v): - if k == "sign_tripped" and v: + if k == TRIPPED_KEY and v: raise RuntimeError("power cut, mid-trip") super().set(k, v) @@ -503,9 +523,9 @@ async def _crash_mid_write(): crashed_store, died = asyncio.run(_crash_mid_write()) check("the second write is the one that sets the flag", died) check("a crash between the two writes leaves the REASON on disk", - "SIGN_DISAGREEMENT" in crashed_store.data.get("sign_reason", "")) + "SIGN_DISAGREEMENT" in crashed_store.data.get(REASON_KEY, "")) check("...and never a latch with nothing to explain it", - not crashed_store.data.get("sign_tripped")) + not crashed_store.data.get(TRIPPED_KEY)) print() if fails: diff --git a/goodwe_controller/tools/alloc_sim.py b/goodwe_controller/tools/alloc_sim.py index a66e492..1e16dfc 100644 --- a/goodwe_controller/tools/alloc_sim.py +++ b/goodwe_controller/tools/alloc_sim.py @@ -6,6 +6,11 @@ python3 tools/alloc_sim.py --sweep # AC 3: gain to 1.0 at every split python3 tools/alloc_sim.py --all + # DEV-02: the split-invariance finding ALLOC-01's review handed on, and the + # sensing change that closed it. The two rows are the evidence. + python3 tools/alloc_sim.py --splits --gain 0.4 --feedback unit # 250 W apart + python3 tools/alloc_sim.py --splits --gain 0.4 --feedback site # identical + WHY THIS IS A SCRIPT AND NOT A `test_*.py` ========================================== It needs admin/goodwe checked out beside this repository, and the add-on's CI @@ -121,20 +126,28 @@ def run_site_feedback(sitesim, control, mode, gain, load_w=2000.0, cycles=40, cycle_s=None, dt=0.05): """`sitesim.run_site()` WITH EXACTLY ONE LINE CHANGED, and it is named here. - ⚠️ THIS IS A COPY AND IT IS A COPY ON PURPOSE. `run_site` feeds `compute()` - ONE inverter's power as `actual_w`, and its docstring says why: the shipped - add-on binds a single `inverter_power` sensor, so an operator who wires up a - second inverter has a loop that sees unit A only, and a rig that fed the sum - would be KINDER than the deployment. That is correct for grading the - broadcast fault and it is exactly the variable this experiment has to move, - because it is the one input to the loop that is a function of the SPLIT. + ⚠️ THIS IS A COPY AND IT IS A COPY ON PURPOSE. `run_site` lives in + admin/goodwe, which this file reads and never writes, and it feeds + `compute()` ONE inverter's power as `actual_w` because that is what the + add-on gave it: a single `batt_entity`, so an operator who wired up a second + inverter had a loop that saw unit A only. A rig that fed the sum would have + been KINDER than the deployment. So this differs from `run_site` in one expression - `site.battery`, the site's total inverter power, instead of `inverters[0].plant.actual` - and in - nothing else. It is not evidence about the shipped add-on; it is the - counterfactual that says WHICH of the two candidate causes explains the - split-dependence that `--splits` measures at gain 0.6. The add-on does not - have this sensor yet. DEV-02 is where it would come from. + nothing else. + + ⚠️ AND SINCE DEV-02 THAT ONE EXPRESSION IS THE SHIPPED ONE, WHICH TURNS THIS + FUNCTION FROM A COUNTERFACTUAL INTO THE MODEL. `Controller.site_batt_w` is + the SUM of the per-instance battery readings and is what main.py hands + `compute()`; `--feedback site` is therefore the add-on as it now stands, and + `--feedback unit` is the PRE-DEV-02 sensing, kept because the comparison is + the evidence. Measured here, 2 kW load, 40 cycles: fed one unit's power the + aggregate trace at the meter is split-dependent by 510 W at gain 0.2, 250 W + at 0.4 and 50 W at 0.6; fed the site total every split is bit-identical at + every gain. `run_site` still models the old sensing and must not be + "corrected" to the new one - the row it produces is what the fix is measured + against. """ from plantsim import INVERTER_A, INVERTER_B, Inverter, Site cycle_s = sitesim.CYCLE_S if cycle_s is None else cycle_s @@ -240,10 +253,11 @@ def main(): ap.add_argument("--load", type=float, default=2000.0) ap.add_argument("--gain", type=float, default=1.0) ap.add_argument("--feedback", choices=("unit", "site"), - default="unit", - help="what the loop is fed as actual_w: one " - "inverter's power (the shipped add-on) or " - "the site total (the counterfactual)") + default="site", + help="what the loop is fed as actual_w: the site total " + "(the shipped add-on since DEV-02, the default here) " + "or one inverter's power (the PRE-DEV-02 sensing, " + "kept because the comparison is the evidence)") a = ap.parse_args() if not (a.table or a.splits or a.sweep): a.all = True -- 2.54.0 From e941bf7f62f69fd93811f8bdf1ac7481bdece170 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Wed, 2 Sep 2026 14:58:54 +0200 Subject: [PATCH 2/3] 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() -- 2.54.0 From d48c6b971dcdd06c520c429cc0498c4dd3ef0603 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Wed, 2 Sep 2026 15:02:58 +0200 Subject: [PATCH 3/3] DEV-02: correct the operator instruction the latch rename made wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HANDOVER.md told an operator to "delete `sign_tripped` from /data/state.json" to clear a sign-disagreement trip by hand. After this card that key is a FOSSIL: the live latch is `unit//sign_tripped`, and the flat one is left in the file only so a rollback finds its latch and is never read again. Following the old instruction would delete a key that clears nothing, on the one path an operator reaches while staring at a FAILSAFE banner - so the instruction would look like it had failed. DOCS.md's §SAFETY-02 section named the file but not the key; it now names both, and says why there is one per inverter. No code change; the suite is unaffected and green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- HANDOVER.md | 12 +++++++++++- goodwe_controller/DOCS.md | 4 +++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/HANDOVER.md b/HANDOVER.md index 192db71..9364ae3 100644 --- a/HANDOVER.md +++ b/HANDOVER.md @@ -83,7 +83,17 @@ rejected. ⚠️ **If you are testing the failsafe by hand, note that stopping and starting the add-on will not clear a trip.** That is the point of it; use the *Start / stop -control* button twice, or delete `sign_tripped` from `/data/state.json`. +control* button twice, or delete the trip from `/data/state.json`. + +⚠️ **DEV-02 renamed that key and the old name is now a fossil.** There is one +`SignWatchdog` per inverter, so the latch lives under +`unit//sign_tripped` — `unit/inverter-a/sign_tripped` on a +single-inverter site. The pre-DEV-02 flat `sign_tripped` is still written in +the file on purpose (so a rollback finds its latch) and is **never read again**, +so deleting it clears nothing. An install upgrading is migrated on first start, +and if the latch cannot be made to survive that migration the controller +re-arms it from the old key and says so in the log rather than coming up +disarmed. ⚠️ `test_loop.py` takes about **95 seconds** (measured 93 s) and that is not a bug. It drives the real control loop, which sleeps 1 s a tick, eight times over, to prove a diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 850a93f..862553e 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -306,7 +306,9 @@ does. That is deliberate on both counts: always vanishes the instant the trip works. A watchdog that took that as proof the fault was over would hand the inverter straight back to the loop that was driving the wrong way, three minutes later, forever. -- The trip is written to `/data/state.json` and restored on startup. Restarting +- The trip is written to `/data/state.json` — under + `unit//sign_tripped`, one key per inverter since DEV-02, so a trip + caused by one inverter cannot latch another — and restored on startup. Restarting the add-on is the first thing most people try when they see a red banner, and a latch that a restart clears would bring the controller back **armed, into the same fault, unattended** — and under a Supervisor that restarts a crashed add-on -- 2.54.0