Give ZhongHong a coordinator for polling and availability (#181897)

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

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:

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],

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)

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}")

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:

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)