Add per-battery distribution weight for fair-share load balancing#413
Merged
Conversation
Add a per-battery "Distribution Weight" Home Assistant number entity (default 1.0) that biases how the load balancer splits demand across multiple batteries. For unequal batteries this avoids the smaller one saturating first every day -- e.g. set 1.5 and 1.0 for a ~60:40 split (discussion #412). The weight rides along in each consumer's report dict and folds into the balancer's existing weight-normalised fair-share math: the pure health/saturation map (eff_part) still decides participation and probing, while a derived share map (eff_part * weight) drives the proportional split, and balance_correction targets the weight-proportional share instead of the plain average. Neutral weights (all 1.0) reproduce the previous behaviour exactly. Persist the per-consumer controls via retained MQTT commands instead of a local store: manual_target, auto_target, active and the new distribution_weight each move onto their own dedicated command sub-topic with retain=true, so Home Assistant restores the values across an AstraMeter restart when the broker redelivers the retained command on re-subscribe. Mirrored across the Python and ESPHome (C++) CT002 stacks per the parity rule; docs, examples, changelog and Python + C++ host tests updated.
Collapse the three identical `float(report.get("weight", 1.0) or 1.0)`
expressions in the balancer into a single `_report_weight` helper, so the
falsy-guard is documented once. Pure refactor — no behavior change.
|
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 (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a per-battery Distribution Weight (float, default 1.0, range [0.0,10.0]) used by the load-balancer to bias fair-share splits; exposes a new HA number entity and retained per-field MQTT command topics; updates balancer logic in Python/C++; and adds tests and docs. ChangesPer-battery Distribution Weight Feature
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 |
tomquist
marked this pull request as ready for review
June 1, 2026 13:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
esphome/components/ct002/ct002.cpp (1)
645-651: 💤 Low valueIntentional silent-ignore behavior differs from Python's ValueError.
The C++ setter silently ignores invalid weights while Python raises a
ValueError. This is a reasonable embedded-friendly design choice (avoiding exceptions), but callers have no feedback when an out-of-range or non-finite value is rejected. Consider logging a warning for debuggability.♻️ Optional: add debug log for rejected values
void CT002Component::set_consumer_distribution_weight(const std::string &consumer_id, float weight) { // Clamp to the same (0, 10] range the Python setter enforces; ignore // non-finite or out-of-range values rather than corrupting the split. - if (!std::isfinite(weight) || weight <= 0.0f || weight > 10.0f) return; + if (!std::isfinite(weight) || weight <= 0.0f || weight > 10.0f) { + ESP_LOGD(TAG, "Ignoring invalid distribution_weight %.2f for %s", weight, consumer_id.c_str()); + return; + } this->get_consumer_(consumer_id).distribution_weight = weight; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esphome/components/ct002/ct002.cpp` around lines 645 - 651, The setter CT002Component::set_consumer_distribution_weight currently silently returns on invalid input; modify it to log a warning before returning so callers can debug rejected values: when !std::isfinite(weight) or weight <= 0.0f or weight > 10.0f, emit a warning that includes the consumer_id and the rejected weight (and optionally the reason) and then return; keep the clamping/behavior unchanged otherwise and locate the change in CT002Component::set_consumer_distribution_weight (and use the same logging facility used elsewhere in this component).src/astrameter/mqtt_insights/mqtt_insights_test.py (1)
941-951: ⚡ Quick winAssert
distribution_weighton the actual MQTT payload, not a reconstructed dict.This test only repeats
data.get("distribution_weight", 1.0), so it still passes ifon_ct002_response()stops publishing the field or renames it. Please putdistribution_weightintoSAMPLE_CT002_DATAand verify it intest_publishes_state_on_ct002_eventso the wire-format contract is actually covered.Suggested test adjustment
SAMPLE_CT002_DATA = { "grid_power": {"l1": 100.0, "l2": 200.0, "l3": 300.0, "total": 600.0}, @@ "manual_target": None, "auto_target": True, + "distribution_weight": 1.5, "smooth_target": 500.0, "active_control": True, "consumer_count": 2, }payload = json.loads(received[0].payload) assert payload["grid_power"]["total"] == 600.0 assert payload["phase"] == "A" @@ assert payload["active"] is True assert payload["poll_interval"] == 5.0 + assert payload["distribution_weight"] == 1.5 assert str(received[0].topic) == f"{base}/ct002/dev1/consumer/consumer1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/astrameter/mqtt_insights/mqtt_insights_test.py` around lines 941 - 951, The test is asserting distribution_weight from a locally reconstructed dict instead of the actual MQTT payload; update SAMPLE_CT002_DATA to include an explicit distribution_weight value and change test_publishes_state_on_ct002_event to assert that the published MQTT payload (as emitted by on_ct002_response or the MQTT publish spy used in tests) contains distribution_weight with the expected value, and remove the redundant reconstruction and local assertion in test_consumer_state_includes_manual_target_fields so the wire-format contract is actually validated against on_ct002_response's output.esphome/components/ct002/mqtt_insights.cpp (1)
341-351: 💤 Low value
parse_float_payloadaccepts trailing non-numeric characters.
strtofstops parsing at the first non-numeric character and setsendto point there. The current checkend == beginonly catches completely unparseable strings, but"1.5abc"would parse as1.5successfully. This differs from Python'sfloat()which would reject"1.5abc".Consider whether this permissiveness is acceptable. If not:
Proposed stricter validation
static bool parse_float_payload(const std::string &payload, float &out) { const char *begin = payload.c_str(); char *end = nullptr; errno = 0; const float v = std::strtof(begin, &end); if (end == begin || errno != 0) return false; + // Reject trailing non-whitespace characters + while (*end != '\0') { + if (!std::isspace(static_cast<unsigned char>(*end))) + return false; + ++end; + } out = v; return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esphome/components/ct002/mqtt_insights.cpp` around lines 341 - 351, The function parse_float_payload currently accepts inputs with trailing non-numeric characters because it only checks end == begin; modify parse_float_payload to ensure the entire payload is numeric (allowing leading/trailing whitespace). After calling std::strtof(begin, &end) and checking end==begin and errno, advance end over any whitespace (use isspace) and then require *end == '\0' (otherwise return false). Keep setting out = v only when the full-string validation passes; reference parse_float_payload, begin, end, std::strtof, errno in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/astrameter/mqtt_insights/mqtt_insights_test.py`:
- Around line 125-131: The test currently asserts the Home Assistant numeric
entity minimum is 0.1 which incorrectly narrows the allowed range; update the
assertion in mqtt_insights_test.py where weight = comps["distribution_weight"]
to expect the contract's lower bound (0.0) instead of 0.1 (i.e., assert
weight["min"] == 0.0) so the HA control doesn't forbid valid positive values
<0.1; if 0.1 was intentional, instead update the public spec/docs to reflect the
stricter lower bound and keep the test in sync.
---
Nitpick comments:
In `@esphome/components/ct002/ct002.cpp`:
- Around line 645-651: The setter
CT002Component::set_consumer_distribution_weight currently silently returns on
invalid input; modify it to log a warning before returning so callers can debug
rejected values: when !std::isfinite(weight) or weight <= 0.0f or weight >
10.0f, emit a warning that includes the consumer_id and the rejected weight (and
optionally the reason) and then return; keep the clamping/behavior unchanged
otherwise and locate the change in
CT002Component::set_consumer_distribution_weight (and use the same logging
facility used elsewhere in this component).
In `@esphome/components/ct002/mqtt_insights.cpp`:
- Around line 341-351: The function parse_float_payload currently accepts inputs
with trailing non-numeric characters because it only checks end == begin; modify
parse_float_payload to ensure the entire payload is numeric (allowing
leading/trailing whitespace). After calling std::strtof(begin, &end) and
checking end==begin and errno, advance end over any whitespace (use isspace) and
then require *end == '\0' (otherwise return false). Keep setting out = v only
when the full-string validation passes; reference parse_float_payload, begin,
end, std::strtof, errno in your change.
In `@src/astrameter/mqtt_insights/mqtt_insights_test.py`:
- Around line 941-951: The test is asserting distribution_weight from a locally
reconstructed dict instead of the actual MQTT payload; update SAMPLE_CT002_DATA
to include an explicit distribution_weight value and change
test_publishes_state_on_ct002_event to assert that the published MQTT payload
(as emitted by on_ct002_response or the MQTT publish spy used in tests) contains
distribution_weight with the expected value, and remove the redundant
reconstruction and local assertion in
test_consumer_state_includes_manual_target_fields so the wire-format contract is
actually validated against on_ct002_response's output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7ef22d5-f8db-4ffc-a10b-28e8430639e5
📒 Files selected for processing (19)
CHANGELOG.mdREADME.mdconfig.ini.exampleesphome.example.yamlesphome/components/ct002/balancer.cppesphome/components/ct002/balancer.hesphome/components/ct002/ct002.cppesphome/components/ct002/ct002.hesphome/components/ct002/ha_discovery.cppesphome/components/ct002/mqtt_insights.cppesphome/components/ct002/mqtt_insights.hsrc/astrameter/ct002/balancer.pysrc/astrameter/ct002/ct002.pysrc/astrameter/main.pysrc/astrameter/mqtt_insights/discovery.pysrc/astrameter/mqtt_insights/mqtt_insights_test.pysrc/astrameter/mqtt_insights/service.pytests/components/ct002/host_balancer_test.cpptests/test_balancer_distribution_weight.py
- C++ parse_float_payload now rejects trailing non-whitespace (e.g. "1.5abc"), matching Python's float() so the two command paths stay behaviourally identical. - Cover distribution_weight on the actual published MQTT payload: add it to SAMPLE_CT002_DATA and assert it in test_publishes_state_on_ct002_event, dropping the redundant reconstructed-dict assertion.
Widen the per-battery weight contract from (0, 10] to [0, 10] across both stacks. A weight of 0.0 now parks a battery at 0 W while leaving it in the pool (distinct from the Active switch's hard pause), and the Home Assistant slider min becomes 0 so the UI matches the backend. Fixes the _report_weight helper to preserve an explicit 0.0 instead of folding it back to 1.0 via the old `or 1.0` guard; missing/None still maps to the neutral 1.0. Updates Python + C++ setters, both command-path range checks, both discovery payloads, docs, and adds zero-weight tests on both stacks.
tomquist
pushed a commit
that referenced
this pull request
Jun 1, 2026
Two test-only follow-ups after rebasing onto the per-battery weight feature (develop #413): - test_shared_e2e.py: disable saturation detection on both stacks in test_python_esphome_wire_identical. The saturation score is a long-running EMA carried in float64 (Python) vs float32 (ESPHome); over a long poll sequence the two drift by sub-ULP amounts that can straddle a participation threshold and amplify into a whole-watt target difference. That is the documented float-vs-double artifact, not a wire-format divergence, so the byte-for-byte handler comparison runs with the EMA off. The EMA path itself stays covered (at integer-watt tolerance) by the differential fuzzer. Adds a saturation_detection flag to PythonBackend and toggles the binary via the existing cfg control command. - test_balancer_parity.py: compare raw target magnitudes against a half-watt tolerance instead of rounding each side and checking integer equality. A value near an X.5 boundary could round to X on one stack and X+1 on the other despite a <0.001 W true difference, which could flake in CI (CodeRabbit review on PR #406). https://claude.ai/code/session_01X6DBrU9twwv92ZG9aAUMjN
tomquist
added a commit
that referenced
this pull request
Jun 1, 2026
* test(ct002): add cross-stack LoadBalancer parity harness and phase-routing E2E Increase Python <-> ESPHome parity coverage for the CT002 balancer: - New differential parity test (test_balancer_parity.py + fixtures/ balancer_parity_harness.cpp): compiles the real esphome/components/ct002/ balancer.cpp with host g++ and drives the same inactive / manual / auto scenarios (import, export, multi-phase, asymmetric reports, mixed AC/DC device types) through both the C++ port and the canonical Python LoadBalancer, asserting identical per-phase targets at wire granularity. Skips cleanly when no C++ compiler is present. - New shared E2E scenario test_phase_routing in test_shared_e2e.py: verifies a battery's target lands only on the phase it reports (zero on the others), parametrized over A/B/C and run against both backends. Both stacks agree across all added scenarios; no behavioral divergence found. * Expand CT002 Python<->ESPHome E2E parity coverage and align fair-share guard Broaden the cross-stack parity tests until they exercise the drift-prone paths, and fix the one divergence they surfaced. Tests: - Rework test_balancer_parity.py into a stateful differential harness: the host-gcc fixture now links the real balancer.cpp and drives a single LoadBalancer through a command stream (cfg/clock/advance/target/sat/last), replayed identically against the canonical Python LoadBalancer. Adds multi-poll efficiency/probe/rotation/saturation scenarios plus 40 seeds of randomized differential fuzzing, so the time-dependent machinery is compared poll-by-poll, not just one-shot phase splits. - test_shared_e2e.py: add cross-talk scenarios (a charging/discharging battery shows up with the right sign in another battery's per-phase chrg/dchrg fields) and a direct dual-backend wire comparison that replays a seeded multi-battery poll sequence through both the Python emulator and the live ESPHome binary and asserts the response fields match byte-for-byte. Factor the esphome-binary launch into a reusable context manager. Fix: - balancer.py compute_auto_target: guard fair_share against a zero/negative total effective weight, mirroring the existing C++ guard in compute_auto_target_. Previously Python could raise ZeroDivisionError where C++ fell back to an even split; both now use grid_total / num_consumers. Defensive (unreachable via compute_target today) but keeps the two stacks structurally identical. Verified with esphome 2026.5.1 installed: full tests/components/ct002 suite green (host gtest, host e2e, shared e2e on both backends, wire comparison) plus tests/test_balancer.py; ruff format/check and mypy clean. https://claude.ai/code/session_01X6DBrU9twwv92ZG9aAUMjN * Make wire-identical parity deterministic and tolerate half-watt rounding Two test-only follow-ups after rebasing onto the per-battery weight feature (develop #413): - test_shared_e2e.py: disable saturation detection on both stacks in test_python_esphome_wire_identical. The saturation score is a long-running EMA carried in float64 (Python) vs float32 (ESPHome); over a long poll sequence the two drift by sub-ULP amounts that can straddle a participation threshold and amplify into a whole-watt target difference. That is the documented float-vs-double artifact, not a wire-format divergence, so the byte-for-byte handler comparison runs with the EMA off. The EMA path itself stays covered (at integer-watt tolerance) by the differential fuzzer. Adds a saturation_detection flag to PythonBackend and toggles the binary via the existing cfg control command. - test_balancer_parity.py: compare raw target magnitudes against a half-watt tolerance instead of rounding each side and checking integer equality. A value near an X.5 boundary could round to X on one stack and X+1 on the other despite a <0.001 W true difference, which could flake in CI (CodeRabbit review on PR #406). https://claude.ai/code/session_01X6DBrU9twwv92ZG9aAUMjN * test(ct002): add subprocess timeouts and dump command stream on parity failure Address CodeRabbit nitpicks on PR #406: - Add timeout=120 to the harness compile and timeout=30 to the harness run so a hung compiler or runaway binary fails fast instead of stalling CI. - Use the previously-unused `lines` argument in _compare: dump the command stream alongside the mismatch list so a failing (randomized) scenario is reproducible straight from the assertion output. https://claude.ai/code/session_01X6DBrU9twwv92ZG9aAUMjN --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jun 5, 2026
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
2 participants
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.
Adds a Distribution Weight control that lets users bias how the load balancer splits demand across multiple batteries. Each battery gets a weight (default
1.0, range0.0 < w ≤ 10.0) that scales its proportional share of the total load—e.g., setting weights to1.5and1.0produces roughly a 60:40 split instead of 50:50.Key Changes
MQTT & Home Assistant Integration:
{base}/ct002/{dev}/consumer/{cid}/set) to per-field scalar topics ({base}/ct002/{dev}/consumer/{cid}/{field}/set), wherefieldis one ofactive,auto_target,manual_target, ordistribution_weightregister_distribution_weight_handler()to the MQTT service and corresponding Home Assistant discovery entityLoad Balancer:
_report_weight()helper to extract per-battery weight from reports (defaults to1.0if missing)Data Model:
distribution_weight: float = 1.0field toConsumer(Python) andConsumerstruct (C++)weight: floatfield toConsumerReport(C++) and report dict (Python)distribution_weightto consumer snapshots and MQTT state publicationsTesting:
test_distribution_weight_command_via_mqtt()integration testtest_handle_consumer_field_command_dispatch()unit test for per-field parsingtests/test_balancer_distribution_weight.pywith three scenarios: fair-share honouring weight, neutral weight matching equal split, and balance correction targeting weighted shareLoadBalancer.AutoSplitHonoursDistributionWeightDocumentation:
Implementation Details
The per-field topic design leverages MQTT's retained message semantics: Home Assistant publishes each control value to its own retained topic, so the broker automatically redelivers it on re-subscribe. This eliminates the need for AstraMeter to persist control state locally.
At the neutral default (all weights
1.0), the weighted fair-share calculation is mathematically identical to the unweighted behaviour, ensuring backward compatibility.https://claude.ai/code/session_01PufmKr2LPyXP7mKSEWxcbp
Summary by CodeRabbit
New Features
Documentation
Tests