diff --git a/HANDOVER.md b/HANDOVER.md index 192db71..c926f1f 100644 --- a/HANDOVER.md +++ b/HANDOVER.md @@ -83,7 +83,24 @@ 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. There is one `SignWatchdog` per inverter, so the +LIVE latch lives under `unit//sign_tripped`** — +`unit/inverter-a/sign_tripped` on a single-inverter site. **That is the one to +delete if you are clearing a trip by hand**; deleting the flat one alone clears +nothing. + +⚠️ **The flat `sign_tripped` is still maintained, though — it is the ROLLBACK +VIEW, not a fossil.** It is written alongside the namespaced key on a trip and +cleared alongside it when you press *Start / stop control*, so an add-on you +**downgrade** past this version still finds the latch it should. It is also read +on **every** start, and the one disagreement it can produce is acted on: *the +flat key says tripped and no inverter came up latched* means the latch was +written by an older add-on while this install was rolled back — or the migration +lost it — and the controller **re-arms the latch and shouts in the log** rather +than coming up disarmed with a green banner. So if you edit `/data/state.json` +by hand, clear **both** spellings or the next start will put the trip back. ⚠️ `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/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 3508c42..98f5c1a 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 write path is gated on the allocation record, not on how many config keys exist (SAFETY-28).** specs §1.5 makes *"any configuration in which two writers can reach one actuator"* a non-goal. Until now that non-goal was enforced by diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 850a93f..db677ae 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -306,7 +306,13 @@ 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. The + pre-DEV-02 flat `sign_tripped` is written and cleared alongside it as the + **rollback view**, so downgrading the add-on past DEV-02 still finds the + latch, and a trip taken while the install is rolled back is re-armed by the + version that comes back up rather than silently lost. 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 diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index 2722727..9a11757 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -127,6 +127,63 @@ Two rules come out of it, and both are cheap: function, that pair is the first thing to try — TEST-16's two survivors were adjacent lines in one function and neither was individually live. +## The third way a check lies: its DATA cannot reach the failure it names + +Stale bytecode manufactures survivors from a mutation that never ran. Masking +manufactures them from a second guard doing the first one's work. This one +manufactures them from a **fixture**: the check runs, the assertion is +evaluated, the sentence above it is true — and the numbers it compares are +arranged so that the failure it describes could not have changed the answer. + +It is not hypothetical and it is not rare. Three instances in one week, all +found by review rather than by a red line: + +* **ARB-03** shipped a call-site scan asserting that every `claims.append` in + `cycle()` routes through `actuator_of()`. Its own parse **saw 3 of the 8 + appends**; the other five were invisible to it and would have passed however + they were routed. +* **ALLOC-01** shipped a replay generator to hunt for envelope escapes. It found + **0**. A widened generator over the same allocator found **39,991**. +* **DEV-02** shipped `"each unit's supervised bound is ITS OWN max_w — give + inverter B a different rating and the claim filed on inverter B moves with + it"`, comparing inverter B's bound against `_b2.max_w`. The fixture defaulted + B's rating to A's, so it compared `(-5000, 5000)` against `(-5000, 5000)` — + **a number against itself**. Filing both per-unit bounds from + `self.inverters[0]` left **all 19 files green**. + +Note what these have in common with a masked rung: the mutation *runs*, and the +suite is *honestly* green. Nothing in a purge, a fingerprint or a disassembly +can see it, because the code under test really did behave identically on the +stimulus it was given. The defect is upstream of the assertion, in what the +check was handed. + +**The rule, and it is one line:** + +> **Any check asserting "X differs from Y" must be run once with X and Y +> actually made different.** + +Three cheap habits fall out of it, and all three are now used in this repository: + +1. **Write the PRECONDITION as its own check.** `test_peakguard.py` 10g asserts + that A and B have different ratings *before* it asserts that B's bound is + B's. If a later edit makes them equal again, the precondition goes red rather + than the claim going quietly vacuous. +2. **Assert against literals, not against the expression under test.** + `== [(-_b2.max_w, _b2.max_w)]` cannot distinguish "B's number" from "the + number this fixture happens to have twice"; `== (-3000.0, 3000.0)` beside a + printed `A = 5000` can. +3. **Print what the check saw.** Every one of the three above would have been + caught by a human reading one line of output — `supervised bounds by + actuator: {'inverter-a': (-5000, 5000), 'inverter-b': (-5000, 5000)}` is + self-evidently not evidence of a difference. + +And the mutation-table consequence: **a variant is not covered because a +near-miss of it is.** DEV-02's table listed the *actuator* variant of the +per-unit claim defect (`device`/`supervised` filed without `inv`), which the +static routing pin catches. The *bound* variant — right actuator, wrong +numbers — is a different defect with a different fix, and it was absent. Both +are in the table now. + ## Where each surface stands * **Local and dev-harness runs — the exposed one.** This is where FCST-03 hit 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 92a3665..3e6b4d5 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,34 @@ 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 LATCH IS GRADED HERE RATHER THAN TRUSTED, ON EVERY START. A + # latch that silently failed to move into its namespace - or that was + # taken by an OLDER add-on while this install was rolled back, which is + # the round trip store.py's rule 2 exists for - is a DISARMED SAFETY + # GUARD on an install whose state file still says `sign_tripped: true`. + # So the flat value is compared against what the roster 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 +328,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))) @@ -322,7 +386,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 @@ -339,17 +406,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 ------------------------------------------------------------- @@ -358,9 +422,340 @@ 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 the roster + 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. + + ⚠️ AND IT RUNS ON EVERY START, NOT ONLY ON THE ONE THAT MIGRATES. It + used to return early on `not rep.ran`, for a real reason: while nothing + cleared the flat key, an install that had ever tripped carried + `sign_tripped: true` in its file forever, and a guard reading it would + have re-armed the failsafe on every restart for the rest of the + install's life. + + THAT GATE SWALLOWED THE EXACT ROUND TRIP store.py's RULE 2 EXISTS FOR, + and it is not hypothetical - a firmware version change with a rollback + path was performed on this site on 2026-09-02: + + 1. the install migrates -> `schema_version = 1`, + `unit/inverter-a/sign_tripped = False` + 2. the operator DOWNGRADES, and SAFETY-02 trips inside the downgrade + window. The old code knows only the flat key, so the file gains + `sign_tripped: true` and nothing namespaced + 3. the operator upgrades again. `rep.ran` is False - the store is + already at schema 1 - so BOTH the migration and this guard are + skipped, `Inverter.__init__` reads the namespaced key, and the + controller comes up with `sign_wd.tripped = False` while + /data/state.json says `sign_tripped: true` + + Which is this module's own sentence, rebuilt by the version marker: the + file still says tripped, the banner is green, and the controller resumes + driving the wrong way. + + The gate is gone because the reason for it is gone. `handle_action()` + now clears the flat keys alongside the namespaced ones when the operator + clears the latch, and the trip mirrors them, so the two spellings can + disagree in exactly one direction: FLAT SAYS TRIPPED AND NO UNIT CAME UP + TRIPPED means the flat key was written by a version that did not know + about namespaces - a downgrade-window trip - or a migration that lost + the latch. Both want the same answer, which is the one below. + """ + rep = self.store_migration + if rep.ran and 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 + # ⚠️ THE QUESTION IS "IS ANY NAMESPACED LATCH SET", AND IT IS ASKED OF + # THE STORE RATHER THAN OF THE ROSTER. At a roster of two a trip on B + # leaves A untripped, so re-arming A off a flat key that B's own trip + # mirrored would latch a healthy inverter on every restart - the + # flat-key defect this card removed, coming back through its own guard. + # And the STORE is the right place to ask: it holds every + # `unit/*/sign_tripped` this install has ever written, including one + # belonging to a unit that is not in TODAY's roster (a roster is + # configuration and it changes), whereas the roster only knows what is + # configured right now. The flat key carries new information only when + # nothing namespaced is latched at all. + namespaced_latched = any( + bool(v) for k, v in data.items() + if k.startswith("unit/") and k.endswith("/sign_tripped")) + if namespaced_latched or any(inv.sign_wd.tripped + for inv in self.inverters): + return + first = self.inverters[0] + # 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 LATCH GUARD: this store carries a LATCHED " + "sign-disagreement failsafe under the pre-DEV-02 flat key and NO " + "inverter came up latched. Either the per-inverter migration lost " + "it, or it was written by an older add-on while this install was " + "rolled back. Both are a disarmed safety guard on an install whose " + "state file says it is protected, so the latch has been re-armed " + "on %s 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: @@ -377,9 +772,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. @@ -425,8 +825,13 @@ class Controller: elif self.inputs_bad_since is None: self.inputs_bad_since = datetime.now(timezone.utc) - def write_refusal(self, value: float) -> str | None: - """Why `value` may NOT be written right now, or None if it may. + 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) + + def write_refusal(self, value: float, inv=None) -> str | None: + """Why `value` may NOT be written to `inv` right now, or None if it may. --- SAFETY-28: THE WRITE PATH IS GATED ON THE ALLOCATION RECORD -------- specs §1.5 makes "any configuration in which two writers can reach one @@ -477,17 +882,68 @@ class Controller: a second writer would pass; the value is a fact about what is going to be written. -0.0 == 0.0 in Python, and a nan never reaches here (the caller has already turned it into 0.0), so the predicate is total. + + ⚠️ DEV-02: THE UNIT IS AN ARGUMENT, NOT THE CONTROLLER'S N=1 VIEW OF + IT. The record holds one share PER UNIT, so "did the allocator place + this number" is a question about a NAMED actuator. Asking it with the + first inverter's name at a roster of two would authorise A's share on + B's wire and refuse B's own - the site/unit confusion this card + removes, wearing the gate's name. `_write_unit()` always passes the + unit it is about; the default is for the N=1 callers and REFUSES at + N>=2 (see `_only()`) rather than answering for the first inverter. """ if value == 0.0: return None + inv = self._only() if inv is None else inv rec = self.allocation if rec is None: return ("no allocation record exists yet - nothing has authorised " - f"{value:+.0f} W") - return rec.authorises(self.unit, value, self.alloc_tick) + f"{value:+.0f} W on {inv.name}") + return rec.authorises(inv.name, value, self.alloc_tick) - async def write_setpoint(self, value: float, *, force: bool = False) -> None: - entity = self.o.get("setpoint_entity", "") + 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 @@ -534,39 +990,74 @@ class Controller: # leave the inverter latched on its last command with the ESP32's # heartbeat still being fed - holding an unauthorised setpoint quietly, # which is the outcome this gate exists to prevent. - refusal = self.write_refusal(value) + # + # ⚠️ AND IT IS ASKED ABOUT `inv`, NOT ABOUT THE SITE. The record holds + # one share per unit, so the question "did the single allocator place + # this number" is only answerable about a named actuator; DEV-02 made + # this method per-unit, and passing the first inverter here would + # authorise A's share on B's wire. + refusal = self.write_refusal(value, inv) if refusal is not None: - self.log_event(f"WRITE REFUSED {value:+.0f} W - commanding 0 W") - _LOG.error("SAFETY-28 REFUSED a write of %+.0f W: %s. Commanding " - "0 W instead. Every setpoint must be one the allocator " - "placed on THIS tick - see Controller.write_refusal().", - value, refusal) + self.log_event( + f"{inv.name}: WRITE REFUSED {value:+.0f} W - commanding 0 W") + _LOG.error("SAFETY-28 REFUSED a write of %+.0f W to %s: %s. " + "Commanding 0 W instead. Every setpoint must be one the " + "allocator placed on THIS tick, for THIS unit - see " + "Controller.write_refusal().", value, inv.name, refusal) 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 @@ -574,7 +1065,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. @@ -590,16 +1081,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. @@ -609,7 +1130,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) @@ -626,25 +1152,66 @@ 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) + # ⚠️ AND THE LEGACY FLAT KEYS ARE MIRRORED, WHICH IS THE OTHER + # HALF OF store.py's RULE 2. That rule keeps the flat keys "so a + # rollback still finds its latch", and it only ever covered the + # latch that existed AT MIGRATION TIME: a trip that happens + # AFTER the migration wrote nothing flat, so an operator who + # downgraded would find `sign_tripped: false` and come up + # DRIVING, into the fault. store.py used to claim the staleness + # could only fail toward a spurious latch; it could not, and + # this is what makes that sentence true. The pair is + # site-level on purpose - the version being rolled back TO + # drives exactly one actuator - and it is cleared by the same + # operator press that clears the namespaced ones. + self.store.set("sign_reason", inv.sign_wd.reason) + self.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 @@ -658,18 +1225,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 @@ -686,10 +1259,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 @@ -748,21 +1322,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"])), @@ -773,8 +1359,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, @@ -840,7 +1427,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, @@ -865,9 +1458,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 @@ -932,7 +1533,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: @@ -952,16 +1554,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 @@ -1034,35 +1651,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) @@ -1108,7 +1736,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. @@ -1185,13 +1835,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 " @@ -1403,11 +2063,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) @@ -1484,9 +2148,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 @@ -1499,7 +2173,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 @@ -1513,7 +2194,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 @@ -1597,19 +2277,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` @@ -1617,7 +2352,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 @@ -1637,10 +2373,34 @@ class Controller: async def shutdown(self) -> None: """Deterministic wind-down. Do not skip this.""" self.stopping = True - _LOG.info("shutting down - commanding 0 W") - await self.write_setpoint(0.0, force=True) + _LOG.info("shutting down - commanding 0 W on every inverter") + 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) + # -- UI ------------------------------------------------------------------ def checks(self, when: datetime | None = None) -> list: """The status rows. `when` is the reference time for anything billed by @@ -1704,8 +2464,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" - @@ -1715,9 +2476,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 @@ -1726,8 +2487,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. @@ -1735,10 +2497,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 " @@ -1833,8 +2597,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 @@ -1849,15 +2620,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 @@ -1876,26 +2660,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, @@ -1914,7 +2708,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.") @@ -1939,8 +2733,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), @@ -1964,20 +2759,61 @@ 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() + # + # ⚠️ 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}") + # ⚠️ AND THE PRE-DEV-02 FLAT KEYS GO WITH THEM, UNCONDITIONALLY. + # This is what makes `flat True + namespaced False` MEAN + # something, and it is the reason `_grade_store_migration()` can + # run on every start instead of only on the start that migrates. + # + # Without it the flat key is a fossil that says `true` forever + # on any install that ever tripped, so a guard comparing against + # it would re-arm the failsafe on every restart for the rest of + # the install's life - which is why that guard used to be gated + # on `rep.ran`, and why the gate then swallowed the rollback + # ROUND TRIP: migrate, downgrade, trip during the downgrade + # window (the old code writes only the flat key), upgrade again + # -> `rep.ran` is False, the guard never runs, and the + # controller comes up UNLATCHED while /data/state.json says + # `sign_tripped: true`. That is store.py's own header sentence, + # rebuilt by the version marker. + # + # Cleared here, the two spellings can only disagree one way: the + # flat key is newer than the namespaced one, which means it was + # written by a version that did not know about namespaces - i.e. + # a trip inside a downgrade window. Unconditional rather than + # inside the loop because an operator pressing START is + # declaring the whole site clear, and the fossil belongs to no + # unit in particular. self.store.set("sign_tripped", False) self.store.set("sign_reason", "") - self.log_event("sign-disagreement failsafe cleared by operator") 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): @@ -1986,8 +2822,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/app/store.py b/goodwe_controller/app/store.py index 0d87959..98e606f 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -39,6 +39,21 @@ _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. The LIVE latch is per inverter, under +# `unit//...`, and that is the one the watchdog is restored from. +# +# ⚠️ THE FLAT PAIR IS NOT DEAD, THOUGH: it is the ROLLBACK VIEW. It is written +# alongside the namespaced keys on a trip and cleared alongside them on the +# operator's clear, so an add-on that is DOWNGRADED past this schema still finds +# the latch it should - and so that a trip taken while the install was rolled +# back is still visible to the version that comes back up. Rule 2 of +# `migrate_unit_keys()` and `Controller._grade_store_migration()` carry the +# argument; the short version is that a latch is only worth persisting if it +# survives the version change an operator makes while staring at the banner. DEFAULTS = { "phase": "idle", "phase_started": None, @@ -48,6 +63,188 @@ 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 + where the WATCHDOG is restored from. Preferring the flat one HERE would + resurrect a trip the operator has already cleared, because a migration + runs before anything can tell the two apart. Deciding between them is + main.py's job and it has the one rule that can: the flat key carries new + information only when NOTHING namespaced is latched. + 2. THE FLAT KEYS ARE LEFT IN THE FILE, AND THEY ARE KEPT LIVE. 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. + + ⚠️ AND "LEFT IN PLACE" IS NOT ENOUGH ON ITS OWN, WHICH IS WHAT THIS RULE + USED TO SAY. The sentence here read "they go stale from this moment - + nothing writes them again - and the direction that staleness fails in is + a SPURIOUS latch, never a missing one". That was true of the latch this + migration MOVES and false of every trip that happens AFTERWARDS: nothing + flat was written for those, so a rollback found `sign_tripped: false` and + came up DRIVING. So main.py mirrors both keys on a trip and clears both + on the operator's clear. The flat pair is the ROLLBACK VIEW of the latch, + maintained, not a fossil - which is also what lets main.py's + `_grade_store_migration()` run on every start and catch a trip that + happened while the install was rolled back (see its docstring: the flat + key saying tripped while no inverter came up tripped is the one + disagreement that can occur, and it is a DOWNGRADE-WINDOW LATCH). + 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 e306684..7f5c3dc 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -84,6 +84,7 @@ from app.clamp import ( from app.allocator import Allocation, UnitPlan, plan from app.arbiter import P_LOOP, Claim, UnitBounds, allocate, resolve from app.control import Tuning, compute +from app.inverter import Inverter from app.main import Controller fails = [] @@ -782,13 +783,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) @@ -1102,9 +1111,20 @@ async def _failsafe_wire(): "max_w": LIVE_MAX_W, "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", "meter_entity": "sensor.grid"}, hass, _Store(), _Pub(), stop=asyncio.Event()) - c.units = (A_UNIT, B_HYPO) + # ⚠️ DEV-02: THE ROSTER IS GROWN BY ADDING AN `Inverter`, NOT BY ASSIGNING + # `units`. `Controller.units` is now DERIVED from `Controller.inverters` and + # has no setter, because a settable roster is a site that claims two + # actuators while holding one writer - a half-driven site with nothing to + # say so. The object carries the writer, which is what makes this fixture a + # roster of two rather than a tuple of two names. + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W # while the roster is one + _b = Inverter(B_HYPO, setpoint_entity="number.sp_b", + soc_entity="sensor.soc", batt_entity="sensor.batt", + max_w=LIVE_MAX_W, store=c.store) + _b.dev_min, _b.dev_max = -LIVE_MAX_W, LIVE_MAX_W + _b.soc, _b.batt = 50.0, 0.0 + c.inverters.append(_b) c.auto = True - c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W for _ in range(3): await c.read_inputs() await c.cycle() @@ -1117,7 +1137,7 @@ print(f" allocator refused every tick " check("the allocator having failed, the 0 W failsafe still REACHES THE WIRE - " "this is the deadlock the gate must not have, driven through the real " "cycle() and its real refusal branch rather than asserted", - not ctl_fs.allocation.ok and hass_fs.writes == [0.0] + not ctl_fs.allocation.ok and hass_fs.writes == [0.0, 0.0] and ctl_fs.target == 0.0) check("...and it was the ALLOCATOR's refusal that produced it, not the gate's " "- if the gate were the thing zeroing this cycle the action log would " diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py new file mode 100644 index 0000000..474026b --- /dev/null +++ b/goodwe_controller/test_inverter.py @@ -0,0 +1,1396 @@ +"""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 +from app.arbiter import P_LOOP, P_SAFETY, Claim, resolve + +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 arm(c, shares): + """Give `c` a real allocation record authorising exactly `shares`. + + ⚠️ SAFETY-28 MADE THIS NECESSARY AND THE FACT THAT IT IS NECESSARY IS THAT + TICKET. `write_setpoint()` refuses any number the single allocator did not + place on this tick, and the probes below call it directly - the way the + heartbeat and the operator's buttons do - so they have to say where their + number came from. Before that gate they did not, and that was the hole. + + ⚠️ AND IT IS BUILT THROUGH THE SHIPPED `resolve()`, NOT BY HAND, for the + reason test_clamp.py's twin says: a helper that assembled an `Allocation` + itself would be a second allocator living in the test file. The per-unit + numbers are forced the way the add-on forces them - each unit files its own + `supervised` envelope and the site target is their sum, so the water-fill + has exactly one answer - and the assert is what says this fixture got it. + """ + c.alloc_tick += 1 + claims = [Claim.set("loop", P_LOOP, float(sum(shares.values())), + "test stimulus")] + for unit, watts in shares.items(): + claims.append(Claim.limit("supervised", P_SAFETY, -abs(float(watts)), + abs(float(watts)), "max_w", actuator=unit)) + res = resolve(claims, units=c.units, live=c.units, tick=c.alloc_tick) + c.allocation = res.allocation + assert res.allocation.shares == {u: float(w) for u, w in shares.items()}, ( + f"arm() asked for {shares} and the shipped allocator placed " + f"{res.allocation.shares} - fix the fixture, not the assert") + return res.allocation + + +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") + +async def _one_cycle_early(c): + """One real cycle. `_one_cycle` below is the same three lines; this one is + needed before it, and duplicating three lines beats moving a definition + somebody is reading in place.""" + await c.read_inputs() + await c.cycle() + return c + + +# ⚠️ 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. +# +# ⚠️ THE CLEAR IS PRESSED, NOT SIMULATED, AND THAT CHANGE IS THE TICKET. This +# check used to clear the NAMESPACED key by hand and leave the flat one saying +# `true`, then assert that a restart did not resurrect the latch. It graded a +# state the shipped clear path does not produce: `handle_action("auto_toggle")` +# clears BOTH spellings, precisely so that `flat True + namespaced False` can +# only ever mean a trip taken while the install was rolled back (see below). +# Driving the button is also the only way to find that out - a fixture that +# hand-writes the store is a check on the fixture. +run(_c2.handle_action("auto_toggle")) # the operator presses START +_writes_before = _pre.writes +_c2b, _ = built(store=_pre) +check("⚠️ RUNNING AGAIN AFTER AN OPERATOR CLEARED THE LATCH DOES NOT RESURRECT " + "IT. A guard that went on comparing against a flat key nothing cleared " + "would re-arm a failsafe the operator has already dealt with, once per " + "restart, forever - which is what the `rep.ran` gate used to buy, and " + "what clearing both spellings buys instead", + 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 OPERATOR'S PRESS CLEARED THE PRE-DEV-02 FLAT KEYS TOO. That " + "is what makes the two spellings comparable at all: cleared together, " + "the only disagreement they can reach is `flat says tripped, nothing " + "namespaced does` - which is a latch written by a version that had no " + "namespaces, i.e. a trip taken while this install was ROLLED BACK", + _pre.data["sign_tripped"] is False and _pre.data["sign_reason"] == "") + +# ⚠️ THE ROLLBACK ROUND TRIP, RUN END TO END. THIS IS THE STATE THE `rep.ran` +# GATE SWALLOWED, and it is not a thought experiment - a firmware version change +# with a rollback path was performed on this site on 2026-09-02. store.py's +# rule 2 keeps the flat keys "so a rollback still finds its latch"; the operator +# path that rule contemplates is a ROUND TRIP, and the guard used to return +# early on every start that did not migrate, which is every start after the +# first. +# +# 1. install migrates -> schema_version=1, unit/inverter-a/... = False +# 2. operator DOWNGRADES, and SAFETY-02 trips inside the downgrade window. +# The old add-on knows only the flat key, so that is all it writes. +# 3. operator upgrades again. `rep.ran` is False - the store is already at +# schema 1 - so the migration and the guard were both skipped and +# `Inverter.__init__` read the namespaced key, which is still False. +# +# Observed before this fix: the controller came up with `sign_wd.tripped = +# False` while /data/state.json said `sign_tripped: true`. That is store.py's +# own header sentence - the file still says tripped, the banner is green, and +# the controller resumes driving the wrong way - rebuilt by the version marker. +_trip = MemStore() +_c2r0, _ = built(store=_trip) # 1. install, migrate +check("PRECONDITION 1: the install migrated and came up armed and untripped", + _trip.data[SCHEMA_KEY] == SCHEMA_VERSION + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False + and not _c2r0.inverters[0].sign_wd.tripped) +# 2. THE DOWNGRADE WINDOW. The old add-on is the one that shipped before this +# card: it writes the flat pair and knows nothing about `unit/...`. Written the +# way that version wrote them - two `Store.set()` calls - rather than by +# reaching into `data`, so this is the file an old add-on would leave behind. +_trip.set("sign_reason", "SIGN_DISAGREEMENT: commanded -1800 W against a " + "meter reading +2100 W for 181 s") +_trip.set("sign_tripped", True) +check("PRECONDITION 2: the rolled-back add-on latched under the flat key ONLY, " + "and left the namespaced one exactly as the migration wrote it. This is " + "the state that made `rep.ran` a trap - the store is already at schema " + "1, so nothing on the next start has any reason to run", + _trip.data["sign_tripped"] is True + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False + and _trip.data[SCHEMA_KEY] == SCHEMA_VERSION) +_c2r, _ = built(store=_trip) # 3. upgrade again +print(f" the rollback round trip: {_c2r.store_migration.explain()}; " + f"inverter-a came up tripped={_c2r.inverters[0].sign_wd.tripped}") +check("PRECONDITION 3: this start did NOT migrate anything, which is exactly " + "why the guard used to be skipped on it", + not _c2r.store_migration.ran) +check("⚠️ AND THE LATCH IS CAUGHT ANYWAY. The guard runs on EVERY start now, " + "so a trip taken while the install was rolled back is re-armed instead " + "of being silently discarded by the version marker. A latch that a " + "version change disarms is worse than one that was never persisted, " + "because the state file is evidence saying it is fine", + _c2r.inverters[0].sign_wd.tripped + and "SIGN_DISAGREEMENT" in _c2r.inverters[0].sign_wd.reason + and "-1800 W" in _c2r.inverters[0].sign_wd.reason) +check("...and it is persisted under the NAMESPACED key as it re-arms, so the " + "next start reads it from the live spelling rather than re-deriving it", + _trip.data[unit_key(UNIT_A, "sign_tripped")] is True) +check("...and the operator is told in the action log, because a latch re-armed " + "on their behalf that only appears in a banner is a 0 W hold with no " + "explanation", + any("re-armed" in e for e in _c2r.events)) +run(_one_cycle_early(_c2r)) +check("...and it commands 0 W on its first cycle, which is what makes the " + "re-arm a latch rather than a log line", + _c2r.target == 0.0 and "failsafe" in _c2r.reason) +run(_c2r.handle_action("auto_toggle")) +_c2r2, _ = built(store=_trip) +check("⚠️ AND THE OPERATOR CAN STILL GET OUT OF IT - ONCE. The press clears " + "both spellings, so the next start does NOT re-arm. This is the " + "'re-arm forever' objection the `rep.ran` gate existed to answer, " + "answered where it belongs: by clearing the fossil rather than by " + "refusing to look at it", + not _c2r2.inverters[0].sign_wd.tripped + and _trip.data["sign_tripped"] is False + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False) + +# 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) +check("⚠️ ...AND THE TRIP IS MIRRORED ONTO THE PRE-DEV-02 FLAT PAIR, WHICH IS " + "THE ROLLBACK VIEW. store.py's rule 2 keeps the flat keys 'so a rollback " + "still finds its latch', and that only ever covered the latch present AT " + "MIGRATION TIME: nothing flat was written for a trip taken afterwards, so " + "an operator who downgraded found `sign_tripped: false` and came up " + "DRIVING, into the fault. The site-level spelling is right for it - the " + "version being rolled back TO drives one actuator", + _s3.data.get("sign_tripped") is True + and "SIGN_DISAGREEMENT" in str(_s3.data.get("sign_reason", ""))) + +# 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")] == "") +check("...and the same press cleared the flat rollback view, so a downgrade " + "after a clear does not find a trip the operator has already dealt with " + "- and neither does the next start, which now reads that key on EVERY " + "boot rather than only on the one that migrates", + _s3.data["sign_tripped"] is False and _s3.data["sign_reason"] == "") +_c3r2, _ = built({"sign_disagree_w": 300, "sign_disagree_s": 1}, store=_s3, + hass=_h3b) +check("⚠️ AND THE RESTART AFTER THAT CLEAR COMES UP UNLATCHED. This is the " + "'re-arm forever' hazard the every-start guard would have if the clear " + "did not reach both spellings, driven through the real button and a real " + "reconstruction rather than argued", + not _c3r2.inverters[0].sign_wd.tripped + and not any("re-armed" in e for e in _c3r2.events)) + + +# ============================================================================== +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()) + # SAFETY-28's gate is upstream of the clamp, so the out-of-range request + # has to be one the allocator actually placed or it is substituted with + # 0 W and this part grades the gate instead of the clamp. + arm(c, {"inverter-a": 9999.0, "inverter-b": 9999.0}) + 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 + arm(c, {"inverter-a": 500.0, "inverter-b": 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 + arm(c, {"inverter-a": 500.0, "inverter-b": 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 + arm(c, {"inverter-a": 500.0, "inverter-b": 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) + + +# ⚠️ 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") + +# ⚠️ 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'"), + # ⚠️ THE THREE ROWS BELOW ARE ONE DEFECT SEEN FROM THREE SIDES, and it was + # found by review rather than by a check: the latch was silently DISARMED + # by the rollback ROUND TRIP store.py's rule 2 exists for. The guard used to + # return early on `not rep.ran`, which is every start after the first, so a + # trip taken inside a downgrade window was discarded by the version marker. + ("`_grade_store_migration()` gated on `rep.ran` again", + "test_inverter.py part 2, the ROLLBACK ROUND TRIP: 'AND THE LATCH IS " + "CAUGHT ANYWAY'. Every other check in that part passes with the gate " + "restored, because every other one runs on the start that migrates"), + ("the operator's clear leaving the pre-DEV-02 flat keys set", + "test_inverter.py part 2: 'THE OPERATOR CAN STILL GET OUT OF IT - ONCE' " + "and part 3's 'the restart after that clear comes up UNLATCHED' - i.e. " + "the re-arm-forever hazard, which is why the `rep.ran` gate existed"), + ("a trip writing only the namespaced keys, not the flat rollback view", + "test_inverter.py part 3: 'THE TRIP IS MIRRORED ONTO THE PRE-DEV-02 FLAT " + "PAIR'. A downgrade after a post-migration trip found no latch at all"), + ("the guard asking only `self.inverters[0]` whether anything is latched " + "instead of asking the STORE - so B's own mirrored trip re-arms A", + "test_inverter.py part 3: 'a brand new Controller comes back with B " + "latched and A NOT' - the flat-key defect returning through its own guard"), + ("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"), + # ⚠️ THE ROW ABOVE IS THE *ACTUATOR* VARIANT AND IT WAS THE ONLY ONE HERE + # FOR A WHOLE REVIEW ROUND. The BOUND variant below routes every claim + # correctly and builds the NUMBERS from the wrong unit - a different defect + # with a different fix, invisible to the routing pin. It survived all 19 + # files, because test_peakguard.py's AC 4 check compared B's bound against + # B's `max_w` while the fixture defaulted B's `max_w` to A's. Both halves + # are fixed: B now has a different rating and the bound is asserted against + # literal numbers. ANY CHECK ASSERTING "X DIFFERS FROM Y" MUST BE RUN ONCE + # WITH X AND Y ACTUALLY MADE DIFFERENT. + ("the per-unit `device`/`supervised` BOUNDS read off `self.inverters[0]` " + "while the `actuator=` stays `inv` - right envelope, wrong numbers", + "test_peakguard.py 10g: 'AND EACH UNIT'S supervised BOUND IS ITS OWN " + "max_w' and the two lines under it, which read literal 5000/3000 rather " + "than an expression that can compare a number against itself"), + ("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 +# 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) == 24) + + +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..6c1e8c0 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,57 @@ 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 HYPOTHESIS: 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 +# whose number each per-unit bound is built from. +# +# ⚠️ AND IT IS 3000 W RATHER THAN A's 5000 W, WHICH IS NOT COSMETIC. THE RULE, +# STATED AT THE LINE THAT NEEDED IT: ANY CHECK ASSERTING "X DIFFERS FROM Y" MUST +# BE RUN ONCE WITH X AND Y ACTUALLY MADE DIFFERENT. This helper used to default +# B's rating and device range to A's `LIVE_MAX_W`, and the AC 4 check below +# compared inverter B's `supervised` bound against `_b2.max_w` - which was +# LITERALLY THE SAME NUMBER as A's. It read as evidence and observed nothing: +# filing both per-unit claims from `self.inverters[0]`'s numbers instead of +# `inv`'s left all 19 files green. That is the third check this session whose +# DATA could not reach the failure it named (ARB-03's call-site scan saw 3 of 8 +# sites; ALLOC-01's replay generator saw 0 escapes where a widened one saw +# 39,991), so the number is different HERE, once, for every fixture in the file. +B_RATING_HYPOTHESIS = LIVE_MAX_W - 2000.0 + + +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().""" + rating = B_RATING_HYPOTHESIS if max_w is None else max_w + inv = Inverter(name, setpoint_entity=setpoint, soc_entity="sensor.soc_b", + batt_entity="sensor.batt_b", max_w=rating, store=ctl.store) + inv.dev_min, inv.dev_max = dev or (-rating, rating) + # 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 +2630,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 +2685,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 +2725,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 +2754,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 +2775,73 @@ 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"}) +# ⚠️ AND NOW THE HALF THAT WAS EVIDENCE-SHAPED AND OBSERVED NOTHING. The three +# checks above grade WHICH ACTUATOR each claim lands on. They say nothing about +# WHOSE NUMBERS the bound is built from - and those are two different defects +# with two different fixes. Filing the per-unit `device` and `supervised` claims +# from `self.inverters[0]`'s numbers while leaving `actuator=self.actuator_of( +# ..., inv)` alone routes every claim correctly and bounds inverter B by +# inverter A's rating: at A=5000/B=3000 the planned envelope for inverter-b +# becomes (-5000, 5000) instead of (-3000, 3000), which is a 2000 W +# over-command the arbiter believes is supervised. +# +# ⚠️ THE RULE THIS LINE EXISTS TO OBEY: ANY CHECK ASSERTING "X DIFFERS FROM Y" +# MUST BE RUN ONCE WITH X AND Y ACTUALLY MADE DIFFERENT. The version this +# replaces compared inverter B's bound against `_b2.max_w` while +# `second_inverter()` defaulted `max_w` to A's `LIVE_MAX_W` - so it compared +# 5000 against 5000, and the mutation above left all 19 files GREEN. The +# PRECONDITION below is what stops that happening again: it fails if the two +# ratings are ever made equal, so the check cannot go quietly vacuous. +_sup2 = {c.actuator: (c.lo, c.hi) for c in _cl2 if c.source == "supervised"} +_dev2 = {c.actuator: (c.lo, c.hi) for c in _cl2 if c.source == "device"} +print(f" supervised bounds by actuator: {_sup2}; device bounds: {_dev2}") +check("PRECONDITION, AND IT IS THE WHOLE POINT: A AND B HAVE DIFFERENT " + "RATINGS. A check that 'B's bound is B's own number' cannot observe " + "anything while B's number IS A's number, and this file shipped that " + "check for a whole review round", + _b2.max_w != c_src.inverters[0].max_w + and (_b2.dev_min, _b2.dev_max) + != (c_src.inverters[0].dev_min, c_src.inverters[0].dev_max)) +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, and it is now stated " + "against LITERAL, DIFFERENT numbers rather than against an expression " + "that could be reading the same object twice: B is bounded at " + f"+/-{B_RATING_HYPOTHESIS:.0f} W and A at +/-{LIVE_MAX_W:.0f} W", + _sup2 == {"inverter-a": (-LIVE_MAX_W, LIVE_MAX_W), + "inverter-b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS)}) +check("...and so is each unit's `device` bound, which is the other number the " + "same loop files and the one that was NEVER READ before this card - one " + "`dev_min`/`dev_max` pair came off one `setpoint_entity`, so a second " + "inverter's reported range bound nothing at all", + _dev2 == {"inverter-a": (-LIVE_MAX_W, LIVE_MAX_W), + "inverter-b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS)}) +check("...and the bounds the ARBITER RESOLVED for inverter B - not just the " + "claims filed at it - are B's own, because a claim filed correctly and " + "then resolved against the wrong envelope would be the same defect one " + "layer down", + _n2_alloc is not None + and next((p for p in _n2_alloc.plans if p.unit == "inverter-b"), None) + is not None + and (lambda p: p.lo == -B_RATING_HYPOTHESIS + and p.hi == B_RATING_HYPOTHESIS)( + next(p for p in _n2_alloc.plans if p.unit == "inverter-b"))) # --- 10h. ALLOC-01: WHAT MAIN.PY DOES WITH AN ALLOCATION IT CANNOT DELIVER --- @@ -2711,13 +2865,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 +2896,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 +3010,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 +3026,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 +3061,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..2b76ec4 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,24 @@ 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 MIRRORED, AS THE ROLLBACK VIEW. " + "This assertion used to read the other way - that the flat key was NOT " + "written - on the argument that writing it would latch every other " + "inverter on the site. That argument was right about the READ and wrong " + "about the WRITE: nothing at runtime reads the flat key, and the one " + "thing that does (`_grade_store_migration()`, on every start) ignores it " + "whenever ANY namespaced latch is set, which a sibling's trip always " + "leaves. What the absence actually cost was the ROLLBACK: store.py's " + "rule 2 keeps this key so a downgrade finds the latch, and a trip taken " + "after the migration wrote nothing here - so an operator who downgraded " + "came up DRIVING. `test_inverter.py` part 3 is where 'B's trip does not " + "latch A across a restart' is graded, on a real roster of two", + store.data.get("sign_tripped") is True + and "SIGN_DISAGREEMENT" in str(store.data.get("sign_reason", ""))) 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 +474,14 @@ 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"] == "") +check("pressing start DOES clear it, and clears the store with it - BOTH " + "spellings, which is what stops the every-start latch guard re-arming a " + "failsafe the operator has already dealt with", + not post.sign_wd.tripped and store.data[TRIPPED_KEY] is False + and store.data[REASON_KEY] == "" + and store.data.get("sign_tripped") is False) # 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 +503,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 +533,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