fix(execute): skip the redundant charge-off call while exporting - #5114
chalfontchubby wants to merge 1 commit into
Conversation
Prevents the charge-off block's unconditional charge_stop from re-firing after adjust_export_immediate() has already stopped charging and started the export - on service-template inverters (e.g. Tesla) that write a shared mode-select entity, the trailing call clobbered the mode discharge_start_service had just set, so the export never engaged.
|
Labelled priority high since the triage bot decided the source issue was. |
There was a problem hiding this comment.
🔵 Needs a closer look
The export guard is fleet-wide and can skip required charge-off actions for later non-exporting inverters in mixed fleets.
Pull request overview
Fixes redundant charge-stop calls during export and adds regression coverage.
Changes:
- Guard charge-off logic during active export.
- Reset test sentinels between scenarios.
- Update export and read-only expectations.
File summaries
| File | Summary |
|---|---|
apps/predbat/tests/test_execute.py |
Updates regression coverage and test isolation. |
apps/predbat/execute.py |
Skips redundant charge-stop calls during export. |
Review details
Suppressed comments (1)
apps/predbat/execute.py:776
isExportingis initialized once before the inverter loop and only ever set toTrue, so this guard is fleet-wide rather than scoped to the current inverter. In a mixed fleet where an earlier inverter is exporting but a later inverter is not (a state this module explicitly supports viastatus_per_inverter), the later inverter now skips its own charge-off call and can remain in its previous charging/mode state. Track the current inverter's export state separately or reconcile the fleet-level service action, and add a mixed-fleet regression case.
if not isCharging and not isExporting and self.set_charge_window:
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Claude wrote this reply, working with me. On the Copilot review point about the The mechanism is real. But it isn't new here. Two things point that way:
More directly: the sibling line immediately below is unmodified from main: if not isExporting and self.set_export_window:
inverter.adjust_export_immediate(int(EXPORT_LIMIT_IDLE))That is the exact condition being flagged as a new risk — fleet-wide So the accurate description is that this extends a pre-existing fleet-scoping limitation to one more branch, in a block where both the adjacent line and the other flag already work that way. Why I'd not fix it here. The limitation is genuine and probably worth fixing, but a real fix means making both flags per-inverter across the whole block, which changes It is also the same underlying gap as the symmetric case I flagged in the PR description (the export-side counterpart having no Happy to split the per-inverter scoping out as its own issue if you agree that's the right shape, or to do it in this PR instead if you'd rather it went in together. |
This is an automated draft PR generated from issue #4641 — a maintainer should review it before merging.
Fixes #4641
Also fixes #4165 (duplicate report, closed in favour of #4641; same reporter, same root cause, independently confirmed against its own attached log/debug.yaml)
Summary
In
Control charge & dischargemode, the "Charging/Discharging off via service" block inexecute_plan()(apps/predbat/execute.py) fired an unconditionaladjust_charge_immediate(0)→charge_stop_servicewheneverisChargingwas False — which is also true while actively exporting. This ran afteradjust_export_immediate()had already issued its owncharge_stop_servicefollowed bydischarge_start_servicemoments earlier in the same cycle.On service-template inverters (e.g. the Tesla custom/Teslemetry integrations) where
charge_stop_serviceanddischarge_start_servicewrite the same mode-select entity, the trailing charge-stop call clobbered the mode back to e.g. "Self-Powered"/"self_consumption" immediately afterdischarge_start_servicehad set it to "Time-Based Control"/"autonomous" — so the export never actually engaged, even though Predbat's own state showed it as active. This reproduced on every export attempt for both reporters (#4641, #4165), across different Predbat versions (v8.42.5 through v9.0.1).The fix adds an
isExportingguard to that block:adjust_export_immediate()already stops charging as part of starting the export, so re-issuing a second charge-stop there added nothing but the clobbering write.Testing
apps/predbat/tests/test_execute.py:discharge_car_full_bat(an existing "Exporting" scenario withset_charge_window=True) now asserts the charge-immediate sentinel stays untouched (-1) rather than being forced to0by the extra charge-stop call.immediate_charge_soc_target/immediate_discharge_soc_targetwere never reset between scenarios inrun_execute_test(), so several "Exporting" scenarios were incidentally passing on stale values left over from whichever scenario ran immediately before them, rather than asserting anything meaningful about the current scenario. Added an explicit reset per scenario, and corrected two read-only scenarios' expectations to-1/-1(no service calls are issued in read-only mode at all) now that the sentinel is properly reset.predbat_debug.yaml/log/apps.yaml from Tesla Powerwall 3 (Teslemetry) — Operation Mode rapidly flip-flops between Autonomous and Self-consumption within same control cycle, export not working #4165's attachments — replayed clean under./run_all --debug_file ... --redo.execute.pychange (test change kept) makes./run_all --test executefail withERROR: Inverter 0/1 Immediate charge SOC target should be -1 got 0on thedischarge_car_full_batscenario; restoring the fix passes../run_all --quickand./run_pre_commitboth pass clean.Notes
.gitnexus/index in this checkout), so the blast-radius check was done by search instead:execute_plan()is called only frompredbat.py's main update loop, the change is entirely internal to its own per-inverter loop, andtest_execute.py's full scenario suite (~180 cases) passes unchanged apart from the intentionally-updated assertions above.if not isExporting and self.set_export_window: inverter.adjust_export_immediate(EXPORT_LIMIT_IDLE)) has noisChargingguard either, and could in principle clobber a charge-mode select on an inverter with nodischarge_stop_serviceconfigured. This is untested by either reporter's logs and not part of the reported symptom, so it's called out here rather than spelled speculatively — a maintainer may want a follow-up issue if it turns out to matter in practice.