From 6c4565659b2c8be1cc1c4dddb7ebcee30965dd4a Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Sun, 6 Sep 2026 13:56:26 +0200 Subject: [PATCH 1/2] SAFETY-13: the output freeze gets an exit, and only where it needs one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the loop is frozen the command's magnitude may fall but never rise. At prev_w == 0 there is no magnitude left to fall, so a charge cannot START. That refusal is deliberate and it is kept: the inverter reporting power we did not command means another writer is on the actuator, which is the §1.5 non-goal. What it did not have was an END - nothing cleared it but the other writer leaving. Reproduced on release/1.0 @ c8dcb38, at the live install's shape (max_w 5000, slew_w 400, target_grid_w -10), with another writer holding the pack at +700 W. The route is not the obvious one: entering from an export, the slew limit moves the command off zero before the counter reaches 3. It is entered from the DEADBAND - a site resting at its target counts saturated cycles while nothing is wrong - and 0 W is then a fixed point: a 2 kW export arriving afterwards leaves the command at exactly 0.0 W on every subsequent cycle. And prev_w == 0 is not where it stops. The same tuning pins at -800 W against its own integrator standing at -2388 W, with the same absence of an exit. So the change is written against the output freeze, not against a special case of it. THE DECISION IS WHICH FREEZES GET AN EXIT, NOT HOW LONG THE BLOCK LASTS. - A divergence a LIMIT could have produced - the inverter doing less than the command, in the command's own direction - clears itself, because the command is pinned at something the inverter can reach. No exit. release/1.0 unchanged, and asserted so: a curtailed inverter is never released over 30 cycles. - A divergence a limit could NOT have produced - overshooting the command, or the opposite sign - cannot clear itself, because clearing it needs the inverter to track OUR command. That one releases. unasked_power() is that predicate and it is physical, not statistical: a limit can only make the inverter do less, in the command's own direction, so everything a limit can produce lies in the interval spanned by 0 and prev_w widened by saturation_w. At prev_w == 0 it IS the saturation test, which is exactly why the zero case is the one that strands. The bound is not a new number. SETTLE_CYCLES = ceil(PLANT_SETTLE_S / LOOP_TICK_S) = 9, SAFETY-39's two measured constants and the reciprocal of SLEW_RAIL_FRAC: a divergence still standing after the plant's own settling time is not this plant catching up. _SAT_COUNT_CAP is written as SETTLE_CYCLES + 1; its value is unchanged at 10. RELEASING THE OUTPUT FREEZE DOES NOT UNFREEZE THE LOOP, which is what makes this bounded. The integrator freeze is untouched, so a released cycle can only move the command toward an accumulator that stopped accumulating, by at most one slew step, and never past it - swept over 107 625 released states. On the run above the command starts on the ninth saturated cycle and settles at -1190 W against a frozen integrator of -1194 W, not at the 5000 W rail. Graded against a transcription of release/1.0 over the 3024-case sweep, which was NOT taught the new rule: 2592 cases bit-identical, 432 releases, and 27 of those change the command - every one a charge from prev_w == 0 that release/1.0 refused to start. SAFETY-04's deliberate assertion is kept rather than inverted: its fixture sits below the bound, so it still asserts the block, and the release is asserted beside it on the other side of the bound. Coverage audit re-measured, same mutation form, __pycache__ purged: output freeze 2 -> 11 (2 reproduced on a pristine c8dcb38 first), the exit 4, unasked_power 6. saturation_cycles at 9 or 10 removes the block entirely, because the freeze cannot engage before its own exit is due. Not refused at startup; asserted in the tests and written in DOCS.md. Nothing was measured on two inverters: inverter B does not physically exist (NOTES.md §1.1b) and sim/plantsim.py:216 INVERTER_B is INVERTER_A copied with measured=False. The N=2 escalation is stated from the code (actual_w is site_batt_w, the SUM) and is labelled as such. 22/22 test files pass in goodwe-addon-ci, mounted :ro, bytecode purged first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 45 +++ .../SAFETY-13-bounding-the-output-freeze.md | 276 ++++++++++++++++++ goodwe_controller/DOCS.md | 49 +++- goodwe_controller/app/control.py | 144 ++++++++- goodwe_controller/test_control.py | 260 ++++++++++++++++- 5 files changed, 761 insertions(+), 13 deletions(-) create mode 100644 goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index bd2141a..fd5bc9c 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -31,6 +31,51 @@ guessing a boundary; saying so is more useful than a guess. ## Unreleased +⚠️ **THE OUTPUT FREEZE NOW HAS AN EXIT, AND ONLY WHERE IT CANNOT END ON ITS OWN +([[SAFETY-13]]).** While the loop is frozen the command's magnitude may fall but +never rise; at `prev_w == 0` there is no magnitude left to fall, so a charge +cannot start. That refusal is deliberate - the inverter reporting power we did +not command means another writer is on the actuator, the §1.5 non-goal - and it +is kept. What it did not have was an END: nothing cleared it but the other +writer leaving. + +The exit is now explicit, and *which* freezes get one is the decision rather +than *how long*: + +- A divergence a **limit** could have produced - the inverter doing less than + the command, in the command's own direction - **clears itself**, because the + command is pinned at something the inverter can reach. It gets no exit, and + that is release/1.0's behaviour kept unchanged. +- A divergence a limit could **not** have produced - the inverter overshooting + the command, or producing the opposite sign - cannot clear itself, because + clearing it needs the inverter to track *our* command. That one releases once + the divergence has outlasted `PLANT_SETTLE_S`, i.e. `SETTLE_CYCLES` = 9 cycles + at the measured tick. No new constant: it is the reciprocal of SAFETY-39's + `SLEW_RAIL_FRAC`. + +⚠️ **RELEASING THE OUTPUT FREEZE DOES NOT UNFREEZE THE LOOP.** The integrator +freeze is untouched, so the command can only walk, one slew step per cycle, +toward an accumulator that stopped accumulating when the freeze engaged. +Measured at the live install's shape (`max_w 5000`, `slew_w 400`, +`target_grid_w -10`) with another writer holding the pack at +700 W and 2 kW of +export standing: the command holds at 0 W, starts on the ninth saturated cycle +and settles at **-1190 W** against a frozen integrator of -1194 W, not at the +5000 W rail. On `release/1.0` @ `c8dcb38` the same run held **0.0 W on every +cycle**, for as long as the other writer stayed. + +⚠️ **THIS IS NOT A REGRESSION FIX AND IT ESCALATES AT TWO INVERTERS.** The +unbounded block is `release/1.0` behaviour, confirmed cycle for cycle against +SAFETY-04's branch. At a roster of two, `actual_w` is the SITE SUM, so the +comparison is a site command against two units' power and one unit still moving +after the other has stopped can put the site into the block with no external +actor at all. + +Graded against `release/1.0` over the 3024-case sweep in `test_control.py`: +**2592 cases bit-identical, 432 releases, and 27 of those change the command** - +every one of them a charge from `prev_w == 0` that `release/1.0` refused to +start. `saturation_cycles` at 9 or 10 removes the block entirely; DOCS.md says +so. + ⚠️ **A PER-INVERTER POWER READING THAT HAS STOPPED FOLLOWING ITS OWN COMMAND IS NOW `None`, NOT A HELD NUMBER ([[SAFETY-37]]).** `Inverter.batt` (register `0x0518`) goes through a freshness gate: unchanged for >= 35 s *while* the diff --git a/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md b/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md new file mode 100644 index 0000000..708380d --- /dev/null +++ b/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md @@ -0,0 +1,276 @@ +# SAFETY-13 — the output freeze gets an exit, and only where it cannot end on its own + +**Status:** decided and implemented on branch `SAFETY-13`, off `release/1.0` @ `c8dcb38`. +**Card:** SAFETY-13 — *"Bound the no-start-while-saturated output block"* +([Trello](https://trello.com/c/uZjmuF7B)). +**Nothing here reached the inverter.** Every figure below is from the pure control +law run in a container against a plant model, on this branch and on a pristine +`c8dcb38` for the before-figures. No rig, no ESP32, no Home Assistant. + +--- + +## 0. The four things a reader should take away + +1. **The card's headline case is real, and the route to it is not the obvious + one.** The block does not engage on the way down into an export; the slew + limit moves the command off zero before the counter reaches 3. It engages + from the **deadband** — a site resting at its target with another writer on + the bus, counting saturated cycles while nothing is wrong — and 0 W is then a + fixed point. §2. +2. **`prev_w == 0` is not a special case, so the fix is not one either.** The + same run pins at −800 W against its own integrator standing at −2388 W, at a + non-zero `prev_w`, with + the same absence of an exit. Zero is only the instance where the pinned + magnitude is the whole of what the battery could have done. §3. +3. **The decision is not "how long", it is "which".** A divergence a *limit* + could have produced clears itself and gets no exit — `release/1.0` unchanged. + A divergence a limit could not have produced cannot clear itself and gets + one. The predicate is physical, not statistical. §4. +4. **Releasing the output freeze does not unfreeze the loop**, and that is what + makes the release bounded rather than merely different. §6. + +--- + +## 1. What was there, in the terms the code uses + +`control.py`, in `compute()`, after the clamp and the slew limit: + +```python +if frozen: + target = min(target, prev_w) if prev_w > 0 else max(target, prev_w) + reason = "saturated-freeze" +``` + +"Magnitude may fall, never rise." At `prev_w == 0` the magnitude is zero and +`max(target, 0)` forbids any negative command, so a charge cannot **start**. + +The reason is written in the file and it is a good one: `frozen` means the +inverter's reported power has diverged from the command by more than +`saturation_w` for `saturation_cycles` consecutive cycles, and at a command of +zero that means the actuator is producing power nobody in this add-on asked for. +Starting to push the other way into it is the configuration §1.5 of the +specification rules out — *"any configuration in which two writers can reach one +actuator"*. + +**The defect is that it has no end.** Not the error growing, not the integrator +moving, not time. `sat_count` stops rising at its cap and the state is a fixed +point. + +--- + +## 2. Reproducing it — and correcting the route + +Driven at the live install's shape: `max_w 5000`, `slew_w 400`, +`target_grid_w -10`, `saturation_w 500`, `saturation_cycles 3`, `gain 0.6`. +Another writer holds the pack at **+700 W** whatever is commanded. + +**The route that does NOT reach it.** Start at `prev_w = 0` with 2 kW of export +already standing. On `c8dcb38` the loop moves off zero at the slew limit before +the counter reaches 3 — commands −400, −800 — so by the time `frozen` is true +`prev_w` is −800 and the block that gets applied is `max(target, −800)`. The +site charges at 800 W, not 0 W, for as long as the run lasts. + +**The route that does.** The site is *resting*. `grid == target_grid_w`, so the +loop is in its deadband and the command is 0 — and `saturated_now` is computed +**before** the deadband branch, so the counter climbs anyway. Three cycles later +the loop is frozen at `prev_w == 0` with nothing wrong. Then the export arrives: + +| cycle | grid | actual | prev | command | integrator | sat | reason | +|---|---|---|---|---|---|---|---| +| 0–1 | −10 | +700 | 0 | 0 | 0 | 1–2 | `deadband` | +| 2–5 | −10 | +700 | 0 | **0** | 0 | 3–6 | `saturated-freeze` | +| 6 | −2000 | +700 | 0 | **0** | −1194 | 7 | `saturated-freeze` | +| 7… | −2000 | +700 | 0 | **0** | −1194 | 8→10 | `saturated-freeze` | + +On `c8dcb38` the command is `0.0` on **every** cycle from there on. The +integrator has moved to −1194 W and stopped (its own freeze holds it), the site +exports 2 kW, and the battery is idle until the other writer leaves. + +That is the card's sentence — *"the site can sit exporting 2 kW with an idle +battery indefinitely"* — reproduced, with the route corrected: it is entered +from the **deadband**, not from a correction that ran out of room. + +--- + +## 3. …and `prev_w == 0` is not where it stops + +The same tuning, same writer, but the loop starts from zero with the export +already standing (the route from §2 that "does not reach it"): + +| | `c8dcb38` | +|---|---| +| command after the freeze engages | **−800 W**, on every cycle | +| the loop's integrator, frozen | −2388 W | +| what ends it | nothing | + +So the missing exit is a property of the **output freeze**, not of the zero +case. `prev_w == 0` is the instance where the pinned magnitude happens to be +zero. A rule written for zero alone would have left −800 W exactly as unbounded +as it found it — which is why the change below is written against the freeze and +not against a special case of it. + +**And this is the half that escalates at two inverters.** `actual_w` is +`site_batt_w`, the SUM (ALLOC-01, DEV-02), so the comparison is a site command +against two units' power. One unit still moving after the other has stopped is +enough to put the site into the freeze with no external actor at all. + +⚠️ **What that claim is and is not.** The escalation is stated from the code — +`prev_w` is `self.target`, the site total, and `actual_w` is the site sum — and +from the single-unit runs above. **It is not measured on two inverters.** +Inverter B does not physically exist (`NOTES.md` §1.1b: on paper, from datasheet +and serial), and `sim/plantsim.py:216` `INVERTER_B` is `INVERTER_A` copied with +`measured=False`. Any two-inverter number would be two copies of A. + +--- + +## 4. The decision: which freezes get an exit, not how long the block lasts + +Three answers were on the table, and the card named them: lift after N frozen +cycles, exempt above some export magnitude, or keep it unbounded and write down +the cost. + +**A plain cycle bound was rejected**, because it bounds the wrong thing. The +freeze is right to be permanent in one case and cannot be permanent in the +other, and a single N treats them alike: + +| the inverter is… | can the freeze clear itself? | +|---|---| +| doing **less** than the command, in the command's own direction — a cold pack, a curtailment, an export cap, the plant still ramping | **Yes.** The command is pinned at something the inverter can reach, so it catches up, `saturated_now` goes false, and the freeze ends. Asking for more would achieve nothing anyway. | +| doing something **outside** what the command can explain — overshooting it, or the opposite sign | **No.** Clearing the freeze requires the inverter to track *our* command, and the other writer is precisely what stops that. | + +**An export-magnitude exemption was rejected** for a narrower reason: it makes +the block's end depend on how big the other actor's disturbance happens to be, +which is not a property of the thing the block is about. + +So the rule implemented is: + +> The output freeze applies as before, **unless** the inverter's reported power +> cannot be explained by the command not being met **and** the divergence has +> outlasted the plant that would otherwise have explained it. + +### 4.1 The predicate is physical + +`unasked_power(prev_w, actual_w, saturation_w)`. A limit can only make the +inverter do **less** than the command, in the command's own direction. It cannot +make it overshoot the command and it cannot make it produce the opposite sign. +So everything a limit can produce lies in the closed interval spanned by `0` and +`prev_w`, widened by `saturation_w` for noise, ramp lag and idle draw. Anything +outside was put there by something else. + +| prev | actual | interval | verdict | +|---|---|---|---| +| +2000 | +700 | [0, 2000] | a limit | +| −5000 | −700 | [−5000, 0] | a limit | +| 0 | +700 | [−500, +500] | **unasked** | +| −800 | +700 | [−1300, +500] | **unasked** | + +⚠️ **At `prev_w == 0` this predicate and the saturation detector are the same +test** — asserted in `test_control.py` over 801 readings. That is not a +coincidence to tidy away: with nothing commanded there is no interval left for a +limit to live in, which is exactly why the zero case is the one that strands. + +⚠️ **It is not a detector and nothing alarms on it.** A stale, wrong-signed or +noisy reading lands outside the interval too. All it decides is whether the +freeze gets an exit; being wrong about it costs a bounded command (§6). +SAFETY-30's `unit_sign_disagree_w` is the thing that alarms on sign, and it is +deliberately a different test with a different threshold. + +### 4.2 The bound is not a new number + +`SETTLE_CYCLES = ceil(PLANT_SETTLE_S / LOOP_TICK_S)` = **9**. + +`PLANT_SETTLE_S` (18.0 s) and `LOOP_TICK_S` (2.0 s) are SAFETY-39's two measured +constants, already in this file. A divergence still standing after the plant's +own settling time is not this plant catching up — so after `SETTLE_CYCLES` the +plant has run out of alibi. It is the reciprocal of `SLEW_RAIL_FRAC`: the slew +limit takes exactly this many ticks to cross the rail, for exactly this reason. + +⚠️ **It inherits SAFETY-39's caveat about `LOOP_TICK_S`, in the opposite +direction.** A meter **slower** than 2 s makes 9 cycles cover more than 18 s, so +the freeze holds longer than the plant needs: that costs energy, never safety. A +meter **faster** makes them cover less, so the freeze can release while the +plant is still moving — bounded by §6 rather than by luck. + +`_SAT_COUNT_CAP` is now written as `SETTLE_CYCLES + 1`. Its value is unchanged +(10, exactly as shipped); writing it against the bound stops a counter capped +below the longest bound read from it, which would silently delete the exit. + +--- + +## 5. What actually changed, graded against `release/1.0` + +`test_control.py`'s 3024-case sweep still compares this law against a +transcription of `release/1.0` — the transcription was **not** taught the new +rule, because an oracle taught the change compares the law against itself. What +changed is the assertion: every case must be identical, or be a release with +both of its conditions true. + +| | cases | +|---|---| +| total | 3024 | +| **bit-identical to `release/1.0`** | **2592** | +| releases | 432 | +| …of those, reason-only (the freeze was not binding) | 405 | +| …of those, **the command changes** | **27** | + +All 27 are at `prev_w == 0`, all with `release/1.0` commanding exactly `0.0 W`, +and every new command is **negative** — a charge start that was refused. Nothing +else in the sweep moved. + +The behaviour after the fix, on §2's run: the command holds at 0 W through +`sat_count` 3–8, releases on the **ninth** saturated cycle, and walks +0 → −400 → −800 → **−1190 W**, where it stops. + +The genuine-limit case, driven for 30 cycles with the inverter curtailed to +±1000 W against a 4 kW import: `freeze-released` never appears and the command +stays pinned at +2400 W. Unchanged, deliberately. + +--- + +## 6. Why the release cannot become windup (the card's fourth criterion) + +**The integrator freeze is not lifted with the output freeze.** The command is +derived from `i_w`, then clamped, then slew-limited from `prev_w` — so a +released cycle can only move the command *toward* an accumulator that stopped +accumulating when the freeze engaged, by at most one slew step, and never past +it. Swept over 107 625 released states in `test_control.py`: no case leaves the +interval between the previous command and the frozen integrator, and no case +moves further than one slew step. + +On §2's run that shows up as the settling point: **−1190 W against a frozen +integrator of −1194 W**, not the 5000 W rail. The tech lead's note on SAFETY-04 +— that the carried `i_w` and `release/1.0`'s recomputed value land on the same +slew-limited command — is preserved: the `i_w=None` path is still graded against +the transcription, cycle for cycle, everywhere except the 432 releases. + +⚠️ **The first form of this check was wrong and the sweep said so**, which is +left in the test file. It asserted `|target| <= |i_w|` on a released cycle and +produced `prev −5000, i_w −3000, target −4600`: the command was walking back +from −5000 toward the integrator and had not arrived. It was moving the right +way and the assertion still failed, because "never exceeds the integrator" is +not what no-windup means here. + +--- + +## 7. What this does NOT buy, said plainly + +**The site still does not reach its target while another writer is on the bus.** +It settles at the frozen integrator, not at the meter's target. What the exit +buys is that the battery stops being *idle* while the site exports; it does not +buy correct regulation against an actuator somebody else is also writing to, and +nothing in a rules engine can. A standing grid error with a second writer +present is the symptom of a configuration this add-on refuses to support. + +**`freeze-released` in the action log means "look for the second writer."** It is +carried on `Decision.reason`, which nothing after the freeze overwrites. No new +alarm was added: SAFETY-41 removed one that narrated normal anti-windup, and +this state is not yet known to be worth waking anybody for. + +**One configuration removes the block entirely.** `saturation_cycles` is +accepted up to 10 by `config.yaml`. At 9 or above the freeze cannot engage +before its own exit is due, so the no-start block never happens. This is +**not** refused at startup — a controller that will not boot over a tuning +nuance is worse than the nuance — but it is asserted in `test_control.py` and +written in DOCS.md so that it is a known consequence and not a surprise. + +**Nothing was measured on two inverters.** See the warning in §3. diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index a4d28f8..1380344 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -473,7 +473,7 @@ behind it would trip the firmware watchdog on a system that is working fine. | `target_grid_w` | -10 | What the meter should rest at. Negative = a slight export | | `step_w` | 10 | Quantisation | | `saturation_w` | 500 | Divergence that counts as "the inverter is at a limit" | -| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one meter update or `cycle_floor_s`, whichever comes first - see the note below. **Do not set to 1** | +| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one meter update or `cycle_floor_s`, whichever comes first - see the note below. **Do not set to 1**, and **do not set it to 9 or 10** either: at 9 the freeze cannot engage before its own exit is due, so the no-start block stops existing (SAFETY-13, below) | | `integrator_max_w` | 0 | Bound on the loop's accumulator, and 0 means "same as `max_w`". Caps how much stale error can be waiting to unwind when the sign flips. **Do not raise it above `max_w`** - the output clamp already bounds what is commanded, so the only thing extra headroom buys is more cycles of wrong-direction power after every saturation event. Lowering it below `max_w` is the useful direction | | `heartbeat_s` | 10 | Refresh interval; must stay well under the firmware watchdog | | `cycle_floor_s` | 15 | The longest the loop may go without evaluating. A deadline on the gap between cycles, reset by every cycle — on a healthy meter it never elapses and changes nothing. Raise it only if your meter is slower than ~10 s; it must stay under `meter_max_age_s`, past which there is no reading to evaluate anyway | @@ -628,6 +628,53 @@ correct toward that wrong value instead of holding a stale command. It is bounde happened to a lying meter whose value merely jittered — bit-exact repetition was never a safety property. +#### When the freeze lets go, and when it deliberately never does (SAFETY-13) + +While the loop is frozen the command's **magnitude may fall but never rise**. That is +the anti-windup rule, and at `prev_w == 0` it reads as a refusal to *start*: with +nothing commanded there is no magnitude left to fall, so a charge cannot begin. That +refusal is deliberate. Commanding 0 W while the inverter reports hundreds of watts +means something is driving the actuator that this add-on did not ask, and starting to +push power the other way into it is how two controllers end up fighting over one +actuator — the configuration §1.5 of the specification rules out. + +**What was missing was an end to it.** Nothing cleared the block: not the error growing, +not the integrator moving, not time. The exit is now explicit, and *which* freezes get +one is the decision: + +| the inverter is… | can it clear itself? | what happens now | +|---|---|---| +| doing **less** than the command, in the command's own direction — a cold pack, a curtailment, an export cap | **yes.** The command is pinned at something the inverter can reach, so it catches up and the freeze clears | unchanged: frozen for as long as it lasts, with no exit | +| doing something **outside** what the command can explain — overshooting it, or the opposite sign | **no.** Clearing the freeze needs the inverter to track *our* command, and the other writer is what stops it | the freeze releases once the divergence has outlasted the plant that could otherwise have explained it | + +The second row's threshold is not a new number. `PLANT_SETTLE_S` (18 s, measured) is how +long this plant takes to settle, so a divergence still standing after that is not this +plant catching up; at the measured 2 s meter tick that is **9 cycles**, which is the +same relationship the slew limit already uses (`control.py`, `SETTLE_CYCLES`). + +**Releasing the output freeze does not unfreeze the loop.** The *integrator* freeze is +untouched, so the command is free to move only as far as an accumulator that stopped +accumulating cycles ago, one slew step per cycle. Measured on the simulator at the live +install's shape (`max_w 5000`, `slew_w 400`, `target_grid_w -10`), with another writer +holding the pack at +700 W and 2 kW of export standing: the command holds at 0 W while +the counter climbs, starts at the ninth saturated cycle, and settles at **−1190 W** +against a frozen integrator of −1194 W — not at the 5000 W rail. Before this change the +same run held 0.0 W on every cycle, for as long as the other writer stayed. + +⚠️ **The site still does not reach its target in that state, and it is not supposed +to.** What the exit buys is that the battery stops being idle while the site exports; +what it does not buy is correct regulation against an actuator somebody else is also +writing to. A standing grid error with another writer on the bus is the symptom of a +configuration this add-on refuses to support, not a tuning problem. If you see +`freeze-released` in the action log, look for the second writer. + +⚠️ **One configuration removes the block entirely, silently.** `saturation_cycles` is +accepted up to 10. Set it at or above 9 and the freeze does not engage until the bound +has already passed, so the no-start block never happens at all. This is not refused at +startup — a controller that will not boot over a tuning nuance is worse than the nuance +— but it is why the default is 3 and why the table above says **do not set to 1** at one +end and leaves you this note at the other. + #### Why `target_grid_w` is not zero The deadband is a one-way ratchet: any resting point inside it holds until diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index e41cef5..8192a03 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -16,6 +16,7 @@ the short version is in the comments below. Do not "clean this up" - each rule exists because its absence produced a specific, observed failure. """ +import math from dataclasses import dataclass # --------------------------------------------------------------------------- @@ -86,6 +87,56 @@ PLANT_SETTLE_S = 18.0 LOOP_TICK_S = 2.0 SLEW_RAIL_FRAC = LOOP_TICK_S / PLANT_SETTLE_S # 0.111..., i.e. 9 ticks +# --------------------------------------------------------------------------- +# SAFETY-13. HOW LONG THE PLANT STAYS A CANDIDATE EXPLANATION. +# +# The output freeze below stops the command growing while the inverter is not +# following. Two completely different things produce that divergence and the +# freeze cannot tell them apart on magnitude alone: +# +# (a) THE PLANT HAS NOT CAUGHT UP YET, or it is at a limit - a cold pack, a +# curtailment, an export cap. Asking for more is pointless, and the +# freeze is right to hold. It also CLEARS ITSELF: the command is pinned +# at something the inverter can do, so the inverter catches up to it and +# `saturated_now` goes false. +# +# (b) SOMETHING ELSE IS DRIVING THE ACTUATOR - the §1.5 non-goal, two writers +# on one actuator. The freeze CANNOT clear itself here, because clearing +# it needs the inverter to track OUR command and the other writer is what +# stops it. Nothing ends this but the other writer going away. +# +# ⚠️ SO THE BOUND BELONGS TO (b) AND ONLY TO (b), and this constant is what +# separates them in TIME rather than by guessing. Case (a) is bounded by the +# plant: PLANT_SETTLE_S is how long this plant takes to settle, measured, and +# after that the plant has run out of alibi. A divergence still standing after +# PLANT_SETTLE_S is not this plant catching up. +# +# SETTLE_CYCLES = ceil(PLANT_SETTLE_S / LOOP_TICK_S) +# +# ⚠️ THE SAME TWO MEASURED NUMBERS SAFETY-39 USES, AND DELIBERATELY NOT A NEW +# ONE. It is the reciprocal of SLEW_RAIL_FRAC - the slew limit takes exactly +# this many ticks to traverse the rail, for exactly this reason. A new constant +# here would be a second thing to re-derive when the plant is re-measured, and +# a number that outlives its derivation is the defect SAFETY-39 was about. +# +# ⚠️ AND IT INHERITS SAFETY-39's CAVEAT ABOUT LOOP_TICK_S, in the opposite +# direction, which is why it is repeated rather than cross-referenced. A meter +# SLOWER than 2 s makes these 9 cycles cover MORE than 18 s, so the freeze +# holds longer than the plant needs: that costs energy - see the case in +# DOCS.md - and never safety. A meter FASTER than 2 s makes them cover LESS, +# so the freeze can release while the plant is genuinely still moving. That one +# is bounded twice over and by neither this constant nor luck: the INTEGRATOR +# freeze is not released with the output freeze, so the command can only reach +# the integrator's frozen value, and the output clamp bounds that in turn. +SETTLE_CYCLES = math.ceil(PLANT_SETTLE_S / LOOP_TICK_S) # 9 + +# The saturation counter's ceiling. It has always been 10 and still is; it is +# written against SETTLE_CYCLES rather than as a literal so that a counter +# capped BELOW the longest bound read from it - which would silently make that +# bound unreachable, i.e. delete the exit condition - cannot happen by editing +# one of the two. +_SAT_COUNT_CAP = SETTLE_CYCLES + 1 # 10 + @dataclass(frozen=True) class Tuning: @@ -185,6 +236,48 @@ def slew_limit_w(tuning: Tuning) -> float: return SLEW_RAIL_FRAC * abs(float(tuning.max_w)) +def unasked_power(prev_w: float, actual_w: float, saturation_w: float) -> bool: + """True when `actual_w` cannot be explained by the command not being met. + + SAFETY-13. The one question the saturation magnitude test cannot answer: + is the inverter FAILING TO REACH what we asked, or is it doing something we + never asked for at all? + + ⚠️ THE ANSWER IS PHYSICAL, NOT STATISTICAL, WHICH IS WHY IT IS A PREDICATE + AND NOT A THRESHOLD. A limit can only make the inverter do LESS than the + command, in the command's own direction. It cannot make it overshoot the + command, and it cannot make it produce power of the OPPOSITE sign. So the + whole set of readings a limit can produce is the closed interval spanned by + 0 and `prev_w`, widened by `saturation_w` for measurement noise, ramp lag + and the inverter's own idle draw. Anything outside that interval was put + there by something other than this loop. + + Worked, at `saturation_w = 500`: + + prev +2000, actual +700 -> inside [0, 2000] a limit. False. + prev -5000, actual -700 -> inside [-5000, 0] a limit. False. + prev 0, actual +700 -> outside [-500, 500] unasked. True. + prev -800, actual +700 -> outside [-1300, 500] unasked. True. + + ⚠️ AT `prev_w == 0` THIS IS EXACTLY `saturated_now`, and that is not a + coincidence to be tidied away - it is the reason the freeze's zero case is + the one the card was written about. With nothing commanded there is no + interval left for a limit to live in, so every saturated cycle at zero is a + cycle in which the actuator is being driven by someone else. + + ⚠️ IT IS NOT A DETECTOR AND MUST NOT BE ALARMED ON. A saturated inverter + whose reading is stale, wrong-signed or simply noisy lands outside the + interval too. All this predicate decides is whether the output freeze gets + an EXIT; being wrong about it costs at most a bounded command that the + frozen integrator and the output clamp both still bound. SAFETY-30's + `unit_sign_disagree_w` is the thing that alarms on sign, and it is + deliberately a different test with a different threshold. + """ + lo = min(0.0, float(prev_w)) - abs(float(saturation_w)) + hi = max(0.0, float(prev_w)) + abs(float(saturation_w)) + return not (lo <= float(actual_w) <= hi) + + def compute( prev_w: float, grid_w: float, @@ -232,7 +325,7 @@ def compute( # clockless; the remaining ceiling is that saturation_cycles expresses a # bound in seconds rather than an exact number of them. saturated_now = abs(prev_w - actual_w) > tuning.saturation_w - sat_count = min(sat_count + 1, 10) if saturated_now else 0 + sat_count = min(sat_count + 1, _SAT_COUNT_CAP) if saturated_now else 0 frozen = sat_count >= tuning.saturation_cycles # --- deadband ---------------------------------------------------------- @@ -346,8 +439,53 @@ def compute( # implementation: commanding 0 while the inverter reports >500 W means # something else is driving the bus, and that is not the moment to # start pushing power the other way. - target = min(target, prev_w) if prev_w > 0 else max(target, prev_w) - reason = "saturated-freeze" + # + # --- SAFETY-13: ...AND IT IS NOT THE MOMENT FOREVER EITHER ---------- + # Everything above stands. What was missing is an END. Held at + # `prev_w == 0` this is a refusal to START, and nothing in this + # function or above it clears it - not the error growing, not the + # integrator moving, not time. Measured on release/1.0 @ c8dcb38, at + # the live install's shape (max_w 5000, slew_w 400, target_grid_w -10): + # a site resting in the deadband with another writer holding the pack + # at +700 W reaches sat_count 3 with the command at 0, and 0 W is then + # a FIXED POINT - a 2 kW export arriving afterwards leaves the command + # at exactly 0.0 W on every subsequent cycle, with the integrator + # standing at -1194 W the whole time. + # + # ⚠️ AND THE ZERO CASE IS NOT SPECIAL, WHICH IS WHY THE FIX IS NOT + # SPECIAL EITHER. The same run pinned at prev_w == -800 W, against its + # own integrator standing at -2388 W, for as long as it ran. + # `prev_w == 0` is only the instance where the pinned magnitude happens + # to be zero - i.e. where the standing cost is the whole of what the + # battery could have done. + # A rule written for the zero case alone would have left the other + # instances exactly as unbounded as it found them. + # + # ⚠️ WHAT DECIDES IS NOT HOW LONG, IT IS WHETHER THE FREEZE CAN EVER + # END ON ITS OWN. See SETTLE_CYCLES and `unasked_power()` at the top: + # a divergence a LIMIT could produce clears itself once the inverter + # catches up to the pinned command, so it needs no exit and gets none - + # unbounded, exactly as shipped. A divergence a limit CANNOT produce is + # another writer, which by construction stops our command ever being + # tracked, so it can only end when the other writer leaves. That is the + # one this releases, and only after the divergence has outlasted the + # plant that would otherwise have explained it. + # + # ⚠️ RELEASING IS NOT UNFREEZING. The INTEGRATOR freeze above is not + # lifted, and that is what makes this bounded rather than merely + # different: the command is free to move, but only as far as an + # integrator that stopped accumulating cycles ago, through the same + # slew limit as any other cycle. It buys back the standing error, not + # the rail. + # + # `reason` is authoritative here in a way it is not elsewhere in this + # function: nothing after this line writes it. + if unasked_power(prev_w, actual_w, tuning.saturation_w) \ + and sat_count >= SETTLE_CYCLES: + reason = "freeze-released" + else: + target = min(target, prev_w) if prev_w > 0 else max(target, prev_w) + reason = "saturated-freeze" # --- quantise ---------------------------------------------------------- # 10 W. The register is 1 W and the inverter reports at 1 W, but its diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 47574b8..8bebc68 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -17,8 +17,9 @@ Read MUTATION.md before you report a survivor. import sys -from app.control import (LOOP_TICK_S, PLANT_SETTLE_S, SLEW_RAIL_FRAC, - Tuning, compute, slew_limit_w, +from app.control import (LOOP_TICK_S, PLANT_SETTLE_S, SETTLE_CYCLES, + SLEW_RAIL_FRAC, Tuning, compute, slew_limit_w, + unasked_power, maintenance_charge_floor, peak_at_risk) # ⚠️ SAFETY-39: `slew_w` IS EXPLICIT HERE NOW, AND IT IS release/1.0's VALUE. @@ -59,6 +60,17 @@ def check(name, cond): # A comment cannot go stale-green, because it never claims to be running. # # Measured at 389d9ec. Numbers are the lead's independent reproduction. +# ⚠️ THE OUTPUT FREEZE ROW WAS RE-MEASURED AT SAFETY-13 AND IT MOVED FROM 2 TO +# 11, because the SAFETY-13 section grades that branch too. Both figures are +# from the SAME mutation form - the whole `target = min(...)/max(...)` line +# removed, `reason` left alone - and 2 was reproduced on a pristine c8dcb38 +# tree before the new figure was taken, so the row moved because checks were +# added and not because the mutation changed shape. The two new rows below it +# are that section's own mechanisms: removing the EXIT restores release/1.0's +# unbounded freeze, and making `unasked_power()` return True gives an exit to +# the limit case as well, which is the half of the decision that has to stay +# unbounded. +# # ⚠️ THE SLEW ROWS WERE RE-MEASURED AT SAFETY-39 AND ONE OF THEM MOVED: the # slew limit was 4 and is now 5, because the SAFETY-39 section grades it too. # The new row is the RAIL DEFAULT specifically - `slew_limit_w()` reduced to @@ -75,7 +87,9 @@ def check(name, cond): # output clamp 3 # slew limit 5 # slew follows the rail (SAFETY-39), `slew_w` read raw 3 -# output freeze 2 +# output freeze 11 +# the output freeze's EXIT (SAFETY-13), freeze made unconditional again 4 +# `unasked_power()` discriminates, replaced by `return True` 6 # deadband 5 # quantisation 2 # saturation detector, `saturated_now = False` 11 @@ -282,8 +296,15 @@ check("frozen at i_w=0.0: an export push moves the integrator", # hundreds of watts means something else is driving the bus. Asserted so that # nobody "fixes" it by accident - the integrator moving is what this ticket # owns, the command rule belongs to the output freeze. +# +# ⚠️ SAFETY-13 KEPT THIS ASSERTION RATHER THAN INVERTING IT, and the fixture is +# why it survived unchanged: `sat_count=3` becomes 4, which is below +# SETTLE_CYCLES, so this is still inside the window in which the block holds. +# SAFETY-13 gave the block an EXIT, not a different opinion about its first +# cycles - see the SAFETY-13 section below, where the same state is driven past +# the bound and the release is asserted on the other side of it. check("frozen at i_w=0.0: the output freeze still blocks a charge from 0 W", - ze.target_w == 0.0) + ze.target_w == 0.0 and ze.sat_count < SETTLE_CYCLES) # Where prev_w is already charging the output freeze does NOT block, and there # the difference reaches the wire: held at 0.0 the integrator abandons the # charge mid-export. @@ -358,6 +379,192 @@ check(f"after saturation ends the command is {tight:.0f} W (<= 1000)", tight <= check(f"headroom above max_w makes that worse ({loose:.0f} W) - hence the default", loose > tight) +print("SAFETY-13: the output freeze has an exit, and only where it needs one") + +# THE SHAPE OF THIS SECTION. release/1.0's output freeze holds the command's +# magnitude down while the inverter is not following, and nothing ends it. At +# `prev_w == 0` the held magnitude is ZERO, so it reads as "refuses to start a +# charge"; that is the case the card was written about, but it is one instance +# of a rule that is unbounded at every prev_w. SAFETY-13 bounds it exactly where +# it CANNOT end on its own - see `unasked_power()` - and nowhere else. + +# --- 1. the predicate, which is the whole of the decision ------------------- +# A limit can only make the inverter do LESS than the command, in the command's +# own direction, so everything a limit can produce lies in the interval spanned +# by 0 and prev_w widened by saturation_w. These are the four cases in the +# docstring, run rather than described. +check("limit: 700 W under a +2000 W command is explainable", + not unasked_power(2000.0, 700.0, 500.0)) +check("limit: -700 W under a -5000 W command is explainable", + not unasked_power(-5000.0, -700.0, 500.0)) +check("unasked: +700 W under a 0 W command is not", + unasked_power(0.0, 700.0, 500.0)) +check("unasked: +700 W under a -800 W command is not", + unasked_power(-800.0, 700.0, 500.0)) +# The band edges belong to the limit side, because saturation_w is the same +# tolerance the detector uses - a reading the detector calls "following" must +# never be called "another writer". +check("the saturation band is inclusive on both edges", + not unasked_power(0.0, 500.0, 500.0) + and not unasked_power(0.0, -500.0, 500.0) + and unasked_power(0.0, 500.01, 500.0)) +# ⚠️ AT prev_w == 0 THE PREDICATE AND THE DETECTOR ARE THE SAME TEST, which is +# why the zero case is the one that strands: with nothing commanded there is no +# interval for a limit to live in. +same = all(unasked_power(0.0, a, 500.0) == (abs(a) > 500.0) + for a in [x * 5.0 for x in range(-400, 401)]) +check("at prev_w == 0 unasked_power IS the saturation test, over 801 readings", + same) + +# --- 2. the bound is derived, not chosen ------------------------------------ +check(f"SETTLE_CYCLES is ceil(PLANT_SETTLE_S / LOOP_TICK_S) = {SETTLE_CYCLES}", + SETTLE_CYCLES == 9 and SETTLE_CYCLES * LOOP_TICK_S >= PLANT_SETTLE_S + and (SETTLE_CYCLES - 1) * LOOP_TICK_S < PLANT_SETTLE_S) +check("...which is the reciprocal of SAFETY-39's SLEW_RAIL_FRAC", + abs(SETTLE_CYCLES - 1.0 / SLEW_RAIL_FRAC) < 1e-9) +# The counter has to be able to REACH the bound or the exit does not exist. +# Driven, not read off the constant: run a saturated state and see how high the +# counter actually goes. +c, sc = 0, 0 +while c < 50: + sc = compute(0.0, -2000.0, 900.0, T, sc).sat_count + c += 1 +check(f"the saturation counter reaches {sc}, which is above the bound", + sc >= SETTLE_CYCLES) + +# --- 3. prev_w == 0: blocked below the bound, released above it ------------- +# The same state either side of the bound. This is the pair the card asks for. +blk = compute(prev_w=0, grid_w=-2000, actual_w=-600, tuning=T, + sat_count=SETTLE_CYCLES - 2, i_w=0.0) +check("prev_w=0: below the bound the charge is still refused", + blk.target_w == 0.0 and blk.reason == "saturated-freeze" + and blk.sat_count == SETTLE_CYCLES - 1) +rel = compute(prev_w=0, grid_w=-2000, actual_w=-600, tuning=T, + sat_count=SETTLE_CYCLES - 1, i_w=0.0) +check(f"prev_w=0: at the bound it is released to {rel.target_w:.0f} W", + rel.target_w < 0.0 and rel.reason == "freeze-released" + and rel.sat_count == SETTLE_CYCLES) +# ...and the released command is the SLEW step from 0, not the rail. A release +# that jumped to the integrator in one cycle would be the windup this ticket +# was told to avoid. +check(f"the released command is exactly one slew step" + f" ({slew_limit_w(T):.0f} W)", + abs(rel.target_w) == slew_limit_w(T)) +# The mirror. Nothing here is sign-specific, so the discharge side must behave +# identically - every freeze rule in this file has a mirror, because the one +# that did not is the defect that got through review. +mblk = compute(prev_w=0, grid_w=2000, actual_w=600, tuning=T, + sat_count=SETTLE_CYCLES - 2, i_w=0.0) +mrel = compute(prev_w=0, grid_w=2000, actual_w=600, tuning=T, + sat_count=SETTLE_CYCLES - 1, i_w=0.0) +check("prev_w=0, discharging: same block, same release, mirrored", + mblk.reason == "saturated-freeze" and mrel.reason == "freeze-released" + and mrel.target_w == slew_limit_w(T)) + +# --- 4. non-zero prev_w, and the two halves are NOT the same -------------- +# (a) A divergence a LIMIT could have produced is never released, however long +# it stands. This is release/1.0's behaviour and it is kept deliberately: the +# freeze clears itself here, because the command is pinned at something the +# inverter can do. +TL = Tuning(max_w=5000.0, slew_w=400.0, target_grid_w=-10.0) +prev, i_w, sat, reasons = 0.0, 0.0, 0, set() +for _ in range(30): + d = compute(prev, 4000.0, max(-1000.0, min(1000.0, prev)), TL, sat, i_w) + prev, i_w, sat, _r = d.target_w, d.i_w, d.sat_count, reasons.add(d.reason) +check("non-zero prev_w, a curtailed inverter: never released over 30 cycles", + "freeze-released" not in reasons and prev == 2400.0) +# (b) A divergence a limit could NOT have produced is released at the bound, +# at a non-zero prev_w exactly as at zero. release/1.0 pinned this run at +# -800 W, against its own integrator standing at -2388 W, for as long as it ran. +prev, i_w, sat, rows = 0.0, 0.0, 0, [] +for _ in range(20): + d = compute(prev, -2000.0, 700.0, TL, sat, i_w) + rows.append((prev, d.target_w, d.i_w, d.reason)) + prev, i_w, sat = d.target_w, d.i_w, d.sat_count +pinned = [r for r in rows if r[3] == "saturated-freeze"] +check(f"non-zero prev_w, another writer: pinned at {pinned[0][1]:.0f} W...", + pinned and all(r[1] == -800.0 for r in pinned)) +check(f"...then released, settling at {prev:.0f} W", prev == -2390.0) + +# --- 5. THE CARD'S OWN CASE, DRIVEN END TO END ----------------------------- +# The live install's shape (max_w 5000, slew_w 400, target_grid_w -10). The site +# rests in the deadband with another writer holding the pack at +700 W - so the +# saturation counter climbs while the command is 0 and nothing is wrong - and +# then 2 kW of export arrives. On release/1.0 @ c8dcb38 the command is 0.0 W on +# every cycle from there on: 0 W is a FIXED POINT, and the export is given away +# for as long as the other writer stays. +prev, i_w, sat, cmds = 0.0, 0.0, 0, [] +for c in range(24): + d = compute(prev, -10.0 if c < 6 else -2000.0, 700.0, TL, sat, i_w) + cmds.append(d.target_w) + prev, i_w, sat = d.target_w, d.i_w, d.sat_count +check("the card's case: the block does hold at first, at 0 W", + cmds[6] == 0.0 and cmds[7] == 0.0) +started = [i for i, c in enumerate(cmds) if c != 0.0] +check(f"the card's case: the charge starts at cycle {started[0]}" + f" instead of never", bool(started)) +# ⚠️ AND IT STOPS AT THE FROZEN INTEGRATOR, NOT AT THE RAIL. This is AC 4: the +# integrator moved while the output was blocked, and the release must not turn +# that divergence into windup. It cannot, because releasing the OUTPUT freeze +# does not release the INTEGRATOR freeze - so the command can only walk, one +# slew step per cycle, to a value the integrator stopped accumulating at. +check(f"the card's case: it settles at {prev:.0f} W, at the frozen integrator " + f"({i_w:.0f} W) and not the {TL.max_w:.0f} W rail", + prev == -1190.0 and abs(prev) <= abs(i_w) and abs(prev) < TL.max_w) +steps = [abs(cmds[i] - cmds[i - 1]) for i in range(1, len(cmds))] +check(f"the card's case: no cycle moves more than the slew limit " + f"(worst {max(steps):.0f} W of {slew_limit_w(TL):.0f})", + max(steps) <= slew_limit_w(TL)) + +# --- 6. AC 4, AS A PROPERTY RATHER THAN A TRAJECTORY ----------------------- +# ⚠️ THE FIRST FORM OF THIS CHECK WAS WRONG AND THE SWEEP SAID SO, which is +# worth leaving in the file. It asserted `|target| <= |i_w|` on a released +# cycle, and the sweep produced prev_w -5000, i_w -3000, target -4600: the +# command was walking BACK from -5000 toward the integrator and had not +# arrived. It was moving the right way and the assertion still failed, because +# "never exceeds the integrator" is not what no-windup means here. +# +# What it means: a released cycle can only move the command TOWARD the +# integrator, by at most one slew step, and never past it. So the divergence +# that built up while the output was blocked cannot arrive as a jump - it is +# spent one slew step at a time, against an integrator that stopped +# accumulating when the freeze engaged and is not released with the output. +step = float(T.step_w) +past, jumped = [], [] +for prev in [x * 250.0 for x in range(-20, 21)]: + for grid in [x * 500.0 for x in range(-12, 13)]: + for actual in [x * 500.0 for x in range(-10, 11)]: + for i0 in (-3000.0, -500.0, 0.0, 500.0, 3000.0): + d = compute(prev, grid, actual, TL, SETTLE_CYCLES, i0) + if d.reason != "freeze-released": + continue + lo, hi = min(prev, d.i_w), max(prev, d.i_w) + if not (lo - step <= d.target_w <= hi + step): + past.append((prev, grid, actual, i0, d.target_w, d.i_w)) + if abs(d.target_w - prev) > slew_limit_w(TL) + step: + jumped.append((prev, grid, actual, i0, d.target_w)) +check(f"over {41*25*21*5} released states the command stays between the" + f" previous command and the frozen integrator" + + (f" (e.g. {past[0]})" if past else ""), not past) +check("...and no released cycle moves further than one slew step" + + (f" (e.g. {jumped[0]})" if jumped else ""), not jumped) + +# --- 7. the one configuration in which the block does not exist ------------ +# ⚠️ SAID OUT LOUD BECAUSE IT IS A SILENT NO-OP OTHERWISE. `saturation_cycles` +# is accepted up to 10 by config.yaml. Set it at or above SETTLE_CYCLES and the +# freeze does not engage until the bound has already passed, so the no-start +# block never happens at all. It is not refused at startup - a controller that +# will not boot over a tuning nuance is worse than the nuance - but it must be +# a known consequence rather than a surprise. +TN = Tuning(slew_w=1000.0, saturation_cycles=SETTLE_CYCLES) +prev, i_w, sat, reasons = 0.0, 0.0, 0, set() +for _ in range(20): + d = compute(prev, -2000.0, 700.0, TN, sat, i_w) + reasons.add(d.reason) + prev, i_w, sat = d.target_w, d.i_w, d.sat_count +check(f"saturation_cycles >= {SETTLE_CYCLES} means the block never engages", + "saturated-freeze" not in reasons) + print("SAFETY-04: the i_w=None path is still release/1.0, exactly") @@ -394,8 +601,19 @@ def legacy(prev, grid, actual, t, sat_count): # "i-clamped", because the truncation happens on the integrator before the # command is derived from it. Aliased here rather than papered over - if any # OTHER reason ever diverges, this check goes red. +# ⚠️ SAFETY-13 IS A DELIBERATE DIVERGENCE FROM `legacy()`, AND THE SWEEP NOW +# GRADES THE DIVERGENCE INSTEAD OF FORBIDDING IT. `legacy()` above stays a +# transcription of release/1.0 - it is not edited to agree, for the reason the +# note on `slew_w` gives: an oracle taught the new rule compares the new law +# against itself. What changed is the assertion. Every case must either be +# IDENTICAL to release/1.0, or be a case where release/1.0 said +# "saturated-freeze" and this law says "freeze-released" with the two conditions +# that permit it both true. Anything else is a regression, and the counts below +# say how much of the sweep each answer covers. ALIAS = {"i-clamped": "clamped"} diffs = [] +released = [] +value_diffs = [] seen = set() # ⚠️ `slew_w` IS WRITTEN OUT IN ALL THREE (SAFETY-39). `legacy()` below is a # TRANSCRIPTION of release/1.0 and reads `t.slew_w` directly, as release/1.0 @@ -414,11 +632,35 @@ for tune in (Tuning(slew_w=1000.0), Tuning(slew_w=1000.0, target_grid_w=-10.0), seen.add(d.reason) got = (d.target_w, d.sat_count, d.frozen, ALIAS.get(d.reason, d.reason)) - if got != legacy(prev, grid, actual, tune, sc): - diffs.append((prev, grid, actual, sc, got, - legacy(prev, grid, actual, tune, sc))) -check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases, reason included" - + (f" (first diff {diffs[0]})" if diffs else ""), not diffs) + was = legacy(prev, grid, actual, tune, sc) + if got == was: + continue + explained = ( + d.reason == "freeze-released" + and was[3] == "saturated-freeze" + and d.sat_count >= SETTLE_CYCLES + and unasked_power(prev, actual, tune.saturation_w)) + if not explained: + diffs.append((prev, grid, actual, sc, got, was)) + continue + released.append((prev, grid, actual, sc)) + if got[0] != was[0]: + value_diffs.append((prev, got[0], was[0])) +TOTAL = 3 * 7 * 9 * 4 * 4 +check(f"i_w=None reproduces release/1.0 over {TOTAL} cases except where" + f" SAFETY-13 releases the freeze, reason included" + + (f" (first unexplained diff {diffs[0]})" if diffs else ""), not diffs) +check(f"{TOTAL - len(released)} of {TOTAL} cases are bit-identical to" + f" release/1.0; {len(released)} are releases", len(released) == 432) +# ⚠️ AND THE RELEASES ARE MOSTLY A LABEL. Only these change what reaches the +# wire; the rest report "freeze-released" on a command the freeze was not +# binding on anyway. Every one of them is a charge that release/1.0 refused to +# start from exactly 0 W, which is the ticket, stated as a count. +check(f"{len(value_diffs)} of those {len(released)} change the command," + f" and every one is a charge start from prev_w == 0 that was refused", + len(value_diffs) == 27 + and all(p == 0.0 and new_ < 0.0 and old_ == 0.0 + for p, new_, old_ in value_diffs)) # ...and the rename is not a quiet deletion: the signal SAFETY-03 alarms on has # to actually occur in that sweep, or its hook is dead. -- 2.54.0 From f156fadac058d3b1cb94d73b424c8d5b647310a7 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 6 Sep 2026 18:32:03 +0200 Subject: [PATCH 2/2] SAFETY-13: the bound came from the wrong half of a two-sided measurement NOTES.md 5.10 measured the 1.78 kW step BOTH ways - "Up: ~18 s from step to deadband. Down: ~23 s" (NOTES.md:1550). SAFETY-39 takes 18 correctly: its bound is a REFUSAL and the looser number refuses less. This branch reused PLANT_SETTLE_S for the opposite job - how long the plant may still be the EXPLANATION for a divergence - where the conservative half is the longer one. ceil(23/2) = 12, not 9, and being short there does not refuse anything, it accuses. The false positive is real and is measured here. One inverter, one compute(), one plant, no second writer anywhere in the harness: the command falls through zero while the pack is still coasting the old way, unasked_power() calls our own power "unasked", the counter reaches 9 while it is still coasting, and the law emits "freeze-released". At the plant's own measured 18 s/23 s shape, over 272 load-step pairs, the 9-cycle bound fires in 62 and the 12-cycle bound in 2. PLANT_SETTLE_DOWN_S = 23.0 is added beside PLANT_SETTLE_S = 18.0, because the plant's settling time is a PAIR and each bound has to say which half it means and why. SETTLE_CYCLES = ceil(PLANT_SETTLE_DOWN_S / LOOP_TICK_S) = 12. It is no longer the reciprocal of SLEW_RAIL_FRAC; that was a coincidence of both reading the same half, and test_control.py now asserts the inequality. _SAT_COUNT_CAP moves with it, 10 -> 13. config.yaml accepts saturation_cycles as int(1,10), so the whole accepted range is now strictly BELOW the bound and the degenerate configuration this branch used to document no longer exists - pinned against config.yaml's own schema line. Three shipped statements were falsified by that measurement and are rewritten, not renumbered - at 12 the residual is smaller, not zero: unasked_power()'s docstring said anything outside the interval "was put there by something other than this loop". It was our own power: eight cycles after a +1990 W command the command is 0 and the pack still reads +522.3 W. control.py said "SO THE BOUND BELONGS TO (b) AND ONLY TO (b)". It fired in case (a). DOCS.md told an operator "if you see freeze-released, look for the second writer" - on a single-inverter install, hunting something that does not exist. Replaced by a section that gives the innocent causes first (a large load or PV swing; a plant slower than the one measured, 8/320 scenarios at 23 s rising to 268/320 at 50 s) and the second writer only when the state STANDS. Round 1's own results are kept and re-graded, not weakened. The release/1.0 sweep had to widen to reach the new bound (3780 cases): 3267 bit-identical, 432 releases, 27 command changes - all still charge starts from prev_w == 0 - and 81 differing in the saturation counter alone, which nothing downstream can read. The card's case releases on the twelfth saturated cycle instead of the ninth and settles at the same -1190 W against the same frozen integrator. Also corrected, because they were claims and did not reproduce: the coverage table's "output freeze" row was 11, and the mutation form its own note describes gives 6 on this tree, 6 on the first SAFETY-13 commit and 2 on release/1.0; the detector row was left at 11 and is 15; the frozen = False footnote was left at 10 and is 13. And section 6 reported 107 625 as "released states" when it is the number of states SWEPT - 67 500 of them release, and both are counted now. Nothing here reached the inverter. 22/22 test files pass in goodwe-addon-ci, tree rsynced to a plain path, mounted :ro, python3 -B, __pycache__ purged and printed first (0 dirs / 0 .pyc; the worktree carries none). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 49 ++- .../SAFETY-13-bounding-the-output-freeze.md | 200 +++++++++--- goodwe_controller/DOCS.md | 75 ++++- goodwe_controller/app/control.py | 167 +++++++--- goodwe_controller/test_control.py | 307 +++++++++++++++--- 5 files changed, 648 insertions(+), 150 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index fd5bc9c..426ad1c 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -47,20 +47,35 @@ than *how long*: command is pinned at something the inverter can reach. It gets no exit, and that is release/1.0's behaviour kept unchanged. - A divergence a limit could **not** have produced - the inverter overshooting - the command, or producing the opposite sign - cannot clear itself, because - clearing it needs the inverter to track *our* command. That one releases once - the divergence has outlasted `PLANT_SETTLE_S`, i.e. `SETTLE_CYCLES` = 9 cycles - at the measured tick. No new constant: it is the reciprocal of SAFETY-39's - `SLEW_RAIL_FRAC`. + the command, or producing the opposite sign - may not clear itself at all, so + it gets one. It releases once the divergence has outlasted the plant's own + measured settling time: `SETTLE_CYCLES` = **12** cycles at the measured tick. + +⚠️ **THE BOUND COMES FROM THE SLOWER OF THE TWO DIRECTIONS THE PLANT WAS +MEASURED IN.** `NOTES.md` §5.10 timed the same 1.78 kW step both ways - ~18 s +up, ~23 s down - so `PLANT_SETTLE_DOWN_S` (23 s) joins `PLANT_SETTLE_S` (18 s) +as a named constant. SAFETY-39 uses 18 correctly, because its bound is a +*refusal* and the looser number refuses less; this bound asks how long the plant +may still be the *explanation*, and there the longer number is the safe one. A +bound of 9 derived from 18 fires the exit on a **single inverter with nothing +else on the bus** - measured in the container, 62 of 272 load-step pairs at the +plant's own measured shape, against 2 at 12. + +⚠️ **`freeze-released` IS A PLACE TO START LOOKING, NOT A FINDING.** It says the +divergence outlasted the plant's measured settling time. It is **not** proof of +a second writer: a large load or PV swing can leave our own power coasting +outside our own command's interval, and a plant slower than the one that was +measured does the same. DOCS.md now gives the innocent causes first and the +second writer only when the state *stands*. ⚠️ **RELEASING THE OUTPUT FREEZE DOES NOT UNFREEZE THE LOOP.** The integrator freeze is untouched, so the command can only walk, one slew step per cycle, toward an accumulator that stopped accumulating when the freeze engaged. Measured at the live install's shape (`max_w 5000`, `slew_w 400`, `target_grid_w -10`) with another writer holding the pack at +700 W and 2 kW of -export standing: the command holds at 0 W, starts on the ninth saturated cycle -and settles at **-1190 W** against a frozen integrator of -1194 W, not at the -5000 W rail. On `release/1.0` @ `c8dcb38` the same run held **0.0 W on every +export standing: the command holds at 0 W, starts on the twelfth saturated +cycle and settles at **-1190 W** against a frozen integrator of -1194 W, not at +the 5000 W rail. On `release/1.0` @ `c8dcb38` the same run held **0.0 W on every cycle**, for as long as the other writer stayed. ⚠️ **THIS IS NOT A REGRESSION FIX AND IT ESCALATES AT TWO INVERTERS.** The @@ -68,13 +83,19 @@ unbounded block is `release/1.0` behaviour, confirmed cycle for cycle against SAFETY-04's branch. At a roster of two, `actual_w` is the SITE SUM, so the comparison is a site command against two units' power and one unit still moving after the other has stopped can put the site into the block with no external -actor at all. +actor at all. ⚠️ That escalation is read from the code and from single-unit +runs; **it is not measured on two inverters.** Inverter B does not physically +exist yet and `sim/plantsim.py`'s `INVERTER_B` is `INVERTER_A` copied with +`measured=False`, so any two-inverter figure would be two copies of A. -Graded against `release/1.0` over the 3024-case sweep in `test_control.py`: -**2592 cases bit-identical, 432 releases, and 27 of those change the command** - -every one of them a charge from `prev_w == 0` that `release/1.0` refused to -start. `saturation_cycles` at 9 or 10 removes the block entirely; DOCS.md says -so. +Graded against `release/1.0` over the 3780-case sweep in `test_control.py`: +**3267 cases bit-identical, 432 releases, 27 of those change the command** - +every one a charge from `prev_w == 0` that `release/1.0` refused to start - and +**81 differ in the saturation counter alone**, because its ceiling moved with +the bound from 10 to 13. That ceiling change also removes a degenerate setting: +`config.yaml` accepts `saturation_cycles` up to 10, which is now strictly below +the release bound, so **no accepted configuration can stop the block engaging**. +At the old bound of 9 a setting of 9 or 10 deleted it silently. ⚠️ **A PER-INVERTER POWER READING THAT HAS STOPPED FOLLOWING ITS OWN COMMAND IS NOW `None`, NOT A HELD NUMBER ([[SAFETY-37]]).** `Inverter.batt` (register diff --git a/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md b/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md index 708380d..742a7aa 100644 --- a/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md +++ b/goodwe_controller/DECISIONS/SAFETY-13-bounding-the-output-freeze.md @@ -24,10 +24,14 @@ law run in a container against a plant model, on this branch and on a pristine magnitude is the whole of what the battery could have done. §3. 3. **The decision is not "how long", it is "which".** A divergence a *limit* could have produced clears itself and gets no exit — `release/1.0` unchanged. - A divergence a limit could not have produced cannot clear itself and gets - one. The predicate is physical, not statistical. §4. + A divergence a limit could not have produced may not, and gets one. The + predicate is physical, not statistical — and it is **not** a second-writer + detector; §4.1 and §8 say what it can and cannot tell apart. 4. **Releasing the output freeze does not unfreeze the loop**, and that is what makes the release bounded rather than merely different. §6. +5. **The bound comes from the SLOWER half of a two-sided measurement, and the + first version of this branch took the faster half.** §8 is the correction and + the measurement behind it; it is the reason `SETTLE_CYCLES` is 12 and not 9. --- @@ -154,8 +158,16 @@ So the rule implemented is: inverter do **less** than the command, in the command's own direction. It cannot make it overshoot the command and it cannot make it produce the opposite sign. So everything a limit can produce lies in the closed interval spanned by `0` and -`prev_w`, widened by `saturation_w` for noise, ramp lag and idle draw. Anything -outside was put there by something else. +`prev_w`, widened by `saturation_w` for noise, ramp lag and idle draw. + +⚠️ **What it does NOT establish is that anything outside was put there by +something else.** The first version of this branch said exactly that, in the +docstring, in `control.py` and in DOCS.md, and it is false: the predicate +compares against **one cycle's** command while the pack carries every command +before it, so our own coast-down through zero lands outside our own interval. +§8 measures it. What the predicate guarantees is only its own first sentence — +*this cycle's command does not account for this cycle's reading* — and the time +bound below is what turns that into a reason to act. | prev | actual | interval | verdict | |---|---|---|---| @@ -175,51 +187,87 @@ freeze gets an exit; being wrong about it costs a bounded command (§6). SAFETY-30's `unit_sign_disagree_w` is the thing that alarms on sign, and it is deliberately a different test with a different threshold. -### 4.2 The bound is not a new number +### 4.2 The bound, and which half of the measurement it comes from -`SETTLE_CYCLES = ceil(PLANT_SETTLE_S / LOOP_TICK_S)` = **9**. +`SETTLE_CYCLES = ceil(PLANT_SETTLE_DOWN_S / LOOP_TICK_S)` = **12**. -`PLANT_SETTLE_S` (18.0 s) and `LOOP_TICK_S` (2.0 s) are SAFETY-39's two measured -constants, already in this file. A divergence still standing after the plant's -own settling time is not this plant catching up — so after `SETTLE_CYCLES` the -plant has run out of alibi. It is the reciprocal of `SLEW_RAIL_FRAC`: the slew -limit takes exactly this many ticks to cross the rail, for exactly this reason. +⚠️ **`PLANT_SETTLE_DOWN_S` is new, and adding it is the point rather than a +cost.** `NOTES.md` §5.10 did not measure *a* settling time; it measured the same +1.78 kW step in **both directions** — "Up: ~18 s from step to deadband. Down: +~23 s from a −1784 W export excursion" (`NOTES.md:1550`). So the plant's +settling time is a **pair**, and which half is the safe one depends on which way +being wrong hurts: + +| the bound | the question | the safe half | why | +|---|---|---|---| +| SAFETY-39's `SLEW_RAIL_FRAC` | may this configuration be **refused**? | **18 s**, the shorter | a looser bound refuses fewer working configurations | +| this one | may the plant still be the **explanation**? | **23 s**, the longer | a shorter bound stops believing the plant too early, and then it accuses | + +The first version of this branch reused `PLANT_SETTLE_S` and got 9 — three +cycles short of the plant's own measured down-ramp, on the side that produces a +false positive. §8 is the measurement and the correction. + +⚠️ **It is no longer the reciprocal of `SLEW_RAIL_FRAC`, and it never should +have been read as one.** The two were equal only while both were being derived +from the same half of the pair, and that coincidence read as a derivation. They +are 9 and 12 now; `test_control.py` asserts the inequality so they cannot be +quietly re-fused. ⚠️ **It inherits SAFETY-39's caveat about `LOOP_TICK_S`, in the opposite -direction.** A meter **slower** than 2 s makes 9 cycles cover more than 18 s, so +direction.** A meter **slower** than 2 s makes 12 cycles cover more than 23 s, so the freeze holds longer than the plant needs: that costs energy, never safety. A meter **faster** makes them cover less, so the freeze can release while the plant is still moving — bounded by §6 rather than by luck. -`_SAT_COUNT_CAP` is now written as `SETTLE_CYCLES + 1`. Its value is unchanged -(10, exactly as shipped); writing it against the bound stops a counter capped -below the longest bound read from it, which would silently delete the exit. +**`_SAT_COUNT_CAP` moved with it, from 10 to 13**, because it is written as +`SETTLE_CYCLES + 1` — a counter capped below the longest bound read from it +would silently delete the exit. That has a consequence worth stating on its own: +`config.yaml` accepts `saturation_cycles` as `int(1,10)`, so at the old bound of +9 a legal setting could sit **above** the exit and delete the block. At 12 the +whole accepted range is strictly below the bound, and the degenerate +configuration this branch used to document no longer exists. §7. --- ## 5. What actually changed, graded against `release/1.0` -`test_control.py`'s 3024-case sweep still compares this law against a -transcription of `release/1.0` — the transcription was **not** taught the new -rule, because an oracle taught the change compares the law against itself. What -changed is the assertion: every case must be identical, or be a release with -both of its conditions true. +`test_control.py`'s sweep still compares this law against a transcription of +`release/1.0` — the transcription was **not** taught the new rule, because an +oracle taught the change compares the law against itself. What changed is the +assertion: every case must be identical, or be a release with both of its +conditions true, or differ **in the saturation counter alone**. + +⚠️ **The sweep had to be widened to keep grading anything.** Its `sat_count` +fixtures ran to 9; with the bound at 12 the counter never reached the exit and +every release silently vanished from the table, which is what the assertion on +the count caught. It runs to 12 now, at 3780 cases. | | cases | |---|---| -| total | 3024 | -| **bit-identical to `release/1.0`** | **2592** | +| total | 3780 | +| **bit-identical to `release/1.0`** | **3267** | | releases | 432 | | …of those, reason-only (the freeze was not binding) | 405 | | …of those, **the command changes** | **27** | +| differ in the **saturation counter alone** | 81 | -All 27 are at `prev_w == 0`, all with `release/1.0` commanding exactly `0.0 W`, -and every new command is **negative** — a charge start that was refused. Nothing -else in the sweep moved. +All 27 command changes are at `prev_w == 0`, all with `release/1.0` commanding +exactly `0.0 W`, and every new command is **negative** — a charge start that was +refused. + +⚠️ **The 81 are the counter's ceiling moving, and they reach nothing.** +`_SAT_COUNT_CAP` went from 10 to 13 with the bound, so above `release/1.0`'s own +ceiling the two counters differ. Nothing else may differ in those cases, and the +sweep asserts that: `frozen` is `sat_count >= saturation_cycles`, `config.yaml` +accepts that at most 10, and no other line in `compute()` reads the counter's +value — so a counter reading 13 instead of 10 cannot change what is commanded. +The 81 all sit at an `sc` above 10, which is a state `release/1.0` could never +hold at all. The behaviour after the fix, on §2's run: the command holds at 0 W through -`sat_count` 3–8, releases on the **ninth** saturated cycle, and walks -0 → −400 → −800 → **−1190 W**, where it stops. +`sat_count` 3–11, releases on the **twelfth** saturated cycle, and walks +0 → −400 → −800 → **−1190 W**, where it stops — the same settling point as at +the old bound, three cycles later. The genuine-limit case, driven for 30 cycles with the inverter curtailed to ±1000 W against a 4 kW import: `freeze-released` never appears and the command @@ -233,9 +281,13 @@ stays pinned at +2400 W. Unchanged, deliberately. derived from `i_w`, then clamped, then slew-limited from `prev_w` — so a released cycle can only move the command *toward* an accumulator that stopped accumulating when the freeze engaged, by at most one slew step, and never past -it. Swept over 107 625 released states in `test_control.py`: no case leaves the -interval between the previous command and the frozen integrator, and no case -moves further than one slew step. +it. `test_control.py` sweeps 107 625 states, of which **67 500 release**: no +released case leaves the interval between the previous command and the frozen +integrator, and none moves further than one slew step. + +⚠️ **107 625 was previously reported as the number of released states. It is +the number of states swept**, and the assertion only ever graded the subset that +released. Both numbers are counted and printed now. On §2's run that shows up as the settling point: **−1190 W against a frozen integrator of −1194 W**, not the 5000 W rail. The tech lead's note on SAFETY-04 @@ -261,16 +313,84 @@ buy correct regulation against an actuator somebody else is also writing to, and nothing in a rules engine can. A standing grid error with a second writer present is the symptom of a configuration this add-on refuses to support. -**`freeze-released` in the action log means "look for the second writer."** It is -carried on `Decision.reason`, which nothing after the freeze overwrites. No new -alarm was added: SAFETY-41 removed one that narrated normal anti-windup, and -this state is not yet known to be worth waking anybody for. +**`freeze-released` is a place to start looking, not a finding.** It says the +divergence outlasted the plant's own measured settling time, and that is all it +says — §8. DOCS.md now tells the operator the innocent causes first (a large +load or PV swing, a plant slower than the one that was measured) and the second +writer only when the state **stands**. It is carried on `Decision.reason`, which +nothing after the freeze overwrites. No new alarm was added: SAFETY-41 removed +one that narrated normal anti-windup, and this state is not yet known to be +worth waking anybody for. -**One configuration removes the block entirely.** `saturation_cycles` is -accepted up to 10 by `config.yaml`. At 9 or above the freeze cannot engage -before its own exit is due, so the no-start block never happens. This is -**not** refused at startup — a controller that will not boot over a tuning -nuance is worse than the nuance — but it is asserted in `test_control.py` and -written in DOCS.md so that it is a known consequence and not a surprise. +**No accepted configuration removes the block any more.** `config.yaml` accepts +`saturation_cycles` as `int(1,10)`; the bound is 12, so every accepted value is +strictly below it and the freeze always engages before its exit is due. This +paragraph used to say the opposite — at a bound of 9, settings of 9 and 10 +deleted the block silently and the branch documented that as a known +consequence. Moving the bound closed it by construction. `test_control.py` pins +it against `config.yaml`'s own schema line rather than a transcription of it. **Nothing was measured on two inverters.** See the warning in §3. + +--- + +## 8. ⚠️ ROUND 2: the bound was derived from the wrong half, and it accused + +**What was wrong.** `SETTLE_CYCLES` was `ceil(PLANT_SETTLE_S / LOOP_TICK_S)` = 9. +`PLANT_SETTLE_S` is 18 s, the **up**-ramp of `NOTES.md` §5.10's kettle step; +the same measurement recorded ~23 s coming back **down** (`NOTES.md:1550`). +SAFETY-39 takes 18 correctly — its bound is a refusal, and the looser number +refuses less. This bound asks the opposite question, and the looser number stops +believing the plant three cycles too early. + +**Why that is not a tuning quibble.** The predicate has no memory. `prev_w` is +one cycle's command; the pack carries every command before it. A command that +falls **through zero** — the ordinary end of a load or PV step — leaves the pack +coasting in the old direction, so the reading has the sign the command *used* to +have and lands outside the interval spanned by 0 and the new `prev_w`. +`unasked_power()` calls that "unasked". With the bound three cycles short of the +plant's own down-ramp, the counter reaches it while the coast-down is still +going, and the law emits `freeze-released` on **one inverter with nothing else +on the bus.** + +**Measured, in the container, N = 1.** One `compute()`, one plant model, one +command; no second writer anywhere in the harness. The plant is a first-order +lag whose two time constants come from the two measured settling times +(tau = t/3, since a first-order step is 95 % done at 3 tau) with +`sim/plantsim.py`'s measured 800 W/s ramp cap. It is a **model**, and it is +labelled one wherever it is used. + +| | bound 9 | bound 12 | +|---|---|---| +| 1280 closed-loop scenarios (a grid of plant pairs, slews, `saturation_cycles`, load steps) | **113 fire** | **24 fire** | +| 272 load-step pairs at the measured 18 s / 23 s plant, `slew_w 400` | **62 fire** | **2 fire** | + +The single case now pinned in `test_control.py`: a 4 kW export falling to +2.5 kW. At cycle 34 the command is −310 W, the pack is still at −1343.4 W +coasting down from −4860 W a few cycles earlier, `unasked_power(−310, −1343.4, +500)` is True, `sat_count` is 9, and the law says `freeze-released`. At 12 the +same run never releases. + +**What the fix does not fix.** The bound makes the false positive rare on *the +plant that was measured*. It cannot make it impossible, because it is a +measurement and the plant can be slower than the measurement. Same sweep, bound +held at 12, lengthening only the plant's down-ramp: 8 of 320 scenarios at the +measured 23 s, 59 of 320 at 34 s, 268 of 320 at 50 s. That residual is why the +three statements below were corrected rather than merely re-numbered. + +**Three shipped statements were falsified by this and are now rewritten:** + +1. `unasked_power()`'s docstring — *"Anything outside that interval was put + there by something other than this loop."* It was our own power. The + docstring now says what the predicate does guarantee (this cycle's command + does not account for this cycle's reading) and states the coast-down as its + known blind spot. +2. `control.py`, in `compute()` — *"SO THE BOUND BELONGS TO (b) AND ONLY TO + (b)"*. It fired in case (a). The comment now says the bound is a reason to + stop believing (a), not a proof of (b), and that the release is written to be + survivable when the inference is wrong. +3. **DOCS.md — *"If you see `freeze-released` in the action log, look for the + second writer."*** This is the one that reaches a person, and on a + single-inverter install it sent them hunting something that does not exist. + Replaced by a section that gives the innocent causes first and the second + writer only when the state stands. diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 1380344..b02013a 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -473,7 +473,7 @@ behind it would trip the firmware watchdog on a system that is working fine. | `target_grid_w` | -10 | What the meter should rest at. Negative = a slight export | | `step_w` | 10 | Quantisation | | `saturation_w` | 500 | Divergence that counts as "the inverter is at a limit" | -| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one meter update or `cycle_floor_s`, whichever comes first - see the note below. **Do not set to 1**, and **do not set it to 9 or 10** either: at 9 the freeze cannot engage before its own exit is due, so the no-start block stops existing (SAFETY-13, below) | +| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one meter update or `cycle_floor_s`, whichever comes first - see the note below. **Do not set to 1**. Every value this file accepts (1-10) is below the freeze's own release bound of 12 cycles, so no accepted setting can stop the block engaging - see SAFETY-13, below | | `integrator_max_w` | 0 | Bound on the loop's accumulator, and 0 means "same as `max_w`". Caps how much stale error can be waiting to unwind when the sign flips. **Do not raise it above `max_w`** - the output clamp already bounds what is commanded, so the only thing extra headroom buys is more cycles of wrong-direction power after every saturation event. Lowering it below `max_w` is the useful direction | | `heartbeat_s` | 10 | Refresh interval; must stay well under the firmware watchdog | | `cycle_floor_s` | 15 | The longest the loop may go without evaluating. A deadline on the gap between cycles, reset by every cycle — on a healthy meter it never elapses and changes nothing. Raise it only if your meter is slower than ~10 s; it must stay under `meter_max_age_s`, past which there is no reading to evaluate anyway | @@ -645,35 +645,76 @@ one is the decision: | the inverter is… | can it clear itself? | what happens now | |---|---|---| | doing **less** than the command, in the command's own direction — a cold pack, a curtailment, an export cap | **yes.** The command is pinned at something the inverter can reach, so it catches up and the freeze clears | unchanged: frozen for as long as it lasts, with no exit | -| doing something **outside** what the command can explain — overshooting it, or the opposite sign | **no.** Clearing the freeze needs the inverter to track *our* command, and the other writer is what stops it | the freeze releases once the divergence has outlasted the plant that could otherwise have explained it | +| doing something **outside** what the command can explain — overshooting it, or the opposite sign | **not necessarily.** If another writer is on the actuator, clearing the freeze needs the inverter to track *our* command and the other writer is what stops it, so nothing ends it | the freeze releases once the divergence has outlasted the plant that could otherwise have explained it — see *what `freeze-released` actually tells you*, below, because this row is also where our own ramp lands | -The second row's threshold is not a new number. `PLANT_SETTLE_S` (18 s, measured) is how -long this plant takes to settle, so a divergence still standing after that is not this -plant catching up; at the measured 2 s meter tick that is **9 cycles**, which is the -same relationship the slew limit already uses (`control.py`, `SETTLE_CYCLES`). +The second row's threshold is the plant's own measured settling time — **the slower of +the two directions it was measured in.** The 2026-08-22 step test timed the same 1.78 kW +step both ways: about 18 s to settle going up, about 23 s coming back down. 23 s is the +one that belongs here, because the question this bound answers is *how long may the plant +still be the explanation*, and the safe answer is to keep believing the plant for as long +as the plant has ever been seen to take. At the measured 2 s meter tick that is +**12 cycles** (`control.py`, `SETTLE_CYCLES`). + +⚠️ **The first version of this feature used 18 s, and 9 cycles was wrong in the direction +that accuses.** Nine cycles is three short of the plant's own down-ramp, so an ordinary +load step on a **single** inverter — the command falling through zero while the pack is +still coasting the old way — could produce `freeze-released` with nothing else on the bus +at all. Driven in the container against a model of the measured plant, one inverter and +no second writer anywhere: over 272 load-step pairs the 9-cycle bound produced a release +in 62 of them and the shipped 12-cycle bound in 2. **Releasing the output freeze does not unfreeze the loop.** The *integrator* freeze is untouched, so the command is free to move only as far as an accumulator that stopped accumulating cycles ago, one slew step per cycle. Measured on the simulator at the live install's shape (`max_w 5000`, `slew_w 400`, `target_grid_w -10`), with another writer holding the pack at +700 W and 2 kW of export standing: the command holds at 0 W while -the counter climbs, starts at the ninth saturated cycle, and settles at **−1190 W** -against a frozen integrator of −1194 W — not at the 5000 W rail. Before this change the -same run held 0.0 W on every cycle, for as long as the other writer stayed. +the counter climbs, starts on the **twelfth** saturated cycle, walks 0 → −400 → −800, and +settles at **−1190 W** against a frozen integrator of −1194 W — not at the 5000 W rail. +Before this change the same run held 0.0 W on every cycle, for as long as the other +writer stayed. ⚠️ **The site still does not reach its target in that state, and it is not supposed to.** What the exit buys is that the battery stops being idle while the site exports; what it does not buy is correct regulation against an actuator somebody else is also writing to. A standing grid error with another writer on the bus is the symptom of a -configuration this add-on refuses to support, not a tuning problem. If you see -`freeze-released` in the action log, look for the second writer. +configuration this add-on refuses to support, not a tuning problem. -⚠️ **One configuration removes the block entirely, silently.** `saturation_cycles` is -accepted up to 10. Set it at or above 9 and the freeze does not engage until the bound -has already passed, so the no-start block never happens at all. This is not refused at -startup — a controller that will not boot over a tuning nuance is worse than the nuance -— but it is why the default is 3 and why the table above says **do not set to 1** at one -end and leaves you this note at the other. +#### What `freeze-released` in the action log actually tells you + +**It says: the inverter's power did not match the command for longer than this plant has +ever been measured to take to settle.** That is all it says. It is a reason to go +looking; it is not a finding, and in particular it is **not** proof that a second writer +exists. + +The test behind it compares the reading against **one cycle's** command, and the pack +carries more than one cycle of history. So our own power, arriving late, can look like +somebody else's: + +- **A big, fast load step is the ordinary innocent cause.** The command falls through + zero while the pack is still coasting in the old direction, and for a few cycles the + reading has the sign the command *used* to have. Measured, one inverter, nothing else + on the bus: eight cycles after a +1990 W command the command was 0 and the pack still + read +522.3 W. +- **A plant slower than the one that was measured does it too.** The 12-cycle bound is + derived from a 23 s down-ramp. A colder or larger pack that takes longer than that will + produce releases on its own, and no constant read off the 2026-08-22 measurement can + prevent it. In the same container sweep, holding the bound at 12 and lengthening only + the plant's down-ramp, single-inverter releases went from 8 of 320 scenarios at the + measured 23 s to 59 of 320 at 34 s and 268 of 320 at 50 s. + +**So, in order:** if you see `freeze-released` **once, during or just after a large load +or PV swing**, that is most likely the loop's own ramp and needs nothing. If you see it +**standing** — cycle after cycle, at a steady load, with a standing grid error the +battery is not correcting — then look for the second writer: another controller, a +scheduler in the inverter's own app, or an installer tool left connected. That is the +configuration §1.5 of the specification rules out, and it is the one this exit exists to +stop the loop stalling on forever. + +⚠️ **No `saturation_cycles` you can set removes the block.** This file accepts 1-10, and +all of those are below the 12-cycle release bound, so the freeze always engages before +its own exit is due. (The first version of this feature had the bound at 9, where a +setting of 9 or 10 *did* silently delete the block; moving the bound to 12 closed that +by construction rather than by warning about it.) #### Why `target_grid_w` is not zero diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 8192a03..180f58a 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -66,6 +66,17 @@ from dataclasses import dataclass # validate.py refuse the impossible case only, never "close # to the rail". # +# ⚠️ AND THAT CHOICE IS LOCAL TO THIS BOUND. §5.10 MEASURED THE STEP IN BOTH +# DIRECTIONS, so "the plant's settling time" is a PAIR and not a number, and +# which half is the safe one depends on which way being wrong hurts. Taking +# 18 for a REFUSAL is right: the looser number refuses less. Taking 18 for a +# question of the form "how long does the plant remain a candidate +# explanation" is WRONG, because the looser number stops believing the plant +# too early - and there the answer is a false accusation. SAFETY-13 asks that +# second question and so reads PLANT_SETTLE_DOWN_S below, deliberately not +# this constant. Anything else added here has to say which of the two it +# means and why. +# # LOOP_TICK_S is the METER, not this loop. `run_control()` runs a cycle on # a changed meter value or on `cycle_floor_s`, whichever comes # first, so the tick is the meter's own update rate. 2.0 s is @@ -87,6 +98,13 @@ PLANT_SETTLE_S = 18.0 LOOP_TICK_S = 2.0 SLEW_RAIL_FRAC = LOOP_TICK_S / PLANT_SETTLE_S # 0.111..., i.e. 9 ticks +# The other half of the same measurement: the descent of the same 1.78 kW step, +# ~23 s from a -1784 W export excursion back into the deadband (goodwe/NOTES.md +# §5.10, the same run, the same day, the same plug). It is slower than the ramp +# up, which is the ordinary asymmetry of a pack that is pushed one way and +# coasts the other. Nothing above this line reads it; SAFETY-13 below does. +PLANT_SETTLE_DOWN_S = 23.0 + # --------------------------------------------------------------------------- # SAFETY-13. HOW LONG THE PLANT STAYS A CANDIDATE EXPLANATION. # @@ -100,42 +118,78 @@ SLEW_RAIL_FRAC = LOOP_TICK_S / PLANT_SETTLE_S # 0.111..., i.e. 9 ticks # at something the inverter can do, so the inverter catches up to it and # `saturated_now` goes false. # +# ⚠️ AND (a) INCLUDES THE CASE `unasked_power()` BELOW CANNOT SEE AS (a): +# the command has fallen THROUGH ZERO and the pack is still coasting in +# the old direction, so the reading has the sign the command used to have +# and not the one it has now. That is this loop's own power, arriving +# late. It is the reason the time bound is what does the work here and +# the predicate alone is not enough. +# # (b) SOMETHING ELSE IS DRIVING THE ACTUATOR - the §1.5 non-goal, two writers # on one actuator. The freeze CANNOT clear itself here, because clearing # it needs the inverter to track OUR command and the other writer is what # stops it. Nothing ends this but the other writer going away. # -# ⚠️ SO THE BOUND BELONGS TO (b) AND ONLY TO (b), and this constant is what -# separates them in TIME rather than by guessing. Case (a) is bounded by the -# plant: PLANT_SETTLE_S is how long this plant takes to settle, measured, and -# after that the plant has run out of alibi. A divergence still standing after -# PLANT_SETTLE_S is not this plant catching up. +# ⚠️ SO THE BOUND IS ONLY EVER A REASON TO STOP BELIEVING (a). It does not +# establish (b) and nothing below claims it does: what it says is that the +# plant has run out of alibi, and (b) is then the only explanation this loop +# has left - not the only one there is. Everything downstream of the release is +# written to be safe when that inference is wrong, because sometimes it is. # -# SETTLE_CYCLES = ceil(PLANT_SETTLE_S / LOOP_TICK_S) +# Case (a) is bounded by the plant, and the plant was measured: # -# ⚠️ THE SAME TWO MEASURED NUMBERS SAFETY-39 USES, AND DELIBERATELY NOT A NEW -# ONE. It is the reciprocal of SLEW_RAIL_FRAC - the slew limit takes exactly -# this many ticks to traverse the rail, for exactly this reason. A new constant -# here would be a second thing to re-derive when the plant is re-measured, and -# a number that outlives its derivation is the defect SAFETY-39 was about. +# SETTLE_CYCLES = ceil(PLANT_SETTLE_DOWN_S / LOOP_TICK_S) # -# ⚠️ AND IT INHERITS SAFETY-39's CAVEAT ABOUT LOOP_TICK_S, in the opposite +# ⚠️ THE **DOWN** NUMBER, AND THE DIRECTION IS THE WHOLE POINT. §5.10 measured +# the same step both ways - 18 s up, 23 s down - and SAFETY-39 takes 18 because +# 18 is the LOOSER bound for a REFUSAL. This is the opposite job. Here the +# question is "how long may the plant still be the explanation", and the +# conservative answer is the LONGER of the two: believe the plant for as long +# as the plant has ever been observed to take. Deriving this from 18 gives 9, +# which is three cycles SHORT of the plant's own measured down-ramp - and being +# short here does not refuse anything, it ACCUSES. Measured, single inverter, +# no second writer anywhere, 1280 closed-loop scenarios: at 9 the exit fires in +# 113 of them and at 12 in 24. The direction of that error is why the pair is +# now two named constants instead of one. +# +# ⚠️ AND IT IS NO LONGER THE RECIPROCAL OF SLEW_RAIL_FRAC. It was, at 9, and +# that coincidence read as a derivation - two different questions that happened +# to share an answer while both were being asked of the same half of a two- +# sided measurement. They are 9 and 12 now. If they ever coincide again it is +# arithmetic, not agreement. +# +# ⚠️ IT INHERITS SAFETY-39's CAVEAT ABOUT LOOP_TICK_S, in the opposite # direction, which is why it is repeated rather than cross-referenced. A meter -# SLOWER than 2 s makes these 9 cycles cover MORE than 18 s, so the freeze +# SLOWER than 2 s makes these 12 cycles cover MORE than 23 s, so the freeze # holds longer than the plant needs: that costs energy - see the case in # DOCS.md - and never safety. A meter FASTER than 2 s makes them cover LESS, # so the freeze can release while the plant is genuinely still moving. That one # is bounded twice over and by neither this constant nor luck: the INTEGRATOR # freeze is not released with the output freeze, so the command can only reach # the integrator's frozen value, and the output clamp bounds that in turn. -SETTLE_CYCLES = math.ceil(PLANT_SETTLE_S / LOOP_TICK_S) # 9 +# +# ⚠️ AND A PLANT SLOWER THAN THE ONE THAT WAS MEASURED DOES THE SAME THING, +# which no constant derived from a measurement can fix. In the same 1280 +# scenarios, holding the bound at 12 and lengthening only the plant's down-ramp +# past the measured 23 s, the exit starts firing again on a single inverter: +# 8/320 at 23 s, 59/320 at 34 s, 268/320 at 50 s. A re-measured plant means a +# re-derived constant here, exactly as for the two above it. +SETTLE_CYCLES = math.ceil(PLANT_SETTLE_DOWN_S / LOOP_TICK_S) # 12 -# The saturation counter's ceiling. It has always been 10 and still is; it is -# written against SETTLE_CYCLES rather than as a literal so that a counter -# capped BELOW the longest bound read from it - which would silently make that -# bound unreachable, i.e. delete the exit condition - cannot happen by editing -# one of the two. -_SAT_COUNT_CAP = SETTLE_CYCLES + 1 # 10 +# The saturation counter's ceiling, one above the longest bound read from it. A +# counter capped BELOW that bound would silently make the bound unreachable - +# i.e. delete the exit condition - so it is written against SETTLE_CYCLES and +# not as a literal, and it moves when SETTLE_CYCLES moves. +# +# ⚠️ IT WAS 10, WHICH IS ALSO config.yaml's MAXIMUM FOR `saturation_cycles`, +# AND THAT COLLISION IS GONE. At a bound of 9 the largest accepted +# `saturation_cycles` (10) sat ABOVE the exit, so a legal configuration could +# push the freeze's engagement past its own release and the no-start block +# stopped existing. At 12 every accepted value 1..10 is strictly below the +# bound, so the freeze always engages before its exit is due and no accepted +# configuration can delete the block. `test_control.py` pins that at the schema +# edge rather than in prose, because it is a property of two files agreeing. +_SAT_COUNT_CAP = SETTLE_CYCLES + 1 # 13 @dataclass(frozen=True) @@ -237,11 +291,11 @@ def slew_limit_w(tuning: Tuning) -> float: def unasked_power(prev_w: float, actual_w: float, saturation_w: float) -> bool: - """True when `actual_w` cannot be explained by the command not being met. + """True when `actual_w` cannot be explained by THIS cycle's command not being met. SAFETY-13. The one question the saturation magnitude test cannot answer: - is the inverter FAILING TO REACH what we asked, or is it doing something we - never asked for at all? + is the inverter FAILING TO REACH what we asked, or is it doing something + the command in front of it does not account for? ⚠️ THE ANSWER IS PHYSICAL, NOT STATISTICAL, WHICH IS WHY IT IS A PREDICATE AND NOT A THRESHOLD. A limit can only make the inverter do LESS than the @@ -249,8 +303,28 @@ def unasked_power(prev_w: float, actual_w: float, saturation_w: float) -> bool: command, and it cannot make it produce power of the OPPOSITE sign. So the whole set of readings a limit can produce is the closed interval spanned by 0 and `prev_w`, widened by `saturation_w` for measurement noise, ramp lag - and the inverter's own idle draw. Anything outside that interval was put - there by something other than this loop. + and the inverter's own idle draw. + + ⚠️ WHAT IT DOES NOT SAY - AND AN EARLIER VERSION OF THIS DOCSTRING DID - + IS THAT ANYTHING OUTSIDE THE INTERVAL WAS PUT THERE BY SOMETHING OTHER THAN + THIS LOOP. THAT IS FALSE AND IT WAS MEASURED FALSE. This predicate has no + memory: `prev_w` is one cycle's command, while the plant carries every + command before it. Drive ONE inverter, with nothing else on the bus, from a + discharge down through zero, and the pack is still coasting in the old + direction after the command has crossed. Measured in the container, one + `compute()` against one plant: eight cycles after a +1990 W command the + command is 0 and the pack still reads +522.3 W, so + `unasked_power(0, 522.3, 500)` is True - and every watt of that 522.3 was + asked for by this loop. In 1280 single-inverter closed-loop scenarios with + no second writer in them at all, the exit built on this predicate fires in + 24 of them. + + So what it guarantees is exactly the sentence at the top: this cycle's + command does not account for this cycle's reading. What it CANNOT do is + tell "somebody else is writing" from "we are". That gap is the reason + SETTLE_CYCLES is derived from the plant's slower direction, why the caller + checks both, and why the reason string it produces is a place to start + looking rather than a finding. Worked, at `saturation_w = 500`: @@ -262,16 +336,19 @@ def unasked_power(prev_w: float, actual_w: float, saturation_w: float) -> bool: ⚠️ AT `prev_w == 0` THIS IS EXACTLY `saturated_now`, and that is not a coincidence to be tidied away - it is the reason the freeze's zero case is the one the card was written about. With nothing commanded there is no - interval left for a limit to live in, so every saturated cycle at zero is a - cycle in which the actuator is being driven by someone else. + interval left for a LIMIT to live in, so at zero this test cannot be + answered by "the inverter is not reaching what we asked" at all. It can + still be answered by "the inverter has not finished leaving what we asked + for LAST time", which is the coast-down above and is why zero needs the + time bound as much as any other `prev_w` does. ⚠️ IT IS NOT A DETECTOR AND MUST NOT BE ALARMED ON. A saturated inverter whose reading is stale, wrong-signed or simply noisy lands outside the - interval too. All this predicate decides is whether the output freeze gets - an EXIT; being wrong about it costs at most a bounded command that the - frozen integrator and the output clamp both still bound. SAFETY-30's - `unit_sign_disagree_w` is the thing that alarms on sign, and it is - deliberately a different test with a different threshold. + interval too, and so does our own ramp, above. All this predicate decides + is whether the output freeze gets an EXIT; being wrong about it costs at + most a bounded command that the frozen integrator and the output clamp both + still bound. SAFETY-30's `unit_sign_disagree_w` is the thing that alarms on + sign, and it is deliberately a different test with a different threshold. """ lo = min(0.0, float(prev_w)) - abs(float(saturation_w)) hi = max(0.0, float(prev_w)) + abs(float(saturation_w)) @@ -465,11 +542,27 @@ def compute( # END ON ITS OWN. See SETTLE_CYCLES and `unasked_power()` at the top: # a divergence a LIMIT could produce clears itself once the inverter # catches up to the pinned command, so it needs no exit and gets none - - # unbounded, exactly as shipped. A divergence a limit CANNOT produce is - # another writer, which by construction stops our command ever being - # tracked, so it can only end when the other writer leaves. That is the - # one this releases, and only after the divergence has outlasted the - # plant that would otherwise have explained it. + # unbounded, exactly as shipped. A divergence a limit CANNOT produce + # does not clear itself by being waited on, so it gets one. + # + # ⚠️ AND THAT IS NOT THE SAME SENTENCE AS "THE OTHER WRITER". An + # earlier version of this comment said the two conditions below fire + # only in case (b); they do not, and the sweep says so. Our own plant + # coasting through a command that has just crossed zero satisfies + # `unasked_power()` too, because that predicate compares against ONE + # cycle's command and the pack remembers more than one - measured, a + # single inverter with nothing else on the bus reaching this branch in + # 24 of 1280 closed-loop scenarios. `SETTLE_CYCLES` is what keeps that + # rare rather than routine (113 of the same 1280 at the old bound of + # 9), and it does it by believing the plant for as long as the plant + # has ever been measured to take. It is a bound on how long we wait, + # not a proof about what we found. + # + # ⚠️ SO THE RELEASE IS WRITTEN TO BE SURVIVABLE WHEN IT IS WRONG, and + # that is the paragraph below, not this one: what a false release costs + # is one slew step per cycle toward a frozen integrator. If it ever has + # to cost less than that, the thing to change is what happens after the + # release, not the confidence of the sentence before it. # # ⚠️ RELEASING IS NOT UNFREEZING. The INTEGRATOR freeze above is not # lifted, and that is what makes this bounded rather than merely diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 8bebc68..976c1b4 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -17,9 +17,9 @@ Read MUTATION.md before you report a survivor. import sys -from app.control import (LOOP_TICK_S, PLANT_SETTLE_S, SETTLE_CYCLES, - SLEW_RAIL_FRAC, Tuning, compute, slew_limit_w, - unasked_power, +from app.control import (LOOP_TICK_S, PLANT_SETTLE_DOWN_S, PLANT_SETTLE_S, + SETTLE_CYCLES, SLEW_RAIL_FRAC, Tuning, compute, + slew_limit_w, unasked_power, maintenance_charge_floor, peak_at_risk) # ⚠️ SAFETY-39: `slew_w` IS EXPLICIT HERE NOW, AND IT IS release/1.0's VALUE. @@ -60,16 +60,33 @@ def check(name, cond): # A comment cannot go stale-green, because it never claims to be running. # # Measured at 389d9ec. Numbers are the lead's independent reproduction. -# ⚠️ THE OUTPUT FREEZE ROW WAS RE-MEASURED AT SAFETY-13 AND IT MOVED FROM 2 TO -# 11, because the SAFETY-13 section grades that branch too. Both figures are -# from the SAME mutation form - the whole `target = min(...)/max(...)` line -# removed, `reason` left alone - and 2 was reproduced on a pristine c8dcb38 -# tree before the new figure was taken, so the row moved because checks were -# added and not because the mutation changed shape. The two new rows below it -# are that section's own mechanisms: removing the EXIT restores release/1.0's -# unbounded freeze, and making `unasked_power()` return True gives an exit to -# the limit case as well, which is the half of the decision that has to stay -# unbounded. +# ⚠️ THE SAFETY-13 ROWS WERE RE-MEASURED IN ROUND 2 AND THREE OF THEM WERE +# WRONG AS FIRST WRITTEN. Every figure below marked (S13) was taken by deleting +# the mechanism, purging __pycache__, running this file in the CI image and +# counting `FAIL` lines, on THIS tree; the release/1.0 comparison figures were +# taken the same way on a pristine c8dcb38 worktree. +# +# - `output freeze` was written as 11. The mutation form its own note +# describes - the whole `target = min(...)/max(...)` line removed, `reason` +# left alone - gives 6 on this tree AND 6 on the first SAFETY-13 commit, +# against 2 on release/1.0. 11 is not reproducible by that form, or by +# removing the line and its `reason` (4), or by swapping min/max (12). The +# row is 6. +# - `saturation detector, saturated_now = False` was left at 11 while the +# SAFETY-13 section added checks that grade it. It is 15. +# - the footnote's `frozen = False` figure was left at 10 for the same +# reason. It is 13. +# +# The rows below `output freeze` are this ticket's own mechanisms: removing the +# EXIT restores release/1.0's unbounded freeze; making `unasked_power()` return +# True gives an exit to the limit case as well, which is the half of the +# decision that has to stay unbounded; deriving SETTLE_CYCLES from +# PLANT_SETTLE_S (18 s, the UP ramp) instead of PLANT_SETTLE_DOWN_S (23 s) is +# the round-1 defect itself; and pinning `_SAT_COUNT_CAP` back at release/1.0's +# literal 10 caps the counter below the bound and deletes the exit silently. +# +# ⚠️ ONLY THE ROWS THIS BRANCH MOVES WERE RE-MEASURED. Everything not marked +# (S13) still carries 389d9ec's figures and was not re-run here. # # ⚠️ THE SLEW ROWS WERE RE-MEASURED AT SAFETY-39 AND ONE OF THEM MOVED: the # slew limit was 4 and is now 5, because the SAFETY-39 section grades it too. @@ -87,12 +104,14 @@ def check(name, cond): # output clamp 3 # slew limit 5 # slew follows the rail (SAFETY-39), `slew_w` read raw 3 -# output freeze 11 -# the output freeze's EXIT (SAFETY-13), freeze made unconditional again 4 -# `unasked_power()` discriminates, replaced by `return True` 6 +# output freeze (S13) 6 +# the output freeze's EXIT (S13), freeze made unconditional again 4 +# `unasked_power()` discriminates, replaced by `return True` (S13) 6 +# SETTLE_CYCLES from PLANT_SETTLE_S, not _DOWN_S (S13) 9 +# `_SAT_COUNT_CAP` pinned back at release/1.0's 10 (S13) 6 # deadband 5 # quantisation 2 -# saturation detector, `saturated_now = False` 11 +# saturation detector, `saturated_now = False` (S13) 15 # saturation duration (AC 2), fires instantly 2 # sat counter reset on a good cycle 6 # target_grid_w bias 3 @@ -100,7 +119,8 @@ def check(name, cond): # # The detector figure is for the `saturated_now = False` form specifically; # disabling it further down as `frozen = False` is a weaker mutation and gives -# 10. Reproduce the same form or the number will not match. +# 13 on this tree (10 on release/1.0). Reproduce the same form or the number +# will not match. # # ⚠️ IT HAS FOUND A DEAD MECHANISM TWICE, BOTH THE SAME WAY: a clamp standing in # for the mechanism under test. Deleting the integrator freeze once failed @@ -416,12 +436,30 @@ same = all(unasked_power(0.0, a, 500.0) == (abs(a) > 500.0) check("at prev_w == 0 unasked_power IS the saturation test, over 801 readings", same) -# --- 2. the bound is derived, not chosen ------------------------------------ -check(f"SETTLE_CYCLES is ceil(PLANT_SETTLE_S / LOOP_TICK_S) = {SETTLE_CYCLES}", - SETTLE_CYCLES == 9 and SETTLE_CYCLES * LOOP_TICK_S >= PLANT_SETTLE_S - and (SETTLE_CYCLES - 1) * LOOP_TICK_S < PLANT_SETTLE_S) -check("...which is the reciprocal of SAFETY-39's SLEW_RAIL_FRAC", - abs(SETTLE_CYCLES - 1.0 / SLEW_RAIL_FRAC) < 1e-9) +# --- 2. the bound is derived, and from WHICH HALF of the measurement -------- +# ⚠️ THE DIRECTION IS THE ASSERTION. goodwe/NOTES.md 5.10 measured the same +# 1.78 kW step both ways: ~18 s up, ~23 s down. SAFETY-39 takes the SHORTER +# one because its bound is a REFUSAL and the looser number refuses less. This +# bound asks the opposite question - how long the plant may still be the +# explanation for a divergence - and for that the conservative half is the +# LONGER one. The first version of this file derived it from 18, got 9, and +# that is three cycles short of the plant's own measured down-ramp, in the +# direction that ACCUSES rather than refuses (section 8 below drives it). +check(f"SETTLE_CYCLES is ceil(PLANT_SETTLE_DOWN_S / LOOP_TICK_S) =" + f" {SETTLE_CYCLES}", + SETTLE_CYCLES == 12 + and SETTLE_CYCLES * LOOP_TICK_S >= PLANT_SETTLE_DOWN_S + and (SETTLE_CYCLES - 1) * LOOP_TICK_S < PLANT_SETTLE_DOWN_S) +check("...from the LONGER of the two measured settle times, so it is strictly" + " above what the shorter one would have given", + PLANT_SETTLE_DOWN_S > PLANT_SETTLE_S + and SETTLE_CYCLES > -(-PLANT_SETTLE_S // LOOP_TICK_S)) +# ...and it is NOT SLEW_RAIL_FRAC's reciprocal any more. It was, while both +# were being derived from the same half of the pair, and that coincidence read +# as a derivation. Asserted as an INEQUALITY so the two cannot silently be +# re-fused into one number again. +check("the slew's rail crossing and this bound are now different numbers", + SETTLE_CYCLES > 1.0 / SLEW_RAIL_FRAC) # The counter has to be able to REACH the bound or the exit does not exist. # Driven, not read off the constant: run a saturated state and see how high the # counter actually goes. @@ -529,41 +567,179 @@ check(f"the card's case: no cycle moves more than the slew limit " # that built up while the output was blocked cannot arrive as a jump - it is # spent one slew step at a time, against an integrator that stopped # accumulating when the freeze engaged and is not released with the output. +# ⚠️ AND THE COUNT BELOW IS THE RELEASED STATES, NOT THE SWEPT ONES. The first +# version of this check named 41*25*21*5 - the size of the sweep - while +# grading only the subset that actually releases, so the number in the message +# was not the number the assertion was about. It is counted now. step = float(T.step_w) -past, jumped = [], [] +swept, past, jumped = 0, [], [] +n_released = 0 for prev in [x * 250.0 for x in range(-20, 21)]: for grid in [x * 500.0 for x in range(-12, 13)]: for actual in [x * 500.0 for x in range(-10, 11)]: for i0 in (-3000.0, -500.0, 0.0, 500.0, 3000.0): + swept += 1 d = compute(prev, grid, actual, TL, SETTLE_CYCLES, i0) if d.reason != "freeze-released": continue + n_released += 1 lo, hi = min(prev, d.i_w), max(prev, d.i_w) if not (lo - step <= d.target_w <= hi + step): past.append((prev, grid, actual, i0, d.target_w, d.i_w)) if abs(d.target_w - prev) > slew_limit_w(TL) + step: jumped.append((prev, grid, actual, i0, d.target_w)) -check(f"over {41*25*21*5} released states the command stays between the" - f" previous command and the frozen integrator" - + (f" (e.g. {past[0]})" if past else ""), not past) +check(f"of {swept} states swept, the {n_released} that release keep the command" + f" between the previous command and the frozen integrator" + + (f" (e.g. {past[0]})" if past else ""), not past and n_released > 0) check("...and no released cycle moves further than one slew step" + (f" (e.g. {jumped[0]})" if jumped else ""), not jumped) -# --- 7. the one configuration in which the block does not exist ------------ -# ⚠️ SAID OUT LOUD BECAUSE IT IS A SILENT NO-OP OTHERWISE. `saturation_cycles` -# is accepted up to 10 by config.yaml. Set it at or above SETTLE_CYCLES and the -# freeze does not engage until the bound has already passed, so the no-start -# block never happens at all. It is not refused at startup - a controller that -# will not boot over a tuning nuance is worse than the nuance - but it must be -# a known consequence rather than a surprise. -TN = Tuning(slew_w=1000.0, saturation_cycles=SETTLE_CYCLES) +# --- 7. NO ACCEPTED CONFIGURATION CAN DELETE THE BLOCK --------------------- +# ⚠️ THIS CHECK REPLACES ITS OWN OPPOSITE, AND THE REASON IS WORTH KEEPING. +# While the bound was 9, `saturation_cycles` - which config.yaml accepts up to +# 10 - could be set AT OR ABOVE it, and then the freeze did not engage until +# its own exit was already due: the no-start block silently stopped existing, +# and this file asserted that as a known consequence. Moving the bound to 12 +# put the whole accepted range strictly below it, so the degenerate +# configuration is gone rather than documented. +# +# It is pinned against config.yaml ITSELF and not a transcription, because the +# property is "two files agree" and a transcription is what lets them drift. +# Regex, not PyYAML: the add-on image ships no PyYAML (test_validate.py says +# the same thing at more length). +import os # noqa: E402 +import re # noqa: E402 + +_CFG = os.path.join(os.path.dirname(os.path.abspath(__file__)), "config.yaml") +with open(_CFG, encoding="utf-8") as _fh: + _m = re.search(r"^ saturation_cycles: int\((\d+),(\d+)\)\s*$", + _fh.read(), re.M) +check("config.yaml's saturation_cycles range really was read", _m is not None) +_sc_lo, _sc_hi = int(_m.group(1)), int(_m.group(2)) +check(f"every accepted saturation_cycles ({_sc_lo}..{_sc_hi}) is strictly" + f" below the bound ({SETTLE_CYCLES}), so no accepted configuration can" + f" push the freeze's engagement past its own exit", + _sc_hi < SETTLE_CYCLES) +# Driven at the schema's edge rather than argued: the block still engages, and +# it engages before the exit is due. +TN = Tuning(slew_w=1000.0, saturation_cycles=_sc_hi) prev, i_w, sat, reasons = 0.0, 0.0, 0, set() +first_freeze = None for _ in range(20): d = compute(prev, -2000.0, 700.0, TN, sat, i_w) reasons.add(d.reason) + if d.reason == "saturated-freeze" and first_freeze is None: + first_freeze = d.sat_count prev, i_w, sat = d.target_w, d.i_w, d.sat_count -check(f"saturation_cycles >= {SETTLE_CYCLES} means the block never engages", - "saturated-freeze" not in reasons) +check(f"at the largest accepted saturation_cycles ({_sc_hi}) the block still" + f" engages, at sat_count {first_freeze}, below the bound", + "saturated-freeze" in reasons and first_freeze is not None + and first_freeze < SETTLE_CYCLES) + +# --- 8. THE BOUND'S DIRECTION, DRIVEN ON ONE INVERTER ---------------------- +# ⚠️ WHY THIS SECTION EXISTS. The exit's two conditions are read as "another +# writer is on the actuator". They are not that, and the first shipped form of +# this file said they were. `unasked_power()` compares a reading against ONE +# cycle's command while the pack carries every command before it, so a command +# that has fallen THROUGH ZERO - or simply fallen faster than the pack coasts - +# leaves our own power outside our own interval. With the bound derived from +# the 18 s UP measurement that is not a corner case: it is what a normal load +# step does. +# +# The plant here is a MODEL and is labelled one. It is a first-order lag whose +# two time constants come from the two measured settling times (a first-order +# step is 95 % done at 3 tau, so tau = t / 3), with the ramp cap +# sim/plantsim.py measured on inverter A. It is not the rig and it is not the +# house; what it is for is that the two settling times ENTER the model in the +# same direction they enter the constant, so the check moves if either does. +# +# ⚠️ AND THERE IS NO SECOND WRITER IN THIS SECTION. One `compute()`, one +# plant, one command. Every `freeze-released` below is therefore false. +import math # noqa: E402 + + +class _Plant: + """One inverter, obeying our command and nothing else. A MODEL.""" + + RAMP_W_S = 800.0 # sim/plantsim.py INVERTER_A, measured=True + + def __init__(self, up_s, down_s): + self.a_up = 1.0 - math.exp(-LOOP_TICK_S / (up_s / 3.0)) + self.a_dn = 1.0 - math.exp(-LOOP_TICK_S / (down_s / 3.0)) + self.w = 0.0 + + def tick(self, cmd): + a = self.a_up if abs(cmd) > abs(self.w) else self.a_dn + cap = self.RAMP_W_S * LOOP_TICK_S + self.w += max(-cap, min(cap, a * (cmd - self.w))) + + +from app import control as _control # noqa: E402 + +TS8 = Tuning(max_w=5000.0, slew_w=400.0, target_grid_w=-10.0, + saturation_w=500.0, saturation_cycles=3) + + +def _drive(bound, load1, load2, cycles=60, switch=25): + """Run one closed loop at an arbitrary bound. -> (release cycles, rows).""" + was_b, was_c = _control.SETTLE_CYCLES, _control._SAT_COUNT_CAP + # ⚠️ BOTH, OR THE PROBE LIES: `_SAT_COUNT_CAP` is what `compute()` caps the + # counter with, so moving the bound without it caps the counter BELOW the + # bound and the exit quietly becomes unreachable - which reads as "no false + # positives". That is the hazard `_SAT_COUNT_CAP`'s own comment names. + _control.SETTLE_CYCLES, _control._SAT_COUNT_CAP = bound, bound + 1 + try: + p = _Plant(PLANT_SETTLE_S, PLANT_SETTLE_DOWN_S) + prev, i_w, sat, rows = 0.0, 0.0, 0, [] + for c in range(cycles): + load = load1 if c < switch else load2 + d = compute(prev, load - p.w, p.w, TS8, sat, i_w) + rows.append((c, prev, p.w, d.target_w, d.sat_count, d.reason)) + prev, i_w, sat = d.target_w, d.i_w, d.sat_count + p.tick(prev) + return [r for r in rows if r[5] == "freeze-released"], rows + finally: + _control.SETTLE_CYCLES, _control._SAT_COUNT_CAP = was_b, was_c + + +_SHORT = int(-(-PLANT_SETTLE_S // LOOP_TICK_S)) # what 18 s would give: 9 + +# A 4 kW export falling to 2.5 kW. Nothing exotic; no second writer. +_bad, _rows = _drive(_SHORT, -4000.0, -2500.0) +check(f"a bound derived from the SHORTER measurement ({_SHORT} cycles) fires" + f" the exit on ONE inverter with nothing else on the bus" + + (f" - {len(_bad)} cycle(s), first at cycle {_bad[0][0]}" if _bad else ""), + bool(_bad)) +if _bad: + _c, _prev, _act, _tgt, _sat, _ = _bad[0] + check(f"...and what it called unasked is our own coast-down: command" + f" {_prev:.0f} W, pack still at {_act:.1f} W, and the pack was" + f" commanded {min(r[1] for r in _rows[:_c]):.0f} W a few cycles" + f" earlier", + unasked_power(_prev, _act, TS8.saturation_w) + and abs(_act) > abs(_prev) + and min(r[1] for r in _rows[:_c]) < _act) +_good, _ = _drive(SETTLE_CYCLES, -4000.0, -2500.0) +check(f"the shipped bound ({SETTLE_CYCLES} cycles, the LONGER measurement)" + f" does not fire on the same run", not _good) +# ...and not on any of these, either. The whole grid of load steps the same +# plant can be given, at the tuning every headline figure in this ticket is +# quoted from. +_L = [x * 500.0 for x in range(-8, 9)] +_fired = {} +for _b in (_SHORT, SETTLE_CYCLES): + _fired[_b] = sum(1 for _a in _L for _z in _L + if _a != _z and _drive(_b, _a, _z)[0]) +_pairs = len(_L) * (len(_L) - 1) +check(f"over {_pairs} load-step pairs on the measured plant, one inverter and" + f" no second writer: the short bound fires in {_fired[_SHORT]} of them," + f" the shipped bound in {_fired[SETTLE_CYCLES]}", + _fired[SETTLE_CYCLES] < _fired[_SHORT]) +# ⚠️ AND THE SHIPPED FIGURE IS NOT ZERO IN GENERAL. It is small here because +# the plant in this model is the one that was measured. A plant slower than +# that brings the false release back, and no constant derived from a +# measurement can fix that - it is why DOCS.md tells the operator what +# `freeze-released` is evidence OF, rather than what it proves. print("SAFETY-04: the i_w=None path is still release/1.0, exactly") @@ -611,9 +787,27 @@ def legacy(prev, grid, actual, t, sat_count): # that permit it both true. Anything else is a regression, and the counts below # say how much of the sweep each answer covers. ALIAS = {"i-clamped": "clamped"} +# ⚠️ SAFETY-13 ROUND 2 MOVED THE COUNTER'S CEILING, SO THE SWEEP NOW HAS A +# THIRD ANSWER. `_SAT_COUNT_CAP` is `SETTLE_CYCLES + 1`, which was 10 - the +# same literal release/1.0 used - and is now 13. Above release/1.0's own +# ceiling the two counters therefore differ, and NOTHING ELSE MAY: `frozen` is +# `sat_count >= saturation_cycles` and config.yaml accepts that at most 10, so +# a counter reading 13 instead of 10 cannot change it, and no other line in +# `compute()` reads the counter's value. That is asserted rather than asserted- +# by-omission, in `cap_diffs` below. +# +# ⚠️ AND THE SWEEP HAS TO REACH THE BOUND OR IT GRADES NOTHING. `sc` runs to 12 +# now; at 9 the counter tops out one short of the exit and every release +# silently disappears from this table, which is exactly what happened when the +# bound moved and is why the count is asserted and not merely printed. 12 is a +# state release/1.0 could never hold - its counter stopped at 10 - so +# `legacy()` is being fed one input outside its own reachable domain, on +# purpose, and `cap_diffs` is where that shows up. +LEGACY_SAT_CAP = 10 # release/1.0's literal, transcribed in legacy() diffs = [] released = [] value_diffs = [] +cap_diffs = [] seen = set() # ⚠️ `slew_w` IS WRITTEN OUT IN ALL THREE (SAFETY-39). `legacy()` below is a # TRANSCRIPTION of release/1.0 and reads `t.slew_w` directly, as release/1.0 @@ -627,7 +821,7 @@ for tune in (Tuning(slew_w=1000.0), Tuning(slew_w=1000.0, target_grid_w=-10.0), for prev in (-2000.0, -500.0, -100.0, 0.0, 100.0, 500.0, 2000.0): for grid in (-6000.0, -1000.0, -500.0, -14.0, 0.0, 14.0, 500.0, 1000.0, 6000.0): for actual in (-2000.0, 0.0, 600.0, 2000.0): - for sc in (0, 2, 3, 9): + for sc in (0, 2, 3, 9, 12): d = compute(prev, grid, actual, tune, sc) # i_w defaults to None seen.add(d.reason) got = (d.target_w, d.sat_count, d.frozen, @@ -635,6 +829,19 @@ for tune in (Tuning(slew_w=1000.0), Tuning(slew_w=1000.0, target_grid_w=-10.0), was = legacy(prev, grid, actual, tune, sc) if got == was: continue + # everything that reaches the wire, without the counter + wire_same = (got[0], got[2], got[3]) == (was[0], was[2], was[3]) + if wire_same: + # the ceiling moved and nothing else did. Only legal + # above release/1.0's own cap, and only with both + # counters standing at their own ceilings. + if (sc >= LEGACY_SAT_CAP + and was[1] == LEGACY_SAT_CAP + and got[1] == _control._SAT_COUNT_CAP): + cap_diffs.append((prev, grid, actual, sc, got[1], was[1])) + continue + diffs.append((prev, grid, actual, sc, got, was)) + continue explained = ( d.reason == "freeze-released" and was[3] == "saturated-freeze" @@ -646,12 +853,28 @@ for tune in (Tuning(slew_w=1000.0), Tuning(slew_w=1000.0, target_grid_w=-10.0), released.append((prev, grid, actual, sc)) if got[0] != was[0]: value_diffs.append((prev, got[0], was[0])) -TOTAL = 3 * 7 * 9 * 4 * 4 +TOTAL = 3 * 7 * 9 * 4 * 5 check(f"i_w=None reproduces release/1.0 over {TOTAL} cases except where" f" SAFETY-13 releases the freeze, reason included" + (f" (first unexplained diff {diffs[0]})" if diffs else ""), not diffs) -check(f"{TOTAL - len(released)} of {TOTAL} cases are bit-identical to" - f" release/1.0; {len(released)} are releases", len(released) == 432) +# ⚠️ EXACT COUNTS, NOT INEQUALITIES. A weaker form of this line ("some +# releases happened") was written while the bound was moving, and the coverage +# audit caught it immediately: a `unasked_power()` that stops discriminating +# went from 6 checks to 5. The three numbers partition the sweep, so any one of +# them moving is a change somebody has to explain. +check(f"{TOTAL - len(released) - len(cap_diffs)} of {TOTAL} cases are" + f" bit-identical to release/1.0; {len(released)} are releases;" + f" {len(cap_diffs)} differ in the saturation counter alone", + len(released) == 432 and len(cap_diffs) == 81 + and TOTAL - len(released) - len(cap_diffs) == 3267) +# ⚠️ THE COUNTER-ONLY DIFFERENCES ARE NOT A LOOPHOLE, so they are pinned twice: +# they may only occur at an `sc` above release/1.0's ceiling, and the largest +# `saturation_cycles` config.yaml accepts is below BOTH ceilings, so no +# accepted configuration can see the difference at all. +check(f"the {len(cap_diffs)} counter-only differences all sit above" + f" release/1.0's cap of {LEGACY_SAT_CAP}, where no accepted" + f" saturation_cycles can read them", + all(r[3] >= LEGACY_SAT_CAP for r in cap_diffs) and _sc_hi <= LEGACY_SAT_CAP) # ⚠️ AND THE RELEASES ARE MOSTLY A LABEL. Only these change what reaches the # wire; the rest report "freeze-released" on a command the freeze was not # binding on anyway. Every one of them is a charge that release/1.0 refused to -- 2.54.0