Name the steps inside the balancer's four longest methods - #641
Draft
tomquist wants to merge 2 commits into
Draft
Conversation
`LoadBalancer` carried its control loop in a handful of very long methods,
each mixing several decisions that have nothing to say to one another. Six
of those steps are now methods with names and contracts:
- `_track_saturation` — the "is this battery following its commands" score,
lifted out of `compute_target`, which is the public entry point and was
spending a third of itself on an aside.
- `_pace_cap` — the ramp-cap state machine (reversal / grow / stall escape),
split from `_pace_reading`, which now just measures the poll interval,
applies the cap and records what was sent. The two halves answer different
questions: what has this battery earned, and what do we send it now.
- `_efficiency_weights` and `_residual_share` — the share weights and the
grid-imbalance slice, out of `_compute_auto_target`. `_residual_share` is
where the tracking and balancing terms are kept apart, which was the
densest arithmetic in the file and the least self-evident.
- `_probe_active_set_change` and `_log_role_changes` — the tail of the
efficiency pass, which ended in two unrelated loops over set differences.
Pure code motion: no condition, constant or order of operations changed.
The four-branch residual selection is now a single positive condition
guarding one assignment, which is the same predicate written the short way.
Every extraction is mirrored into `esphome/components/ct002/balancer.{h,cpp}`
under the same names, as the parity rule requires; `_log_role_changes` has no
counterpart because the firmware balancer has no logging beyond the steer
sink, which is a pre-existing deliberate divergence.
Verified behaviour-identical: the steering evaluation over 33 scenarios x 5
seeds produces a byte-identical JSON to the pre-change baseline (same md5),
so no metric moved at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MprqHXqPgjAXwWAwWFEevx
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Contributor
Steering evaluation (base vs head)Overall: 0 improved, 0 regressed, 15 unchanged across 15 metrics — mean 0% (unchanged). Priority: priority-weighted 0% (unchanged) — ✅ no do-no-harm guardrail regressions. Lower is better for every metric. See Metrics are the per-scenario mean of 5 seeds. Aggregate — mean across 33 scenarios
📊 Interactive grid-power charts (zoom / hover / toggle series) are in the self-contained What do these metrics mean?
Per-scenario tables (33 scenarios)b2500_pair_dc_floor — settle 103.9→103.9s, overshoot 102.1→102.1W, RMS 155.8→155.8W
full_battery_low_pace — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 22.5→22.5W
mixed_cadence/eff — settle 43.9→43.9s, overshoot 148.2→148.2W, RMS 21.2→21.2W
mixed_cadence/fair — settle 43.2→43.2s, overshoot 43.6→43.6W, RMS 13.1→13.1W
mixed_cadence_solar/eff — settle 46.6→46.6s, overshoot 1077.6→1077.6W, RMS 51.6→51.6W
mixed_cadence_solar/fair — settle 51.1→51.1s, overshoot 65.5→65.5W, RMS 22.6→22.6W
mixed_venus_b2500/eff — settle 108.6→108.6s, overshoot 319.9→319.9W, RMS 28.6→28.6W
mixed_venus_b2500/fair — settle 128.0→128.0s, overshoot 319.0→319.0W, RMS 37.6→37.6W
phase_imbalance — settle 60.0→60.0s, overshoot 163.7→163.7W, RMS 30.3→30.3W
single_venus_d_solar — settle 23.7→23.7s, overshoot 83.2→83.2W, RMS 16.0→16.0W
single_venus_d_steps — settle 25.2→25.2s, overshoot 86.6→86.6W, RMS 14.6→14.6W
single_venus_d_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 59.9→59.9W
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.0→94.0W
single_venus_pv — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 59.2→59.2W
single_venus_solar — settle 26.0→26.0s, overshoot 93.0→93.0W, RMS 17.8→17.8W
single_venus_solar_slow — settle 34.0→34.0s, overshoot 66.2→66.2W, RMS 22.7→22.7W
single_venus_steps — settle 25.2→25.2s, overshoot 86.6→86.6W, RMS 14.6→14.6W
single_venus_steps_slow — settle 41.1→41.1s, overshoot 101.9→101.9W, RMS 14.7→14.7W
single_venus_trace — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 274.3→274.3W
single_venus_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 59.9→59.9W
two_venus/eff — settle 17.2→17.2s, overshoot 124.1→124.1W, RMS 14.3→14.3W
two_venus/fair — settle 17.5→17.5s, overshoot 122.4→122.4W, RMS 14.2→14.2W
two_venus_noisy/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.2→94.2W
two_venus_noisy/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.0→94.0W
two_venus_slow/fair — settle 41.4→41.4s, overshoot 19.6→19.6W, RMS 14.1→14.1W
two_venus_solar/eff — settle 25.9→25.9s, overshoot 534.9→534.9W, RMS 20.6→20.6W
two_venus_solar/fair — settle 25.3→25.3s, overshoot 143.8→143.8W, RMS 20.4→20.4W
two_venus_trace/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 285.0→285.0W
two_venus_trace/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 284.8→284.8W
venus_d_plus_c/eff — settle 17.2→17.2s, overshoot 122.0→122.0W, RMS 14.3→14.3W
venus_d_plus_c/fair — settle 17.5→17.5s, overshoot 122.4→122.4W, RMS 14.2→14.2W
📊 Open the interactive report — |
…ormula The previous commit made every method shorter and the file longer, which is only half a refactor. Three things it should have removed: - `_efficiency_weights` was a five-line body behind eleven lines of signature and docstring — an abstraction that cost more than it held. Inlined. - `faded_adjustments and consumer_id and faded_adjustments.get(...) == 0.0` guards an empty dict that `.get` already handles: an empty mapping returns `None`, which is not `0.0`. Five lines to one. - The weight-proportional share formula — sum the weights, divide, fall back to an even split — was written out three times in Python and three times in C++, each with its own spelling of the same fallback. It is now `weighted_share`, one free function per stack, used by `_fair_share`, `_balance_correction` and `_concentration_pool_balanced`. The C++ helper is a template over the three container shapes those sites walk (a report map, a vector of ids, a vector of id pointers) with three one-line key adapters, rather than three copies of the loop. Behaviour is unchanged and checked the same way: the steering evaluation over 33 scenarios x 5 seeds still produces a JSON byte-identical to the pre-change baseline (md5 b024199f0ce47bab4316a7552b84cf5d). The equal-split fallbacks are reachable in only one of the three sites; the other two are guarded by non-empty preconditions, so folding `max(1, n)` into the shared helper cannot change them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MprqHXqPgjAXwWAwWFEevx
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
1 participant
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.
Why
LoadBalanceris the one part of the CT002 stack #639 deliberately left alone. The reason wasparity, not that it didn't need the work:
balancer.hmirrors the Pythonprivate method names one for one, so restructuring one side means
restructuring both, and that was too much to carry alongside 122 other files.
The file's problem is not its size — a control loop is genuinely large — but
that its longest methods each interleave several decisions that have nothing
to say to one another. Reading
_pace_readingmeant holding a clock, afour-branch learning state machine and a clamp in your head at once, and the
only way to see which of the three a line belonged to was to know already.
What this actually trades
Be clear about the cost: the file gets longer. Extraction buys shorter
methods by paying for signatures, and here every signature is paid three times
— Python,
.cpp, and the.hdeclaration.balancer.pyWhat the +68 buys:
_compute_auto_target_pace_readingcompute_target_compute_efficiency_deprioritizedWhy there is no smaller version of this file
Worth recording, since it is the obvious follow-up question and the answer is
measured rather than assumed:
balancer.pyis 1605 lines of code. The 2766 figure includes 870 linesof comments and docstrings (31% of the file) and 291 blanks.
src/andtests/).BalancerConfigknobs are reachablefrom user config, so no branch exists to serve an option nobody can set.
constants normalised away, 4-statement windows) finds one cross-function
match, and it is a false positive — runs of
self._x = 0.0in__init__/_reset_window/clear. The triplicated share formula this PR removed wasthe real one.
So the file is not reducible by deletion. It is decomposable: splitting it
into ~8 mirrored modules would take the largest file to ~680 lines and
LoadBalancerfrom 65 methods to ~28 — but it raises the total by ~7%, and itis a separate change, not this one.
One correction to an earlier version of this description, which claimed the
probe subsystem was an obvious candidate to lift out on its own: it is not a
clean seam.
_reject_proberewritesself._priority,_commit_probewrites
_last_rotation, three of them drivefade_weightand the saturationtracker, and
_compute_probe_targetcalls_emit. Extracted alone it wouldbe a set of methods that all reach back into the class. It is the probe and
efficiency-rotation machinery together (23 methods) or neither.
What moved (commit 1)
_track_saturation— the "is this battery following its commands"score, out of
compute_target. That method is the balancer's public entrypoint and was spending a third of itself on an aside, comment included.
_pace_cap— the ramp-cap state machine (reversal / grow / stallescape), split from
_pace_reading. The two halves answer differentquestions: what movement has this battery earned versus what do we send
it now. Only the first is a state machine; only the second touches the
clock.
_residual_share— the grid-imbalance slice, out of_compute_auto_target. This is where the grid-tracking and pool-balancingterms are held apart so only the first is clamped against the grid direction
(issue Uneven battery charging behavior in version 2.2.1 compared to 2.1.2 #523); it was the densest arithmetic in the file.
_probe_active_set_changeand_log_role_changes— the tail of theefficiency pass, which ended in two unrelated loops over set differences.
What was deleted (commit 2)
Commit 1 made every method shorter and the file longer, which is only half a
refactor. Three things it should have removed:
_efficiency_weightswas a five-line bodybehind eleven lines of signature and docstring. Inlined.
faded_adjustments and consumer_id and faded_adjustments.get(...) == 0.0guards an empty dict that.getalreadyhandles — an empty mapping returns
None, which is not0.0. Five lines toone.
divide, fall back to an even split — was written out three times in Python
and three times in C++, each with its own spelling of the fallback. It is
now
weighted_share, one free function per stack, used by_fair_share,_balance_correctionand_concentration_pool_balanced. The C++ helper isa template over the three container shapes those sites walk, with three
one-line key adapters, rather than three copies of the loop.
Parity
Every extraction is mirrored into
esphome/components/ct002/balancer.{h,cpp}under the same name, with the
Mirrors balancer.py <name>comment the fileuses.
_log_role_changeshas no counterpart: that is pre-existing anddeliberate —
balancer.{h,cpp}carries no ESPHome includes at all (so twohost build paths can compile it), and the firmware's only balancer logging is
the
set_steer_log_sinkseam.Verification
uv run ruff format . && uv run ruff check . && uv run mypy src/— clean.uv run pytest— 1741 passed, 8 skipped (5 need Docker, 3 arePython-only paths).
tests/components/ct002/— 163 passed, including the 45 cross-stackbalancer parity tests, which build the bare-
g++harness and compare theC++ numerically against Python.
host_balancer_test).g++ -std=c++17 -Wall -Wextra -Wshadow -Werror -O2onbalancer.cpp— clean.ct002-esphome-compiletargets pass, which is the ESP32 coverage Icould not run locally (PlatformIO's
tool-cmakeextracts truncated throughmy sandbox's proxy).
Steering evaluation. Locally the result is byte-identical to the
pre-change baseline, and stayed so after commit 2's expression rewrites — the
same md5 (
b024199f0ce47bab4316a7552b84cf5d) for base, commit 1 and commit 2,33 scenarios × 5 seeds. The baseline was taken from a separate
git worktreeat
developwithPYTHONPATHoverriding the editable install, so it cannothave picked up the working tree.
The equivalence of the shared
weighted_sharewas also checked directlyrather than only through the simulator: 400,000 random cases × 3 call sites,
each original expression against the helper, compared at the bit level —
zero mismatches, including zero-weight, tiny-weight and negative-total cases.
The equal-split fallback is reachable in only one of the three sites; the
other two are guarded by non-empty preconditions, so folding
max(1, n)intothe helper cannot change them.
Note for reviewers: the steering eval is not reproducible in CI
The first CI run on this head reported
mixed_cadence/effmoving(
band_crossings_per_h+8%,grid_p2p_w−6%). Re-running the identicalcommit produced no movement at all. From the two job logs:
Base is identical across both attempts and matches every local run; only the
head run moved, and only once. The uploaded artifact changed size (25420 →
25356 bytes) on identical inputs.
I could not reproduce the non-determinism locally: 15+ runs, Python 3.11 and
3.13,
os.cpu_count()forced to 1–5, fixedPYTHONHASHSEED— all identical,all matching CI's base.
run_scenarioreseeds per task(
random.seed(seed)), so the harness is deterministic by construction on mymachine. Something about CI's environment defeats that, and I have not
identified what; it is a pre-existing harness issue rather than something
this PR introduces, and worth its own investigation before the eval is trusted
as a gate on a chaotic scenario.
Checklist
develop, notmainuv run ruff format . && uv run ruff check . && uv run mypy src/ && uv run pytestpassesweb/changes — noneCHANGELOG.mdentry🤖 Generated with Claude Code
https://claude.ai/code/session_01MprqHXqPgjAXwWAwWFEevx