SAFETY-04: clamp the integrator independently of the output, freeze it while saturated #1
Closed
admin
wants to merge 0 commits from
SAFETY-04 into release/1.0
pull from: SAFETY-04
merge into: :release/1.0
:master
:release/1.0
:PRICE-02
:UX-46
:SAFETY-121
:SAFETY-119
:UX-45
:DOC-8
:deploy-0.18.2
:release-0.18.2
:SAFETY-118
:deploy-0.18.1
:release-0.18.1
:UX-44
:UX-43
:deploy-0.18.0
:release-0.18.0
:OPT-20
:PRICE-01
:MAINT-11
:opt15-price-run-fixes
:SAFETY-112
:OPT-17-forecast-entities
:deploy-0.17.0
:release-0.17.0
:MAINT-15
:SAFETY-107
:MAINT-14
:MAINT-13-full-pack-complete
:SAFETY-103
:deploy-0.16.0
:release-0.16.0
:safety-95-drain-end-reseed
:safety-96-stale-unit-site-total
:maint-10-local-start-hour
:safety-90-peak-guard-gap
:deploy-0.15.0
:release-0.15.0
:ci-14-infra-pin-3b4ff84
:safety-94-no-false-stale-or-refusal
:deploy-0.14.0
:release-0.14.0
:ci-13-infra-pin-0c04488
:safety-93-no-grid-charge-at-charge-end
:safety-91-held-battery-last
:deploy-0.13.0
:release-0.13.0
:safety-85-no-false-not-keeping-up
:safety-87-guard-uses-able-batteries
:safety-86-peak-over-hold
:safety-84-n2-maint-charge
:safety-83-peak-over-drain
:opt11-p4-solar-forecast
:safety-78-drain-at-two
:safety-81-load-too-high
:opt11-p3-price-runs
:safety-79-peak-margin
:opt11-p2-ha-entities
:UX-13-p3-batteries
:opt11-p1-modes
:UX-13-p5
:UX-15-setup-only
:UX-13-p4-setup
:UX-13-p2-installer
:SAFETY-73-per-unit-refusal
:UX-13-p1-home
:UX-13-P0-foundation
:SAFETY-52-refused-hold
:UX-11-short-safety-lines
:pin-bump-d637d0b
:UX-10-plain-leftovers
:pin-bump-ecd3864
:UX-08-2d-logs-docs
:UX-08-2c-refusals
:UX-09-2a-fixes
:UX-08-status-rows
:pin-bump-7081df4
:UX-08-plain-notifications
:SAFETY-40-clamp-alarm
:pin-bump-7689816
:SAFETY-34
:pin-bump-a1d0618
:TOOL-9
:CI-12
:TEL-14-pv-sign
:DOC-5-watchdog
:TEST-26
:SAFETY-62-refuse-unread-input
:safety-61-control-task-death
:doc-1-operator-docs
:PKG-18-cut-0.12.0
:TEL-12
:PKG-17
:TEL-11
:ci9-gateproof
:SAFETY-56
:SEC-2-release-procedure
:PKG-16-release-0.10.1
:SAFETY-57
:pkg15-changelog
:CI-9
:SAFETY-48
:TEL-10
:PKG-14
:SAFETY-47
:SAFETY-10
:SAFETY-45
:TARIFF-16
:TARIFF-18
:TARIFF-17
:TARIFF-11
:TARIFF-13
:TARIFF-15
:TARIFF-07
:TARIFF-14
:TARIFF-12
:TARIFF-10
:TARIFF-09
:TARIFF-08
:FCST-06
:FCST-05
:FCST-04
:INSTR-01
:SAFETY-44
:ARCH-02
:SAFETY-43
:TEST-21
:SAFETY-31-attribution
:UX-04
:PKG-13
:PKG-12
:PKG-09
:OPT-09
:UX-02
:PKG-07
:SAFETY-42
:SAFETY-13
:SAFETY-37
:SAFETY-39
:SAFETY-38
:CI-06
:PKG-06-version-0.7.0
:SAFETY-41-reassert
:TEL-07
:UX-01
:SAFETY-32
:SPIKE-04a
:MAINT-05
:SAFETY-30
:CFG-04
:DATA-01
:DATA-02
:SAFETY-24
:SAFETY-26
:OPT-04
:FCST-03
:SAFETY-07
:TARIFF-03
:TARIFF-02
:TARIFF-01
:TEST-06
:SAFETY-03
:SAFETY-18
:MAINT-04
:safety-02-latch-persistence
:safety-02-sign-watchdog
:safety-12-loop-floor
:CFG-01
:TEL-05
:TEL-04
:DEPLOY-01
:TEL-01
:SAFETY-04
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes SAFETY-04 (P0). Body rewritten after review round 1 — the original described the rejected first cut.
The structural finding.
control.pywas in velocity form:want = prev_w + gain*error, withprev_wfed back post-clamp. The accumulator was the commanded power, so "clamp the integrator, not only the output" (§10.3) was not expressible. The accumulator is now explicit (i_w, carried bymain.py) with its own bound,integrator_max_w.The bound. Default is "follow
max_w" (integrator_max_w: 0= unset). It is a separate key because the useful direction is below the rail, where it binds first and caps unwind latency tighter thanmax_wdoes. Above the rail it only adds windup thatrelease/1.0did not have — measured on the 4000 W load-drop sim, the command on the first cycle after the drop is 1000 W at the default and 1800 W at 3000 W.DOCS.mdsays so, and a test asserts both halves of it.The freeze. While saturated the integrator may not wind further in the direction it is already pushing; it may fall, cross zero, or reverse. This is the same asymmetric rule the output freeze has always used. It is deliberately not encoded as "only corrections that shrink |i_w|" — that is unsatisfiable for both signs of error whenever the correction exceeds twice the integrator, so near zero the loop deadlocks and the freeze can never clear (clearing needs the inverter to track, and not-tracking is the definition of saturation). That was blocker S-1 and it now has two regression tests.
Entering failsafe.
main.pyre-seedsi_wfrom the arbiter's applied target whenever the loop did not get what it asked for. Every layer-1 stop resolves to 0 W, so entering failsafe zeroes the integrator and the first cycle after release does not dump the stale period as power.Seconds vs samples. Corrected, and it is worse than the spec's ambiguity:
cycle()runs only when the meter value changes, sosaturation_cyclescounts distinct meter readings and has no wall-clock bound at all. That is detection latency rather than a windup hazard — the same stall freezes the whole loop — but the comment and DOCS no longer claim "~15 s". A guaranteed-seconds window would have to live inController, which has a clock;compute()deliberately does not.Tests.
python test_control.py: 24 (release/1.0) → 41, all passing.test_arbiter.py/test_maintenance.pyunchanged and passing. Includes the §10.3 runaway replay (130 W per 4 s = a standing 217 W error at gain 0.6, inverter reporting 0 W, 150 cycles) asserted on the emitted command; the deadlock repro ati_w=0andi_w=+100; a carried-i_wconvergence/overshoot sim (the shipped configuration, previously uncovered); the post-saturation unwind figure; and a 3024-case exhaustive equivalence check proving thei_w=Nonepath still reproducesrelease/1.0exactly, freeze branch included.Non-vacuity, all verified by mutation. Restore the rejected freeze gate → 3 red (including the equivalence check, which is independent confirmation of S-3). Delete the integrator clamp → 4 red, accumulator reaches 19 500 W. Put the default bound back above
max_w→ 4 red.Coordination.
main.pyandconfig.yamlchanges stay additive in the options wiring and schema. TEL-01 has already stacked on this branch; they do not touchcontrol.py, so their rebase is the two option lines.Review round 1 addressed in
e461755, pushed to the same branch. Body above rewritten; it described the rejected cut.i_w=0against a 2 kW import now commands 1000 W and the freeze clears the next cycle. Two regression tests, both go red against the old gate.integrator_max_wdefault is now "followmax_w" (0= unset), schemaint(0,15000), DOCS row inverted. Post-saturation unwind: 1000 W at the default vs 1800 W at 3000, asserted in a test so the DOCS advice is checkable.release/1.0law, plus the carried-i_wconvergence/overshoot sim that was missing. Confirming your diagnosis: the old gate breaks that equivalence test, which is how it would have been caught.compute()rather than moved toController: the stall is detection latency, not windup, since the same condition stalls the loop. Say the word if you want the real seconds window and it goes inController.24 -> 41 checks. Mutation results: old gate 3 red, clamp deleted 4 red (19 500 W), default above
max_w4 red.Dead-reason defect fixed in
7123aa0, same branch.i-clamped. Contract for SAFETY-03's owner: that is the signal to alarm on, notclamped- at the defaultlimit == max_wthe integrator is truncated first and the command derived from it can then never exceedmax_w, soclampedcannot fire on a default install. It stays reachable for a config that deliberately lets the integrator run above the rail, where both fire andclampedwins because it describes the value actually emitted. Both are pinned by tests.(target_w, sat_count, frozen, reason)-frozenwas missing too. The deliberate rename is aliased explicitly rather than papered over, so any other reason divergence still goes red.Answering the question about the 189 cases: on my 3024-case sweep it is 105, and every single one is the
i-clamped/clampedrename - one distinct pair, nothing else. Value divergences remain zero withfrozennow compared. New reason set{tracking, deadband, i-clamped, slew-limited, saturated-freeze}, same cardinality as the old five.Mutations: drop the emit -> 2 red including the sweep (i.e. the widened tuple catches exactly the defect you found); emit it unconditionally -> 5 red; delete the
clampedline -> 1 red. 41 -> 43 checks.test_arbiter.py/test_maintenance.pyunchanged and passing. Grepped for consumers of the reason strings: none outsidecontrol.py, so today's impact really is observability only.Zero-boundary defect fixed in
53d301b, same branch.i_w == 0.0is now its own branch and both directions are allowed - nothing is wound, so "may not wind further" has no referent, and a first step from zero is bounded by the gain, the output clamp and the slew limit like any other step.One correction to the report, and it matters for re-test. The tie-break was NOT what made the command stick at 0 W in the quoted scenario. At
prev_w == 0with the inverter reporting -600 W,release/1.0holds 0 W forever too - verified side by side, same values cycle for cycle - because the output freeze deliberately forbids starting a charge while saturated ("commanding 0 while the inverter reports hundreds of watts means something else is driving the bus").sat_countpinning at 10 is likewise the counter saturating by design. After this fix the integrator moves to -1200 W but the command still holds at 0 W in that exact case, and there is now a test asserting that on purpose so nobody removes it by accident. If the intent is that the command should move there, that is a change torelease/1.0's output-freeze rule and wants its own ticket - it is a real safety rule, not a leftover.The defect is real regardless, and it does reach the wire where
prev_wis already charging: measured ati_w == 0.0and frozen, 12 800 of 25 920 ticks held the integrator, 8 304 of them changing the emitted command, worst case abandoning a 2 kW charge into a 4 kW export (-1000 Wwhererelease/1.0commands-2000 W).On "three of us missed the same half": the durable fix is a property, not more points. Over 13 041 frozen states the suite now asserts the integrator may be held ONLY by a correction pushing it further from zero on the side it already sits; any other hold fails, whichever side it is on. It catches the reported defect, its sign-mirror, and a freeze-everything-at-zero variant. Points would not have.
I also swept the suite for one-sided tests, per your note, and mirrored all of them: freeze wind/unwind while charging,
i_w=-100under import, the export-direction runaway, the negative clamp and the negative slew.Nothing else surfaced at that boundary: 128 160 frozen ticks swept across
i_wand grid, zero illegitimate holds remaining.DOCS now carries the cycles-vs-seconds deviation explicitly - the "> 10 s" criterion is not met as literally written, a cycle is one changed meter reading, no guaranteed wall-clock window, and why that is acceptable.
43 -> 55 checks, all passing;
test_arbiter.py18 andtest_maintenance.py21 unchanged. Mutations: reported defect restored -> 3 red; sign-mirrored (>= 0) -> 4 red including the equivalence sweep; freeze-everything-at-zero -> 6 red.Noted on the
i_clamped: boolcorrection - agreed, last-writer-wins makes the string unusable as an alarm hook, and SAFETY-03 owning the boolean is the right split.AC 3 now has a non-vacuous test -
389d9ec, same branch. The lead's diagnosis and their fix were both exactly right:TFlifted tomax_w=5000, all 55 stay green, and deleting the integrator freeze outright now fails 2 where it failed 0.I then swept the rest of the function for the same pattern rather than eyeballing it - delete one mechanism, count which checks notice - and it found one more instance: the output clamp.
clamped to max_wandclamped to -max_wwere both satisfied by the integrator bound truncating first, so deleting the output clamp failed only the reason-string check. Those two fixtures now setintegrator_max_wabovemax_w, so the mechanism each one names is the binding one. Output clamp: 1 failure -> 3.Final state, every mechanism in
compute()caught by a check that names it:max_wtarget_grid_wbiasi_w=NoneseedingNothing at zero.
A method warning that affects how you read any mutation figure from me, including earlier rounds. Rewriting
control.pyrepeatedly inside one second can leave a staleapp/__pycache__entry, and the suite then runs the OLD bytecode and under-reports. It bit me here: one mutation reported 2 failures where the true figure is 6. The audit now runs withPYTHONDONTWRITEBYTECODE=1and clears the cache first. The earlier rounds' numbers were taken from separate sequential runs with enough elapsed time between them, and you and the lead reproduced all of them independently, so they stand - but the hazard is real for anyone re-running this quickly in a loop.The audit script is not committed: it hardcodes exact source lines from
control.py, so it would go stale on the next edit and generate noise. It is a throwaway tool, ~60 lines, and I can commit it or hand it over if you would rather it lived in the repo.55 checks (unchanged - this round fixed what the existing checks actually test, it did not add any),
test_arbiter.py18,test_maintenance.py21, all passing.Audit recorded as a comment block in
test_control.py, next to the checks it describes -680461c. Script stays in my scratchpad; agreed on all three grounds, and the decomposition argument is the decisive one.The block carries the commit it was measured at (
389d9ec), the thirteen figures, and the invariant with the teeth in it: every mechanism incompute()must be noticed by at least two checks when deleted; adding a mechanism means re-running the audit. Plus the technique itself in prose, since there is no script to point at, and the two rules the misses produced - a fixture must sit clear of every rail it is not testing, and mutations run underpython -Bwithapp/__pycache__cleared.I re-measured the figure that differed rather than transcribing it. Both forms reproduce exactly here:
saturated_now = Falsegives 11, the weakerfrozen = Falsegives 10. Your read of the cause was right, and the table names the form so the next person reproduces the same one.Also wrote in your sharpening of the caching mechanism - invalidation on (source mtime in whole seconds, source size), so a same-second rewrite that preserves file size reuses stale bytecode - along with why it casts no doubt on the numbers: the error is one-directional, so every figure is a lower bound at worst, and the only two zeros ever recorded were both confirmed by fixing them and watching the count rise, which an artefact cannot do.
git diff 389d9ec..680461cistest_control.pyonly, 57 insertions, no deletions. 55 checks,test_arbiter.py18,test_maintenance.py21, all passing.Nothing pre-empted on the four sweep targets - they are the tester's.
Closing: the content of this branch is already on
release/1.0—git rev-list --count origin/release/1.0..returns 0.The work shipped, but it reached the release branch by a different route (a direct push, or carried in with another merge) rather than through this PR, so Gitea never auto-closed it. That is a process failure on my part: it left the PR list looking like nothing had shipped, and it orphans any review discussion held here.
Nothing is lost and nothing needs re-doing. Going forward, work lands by merging its PR.
Pull request closed