Repository navigation
fix(inverter): stop re-committing a stable export window every cycle (Solis) - #4711
Merged
Merged
Conversation
…changed Addresses #2328. press_and_poll_button()'s "separate buttons" fallback (charge_update_button/discharge_update_button, used by e.g. some Solis and SolaX SX4 configs instead of a single combined button) pressed both buttons on every call, regardless of which side actually needed updating - a charge-only window change also pressed the unrelated discharge button, and vice versa. rszemeti's report showed exactly this: both update_charge_times and update_discharge_times pressed together even when the charge window's own start/end times were provably unchanged (every underlying write correctly logged "No write needed"). He disabled Predbat after reaching ~75k inverter writes in a year to avoid EEPROM wear. Added a `side` parameter ("charge"/"discharge"/"both", default "both" for any caller not yet updated) and passed the correct side from each of the four call sites (adjust_battery_target, adjust_force_export, disable_charge_window, adjust_charge_window). A single combined button (schedule_write_button or charge_discharge_update_button) is unaffected - there's only one button either way. New test_press_and_poll_button_side_scoping isolates the branching logic by stubbing _press_single_button_and_poll (its own retry/timestamp-polling behaviour is a separate concern), asserting each side presses only its own button. No prior coverage existed for the separate-buttons path at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
For H M format inverters the discharge slot-time registers are rewritten on every cycle as a write-reliability workaround (#1529), so changed_start_end is True on every cycle of an unchanged export window. #4000 then used that flag to gate the update button press, which made the button fire every 5 minutes for the whole window. On Solis each press zeroes the timed charge/discharge current registers, so the inverter stops exporting until something restores them (#4709). The same flag also triggers the 30 second GivTCP settle sleep in adjust_inverter_mode, and the export-slot notification, on every cycle. Separate "we rewrote the registers" from "the schedule needs committing" and gate the press, the sleep and the notify on the latter. Tracking the last committed schedule keeps a stable window quiet while still committing once after a restart, when nothing has been committed yet - so #4000's own regression test passes unchanged. The extra commit-once safety net is scoped to the H M path; every other format already commits on a real change alone. The register writes themselves are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
press_and_poll_button returns a success flag that was being discarded, so a failed button press was still recorded in last_export_schedule_committed and never retried - the schedule would sit uncommitted until it happened to change on its own. The behaviour being replaced retried implicitly, because changed_start_end fired every cycle. Gating on the real result keeps that retry without bringing back the every-cycle press. Same failure shape as the known call_service_template issue, where a service call is marked done before its success is checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ble-only flip schedule_changed also goes True on a bare scheduled_discharge_enable flip with start/end unchanged (e.g. Fox and other HH:MM:SS-format cloud inverters), but that flag was being passed straight into adjust_inverter_mode's changed_start_end parameter, which gates a 30 second sleep intended specifically for "start/end of discharge window was just adjusted". No time register was written in that case, so there's nothing for GivTCP to settle. Split out times_changed (start/end genuinely differing, or the H M format commit-once-after-restart case) and gate the settle sleep on that instead, leaving schedule_changed - correctly the broader flag - for the button press, last_export_schedule_committed tracking and notify gating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
execute.py calls adjust_force_export(False) with no times at all whenever nothing is being exported. On an inverter with has_discharge_enable_time set the midnight override is skipped, so new_start/new_end stay None while the inverter still reports a real time - and None never compares equal, so every idle cycle looked like a schedule change and pressed the update button. That is most of the day, not just export windows (#2328). Only compare times the caller actually asked us to set. A genuine transition out of export is still caught by force_export != old_discharge_enable, and the GE branch that deliberately clears the times keeps its existing behaviour - it is distinguished by whether the caller supplied any times, not by the times being None once we are past that branch. Affects GS_fb00 and the FoxCloud/TESLA/Enphase/Deye/Sunsynk/AlphaESS cloud types; plain GS takes the midnight-override path and was never affected. Not yet confirmed against a live install - awaiting logs on #4709. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ss_every_cycle - save/restore the discharge_* args and dummy_items this test overwrites, not just inverter_type, so nothing leaks into later tests - reword the docstring: plain GS wasn't affected by this test's specific None-comparison bug, but it does use the same "H M" time format that #4711's separate unconditional-write bug affected
… on idle non-export cycles fix(inverter): don't press the update button on idle non-export cycles
test_force_export_off_does_not_press_every_cycle named the FoxCloud/TESLA/ Enphase/Deye/Sunsynk/AlphaESS cloud types in its docstring as the affected set but only parametrised GS_fb00 and GS, so the cloud behaviour rested on GS_fb00 happening to share has_discharge_enable_time rather than on a test. FoxCloud stands in for those types, and its expected press count is 0 rather than 1: being HH:MM:SS format it never takes the H M unconditional register rewrite, so the commit-once safety net gated on that format is skipped too and an idle cloud inverter should be silent, not spend one press on a first post-restart commit the way GS_fb00 does. Also adds a positive control - entering an export window straight after the idle cycles must still commit exactly once. Without it an inverter that had stopped pressing altogether would satisfy the idle assertion just as well, which is the failure mode that would actually matter to a Fox user. inv.sleep is stubbed for the loop: TestHAInterface applies a service call synchronously, so the write-and-poll read-back already sees the new value and the sleep between them is pure wall clock. discharge_target_soc and the idle time selects join saved_items, as the positive control writes them and this test shares my_predbat/ha with the rest of the run. Verified against two mutations of inverter.py, each reverted after: restoring the pre-fix None comparison (times_supplied = True) makes FoxCloud press on all 4 idle cycles, and disabling the press block trips the positive control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # apps/predbat/inverter.py # apps/predbat/tests/test_inverter.py
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Commit failure propagation, schedule-write result handling, and test-state isolation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR reduces redundant Solis inverter schedule commits while preserving necessary register writes and retries.
Changes:
- Tracks committed export schedules separately from register rewrites.
- Scopes update-button presses and related actions to actual changes.
- Adds regression tests for stable windows, failed commits, idle cycles, and button scoping.
File summaries
| File | Summary |
|---|---|
apps/predbat/inverter.py |
Updates export commit tracking and button handling. |
apps/predbat/tests/test_inverter.py |
Adds regression coverage for commit behavior and button scoping. |
Review details
Suppressed comments (2)
apps/predbat/inverter.py:2789
times_suppliedis false foradjust_force_export(False)with no arguments, but the no-discharge-enable branch above deliberately synthesizes and writes a midnight start/end. If those registers drift while export is already disabled, this check treats the write as unchanged once the cache holds the midnight tuple, so no update button is pressed to commit the new values. Include the internally generated disable window in the managed-time dirty check while keeping the GE immediate-control branch excluded.
start_changed = times_supplied and new_start != old_start
end_changed = times_supplied and new_end != old_end
apps/predbat/inverter.py:2641
- This
times_suppliedguard, together with the new FoxCloud/GS_fb00 idle regression, implements the None-comparison fix described for #4713. That contradicts the PR description's claim that this route is untouched and is left to the stacked #4713; please update the stated scope/stacking or remove the duplicated behavior so the two PRs do not conflict.
times_supplied = (new_start_time is not None) or (new_end_time is not None)
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: chalfontchubby <48563392+chalfontchubby@users.noreply.github.com>
Contributor
Automated reply from Copilot: addressed the review items in cfeaad4. This covers button-failure propagation, commit-cache updates only after the relevant schedule writes succeed, and shared-state restoration in the inverter regression tests; the GS midnight disable-window path is also kept commit-aware. |
Purely black's formatting of the saved_items comprehension in
test_force_export_enable_only_flip_skips_settle_sleep; it fits inside
the 256-char line limit, so black joins it. No behaviour change.
pre-commit.ci lite skipped auto-fixing this one ("skip: triggered by a
bot"), so it needed doing by hand.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 17, 2026
CossieRob
pushed a commit
to CossieRob/batpred
that referenced
this pull request
Sep 26, 2026
fetch_inverter_data() rebuilt every Inverter on every call, and it is called one or more times per cycle - the plan run re-reads the inverter again before execute_plan(). Any state an Inverter accumulated was therefore wiped before it could be used, which is why the commit-once guard added in springfall2008#4711 held in the unit tests (one long-lived Inverter) but never suppressed anything in production. A reporter's log on v9.0.3 shows 377 update-button presses over one night, one or two per cycle, with every register logging "No write needed". The rebuild was never a deliberate design: before springfall2008#1803 fetch_inverter_data() had no choice, it always did self.inverters = []. That PR added a create parameter to allow reuse but kept create=True as the default, and only one caller ever opted out, so the main loop kept rebuilding. Keep the objects instead, and split __init__ into the identity/state half that runs once and a refresh_config() that runs every cycle. Everything that reads runtime-changeable config (battery_min_soc, battery_scaling, the rate limits, battery_calibration, battery_temperature) or has a per-cycle side effect (battery size tracking, clock skew detection, reserve bounds) moves into refresh_config, so persisting the object does not freeze a setting a user can change while Predbat is running. The static half - inverter type, capability flags, dummy entity creation - cannot change under a live object, since apps.yaml changes restart Predbat. The create parameter is now meaningless and is removed rather than left as a flag that silently does nothing. The two Inverter test doubles gain a no-op refresh_config; they carry no config to re-read. Measured with the reporter's config (GS, hour/minute entities only, a charge_discharge_update_button), driven through fetch_inverter_data() as production does: main presses/cycle = [2, 1, 1, 1, 1, 1] fixed presses/cycle = [2, 0, 0, 0, 0, 0] State is deliberately not persisted across restarts: losing it costs one re-commit on startup, which is defensible anyway, and is not worth an HA storage dependency. Addresses springfall2008#4712. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #4709
Problem
On Solis
GS_fb00(solax_modbus), pressing the charge/discharge update button zeroes the timed charge/discharge current registers. The reporter measured the current register being knocked to 0 A at:xx:22on every 5-minute boundary throughout an active export window, stopping export until something restored it.The cause is in
adjust_force_export. ForH Mtime format the discharge slot-time registers are rewritten unconditionally every cycle — a deliberate write-reliability workaround from #1529. #4000 then reused that same "we wrote something" flag (changed_start_end) to decide whether to press the update button, so the button fired every cycle even when the window had not changed.#4000's stated premise was that
adjust_charge_window"always re-writes time entities for H M format → button press fires every cycle". The write half is correct; the button-press half is not — the charge side's press is gated on a plain dirty check and always has been. So the every-cycle press was never symmetric with anything.The same flag also gates the 30 second GivTCP settle sleep in
adjust_inverter_modeand the export-slot notification, both of which were therefore firing every cycle too.Fix
Separate "we rewrote the registers" from "the schedule needs committing", and gate the press, the sleep and the notify on the latter.
Tracking the last committed schedule keeps a stable window quiet while still committing once after a restart, when nothing has been committed yet — so #4000's own regression test passes unchanged. The commit-once safety net is scoped to the
H Mpath; every other format already commits on a real change alone.A commit is only recorded once it has actually succeeded.
press_and_poll_buttonreturns a success flag, and discarding it would mean a failed press was recorded as done and never retried — the schedule sitting uncommitted until it happened to change on its own. The behaviour being replaced retried implicitly, becausechanged_start_endfired every cycle, so gating on the real result preserves that retry without bringing back the every-cycle press. (Same failure shape as the knowncall_service_templateissue, where a service call is marked done before its success is checked.)The register writes themselves are untouched.
Also fixes idle-cycle presses on plain
GSBeyond the export window, this PR also stops a second symptom that only came to light during triage of #4712.
GS(pre-FB00 Solis) ischarge_time_format: "H M", sochanged_start_endis set unconditionally there too — which means on currentmainit presses the update button on every idle cycle as well, all day, not just during export. Droppingchanged_start_endfrom the commit condition fixes that. Measured over four idleadjust_force_export(False)calls:GS_fb00and the cloud types reach the same press by a second, independent route (aNonetime comparison) which this PR does not touch — that is #4712, fixed by #4713 stacked on top of this one.Also included
press_and_poll_buttonnow presses only the side that changed. Where separatecharge_update_button/discharge_update_buttonentities are configured, both were pressed whenever either side had something to send — unchanged since the original split-button support. Halves the presses for those users; no effect on the combined-button setups the Solis template ships by default. This was written for #2328 and parked as too small to be worth landing alone.Commits
9a3a0e23press_and_poll_buttononly presses the button(s) that changedbe3d225f4499357cTesting
4499357c.adjust_force_export1passes unchanged, which is what pins the GE path's behaviour.--quicksuite green.Not yet confirmed against a live install — logs requested on #4709.
🤖 Generated with Claude Code