Exclude DC-only Marstek batteries from charge distribution (#338)#342
Open
Exclude DC-only Marstek batteries from charge distribution (#338)#342
Conversation
When a DC-only battery (B2500) is paired with an AC battery (Venus) in the same CT002 pool, LoadBalancer splits any grid surplus evenly across every reporter. The B2500 is charge-blind, so it silently ignores its half, and the Venus only ever sees half the real surplus — which can sit below its inverter's start-up threshold, leaving the Venus stuck in standby and the full surplus feeding to the grid. Adds a direct LoadBalancer-driven test that models a B2500-like charge-capped battery and a Venus-like battery with a start-up threshold, and asserts the observed symptom under a 600 W surplus. Includes solo-Venus and discharging controls to confirm the failure is specific to the DC+AC mix under surplus.
Real Marstek batteries advertise their model in the first field of the CT002 UDP request. The only AC-coupled family is the Venus lineup (``HMG`` and ``VNS`` prefixes); every other family (B2500 variants, Jupiter, future DC models) can discharge via AC but cannot charge. Previously the balancer split every surplus evenly across all reporters, asking DC-only batteries to charge — which they silently ignored, leaving the Venus with only half the real surplus. If that half sat below the Venus's inverter start-up threshold, the Venus stayed in standby and the full surplus kept feeding back to the grid. ``_compute_auto_target`` now treats any consumer whose ``device_type`` does not match a recognised AC prefix as DC-only and excludes it from charge distribution under ``grid_total < 0``. Positive grid_total (discharge) behaviour is unchanged; DC batteries continue to share the import load normally. Empty / unknown device types default to DC (fail-closed). If no recognised AC-chargeable battery is reporting under surplus, the balancer logs a single info-level notice and holds everyone at 0 W. Updates ``test_negative_target_concentrates`` to provide a Venus device_type for the two batteries under test — the efficiency pipeline it exercises is unrelated to the new guard. Test coverage: * b2500_excluded_from_charge_share_lets_venus_wake — canonical fix * all_known_venus_prefixes_are_charge_capable — HMG/VNS variants * non_venus_prefixes_are_treated_as_dc — HMA/HMB/HMJ/HMK/Jupiter * dc_discharge_still_shared_with_ac_sibling — discharge untouched * all_dc_under_surplus_holds_zero_and_logs — degenerate case * unknown_device_type_deadlock_persists — fail-closed default
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCT002 balancer now recognizes Marstek AC-chargeable batteries via Changes
Sequence Diagram(s)sequenceDiagram
participant Grid as Grid
participant Balancer as LoadBalancer
participant BatteryAC as AC Battery<br/>(Venus)
participant BatteryDC as DC-Only Battery<br/>(Marstek)
Grid->>Balancer: Report surplus (grid_total < 0)
Balancer->>Balancer: Inspect reporters' `device_type` prefixes
alt No AC-chargeable reporters detected
Balancer->>Balancer: Log once: "No AC-chargeable battery" (latched)
Balancer->>BatteryAC: Target = 0 W
Balancer->>BatteryDC: Target = 0 W
else AC-chargeable present
Balancer->>Balancer: Zero DC-only efficiency share
Balancer->>BatteryAC: Target = absorb surplus (charge)
Balancer->>BatteryDC: Target = 0 W
end
BatteryAC->>Grid: Absorb surplus
BatteryDC->>Grid: Remain idle
sequenceDiagram
participant Grid as Grid
participant Balancer as LoadBalancer
participant BatteryAC as AC Battery
participant BatteryDC as DC-Only Battery
Grid->>Balancer: Report deficit (grid_total > 0)
Balancer->>Balancer: Check device_type (no filtering for discharge)
Balancer->>BatteryAC: Target = fair-share (discharge)
Balancer->>BatteryDC: Target = fair-share (discharge)
BatteryAC->>Grid: Supply power
BatteryDC->>Grid: Supply power
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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
changed the title
Reported by the repo owner as a follow-up: when the B2500 is at 100% SoC any incoming DC solar flows straight through as AC output, so it reports positive power regardless of commands. Against the pre-fix balancer this pins the Venus around -340 W via the balance-correction + sign-clamp interaction, leaving ~160 W of sustained feed-in — and reproduces the user's symptom without any Venus startup-threshold assumption. The earlier commit fixed the split-evenly deadlock but the strict ``grid_total < 0`` guard let go at the exact zero-crossing during pass-through equilibrium (Venus at -500 W, B2500 at +500 W, grid at 0 W): the balance-correction fired, pushed Venus positive by one max_correction_per_step, and the system oscillated. Extend the charge-blind mask to also fire at ``grid_total == 0`` when any AC-chargeable battery is currently charging (``power < 0``). That signals pass-through equilibrium specifically, and keeps the discharge path untouched — a pure-discharge equilibrium at grid=0 has both batteries at positive power, so the extension doesn't activate. Adds ``B2500PassThrough`` to the test harness and a regression test verifying Venus converges to -500 W and grid to 0 W under the 500 W pass-through scenario.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_balancer_mixed_battery_charging.py (2)
411-418: Nit: hard-coded/30divisor is brittle.If
ticksor the tail slice size ever changes, these averages silently skew. Prefer computing the divisor from the slice so the assertions stay self-consistent.♻️ Suggested tweak
- avg_grid = sum(grid[-30:]) / 30 + grid_tail = grid[-30:] + avg_grid = sum(grid_tail) / len(grid_tail) assert abs(avg_grid) < 50, ( f"Discharge across both batteries should drain the grid; got {avg_grid:.0f} W" ) - assert sum(power["b2500"][-30:]) / 30 > 400, "B2500 should be discharging" - assert sum(power["venus"][-30:]) / 30 > 400, "Venus should be discharging" + b2500_tail = power["b2500"][-30:] + venus_tail = power["venus"][-30:] + assert sum(b2500_tail) / len(b2500_tail) > 400, "B2500 should be discharging" + assert sum(venus_tail) / len(venus_tail) > 400, "Venus should be discharging"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_balancer_mixed_battery_charging.py` around lines 411 - 418, Replace the hard-coded "/30" divisors when computing tail averages with the actual slice length to avoid brittle tests: compute the tail slice once (e.g., last_grid = grid[-30:]) and use len(last_grid) as the divisor when computing avg_grid and likewise use len(power["b2500"][-30:]) and len(power["venus"][-30:]) (or store those slices and reuse len) for the two battery power averages in the test that calls _run_scenario, ensuring the assertion math uses the slice length instead of the literal 30.
464-484: Note the one-shot info log also fires here.Both device_types are empty → both classified DC-only → this scenario also hits the
all_dc_under_surplusbranch and emits the same latched info log astest_all_dc_under_surplus_holds_zero_and_logs. The test doesn’t assert on or silence it (harmless), but the docstring frames this strictly as legacy deadlock behavior, which may mislead a future reader into thinking no log is expected. Consider a one-liner clarifying that the log is emitted as part of the fail-closed policy, or add a light assertion to pin the behavior.💡 Optional clarification
both batteries look like unknown DC batteries to the balancer, so nobody is asked to charge and the grid keeps feeding back the surplus. This is the expected fail-closed behaviour for consumers we can't identify. + + Note: this path also emits the one-shot "no AC-chargeable + battery" info log (same as the degenerate all-DC case), since + from the balancer's perspective these are indistinguishable + from unknown DC reporters. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_balancer_mixed_battery_charging.py` around lines 464 - 484, Update test_unknown_device_type_deadlock_persists to document or assert the emitted one-shot info log: either add a one-line sentence to the docstring stating that the `all_dc_under_surplus` branch emits the same latched info log (as in test_all_dc_under_surplus_holds_zero_and_logs), or explicitly capture the logger within the test and assert the expected info message was emitted (pinning the latched/logging behavior). Reference the test function name `test_unknown_device_type_deadlock_persists`, the related test `test_all_dc_under_surplus_holds_zero_and_logs`, and the `all_dc_under_surplus` branch when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_balancer_mixed_battery_charging.py`:
- Around line 411-418: Replace the hard-coded "/30" divisors when computing tail
averages with the actual slice length to avoid brittle tests: compute the tail
slice once (e.g., last_grid = grid[-30:]) and use len(last_grid) as the divisor
when computing avg_grid and likewise use len(power["b2500"][-30:]) and
len(power["venus"][-30:]) (or store those slices and reuse len) for the two
battery power averages in the test that calls _run_scenario, ensuring the
assertion math uses the slice length instead of the literal 30.
- Around line 464-484: Update test_unknown_device_type_deadlock_persists to
document or assert the emitted one-shot info log: either add a one-line sentence
to the docstring stating that the `all_dc_under_surplus` branch emits the same
latched info log (as in test_all_dc_under_surplus_holds_zero_and_logs), or
explicitly capture the logger within the test and assert the expected info
message was emitted (pinning the latched/logging behavior). Reference the test
function name `test_unknown_device_type_deadlock_persists`, the related test
`test_all_dc_under_surplus_holds_zero_and_logs`, and the `all_dc_under_surplus`
branch when implementing the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57c3b8e8-dd93-4b56-b2ce-bff32cdea3b2
📒 Files selected for processing (2)
src/astrameter/ct002/balancer.pytests/test_balancer_mixed_battery_charging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/astrameter/ct002/balancer.py
Review nitpicks: * test_dc_discharge_still_shared_with_ac_sibling now computes tail averages with ``len(slice)`` instead of a hard-coded ``/30``. * test_unknown_device_type_deadlock_persists docstring now notes that it also exercises the ``all_dc_under_surplus`` branch, with the log-latching behaviour asserted in the dedicated ``test_all_dc_under_surplus_holds_zero_and_logs`` test.
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.
Fixes #338.
Problem
When a DC-only battery (B2500 family, Jupiter) is paired with an AC battery (Venus) in the same CT002 pool,
LoadBalancer._compute_auto_targetsplits every grid surplus evenly across all reporters. The DC battery silently ignores its charge command and the Venus only sees half the real surplus — which can sit below the Venus's inverter start-up threshold (~300-500 W), leaving the Venus stuck in standby and the full surplus feeding back to the grid. Discharging works because the DC battery can contribute on positive grid.Fix
Real Marstek batteries advertise their model in the first field of the CT002 UDP request (already captured on
Consumer.device_type). The only AC-coupled family is the Venus lineup (HMGandVNSprefixes)._compute_auto_targetnow treats every other reporter as DC-only and excludes it from charge distribution undergrid_total < 0; the Venus receives the full surplus. Positivegrid_total(discharge) is unchanged — DC batteries continue to share the import load.device_typedefaults to DC (fail-closed) — an unrecognised battery cannot be assumed to accept charge commands.AC_CHARGEABLE_DEVICE_PREFIXES = ("HMG", "VNS")lives at the top ofbalancer.py; add prefixes there as new Venus variants ship.Changes
src/astrameter/ct002/balancer.py— newAC_CHARGEABLE_DEVICE_PREFIXESconstant,_is_ac_chargeablehelper, DC-exclusion step in_compute_auto_target, one-shot info log on all-DC-under-surplus.src/astrameter/ct002/ct002.py— one line:device_typenow included in the report dict passed to the balancer.tests/test_balancer_mixed_battery_charging.py— 20 tests exercising the fix path, every known Venus prefix (HMG / VNSE3 / VNSA / VNSD / bareVNS), every known DC prefix (HMA- / HMB- / HMJ- / HMK-) plusJUPITER-1and unknown strings, discharge untouched, all-DC degenerate case with log assertion, and fail-closed unknown-device default.tests/test_ct002_active_control.py—test_negative_target_concentratesnow provides a Venusdevice_typefor the two batteries under test; the efficiency pipeline it exercises is unrelated to the new guard.Test plan
uv run ruff format .uv run ruff check .uv run mypy src/uv run pytest— 588 passed, 17 skipped (pre-existing mosquitto skips), 1 pre-existing unrelated PTY test deselected in sandboxFollow-ups out of scope
AC_CHARGEABLE_DEVICE_PREFIXES. Symptom is "battery held at 0 W under surplus with no obvious reason" plus the info-level log message.Summary by CodeRabbit
New Features
Bug Fixes
Tests