fix(inverter): don't silently fall back to a real charge when charge freeze isn't available - #4435
Open
chalfontchubby wants to merge 5 commits into
Open
fix(inverter): don't silently fall back to a real charge when charge freeze isn't available#4435chalfontchubby wants to merge 5 commits into
chalfontchubby wants to merge 5 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>
springfall2008
requested changes
Aug 4, 2026
…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>
Collaborator
I think it should be explicitly surfaced as a Warn in the logfile so its visible |
Contributor
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.
Collaborator
Author
|
Think your change request is addressed in the response above |
…ot Note gcoan's PR review ask: a plain "Note:" log line doesn't appear in the log viewer's Warn/Error tabs, so the disable-path messages for a missing charge_freeze_service/discharge_freeze_service went unnoticed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
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
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.
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