Skip to content

fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX#440

Open
nanomad wants to merge 2 commits into
developfrom
fix/ha-number-optimistic-state
Open

fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX#440
nanomad wants to merge 2 commits into
developfrom
fix/ha-number-optimistic-state

Conversation

@nanomad
Copy link
Copy Markdown
Contributor

@nanomad nanomad commented May 9, 2026

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)

  • Add `"optimistic": false` to `_publish_number`, `_publish_select`, `_publish_text` payloads so HA reads state from `state_topic` instead of treating its cache as authoritative.
  • Switches and locks already had this set; binary_sensor / sensor / event are read-only so don't apply.

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:

  1. Eager echo on receipt — publish the requested value to the state topic before calling the SAIC API. The slider updates instantly.
  2. Rollback on failure — if the SAIC call (or any handler exception) fails, republish the previously captured value so the UI snaps back to the prior setting and the user sees the rejection.
  3. Skip echo when payload is invalid — `echo_payload` returns None for unparseable input, suppressing both the eager echo and the rollback.

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:

  • `tests/test_ha_discovery_optimistic.py` — discovery payload regression: number / select / text all carry `optimistic: false` (and switches still do, as a guard).
  • `tests/handlers/test_vehicle_command.py::TestNumberEntityEagerEcho`:
    • eager echo publishes the new value to the state topic
    • SAIC failure rolls back to the prior value
    • no rollback when no prior state was captured (target_soc is None)
    • invalid payload skips echo (and therefore skips rollback)
    • switches don't trigger echo

`pytest tests` → 177 passed.

Closes #375

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.
@nanomad nanomad force-pushed the fix/ha-number-optimistic-state branch from 8bfc57f to bde1bd7 Compare May 9, 2026 16:24
@nanomad nanomad changed the title fix: prevent stale HA values from overwriting writable entities fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX May 9, 2026
@nanomad nanomad force-pushed the fix/ha-number-optimistic-state branch 7 times, most recently from dd1e975 to d0607aa Compare May 9, 2026 16:55
@nanomad nanomad force-pushed the fix/ha-number-optimistic-state branch 2 times, most recently from 91d9145 to 6f8ada2 Compare May 10, 2026 10:04
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
@nanomad nanomad force-pushed the fix/ha-number-optimistic-state branch from 6f8ada2 to 23c3101 Compare May 10, 2026 10:27
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant