fix(control): the slew limiter may not re-open a closed charge block - #809
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b3a1f4402
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opts.noSelfDischarge { | ||
| targets = floorNegativeTargets(targets) | ||
| } | ||
| targets = floorBlockedCharge(targets, opts.noSelfCharge, opts.chargeBlocked) |
There was a problem hiding this comment.
Apply the charge floor before deadband exits
When the grid error is already inside GridToleranceW, the legacy path returns through fuseSaverEarlyExit before this newly added floor runs. In an arbitrage idle/live-export or stale-plan tick where the battery is already charging enough to keep the meter near zero (for example meter -50 W, battery +2000 W, tolerance 60 W), noSelfCharge is true but ComputeDispatch emits no zero target, and main.go only sends commands for returned targets, so the previous charging command can continue despite the charge block. The same bypass affects a charge_capable=false battery if the site is in the deadband; the floor needs to be reached (or the early-exit condition suppressed) whenever a charge block must actively stop existing charge.
Useful? React with 👍 / 👎.
48b1048 to
ef7eb0b
Compare
The limiter anchors every target on the battery's MEASURED output, not on the previous command. That is deliberate and right — it lets dispatch pivot the instant a setpoint reverses instead of ramping away from a stale command. But it also means a battery physically charging drags its own command back toward that charge, whatever the tick above decided. noSelfCharge pins the fleet total to 0 W. The limiter then walks it back. Nothing downstream put it right: applyFuseGuard only shrinks toward zero, floorNegativeTargets covers the discharge side only, and planSignIntent reports "idle, no opinion" for an idle slot. A passive-arbitrage idle slot, meter at -2000 W, battery live at +2000 W, SlewRateW 500, commanded +1500 W of charging on the tick whose entire purpose was to let that surplus reach the meter. The same mechanism re-opened a battery that had just reported charge_capable=false, after the distributor had already handed its share to a capable sibling — the fleet charged that share twice. The fix is a floor, floorBlockedCharge, applied after slew beside the discharge-side floor it mirrors. It bounds the output rather than enumerating the reasons the output was closed. The alternative was to complete the snap-to-zero carve-out inside the slew loop, which today names plannerSelfExportSurplusGate and manual hold. That is the enumeration, and it is how this bug was born: two charge gates were added to ComputeDispatch afterwards and neither was added to the list. It is also weaker than it looks — it only fires when the pre-slew target is already within 1 W of zero, so a total pinned to 0 by a +800/-800 split would slip through it in both directions. A floor cannot be born that way and cannot be split that way. Placed after applyFuseGuard rather than before it, so the guard's predicted grid is unchanged and the blast radius is exactly "targets that command charge into a closed direction". Nothing between the floor and the driver raises charge: applyBatteryBoostReserve touches negative targets only, and forceFuseDischarge only forces discharge. The discharge-side twin of the per-driver half is deliberately left alone. A dischargeBlocked battery re-opened by slew is the same shape, but flooring it has to answer whether the fuse emergency in forceFuseDischarge outranks a driver's "I cannot discharge" — a question this change does not need to answer, because charge has no such override. Every new test runs at a rate a site actually runs (250-1500 W) with the battery measured mid-charge; at SlewRateW 100000, the way most older dispatch tests neutralise the limiter, all four bug tests pass against the broken code. The golden corpus deliberately recorded this bug in #799. It moves 11 records and is re-recorded in the next commit, so the fix and the fixture update read apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Predicted before looking, verified after. Eleven records move, all in the slew_limiter family, all the same law: a target the tick had closed, walked back open by the limiter's measured-power anchor. The eight the corpus named as bugs when #799 recorded them: | record | before | after | |---|---|---| | bug_no_self_charge_idle_slot_leak_250 | +1750 W | 0 W | | bug_no_self_charge_idle_slot_leak_500 | +1500 W | 0 W | | bug_no_self_charge_idle_slot_leak_1500 | +500 W | 0 W | | bug_no_self_charge_idle_slot_leak_with_pv | +2000 W | 0 W | | bug_no_self_charge_arbitrage_idle_slot_leak | +1500 W | 0 W | | bug_no_self_charge_idle_slot_leak_two_batteries | +1500 / +1000 W | 0 / 0 W | | bug_no_self_charge_stale_plan_leak | +2000 W | 0 W | | blocked_sibling_slew_reopens_parked_charge | +1500 / +500 W | 0 / +500 W | The first seven are the site-wide charge block: the fleet total was already pinned to 0 W, so 0 W is the answer the tick computed and the limiter overwrote. The eighth is per-driver: ferroamp reported charge_capable=false and goes to 0; sungrow keeps +500 W, which is its own legitimate one-step ramp from a 0 W anchor and not something anything closed. Three more were predicted by shape rather than by name — the seeded half of the family, where the same gates fire on randomly generated sites: | record | before | after | why | |---|---|---|---| | seeded_039_planner_arbitrage | +3000 W | 0 W | idle arbitrage slot, meter exporting 12.6 kW, battery measured +5377 W | | seeded_068_planner_self | +2525 W | 0 W | stale plan, battery measured +4025 W | | seeded_078_planner_passive_arbitrage | +600 / +787 W | 0 / 0 W | idle slot (28 Wh), meter exporting 3.1 kW, both batteries measured charging | Every one of the eleven is a tick where the site charged while the meter exported, or charged hardware that said it could not. None moved that was not predicted. The records predicted NOT to move did not: carveout_export_surplus_gate_snaps_to_zero and its discharge twin are already 0 W — they are the A/B partners whose difference from the bug rows WAS the bug, and they stay put. no_self_charge_idle_slot_slew_3000_reaches_zero stays at 0 W: a 3000 W rate already reached zero in one step, which is why it never leaked. blocked_sibling_slew_reopens_parked_target stays at -1500 W — the discharge-side twin this change deliberately does not touch. The seven other families are NOT re-recorded. None of their records moved, and rewriting their ftw_commit would claim they were re-examined under this change when they were not. Re-recording them does churn ~150 lines of sub-tolerance float noise (e.g. 3704.0000041155554 -> 3704), which is worth knowing about but is not this commit's business. One record inside slew_limiter changes without moving: seeded_023_planner_arbitrage's derived battery_target_sum_w goes from 235.00000000000006 to 235 while its three per-driver targets stay byte-identical. Isolated by re-dumping with the floor disabled: the shift is already present on this branch's base and belongs to the fuse-relief refactor below it, 6e-14 W, fourteen orders of magnitude under the 0.01 W tolerance. 590 records in 8 families, unchanged. Tolerance unchanged at 0.01 W. slew_limiter.json sha256 f7ff7a54bc49321a5c232d58eed9230b2e25e4b3b7474544e32b46bf0b78885b Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7b3a1f4 to
1d4935e
Compare
floorBlockedCharge (#809) runs inside applyDispatchSafetyPipeline. The three early exits return through fuseSaverEarlyExit and never reach it, and an exit that issues no target withdraws nothing: main.go only sends commands for the targets ComputeDispatch returned, and a driver holds its last accepted setpoint until it gets another one. Only the deadband exit turns that into a trap that does not end, because its own condition can be satisfied by the violation. An idle arbitrage slot over a 2 kW solar surplus: the battery absorbs it, the meter reads -50 W, the error stays inside the 60 W deadband, and the tick that would have stopped the charge walks away for as long as the sun holds. The fix is the charge-side mirror of the carve-out already on the line above it. That condition already declines to exit when noSelfDischarge is armed and a battery is measured discharging; it now also declines when a charge authority is closed and a battery is measured charging. The tick then runs the normal control law, which reaches floorBlockedCharge and commands the 0 W the block already decided. Gated on measured charge rather than on the block alone, so a tick with nothing to withdraw stays quiet and no driver is handed a command it could refuse. Idle and holdoff are left alone, with tests for why: the site-wide block cannot be armed at the idle exit, and idle withholds commands from every battery equally by contract; the holdoff window is bounded by MinDispatchIntervalS and then the normal path runs. Golden corpus: no record moves. None of the 33 no-dispatch records has a closed charge direction. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) * test(control): record the early exits meeting a binding protection Three branches of ComputeDispatch walk away from a cycle before the safety pipeline: idle mode, the holdoff window, and the reactive deadband. The golden corpus had no record of any of them meeting a protection that binds. Of the 590 records, 33 returned no targets — 25 idle, 1 holdoff, 8 deadband — and every one of the 8 deadband records ran with site_fuse_amps=0 and peak_import_ceiling_w=0. Nothing was over any limit to defend, so 590 recorded ticks could not see the deadband exit skipping every protection. That is the bug #803 fixed, and #803 left this gap open on purpose: recording new records is a corpus change, not a fix. early_exit_protections adds 21 records, 611 in total. Each shape appears twice — once where the protection binds, once where the same site is configured identically and nothing is over its limit. The quiet half is not filler: a corpus of only-firing records cannot tell a fix from an over-fire, and a fuse-saver that discharges on every tick drains the pack as surely as one that never fires trips the breaker. Covered: deadband against a tariff peak ceiling, against a fuse ceiling left above the breaker by a misconfigured peak limit, against a phase over the breaker on three-phase and on single-phase services, with a safety-amp margin, split across two batteries by discharge headroom, denied by a pack under the 5 % floor, and reached through a stale plan's reactive fallback; idle and holdoff against the peak ceiling and against both phase counts. One record is an export-side phase 10 A over the breaker that must stay quiet, because forceFuseDischarge's only lever is more discharge and honouring per-phase relief there would push that phase further over. Single-phase sites get their own records because the phase count is the conversion #812 gave one owner: on one phase the aggregate meter and the phase are the same wire, so relief is the overage once. Under the bare `* 3.0` those records would command three times the overage. This family records the law as fixed, not a bug. slew_limiter deliberately holds a known defect still so its fix reads as a diff in watts; these records were taken after #803 landed and state what dispatch is supposed to do, so a record that moves is a protection that stopped protecting. The family doc comment says so. Verified against the un-fixed code. Reverting #803's dispatch.go hunk locally moves 7 of the 611 records, all of them deadband records where a protection binds, each reading as the commanded discharge disappearing — deadband_per_phase_3p_binds goes from -1380 W clamped to no targets. The 6 quiet deadband records and all 8 idle and holdoff records hold still, because idle and holdoff already ran the fuse-saver before #803. Re-recording to silence the failure does not work either: the coverage assertions fail on what was just written, naming the deadband exit as unguarded again. The eight existing families are byte-identical; only the new file is added. A full re-recording on this machine rewrites 27 of the 590 old records with float noise up to 8.5 microwatts — a thousandth of the 0.01 W tolerance, and nothing a reviewer should have to read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(control): point slew_limiter.json at the commit it was recorded on The family carried ftw_commit fe2c904, a SHA that is not on master and never was. The other eight families name real commits — seven at c7fe6c9, where #790 recorded them, and the new one at c85bfe1. fe2c904 is the pre-squash tip of #809's branch: `git cat-file` finds it locally with #809's subject line, and `git merge-base --is-ancestor fe2c904 origin/master` says it is not an ancestor of master. GitHub squash-merged that branch as c85bfe1, and `git log -- testdata/golden/slew_limiter.json` shows c85bfe1 as the last commit to rewrite the file: #809's fix moved 94 lines of records, and they were re-recorded in the branch worktree before the squash. So the behaviour these 155 records hold is the behaviour at c85bfe1, which is what the field now says. No record changes; only the provenance line. It matters because the field is the one thing telling a reader which dispatch produced these watts, and a SHA that resolves nowhere on master sends anyone tracing the file's history to a dead end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Both bases have landed: #803 as f4eedde and the per-phase relief fix as #812 (which replaced #808 after GitHub auto-closed it). This branch is now rebased straight onto master and carries only its own two commits, so the whole diff is the change under review.
Two commits on purpose: the fix, then the fixture update, so the behaviour change and the re-recorded corpus read apart.
The bug
The slew limiter anchors every target on the battery's measured output rather than on the previous command. That is deliberate and right — it is what lets dispatch pivot the instant a setpoint reverses instead of spending ten cycles ramping away from a stale command, and the comment above it explains why.
But it also means a battery that is physically charging drags its own command back toward that charge, whatever the tick above decided.
noSelfChargepins the fleet total to 0 W. The limiter then walks it back, and nothing downstream put it right:applyFuseGuardonly shrinks toward zero;floorNegativeTargetscovers the discharge side only;planSignIntentreports "idle, no opinion" for an idle slot, soapplyPlanSignFloorhas nothing to compare against.SlewRateW500The site charged on the tick whose entire purpose was to let that surplus cross the meter. With slew disabled, the same inputs give 0 W.
The same mechanism re-opens a battery that just reported
charge_capable=false: the distributor parks it at 0 and hands its share to a capable sibling, then the limiter anchors it on its own live charge and commands +1500 W into hardware that said it cannot take it — while the sibling is already absorbing that same share. The fleet charges it twice.Complete the carve-out, or add a floor?
There is a snap-to-zero carve-out inside the slew loop already. It fires when a gate wants the battery stopped now rather than ramped, and it names
plannerSelfExportSurplusGate, the scoped-sibling stand-down, and a zero manual hold. Completing it means adding the two charge gates it never learned about.The floor is the right answer, and the carve-out is the reason.
That list is an enumeration of reasons a target was closed. It is exactly how this bug was born:
plannerSelfNoChargeStalePlanandarbitrageFamilyIdleLiveExportGatewere added toComputeDispatchafter the carve-out was written, and neither was added to it. Completing it fixes today's two gates and leaves the same trap armed for the third.It is also weaker than it looks. The carve-out only fires when the pre-slew target is already within 1 W of zero. A fleet total pinned to 0 W by a +800 / -800 split satisfies the gate and defeats the carve-out on both batteries — in both directions.
A floor bounds the output instead. It cannot be born incomplete, because there is nothing to enumerate, and it does not care how the total was split.
floorNegativeTargetsis the same shape on the discharge side; this is its mirror, sitting on the next line.The carve-out stays as it is. Its job is latency — reaching 0 W in one tick instead of ramping — and that job is real for the discharge direction the floor does not touch.
The fix
floorBlockedCharge, applied inapplyDispatchSafetyPipelineimmediately afterfloorNegativeTargets. Two authorities close the charge direction, both decided before slew runs:noSelfCharge— the site-wide block:planner_self's export-surplus gate,planner_self's stale plan, the arbitrage-family idle live-export gate. It pins the fleet total; the floor bounds each command.chargeBlocked— the driver's own capability report, carried past the limiter, which knows nothing about capability.Placed after
applyFuseGuardrather than before it, so the guard's predicted grid is unchanged and the blast radius is exactly "targets that command charge into a closed direction" — nothing else. Nothing between the floor and the driver raises charge:applyBatteryBoostReservetouches negative targets only, andforceFuseDischargeonly forces discharge.The discharge-side twin of the per-driver half is deliberately left alone. A
dischargeBlockedbattery re-opened by slew is the same shape — the corpus records it asblocked_sibling_slew_reopens_parked_target— but flooring it has to answer whether the fuse emergency inforceFuseDischargeoutranks a driver's "I cannot discharge".forceFuseDischargeruns last precisely so it can overridefloorNegativeTargetsfor the physical emergency. Charge has no such override, so it is the half that can be fixed without deciding that. The discharge half deserves its own change and its own answer.Structural direction, not built here
Express slew as a low-rank claim intersected after the authority constraints, so a lower-ranked stage can never widen a higher-ranked one. That makes this whole class of bug unrepresentable instead of repaired, and it is the constraint-algebra direction — a deliberate change of its own, not a rider on a live-site fix.
Golden corpus
The
slew_limiterfamily recorded this bug on purpose in #799, so the fix would show up as records moving. Predicted before looking, then verified.Eleven records move, all in
slew_limiter, all the same law. The eight the corpus named:bug_no_self_charge_idle_slot_leak_250bug_no_self_charge_idle_slot_leak_500bug_no_self_charge_idle_slot_leak_1500bug_no_self_charge_idle_slot_leak_with_pvbug_no_self_charge_arbitrage_idle_slot_leakbug_no_self_charge_idle_slot_leak_two_batteriesbug_no_self_charge_stale_plan_leakblocked_sibling_slew_reopens_parked_chargeThe first seven are the site-wide block: the total was already 0 W, so 0 W is the answer the tick computed and the limiter overwrote. The eighth is per-driver — ferroamp reported
charge_capable=falseand goes to 0; sungrow keeps +500 W, which is its own legitimate one-step ramp from a 0 W anchor and not something anything closed.Three more were predicted by shape rather than by name, in the seeded half of the family where the same gates fire on generated sites:
seeded_039_planner_arbitrageseeded_068_planner_selfseeded_078_planner_passive_arbitrageEvery one of the eleven is a tick where the site charged while the meter exported, or charged hardware that said it could not. Nothing moved that was not predicted.
Predicted not to move, and did not:
carveout_export_surplus_gate_snaps_to_zeroandcarveout_export_surplus_gate_snaps_discharge— already 0 W. These are the A/B partners whose difference from the bug rows was the bug, and they stay put.no_self_charge_idle_slot_slew_3000_reaches_zero— 0 W. A 3000 W rate already reached zero in one step, which is why it never leaked.blocked_sibling_slew_reopens_parked_target— -1500 W. The discharge-side twin, out of scope by design.slew_rate_w=100000, so the limiter never binds and the 0 W the gate pinned already survives;seeded_reactive's 32 realistic rates are reactive modes wherenoSelfChargeis structurally false; and the charge-blocked batteries inseeded_siblingscarry negative targets, which a charge floor does not touch.Only
slew_limiter.jsonis re-recorded. No record in the other seven moved, and rewriting theirftw_commitwould claim they were re-examined under this change when they were not.590 records in 8 families, unchanged. Tolerance unchanged at 0.01 W.
slew_limiter.jsonsha256f7ff7a54bc49321a5c232d58eed9230b2e25e4b3b7474544e32b46bf0b78885b.Two things found while re-recording
slew/seeded_023_planner_arbitrageshifts without moving. Its derivedbattery_target_sum_wgoes from235.00000000000006to235while all three per-driver targets stay byte-identical. Isolated by re-dumping with the floor disabled: the shift is already present on this branch's base, so it belongs to the fuse-relief refactor in fix(control): size per-phase relief by the site's phase count #808 rather than to this change. 6e-14 W, fourteen orders of magnitude under the tolerance, which is why fix(control): size per-phase relief by the site's phase count #808's replay passed and its "zero records move" claim is correct — the record's behaviour did not change. Worth knowing that a re-record surfaces it.The seven un-re-recorded families carry ~150 lines of sub-tolerance float churn (e.g.
3704.0000041155554→3704,-494.3225806451613→-494.32258064516134), accumulated since they were last recorded atc7fe6c98. Nobody should re-record them to make that go away; it just means a future genuine re-record of those families will have noise mixed into its diff.Tests
go/internal/control/slew_charge_floor_test.go. Every test runs at a rate a site actually runs (250-1500 W, next toNewState's 500 W default) with the battery measured mid-charge. AtSlewRateW = 100000, the way most older dispatch tests neutralise the limiter, all four bug tests pass against the broken code — that is the trap #799 was written to close, and these tests stay out of it.Four fail against the old code:
TestSlewMayNotReopenArbitrageIdleChargeBlock— the reproduction watt for watt. Was +1500 W.TestSlewMayNotReopenPlannerArbitrageIdleChargeBlock— same law,planner_arbitragerather than passive. Was +1500 W.TestSlewMayNotReopenStalePlanChargeBlock—planner_selfwith no fresh plan is discharge-only. Was +2000 W. Asserts the gate actually armed, so the test cannot pass vacuously.TestSlewMayNotReopenChargeBlockedBatterysParkedTarget— the per-driver half. Was +1500 W, and pins the capable sibling's +500 W ramp so the floor cannot pass by zeroing everything.Two more guard against over-reach, and pass either way by design:
TestChargeFloorLeavesUnblockedChargeAlone— same slew shape, no gate: the +2500 W ramp survives.TestChargeFloorLeavesDischargeAlone— a charge-blocked tick with the battery measured mid-discharge still ramps to -1500 W. The floor is one-sided; covering live load is not collateral damage.TestFloorBlockedChargeContractpins the function's contract directly: positives only, blocked drivers only, never away from zero.make verifygreen as the pre-commit gate on the re-record commit. The fix commit was made with--no-verifyon purpose — between the two commits the corpus is knowingly stale, which is the point of splitting them.