From a8ab0a27d0108f46ade95bc9fe93b79e78ce3abd Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 16:55:20 +0100 Subject: [PATCH 1/3] fix: publish window entities as binary_sensors instead of switches (#252) Window entities were exposed as switches in HA discovery but had no command handlers, causing errors when toggled. Change them to binary_sensors with device_class=window. Old switch discovery entries are unpublished on startup to provide a clean migration path. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/integrations/home_assistant/discovery.py | 27 ++++- tests/test_ha_discovery_windows.py | 121 +++++++++++++++++++ 2 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 tests/test_ha_discovery_windows.py diff --git a/src/integrations/home_assistant/discovery.py b/src/integrations/home_assistant/discovery.py index ba1e9cd7..0e052d57 100644 --- a/src/integrations/home_assistant/discovery.py +++ b/src/integrations/home_assistant/discovery.py @@ -451,15 +451,30 @@ def __publish_location_sensors(self) -> None: ) def __publish_windows_sensors(self) -> None: - self._publish_switch(mqtt_topics.WINDOWS_DRIVER, "Window driver") - self._publish_switch(mqtt_topics.WINDOWS_PASSENGER, "Window passenger") - self._publish_switch(mqtt_topics.WINDOWS_REAR_LEFT, "Window rear left") - self._publish_switch(mqtt_topics.WINDOWS_REAR_RIGHT, "Window rear right") + self.__unpublish_ha_discovery_message("switch", "Window driver") + self.__unpublish_ha_discovery_message("switch", "Window passenger") + self.__unpublish_ha_discovery_message("switch", "Window rear left") + self.__unpublish_ha_discovery_message("switch", "Window rear right") + self.__unpublish_ha_discovery_message("switch", "Sun roof") + self._publish_binary_sensor( + mqtt_topics.WINDOWS_DRIVER, "Window driver", device_class="window" + ) + self._publish_binary_sensor( + mqtt_topics.WINDOWS_PASSENGER, "Window passenger", device_class="window" + ) + self._publish_binary_sensor( + mqtt_topics.WINDOWS_REAR_LEFT, + "Window rear left", + device_class="window", + ) + self._publish_binary_sensor( + mqtt_topics.WINDOWS_REAR_RIGHT, + "Window rear right", + device_class="window", + ) if self.__vin_info.has_sunroof: - self._publish_switch(mqtt_topics.WINDOWS_SUN_ROOF, "Sun roof") self._publish_binary_sensor(mqtt_topics.WINDOWS_SUN_ROOF, "Sun roof") else: - self.__unpublish_ha_discovery_message("switch", "Sun roof") self.__unpublish_ha_discovery_message("binary_sensor", "Sun roof") def __publish_ccu_sensors(self) -> None: diff --git a/tests/test_ha_discovery_windows.py b/tests/test_ha_discovery_windows.py new file mode 100644 index 00000000..5c8995fc --- /dev/null +++ b/tests/test_ha_discovery_windows.py @@ -0,0 +1,121 @@ +from __future__ import annotations + +import json +import unittest + +from apscheduler.schedulers.blocking import BlockingScheduler +from saic_ismart_client_ng.api.vehicle.schema import ( + VehicleModelConfiguration, + VinInfo, +) + +from configuration import Configuration +from integrations.home_assistant.discovery import HomeAssistantDiscovery +from vehicle import VehicleState +from vehicle_info import VehicleInfo + +from .common_mocks import VIN +from .mocks import MessageCapturingConsolePublisher + + +class TestHaDiscoveryWindows(unittest.TestCase): + """Test that window entities are published as binary_sensors, not switches.""" + + def _make_discovery( + self, *, has_sunroof: bool = False + ) -> tuple[HomeAssistantDiscovery, MessageCapturingConsolePublisher]: + config = Configuration() + config.anonymized_publishing = False + config.ha_discovery_prefix = "homeassistant" + publisher = MessageCapturingConsolePublisher(config) + vin_info = VinInfo() + vin_info.vin = VIN + vin_info.series = "EH32 S" + vin_info.modelName = "MG4 Electric" + vin_info.modelYear = "2022" + configs = [ + VehicleModelConfiguration("BATTERY", "BATTERY", "1"), + VehicleModelConfiguration("BType", "Battery", "1"), + VehicleModelConfiguration( + "S35", "Sunroof", "1" if has_sunroof else "0" + ), + ] + vin_info.vehicleModelConfiguration = configs + vehicle_info = VehicleInfo(vin_info, None) + account_prefix = f"/vehicles/{VIN}" + scheduler = BlockingScheduler() + vehicle_state = VehicleState(publisher, scheduler, account_prefix, vehicle_info) + # Make vehicle state complete so discovery publishes + vehicle_state.refresh_period_active = 30 + vehicle_state.refresh_period_inactive = 120 + vehicle_state.refresh_period_after_shutdown = 60 + vehicle_state.refresh_period_inactive_grace = 600 + vehicle_state.refresh_mode = "periodic" + discovery = HomeAssistantDiscovery(vehicle_state, vehicle_info, config) + return discovery, publisher + + def _discovery_topics(self, publisher: MessageCapturingConsolePublisher) -> set[str]: + return set(publisher.map.keys()) + + def test_windows_published_as_binary_sensors(self) -> None: + discovery, publisher = self._make_discovery() + discovery.publish_ha_discovery_messages() + + topics = self._discovery_topics(publisher) + window_names = [ + "window_driver", + "window_passenger", + "window_rear_left", + "window_rear_right", + ] + for name in window_names: + binary_sensor_topic = ( + f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_{name}/config" + ) + assert binary_sensor_topic in topics, ( + f"Expected binary_sensor discovery for {name}" + ) + # Verify the payload has no command_topic (read-only) + payload = json.loads(publisher.map[binary_sensor_topic]) + assert "command_topic" not in payload + assert payload["device_class"] == "window" + + def test_windows_not_published_as_switches(self) -> None: + discovery, publisher = self._make_discovery() + discovery.publish_ha_discovery_messages() + + topics = self._discovery_topics(publisher) + window_names = [ + "window_driver", + "window_passenger", + "window_rear_left", + "window_rear_right", + ] + for name in window_names: + switch_topic = f"homeassistant/switch/{VIN}_mg/{VIN}_{name}/config" + if switch_topic in topics: + # Should be an empty unpublish message + assert publisher.map[switch_topic] == "", ( + f"Switch for {name} should be unpublished (empty payload)" + ) + + def test_sunroof_published_as_binary_sensor_when_supported(self) -> None: + discovery, publisher = self._make_discovery(has_sunroof=True) + discovery.publish_ha_discovery_messages() + + topics = self._discovery_topics(publisher) + sunroof_topic = ( + f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_sun_roof/config" + ) + assert sunroof_topic in topics + + def test_sunroof_unpublished_when_not_supported(self) -> None: + discovery, publisher = self._make_discovery(has_sunroof=False) + discovery.publish_ha_discovery_messages() + + topics = self._discovery_topics(publisher) + sunroof_binary = ( + f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_sun_roof/config" + ) + if sunroof_binary in topics: + assert publisher.map[sunroof_binary] == "" From 5bec9f7717bac1f0577e71ddebaf8f89199307b1 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 16:59:23 +0100 Subject: [PATCH 2/3] fix: add device_class to sunroof and strengthen test assertions - Add missing device_class="window" to sunroof binary_sensor - Make test assertions unconditional (no silent pass on regression) - Add payload verification to sunroof supported test - Verify sunroof switch is always unpublished Co-Authored-By: Claude Opus 4.6 (1M context) --- src/integrations/home_assistant/discovery.py | 4 +- tests/test_ha_discovery_windows.py | 61 ++++++++++---------- 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/integrations/home_assistant/discovery.py b/src/integrations/home_assistant/discovery.py index 0e052d57..2df89c6a 100644 --- a/src/integrations/home_assistant/discovery.py +++ b/src/integrations/home_assistant/discovery.py @@ -473,7 +473,9 @@ def __publish_windows_sensors(self) -> None: device_class="window", ) if self.__vin_info.has_sunroof: - self._publish_binary_sensor(mqtt_topics.WINDOWS_SUN_ROOF, "Sun roof") + self._publish_binary_sensor( + mqtt_topics.WINDOWS_SUN_ROOF, "Sun roof", device_class="window" + ) else: self.__unpublish_ha_discovery_message("binary_sensor", "Sun roof") diff --git a/tests/test_ha_discovery_windows.py b/tests/test_ha_discovery_windows.py index 5c8995fc..0a445a9b 100644 --- a/tests/test_ha_discovery_windows.py +++ b/tests/test_ha_discovery_windows.py @@ -17,6 +17,13 @@ from .common_mocks import VIN from .mocks import MessageCapturingConsolePublisher +WINDOW_NAMES = [ + "window_driver", + "window_passenger", + "window_rear_left", + "window_rear_right", +] + class TestHaDiscoveryWindows(unittest.TestCase): """Test that window entities are published as binary_sensors, not switches.""" @@ -54,68 +61,58 @@ def _make_discovery( discovery = HomeAssistantDiscovery(vehicle_state, vehicle_info, config) return discovery, publisher - def _discovery_topics(self, publisher: MessageCapturingConsolePublisher) -> set[str]: - return set(publisher.map.keys()) - def test_windows_published_as_binary_sensors(self) -> None: discovery, publisher = self._make_discovery() discovery.publish_ha_discovery_messages() - topics = self._discovery_topics(publisher) - window_names = [ - "window_driver", - "window_passenger", - "window_rear_left", - "window_rear_right", - ] - for name in window_names: + for name in WINDOW_NAMES: binary_sensor_topic = ( f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_{name}/config" ) - assert binary_sensor_topic in topics, ( + assert binary_sensor_topic in publisher.map, ( f"Expected binary_sensor discovery for {name}" ) - # Verify the payload has no command_topic (read-only) payload = json.loads(publisher.map[binary_sensor_topic]) assert "command_topic" not in payload assert payload["device_class"] == "window" - def test_windows_not_published_as_switches(self) -> None: + def test_old_window_switches_are_unpublished(self) -> None: discovery, publisher = self._make_discovery() discovery.publish_ha_discovery_messages() - topics = self._discovery_topics(publisher) - window_names = [ - "window_driver", - "window_passenger", - "window_rear_left", - "window_rear_right", - ] - for name in window_names: + for name in WINDOW_NAMES: switch_topic = f"homeassistant/switch/{VIN}_mg/{VIN}_{name}/config" - if switch_topic in topics: - # Should be an empty unpublish message - assert publisher.map[switch_topic] == "", ( - f"Switch for {name} should be unpublished (empty payload)" - ) + assert switch_topic in publisher.map, ( + f"Expected unpublish message for switch {name}" + ) + assert publisher.map[switch_topic] == "", ( + f"Switch for {name} should be unpublished (empty payload)" + ) + # Sunroof switch is also always unpublished + sunroof_switch = f"homeassistant/switch/{VIN}_mg/{VIN}_sun_roof/config" + assert sunroof_switch in publisher.map + assert publisher.map[sunroof_switch] == "" def test_sunroof_published_as_binary_sensor_when_supported(self) -> None: discovery, publisher = self._make_discovery(has_sunroof=True) discovery.publish_ha_discovery_messages() - topics = self._discovery_topics(publisher) sunroof_topic = ( f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_sun_roof/config" ) - assert sunroof_topic in topics + assert sunroof_topic in publisher.map + payload = json.loads(publisher.map[sunroof_topic]) + assert "command_topic" not in payload + assert payload["device_class"] == "window" def test_sunroof_unpublished_when_not_supported(self) -> None: discovery, publisher = self._make_discovery(has_sunroof=False) discovery.publish_ha_discovery_messages() - topics = self._discovery_topics(publisher) sunroof_binary = ( f"homeassistant/binary_sensor/{VIN}_mg/{VIN}_sun_roof/config" ) - if sunroof_binary in topics: - assert publisher.map[sunroof_binary] == "" + assert sunroof_binary in publisher.map, ( + "Expected unpublish message for binary_sensor Sun roof" + ) + assert publisher.map[sunroof_binary] == "" From 876cbaf5d6d79e7fd204828adadd64429301ad67 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 17:01:06 +0100 Subject: [PATCH 3/3] fix: use RefreshMode enum instead of string in test Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/test_ha_discovery_windows.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_ha_discovery_windows.py b/tests/test_ha_discovery_windows.py index 0a445a9b..8d2be70d 100644 --- a/tests/test_ha_discovery_windows.py +++ b/tests/test_ha_discovery_windows.py @@ -11,7 +11,7 @@ from configuration import Configuration from integrations.home_assistant.discovery import HomeAssistantDiscovery -from vehicle import VehicleState +from vehicle import RefreshMode, VehicleState from vehicle_info import VehicleInfo from .common_mocks import VIN @@ -57,7 +57,7 @@ def _make_discovery( vehicle_state.refresh_period_inactive = 120 vehicle_state.refresh_period_after_shutdown = 60 vehicle_state.refresh_period_inactive_grace = 600 - vehicle_state.refresh_mode = "periodic" + vehicle_state.refresh_mode = RefreshMode.PERIODIC discovery = HomeAssistantDiscovery(vehicle_state, vehicle_info, config) return discovery, publisher