Re-read a ZhongHong unit shortly after commanding it (#182074)

homeassistant/components/zhong_hong/climate.py

@@ -1,5 +1,6 @@
 """Support for ZhongHong HVAC Controller."""
 
+from collections.abc import Callable
 from typing import Any, override
 
 import probatio
@@ -251,53 +252,61 @@ class ZhongHongClimate(CoordinatorEntity[ZhongHongCoordinator], ClimateEntity):
         """Return the maximum temperature."""
         return self._device.max_temp
 
-    def _command(self, sent: bool, command: str) -> None:
-        """Fail if the command did not go out.
+    async def _command(
+        self, command: str, send: Callable[..., bool], *args: Any
+    ) -> None:
+        """Send a command to the unit, and re-read it shortly after.
 
-        Nothing is written here on success: the unit reports the state it
-        actually reached, which is not always the one it was asked for.
+        The library talks to the gateway over a blocking socket, so the call
+        goes to the executor. The unit reports the new state itself once it
+        acts on the command, so the re-read is only there for the reports that
+        go missing.
         """
-        if not sent:
+        if not await self.hass.async_add_executor_job(send, *args):
             raise _send_failed(command)
 
+        self.coordinator.async_schedule_readback()
+
     @override
-    def turn_on(self) -> None:
+    async def async_turn_on(self) -> None:
         """Turn on ac."""
-        self._command(self._device.turn_on(), "turn-on")
+        await self._command("turn-on", self._device.turn_on)
 
     @override
-    def turn_off(self) -> None:
+    async def async_turn_off(self) -> None:
         """Turn off ac."""
-        self._command(self._device.turn_off(), "turn-off")
+        await self._command("turn-off", self._device.turn_off)
 
     @override
-    def set_temperature(self, **kwargs: Any) -> None:
+    async def async_set_temperature(self, **kwargs: Any) -> None:
         """Set new target temperature."""
         if (temperature := kwargs.get(ATTR_TEMPERATURE)) is not None:
-            self._command(self._device.set_temperature(temperature), "temperature")
+            await self._command(
+                "temperature", self._device.set_temperature, temperature
+            )
 
         if (operation_mode := kwargs.get(ATTR_HVAC_MODE)) is not None:
-            self.set_hvac_mode(operation_mode)
+            await self.async_set_hvac_mode(operation_mode)
 
     @override
-    def set_hvac_mode(self, hvac_mode: HVACMode) -> None:
+    async def async_set_hvac_mode(self, hvac_mode: HVACMode) -> None:
         """Set new target operation mode."""
         if hvac_mode == HVACMode.OFF:
             if self.is_on:
-                self.turn_off()
+                await self.async_turn_off()
             return
 
         if not self.is_on:
-            self.turn_on()
+            await self.async_turn_on()
 
-        self._command(self._device.set_operation_mode(hvac_mode.upper()), "mode")
+        await self._command("mode", self._device.set_operation_mode, hvac_mode.upper())
 
     @override
-    def set_fan_mode(self, fan_mode: str) -> None:
+    async def async_set_fan_mode(self, fan_mode: str) -> None:
         """Set new target fan mode."""
         mapped_mode = FAN_MODE_MAP.get(fan_mode)
         if not mapped_mode:
             LOGGER.error("Unsupported fan mode: %s", fan_mode)
             return
 
-        self._command(self._device.set_fan_mode(mapped_mode), "fan")
+        await self._command("fan", self._device.set_fan_mode, mapped_mode)

homeassistant/components/zhong_hong/coordinator.py

@@ -1,18 +1,27 @@
 """Coordinator for the ZhongHong integration."""
 
 from dataclasses import dataclass
-from typing import override
+from datetime import datetime
+from typing import Final, 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.core import CALLBACK_TYPE, HomeAssistant, callback
+from homeassistant.helpers.event import async_call_later
 from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed
 
 from .const import LOGGER, SCAN_INTERVAL
 
+# A unit acts on a command and then reports the new state unprompted. This is
+# how long to wait before asking for it anyway, to cover the reports that never
+# arrive. Ten runs against a Haier unit took between one and 3.4 seconds to
+# act, so this sits past the slowest of them: asking before the unit has moved
+# would read back the state the command was meant to change.
+READBACK_DELAY: Final = 5
+
 type DeviceAddress = tuple[int, int]
 
 
@@ -81,10 +90,47 @@ class ZhongHongCoordinator(DataUpdateCoordinator[None]):
             update_interval=SCAN_INTERVAL,
         )
         self.hub = hub
+        self._readback_cancel: CALLBACK_TYPE | None = None
 
         for device in devices.values():
             device.register_update_callback(self._handle_device_update)
 
+    @callback
+    def async_schedule_readback(self) -> None:
+        """Re-read the gateway shortly after it has been commanded.
+
+        A unit takes a second or three to act on a command, and the gateway
+        pushes the new state once it has. That push is the only thing the
+        state comes from, so if it goes missing the entity keeps showing what
+        the unit was doing before, until the next poll a minute later. Asking
+        again a few seconds in costs one round trip and closes that window.
+        """
+        # A command sits in the executor while it is sent, and the entry can
+        # be unloaded in the meantime, so this can be reached afterwards.
+        # Scheduling then would put back the timer the shutdown has just
+        # taken away.
+        if self._shutdown_requested:
+            return
+
+        if self._readback_cancel is not None:
+            self._readback_cancel()
+
+        @callback
+        def _readback(_now: datetime) -> None:
+            self._readback_cancel = None
+            # Refreshed rather than requested: a request goes through the
+            # coordinator's debouncer, whose cooldown is twice this delay, so
+            # a command given shortly after a re-read would have its own one
+            # held back past the point the unit has acted. The timer above is
+            # the rate limit this needs.
+            self.config_entry.async_create_background_task(
+                self.hass,
+                self.async_refresh(),
+                name=f"{self.name} readback",
+            )
+
+        self._readback_cancel = async_call_later(self.hass, READBACK_DELAY, _readback)
+
     def _handle_device_update(self, device: ZhongHongHVAC) -> None:
         """Handle a state push from the gateway.
 
@@ -107,3 +153,14 @@ class ZhongHongCoordinator(DataUpdateCoordinator[None]):
 
         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}")
+
+    @override
+    async def async_shutdown(self) -> None:
+        """Drop the pending re-read, which would outlive the entry."""
+        # Shutting down first, so that anything on its way here from the
+        # executor finds the door already closed.
+        await super().async_shutdown()
+
+        if self._readback_cancel is not None:
+            self._readback_cancel()
+            self._readback_cancel = None

tests/components/zhong_hong/conftest.py

@@ -1,6 +1,7 @@
 """Common fixtures for the ZhongHong tests."""
 
 from collections.abc import Callable, Generator
+import threading
 from unittest.mock import AsyncMock, patch
 
 import pytest
@@ -77,6 +78,10 @@ class FakeGateway:
 
         self.send_result = True
         self.send_results: list[bool] = []
+        # Set to hold a command inside the executor, so that a test can
+        # have one still on its way out while something else happens.
+        self.send_gate: threading.Event | None = None
+        self.send_entered = threading.Event()
         self.query_all_status_result = True
         # The wire names of the speeds the units behind this gateway have.
         self.supported_fan_modes = {mode.name for mode in StatusFanMode}
@@ -138,6 +143,9 @@ class FakeGateway:
         let one command succeed and the next one fail.
         """
         self.send_calls += 1
+        if self.send_gate is not None:
+            self.send_entered.set()
+            self.send_gate.wait(timeout=10)
 
         header = ac_data.header
         if header.func_code is FuncCode.CTL_FAN_MODE:

tests/components/zhong_hong/test_climate.py

@@ -1,6 +1,7 @@
 """Test the zhong_hong climate platform."""
 
 from datetime import timedelta
+import threading
 
 from freezegun.api import FrozenDateTimeFactory
 import pytest
@@ -28,6 +29,7 @@ from homeassistant.components.zhong_hong.const import (
     FAN_MEDIUM_HIGH,
     FAN_MEDIUM_LOW,
 )
+from homeassistant.config_entries import ConfigEntryState
 from homeassistant.const import (
     ATTR_ENTITY_ID,
     ATTR_TEMPERATURE,
@@ -45,9 +47,11 @@ from .conftest import DEVICE_ADDRESS, ENTITY_ID, FakeGateway, build_status
 
 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.
+# Spelled out instead of importing SCAN_INTERVAL and READBACK_DELAY, so that
+# changing either in the integration makes these tests fail instead of
+# following along.
 POLL_INTERVAL = timedelta(seconds=60)
+READBACK_DELAY = timedelta(seconds=5)
 
 
 async def test_entity_registration(
@@ -395,6 +399,180 @@ async def test_device_address_is_used_for_the_entity(
     assert hass.states.get("climate.ac_1_2") is not None
 
 
+async def test_a_command_is_read_back(
+    hass: HomeAssistant,
+    mock_gateway: FakeGateway,
+    mock_config_entry: MockConfigEntry,
+    freezer: FrozenDateTimeFactory,
+) -> None:
+    """Test the gateway is re-read shortly after being commanded.
+
+    A unit reports the new state itself once it acts, so this only matters
+    for the reports that go missing: without it the entity would show the old
+    state until the next scheduled poll.
+    """
+    await setup_integration(hass, mock_config_entry)
+
+    assert mock_gateway.query_all_status_calls == 1
+
+    await hass.services.async_call(
+        CLIMATE_DOMAIN,
+        SERVICE_SET_FAN_MODE,
+        {ATTR_ENTITY_ID: ENTITY_ID, ATTR_FAN_MODE: FAN_HIGH},
+        blocking=True,
+    )
+
+    assert mock_gateway.query_all_status_calls == 1
+
+    freezer.tick(READBACK_DELAY)
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done(wait_background_tasks=True)
+
+    assert mock_gateway.query_all_status_calls == 2
+
+
+async def test_commands_in_a_row_are_read_back_once(
+    hass: HomeAssistant,
+    mock_gateway: FakeGateway,
+    mock_config_entry: MockConfigEntry,
+    freezer: FrozenDateTimeFactory,
+) -> None:
+    """Test a burst of commands does not queue up a re-read for each one.
+
+    Each command cancels the re-read the one before it scheduled, so only the
+    last should survive to query the gateway. The commands are spread out
+    rather than sent at once to put each re-read at its own moment: one left
+    over from an earlier command then comes due on its own, where the
+    assertion below catches it, instead of landing on the same tick as the
+    survivor and passing for it.
+    """
+    await setup_integration(hass, mock_config_entry)
+
+    assert mock_gateway.query_all_status_calls == 1
+
+    commands = (FAN_HIGH, FAN_LOW, FAN_MIDDLE)
+    for fan_mode in commands:
+        await hass.services.async_call(
+            CLIMATE_DOMAIN,
+            SERVICE_SET_FAN_MODE,
+            {ATTR_ENTITY_ID: ENTITY_ID, ATTR_FAN_MODE: fan_mode},
+            blocking=True,
+        )
+        freezer.tick(timedelta(seconds=1))
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done(wait_background_tasks=True)
+
+    # The clock stands a second per command past the first of them. Take it
+    # the rest of the way to where that command's own re-read would have come
+    # due: one left over from it fires here, and the commands after it have
+    # not pushed their re-read this far forward.
+    freezer.tick(READBACK_DELAY - timedelta(seconds=len(commands)))
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done(wait_background_tasks=True)
+
+    assert mock_gateway.query_all_status_calls == 1
+
+    freezer.tick(READBACK_DELAY)
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done(wait_background_tasks=True)
+
+    assert mock_gateway.query_all_status_calls == 2
+
+
+async def test_a_later_command_is_read_back_on_time(
+    hass: HomeAssistant,
+    mock_gateway: FakeGateway,
+    mock_config_entry: MockConfigEntry,
+    freezer: FrozenDateTimeFactory,
+) -> None:
+    """Test a command soon after a re-read gets its own re-read on time.
+
+    The delay is chosen to sit past the time a unit takes to act. A re-read
+    held back beyond it would read the state the command was meant to change.
+    """
+    await setup_integration(hass, mock_config_entry)
+
+    assert mock_gateway.query_all_status_calls == 1
+
+    for _ in range(2):
+        await hass.services.async_call(
+            CLIMATE_DOMAIN,
+            SERVICE_SET_FAN_MODE,
+            {ATTR_ENTITY_ID: ENTITY_ID, ATTR_FAN_MODE: FAN_HIGH},
+            blocking=True,
+        )
+        freezer.tick(READBACK_DELAY)
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done(wait_background_tasks=True)
+
+    assert mock_gateway.query_all_status_calls == 3
+
+
+@pytest.mark.parametrize("expected_lingering_timers", [False])
+async def test_a_pending_readback_does_not_outlive_the_entry(
+    hass: HomeAssistant,
+    mock_gateway: FakeGateway,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test unloading the entry drops a re-read that was still to come.
+
+    The re-read sits on a timer of its own, which nothing else knows to
+    cancel. A leftover one does not reach the gateway — the coordinator it
+    would refresh has been shut down by then — but it stays on the loop, and
+    holds on to that coordinator until it comes due. So what this test looks
+    at is the timer rather than the gateway: the harness is asked not to
+    forgive a lingering one, and the test ends while it would still be there.
+    """
+    await setup_integration(hass, mock_config_entry)
+
+    await hass.services.async_call(
+        CLIMATE_DOMAIN,
+        SERVICE_SET_FAN_MODE,
+        {ATTR_ENTITY_ID: ENTITY_ID, ATTR_FAN_MODE: FAN_HIGH},
+        blocking=True,
+    )
+
+    assert await hass.config_entries.async_unload(mock_config_entry.entry_id)
+    await hass.async_block_till_done()
+
+    assert mock_config_entry.state is ConfigEntryState.NOT_LOADED
+
+
+@pytest.mark.parametrize("expected_lingering_timers", [False])
+async def test_a_command_landing_after_the_unload_schedules_nothing(
+    hass: HomeAssistant,
+    mock_gateway: FakeGateway,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test a command still in flight at unload does not leave a re-read behind.
+
+    A command sits in the executor while it is sent, so one held up there is
+    still on its way out when the entry is taken down, and asks for its
+    re-read once the unload has already been through and found nothing to
+    cancel. Asking then would put back the timer the unload has just taken
+    away, and it would outlive the entry.
+    """
+    await setup_integration(hass, mock_config_entry)
+
+    mock_gateway.send_gate = threading.Event()
+    command = hass.async_create_task(
+        hass.services.async_call(
+            CLIMATE_DOMAIN,
+            SERVICE_SET_FAN_MODE,
+            {ATTR_ENTITY_ID: ENTITY_ID, ATTR_FAN_MODE: FAN_HIGH},
+            blocking=True,
+        )
+    )
+
+    assert await hass.async_add_executor_job(mock_gateway.send_entered.wait, 10)
+
+    assert await hass.config_entries.async_unload(mock_config_entry.entry_id)
+
+    mock_gateway.send_gate.set()
+    await command
+    await hass.async_block_till_done()
+
+
 async def test_every_fan_mode_has_a_name(
     hass: HomeAssistant,
     entity_registry: er.EntityRegistry,