fix: decide the Solis TOU bit from the firmware, not from a read-back - #4783
Conversation
A single mismatched CID 636 verify read latched "this inverter will not accept the time-of-use bit" for 8 hours. On the inverters in #4774 that mismatch is transient - the same write of 179 verifies minutes either side of one that reads back 177 - so one post-write read cannot establish a firmware property. Latching it was silent rather than noisy: the verify read refreshes cached_values, so after a stripped write the cache held the stripped value, the suppressed computed value matched it, and set_storage_mode_if_needed() stopped writing CID 636 at all for the whole verdict, across an overnight charge window. Gate the bit on is_tou_v2_mode() instead - never asked for on TOU V2, where the timed enable moved to the per-slot registers, always asked for and retried on V1, where bit 1 still is the timed enable. Also two robustness changes around the same failure: - read_and_write_cid() re-reads once after a settle delay before calling a mismatch a failure, so a read served from a snapshot that predates the write is not reported as a refused register. - The slot registers are re-read every 5 minutes while a window is in force, rather than only once an hour, so drift during the window that matters is seen and corrected in the same cycle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The hourly slot-register poll unconditionally marks an inverter as “polled”, which can incorrectly suppress the in-window re-read when the hourly poll fails and the slot registers were not actually refreshed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Solis control loop to avoid “learning” a TOU-bit refusal from a single transient verify read-back, and instead decides whether CID 636 should carry the TOU bit based on detected firmware mode (TOU V1 vs TOU V2). It also adds a settle re-read to reduce false verification failures and increases slot-register polling cadence while a charge/discharge window is active to detect and correct drift during the window.
Changes:
- Decide whether to include the CID 636 TOU bit via
is_tou_v2_mode()(firmware-driven), removing the previous latch/re-probe mechanism. - Harden
read_and_write_cid()by adding a single settle re-read after a verify mismatch, using sharedcid_value_matches()logic. - Re-poll slot registers every 5 minutes while an active window is in force, and centralize window inclusion logic via
time_in_window()/is_inside_active_window().
File summaries
| File | Description |
|---|---|
| apps/predbat/solis.py | Removes TOU-bit refusal learning, adds verify settle re-read, adds active-window slot re-polling, and shares window/time comparison helpers. |
| apps/predbat/tests/test_solis.py | Adds new unit tests covering TOU V1/V2 behavior, settle re-read verification, and active-window slot re-polling. |
| .claude/skills/issue-triage/references/debug-journal.md | Updates Solis debugging notes to reflect the new firmware-driven TOU-bit decision and settle re-read behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The V1 control path picks its mode from in_charge_slot/in_discharge_slot, which are clock tests, so CID 636 changes value and is written at the moment a window opens. The V2 path picks from slot1_active - whether slot 1 has a window configured at all - so the mode is written when the slot is programmed and nothing happens at the window boundary. On the night in #4774 the last CID 636 write was 22 minutes before the window opened; the comparison night that charged normally had one near the window. Gating the TOU bit on the firmware removes the failing write that used to happen every cycle, so without this V2 can now go from slot-programming time to the end of the window with no mode write at all. active_window_key() names the window in force, including its times so a window rewritten mid-flight counts as a new one, and claim_window_mode_assertion() lets the mode be asserted once per window even when the computed value already equals the cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
poll_inverter_data() returns False only when the read raised, and it preserves the old cache in that case, so nothing was refreshed. Marking the inverter as polled regardless meant a failed hourly poll suppressed the in-window re-read at the same second boundary, leaving stale slot data in place for another 5 minutes during a live window - the one period that read exists for. Raised by Copilot on PR #4783. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Draft PR for issue #4774 — please review before merging.
Fixes #4774
Summary
Three related changes to the Solis control path.
1. Decide the TOU bit from the firmware, not from a read-back
note_tou_bit_refused()latched "this inverter will not accept the time-of-use bit on CID 636" from a single mismatched read-back taken about half a second after the write, and held it for 8 hours. On the six inverters in #4774 that mismatch is transient — the same write of 179 verifies successfully minutes either side of one that reads back 177 — so one post-write read cannot establish a firmware property.Latching it was silent rather than noisy. The verify read refreshes
cached_values, so after a stripped write the cache held the stripped value; with the bit suppressed the computed value matched it, andset_storage_mode_if_needed()stopped writing CID 636 altogether for the whole verdict — across an overnight charge window.The bit is now decided from
is_tou_v2_mode(): never asked for on TOU V2, where the timed charge/discharge enable moved into the per-slot registers (CIDs 5916/5922); always asked for and retried on V1, where bit 1 still is the timed enable.SOLIS_TOU_BIT_REPROBE_HOURS,tou_bit_refused,is_tou_bit_refused()andnote_tou_bit_refused()are removed.2. Assert the storage mode once when a window opens
Follows from the maintainer's correction on the issue: what differs between the failing night and the working one is whether CID 636 was written at all near the window.
The two control paths choose the mode differently. V1 picks from
in_charge_slot/in_discharge_slot, which are clock tests, so the value changes and CID 636 is written the moment a window opens. V2 picks fromslot1_active, which is only "slot 1 has a window configured" — Predbat's intent, not the time — so the mode is written when the slot is programmed and nothing happens at the window boundary. On the reported night the last CID 636 write was 22 minutes before the window opened.Change 1 makes that worse on its own: it removes the failing write that previously happened every cycle, so a V2 inverter can now go from slot-programming time to the end of the window with no mode write at all.
active_window_key()names the window in force — including its times, so a window rewritten mid-flight (the 00:00-01:30 rewrite that did charge) counts as a new window — andclaim_window_mode_assertion()lets the mode be asserted once per window even when the computed value already equals the cache. One write per window per inverter.This does not assert that the inverter needs a mode write to arm its schedule; it removes V2 being the only path that sets the mode on intent rather than on the clock.
3. Two robustness changes
read_and_write_cid()re-reads once afterverify_settle_seconds(default 5) before calling a mismatch a failure, so a read served from a snapshot that predates the write is not reported as a refused register. A write that still disagrees after the settle read is reported as a failure exactly as before, so thecontrol_successhealth signal from Solis: discharge slot cut-off SoC write silently rejected — PredBat always targets 1% below the inverter's Recovery SoC floor #4702 is unaffected. The value comparison moved into a sharedcid_value_matches()helper so both reads judge identically.write_time_windows_if_changed()and corrected in the same cycle. One batch call on V2, CID 103 on V1, and skipped for any inverter the hourly poll has already read this cycle. The window comparison moved into a module-leveltime_in_window(), which the V1in_charge_slot/in_discharge_slotpath now shares so the two cannot drift apart on the midnight-wrap case.The "charge window did not start" half of #4774 is not claimed as fixed. Change 2 restores the write the maintainer's n=2 comparison points at, and change 3 should make the next occurrence visible, but the inverter-side mechanism is not established.
Testing
cd coverage && ./run_pre_commit— all hooks pass, full quick suite passestools/triage_test.sh solis— passesTen new unit tests in
apps/predbat/tests/test_solis.py, each watched failing before the change:test_storage_mode_drops_the_tou_bit_on_tou_v2_firmwaretest_storage_mode_keeps_asking_for_the_tou_bit_on_tou_v1_firmwaretest_a_transient_tou_bit_strip_is_recovered_on_the_next_cycletest_a_lagging_verify_read_is_settled_before_it_is_called_a_failuretest_a_write_that_never_verifies_is_still_a_failuretest_is_inside_active_windowtest_slot_registers_are_re_read_while_a_window_is_livetest_active_window_key_identifies_the_window_in_forcetest_the_storage_mode_is_re_asserted_when_a_window_openstest_a_window_rewritten_mid_flight_gets_its_own_mode_assertionThe four obsolete re-probe/latch tests are removed;
test_storage_mode_does_not_learn_from_an_unrelated_verify_failureis kept astest_storage_mode_retries_a_wholesale_write_refusal.Notes
debug-journal.md's Solis entry describednote_tou_bit_refused()as the answer, so it is rewritten rather than appended to. It now records why read-back learning cannot work, the V1/V2 mode-selection asymmetry behind change 2, and the cache-mirror invariant:write_cid()caches the requested value and the post-writeread_cid()deliberately overwrites it with what the inverter reports, so suppressing that update on a failed verify would leave the cache agreeing with a write that just failed and change detection would never retry it.