Files
goodwe-addon/goodwe_controller/DECISIONS/SAFETY-25-loop-soc-floor.md
T
glenn schrooyenandClaude Opus 5 e498dd4c1c
checks / unit (pull_request) Successful in 3m53s
SAFETY-25: pin the claim-source set, so the tripwire is one
The durability claim this PR shipped was FALSE, and the reviewer of #31
disproved it by building the fix. Part 10 said, in four committed places,
that it "goes RED the day a loop-side floor is built, so the exposure
cannot change size under a green suite". The reviewer implemented the
memo's own recommended Option B in main.py - a
`Claim.limit("reserve", P_LOOP, hi=0.0)` filed under a `loop_soc_floor` -
and the ENTIRE SUITE STAYED GREEN. Part 10 green. Mutation 13 went on
printing "NOTHING PREVENTS IT, ON PURPOSE" and the same 21 % -> 10.0 %,
on a tree where something DID prevent it. Pushed to an absurd
always-binding 95 % reserve it was caught only by SAFETY-24's pre-existing
part 7 wiring checks; part 10 was still green.

THE CAUSE IS STRUCTURAL, not a missing assertion. `replay_loop()`
hand-builds main.py's claim list as a literal
`[supervised, loop, peakguard.claim(...)]`, so it is blind to any claim
main.py ADDS - which is exactly where AC 2 would be built. That is this
repository's named failure mode: a check that observed something other
than what it claimed to observe, shipped as a guarantee inside the
document that recommends the change.

THE FIX IS 10f: THE CLAIM-SOURCE SET, PINNED, two ways because the two
fail differently.

  * STATICALLY, over `Controller.cycle()`'s own AST. Every
    `claims.append(...)` in it must be one the scan can read - an
    unreadable append FAILS rather than counting as nothing, which is how
    a scan like this goes quietly vacuous - and the sources it can file
    must be exactly {device, supervised, failsafe, safety, peak,
    arbitrage, maintenance, loop}. A ninth takes part 10 red on the line
    it is written, in every configuration, whether or not any fixture
    here reaches the state that files it.
  * DYNAMICALLY, through REAL `Controller.cycle()`s with main.py's OWN
    `resolve` spied - not a list this file built and hoped matched -
    swept over the WHOLE 0..100 % SoC range in the two states 10a-10c
    replay. Idle: exactly {device, supervised, loop}. Floor-limited:
    exactly {device, supervised, loop, peak}. A claim conditioned on a
    low pack, which is the shape a loop-side floor has, cannot hide from
    a sweep that visits 0 %.

And the fold is checked rather than assumed: main.py's two P_SAFETY
rating limits are both symmetric LIMITs, so replay_loop() modelling them
as one `supervised` claim at the tighter bound is exact.

MEASURED AGAINST THE COUNTEREXAMPLE. Reproduced first on the unfixed
tree (green, as reported). On this tree the reviewer's Option B at its
recommended default takes part 10 RED on 3 checks, and the 95 % variant
on 6. Nine mutations were run in all, each against a freshly copied tree
with no `__pycache__` present, mounted `:ro`; all nine red, no survivors,
no `+0 B` delta. MUTATION.md records why a second round was needed: the
first round's mutations were all aimed at DISARMING the guard, while the
claim part 10 actually made was about the guard being FIXED.

The four false sentences are replaced with what is now true, and each
says WHICH half catches WHICH shape - 10f for an arbiter claim
(Options B and C), 10a-10c for a clamp inside control.compute() or the
guard (Option D), which the replay drives directly.

AC 2 IS STILL OPEN AND NOTHING HERE BUILDS IT. The loop-side floor was a
test mutant, reverted before commit. The memo still says "Status: OPEN.
This is a memo, not a decision."

Also, non-blocking, from the same review: the NOT READY banner had
inherited the new scope sentence and become a ~550-character paragraph
that the row three lines below repeated verbatim. A checks() row may now
carry an optional short `banner` form; the floor-limited row does, and
`status_payload()` prefers it. THE ROW IS UNCHANGED - shortening that
would have been weakening AC 3 - and rows without the key are
unaffected. Graded, and mutated: dropping the short form goes red.

Merged origin/release/1.0 first (CI-02), keeping both tickets' CHANGELOG
entries.

Full suite green: 17/17 in goodwe-addon-ci (test_ci_gate.py needs `git`,
which CI-02 added to .gitea/ci/Dockerfile but the local image lacks; run
with it installed and it passes).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 05:04:22 +02:00

