fix: defer Growatt MIN TOU writes until a segment is about to take effect - #568
Conversation
Code reviewThe core design checks out — deferring TOU writes to a 45-minute horizon, reserving slots for planned-but-deferred segments, diffing disables against the full plan, and the Three findings. 1. HIGH —
|
Review findings on #568. The walk-back that finds a running segment's true start indexed past the end of the schedule on a spring-forward day: current_period is derived from the wall clock and reaches 95, while the day holds 92 periods. The raise escaped apply_intents, which _apply_schedule calls outside its try, so the whole optimization cycle aborted. Previously the loop's range was simply empty there. The hardware-eligible comparison compared a candidate built at the current period against a set committed one period earlier, so a segment merely expiring made the two differ. That fired the gate and cost a Growatt cloud read producing no writes — on the API this branch exists to relieve. Both sides are now filtered by the same cutoff. Per-interval deferral logging drops to DEBUG. Deferral is the normal state on every cycle, so at INFO it alone would add thousands of lines a day to the debug bundles this system is diagnosed from.
|
Fixed all three, each verified by reverting the fix and watching the test go red. 1. HIGH — DST spring-forward Test: 2. MEDIUM — gate firing on mere expiry. Confirmed. Both sides of Worth noting for the record: my first attempt at the test used a plan whose only segment expired, and it still failed — but on a different branch, the pre-existing Test: 3. LOW — deferral log volume. Per-interval lines moved to DEBUG, summary stays at INFO. Agreed on the reasoning: the Also confirmed your checks on the parts I could not have verified as cheaply: the Gate after the fixes: fast 1826 passed, slow 534 passed, black/ruff clean, 12/12 mutations killing their intended test. |
Code reviewReviewed the full diff ( Verification performed
Findings1.
Concretely: at 08:14 a segment starting 08:59 is deferred by the write path ( Fix: floor to the period ( 2. The walk-back that recovers a running segment's true start reads Self-healing from the next cycle on, and Checked and cleanThe |
Review findings on #568. get_all_tou_segments measured the write horizon from the raw wall clock while _select_hardware_intervals measured it from the period floor, so a segment inside the band between them was deferred by the write path and called pending by the display path — the amber "Pending Write" badge this branch exists to clear. Only reachable for a segment whose start is off the 15-minute grid, i.e. read back from hardware rather than produced by the optimizer, since _period_to_time only yields :00/:15/:30/:45. Also documents that the walk-back recovering a running segment's true start is defeated on the first cycle after a restart, when no previous schedule exists to carry past intents forward and they initialise to IDLE. Costs one disable plus one rewrite, then self-heals. Reconstructing the missing past from actuals is deliberately not done — inferring intent from measured export feeds back into the plan. The retry-budget comment said three attempts where the reviewer counted four; both were right under different readings, so it now states the count explicitly: eligible on four cycles, three landing strictly before the segment takes effect.
|
Both fixed. One correction on the reachability of #1, and one on the retry count. 1 — display and write paths disagree on the horizon. Real, and fixed by flooring the wall-clock branch of The specific example does not hold, though: a segment starting at 08:59 cannot be produced by the optimizer —
For a start on the grid the band closes exactly: the disagreement window is Test: 2 — walk-back defeated on the first cycle after a restart. Confirmed at Documented rather than fixed, deliberately. The only way to recover the true past is to infer it from actuals, which the carry-forward block immediately above it exists to prevent ("export → inferred BATTERY_EXPORT → grid_first → more export"). Trading a documented one-cycle rewrite for a feedback loop would be a bad deal. The comment now states the cost (one disable plus one rewrite), why it self-heals from the next cycle, and why the obvious fix is not taken. On the retry budget — we were both right, which means the comment was ambiguous. Eligible cycles for a segment starting 12:00 are 11:15, 11:30, 11:45, 12:00: four in total, three landing strictly before it takes effect. Your four counts the cycle that fires as the segment starts. The comment and Thanks for the independent full-day simulation and the reclaim arithmetic — the Gate: fast 1864 passed, slow 538 passed, black/ruff clean, 13/13 mutations killing their intended test. Branch merged up to current main (your remote merge of main is included — I merged it in rather than force-pushing over it). |
…fect Every re-optimization cycle pushed the whole upcoming TOU plan to the inverter. A marginal 15-minute period crossing the economic boundary back and forth rewrote a segment hours before it started -- 11 rewrites of one 13:45 window in a single day, ending on the value written at 08:16. A segment has no effect until it starts, so the write can wait. _select_hardware_intervals already owned "which intervals reach hardware now, rest deferred to a later cycle" for the 9-slot cascade; the write horizon is the same idea keyed on time, so it goes there rather than into a new mechanism. Three things had to follow, because "eligible to write" and "in the plan" stop being the same set once writes are deferred: - evaluate_intents decides whether sync_to_hardware runs at all, and compared only the whole daily plan. It already computed the hardware-eligible set and discarded it. A segment deferred at 08:15 would never be written once the plan stabilised. Both sides of that comparison are filtered by the same cutoff, so a segment merely expiring cannot fire the gate and spend a cloud read for no writes. - The disable side must diff against the full plan, not the eligible subset, or a correctly-programmed future segment reads as unplanned and gets cleared, then rewritten when it comes back into range. - Slot assignment must reserve slots for planned-but-deferred segments, or the next imminent write lands on top of one and destroys it with no disable ever issued. When the table is full the furthest-out segment yields its slot, having the most cycles left to be rewritten. Separately, a segment that was already running got rewritten every cycle: the plan is rebuilt from the current period, which truncated an in-progress segment's start_time forward to "now", renaming it every 15 minutes. _group_periods_by_mode now reports the group covering the current period from its true start, guarded against a short DST day where the wall-clock period outruns the schedule. pending_write is narrowed to mean "due but missing" rather than "not on hardware", and the display path measures the horizon from the same period floor the write path uses, so the dashboard does not paint correctly-deferred segments amber. Measured end to end against mock-HA, four separated windows over 96 real optimization cycles: 26 hardware writes before, 13 after. A stable two-hour window costs 2 writes rather than 16. The issue's 13:45 window takes 1 write where production did 11.
33aca3f to
1f92ac0
Compare
Review findings on #568. Deferral removed the last periodic reconciliation with hardware. The write gate compares plan against plan, so once a segment is written and the plan stops changing, nothing looks at the inverter again. Before this branch the in-progress segment was renamed every cycle, which forced a read and repaired drift by accident; that churn is exactly what this branch removes, and the self-healing went with it. Reproduced: an inverter table cleared at 03:30 during an unchanged 03:00-04:59 window was never restored, the window ran load_first for its remainder, and nothing appeared in the logs. Issue #551 established that this table does drift. needs_hardware_reconciliation is a no-op on the base class — a platform that rewrites its whole control state every cycle cannot drift undetected — and implemented for Growatt MIN, which skips the write while the plan holds steady. It compares against the hardware-eligible set, not the whole plan, so a deliberately deferred segment is not mistaken for drift and does not rewrite every cycle. This also gives the extra eligible cycles something to do: a write that raises was already retried via _hardware_write_pending, but one that vanished without raising was not retried at all. The horizon comment claimed attempts that never happened and now describes the two paths that exist. The restart test fed past periods the intent they were planned with, which a restart cannot do — with no previous schedule those arrive as IDLE. It now models that, and asserts the property that matters: the period the restart landed inside is covered.
|
All three fixed. Finding 1 was the important one — you're right that it's a regression this branch introduced, and I reproduced it before believing it. 1 — deferral removed the last periodic reconciliation. Confirmed exactly as described. Side by side, table wiped at 03:30 during an unchanged 03:00–04:59 window:
So Fixed with Same scenario after the fix — and it now restores the window with its true start, where main could only restore the truncated remainder:
Two hardware calls for the whole day, against eight firing cycles on main. 2 — the retry budget didn't exist. Correct, and it was my error: 3 — the restart test didn't model a restart. Correct. Gate: fast 1866, slow 538, ruff/black clean, 14/14 mutations killing their intended test (two new ones for reconciliation). Merged up to current main. |
CI caught this: the Growatt VPP E2E scenario died with "Growatt device_id not configured", taking the whole schedule update with it. SolaxModbusGrowattController subclasses GrowattMinController and overrides every method that would otherwise reach the growatt_server cloud services. needs_hardware_reconciliation was not one of them, so it inherited a read this modbus platform has no device_id to address. It now returns False explicitly, and says why: TOU mode rewrites its single segment whenever it differs and VPP mode issues per-period commands, so neither has a window in which drift could go unnoticed. The check also ran outside _apply_schedule's error handling, so any read failure ended the cycle rather than degrading it — an inverter that is unconfigured or unreachable would stop the optimization, which needs no inverter at all. A failed check now logs and proceeds; the real fault still surfaces through the write path and the health checks. Verified against the ci-growatt-vpp scenario locally, which is what CI asserts on: zero occurrences of "Failed to update battery schedule", schedule cycles running, gate deciding normally.
johanzander
left a comment
johanzander
left a comment
There was a problem hiding this comment.
Code review of the real PR scope (merge-base(origin/main, head)...head). The core deferral mechanics hold up: new_tou ⊆ planned_tou under the same effective_period, the reclaim path is self-healing, the len(planned_tou) == 0 clear-all keying is correct (keying on new_tou would have wiped the table whenever everything was deferred), the start_period >= len(intents) walk-back guard is genuinely required by the new mode_at() indexing, and the carry-forward of past strategic intents it depends on exists (battery_system_manager.py:2298).
Four findings, all in the new needs_hardware_reconciliation gate rather than the deferral logic itself — 2 medium, 2 low, left inline.
One non-finding: the RuntimeError in _assign_hardware_slots is now unreachable (k + n <= 9 from the max_intervals cap makes len(reclaimable) >= shortfall always). Harmless retained invariant guard, not a bug.
| and self._time_to_minutes(s["end_time"]) >= effective_minute | ||
| } | ||
|
|
||
| missing = live(self.active_tou_intervals) - live( |
There was a problem hiding this comment.
medium — orphan segments are never detected.
needs_hardware_reconciliation computes live(active_tou_intervals) - live(hardware), so it only detects missing segments, never extra ones. evaluate_intents is plan-vs-plan, and sync_to_hardware — the only place the disable side runs — now fires far less often by design.
Concrete: the inverter restores a stale slot 5 holding 18:00-20:00 grid_first that the plan does not contain. The plan is stable, so no sync is triggered, and the battery exports for two hours against the plan.
The function's own docstring cites #551 as evidence the inverter alters its table unprompted — this is the half of that failure mode left uncovered, and the reduced write frequency makes the exposure window longer than before this PR. The orphan set is cheap to compute from the planned_tou the disable side already builds.
| } | ||
|
|
||
| missing = live(self.active_tou_intervals) - live( | ||
| self._read_segments_from_hardware(controller) |
There was a problem hiding this comment.
medium — no attempt cap or backoff on a persistent mismatch.
Any persistent readback mismatch becomes a write every optimization cycle for the rest of the segment's life.
Concrete trigger: _normalize_segments (line 1329) defaults enabled to False when the vendor payload omits the key. live(hardware) is then empty, every programmed segment reads as missing, and the controller rewrites the whole eligible set to the Growatt cloud every 15 minutes — strictly worse than the pre-PR behaviour, and the exact API-load condition #554 exists to reduce.
Same loop for any firmware that echoes a normalized end_time (e.g. 14:45 for our 14:44), since live() compares the time strings verbatim.
| # write path below, which records it as a pending write, and | ||
| # through the health checks — whereas raising here would kill | ||
| # the optimization itself, which needs no inverter at all. | ||
| logger.warning("Could not check the inverter for drift: %s", e) |
There was a problem hiding this comment.
low — the swallow comment's reasoning does not hold.
It justifies itself with "surfaces through the write path below, which records it as a pending write", but that write path is only reached when evaluate_intents reports a difference. On a stable plan it never runs.
So an unconfigured growatt_device_id (_read_segments_from_hardware raises SystemConfigurationError) or an unreachable cloud yields a logger.warning every cycle and nothing else — _hardware_write_pending stays False. The behaviour is safe (health checks cover the user-visible part), but the stated reasoning is wrong and it produces ~96 warnings/day on an unconfigured install.
| if self._controller is not None and not prepare_next_day: | ||
| try: | ||
| drifted, drift_reason = ( | ||
| self._inverter_controller.needs_hardware_reconciliation( |
There was a problem hiding this comment.
low — duplicate cloud read per cycle.
The drift check adds an unconditional read_time_segments cloud call on every optimization cycle (~96/day), and on cycles that do write, sync_to_hardware performs a second independent read at growatt_min_controller.py:1367.
Given #554's own premise that Growatt 500s on this endpoint under load, reads are worth counting alongside writes — the fresh read could be threaded through to the sync rather than repeated.
Review findings on #568. The drift check only computed missing = committed - hardware, so it never saw the other half of #551: a slot the inverter restores that the plan does not contain. With writes now deferred, the cycle that would have disabled it may not run for hours, and the battery follows a window nobody planned. It also read the inverter on its own, so a cycle that went on to write read the table twice — on the endpoint this change exists to relieve, which #554 reports 500ing under load. Both come from the check duplicating what sync_to_hardware already does properly: one read, diffed in both directions, deferral applied. Reconciliation is now that call. A quiet cycle writes nothing, an orphan is disabled, a lost segment is rewritten, and there is one read. This also puts the write where a write belongs. The check ran inside _should_apply_schedule — a predicate — and swallowed its own errors with a comment claiming they would surface through the write path, which is only reached when the plan differs. Reconciliation now runs in the not-applying branch of the caller and records a failure the same way a failed apply does, so the claim is true: _hardware_write_pending is set and the next cycle retries. Reported separately: a persistent readback mismatch (a vendor payload omitting `enabled`, or echoing a normalized end_time) would rewrite the eligible set every cycle. That is unchanged from before this branch, where sync ran on nearly every cycle for the same effect, so it is not a regression introduced here — but it is real, and worth its own issue rather than a cap bolted on late in this one.
|
All four addressed — and following the first one showed the drift check should not have existed at all. It is deleted; reconciliation is now a straight call to 1 — orphans never detected (medium). Right, and you pointed at the fix: the orphan set is cheap to compute from the Your scenario — stale slot 5 holding 4 — duplicate cloud read (low). Fixed by the same change: one read per cycle, and it is the one the writes are diffed against, rather than a check-read plus a sync-read. Pinned by 3 — the swallow comment was wrong (low). Correct: the write path is only reached when Test 2 — no cap or backoff on a persistent mismatch (medium). Confirmed as a real trigger, and the I have not added a cap, deliberately. Under the new shape a persistent mismatch rewrites the eligible set every cycle — which is what Gate: fast 1922, slow 538, ruff/black clean, 12 mutations / 21 kills, none surviving. |
Both tests drive update_battery_schedule(current_period=10) while the rest of
the system reads the real clock. Before 02:30 local, period 9 is still in the
future, so data collection raises ("Period 9 is still in progress or in the
future") and the cycle aborts long before reaching reconcile_hardware. The
assertion then fails for a reason unrelated to reconciliation.
The window is real, not theoretical: these fail every day between local
midnight and 02:30. That is why they passed in CI on the way in with #568
(21:49 UTC = 23:49 local) and failed on the very next PR (22:30 UTC = 00:30
local) -- and why they reproduce locally right now, on a clean origin/main
checkout as well as on this branch.
Pins the time of day to 15:00 while keeping today's date, matching how the
neighbouring lifecycle tests in this file already control time. Verified by
running both test bodies unchanged under a pinned clock: they pass, while the
originals fail at the same moment.
Unrelated to this branch's own change; fixed here because it blocks its CI.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8Kyf8tver2jbB93ymB5XM
…ry polarity (#542 follow-up) (#591) * fix: split signed power in sensor diagnostics, raise on unknown battery polarity Follow-up to #542 / PR #560, clearing items 2 and 3 of that PR's review. get_method_sensor_info reads /api/states/{entity_id} directly instead of going through the getters, so on a platform whose charge and discharge (or import and export) keys resolve to ONE signed entity it assigned the same raw signed state to current_value for both directional rows -- -800 for both "Battery Charging Power" and "Battery Discharging Power" on a native SolaX discharging at 800 W. It now routes that field through _signed_split_state(), which reuses the getters' own _is_shared_signed_* predicates rather than re-deriving polarity at the display site. Applied to the grid pairing as well as the battery one: the defect is identical there and fixing one copy would leave a known duplicate. This is latent, not user-visible. The health panel was never affected -- perform_health_check calls the getter for rawValue/displayValue, so it has always rendered 0 W / 800 W, as docs/SOFTWARE_DESIGN.md already stated. No consumer reads current_value today. TODO.md carries the correction to its own earlier claim rather than a silent deletion, because the distinction is what a future reader of that list needs. The battery split also moves into _split_signed_battery_power(), which branches on battery_power_polarity explicitly and raises ValueError on anything but charge_positive, instead of hardcoding max(0.0, +/-raw) behind a comment. A typo'd entry in PLATFORM_BATTERY_POWER_POLARITY would otherwise have silently inverted every battery reading. Valid configurations are unchanged. The raise is caught separately inside get_method_sensor_info so a configuration fault is not reported as a connectivity error, which would hide the very failure it exists to surface. The grid helper stays deliberately lax, matching its prior behaviour. No CHANGELOG entry: the #542 feature these defects live in is itself still under [Unreleased], so this is pre-release iteration, not a user-facing fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8Kyf8tver2jbB93ymB5XM * fix: pin the clock in TestQuietCycleReconcilesHardware Both tests drive update_battery_schedule(current_period=10) while the rest of the system reads the real clock. Before 02:30 local, period 9 is still in the future, so data collection raises ("Period 9 is still in progress or in the future") and the cycle aborts long before reaching reconcile_hardware. The assertion then fails for a reason unrelated to reconciliation. The window is real, not theoretical: these fail every day between local midnight and 02:30. That is why they passed in CI on the way in with #568 (21:49 UTC = 23:49 local) and failed on the very next PR (22:30 UTC = 00:30 local) -- and why they reproduce locally right now, on a clean origin/main checkout as well as on this branch. Pins the time of day to 15:00 while keeping today's date, matching how the neighbouring lifecycle tests in this file already control time. Verified by running both test bodies unchanged under a pinned clock: they pass, while the originals fail at the same moment. Unrelated to this branch's own change; fixed here because it blocks its CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8Kyf8tver2jbB93ymB5XM --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
WRITE_HORIZON_MINUTES(45). A segment hours away is no longer rewritten every time the plan shifts around a marginal period.Root cause
sync_to_hardwarehad no notion of when a segment starts. It wrote any segment whose content differed from the fresh hardware read, filtered only byend_minute(segment) >= effective_minute— "hasn't ended yet" — with no upper bound. So a marginal boundary flip five hours out failed the content match and was written, every cycle.The reporter's worked example: the 13:45
battery_firstwindow rewritten 11 times between 08:16 and 13:45, ending on the value written at 08:16.Fix
_select_hardware_intervalsalready owned "which intervals reach hardware now, rest deferred to a later cycle" for the 9-slot cascade. The write horizon is that same idea keyed on time, so it goes there rather than into a new mechanism. Already-active intervals havestart <= now, so they can never be deferred, and a restart pushes the imminent plan immediately as an emergent property.Three things had to follow, because "eligible to write" and "in the plan" stop being the same set once writes are deferred:
evaluate_intentsdecides whethersync_to_hardwareruns at all, and compared only the whole daily plan — while already computing the hardware-eligible set and discarding it (candidate_intervals, _active = ...). Without this, a segment deferred at 08:15 is never written once the plan stabilises. This also fixes the same latent gap for the 9-slot cascade, where a pending interval could never be promoted on a stable plan.Separately,
_group_periods_by_modenow reports the group covering the current period from its true start. The plan is rebuilt from the current period each cycle, which truncated an in-progress segment'sstart_timeforward to "now", renaming it every 15 minutes — 16 writes for a stable two-hour window.pending_writeis narrowed to mean "due but missing" rather than "not on hardware", so the dashboard does not paint most of the day amberPending Write. Backend-only; the frontend is untouched.Scope assessment
Local. Everything is inside
GrowattMinController, within the target methods' existing contracts. No new class, no new owner. The diff adds no parameter, flag, or extra trigger whose job is to route around a problem —_assign_hardware_slotsgainsplanned_toubecause slot reservation genuinely needs to know what the plan still wants, which is the same information the disable side now uses.Consequence worth noting: at a 45-minute horizon at most
floor(45/15) + 1 = 4intervals are ever eligible (verified against a worst case alternating mode every 15 minutes all day), so the 9-slot cap can no longer bind end to end. The cap is kept and still enforced as the inverter's hard contract; its guard is now exercised directly on_select_hardware_intervalsrather than through a plan that can no longer reach it.Test plan
pytest -m "not slow"— 1824 passed, on the tree merged withorigin/mainpytest -m slow— 534 passed, same treeblack / ruff clean; mypy byte-identical to the
origin/mainbaseline for this file (19 pre-existing errors, none added)Observed through the real HA API layer against the mock-HA container (
docker-compose.ci.yml), driving the issue's own 13:45 window across four cycles withHomeAssistantAPIController→growatt_server/update_time_segment:update_time_segmentFull-day in-process run (96 real optimization cycles, real DP →
BatterySystemManager→ controller), four separated windows: 26 writes before, 13 afterEvidence the test discriminates
Ten mutations, each reverting one behaviour; every one reddens its intended test. Source verified restored intact after each.
WRITE_HORIZON_MINUTES→ 24h (deferral off)evaluate_intentseligible-set check →if False and …[: self.max_intervals]Two of these caught genuinely vacuous tests during development rather than after: the reclaim test initially used nine contiguous hours, which merge into one interval, so the reclaim path never fired and the mutation survived.
Outcome-level coverage
Asserted on the inverter's segment table — what is programmed, and when — via the
_SimulatingControllerstub that models the 9 hardware slots, not on command arguments.A plan-faithfulness (
R == P) scenario is deliberately not the primary test here: the diff touches only the TOU hardware-write layer, and deferral is designed to have exactly zero effect on battery behaviour, soR == Pis unchanged by construction and would prove nothing.inverter_simulatorhas no notion of segment writes. The correct outcome for this layer is "what the segment table holds at the moment each window becomes active", which is what these tests pin.Notes for review
Two things found during review and deliberately left alone, both pre-existing and out of scope:
load_first— normal self-consumption, a lost optimization opportunity rather than unsafe behaviour.docs/SOFTWARE_DESIGN.md's "Schedule generation" list is updated — it documented this exact mechanism.bess-knowledge.mdmentions nothing this diff touches.Closes #554