fix(inverter): don't silently fall back to a real charge when charge freeze isn't available - #4435
fix(inverter): don't silently fall back to a real charge when charge freeze isn't available#4435chalfontchubby wants to merge 4 commits into
Conversation
… configured For #4424: when an inverter type generically supports charge freeze (inv_support_charge_freeze) but no charge_freeze_service is actually configured for this setup, adjust_charge_immediate() silently falls back to a real charge_start_service call instead of a passive hold. Extend the existing capability gate in fetch_inverter_data() to also check for this. Holding locally pending direction from #4432 before pushing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_freeze For #4432: the discharge-hold-while-holding-charge path used for car charging and iBoost (carHolding/boostHolding in execute_plan) calls adjust_charge_immediate(soc_percent, freeze=True) unconditionally whenever either hold is active, regardless of self.set_charge_freeze. This is a separate code path from the optimiser's planned charge-freeze windows (is_freeze_charge()), so the earlier fix for #4424 - which only disables self.set_charge_freeze when no charge_freeze_service is configured - didn't protect it: adjust_charge_immediate() still tried charge_freeze_service first and silently fell back to a real charge_start_service targeting the current SoC. Some inverters treat that as a fresh command each cycle and briefly ramp to full power, producing the repeated short full-rate import bursts reported on the issue, rather than a passive hold. Now gate the freeze attempt on self.set_charge_freeze, falling back to a plain charge-stop when no genuine passive freeze is available - the discharge-side hold already applied (pause/rate/reserve) is sufficient on its own in that case. Also fixes a test-harness staleness bug this exposed: run_execute_test() called fetch_inverter_data() directly without first re-deriving set_charge_freeze from raw config the way fetch_config_options() does every cycle in production, so a capability narrowing from one scenario (e.g. an unsupported inverter) silently leaked into every later scenario in the same test run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| # fall back to a real charge_start_service call instead of a passive hold (#4424), | ||
| # so treat it the same as an inverter type with no support at all. | ||
| self.log("Note: No charge_freeze_service configured - charge freeze disabled") | ||
| self.set_charge_freeze = False |
There was a problem hiding this comment.
This seems wrong, won't it disable charge freeze for all inverters that doesn't use the service?
There was a problem hiding this comment.
Good catch - you're right, this was too broad. Checked the default templates: GE's own givenergy_givtcp.yaml never sets charge_freeze_service either, and neither do any of the cloud integrations (FoxCloud, GEC, GEE, TESLA, EnphaseCloud, DeyeCloud, SolaxCloud, SolisCloud, SIGCLOUD, GWMQTT) - they all hold via adjust_battery_target() (target SoC) instead, which never touches charge_freeze_service at all. As written this would have disabled freeze-charge for the default GivEnergy setup and every cloud inverter.
Rescoped to only inverters with no target-SoC fallback (has_target_soc: False - SIG, FoxESS), which is what #4424's actual reporter (FoxESS via generic Modbus) is. Also found and fixed the same gap on the discharge/export side while I was in there, since GE/GEC's target-SoC write is explicitly skipped for export (target_soc_used_for_discharge: False), so they had zero protection there either - separate commit with its own explanation, 32e639f4.
Fix is in 1a2e5956 (charge side) + 32e639f4 (discharge side), tests updated to cover both the "has fallback" and "no fallback" cases for each.
There was a problem hiding this comment.
One additional comment, a 'Note:' message in the logfile won't get surfaced visibly in the predbat log viewer as 'note' isn't one of the keywords it looks for
Better to set to 'Warn:' so it appears in the Warn and Error tabs
…ithout a target-SoC fallback Trefor flagged on #4435 that disabling set_charge_freeze whenever charge_freeze_service isn't configured is too broad. Confirmed: GE's own default template (givenergy_givtcp.yaml) never sets charge_freeze_service, and neither do any of the cloud integrations (FoxCloud, TESLA, EnphaseCloud, DeyeCloud, SolaxCloud, SolisCloud, SIGCLOUD, GWMQTT, GEC, GEE) - all of these achieve a passive hold via adjust_battery_target() instead, which never touches charge_freeze_service. As written, the check would have disabled the optimiser's freeze-charge search entirely for the default GivEnergy setup and every cloud inverter. Only inverters without a target-SoC fallback (has_target_soc: False, e.g. SIG, FoxESS) genuinely depend on charge_freeze_service - #4424's actual reporter is one of these. Scope the check to that class only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rge_freeze_service Same class of bug as the previous commit, but on the export/discharge side, and never previously guarded at all - inv_support_discharge_freeze was checked, but a missing discharge_freeze_service was not, so adjust_export_immediate() could already silently fall back to a real discharge_start_service call (the export-side equivalent of #4424). The fallback condition differs from the charge side though: adjust_battery_target() (inverter.py:1899) only writes a target SoC during export when inv_target_soc_used_for_discharge is also set. GE/GEC have a target SoC but target_soc_used_for_discharge is False for them, so unlike charge freeze they get no passive-hold protection from it and genuinely depend on discharge_freeze_service - the scoping condition has to check both flags together, not just inv_has_target_soc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
I think it should be explicitly surfaced as a Warn in the logfile so its visible |
There was a problem hiding this comment.
Pull request overview
This PR tightens Predbat’s “freeze” behavior to avoid silently falling back to an active charge when a true freeze mechanism isn’t available, covering both optimiser-driven freeze windows and the direct carHolding/boostHolding execution path.
Changes:
- Gate
carHolding/boostHolding’s charge-side “hold” to only run whenset_charge_freezeis enabled. - In
fetch_inverter_data(), disable charge/export freeze when the inverter lacks the required freeze primitive (or the config implies it’s missing), to prevent unsafe fallbacks. - Add/extend execute-layer tests to validate the new capability narrowing and prevent stale capability leakage across test scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/predbat/execute.py | Adds gating in execute_plan() and adds freeze-capability narrowing in fetch_inverter_data() to avoid unsafe fallbacks. |
| apps/predbat/tests/test_execute.py | Adds regression tests for freeze capability narrowing and for carHolding/boostHolding fallback behavior; fixes test harness state leakage for set_charge_freeze. |
Suppressed comments (1)
apps/predbat/execute.py:832
- This disables charge freeze whenever charge_freeze_service isn’t configured and the inverter lacks target SoC, but some inverter types can still support freeze via rate/pause control without any services (e.g. Huawei’s template sets support_charge_freeze true but does not define charge_freeze_service). To avoid disabling a valid freeze implementation, consider gating this only when the dangerous fallback can occur (i.e. charge_start_service is configured).
elif not inverter.inv_has_target_soc and not self.args.get("charge_freeze_service", ""):
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.log("Note: Inverter does not support discharge freeze - disabled") | ||
| self.set_export_freeze = False | ||
| self.set_export_freeze_only = False | ||
| elif not (inverter.inv_has_target_soc and inverter.inv_target_soc_used_for_discharge) and not self.args.get("discharge_freeze_service", ""): |
There was a problem hiding this comment.
Traced every call site that this gate feeds (adjust_charge_immediate/adjust_export_immediate with freeze=True, at lines 526-527, 536-609, and the carHolding/boostHolding block) - none of them have a rate-based fallback. adjust_export_immediate()/adjust_charge_immediate() are service-calls only; there's no branch that uses inv_charge_discharge_with_rate to freeze via rate=0 instead.
So for Huawei specifically: it does define discharge_start_service/charge_start_service (huawei_solar.forcible_discharge_soc/forcible_charge_soc in templates/huawei.yaml) - the real, active command these methods fall back to when the freeze service is missing. Your own suggested narrower condition ("only disable when the unsafe fallback is actually possible, i.e. discharge_start_service is configured") would still land on disabling it for Huawei, since that service is configured there.
Worth noting this gate is actually a fix, not a regression, for Huawei on both sides: before this PR, the only check was inv_support_discharge_freeze/inv_support_charge_freeze (both true for Huawei via its template), so the optimiser would happily plan freeze windows for it and execution would hit exactly the dangerous fallback this PR exists to close (a real forcible_charge_soc/forcible_discharge_soc call instead of a passive hold). Huawei still gets a safe hold via the existing reserve/pause "Hold charging"/"Hold exporting" path when freeze is disabled - it just isn't labelled/optimised specifically as "Freeze charging"/"Freeze exporting".
@copilot does this match what you were seeing, or is there a rate-based freeze path I'm missing somewhere?
Summary
Opening this as a straw man for discussion on #4432/#4424, not as a finished proposal - happy to rework based on direction from @gcoan / @springfall2008.
adjust_charge_immediate(soc, freeze=True)triescharge_freeze_serviceand silently falls back to a realcharge_start_service(targeting current SoC) when that service isn't configured for this setup, even though the inverter type generically supports charge freeze. Some inverters treat that as a fresh command each cycle and briefly ramp to full power, rather than a passive hold.ded65145): when nocharge_freeze_serviceis configured,set_charge_freezeis now disabled the same way it already is for inverter types with no support at all - this covers the optimiser's planned charge-freeze windows.0503950): that flag wasn't checked by the separatecarHolding/boostHoldingdirect-call path (holding the battery SoC while a car charges or iBoost diverts surplus to the immersion) - widened the gate to cover that path too, since it hits the exact same fallback. Falls back to a plain charge-stop when no genuine freeze is available; the discharge-side hold already in place is sufficient on its own.See discussion on #4432 for the fuller trace of the
boostHoldingpath and why it's structurally identical tocarHolding.Open questions for review
set_charge_freezeentirely (vs. some narrower per-call gate) the right granularity, or should this be surfaced to the user differently (e.g. a warning rather than silent disable)?carHolding/boostHoldingwhen freeze isn't available, or is there a better degraded behaviour?Test plan
./run_all --quickpasses with no regressions./run_pre_commitpasses (ruff, black, cspell, markdownlint, full test suite)set_charge_freezenarrowing whencharge_freeze_serviceis/isn't configured and when the inverter type doesn't support freeze at all;carHolding/boostHoldingfalling back to a plain charge-stop when no freeze service is configuredrun_execute_test()wasn't re-derivingset_charge_freezefrom raw config each scenario the way production does every cycle, so a capability narrowing from one scenario was leaking into every later one🤖 Generated with Claude Code