277 lines
15 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# SAFETY-25 — does the grid-following loop get a reserve floor?
**Status: OPEN. This is a memo, not a decision.** It sets out the options, what
each costs, what each protects, and a recommendation. **The choice belongs to the
product owner**, because it decides whether this house's battery is allowed to
discharge to its BMS cut-out on a sustained-load evening, and that moves real
energy.
Filed with SAFETY-25. The other half of that ticket — making `peak_soc_floor`'s
scope honest in the config, the docs and on the status page — **shipped in the
same PR** and did not wait on this, because it is required whichever way the
answer goes.
*This directory exists so a recorded decision has somewhere to live that is not a
comment in a source file nobody reads and not a Trello card that scrolls away.
`SAFETY-27` asks for a second one (the naming of the floors); it belongs here
too.*
---
## 1. The question
> Does the grid-following (self-consumption) loop get an SoC floor of its own, or
> is the BMS cut-out **deliberately** the only backstop under demand-driven
> discharge?
Either answer is defensible. What is not defensible is the state we have been in
for four tickets: **silence, plus a config key called `peak_soc_floor` that reads
like the answer is "yes".**
## 2. What is actually exposed — replayed, not argued
The exposure was written down three times (SAFETY-18's finding, SAFETY-24's
module note and PR body, and SAFETY-24's honestly-uncaught 13th mutation) and
never measured. SAFETY-25 measured it. The replay is `test_peakguard.py` part 10;
it drives the **shipped** `control.compute()`, `peakguard.decide()`,
`peakguard.claim()` and `arbiter.resolve()` over the committed capture
`scenarios/p1-net-2026-08-20.jsonl.gz`, with the battery and the state of charge
modelled (the captures carry meter power only — see §7).
### 2a. A sustained-load evening takes the pack to the BMS cut-out
19:00–22:00 UTC on 2026-08-20 — three hours of house load after the sun has gone,
**830 W mean, minimum 701 W, maximum 1223 W, not one watt of export in the whole
window.** The pack is the 13.8 kWh reference pack (300 Ah × 51.2 V × DoD 90),
started at **21 %** — one point above the shipped `peak_soc_floor`.
| | |
|---|---|
| pack at the start of the evening | **21.0 %** |
| pack three hours later | **10.0 %** — `bms_soc_floor` |
| energy the loop delivered | **1525 Wh** |
| control cycles with the pack below `peak_soc_floor` | **549 of 587** |
| …of those, cycles still delivering discharge | **343** |
| cycles on which the peak guard filed *any* floor | **0** |
### 2b. It is a behaviour, not a config value
The same replay, same pack, same evening, with `peak_soc_floor` set to **5, 11,
20 and 60** — the shipped value and both ends of the schema range:
peak_soc_floor = 5 -> ends at 9.9715 %, 1524.58 Wh out
peak_soc_floor = 11 -> ends at 9.9715 %, 1524.58 Wh out
peak_soc_floor = 20 -> ends at 9.9715 %, 1524.58 Wh out
peak_soc_floor = 60 -> ends at 9.9715 %, 1524.58 Wh out
Identical to four decimal places. **Raising the option does nothing, because the
guard it bounds is not the thing doing the discharging.**
### 2c. The two states coexist in the same cycle
On the same day's high-load block (15:00–17:00 UTC), with the pack started one
point *below* the floor, the guard reports `PEAK GUARD FLOOR-LIMITED` —
*"commanding no forced discharge"* — on **49 cycles**, and on **all 49** the
arbiter simultaneously grants the loop a discharge of up to **2000 W**. The
status page tells the operator the truth about the guard while the pack empties.
### 2d. Stated fairly, because overstating it is how a real finding gets ignored
This is **not** a runaway. Self-consumption discharge is demand-driven and
self-limiting: it stops when the house stops drawing, and every watt of it is
serving load that would otherwise have been imported. It is not SAFETY-18's
frozen-meter rail, and nothing here is spending the pack for a reason of its own.
What it means is narrower and still real:
- the **depth** the pack reaches on a heavy evening is set by the house, not by
anything an operator can configure; and
- `peak_soc_floor` **reads like a system-wide guarantee it is not.** A floor that
binds one consumer and not the other is worse than no floor if its name does
not say so — which is precisely what SAFETY-25's other half fixed.
## 3. Why SAFETY-24's floor cannot be extended to cover this
The peak guard files a `lo` limit — a **lower** bound on the setpoint. That
asymmetry is what makes the guard trustworthy: a reviewer proved across 19,800
combinations that it can never lower a target, never cap a discharge, never
contradict the arbiter, and never file a negative floor. `peakguard.claim()`
leaves `hi` at `+inf` as a fact about the object rather than a rule someone has
to remember.
**A `lo` limit is structurally incapable of stopping a discharge the loop itself
wants.** So the same property that makes the guard safe makes it useless here.
Raising `peak_soc_floor` cannot become the answer; §2b is that statement
measured.
## 4. The floors we already have — and this would be the fifth key
SAFETY-24 recorded a decision to keep `maintenance_soc_floor` separate with an
explicit revisit trigger: *"if a third floor appears."* **The trigger has already
fired, unremarked**, when OPT-04 shipped `arbitrage_soc_floor`.
| option | default | bounds | priority of the consumer |
|---|---|---|---|
| `maintenance_soc_floor` | 11 | the monthly recalibration's **drain target** | `P_MAINTENANCE` 60 |
| `peak_soc_floor` | 20 | the peak guard's forced discharge | `P_PEAK` 50 (a `lo` limit) |
| `bms_soc_floor` | 10 | *nothing* — it describes the pack, so rule 8 can refuse a floor that cannot bind | — |
| `arbitrage_soc_floor` | 20 | arbitrage discharge | `P_ARBITRAGE` 30 |
| **the loop** | — | **nothing** | `P_LOOP` 10 |
Two of them ship the same default (20) from different cards for different
reasons, which is how a value that looks deliberate becomes a coincidence nobody
dares change. **`SAFETY-27` owns the naming question and this memo does not
pre-empt it** — but any answer to §1 that adds a fifth key should land *after*
SAFETY-27's decision, or it adds a fifth accreted name to four.
## 5. The options
### Option A — record the BMS as the deliberate backstop; add nothing
Say plainly, in `DOCS.md` and in `freeze.py`'s finding, that demand-driven
discharge is bounded by the pack's own cut-out on purpose, and close the
question.
- **Protects:** the whole point of the product. Every watt the loop discharges is
self-consumption the house would otherwise have imported; a reserve is
literally energy bought at the import tariff and held back.
- **Costs:** on a heavy evening the pack ends at the BMS cut-out with no EMS-side
record of why. That is the SoC-drift condition **MAINT-01** exists to correct,
and the first symptom — a battery that is never available when it is wanted —
is indistinguishable from a coulomb counter that has drifted. It also means the
morning's peak-guard capacity is whatever last night left, which the peak guard
cannot influence.
- **Cost of building it:** zero. It is already the shipped behaviour, and after
SAFETY-25's honesty half it is already documented.
### Option B — a `hi` limit at `P_LOOP`, with an enforced ordering *(recommended)*
A new option `loop_soc_floor`. While `soc <= loop_soc_floor`, `main.py` files
`Claim.limit("reserve", P_LOOP, hi=0.0)`. Zero forbids **discharge** and permits
charging, which is the mirror image of the guard's `lo = 0` and one-directional
in the safe direction.
**⚠️ This shape has one trap and it is fatal if missed.** `arbiter.resolve()`
applies **every** limit at or above the winning `set` claim's priority and takes
the most restrictive, *with no notion of one limit outranking another* — that is
deliberate and is what makes the safety bounds unconditional. So when the loop
wins at `P_LOOP`, a reserve `hi = 0` and the peak guard's `lo = excess > 0` are
**both** applied, `lo > hi`, and the arbiter resolves a **CONTRADICTION to 0 W**:
the battery parked, in the one quarter of the month that sets the bill. This is
the same crossing-limits hazard `shaping_suspended()` already exists to avoid for
the maintenance charge-only clamp.
**The fix is an ordering constraint, and it is provable:** require
bms_soc_floor < loop_soc_floor <= peak_soc_floor
Then whenever the reserve binds (`soc <= loop_soc_floor <= peak_soc_floor`) the
guard is *already* floor-limited and files `lo = 0.0`, so `lo == hi == 0` and the
result is 0 W with no contradiction. Refuse any other ordering at startup, in
CFG-01's manner and with rule 8's message shape. **A `loop_soc_floor` above
`peak_soc_floor` must not be merely discouraged; it is the parked-battery bug.**
- **Protects:** the pack from routinely sitting on its own cut-out, which is the
MAINT-01 drift condition; and it gives the operator one number that means what
its name says.
- **Costs:** a fifth SoC key (see §4), a validator rule, and — the real cost —
**every watt below the reserve becomes billed import.** Set too high it turns a
working battery into a smaller one.
- **What it does *not* touch, correctly:** a maintenance DRAIN sets at
`P_MAINTENANCE` 60 and arbitrage sets at `P_ARBITRAGE` 30, both above `P_LOOP`
10, so a limit filed at `P_LOOP` is not applied to either. The monthly
recalibration still drains to `maintenance_soc_floor`; arbitrage is still
bounded by its own key. That is the right answer and it falls out of the
priorities rather than needing a special case.
- **Recommended starting value if this option is taken:** a *low* one — three
points above `bms_soc_floor`, i.e. **13 %**, well under `peak_soc_floor`'s 20.
The purpose is to keep the pack off its own cut-out, **not** to reserve energy
for anything, and an over-tight reserve is the version of this that fights
MAINT-01 from the other side: it shrinks the band the pack actually cycles in,
which is the condition a coulomb counter drifts in. It does not matter that 13
sits above `maintenance_soc_floor`'s 11 — the maintenance drain sets at
`P_MAINTENANCE` 60 and a `P_LOOP` limit is not applied to it, so the two never
interact. **The number itself is part of the owner's answer**; what this memo
offers is the derivation and the ordering constraint that makes any number in
that range safe.
### Option C — a `set` claim above `P_PEAK`
File `Claim.set("reserve", 55, 0.0)` when the pack is under the reserve. Because
clause 2 applies only limits at or **above** the winner's priority, the guard's
`lo` at 50 would then be dropped and there is no contradiction to engineer
around.
- **Protects:** the same as B, without the ordering rule.
- **Costs:** it is a **stop**, not a bound. It parks the battery outright rather
than merely forbidding discharge, so it also blocks the charging that would end
the condition, and it silently outranks peak shaving in a way that is invisible
on the priority table. This project's rule is that a floor about **energy** is
not a stop of last resort (`peakguard.py`, point 1). **Not recommended.**
### Option D — clamp inside `control.compute()`
- **Costs:** it puts precedence back inside a strategy, which is the exact thing
`arbiter.py` was built to end, and it would be invisible to `explain()`. **Not
recommended.**
## 6. Recommendation
**Option A is honest and cheap; Option B is what I would build.** Concretely:
1. **Now (shipped with SAFETY-25):** the honesty half. `peak_soc_floor`, its
documentation and the status page say plainly that it bounds the peak guard
only. This is required under every option and it is the actual dishonesty on
the live system today.
2. **Owner's call:** A or B. If B, take it **after `SAFETY-27`'s naming decision**
so the fifth key lands into a scheme rather than onto a pile, and **after
`MAINT-01`** has run one real cycle — until then nobody has observed what this
pack's SoC reading does over a full cycle, and picking a reserve against a
reading that has never been recalibrated is picking a number against an
instrument we have not checked.
3. **Whichever way:** the tripwire in `test_peakguard.py` part 10 stays until the
question is answered, so the day someone builds a loop-side floor it goes red
and has to be updated deliberately, rather than the exposure quietly changing
under a green suite. It has two halves and they cover the two places the floor
can land:
- **10f pins the claim-source set.** Every `claims.append(...)` in
`Controller.cycle()` is read off its AST and must be one of eight named
sources, and real cycles — spied at main.py's own `resolve`, swept over the
whole 0–100 % SoC range in both the idle and the floor-limited state — must
file exactly those. **Option B is a ninth source**, and so is **Option C**'s
`set` claim, so filing either takes part 10 red on the line it is written.
**Option A** files no claim at all — it is the honesty half already
shipped, and part 10 is the record it leaves behind.
- **10a–10c replay the shipped code**, so **Option D** — a clamp inside
`control.compute()` — moves the replayed numbers instead.
⚠️ **10f is load-bearing and was added late.** PR #31 shipped this section
claiming the tripwire without it, and the reviewer disproved the claim by
building Option B in `main.py`: the entire suite, part 10 included, stayed
green, and mutation 13 went on printing *"nothing prevents it, on purpose"* on
a tree where something did. Do not remove 10f while this question is open.
## 7. What the replay is and is not
The committed captures carry **one** series — signed net power at the meter — so
the pack is modelled, and the model is stated rather than hidden:
- the captured `w` is the load presented to the meter; the modelled battery's
delivered power is subtracted from it, one telegram late (an actuation delay);
- the loop's command is the **shipped** `control.compute()` at the shipped
tuning, resolved through the **shipped** `arbiter.resolve()` against the
**shipped** `peakguard.claim()`, at a `cycle_floor_s` tick;
- SoC falls by the delivered discharge integrated over the real telegram
timestamps, on a pack of 300 Ah × 51.2 V × DoD 90;
- the BMS is modelled as a hard cut: at or below `bms_soc_floor` the modelled
battery delivers 0 W whatever is asked of it. Without that the run would be
unphysical — the BMS really does stop the pack, and the whole finding is that
it is then the **only** thing that does;
- nothing recharges the pack inside the window. One evening, one direction.
**The fixtures cannot express a low pack on their own** — there is no battery or
SoC telemetry in them — so the starting state of charge is a fixture parameter,
not a measurement. It is chosen as *one point above `peak_soc_floor`*, which is
derived from the shipped option rather than picked. Everything downstream of that
one number is the real capture and the real code.