Fix uneven multi-battery charging from deadband concentration (#523)#526
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughCT002 balancer now requires a concentration pool to be balanced before deadband concentration runs, and it preserves balance-correction residuals when grid-tracking signs disagree. The Python mirror, tests, and changelog were updated to match. ChangesCT002 deadband balance fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deadband concentration (added in 2.2.0, concentrate_deadband) hands the whole small grid correction to the single most-active battery near steady state so it clears the firmware input deadband, and bypasses balance correction for that tick. But it fired whenever the grid error was small regardless of how the pool was split, so an out-of-balance pair (issue #523: two Venus E3 at ~88 W and ~890 W) was pinned: the equalizing balance correction never ran and the busiest battery kept absorbing every correction. Gate concentration on the pool already being balanced — skip it (and let balance correction equalize) whenever any participating battery deviates from its weight-proportional fair share by at least balance_deadband. This keeps the steady-state self-consumption benefit of #480 while never suppressing equalization of a real imbalance. Mirrored on the C++/ESPHome side per the parity rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiZvgThQbNq5hUfGHARuWB
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiZvgThQbNq5hUfGHARuWB
894138d to
ee48b26
Compare
Steering evaluation (base vs head)Overall: 4 improved, 11 regressed, 0 unchanged across 15 metrics — mean +3.9% (worse). Priority: priority-weighted -1.7% (better) — Lower is better for every metric. See Metrics are the per-scenario mean of 5 seeds. Aggregate — mean across 31 scenarios
📊 Interactive grid-power charts (zoom / hover / toggle series) are in the self-contained What do these metrics mean?
Per-scenario tables (31 scenarios)mixed_cadence/eff — settle 26.1→50.8s, overshoot 232.6→164.5W, RMS 25.6→21.9W
mixed_cadence/fair — settle 24.8→47.7s, overshoot 176.3→70.4W, RMS 13.1→12.9W
mixed_cadence_solar/eff — settle 34.6→52.5s, overshoot 351.2→384.7W, RMS 30.3→28.6W
mixed_cadence_solar/fair — settle 30.8→56.0s, overshoot 169.0→75.4W, RMS 20.3→23.7W
mixed_venus_b2500/eff — settle 56.2→81.2s, overshoot 220.6→221.9W, RMS 14.9→18.6W
mixed_venus_b2500/fair — settle 30.1→75.4s, overshoot 106.9→231.1W, RMS 13.2→22.3W
phase_imbalance — settle 44.9→53.4s, overshoot 166.7→145.2W, RMS 30.2→30.3W
single_venus_d_solar — settle 24.2→24.2s, overshoot 94.4→94.4W, RMS 15.9→15.9W
single_venus_d_steps — settle 26.3→26.3s, overshoot 90.3→90.3W, RMS 15.5→15.5W
single_venus_d_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 61.0→61.0W
single_venus_drain — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 907.3→907.3W
single_venus_fill — settle 360.0→360.0s, overshoot 0.0→0.0W, RMS 953.6→953.6W
single_venus_noisy — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.3→94.3W
single_venus_pv — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 60.8→60.8W
single_venus_solar — settle 26.8→26.8s, overshoot 80.3→80.3W, RMS 17.8→17.8W
single_venus_solar_slow — settle 33.9→33.9s, overshoot 68.3→68.3W, RMS 22.8→22.8W
single_venus_steps — settle 26.0→26.0s, overshoot 88.0→88.0W, RMS 14.7→14.7W
single_venus_steps_slow — settle 40.5→40.5s, overshoot 98.5→98.5W, RMS 14.8→14.8W
single_venus_trace — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 278.9→278.9W
single_venus_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 61.0→61.0W
two_venus/eff — settle 18.9→18.1s, overshoot 150.3→126.1W, RMS 13.9→14.0W
two_venus/fair — settle 18.2→18.4s, overshoot 128.7→116.7W, RMS 13.8→13.8W
two_venus_noisy/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.5→94.3W
two_venus_noisy/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.5→94.2W
two_venus_slow/fair — settle 41.3→41.8s, overshoot 181.1→174.5W, RMS 14.0→14.0W
two_venus_solar/eff — settle 25.5→26.0s, overshoot 417.5→396.6W, RMS 19.6→20.4W
two_venus_solar/fair — settle 22.7→25.9s, overshoot 139.8→151.4W, RMS 19.4→20.4W
two_venus_trace/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 283.2→283.1W
two_venus_trace/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 282.6→282.1W
venus_d_plus_c/eff — settle 17.4→20.1s, overshoot 175.4→128.9W, RMS 14.7→14.7W
venus_d_plus_c/fair — settle 18.6→21.6s, overshoot 118.4→121.0W, RMS 14.6→14.6W
📊 Open the interactive report — |
The #523 balance gate re-enabled balance correction for an out-of-balance pool, but the blanket grid-direction sign clamp zeroed any residual whose sign opposed the grid. fair_share always carries the grid's sign, so that clamp only ever fired on the balance-correction term — and near steady state that term is exactly the over-served battery's "back off" half of the equalizing swap. The clamp therefore left equalization one-sided (only the under-served battery moved), pushing the pool's net output around and disturbing the grid: the overshoot / slow-settle cost the balance fix carried. Clamp only the grid-tracking half (fair_share) against the grid direction and let the balance-correction redistribution through. That term is ~zero-sum across the same-phase pool, so applying it is grid-neutral — the over-served battery backs off by exactly what the under-served one takes on. The change is inert during load steps (fair_share dominates, so the residual never flips sign) and only engages near steady state, where #523 lives. Steering-eval (vs develop): share_imbalance_w -60%, overshoot_max_w -5% (was +24% with the gate alone), overshoot_mean_w -13%, priority-weighted verdict now net better. Mirrored on the C++/ESPHome side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiZvgThQbNq5hUfGHARuWB
Reproduces the reported failure where a full battery's saturation stays at 0% with the default-style pace settings, so excess solar is never transferred to the healthy battery. Root cause: saturation is scored from the post-pacing reading actually sent (last_target). A battery that cannot follow its command never grows its pace cap, so its reading stays pinned at PACE_BASE_STEP. When PACE_BASE_STEP is below MIN_TARGET_FOR_SATURATION the tracker treats every poll as "idle" and decays the score (and the stall-timeout rescue can't fire either, since it also requires |target| >= MIN_TARGET) — the full battery is never recognised as saturated. The reporter ran PACE_BASE_STEP=15 against MIN_TARGET_FOR_SATURATION=20. - PART A replays the reporter's own debug log (issue522_logs.txt): Venus E3 reports 0 W while the balancer sends it a -15 reading every poll; the real SaturationTracker holds the score at 0.000. - PART B drives the LoadBalancer end-to-end and shows the sharp break exactly at PACE_BASE_STEP == MIN_TARGET_FOR_SATURATION. Verified on develop @ ba42b9c (after #526). Not collected by pytest (repro_ prefix); kept to confirm the eventual fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvyVYm5tJWG6P2qP3DxJyB
Reproduces the reported failure where a full battery's saturation stays at 0% with the default-style pace settings, so excess solar is never transferred to the healthy battery. Root cause: saturation is scored from the post-pacing reading actually sent (last_target). A battery that cannot follow its command never grows its pace cap, so its reading stays pinned at PACE_BASE_STEP. When PACE_BASE_STEP is below MIN_TARGET_FOR_SATURATION the tracker treats every poll as "idle" and decays the score (and the stall-timeout rescue can't fire either, since it also requires |target| >= MIN_TARGET) — the full battery is never recognised as saturated. The reporter ran PACE_BASE_STEP=15 against MIN_TARGET_FOR_SATURATION=20. - PART A replays the reporter's own debug log (issue522_logs.txt): Venus E3 reports 0 W while the balancer sends it a -15 reading every poll; the real SaturationTracker holds the score at 0.000. - PART B drives the LoadBalancer end-to-end and shows the sharp break exactly at PACE_BASE_STEP == MIN_TARGET_FOR_SATURATION. Verified on develop @ ba42b9c (after #526). Not collected by pytest (repro_ prefix); kept to confirm the eventual fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvyVYm5tJWG6P2qP3DxJyB
Reproduces the reported failure where a full battery's saturation stays at 0% with the default-style pace settings, so excess solar is never transferred to the healthy battery. Root cause: saturation is scored from the post-pacing reading actually sent (last_target). A battery that cannot follow its command never grows its pace cap, so its reading stays pinned at PACE_BASE_STEP. When PACE_BASE_STEP is below MIN_TARGET_FOR_SATURATION the tracker treats every poll as "idle" and decays the score (and the stall-timeout rescue can't fire either, since it also requires |target| >= MIN_TARGET) — the full battery is never recognised as saturated. The reporter ran PACE_BASE_STEP=15 against MIN_TARGET_FOR_SATURATION=20. - PART A replays the reporter's own debug log (issue522_logs.txt): Venus E3 reports 0 W while the balancer sends it a -15 reading every poll; the real SaturationTracker holds the score at 0.000. - PART B drives the LoadBalancer end-to-end and shows the sharp break exactly at PACE_BASE_STEP == MIN_TARGET_FOR_SATURATION. Verified on develop @ ba42b9c (after #526). Not collected by pytest (repro_ prefix); kept to confirm the eventual fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvyVYm5tJWG6P2qP3DxJyB
Fixes #523. The
share_imbalance_wmetric this relies on landed in #525 (merged), so this PR targetsdevelopdirectly and the CI steering-eval comparison shows the balance change against a base that already has the metric.Root cause
Deadband concentration (
concentrate_deadband, added in 2.2.0 via #480) hands the whole small grid correction to the single most-active battery near steady state so it clears the firmware input deadband, and bypasses balance correction for that tick. But it fired whenever the grid error was small regardless of how the pool was split. An out-of-balance pair (two Venus E3 at ~88 W and ~890 W) produces no grid error — the sum is correct — so the only thing that could equalize them is balance correction, which concentration was suppressing. Result: the imbalance was pinned forever. This is absent in 2.1.2, which is why rolling back fixed it.Fix (two parts)
balance_deadbandoff its weight-proportional fair share.fair_sharealways carries the grid's sign, so that clamp only ever fired on the balance-correction term — near steady state, exactly the over-served battery's "back off" half of the swap. That left equalization one-sided (only the under-served battery moved), pushing the pool's net output around and disturbing the grid. Now only the grid-tracking half is clamped; the balance redistribution (≈zero-sum across the phase) goes through, so the over-served battery backs off by exactly what the under-served one takes on. Inert during load steps; only engages near steady state.Mirrored on the C++/ESPHome side per the parity rule. New regression tests cover both the gate and the two-sided (grid-neutral) swap.
Measured effect (CI steering-eval, seed-averaged across 31 scenarios)
Priority-weighted verdict: −1.4% (better) — vs +3.2% worse with the gate alone (which tripped
overshoot_max_w +24%). The grid-neutral swap turned the fix from a net regression into a net improvement: #523's imbalance is fixed (−60%, per-scenario −22% to −86%), overshoot now improves, and what remains is the inherent cost of equalizing two batteries (you have to move power between them):battery_travel +9%,settle +17%, andgrid_p2p_w +6%(a smaller guardrail flag, traded down from the +24% overshoot one). All confined to multi-battery same-phase setups; single-battery scenarios are untouched.This effectively restores 2.1.2-style balancing (what the reporter rolled back to and prefers) without 2.1.2's overshoot.
🤖 Generated with Claude Code
Summary by CodeRabbit