From 5ba6d59c89c4668456c5540ea87de3990cda0333 Mon Sep 17 00:00:00 2001 From: "ruohan.chen" Date: Mon, 14 Sep 2026 22:52:57 +0800 Subject: [PATCH] Re-read a ZhongHong unit shortly after commanding it (#182074) --- .../components/zhong_hong/climate.py | 45 +++-- .../components/zhong_hong/coordinator.py | 61 +++++- tests/components/zhong_hong/conftest.py | 8 + tests/components/zhong_hong/test_climate.py | 182 +++++++++++++++++- 4 files changed, 274 insertions(+), 22 deletions(-) diff --git a/homeassistant/components/zhong_hong/climate.py b/homeassistant/components/zhong_hong/climate.py index adf252e28ec4..f2f3fa5ea9be 100644 --- a/homeassistant/components/zhong_hong/climate.py +++ b/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) diff --git a/homeassistant/components/zhong_hong/coordinator.py b/homeassistant/components/zhong_hong/coordinator.py index ec96c8ef5bff..5e7a26540684 100644 --- a/homeassistant/components/zhong_hong/coordinator.py +++ b/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 diff --git a/tests/components/zhong_hong/conftest.py b/tests/components/zhong_hong/conftest.py index dc70046ab53b..4c1cc88498fb 100644 --- a/tests/components/zhong_hong/conftest.py +++ b/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: diff --git a/tests/components/zhong_hong/test_climate.py b/tests/components/zhong_hong/test_climate.py index 45a19de0b4d7..94cdc16c49f4 100644 --- a/tests/components/zhong_hong/test_climate.py +++ b/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,