fix(inverter): Persist inverters across planning cycles

[chalfontchubby]

(Written by Claude, posted by @chalfontchubby.)

Problem

The update-button-commit mechanism on time_button_press inverters (Solis and others) was pressing the button every 5-minute cycle instead of once per genuine schedule change. A reporter on v9.0.3 supplied full logs (predbat.log/apps.yaml/debug.yaml) showing 377 presses in one night — every register logging "No write needed", the button pressed anyway, and count register writes 0 on the same cycle, hiding the repeats from the write counter entirely.

That's the same signature as the original #2328 report:

Inverter 0 write_and_poll_value: No write needed for charge_start_time: 23:59:00 == 23:59:00 fuzzy 0
...
Successfully pressed button button.solis8_update_charge_times on Inverter 0
Successfully pressed button button.solis8_update_discharge_times on Inverter 0

On Solis this is a non-volatile write each time (EEPROM wear), and each press also zeroes the timed current registers (#4415/#4709).

Root cause

fetch_inverter_data() defaults to create=True and runs one or more times per cycle — the plan run re-reads the inverter again before execute_plan() — so the Inverter objects were rebuilt before any commit-once guard could ever latch. #4711 added exactly such a guard on the export side, and it worked in the unit tests (which reuse one long-lived Inverter) but did nothing in production, because the object it lived on didn't survive to the next cycle.

The rebuild itself was never a deliberate design — before #1803 (Dec 2024) fetch_inverter_data() had no create parameter at all, it always rebuilt. That PR added the parameter to allow reuse but kept create=True as the default, and only one caller ever opted out, so the main loop kept rebuilding regardless.

Fix

Commit 1 — persist the Inverter objects across cycles instead of rebuilding them. __init__ is split into a one-time identity half and a refresh_config() that runs every cycle, re-reading everything that's genuinely runtime-changeable (battery_min_soc, battery_scaling, the rate limits, battery_calibration) so persisting the object doesn't freeze a setting a user can change while Predbat runs. Also adds the charge-side commit-once guard that never existed (adjust_charge_window() had no equivalent to the export side's protection), with the same write-result accumulation (schedule_write_ok) that review required on the export side in #4711.

Measured with a real reporter 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]

Commit 2 — persisting the objects exposed a mess in __init__: real one-shot state, real None sentinels, and placeholder values indistinguishable from genuine config (reserve_percent = 4.0, battery_rate_max_raw = 2600.0) were all set the same way in the same block. Harmless while the object was rebuilt every cycle; not harmless once it persists, since a stale placeholder can now live long enough to be read for real. Regrouped by actual lifetime into a new reset_cycle_state(), seeded to None rather than a real-looking value wherever nothing depends on a specific default.

That pass caught two real bugs the persistence work introduced along the way:

• in_calibration was write-only — only ever set True, never back False — previously masked by __init__ reseeding it every cycle. On a persisted object a battery leaving calibration would stay stuck. Now a real two-way read.
• battery_voltage was seeded 52.0, a guessed value used as a divisor in set_current_from_power(), reachable before update_status() has ever fetched a real reading. Seeded None instead; the divide site now checks explicitly and skips with a warning rather than computing from a made-up voltage.

Scope

Deliberately not persisted across Home Assistant restarts — that would need HA storage, which is overkill for a cost of one re-commit on reboot.

The register-write counter fix (a button press wasn't counted, hiding the repeats from count_register_writes) is independent and already up as #5123.

Testing

• Full quick suite green, pre-commit clean, on both commits individually and at the tip
• New regression tests: a commit survives object recreation-that-no-longer-happens, a stuck scheduled_charge_enable commits once not every cycle, a failed write/press retries on the next cycle rather than being recorded as done, a same-count inverter_type change at runtime rebuilds correctly, in_calibration clears when battery_calibration goes off, set_current_from_power() doesn't crash before update_status() has run
• run_shuffle --hunt --quick run against a static tree: 50/50 clean, no order-dependency pair found (Test infrastructure keeps producing fixture state-leak bugs #5079)

Fixes #4712. Fixes #2328 - matches that report's log signature exactly (every register "No write needed", button pressed anyway). Follow-ups were split off #2328 and #4712's later comments so they survive this close as their own tracked items, unrelated to this PR's fix: #5127 (unconfirmed GS_fb00 + charge_limit micromanagement report, needs fresh logs), #5128 (Feed-in priority modes 64/96/98 never reachable), #5129 (charge_limit should default on for GS_fb00), #5130 (a register read-back briefly bouncing through a wrong value after a write - a write-settling question, not this PR's object-recreation fix).

[Copilot]

🟡 Changes recommended

refresh_config() should re-seed standing defaults like inverter_limit/export_limit each cycle to avoid stale overrides persisting after config keys are removed, and one new test comment misstates the current suite-level fixture isolation model.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes repeated schedule “commit” button presses on time_button_press inverters by persisting Inverter instances across cycles (so commit-once guards can latch), while still re-reading runtime-changeable configuration each cycle via a new refresh_config() split out of Inverter.__init__.

Changes:

• Persist Inverter objects across planning/execution cycles in fetch_inverter_data() and refresh their runtime config each cycle instead of rebuilding.
• Refactor Inverter initialization into persistent vs per-cycle state (reset_cycle_state() + refresh_config()), and add a charge-side commit-once guard mirroring the export-side behavior.
• Add regression tests and update test doubles to account for per-cycle refresh_config().

File summaries

File	Description
apps/predbat/execute.py	Reuses existing Inverter objects across cycles; rebuilds only on count/type change and calls refresh_config() on reuse.
apps/predbat/inverter.py	Splits init vs runtime refresh, introduces cycle-state seeding, adds charge commit-once guard, and avoids dividing by unknown battery_voltage.
apps/predbat/tests/test_inverter.py	Adds regression tests for persistence, charge commit-once behavior, calibration clearing, and current calc before voltage is known.
apps/predbat/tests/test_execute.py	Updates stubs and calls to match new fetch_inverter_data() signature and per-cycle refresh_config().
apps/predbat/tests/test_inverter_config_sensor.py	Updates calls to fetch_inverter_data() after signature change.
apps/predbat/tests/test_infra.py	Updates inverter test double to include inverter_type and refresh_config() required by the new reuse path.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chalfontchubby]

(Posted by Rik; working with Claude.)

Both Copilot findings on this PR are fixed in 58c634b:

refresh_config() left a stale inverter_limit/export_limit override in place. Both were only overwritten if "inverter_limit"/"export_limit" in self.base.args, so removing either key from config at runtime (or a test popping it) left the previous cycle's value in place indefinitely instead of reverting to the standing default. Now reset to the default (7500W/99999W) at the start of every refresh, before the conditional re-read.

A test comment described the wrong scope. Said "the suite shares one PredBat/HA fixture across every registry test" when that's actually true of run_inverter_tests()'s own scenarios, not the whole suite. Reworded.

Both covered by the existing inverter test suite, green.

[Copilot]

🔵 Needs a closer look

It changes core inverter lifecycle/state semantics (object persistence + per-cycle refresh) with broad runtime impact, so it warrants final human review despite strong regression coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[springfall2008]

(Review by Claude, posted by @springfall2008.)

Reviewed the whole diff and verified the findings against the branch rather than just reading it. Two reproduce on the PR head; details are inline.

Two things that check out first: I ran an attribute-lifetime audit comparing main's __init__ against the PR's refresh_config() + update_status() — apart from inverter_time (noted inline), nothing that used to be reseeded per cycle can now go stale, and both count_register_writes (zeroed by execute.py:788) and the window lists (rebuilt from []) are safe to persist. I also test-merged into current main: clean, and #2444's multi-format clock parse survives the move into refresh_config().

Blocking, reproduced:

1. A partially-mapped apps.yaml defeats the entire fix — the charge side folds write results into schedule_write_ok without the elif start_hour_id: guards the export side has, so one unmapped hour/minute entity latches charge_schedule_commit_pending forever and presses every cycle. Measured on this branch with a GS config missing charge_start_minute/charge_end_minute:

unmapped minute entities:  cycles 1-5, presses=1 each, pending=True, committed=None  -> 5 presses
all four mapped:           cycle 1 presses=1, cycles 2-5 presses=0                   -> 1 press

2. A dropped inverter_time reading now leaves a stale timestamp on the persisted object, so clock-skew detection runs against it and eventually calls auto_restart() for a clock that is correct:

PROBE first refresh (good reading): inverter_time = 2026-09-19 12:00:00+01:00
PROBE after 'unavailable':          inverter_time = 2026-09-19 12:00:00+01:00   <- stale

3. Separately, quiet=not create silently removes every per-cycle inverter diagnostic line from predbat.log for the life of the process — the evidence this PR itself was diagnosed from.

The core idea (persist the objects, split the per-cycle config re-read out of __init__) looks right to me, and the lifetime regrouping in commit 2 is a genuine improvement. The issues below are about the guard's edges, not the approach.

[chalfontchubby]

(Posted by Rik; written by Claude.)

Thanks for the review - the reproductions on the two blocking items made them quick to confirm and fix. All 13 points have a reply on their own thread; leaving them for you to resolve. Summary, and the two things worth your eye.

Blocking

• inverter.py:3357 unmapped minute entity - fixed, export side's elif guards copied over, regression test added (d99a705)
• inverter.py:374 stale inverter_time - fixed, reset every cycle above the read (d99a705)

Should fix

• execute.py:977 diagnostics silenced - fixed via an explicit quiet parameter rather than always-on, so balance_inverters() stays quiet (3d8f3dc)

Consolidated (0d60ef6) - your press_and_poll_button() suggestion, taken as the design steer. commit_needed()/record_commit() keyed by scope, last_committed/commit_pending dicts replacing the four per-side attributes, all four call sites guarded. That resolves 3397 (export mirror), 3407 (observed state in the key), 3412 (both ordering issues) and 3434 (third and fourth sites) by deleting the duplication rather than patching each instance.

Also fixed

• execute.py:946 - the branch is removed, not the comment reworded. refresh_config() really does re-derive all of it, so the rebuild was redoing that work while discarding the commit state (1650a57)
• execute.py:1179 - refresh failure no longer wipes the other inverters (3a5da63)
• inverter.py:362 dead seeds gone, literals named in const.py (3d8f3dc)
• inverter.py:344 renamed _init_attribute_defaults() (1650a57)
• test_inverter.py:202 reset moved into both scenario helpers (0d60ef6)

Deferred - test_inverter.py:3074, replied on the thread and left open. Real, but it predates this PR and belongs with #5079.

---

Two things I would rather you saw than found:

Two adjust_charge_window expectations now assert a press where they previously asserted none. Both were relying on suppression that came from observed state (new == old, enable read back "on") rather than from anything having been committed - the reasoning you identified as broken. The first commit of a window in a run now presses once, matching the export side's post-restart behaviour since #4000. Called out in the commit message too, so it is not mistaken for an accident.

The commit memory is keyed by scope, not by side. Three scopes share the charge-side button, so keying by button would let a target-SoC commit mask a pending window commit. test_commit_scopes_are_independent pins it, and I verified it fails if keyed by button.

Merged main in (b8545c7) - it had moved 130 commits. INVERTER_QUICK_UPDATE_SECONDS dropping to 60 makes the quiet=True on the balance path matter more than when I scoped it. Full quick suite and pre-commit green throughout; CI green on the branch.

[springfall2008]

(Review by Claude, posted by @tdlj.)

Reviewed at 3a5da63e. The diagnosis is right and the core change is the right shape — fetch_inverter_data() defaulting to create=True did make every commit-once guard unreachable in production, and persisting the objects is the fix. The __init__ split into identity / refresh_config() / reset_cycle_state() is a real improvement, and the in_calibration and battery_voltage catches found along the way are genuine bugs.

Three things I'd want resolved before merge, then a set of smaller ones.

---

1. The commit guards can now suppress a press that is genuinely needed

This is the one I'd most like your view on, because commit_needed()'s docstring already argues against the obvious fix and I think it's arguing against a different proposal than the problem here.

adjust_charge_window() — inverter.py#L3427-L3435

charge_schedule = (new_start, new_end, new_start != new_end) is built entirely from Predbat's intent. The write block above it at L3339 is gated on new_start != old_start — an observed comparison. Those two can disagree, and when they do the registers get rewritten and the button never fires.

Confirmed by driving adjust_charge_window() against this branch — Solis-style inverter (inv_time_button_press=True, inv_has_charge_enable_time=True):

• Cycle 1: window 11:00–13:00, registers written, button pressed, last_committed["charge_window"] = ("11:00:00", "13:00:00", True).
• Cycle 3: the integration resets select.charge_start_time to 00:00:00 while minutes_now is inside the window, so in_new_window is True and disable_charge_window() is not called.
• Predbat sees old_start 00:00 ≠ new_start 11:00 and rewrites both time registers to 11:00/13:00 — but commit_needed() returns False, because the plan's intent hasn't moved. Button stays off.

The inverter keeps 00:00–00:00 until the plan happens to change the window on its own. main's (new_start != old_start) term did press here.

adjust_battery_target() — inverter.py#L2183-L2184

Same shape, and this one is self-concealing. The press is inside if current_soc != soc:, which is already the "observed state is wrong" gate — so by the time we reach the press we have positive evidence a commit is needed. Adding commit_key=soc on top can only subtract from that:

• Cycle 1: charge_limit 100 → 80, pressed, last_committed["target_soc"] = 80.
• Something external puts charge_limit back to 100 (firmware, the vendor app, a schedule reset).
• Cycle 3: current_soc(100) != soc(80), so we enter and write_and_poll_value puts 80 back — but press_and_poll_button(scope="target_soc", commit_key=80) sees an unchanged key and returns without pressing.
• The register now reads 80, so every later cycle skips the block entirely. On Fox/Solis the inverter never receives the 80% target.

On the docstring's counter-argument. L3450-L3455 says this is deliberately the whole decision rather than a term a caller ANDs with its own "did anything move" flag, because those flags are pinned true on H M formats and on non-reading hardware. That's correct, and I'm not proposing the AND — an AND can only suppress, which is the failure above. The gap is on a different axis: the key describes intent, so divergence introduced outside Predbat is structurally invisible to the guard, and there is no path back to a commit except a plan change.

The useful distinction is that "did the register actually move" is not the same signal as "did the plan's intent move", and it isn't pinned true on the #2328 hardware — that report's whole signature is every register logging No write needed. write_and_poll_value() (L2398-L2399) and write_and_poll_switch() (L2285-L2286) both already short-circuit and know they no-opped; write_and_poll_option() does not (it calls the service unconditionally), so it would need to learn to.

Two shapes that would both preserve your invariant, since each can only add presses:

• OR a "a write actually changed a value this cycle" signal into commit_needed(). Cheap, and exactly right on the reading hardware that Unneccessay updates to inverter causing eeprom wear #2328 was reported from — but on non-reading hardware every write looks real, so the spam comes back for that class.
• A bounded periodic re-commit: when a scope is believed committed, allow a re-press at most once every N minutes. That caps a stuck inverter at ~24 presses/day instead of 288, recovers from external drift on every hardware class, and doesn't depend on read-back working at all.

I'd lean to the second. If you'd rather accept the drift and keep the guard as-is, that's a defensible call — but it's worth saying so explicitly in the docstring, since right now it reads as though intent-keying has no cost.

2. refresh_config() is now on the 60s quick-update path

execute.py#L1173-L1176. quick_inverter_data_update() calls fetch_inverter_data(quiet=True), and with self.inverters populated that now reaches refresh_config() on every INVERTER_QUICK_UPDATE_SECONDS tick. On main the create=False path did nothing but update_status().

What that newly pulls into the 60s loop:

• check_clock_skew() → auto_restart(), which runs os.system(shell) / restarts services and self.sleep(15) — inside update_time_loop, 5× more often than the plan cycle.
• battery_size_tracking()'s two load_previous_value_from_ha() calls.
• the expose_config("set_reserve_min") write.

Compounding it, L509-L511 forces quiet = False for every non-GE inverter, so the caller's quiet=True is discarded. A Solis user gets the full Inverter 0 with soc_max ... block plus Inverter 0: Type GS Ginlong Solis / Note: Using inverter type GS / Reserve min: every 60 seconds — on the order of 7000 extra lines a day in the log people attach to bug reports.

Simplest fix is probably to split refresh_config() into the config re-read and the once-per-plan-cycle diagnostics, and have the quick path call only the former — or skip it entirely there, since nothing on the dashboard path needs a config refresh.

3. One path can still press every cycle — the bug this PR fixes

inverter.py#L3348-L3357. The new elif start_hour_id: / elif start_minute_id: guards only skip falsy ids. If apps.yaml holds a literal where an entity id is expected — the exact shape check_write_entity() exists for, GH#5003, e.g. charge_start_hour: 0 or charge_start_time left as a plain time string — the id is truthy, write_and_poll_option() → check_write_entity() returns False on every cycle, schedule_write_ok is False on every cycle, record_commit() sets commit_pending["charge_window"] = True on every cycle, and commit_needed() therefore returns True forever. The Solis update button is pressed every 5 minutes for the life of the window.

is_entity_id() is already imported at the top of the file and is the check those truthiness tests should be.

Related, at the same site: changing else: to elif start_minute_id: also drops the check_write_entity() warning for a half-mapped window. On main, an H M inverter with charge_start_hour configured but charge_start_minute missing logged Warn: Inverter 0 write_and_poll_option: No entity_id for charge_start_minute and record_status(had_errors=True) every cycle. Now the write is skipped silently, schedule_write_ok stays True, and record_commit("charge_window", key, True) caches the window as committed — so the user gets no diagnostic and Predbat believes a half-written window is applied.

---

Smaller

Test isolation (#5079). test_inverters_persist_across_cycles() (L4311) and test_set_current_from_power_before_battery_voltage_known() set args["inverter_type"] = ["GS"] against the shared my_predbat and restore only inverter_type, battery_min_soc and inverters. GS has has_charge_enable_time / has_target_soc / has_reserve_soc / has_idle_time all False, so refresh_config() runs create_missing_arg() / create_entity() for scheduled_charge_enable, charge_limit, reserve, inverter_mode, charge_start_time/charge_end_time, discharge_* and idle_*. create_missing_arg() replaces a string arg with [default] * 4 before writing the dummy id into [0], so the original mapping is destroyed — every later test in run_inverter_tests() then sees args["charge_start_time"] = "sensor.predbat_GS_0_charge_start_time". Worth snapshotting the whole of args (or at least the keys refresh_config() can create) in those finally blocks.

test_force_export_enable_only_flip_skips_settle_sleep() (L3125) snapshots and restores last_committed but not the new commit_pending dict. A failed press inside the body leaks commit_pending["export_window"] = True onto the shared inv, where commit_needed() then returns True regardless of key.

battery_voltage guard covers None but not 0 — inverter.py#L3084. An integration publishing 0, or a unit conversion producing 0.0, passes is None and then divides. if not self.battery_voltage: covers both.

Runtime type change can leave args on phantom sensors — execute.py#L1151-L1158. The comment's claim that refresh_config() absorbs a type change holds only when the new type needs at least as many dummy entities, because dummy args are created but never removed. An inverter built as GEC (has_idle_time False) gets args["idle_start_time"][0] = "sensor.predbat_GEC_0_idle_start_time"; if gecloud.py then sets the type to GEE (has_idle_time True), refresh_config() skips the idle block entirely and the args still point at the GEC dummy — Predbat reads and writes the idle window on a phantom entity. Same for scheduled_charge_enable / inverter_mode on a GS → GS_fb00 transition. main's rebuild-on-type-change didn't have this hole. Incidentally test_inverters_persist_across_cycles() goes GE → GS, which is the direction that doesn't expose it.

Docstring names the wrong caller and interval — execute.py#L1113-L1114 says "balance_inverters() passes quiet=True because it runs every 120s". The only quiet=True caller is quick_inverter_data_update() (L1359) and the throttle is INVERTER_QUICK_UPDATE_SECONDS = 60. The inline comment at L1352 says 120s too. Worth fixing given finding 2 is precisely about which paths reach refresh_config().

Inverter(self, id) ignores the caller's quiet flag — execute.py#L1167, while the next lines thread quiet into refresh_config() and update_status() correctly. Reachable on the first quick-update tick after startup, before the first plan run.

Dead code. self.inverter_time = None at inverter.py#L605 duplicates the new unconditional reset at L595 — a reader has to check both to know which is load-bearing. And self.inverter_type = "GE" on both test doubles (test_execute.py#L21, test_infra.py#L388) was needed by the same-count type-change rebuild that 1650a57b removed; fetch_inverter_data() never reads it now.

---

Inverter, execute, balance and ledger suites all pass on the branch.

[chalfontchubby]

(Written by Claude, posted by @chalfontchubby.)

Status, and it is not a good one: we have been round this eight times and have not landed on a stable refactoring. I would rather say that plainly than keep pushing commits at it.

Where it actually stands

Your review was right on all 13 points and they are all addressed. But working through them produced two further rounds of self-inflicted churn:

• The fix for finding 1 (intent-only keying) reintroduced Unneccessay updates to inverter causing eeprom wear #2328 by a different route — it recorded "a register moved" from deciding to write rather than from a write landing, so on hardware that never reads a value back it fired every cycle. charge_rate/discharge_rate are rewritten every cycle by design, so that was live.
• The fix for that held the signal as one flag per inverter when the question is per-scope. execute_plan() commits export, charge and target-SoC in the same cycle, so whichever committed first consumed the evidence for the rest.

Both are fixed with fail-first tests, but the pattern is the point: each round found something, and each round was found by review or by a test I only wrote because I was told to check my working rather than because I doubted myself. Several of the wrong turns were accompanied by confident comments asserting properties I had not tested. That is worse than no comment, because it discourages the next reader from checking.

I do not think "one more pass will be clean" is a claim I have earned today.

What has held up

The persistence premise itself. You were content with the direction and I still think it is right — none of the churn above is in persistence, it is all in the commit-guard work built on top. The two are entangled in the branch's first commit, which is a structural mistake on my part rather than a necessary one.

What it turned up along the way

This is the part I think is worth your time regardless of what happens to the branch:

Verified by execution. press_and_poll_button() reports failure only after pressing INVERTER_MAX_RETRY times — I measured one reported failure as 10 real button presses. Combined with commit_pending retrying every cycle (which main also does on the export side), a button that physically works but reports its last_updated too slowly — a laggy cloud integration, a clock-skewed host — is indistinguishable from a dead one, and gets pressed on the order of 2880 times a day. Each of those is a non-volatile write on the hardware this is all about. That measurement stands independently of anything in this branch.

Latent bugs the lifetime audit found, all real on a persisted object and all previously masked by the per-cycle rebuild: in_calibration was write-only (set True, never False); battery_voltage was seeded to a guessed 52.0 and used as a divisor; inverter_limit/export_limit never reverted when a config key was removed at runtime.

Not verified, so stating it as a reading rather than a finding: tracing the code suggests #4711's export-side commit guard has never actually suppressed anything in production — it lives on the Inverter, and fetch_inverter_data() defaulted to create=True, so the object was rebuilt before the guard could latch. That is consistent with the 377-presses-per-night report and with the unit tests passing (they reuse one long-lived Inverter). I have not executed this against main to prove it, so please treat it as a hypothesis worth ten minutes rather than a result.

What I would suggest

I do not want to push more code at this until there is agreement on scope. Options as I see them:

1. Split it: persistence plus the lifetime fixes as one change, the commit-guard consolidation as its own PR with its own review. Honest caveat — persistence alone does not close Unneccessay updates to inverter causing eeprom wear #2328, since the charge side has no guard in main at all. It would make fix(inverter): stop re-committing a stable export window every cycle (Solis) #4711's existing export guard functional, if the reading above is right.
2. Take the analysis and leave the patch. The findings above are independently checkable and you may well prefer to build the guard yourself.
3. Park it. Unneccessay updates to inverter causing eeprom wear #2328 has been open a long time and nobody is harmed by another week; a bad fix to inverter write paths can harm people immediately.

I lean to 1 or 2. Happy to do the work either way, but the call on scope should be yours rather than mine.

[chalfontchubby]

(Written by Claude, posted by @chalfontchubby.)

Following the status note above — one design point that I think is more important than anything still sitting in the branch, and which came out of Rik pushing back on the shape of this rather than on any particular bug.

The guard has been trying to make a misbehaving inverter look fine.

Silently re-asserting a schedule every N minutes, forever, is Predbat covering for hardware that is not doing what it was told. The user sees a plan that says "charging 02:00–05:00" and an inverter that is not charging. No error, no status, nothing in the log they would think to look for. The cost lands on them and the diagnosis is hidden. That is the worst combination available, and it is what the current design does by construction.

I had framed the open question as "what should COMMIT_RECHECK_MINUTES be", and treated it as unanswerable without hardware data. That was the wrong question. The number only looks load-bearing because the failure is silent — it is simultaneously deciding how long a user sits on a wrong schedule and how much flash wear is acceptable, with no third option. Once the condition is reported, it stops being a silent compromise and becomes "we have told you, and we are not going to hammer your inverter while you look at it."

Specifically, and this is a defect in what I have already written: a single failed button press does already record_status(had_errors=True) (_press_single_button_and_poll, and the toggle equivalent). But a sustained failure looks identical to a one-off blip — same warning, once per attempt, no escalation. And the retry backoff I added makes it quieter as it gets worse: from a warning every 5 minutes to one every 30. I made the signal fainter exactly as the problem became more serious. That is backwards, and a backoff without escalation is worse than no backoff.

Two things I think belong in whatever version of this lands:

1. Sustained commit failure should escalate, not fade. The state is already there — commit_failed_attempts counts consecutive failures per scope. After some threshold this is a fault worth surfacing distinctly from a transient one: this inverter is not accepting its schedule, here is which one, here is how long it has been going on.

2. Repeated external drift should be reported too. If the same register keeps being moved back by something outside Predbat, silently re-committing it is fighting an unseen opponent indefinitely. Once is normal — a reset, a reconnect. Every cycle for an hour means a second controller, a vendor schedule, or a firmware fault, and only the user can resolve it. Honest caveat: this one is not free with the current state. registers_moved is a set cleared on each successful commit, so it records that drift happened, not how often. Detecting a repeat needs a counter that is not there yet.

Neither is in the branch. I am not adding them unilaterally given where this PR has got to — flagging them because I think the "report loudly rather than paper over it" principle is more valuable to Predbat than this particular guard is, and it applies well beyond adjust_charge_window().

More generally, and with the self-criticism from the previous comment still standing: the right move some rounds ago was to stop patching and put up a flare, rather than keep trying to make it right after the point where the design question had outgrown the patch. That is on me.

[chalfontchubby]

(Written by Claude, posted by @chalfontchubby.)

One more on the guard, and this one is a question rather than a finding — it is the gap I am least able to answer from outside your design intent.

A cloud outage and a misbehaving inverter are currently indistinguishable to the guard. Writes fail, the button does not confirm, commit_pending goes true, retries escalate. Identical evidence, opposite correct responses:

• Cloud down: retry patiently, say nothing. Not the user's problem, will fix itself.
• Inverter not accepting its schedule: flare, because only the user can act.

Treat them the same and you get both wrong — cry wolf on every provider hiccup until people stop reading the warnings, or stay silent on a real fault while it quietly costs them a cheap-rate window.

This makes the escalation I proposed in the previous comment not safe as stated. It would fire on every cloud outage. It needs a way to tell the two apart or it should not go in.

Two defects in the branch that follow from the same gap:

The retry backoff delays recovery after an outage. A two-hour GE Cloud or FoxESS outage puts every scope into the backed-off state, so the first cycle after connectivity returns may not re-commit for up to COMMIT_RECHECK_MINUTES. On a cheap-rate window starting at 02:00 that is real money, and it is precisely the "does not commit until the plan happens to change" failure you caught in finding 1, arriving by a different route.

And commit_failed_attempts counts an outage and a hardware fault into the same number, so any threshold built on it inherits the ambiguity.

The question. ComponentBase already carries last_success_timestamp and health with exponential backoff, so the cloud components evidently know when they are down — the guard simply is not asking. Is that the right signal to key this on? Concretely:

• If the component serving an inverter is unhealthy, treat a failed commit as expected: retry, do not escalate, and clear the backoff when health returns so the first good cycle re-commits immediately rather than waiting out a timer.
• If the component is healthy and the write still fails, that is the inverter, and it earns the loud report.

What I have not verified, and would want to before building anything on it: how a cloud inverter's write path actually relates to its component's health state. Whether write_and_poll_* failing implies the component has already marked itself unhealthy, or whether the two are independent and can disagree. That is the load-bearing assumption and I am not going to assert it works.

If the answer is "yes, key it on component health", I think the guard gets simpler rather than more complex — the health signal replaces some of the guessing the four-term OR is currently doing, and the arbitrary constants stop having to cover the outage case at all. If the answer is "those are independent and cannot be relied on to agree", then I think loud reporting has to be more conservative than I suggested, and the backoff needs a different recovery trigger.

Either way it seems worth settling before more code goes on this branch.