Repository navigation
fix(control): a down battery's PV no longer takes part of the charge target - #1815
Merged
Merged
Conversation
…target splitByCapacityAndPV summed the PV of every inverter group, including a group with no battery in the split (offline, faulted or charge-blocked). That group's local share went to no battery, so the fleet charged up to that PV short. Golden seeded_siblings/039 charged 6,284 W of 9,600 W. Count only groups with a battery in the split. Their PV reaches the other batteries over the AC bus, which split the whole target by capacity. Re-record seeded_siblings: 8 device_fault records move. 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. |
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
On a site with inverter groups (#143),
splitByCapacityAndPVsends charge to the battery on the same inverter as live PV. It summed the PV of every group, including a group whose battery was not in the split: offline, faulted, or unable to charge (chargeBlocked, parked bydistributeProportional). That group's share of the local pool went to no battery, and the overflow had already been reduced by it. The fleet total came out short by up to that group's PV.Golden
seeded_siblings/039_device_fault: Ferroamp faulted, 3,316 W of PV on its inverter, planner asks for 9,600 W of charge. Sungrow and Pixii got 3,142 W each, 6,284 W in total. 3,316 W went to the grid.When the target was smaller than that PV, the whole target went to no battery. In
003,027and033the site exported 3.1 to 4.4 kW in self-consumption while both healthy batteries got 0 W.Found while building #1814, not fixed there.
Fix
PV in a group with no battery in the split is not DC-local to any battery that can take charge. It reaches them over the AC bus, so it now stays out of the local pool. Physically it still flows out of its own inverter; only the split's accounting changes. The other batteries share the whole target. Where one hits its power limit, the rest goes to the grid as before, the same as for a site with no groups. No new setting.
The change sums
groupPVonly over groups with capacity inbats.capByGroupmoves up to the first loop so the sum can use it.Evidence
TestInverterAffinity_PVWithoutItsBatteryKeepsTheTotalfails on master and passes here, 3,316 W of PV on the down battery's inverter:chargeBlocked, same target: the same numbers;Golden corpus: 8 of 551 records move, all
seeded_siblings/*_device_faultwith groups and a charge target. Only that family is re-recorded.051and057, the same variant with a discharge target, do not move. A scratch check (not committed) ran all tendevice_faultrecords with groups twice, with and withoutInverterGroups. The answers are now identical, as they should be when the only PV sits on the down battery's inverter. Five other records in the family changed by under 10 µW when re-recorded (old clock noise, inside the replay's 0.01 W tolerance).TestSiteSimFieldMatrix:sitesimnever setsInverterGroups, so the matrix cannot move. It passes withsitesim_field_known.txtunchanged.go vet ./internal/... ./cmd/ftw/...andgo test ./internal/... ./cmd/ftw/...pass.Independent review (Sonnet subagent): approve, no findings. It regenerated all nine families with master's and this
dispatch.go; only these 8 records differ. A 300k-case random check ofsplitByCapacityAndPV(1–4 batteries, groups, zero capacity, charge caps, PV, targets −3 to 9.6 kW) never put the sum above the target, kept it equal to the target whenever no battery clamped (master broke that 102,292 times), and matched master exactly whenever every PV group had a battery, and for every target ≤ 0. The third test case came from this review.Not changed
009: Pixii at 3,716 W could take 884 W more). That holds with or without groups, so it is separate from this bug.seeded_siblings.json. The code does not overlap.🤖 Generated with Claude Code