From 23c3101319f1d9108437abeb9bb12721e64947d0 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 9 May 2026 16:32:45 +0200 Subject: [PATCH 1/2] fix: HA Number/Select/Text optimistic-mode + slow-SoC slider UX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/handlers/command/base.py | 19 +- .../drivetrain_battery_heating_schedule.py | 29 ++ .../drivetrain_chargecurrent_limit.py | 18 ++ .../drivetrain_charging_schedule.py | 38 ++- .../drivetrain/drivetrain_soc_target.py | 18 ++ src/handlers/vehicle_command.py | 137 ++++++--- src/integrations/home_assistant/base.py | 3 + src/status_publisher/charge/chrg_mgmt_data.py | 6 +- src/vehicle.py | 25 +- tests/handlers/test_vehicle_command.py | 268 +++++++++++++++++- tests/test_ha_discovery_optimistic.py | 97 +++++++ 11 files changed, 609 insertions(+), 49 deletions(-) create mode 100644 tests/test_ha_discovery_optimistic.py diff --git a/src/handlers/command/base.py b/src/handlers/command/base.py index 888be4fd..7424ca4c 100644 --- a/src/handlers/command/base.py +++ b/src/handlers/command/base.py @@ -11,7 +11,7 @@ from saic_ismart_client_ng import SaicApi - from publisher.core import Publisher + from publisher.core import Publishable, Publisher from vehicle import VehicleState @@ -68,6 +68,23 @@ async def handle( ) -> CommandProcessingResult: raise NotImplementedError + # Optional eager-publish + rollback hooks. A handler opts in by overriding + # `state_topic` to return a non-None topic, in which case the dispatcher + # will publish `expected_state(payload)` to that topic on receipt and + # republish `current_state` if the SAIC call later fails. See + # `vehicle_command.VehicleCommandHandler.__execute_mqtt_command_handler`. + + @property + def state_topic(self) -> str | None: + return None + + @property + def current_state(self) -> Publishable | None: + return None + + def expected_state(self, _raw_payload: str) -> Publishable | None: + return None + @property def saic_api(self) -> SaicApi: return self.__saic_api diff --git a/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py b/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py index bc4728b8..d8bb8447 100644 --- a/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py +++ b/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py @@ -42,6 +42,35 @@ class DrivetrainBatteryHeatingScheduleCommand( def topic(cls) -> str: return mqtt_topics.DRIVETRAIN_BATTERY_HEATING_SCHEDULE_SET + @property + @override + def state_topic(self) -> str: + return mqtt_topics.DRIVETRAIN_BATTERY_HEATING_SCHEDULE + + @property + @override + def current_state(self) -> dict[str, Any] | None: + start_time = self.vehicle_state.scheduled_battery_heating_start + if start_time is None: + return None + return { + "startTime": start_time.strftime("%H:%M"), + "mode": "on" + if self.vehicle_state.scheduled_battery_heating_enabled + else "off", + } + + @override + def expected_state(self, raw_payload: str) -> dict[str, Any] | None: + try: + parsed = self.convert_payload(raw_payload) + except (ValueError, KeyError, json.JSONDecodeError): + return None + return { + "startTime": parsed.start_time.strftime("%H:%M"), + "mode": "on" if parsed.enable else "off", + } + @staticmethod @override def convert_payload(payload: str) -> BatteryHeatingScheduleCommandPayload: diff --git a/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py b/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py index 2d6d0feb..b7343b67 100644 --- a/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py +++ b/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py @@ -25,6 +25,24 @@ class DrivetrainChargeCurrentLimitCommand( def topic(cls) -> str: return mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT_SET + @property + @override + def state_topic(self) -> str: + return mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT + + @property + @override + def current_state(self) -> str | None: + limit = self.vehicle_state.charge_current_limit + return None if limit is None else limit.limit + + @override + def expected_state(self, raw_payload: str) -> str | None: + try: + return self.convert_payload(raw_payload).limit + except (ValueError, KeyError): + return None + @staticmethod @override def convert_payload(payload: str) -> ChargeCurrentLimitCode: diff --git a/src/handlers/command/drivetrain/drivetrain_charging_schedule.py b/src/handlers/command/drivetrain/drivetrain_charging_schedule.py index 713298d7..b76a312e 100644 --- a/src/handlers/command/drivetrain/drivetrain_charging_schedule.py +++ b/src/handlers/command/drivetrain/drivetrain_charging_schedule.py @@ -15,6 +15,7 @@ PayloadConvertingCommandHandler, ) import mqtt_topics +from status_publisher.charge.chrg_mgmt_data import ScheduledCharging if TYPE_CHECKING: from collections.abc import Mapping @@ -45,6 +46,35 @@ class DrivetrainChargingScheduleCommand( def topic(cls) -> str: return mqtt_topics.DRIVETRAIN_CHARGING_SCHEDULE_SET + @property + @override + def state_topic(self) -> str: + return mqtt_topics.DRIVETRAIN_CHARGING_SCHEDULE + + @property + @override + def current_state(self) -> dict[str, Any] | None: + schedule = self.vehicle_state.scheduled_charging + if schedule is None: + return None + return { + "startTime": schedule.start_time.strftime("%H:%M"), + "endTime": schedule.end_time.strftime("%H:%M"), + "mode": schedule.mode.name, + } + + @override + def expected_state(self, raw_payload: str) -> dict[str, Any] | None: + try: + parsed = self.convert_payload(raw_payload) + except (ValueError, KeyError, json.JSONDecodeError): + return None + return { + "startTime": parsed.start_time.strftime("%H:%M"), + "endTime": parsed.end_time.strftime("%H:%M"), + "mode": parsed.mode.name, + } + @staticmethod @override def convert_payload(payload: str) -> ChargingScheduleCommandPayload: @@ -71,5 +101,11 @@ async def handle_typed_payload( end_time=payload.end_time, mode=payload.mode, ) - self.vehicle_state.update_scheduled_charging(payload.start_time, payload.mode) + self.vehicle_state.update_scheduled_charging( + ScheduledCharging( + start_time=payload.start_time, + end_time=payload.end_time, + mode=payload.mode, + ) + ) return RESULT_REFRESH_ONLY diff --git a/src/handlers/command/drivetrain/drivetrain_soc_target.py b/src/handlers/command/drivetrain/drivetrain_soc_target.py index 904990c4..1aed2ea4 100644 --- a/src/handlers/command/drivetrain/drivetrain_soc_target.py +++ b/src/handlers/command/drivetrain/drivetrain_soc_target.py @@ -22,6 +22,24 @@ class DrivetrainSoCTargetCommand(PayloadConvertingCommandHandler[TargetBatteryCo def topic(cls) -> str: return mqtt_topics.DRIVETRAIN_SOC_TARGET_SET + @property + @override + def state_topic(self) -> str: + return mqtt_topics.DRIVETRAIN_SOC_TARGET + + @property + @override + def current_state(self) -> int | None: + target = self.vehicle_state.target_soc + return None if target is None else target.percentage + + @override + def expected_state(self, raw_payload: str) -> int | None: + try: + return self.convert_payload(raw_payload).percentage + except (ValueError, KeyError): + return None + @staticmethod @override def convert_payload(payload: str) -> TargetBatteryCode: diff --git a/src/handlers/vehicle_command.py b/src/handlers/vehicle_command.py index e7f994ca..31cb6a56 100644 --- a/src/handlers/vehicle_command.py +++ b/src/handlers/vehicle_command.py @@ -17,7 +17,7 @@ from saic_ismart_client_ng import SaicApi from handlers.relogin import ReloginHandler - from publisher.core import Publisher + from publisher.core import Publishable, Publisher from vehicle import VehicleState CommandHandler = Callable[[str], Awaitable[bool]] @@ -57,6 +57,17 @@ def __init__( def publisher(self) -> Publisher: return self.vehicle_state.publisher + def __publish_state(self, *, topic: str, value: Publishable) -> None: + full_topic = self.vehicle_state.get_topic(topic) + try: + self.publisher.publish(full_topic, value) + except Exception: + LOG.warning( + "Failed to publish state for topic %s", + full_topic, + exc_info=True, + ) + def __report_command_failure( self, *, @@ -111,6 +122,24 @@ async def handle_mqtt_command( retained=retained, ) + async def __run_handler_and_report_success( + self, + *, + handler: CommandHandlerBase, + payload: str, + analyzed_topic: _MqttCommandTopic, + retained: bool, + ) -> None: + execution_result = await handler.handle(payload, retained=retained) + self.publisher.publish_str(analyzed_topic.response_no_global, "Success") + if execution_result.force_refresh: + self.vehicle_state.set_refresh_mode( + RefreshMode.FORCE, + f"after command execution on topic {analyzed_topic.command_no_vin}", + ) + if execution_result.clear_command and not retained: + self.publisher.clear_topic(analyzed_topic.command_no_global) + async def __execute_mqtt_command_handler( self, *, @@ -120,7 +149,6 @@ async def __execute_mqtt_command_handler( retained: bool, ) -> None: topic = analyzed_topic.command_no_vin - topic_no_global = analyzed_topic.command_no_global result_topic = analyzed_topic.response_no_global if retained and not handler.is_replayable_when_retained(): @@ -136,55 +164,47 @@ async def __execute_mqtt_command_handler( ) return + state_topic = handler.state_topic + prior_state: Publishable | None = None + published = False + if state_topic is not None and not retained: + expected = handler.expected_state(payload) + if expected is not None: + prior_state = handler.current_state + self.__publish_state(topic=state_topic, value=expected) + published = True + + def rollback_state() -> None: + if published and state_topic is not None and prior_state is not None: + self.__publish_state(topic=state_topic, value=prior_state) + try: - execution_result = await handler.handle(payload, retained=retained) - self.publisher.publish_str(result_topic, "Success") - if execution_result.force_refresh: - self.vehicle_state.set_refresh_mode( - RefreshMode.FORCE, f"after command execution on topic {topic}" - ) - if execution_result.clear_command and not retained: - self.publisher.clear_topic(topic_no_global) + await self.__run_handler_and_report_success( + handler=handler, + payload=payload, + analyzed_topic=analyzed_topic, + retained=retained, + ) except MqttGatewayException as e: + rollback_state() self.__report_command_failure( command=topic, result_topic=result_topic, detail=e.message, exc=e ) except SaicLogoutException: - LOG.warning( - "API Client was logged out, attempting immediate relogin and retry" + await self.__handle_logout_and_retry( + handler=handler, + payload=payload, + analyzed_topic=analyzed_topic, + retained=retained, + rollback_state=rollback_state, ) - try: - await self.relogin_handler.force_login() - except Exception as login_err: - self.__report_command_failure( - command=topic, - result_topic=result_topic, - detail=f"relogin failed ({login_err})", - exc=login_err, - ) - return - try: - execution_result = await handler.handle(payload, retained=retained) - self.publisher.publish_str(result_topic, "Success") - if execution_result.force_refresh: - self.vehicle_state.set_refresh_mode( - RefreshMode.FORCE, - f"after command execution on topic {topic}", - ) - if execution_result.clear_command and not retained: - self.publisher.clear_topic(topic_no_global) - except Exception as retry_err: - self.__report_command_failure( - command=topic, - result_topic=result_topic, - detail=str(retry_err), - exc=retry_err, - ) except SaicApiException as se: + rollback_state() self.__report_command_failure( command=topic, result_topic=result_topic, detail=se.message, exc=se ) except Exception as e: + rollback_state() self.__report_command_failure( command=topic, result_topic=result_topic, @@ -192,6 +212,45 @@ async def __execute_mqtt_command_handler( exc=e, ) + async def __handle_logout_and_retry( + self, + *, + handler: CommandHandlerBase, + payload: str, + analyzed_topic: _MqttCommandTopic, + retained: bool, + rollback_state: Callable[[], None], + ) -> None: + topic = analyzed_topic.command_no_vin + result_topic = analyzed_topic.response_no_global + LOG.warning("API Client was logged out, attempting immediate relogin and retry") + try: + await self.relogin_handler.force_login() + except Exception as login_err: + rollback_state() + self.__report_command_failure( + command=topic, + result_topic=result_topic, + detail=f"relogin failed ({login_err})", + exc=login_err, + ) + return + try: + await self.__run_handler_and_report_success( + handler=handler, + payload=payload, + analyzed_topic=analyzed_topic, + retained=retained, + ) + except Exception as retry_err: + rollback_state() + self.__report_command_failure( + command=topic, + result_topic=result_topic, + detail=str(retry_err), + exc=retry_err, + ) + def __get_command_topics(self, topic: str) -> _MqttCommandTopic: global_topic_removed = topic.removeprefix(self.global_mqtt_topic).removeprefix( "/" diff --git a/src/integrations/home_assistant/base.py b/src/integrations/home_assistant/base.py index a7603ddc..142b7e23 100644 --- a/src/integrations/home_assistant/base.py +++ b/src/integrations/home_assistant/base.py @@ -57,6 +57,7 @@ def _publish_select( "command_template": command_template, "retain": str(retain).lower(), "options": options, + "optimistic": False, "enabled_by_default": enabled, } if entity_category is not None: @@ -88,6 +89,7 @@ def _publish_text( "value_template": value_template, "command_template": command_template, "retain": str(retain).lower(), + "optimistic": False, "enabled_by_default": enabled, } if min_value is not None: @@ -154,6 +156,7 @@ def _publish_number( "command_topic": self._get_command_topic(topic), "value_template": value_template, "retain": str(retain).lower(), + "optimistic": False, "mode": mode, "min": min_value, "max": max_value, diff --git a/src/status_publisher/charge/chrg_mgmt_data.py b/src/status_publisher/charge/chrg_mgmt_data.py index b0444fc9..a527a186 100644 --- a/src/status_publisher/charge/chrg_mgmt_data.py +++ b/src/status_publisher/charge/chrg_mgmt_data.py @@ -25,6 +25,7 @@ @dataclasses.dataclass(kw_only=True, frozen=True) class ScheduledCharging: start_time: datetime.time + end_time: datetime.time mode: ScheduledChargingMode @@ -192,6 +193,7 @@ def __publish_charging_schedule( start_time = datetime.time(hour=start_hour, minute=start_minute) end_hour = charge_mgmt_data.bmsReserSpHourDspCmd end_minute = charge_mgmt_data.bmsReserSpMintueDspCmd + end_time = datetime.time(hour=end_hour, minute=end_minute) mode = ScheduledChargingMode(charge_mgmt_data.bmsReserCtrlDspCmd) self._publish( topic=mqtt_topics.DRIVETRAIN_CHARGING_SCHEDULE, @@ -201,7 +203,9 @@ def __publish_charging_schedule( "mode": mode.name, }, ) - scheduled_charging = ScheduledCharging(start_time=start_time, mode=mode) + scheduled_charging = ScheduledCharging( + start_time=start_time, end_time=end_time, mode=mode + ) except ValueError: LOG.exception("Error parsing scheduled charging info") diff --git a/src/vehicle.py b/src/vehicle.py index 731293d2..8dc14d66 100644 --- a/src/vehicle.py +++ b/src/vehicle.py @@ -40,6 +40,7 @@ ) from publisher.core import Publisher + from status_publisher.charge.chrg_mgmt_data import ScheduledCharging from vehicle_info import VehicleInfo DEFAULT_AC_TEMP = 22 @@ -141,6 +142,7 @@ def __init__( self.__remote_heated_seats_front_left_level: int = 0 self.__remote_heated_seats_front_right_level: int = 0 self.__scheduler = scheduler + self.__scheduled_charging: ScheduledCharging | None = None self.__scheduled_battery_heating_enabled = False self.__scheduled_battery_heating_start: datetime.time | None = None self.__user_timezone: ZoneInfo | None = user_timezone @@ -236,9 +238,10 @@ def update_charge_current_limit( except ValueError: LOG.exception(f"Unhandled charge current limit {charge_current_limit}") - def update_scheduled_charging( - self, start_time: datetime.time, mode: ScheduledChargingMode - ) -> None: + def update_scheduled_charging(self, schedule: ScheduledCharging) -> None: + self.__scheduled_charging = schedule + start_time = schedule.start_time + mode = schedule.mode scheduled_charging_job_id = f"{self.vin}_scheduled_charging" existing_job: Job | None = self.__scheduler.get_job(scheduled_charging_job_id) if mode in [ @@ -585,9 +588,7 @@ def handle_charge_status( result = self.__charge_response_publisher.publish(charge_info_resp) if result.scheduled_charging is not None: - self.update_scheduled_charging( - result.scheduled_charging.start_time, result.scheduled_charging.mode - ) + self.update_scheduled_charging(result.scheduled_charging) if result.charge_current_limit is not None: self.update_charge_current_limit(result.charge_current_limit) @@ -818,3 +819,15 @@ def __publish[V: Publishable]( @property def vin(self) -> str: return self.vehicle.vin + + @property + def scheduled_charging(self) -> ScheduledCharging | None: + return self.__scheduled_charging + + @property + def scheduled_battery_heating_start(self) -> datetime.time | None: + return self.__scheduled_battery_heating_start + + @property + def scheduled_battery_heating_enabled(self) -> bool: + return self.__scheduled_battery_heating_enabled diff --git a/tests/handlers/test_vehicle_command.py b/tests/handlers/test_vehicle_command.py index d9009831..20074e7c 100644 --- a/tests/handlers/test_vehicle_command.py +++ b/tests/handlers/test_vehicle_command.py @@ -1,14 +1,24 @@ from __future__ import annotations +import datetime +import json from typing import cast import unittest from unittest.mock import AsyncMock, MagicMock, patch +from saic_ismart_client_ng.api.vehicle.schema import VehicleModelConfiguration, VinInfo +from saic_ismart_client_ng.api.vehicle_charging import ( + ChargeCurrentLimitCode, + ScheduledChargingMode, + TargetBatteryCode, +) from saic_ismart_client_ng.exceptions import SaicApiException, SaicLogoutException from handlers.vehicle_command import VehicleCommandHandler import mqtt_topics +from status_publisher.charge.chrg_mgmt_data import ScheduledCharging from vehicle import RefreshMode +from vehicle_info import VehicleInfo MQTT_TOPIC = "saic" VIN = "vin_test_000000000" @@ -20,12 +30,33 @@ f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGING}/{mqtt_topics.RESULT_SUFFIX}" ) COMMAND_ERROR_TOPIC = f"{VEHICLE_PREFIX}/{mqtt_topics.COMMAND_ERROR}" +SOC_TARGET_SET_TOPIC = ( + f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_SOC_TARGET_SET}" +) +SOC_TARGET_STATE_TOPIC = f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_SOC_TARGET}" +CHARGECURRENT_SET_TOPIC = ( + f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT_SET}" +) +CHARGECURRENT_STATE_TOPIC = ( + f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT}" +) +CHARGING_SCHEDULE_SET_TOPIC = ( + f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGING_SCHEDULE_SET}" +) +CHARGING_SCHEDULE_STATE_TOPIC = ( + f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGING_SCHEDULE}" +) +BATTERY_HEATING_SET_TOPIC = f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_BATTERY_HEATING_SCHEDULE_SET}" +BATTERY_HEATING_STATE_TOPIC = ( + f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_BATTERY_HEATING_SCHEDULE}" +) def _build( *, saic_api: AsyncMock | None = None, relogin_handler: AsyncMock | None = None, + vehicle_state: MagicMock | None = None, ) -> tuple[VehicleCommandHandler, MagicMock]: """Build a VehicleCommandHandler with a MagicMock publisher. @@ -33,7 +64,8 @@ def _build( without going through the typed Publisher interface. """ mock_publisher = MagicMock() - vehicle_state = MagicMock() + if vehicle_state is None: + vehicle_state = MagicMock() vehicle_state.publisher = mock_publisher vehicle_state.vin = VIN vehicle_state.get_topic.side_effect = lambda t: f"{VEHICLE_PREFIX}/{t}" @@ -49,6 +81,26 @@ def _build( ) +def _make_target_soc_state( + *, current: TargetBatteryCode | None = TargetBatteryCode.P_80 +) -> MagicMock: + """Build a vehicle state mock that supports target SoC. + + Reports `current` as the previously applied value (for rollback testing). + """ + vin_info = VinInfo() + vin_info.vin = VIN + vin_info.vehicleModelConfiguration = [ + VehicleModelConfiguration(itemCode="BType", itemValue="1"), + ] + vehicle_info = VehicleInfo(vin_info, None) + + state = MagicMock() + state.vehicle = vehicle_info + state.target_soc = current + return state + + class TestSuccessPath(unittest.IsolatedAsyncioTestCase): async def test_successful_command_publishes_success(self) -> None: handler, pub = _build() @@ -216,6 +268,220 @@ async def test_publish_json_failure_does_not_raise(self) -> None: pub.publish_str.assert_called_once() +class TestEagerStatePublish(unittest.IsolatedAsyncioTestCase): + """Verify the dispatcher's eager-publish + rollback path. + + With `optimistic: false`, HA waits for `state_topic` before updating the + slider. The dispatcher must therefore publish the expected state on + receipt (instant UX feedback) and revert to the prior value if the SAIC + call fails (visible rejection). + """ + + async def test_publishes_expected_state_on_receipt(self) -> None: + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + pub.publish.assert_any_call(SOC_TARGET_STATE_TOPIC, 90) + + async def test_rollback_on_saic_api_failure(self) -> None: + saic_api = AsyncMock() + saic_api.set_target_battery_soc.side_effect = SaicApiException( + "rejected", return_code=4 + ) + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + # First the expected state for the requested value (90), then the + # rollback to the captured prior value (80). + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [90, 80] + + async def test_no_rollback_when_no_prior_state_captured(self) -> None: + # If the gateway has not yet learned the vehicle's current target SoC + # there is nothing to roll back to, so we must not publish None. + saic_api = AsyncMock() + saic_api.set_target_battery_soc.side_effect = SaicApiException( + "rejected", return_code=4 + ) + state = _make_target_soc_state(current=None) + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [90] + + async def test_non_numeric_payload_skips_publish(self) -> None: + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command( + topic=SOC_TARGET_SET_TOPIC, payload="not_a_number" + ) + + state_publishes = [ + call.args + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [] + + async def test_numeric_but_unsupported_bucket_skips_publish(self) -> None: + # 85% is numeric but not one of the discrete TargetBatteryCode buckets + # (40/50/60/70/80/90/100). `from_percentage` raises, expected_state + # returns None, and nothing is published to state_topic. + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="85") + + state_publishes = [ + call.args + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [] + + async def test_switch_command_does_not_publish_state(self) -> None: + # Switches don't override state_topic on CommandHandlerBase, so it + # stays None and the dispatcher skips the eager-publish path. Verify + # nothing leaks to a state topic. + handler, pub = _build() + + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") + + for call in pub.publish.call_args_list: + assert "drivetrain/charging" not in call.args[0] or "/set" in call.args[0] + + async def test_retained_soc_target_does_not_leak_to_state_topic(self) -> None: + # SoC target has not opted into is_replayable_when_retained(), so a + # retained `/set` (e.g. from a misbehaving non-HA client that retained + # the topic) is dropped at the dispatcher gate before the eager-publish + # block runs. Nothing must leak to state_topic — otherwise the slider + # would jump on reconnect to a value the SAIC API never confirmed. + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command( + topic=SOC_TARGET_SET_TOPIC, payload="90", retained=True + ) + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [] + + +class TestEagerStatePublishOtherEntities(unittest.IsolatedAsyncioTestCase): + """Eager-publish + rollback for the other API-backed writable entities. + + Same contract as the SoC slider: charge current limit (string state) and + the two schedule entities (JSON-dict state) all need eager-echo because + HA's `optimistic: false` would otherwise leave the user staring at a + frozen control while the SAIC roundtrip completes. + """ + + async def test_chargecurrent_limit_echo_and_rollback(self) -> None: + saic_api = AsyncMock() + saic_api.set_target_battery_soc.side_effect = SaicApiException( + "rejected", return_code=4 + ) + state = MagicMock() + state.charge_current_limit = ChargeCurrentLimitCode.C_6A + state.target_soc = TargetBatteryCode.P_80 + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + await handler.handle_mqtt_command(topic=CHARGECURRENT_SET_TOPIC, payload="MAX") + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == CHARGECURRENT_STATE_TOPIC + ] + assert state_publishes == [ + ChargeCurrentLimitCode.C_MAX.limit, + ChargeCurrentLimitCode.C_6A.limit, + ] + + async def test_charging_schedule_echo_and_rollback(self) -> None: + saic_api = AsyncMock() + saic_api.set_schedule_charging.side_effect = SaicApiException( + "rejected", return_code=4 + ) + prior = ScheduledCharging( + start_time=datetime.time(7, 0), + end_time=datetime.time(9, 0), + mode=ScheduledChargingMode.DISABLED, + ) + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + state.scheduled_charging = prior + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + payload = json.dumps( + {"startTime": "08:00", "endTime": "10:00", "mode": "UNTIL_CONFIGURED_TIME"} + ) + await handler.handle_mqtt_command( + topic=CHARGING_SCHEDULE_SET_TOPIC, payload=payload + ) + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == CHARGING_SCHEDULE_STATE_TOPIC + ] + assert state_publishes == [ + { + "startTime": "08:00", + "endTime": "10:00", + "mode": "UNTIL_CONFIGURED_TIME", + }, + {"startTime": "07:00", "endTime": "09:00", "mode": "DISABLED"}, + ] + + async def test_battery_heating_schedule_echo_and_rollback(self) -> None: + saic_api = AsyncMock() + saic_api.enable_schedule_battery_heating.side_effect = SaicApiException( + "rejected", return_code=4 + ) + state = MagicMock() + state.scheduled_battery_heating_start = datetime.time(6, 30) + state.scheduled_battery_heating_enabled = True + state.user_timezone = None + # The handler short-circuits via update_scheduled_battery_heating; force + # it to report a real change so the SAIC call (and thus rollback) fires. + state.update_scheduled_battery_heating.return_value = True + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + payload = json.dumps({"startTime": "08:00", "mode": "ON"}) + await handler.handle_mqtt_command( + topic=BATTERY_HEATING_SET_TOPIC, payload=payload + ) + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == BATTERY_HEATING_STATE_TOPIC + ] + assert state_publishes == [ + {"startTime": "08:00", "mode": "on"}, + {"startTime": "06:30", "mode": "on"}, + ] + + class TestErrorEventPayload(unittest.IsolatedAsyncioTestCase): async def test_topic_uses_vehicle_prefix(self) -> None: saic_api = AsyncMock() diff --git a/tests/test_ha_discovery_optimistic.py b/tests/test_ha_discovery_optimistic.py new file mode 100644 index 00000000..a1ee1e51 --- /dev/null +++ b/tests/test_ha_discovery_optimistic.py @@ -0,0 +1,97 @@ +from __future__ import annotations + +import json +from typing import TYPE_CHECKING, Any, override +import unittest + +from configuration import Configuration +from integrations.home_assistant.base import HomeAssistantDiscoveryBase + +from .common_mocks import VIN +from .mocks import MessageCapturingConsolePublisher + +if TYPE_CHECKING: + from integrations.home_assistant.availability import HaCustomAvailabilityConfig + + +class _Discovery(HomeAssistantDiscoveryBase): + """Minimal concrete subclass to exercise the protected publish helpers. + + Used to build HA MQTT discovery payloads for writable entities. + """ + + def __init__(self, publisher: MessageCapturingConsolePublisher) -> None: + self.publisher = publisher + + @override + def _get_state_topic(self, raw_topic: str) -> str: + return f"vehicles/{VIN}/{raw_topic}" + + @override + def _get_command_topic(self, raw_topic: str) -> str: + return f"vehicles/{VIN}/{raw_topic}/set" + + @override + def _publish_ha_discovery_message( + self, + sensor_type: str, + sensor_name: str, + payload: dict[str, Any], + custom_availability: HaCustomAvailabilityConfig | None = None, + ) -> str: + topic = f"homeassistant/{sensor_type}/{VIN}/{sensor_name}/config" + self.publisher.publish_json(topic, payload, no_prefix=True) + return topic + + def _decode(self, topic: str) -> dict[str, Any]: + decoded: dict[str, Any] = json.loads(self.publisher.map[topic]) + return decoded + + def number_payload(self) -> dict[str, Any]: + topic = self._publish_number(topic="drivetrain/socTarget", name="target_soc") + return self._decode(topic) + + def select_payload(self) -> dict[str, Any]: + topic = self._publish_select( + topic="drivetrain/chargeCurrentLimit", + name="charge_current_limit", + options=["MAX", "6A"], + ) + return self._decode(topic) + + def text_payload(self) -> dict[str, Any]: + topic = self._publish_text( + topic="drivetrain/scheduledChargingStart", name="schedule" + ) + return self._decode(topic) + + def switch_payload(self) -> dict[str, Any]: + topic = self._publish_switch(topic="drivetrain/charging", name="charging") + return self._decode(topic) + + +class TestWritableEntitiesAreNonOptimistic(unittest.TestCase): + """Writable entities must declare `optimistic: false`. + + HA reads state from `state_topic` rather than republishing its own cached + value on restart. Guards #375 (target SoC reset on restart) and the + analogous battery-capacity report. + """ + + def setUp(self) -> None: + publisher = MessageCapturingConsolePublisher(Configuration()) + self.discovery = _Discovery(publisher) + + def test_number_payload_sets_optimistic_false(self) -> None: + assert self.discovery.number_payload().get("optimistic") is False + + def test_select_payload_sets_optimistic_false(self) -> None: + assert self.discovery.select_payload().get("optimistic") is False + + def test_text_payload_sets_optimistic_false(self) -> None: + assert self.discovery.text_payload().get("optimistic") is False + + def test_switch_payload_keeps_optimistic_false(self) -> None: + # Regression guard: switches were already correct; ensure the audit + # didn't accidentally remove the flag. + assert self.discovery.switch_payload().get("optimistic") is False From 694dc0305be814ed5400eab6458c16753e4b5675 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sun, 10 May 2026 15:38:00 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20PR=20#440=20review=20?= =?UTF-8?q?=E2=80=94=20battery-heating=20ordering=20and=20RESULT=5FDO=5FNO?= =?UTF-8?q?THING=20rollback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/handlers/command/base.py | 2 +- .../drivetrain_battery_heating_schedule.py | 37 +++--- .../drivetrain_chargecurrent_limit.py | 1 + src/handlers/vehicle_command.py | 20 +++ tests/handlers/test_vehicle_command.py | 114 +++++++++++++++++- 5 files changed, 155 insertions(+), 19 deletions(-) diff --git a/src/handlers/command/base.py b/src/handlers/command/base.py index 7424ca4c..3fa939dc 100644 --- a/src/handlers/command/base.py +++ b/src/handlers/command/base.py @@ -72,7 +72,7 @@ async def handle( # `state_topic` to return a non-None topic, in which case the dispatcher # will publish `expected_state(payload)` to that topic on receipt and # republish `current_state` if the SAIC call later fails. See - # `vehicle_command.VehicleCommandHandler.__execute_mqtt_command_handler`. + # `VehicleCommandHandler` in `handlers/vehicle_command.py`. @property def state_topic(self) -> str | None: diff --git a/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py b/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py index d8bb8447..70817e06 100644 --- a/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py +++ b/src/handlers/command/drivetrain/drivetrain_battery_heating_schedule.py @@ -83,20 +83,27 @@ async def handle_typed_payload( ) -> CommandProcessingResult: start_time = payload.start_time should_enable = payload.enable - changed = self.vehicle_state.update_scheduled_battery_heating( - start_time, should_enable - ) - if changed: - if should_enable: - LOG.info(f"Setting battery heating schedule to {start_time}") - await self.saic_api.enable_schedule_battery_heating( - self.vin, - start_time=start_time, - tz=self.vehicle_state.user_timezone, - ) - else: - LOG.info("Disabling battery heating schedule") - await self.saic_api.disable_schedule_battery_heating(self.vin) - else: + # Mutate in-memory state only after the SAIC call succeeds: otherwise + # an API failure leaves the gateway holding the failed-new value, so + # the next eager-echo `current_state` would be wrong on rollback. + if ( + self.vehicle_state.scheduled_battery_heating_start == start_time + and self.vehicle_state.scheduled_battery_heating_enabled == should_enable + ): + # No state change: skip the canonical-schedule republish that + # `update_scheduled_battery_heating` would do as a side effect. + # The eager echo already stamped the broker so it stays consistent. LOG.info("Battery heating schedule not changed") + return RESULT_REFRESH_ONLY + if should_enable: + LOG.info(f"Setting battery heating schedule to {start_time}") + await self.saic_api.enable_schedule_battery_heating( + self.vin, + start_time=start_time, + tz=self.vehicle_state.user_timezone, + ) + else: + LOG.info("Disabling battery heating schedule") + await self.saic_api.disable_schedule_battery_heating(self.vin) + self.vehicle_state.update_scheduled_battery_heating(start_time, should_enable) return RESULT_REFRESH_ONLY diff --git a/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py b/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py index b7343b67..15e09239 100644 --- a/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py +++ b/src/handlers/command/drivetrain/drivetrain_chargecurrent_limit.py @@ -22,6 +22,7 @@ class DrivetrainChargeCurrentLimitCommand( PayloadConvertingCommandHandler[ChargeCurrentLimitCode] ): @classmethod + @override def topic(cls) -> str: return mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT_SET diff --git a/src/handlers/vehicle_command.py b/src/handlers/vehicle_command.py index 31cb6a56..fba19298 100644 --- a/src/handlers/vehicle_command.py +++ b/src/handlers/vehicle_command.py @@ -8,6 +8,7 @@ from exceptions import MqttGatewayException from handlers.command import ALL_COMMAND_HANDLERS, CommandHandlerBase +from handlers.command.base import RESULT_DO_NOTHING import mqtt_topics from vehicle import RefreshMode @@ -129,8 +130,21 @@ async def __run_handler_and_report_success( payload: str, analyzed_topic: _MqttCommandTopic, retained: bool, + eager_echo_published: bool = False, + rollback_state: Callable[[], None] | None = None, ) -> None: execution_result = await handler.handle(payload, retained=retained) + # If the handler short-circuited via RESULT_DO_NOTHING (e.g. unsupported + # vehicle capability) after we eagerly echoed, the broker still holds + # the requested-but-unapplied value. Roll it back so the slider snaps + # to the actual on-vehicle state. + if execution_result == RESULT_DO_NOTHING and eager_echo_published: + LOG.warning( + "Handler %s returned RESULT_DO_NOTHING after eager echo; rolling back", + handler.name(), + ) + if rollback_state is not None: + rollback_state() self.publisher.publish_str(analyzed_topic.response_no_global, "Success") if execution_result.force_refresh: self.vehicle_state.set_refresh_mode( @@ -184,6 +198,8 @@ def rollback_state() -> None: payload=payload, analyzed_topic=analyzed_topic, retained=retained, + eager_echo_published=published, + rollback_state=rollback_state, ) except MqttGatewayException as e: rollback_state() @@ -196,6 +212,7 @@ def rollback_state() -> None: payload=payload, analyzed_topic=analyzed_topic, retained=retained, + eager_echo_published=published, rollback_state=rollback_state, ) except SaicApiException as se: @@ -219,6 +236,7 @@ async def __handle_logout_and_retry( payload: str, analyzed_topic: _MqttCommandTopic, retained: bool, + eager_echo_published: bool, rollback_state: Callable[[], None], ) -> None: topic = analyzed_topic.command_no_vin @@ -241,6 +259,8 @@ async def __handle_logout_and_retry( payload=payload, analyzed_topic=analyzed_topic, retained=retained, + eager_echo_published=eager_echo_published, + rollback_state=rollback_state, ) except Exception as retry_err: rollback_state() diff --git a/tests/handlers/test_vehicle_command.py b/tests/handlers/test_vehicle_command.py index 20074e7c..6254e4af 100644 --- a/tests/handlers/test_vehicle_command.py +++ b/tests/handlers/test_vehicle_command.py @@ -34,6 +34,9 @@ f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_SOC_TARGET_SET}" ) SOC_TARGET_STATE_TOPIC = f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_SOC_TARGET}" +SOC_TARGET_RESULT_TOPIC = ( + f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_SOC_TARGET}/{mqtt_topics.RESULT_SUFFIX}" +) CHARGECURRENT_SET_TOPIC = ( f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGECURRENT_LIMIT_SET}" ) @@ -461,9 +464,6 @@ async def test_battery_heating_schedule_echo_and_rollback(self) -> None: state.scheduled_battery_heating_start = datetime.time(6, 30) state.scheduled_battery_heating_enabled = True state.user_timezone = None - # The handler short-circuits via update_scheduled_battery_heating; force - # it to report a real change so the SAIC call (and thus rollback) fires. - state.update_scheduled_battery_heating.return_value = True handler, pub = _build(saic_api=saic_api, vehicle_state=state) payload = json.dumps({"startTime": "08:00", "mode": "ON"}) @@ -480,6 +480,114 @@ async def test_battery_heating_schedule_echo_and_rollback(self) -> None: {"startTime": "08:00", "mode": "on"}, {"startTime": "06:30", "mode": "on"}, ] + # The fix moves the in-memory mutation to after API success. With the + # API call failing, update_scheduled_battery_heating must NOT have run + # — otherwise the gateway holds the failed-new value and the next + # eager-echo's `current_state` would be wrong. + state.update_scheduled_battery_heating.assert_not_called() + + +class TestEagerStateEdgeCases(unittest.IsolatedAsyncioTestCase): + """Edge cases around the eager-echo path that aren't tied to a single entity.""" + + async def test_result_do_nothing_rolls_back_eager_echo(self) -> None: + # Vehicle without target-SoC support: the SoC handler returns + # RESULT_DO_NOTHING after we've already echoed the requested value. + # Without the fix the slider would stick on the unsupported value. + vin_info = VinInfo() + vin_info.vin = VIN + vin_info.vehicleModelConfiguration = [] + state = MagicMock() + state.vehicle = VehicleInfo(vin_info, None) + state.target_soc = TargetBatteryCode.P_80 + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert state_publishes == [90, 80] + + async def test_logout_retry_success_preserves_eager_echo(self) -> None: + # First SAIC call hits a logout, retry after relogin succeeds. The + # eager-echoed value must remain on the broker (no spurious rollback). + saic_api = AsyncMock() + saic_api.set_target_battery_soc.side_effect = [ + SaicLogoutException("logged out"), + None, + ] + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + state_publishes = [ + call.args[1] + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + # Only the eager echo — no rollback on the successful retry. + assert state_publishes == [90] + assert saic_api.set_target_battery_soc.await_count == 2 + pub.publish_str.assert_any_call(SOC_TARGET_RESULT_TOPIC, "Success") + + async def test_broker_failure_during_rollback_does_not_crash(self) -> None: + # The broker drops while we try to roll back the eager echo. The + # dispatcher must still attempt the rollback and publish the failure + # to the result topic, not propagate the publish exception. + saic_api = AsyncMock() + saic_api.set_target_battery_soc.side_effect = SaicApiException( + "rejected", return_code=4 + ) + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + handler, pub = _build(saic_api=saic_api, vehicle_state=state) + # First publish is the eager echo (succeeds), second is the rollback + # (broker is now down). + pub.publish.side_effect = [None, ConnectionError("broker down")] + + await handler.handle_mqtt_command(topic=SOC_TARGET_SET_TOPIC, payload="90") + + # Both publish calls to the state topic must fire: the rollback was + # attempted even though the broker raised on it. + state_publishes = [ + call.args + for call in pub.publish.call_args_list + if call.args[0] == SOC_TARGET_STATE_TOPIC + ] + assert len(state_publishes) == 2 + # And the dispatcher kept going to publish the failure result. + result_calls = [ + call.args + for call in pub.publish_str.call_args_list + if call.args[0] == SOC_TARGET_RESULT_TOPIC + ] + assert any("Failed:" in args[1] for args in result_calls) + + async def test_malformed_charging_schedule_skips_eager_echo(self) -> None: + # Bad JSON in the payload: convert_payload (and thus expected_state) + # raises and the dispatcher must skip the eager publish entirely. + prior = ScheduledCharging( + start_time=datetime.time(7, 0), + end_time=datetime.time(9, 0), + mode=ScheduledChargingMode.DISABLED, + ) + state = _make_target_soc_state(current=TargetBatteryCode.P_80) + state.scheduled_charging = prior + handler, pub = _build(vehicle_state=state) + + await handler.handle_mqtt_command( + topic=CHARGING_SCHEDULE_SET_TOPIC, payload="not json {" + ) + + state_publishes = [ + call.args + for call in pub.publish.call_args_list + if call.args[0] == CHARGING_SCHEDULE_STATE_TOPIC + ] + assert state_publishes == [] class TestErrorEventPayload(unittest.IsolatedAsyncioTestCase):