fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX#440
Open
nanomad wants to merge 2 commits into
Open
fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX#440nanomad wants to merge 2 commits into
nanomad wants to merge 2 commits into
Conversation
4 tasks
nanomad
added a commit
that referenced
this pull request
May 9, 2026
) * fix: persist HA-driven gateway settings via retained /set commands Adds retain=true to HA discovery for the four refresh_period_* numbers, refresh_mode, and totalBatteryCapacity, so the broker keeps the user's last `/set` value across gateway restarts. Plumbs the gmqtt retain flag through the command-dispatch path and drops a retained one-shot refresh mode (force, charging_detection) so a single-shot poll does not loop on every restart. Also suppresses clear_command on retained replays so the broker does not erase the user's intent. Unblocks PR #440 (fix/ha-number-optimistic-state). * fix: log dropped retained replays at WARN, not INFO A retained /set arriving for an unknown VIN means we lost the user's intent — surface it at WARN so it shows up in default log views. * fix: preserve retained OFF refresh mode across restarts Drops RefreshMode.OFF from INVALID_STARTUP_REFRESH_MODES and changes the constructor default for refresh_mode from OFF to PERIODIC. Before, OFF was in the INVALID set because OFF was the constructor default and a gateway booting in OFF would never poll. Now that retained `/set off` is replayed on reconnect (via the parent commit), OFF is a legitimate persistent user choice and must be preserved by configure_missing. FORCE / CHARGING_DETECTION remain in INVALID_STARTUP_REFRESH_MODES as a belt-and-braces guard alongside the primary drop in RefreshModeCommand. Also addresses two review nits: - refresh_mode.handle empty-payload guard now mirrors the parent's `not self.supports_empty_payload` clause for contract parity - drops a redundant `if _properties` defensive check; gmqtt always populates the properties dict with the retain flag * fix: drop retained replays for non-replayable commands at dispatcher Adds an opt-in CommandHandlerBase.is_replayable_when_retained() classmethod (default False) and gates the dispatcher: any retained `/set` for a handler that hasn't opted in is dropped with a WARN log before reaching the handler. Defense-in-depth against non-HA producers (node-RED, custom scripts, mosquitto_pub) that may mistakenly publish action-bearing commands with retain=true. Without this guard such a stale retained command (e.g. a retained `charging/set true`) would re-fire the SAIC API call on every gateway restart. Opted in: RefreshMode, the four RefreshPeriod_*, and TotalBatteryCapacity — exactly the six entities whose HA discovery payload also declares retain=true. Single source of truth lives next to the handler logic.
8bfc57f to
bde1bd7
Compare
dd1e975 to
d0607aa
Compare
91d9145 to
6f8ada2
Compare
HA MQTT Number / Select / Text discovery defaults to optimistic mode, so HA treats its locally cached value as authoritative even when the gateway publishes a different state on state_topic — the trigger for target SoC resetting on restart (#375). PR #441 narrowed the symptom for the six retained entities by replaying the user's last /set on reconnect; this PR closes the underlying optimistic-mode gap. Discovery: add `optimistic: false` to _publish_number, _publish_select, and _publish_text. Switches already had it. Dispatcher: with optimistic: false the slider waits for state_topic to confirm before updating, which on a sleeping car can take minutes for SAIC-API-backed values. Add three optional hooks on CommandHandlerBase (echo_state_topic / capture_current_state / echo_payload) and an eager-echo + rollback path in the dispatcher: echo the requested value to state_topic on receipt, roll back on SAIC failure. Skipped on retained replays — no user is watching the slider on startup. Only DrivetrainSoCTargetCommand opts in: it's the one writable entity that is API-backed (so #441's retained-replay path doesn't cover it) and slow enough that the slider would otherwise freeze. Closes #375
6f8ada2 to
23c3101
Compare
…NOTHING rollback Two correctness bugs in the eager-echo flow plus a few smaller items caught in review. drivetrain_battery_heating_schedule.py: the handler mutated in-memory state via update_scheduled_battery_heating(...) before the SAIC API call. On API failure the broker rolled back via the dispatcher's __publish_state, but the gateway still held the failed-new value, so the next eager echo's `current_state` would be wrong. Restructure to compare prior vs. requested first, call the API, and only mutate state on success. The no-change branch deliberately drops the redundant canonical-schedule republish (the eager echo already stamped it). vehicle_command.py: handlers that return RESULT_DO_NOTHING after we have already published the eager echo (e.g. SoC handler when the vehicle does not support target SoC, or charging-schedule's UNTIL_CONFIGURED_SOC guard) used to leave the broker holding the requested-but-unapplied value, with the dispatcher reporting "Success". __run_handler_and_report_success now accepts eager_echo_published + rollback_state and rolls back on RESULT_DO_NOTHING; both call sites (success path and logout-retry) thread these through. Nits: add @OverRide on DrivetrainChargeCurrentLimitCommand.topic(); replace a brittle private-mangled-name reference in the CommandHandlerBase docstring with a stable file/class anchor. Tests: drop the update_scheduled_battery_heating mock from the existing battery-heating test and assert it was *not* called on API failure (the mock previously hid the ordering bug). New TestEagerStateEdgeCases covers RESULT_DO_NOTHING rollback, logout retry success preserving the eager echo, broker-failure during rollback (asserts both publishes were attempted and the failure result still made it out), and malformed schedule JSON skipping the eager echo entirely.
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
HA's MQTT Number entity defaulted to optimistic mode in our discovery payload, so on restart HA treats its locally cached value as authoritative and republishes it to the command topic — the gateway then forwards it to the vehicle. That's how target SoC ends up reset to the user's last cached HA value (#375), and the same mechanism explains the analogous battery-capacity report. `_publish_select` and `_publish_text` had the same gap.
Changes
Discovery (structural fix)
Dispatcher (UX + correctness)
With `optimistic: false`, HA waits for `state_topic` to confirm before updating the slider. SAIC commands can take minutes when the car is asleep, so a naive `optimistic: false` would feel broken. The dispatcher now does:
Handlers opt in via three optional methods on `CommandHandlerBase` (`echo_state_topic`, `capture_current_state`, `echo_payload`). The 6 number-entity handlers (target SoC, total battery capacity, 4 refresh periods) implement them; everything else is unchanged.
Test plan
New tests:
`pytest tests` → 177 passed.
Closes #375