diff --git a/homeassistant/components/zhong_hong/__init__.py b/homeassistant/components/zhong_hong/__init__.py index 2533d981f48f..5aadcae03984 100644 --- a/homeassistant/components/zhong_hong/__init__.py +++ b/homeassistant/components/zhong_hong/__init__.py @@ -1,56 +1,28 @@ """The ZhongHong HVAC integration.""" from collections.abc import Iterable -from dataclasses import dataclass from zhong_hong_hvac.hub import ZhongHongGateway from zhong_hong_hvac.hvac import HVAC as ZhongHongHVAC from homeassistant.components.climate import DOMAIN as CLIMATE_DOMAIN -from homeassistant.config_entries import ConfigEntry from homeassistant.const import CONF_HOST, CONF_PORT, Platform from homeassistant.core import HomeAssistant, callback from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers import entity_registry as er from .const import CONF_GATEWAY_ADDRESS, DOMAIN +from .coordinator import ( + DeviceAddress, + ZhongHongConfigEntry, + ZhongHongCoordinator, + ZhongHongData, + device_unique_id, + legacy_device_unique_id, +) PLATFORMS: list[Platform] = [Platform.CLIMATE] -type DeviceAddress = tuple[int, int] - - -@dataclass -class ZhongHongData: - """What a loaded config entry holds. - - The air conditioners are found once, when the entry is set up: discovery - needs the listener thread stopped, so the gateway cannot be asked again - while the entry is running. - """ - - hub: ZhongHongGateway - devices: dict[DeviceAddress, ZhongHongHVAC] - - -type ZhongHongConfigEntry = ConfigEntry[ZhongHongData] - - -def device_unique_id(entry: ZhongHongConfigEntry, address: DeviceAddress) -> str: - """Return the unique ID of the air conditioner at an address.""" - return f"{entry.entry_id}_{address[0]}_{address[1]}" - - -def legacy_device_unique_id(address: DeviceAddress) -> str: - """Return the identifier the YAML platform gave the air conditioner. - - It carried only the address on the bus, so two gateways with an air - conditioner at the same address, which `(1, 1)` commonly is, produced the - same one and the second entity was dropped. Entities are moved off it on - setup; it is still needed to find them. - """ - return f"zhong_hong_hvac_{address[0]}_{address[1]}" - @callback def _async_migrate_unique_ids( @@ -93,8 +65,8 @@ def _connect(hub: ZhongHongGateway) -> dict[DeviceAddress, ZhongHongHVAC]: """Ask the gateway what is on its bus, then start listening to it. Discovery has to finish before the listener thread starts, because the two - read from the same socket. All of it blocks, so it runs as one executor - job rather than hopping back to the event loop in between. + read from the same socket. Both block, so this runs as one executor job + rather than hopping back to the event loop in between. """ addresses = hub.discovery_ac() if not addresses: @@ -105,12 +77,6 @@ def _connect(hub: ZhongHongGateway) -> dict[DeviceAddress, ZhongHongHVAC]: devices = {address: ZhongHongHVAC(hub, *address) for address in addresses} hub.start_listen() - - # The gateway reports a unit only when it changes, so without asking once - # here the entities would have no state until someone touched a unit. - if not hub.query_all_status(): - raise OSError(f"The gateway at {hub.ip_addr} did not answer the first query") - return devices @@ -155,7 +121,10 @@ async def async_setup_entry(hass: HomeAssistant, entry: ZhongHongConfigEntry) -> devices = await _async_connect(hass, hub) - entry.runtime_data = ZhongHongData(hub, devices) + coordinator = ZhongHongCoordinator(hass, entry, hub, devices) + await coordinator.async_config_entry_first_refresh() + + entry.runtime_data = ZhongHongData(hub, devices, coordinator) _async_migrate_unique_ids(hass, entry, devices) await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) return True diff --git a/homeassistant/components/zhong_hong/climate.py b/homeassistant/components/zhong_hong/climate.py index be5566e467e3..3af5dff40ccb 100644 --- a/homeassistant/components/zhong_hong/climate.py +++ b/homeassistant/components/zhong_hong/climate.py @@ -28,8 +28,8 @@ from homeassistant.helpers.entity_platform import ( AddEntitiesCallback, ) from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType +from homeassistant.helpers.update_coordinator import CoordinatorEntity -from . import DeviceAddress, ZhongHongConfigEntry, device_unique_id from .const import ( ALL_FAN_MODES, BREAKS_IN_HA_VERSION, @@ -42,6 +42,12 @@ from .const import ( INTEGRATION_TITLE, LOGGER, ) +from .coordinator import ( + DeviceAddress, + ZhongHongConfigEntry, + ZhongHongCoordinator, + device_unique_id, +) # The gateway serializes everything onto a single socket, so there is nothing # to gain from issuing commands in parallel. @@ -154,13 +160,14 @@ async def async_setup_entry( async_add_entities: AddConfigEntryEntitiesCallback, ) -> None: """Set up the ZhongHong climate entities from a config entry.""" + data = entry.runtime_data async_add_entities( - ZhongHongClimate(entry, address, device) - for address, device in entry.runtime_data.devices.items() + ZhongHongClimate(data.coordinator, entry, address, device) + for address, device in data.devices.items() ) -class ZhongHongClimate(ClimateEntity): +class ZhongHongClimate(CoordinatorEntity[ZhongHongCoordinator], ClimateEntity): """Representation of an air conditioner behind a ZhongHong gateway.""" _attr_fan_modes = ALL_FAN_MODES @@ -171,9 +178,6 @@ class ZhongHongClimate(ClimateEntity): HVACMode.FAN_ONLY, HVACMode.OFF, ] - # The gateway reports every change on its own socket, so there is nothing - # to poll for. - _attr_should_poll = False _attr_supported_features = ( ClimateEntityFeature.TARGET_TEMPERATURE | ClimateEntityFeature.FAN_MODE @@ -185,30 +189,18 @@ class ZhongHongClimate(ClimateEntity): def __init__( self, + coordinator: ZhongHongCoordinator, entry: ZhongHongConfigEntry, address: DeviceAddress, device: ZhongHongHVAC, ) -> None: """Set up a ZhongHong climate device.""" + super().__init__(coordinator) self._device = device addr_out, addr_in = address self._attr_name = f"AC {addr_out}-{addr_in}" self._attr_unique_id = device_unique_id(entry, address) - @override - async def async_added_to_hass(self) -> None: - """Take the state the gateway pushes for this air conditioner.""" - self._device.register_update_callback(self._handle_device_update) - - def _handle_device_update(self, device: ZhongHongHVAC) -> None: - """Handle a state push from the gateway. - - The library writes the new state into the device object before calling - this, and it does so on its own listener thread, so all that is left is - to ask for the entity to be written from that thread. - """ - self.schedule_update_ha_state() - @property @override def current_temperature(self) -> float | None: diff --git a/homeassistant/components/zhong_hong/config_flow.py b/homeassistant/components/zhong_hong/config_flow.py index e3d34b254cfd..485b88a56fc4 100644 --- a/homeassistant/components/zhong_hong/config_flow.py +++ b/homeassistant/components/zhong_hong/config_flow.py @@ -75,8 +75,8 @@ class ZhongHongConfigFlow(ConfigFlow, domain=DOMAIN): if user_input is not None: # A gateway is identified by the endpoint it is reached on and the - # address it answers to, all three of which are needed to talk - # to it. + # address it answers to, all three of which the coordinator needs + # to talk to it. self._async_abort_entries_match( { CONF_HOST: user_input[CONF_HOST], diff --git a/homeassistant/components/zhong_hong/const.py b/homeassistant/components/zhong_hong/const.py index dc5b714626bf..2adf190521f6 100644 --- a/homeassistant/components/zhong_hong/const.py +++ b/homeassistant/components/zhong_hong/const.py @@ -1,5 +1,6 @@ """Constants for the ZhongHong integration.""" +from datetime import timedelta import logging from typing import Final @@ -38,3 +39,7 @@ FAN_MODE_REVERSE_MAP: Final = {v: k for k, v in FAN_MODE_MAP.items()} DEFAULT_PORT: Final = 9999 DEFAULT_GATEWAY_ADDRESS: Final = 1 + +# The gateway pushes state changes, so polling only has to cover pushes that +# were missed while the connection was down. +SCAN_INTERVAL: Final = timedelta(seconds=60) diff --git a/homeassistant/components/zhong_hong/coordinator.py b/homeassistant/components/zhong_hong/coordinator.py new file mode 100644 index 000000000000..ec96c8ef5bff --- /dev/null +++ b/homeassistant/components/zhong_hong/coordinator.py @@ -0,0 +1,109 @@ +"""Coordinator for the ZhongHong integration.""" + +from dataclasses import dataclass +from typing import override + +from zhong_hong_hvac.hub import ZhongHongGateway +from zhong_hong_hvac.hvac import HVAC as ZhongHongHVAC + +from homeassistant.config_entries import ConfigEntry +from homeassistant.const import CONF_HOST +from homeassistant.core import HomeAssistant +from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed + +from .const import LOGGER, SCAN_INTERVAL + +type DeviceAddress = tuple[int, int] + + +@dataclass +class ZhongHongData: + """What a loaded config entry holds. + + The air conditioners are found once, when the entry is set up: discovery + needs the listener thread stopped, so the gateway cannot be asked again + while the entry is running. They belong to the entry that found them + rather than to whatever happens to be updating them. + """ + + hub: ZhongHongGateway + devices: dict[DeviceAddress, ZhongHongHVAC] + coordinator: ZhongHongCoordinator + + +type ZhongHongConfigEntry = ConfigEntry[ZhongHongData] + + +def device_unique_id(entry: ZhongHongConfigEntry, address: DeviceAddress) -> str: + """Return the unique ID of the air conditioner at an address.""" + return f"{entry.entry_id}_{address[0]}_{address[1]}" + + +def legacy_device_unique_id(address: DeviceAddress) -> str: + """Return the identifier the YAML platform gave the air conditioner. + + It carried only the address on the bus, so two gateways with an air + conditioner at the same address, which `(1, 1)` commonly is, produced the + same one and the second entity was dropped. Entities are moved off it on + setup; it is still needed to find them. + """ + return f"zhong_hong_hvac_{address[0]}_{address[1]}" + + +class ZhongHongCoordinator(DataUpdateCoordinator[None]): + """Tell the entities when to look at their air conditioner again. + + There is no data to hand out. The gateway pushes state on its own socket + and the library writes it into the device objects in place, so all this + has to carry is that something changed; the entities read the device they + were given. Polling remains as a fallback for pushes missed while the + connection was down, and is what decides availability. + + The connection and the devices belong to the config entry, which hands + them over already listening. + """ + + config_entry: ZhongHongConfigEntry + + def __init__( + self, + hass: HomeAssistant, + config_entry: ZhongHongConfigEntry, + hub: ZhongHongGateway, + devices: dict[DeviceAddress, ZhongHongHVAC], + ) -> None: + """Initialize the coordinator.""" + super().__init__( + hass, + LOGGER, + config_entry=config_entry, + name=config_entry.data[CONF_HOST], + update_interval=SCAN_INTERVAL, + ) + self.hub = hub + + for device in devices.values(): + device.register_update_callback(self._handle_device_update) + + def _handle_device_update(self, device: ZhongHongHVAC) -> None: + """Handle a state push from the gateway. + + Called on the library's listener thread, so the update has to be handed + back to the event loop before touching any coordinator state. + + The listeners are told directly rather than through + `async_set_updated_data`, which would also push the next poll back a + full interval. A push says one unit changed, not that every unit was + accounted for, so a gateway with something on it that reports often + would keep postponing the poll the units that went quiet depend on. + """ + self.hass.loop.call_soon_threadsafe(self.async_update_listeners) + + @override + async def _async_update_data(self) -> None: + """Ask the gateway to re-send the state of every device.""" + if not self.hub.connected: + raise UpdateFailed(f"Lost connection to the gateway at {self.hub.ip_addr}") + + if not await self.hass.async_add_executor_job(self.hub.query_all_status): + raise UpdateFailed(f"Failed to query the gateway at {self.hub.ip_addr}") diff --git a/tests/components/zhong_hong/test_climate.py b/tests/components/zhong_hong/test_climate.py index 4fe7bc4862ea..d6f6d72ff0cf 100644 --- a/tests/components/zhong_hong/test_climate.py +++ b/tests/components/zhong_hong/test_climate.py @@ -1,5 +1,8 @@ """Test the zhong_hong climate platform.""" +from datetime import timedelta + +from freezegun.api import FrozenDateTimeFactory import pytest from zhong_hong_hvac.protocol import StatusFanMode, StatusOperation, StatusSwitch @@ -22,6 +25,7 @@ from homeassistant.const import ( ATTR_ENTITY_ID, ATTR_TEMPERATURE, STATE_OFF, + STATE_UNAVAILABLE, STATE_UNKNOWN, ) from homeassistant.core import HomeAssistant @@ -31,7 +35,11 @@ from homeassistant.helpers import entity_registry as er from . import setup_integration from .conftest import DEVICE_ADDRESS, ENTITY_ID, FakeGateway, build_status -from tests.common import MockConfigEntry +from tests.common import MockConfigEntry, async_fire_time_changed + +# Spelled out instead of importing SCAN_INTERVAL, so that changing it in the +# integration makes these tests fail instead of following along. +POLL_INTERVAL = timedelta(seconds=60) async def test_entity_registration( @@ -100,6 +108,98 @@ async def test_push_of_an_unknown_operation( assert hass.states.get(ENTITY_ID).state == STATE_UNKNOWN +async def test_scheduled_poll_queries_the_gateway( + hass: HomeAssistant, + mock_gateway: FakeGateway, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, +) -> None: + """Test the gateway is polled for the pushes that were missed.""" + await setup_integration(hass, mock_config_entry) + + assert mock_gateway.query_all_status_calls == 1 + + freezer.tick(POLL_INTERVAL) + async_fire_time_changed(hass) + # The coordinator refreshes in a background task. + await hass.async_block_till_done(wait_background_tasks=True) + + assert mock_gateway.query_all_status_calls == 2 + + +async def test_a_push_does_not_postpone_the_poll( + hass: HomeAssistant, + mock_gateway: FakeGateway, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, +) -> None: + """Test a unit that reports often does not hold off the poll. + + The poll is there for the units whose reports went missing, and a gateway + usually has more than one air conditioner on it. Were a push to put the + next poll a full interval out, one unit reporting every few seconds would + be enough to keep the others from ever being asked about. + """ + await setup_integration(hass, mock_config_entry) + + assert mock_gateway.query_all_status_calls == 1 + + freezer.tick(POLL_INTERVAL / 2) + mock_gateway.push_status(build_status()) + await hass.async_block_till_done() + + freezer.tick(POLL_INTERVAL / 2) + async_fire_time_changed(hass) + # The coordinator refreshes in a background task. + await hass.async_block_till_done(wait_background_tasks=True) + + assert mock_gateway.query_all_status_calls == 2 + + +async def test_unavailable_when_the_gateway_connection_drops( + hass: HomeAssistant, + mock_gateway: FakeGateway, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, +) -> None: + """Test a dropped gateway connection makes the entity unavailable.""" + await setup_integration(hass, mock_config_entry) + + assert hass.states.get(ENTITY_ID).state == STATE_OFF + + mock_gateway.connected = False + freezer.tick(POLL_INTERVAL) + async_fire_time_changed(hass) + # The coordinator refreshes in a background task. + await hass.async_block_till_done(wait_background_tasks=True) + + assert hass.states.get(ENTITY_ID).state == STATE_UNAVAILABLE + # The gateway is not talked to while the connection is known to be down. + assert mock_gateway.query_all_status_calls == 1 + + +async def test_unavailable_when_the_query_cannot_be_sent( + hass: HomeAssistant, + mock_gateway: FakeGateway, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, +) -> None: + """Test a poll that cannot be sent makes the entity unavailable. + + A gateway that goes quiet without dropping the connection is caught by + the connection going stale instead, which is what `connected` reports. + """ + await setup_integration(hass, mock_config_entry) + + mock_gateway.query_all_status_result = False + freezer.tick(POLL_INTERVAL) + async_fire_time_changed(hass) + # The coordinator refreshes in a background task. + await hass.async_block_till_done(wait_background_tasks=True) + + assert hass.states.get(ENTITY_ID).state == STATE_UNAVAILABLE + + async def test_turn_on_success( hass: HomeAssistant, mock_gateway: FakeGateway, mock_config_entry: MockConfigEntry ) -> None: diff --git a/tests/components/zhong_hong/test_init.py b/tests/components/zhong_hong/test_init.py index 300890f4f26b..f40cd38a2f09 100644 --- a/tests/components/zhong_hong/test_init.py +++ b/tests/components/zhong_hong/test_init.py @@ -63,7 +63,7 @@ async def test_setup_retries_without_devices( assert mock_gateway.start_listen_calls == 0 -async def test_setup_stops_listener_when_the_first_query_fails( +async def test_setup_stops_listener_when_first_refresh_fails( hass: HomeAssistant, mock_gateway: FakeGateway, mock_config_entry: MockConfigEntry ) -> None: """Test the listener is stopped when the entry fails after it was started. @@ -86,7 +86,8 @@ async def test_setup_asks_for_the_state_of_every_device( """Test the entities have state without waiting for someone to touch a unit. The gateway reports a unit when it changes and not before, so the first - state of each one has to be asked for. + state of each one has to be asked for. That is what the coordinator's + first refresh is doing. """ await setup_integration(hass, mock_config_entry)