Repository navigation
test(control): port #1816's battery relief reproductions to master - #1918
Merged
Merged
Conversation
Two reproductions from #1816, adapted to master's capBatteryCharge and clampEVBatteryHeadroom: - Pending rise: 25 A site, 1 A margin, phases 22.3/12.3/12.3 A. The battery measures -2000 W and our newer -1000 W order may not show yet. Ending the relief must keep -827 W so L1 lands on 24 A, not 25.2 A. - Shared relief: two batteries end -1000 W and -2000 W of relief. When the first cannot keep its share (SoC floor or a 300 W bound), the second keeps the missing 1827 W instead of the old 1218 W. Master's keepEndingDischarge (#1847) passes both. Dropping it, ignoring pending rises, or sharing before bounds makes them fail. Import only: master leaves the export direction to the fuse pipeline. Co-authored-by: segran2 <268038031+segran2@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The header said master leaves the export case to the fuse pipeline. Nothing on master holds it. Name both no-car gaps instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#1816 by @segran2 holds two reproductions of a battery ending fuse relief too fast. We plan to close #1816 because master's
keepEndingDischarge(#1847) covers the same case while a car charges. Its tests should not be lost.No rig seed comes from #1816 itself. The same class of fault shows on the random rig as C3 in the 27 Oct triage (seeds 207530, 207533).
Change
One new test file,
go/internal/control/battery_relief_pending_test.go. No code change and no changeset.The two reproductions, adapted to master's
capBatteryChargeandclampEVBatteryHeadroom:capBatteryChargecovers settled, room for both, blocked, bounded and no-relief cases.Changes from #1816's tests:
evSplitFixture,sent,batteryReading).lowest, the lowest order each battery may receive.fitBatteryRisesToFusecomes from fix(control): fuse protection as one function #1789 and does not exist on master, so the allocation layer is gone.keepEndingDischargehandles import only, and nothing else on master holds the export case (see Risk).The file header names both no-car gaps. An earlier version said master left export to the fuse pipeline; that was wrong.
Tests
ev_battery_headroom.go, each reverted afterwards:keepEndingDischargecall, all four tests fail.pendingWremoved from the shortfall, the pending-rise cases fail.share := shortW / keepW), only these new tests fail. No older master test catches it.go test -race -count=1 ./internal/controlpasses at f5da511.make verifysteps passed on the first commit: tests, vet, build and boundary checks. Docker steps were skipped because no Docker daemon was running. The follow-up commit changes only a test comment.Risk
Test only. Two gaps stay open, and these tests cover neither. On master,
keepEndingDischargeruns only insideclampEVBatteryHeadroom, which returns early when no EV demand is active. I ran #1816's pending-rise scenario throughComputeDispatchwith no EV, in Idle and with a manual 0 W hold, in both directions:phaseFuseReliefW, C3) closes it in this scenario.So #1847 matches #1816 only when a car charges, and only on import. The note that closes #1816 must name both gaps.
Stack and merge order
evBatteryPhaseSpareWto usefuseLineA(). These tests setSiteFuseSafetyA = 1explicitly, so the line stays at 24 A either way. Review test-merged fix(control): car and battery work to the fuse rating (fuse stack #1910, #1891, #1913, #1923, #1924, #1925, #1926, #1914) #1910, fix(control): battery fuse cut lands on the line, not half a margin below #1913, fix(config): fuse safety_margin_a defaults to 0 #1914, fix(control): no discharge at or under the SoC floor #1916, fix(control): one SoC floor for the stale-plan fallback, fuse relief and the hourly import limit #1878 and fix(control): battery fuse guard reads each phase with its direction #1891 into this branch; each merged cleanly and these tests pass.go/internal/control/ev_battery_headroom.go, which holdsclampEVBatteryHeadroomandkeepEndingDischarge, and movescapBatteryChargetofuse_phase.gowith a new signature. These tests call both, so they would not build on fix(control): fuse protection as one function #1789. fix(control): reserve pending battery rises before releasing relief #1816 is based on fix(control): fuse protection as one function #1789's branch (fix/fuse-one-function) and patches fix(control): fuse protection as one function #1789'sfitBatteryRisesToFuse. Closing fix(control): reserve pending battery rises before releasing relief #1816 is safe only if fix(control): fuse protection as one function #1789, a conflicting draft, is dropped or rebuilt on master. If fix(control): fuse protection as one function #1789 comes back, it must port these tests. The note that closes fix(control): reserve pending battery rises before releasing relief #1816 should say so.Credit to @segran2 for both reproductions (Co-authored-by in the commit).
🤖 Generated with Claude Code