Skip to content

fix(control): the slew limiter may not re-open a closed charge block - #809

Merged
frahlg merged 2 commits into
masterfrom
agent/fix-slew-reopens-charge-block
Aug 5, 2026
Merged

fix(control): the slew limiter may not re-open a closed charge block#809
frahlg merged 2 commits into
masterfrom
agent/fix-slew-reopens-charge-block

Conversation

@frahlg

@frahlg frahlg commented Aug 4, 2026

Copy link
Copy Markdown
Member

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.

noSelfCharge pins the fleet total to 0 W. The limiter then walks it back, and nothing downstream put it right:

  • applyFuseGuard only shrinks toward zero;
  • floorNegativeTargets covers the discharge side only;
  • planSignIntent reports "idle, no opinion" for an idle slot, so applyPlanSignFloor has nothing to compare against.
passive-arbitrage idle slot, meter -2000 W, battery live +2000 W, SlewRateW 500 commanded
what the tick decided 0 W
what reached the driver +1500 W of charging

The 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: plannerSelfNoChargeStalePlan and arbitrageFamilyIdleLiveExportGate were added to ComputeDispatch after 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. floorNegativeTargets is 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 in applyDispatchSafetyPipeline immediately after floorNegativeTargets. 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 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 else. 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 — the corpus records it as blocked_sibling_slew_reopens_parked_target — but flooring it has to answer whether the fuse emergency in forceFuseDischarge outranks a driver's "I cannot discharge". forceFuseDischarge runs last precisely so it can override floorNegativeTargets for 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_limiter family 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:

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 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=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, in the seeded half of the family where the same gates fire on 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. Nothing moved that was not predicted.

Predicted not to move, and did not:

  • carveout_export_surplus_gate_snaps_to_zero and carveout_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.
  • The other seven families. They run 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 where noSelfCharge is structurally false; and the charge-blocked batteries in seeded_siblings carry negative targets, which a charge floor does not touch.

Only slew_limiter.json is re-recorded. No record in the other seven moved, and rewriting their ftw_commit would 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.json sha256 f7ff7a54bc49321a5c232d58eed9230b2e25e4b3b7474544e32b46bf0b78885b.

Two things found while re-recording

  1. slew/seeded_023_planner_arbitrage shifts without moving. Its derived battery_target_sum_w goes from 235.00000000000006 to 235 while 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.

  2. The seven un-re-recorded families carry ~150 lines of sub-tolerance float churn (e.g. 3704.00000411555543704, -494.3225806451613-494.32258064516134), accumulated since they were last recorded at c7fe6c98. 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 to NewState's 500 W default) 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 — 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_arbitrage rather than passive. Was +1500 W.
  • TestSlewMayNotReopenStalePlanChargeBlockplanner_self with 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.

TestFloorBlockedChargeContract pins the function's contract directly: positives only, blocked drivers only, never away from zero.

make verify green as the pre-commit gate on the re-record commit. The fix commit was made with --no-verify on purpose — between the two commits the corpus is knowingly stale, which is the point of splitting them.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@frahlg
frahlg force-pushed the agent/fix-per-phase-relief-phase-count branch from 48b1048 to ef7eb0b Compare August 5, 2026 04:15
@frahlg
frahlg deleted the branch master August 5, 2026 04:21
@frahlg frahlg closed this Aug 5, 2026
@frahlg frahlg reopened this Aug 5, 2026
@frahlg
frahlg changed the base branch from agent/fix-per-phase-relief-phase-count to master August 5, 2026 04:22
frahlg and others added 2 commits August 5, 2026 06:22
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>
@frahlg
frahlg force-pushed the agent/fix-slew-reopens-charge-block branch from 7b3a1f4 to 1d4935e Compare August 5, 2026 04:26
@frahlg
frahlg merged commit c85bfe1 into master Aug 5, 2026
13 checks passed
@frahlg
frahlg deleted the agent/fix-slew-reopens-charge-block branch August 5, 2026 04:29
frahlg added a commit that referenced this pull request Aug 5, 2026
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>
frahlg added a commit that referenced this pull request Aug 5, 2026
)

* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant