Skip to content

test(control): port #1816's battery relief reproductions to master - #1918

Merged
frahlg merged 2 commits into
masterfrom
tests-from-1816
Oct 28, 2026
Merged

frahlg merged 2 commits into
masterfrom
tests-from-1816

Conversation

@frahlg

@frahlg frahlg commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

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 capBatteryCharge and clampEVBatteryHeadroom:

  • Pending rise. A 25 A site with a 1 A margin, phases at 22.3/12.3/12.3 A, leaving 1173 W. The battery measures -2000 W, and our newer -1000 W order may not show yet. Ending the relief at once would take L1 to 25.2 A. The battery must keep -827 W, so L1 lands on 24 A. A table at capBatteryCharge covers settled, room for both, blocked, bounded and no-relief cases.
  • Shared relief. Two batteries end -1000 W and -2000 W of relief, and 1827 W must stay. When the first cannot keep its share (at its SoC floor, or bounded to 300 W), the second keeps the rest. fix(control): reserve pending battery rises before releasing relief #1816's old head gave the second only 1218 W, so L1 reached 24.88 A. Both target orders are tested, and a table covers too little supply, no prior relief and a pending rise past supply.

Changes from #1816's tests:

  • They use master's helpers (evSplitFixture, sent, batteryReading).
  • Bounds pass in as lowest, the lowest order each battery may receive.
  • fitBatteryRisesToFuse comes from fix(control): fuse protection as one function #1789 and does not exist on master, so the allocation layer is gone.
  • Export cases are dropped. Master's keepEndingDischarge handles 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

  • New tests pass on master (09ebd42).
  • Mutation checks on ev_battery_headroom.go, each reverted afterwards:
  • go test -race -count=1 ./internal/control passes at f5da511.
  • make verify steps 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, keepEndingDischarge runs only inside clampEVBatteryHeadroom, which returns early when no EV demand is active. I ran #1816's pending-rise scenario through ComputeDispatch with no EV, in Idle and with a manual 0 W hold, in both directions:

Direction master master + #1891
Import (battery discharging -2000 W, order -1000 W) 0 W, L1 imports 25.2 A -827 W, L1 imports 24.0 A
Export (battery charging +2000 W, order +1000 W) 0 W, L1 exports 25.2 A 0 W, L1 exports 25.2 A

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

Credit to @segran2 for both reproductions (Co-authored-by in the commit).

🤖 Generated with Claude Code

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>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T18:41:55.606928Z 3ed607a PR opened
ℹ️ 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" or "@codex security review".

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>
@frahlg frahlg added the no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping) label Oct 28, 2026
@frahlg
frahlg merged commit bc30ceb into master Oct 28, 2026
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant