From 62c3cc8871ddb4312c006353fa17d1d7fb92a6e5 Mon Sep 17 00:00:00 2001 From: Ronald van der Meer Date: Tue, 8 Sep 2026 11:04:05 +0200 Subject: [PATCH] Fix delayed Duco node updates after ventilation changes (#181528) --- homeassistant/components/duco/coordinator.py | 55 +++- homeassistant/components/duco/fan.py | 7 +- homeassistant/components/duco/select.py | 8 +- tests/components/duco/conftest.py | 9 + tests/components/duco/test_fan.py | 6 +- tests/components/duco/test_select.py | 324 +++++++++++++++++-- 6 files changed, 360 insertions(+), 49 deletions(-) diff --git a/homeassistant/components/duco/coordinator.py b/homeassistant/components/duco/coordinator.py index 899ad0e5a2f9..d164482ba855 100644 --- a/homeassistant/components/duco/coordinator.py +++ b/homeassistant/components/duco/coordinator.py @@ -1,11 +1,12 @@ """Data update coordinator for the Duco integration.""" +import asyncio from contextlib import suppress from dataclasses import dataclass, replace import logging from typing import cast, override -from duco_connectivity import DucoClient +from duco_connectivity import DucoClient, VentilationState from duco_connectivity.exceptions import ( DucoConnectionError, DucoError, @@ -52,6 +53,9 @@ class DucoCoordinator(DataUpdateCoordinator[DucoData]): config_entry: DucoConfigEntry board_info: BoardInfo _configured_node_names: dict[int, str] + _full_update_failed: bool + _node_update_errors: dict[int, DucoError] + _request_lock: asyncio.Lock def __init__( self, @@ -69,6 +73,46 @@ class DucoCoordinator(DataUpdateCoordinator[DucoData]): ) self.client = client self._configured_node_names = {} + self._full_update_failed = False + self._node_update_errors = {} + self._request_lock = asyncio.Lock() + + async def async_set_ventilation_state( + self, node_id: int, state: str | VentilationState + ) -> None: + """Set and refresh a node's ventilation state.""" + # Keep an older read from publishing after this write completes. + async with self._request_lock: + await self.client.async_set_ventilation_state(node_id, state) + await self._async_refresh_node(node_id) + + async def _async_refresh_node(self, node_id: int) -> None: + """Refresh one node while holding the request lock.""" + try: + node = await self.client.async_get_node_info(node_id) + except DucoError as err: + self._node_update_errors[node_id] = err + self.async_set_update_error(err) + return + + if current_node := self.data.nodes.get(node_id): + node = replace( + node, + general=replace( + node.general, + name=current_node.general.name, + ), + ) + + self._node_update_errors.pop(node_id, None) + self.data = replace( + self.data, + nodes={**self.data.nodes, node_id: node}, + ) + if not self._full_update_failed and not self._node_update_errors: + self.last_update_success = True + # A targeted readback must not postpone the periodic full refresh. + self.async_update_listeners() async def _async_load_node_names(self) -> None: """Load configured Duco node names during setup.""" @@ -117,6 +161,15 @@ class DucoCoordinator(DataUpdateCoordinator[DucoData]): @override async def _async_update_data(self) -> DucoData: """Fetch node data from the Duco box.""" + async with self._request_lock: + self._full_update_failed = True + data = await self._async_fetch_data() + self._full_update_failed = False + self._node_update_errors.clear() + return data + + async def _async_fetch_data(self) -> DucoData: + """Fetch node data while holding the request lock.""" try: nodes = await self.client.async_get_nodes() except DucoConnectionError as err: diff --git a/homeassistant/components/duco/fan.py b/homeassistant/components/duco/fan.py index 5c5bf3bc6314..a0406b1a34c6 100644 --- a/homeassistant/components/duco/fan.py +++ b/homeassistant/components/duco/fan.py @@ -122,11 +122,9 @@ class DucoVentilationFanEntity(DucoEntity, FanEntity): await self._async_set_state(state) async def _async_set_state(self, state: VentilationState) -> None: - """Send the ventilation state to the device and refresh coordinator.""" + """Set the ventilation state.""" try: - await self.coordinator.client.async_set_ventilation_state( - self._node_id, state - ) + await self.coordinator.async_set_ventilation_state(self._node_id, state) except DucoRateLimitError as err: _LOGGER.warning("Duco write rate limit exceeded for node %s", self._node_id) raise HomeAssistantError( @@ -138,4 +136,3 @@ class DucoVentilationFanEntity(DucoEntity, FanEntity): translation_domain=DOMAIN, translation_key="failed_to_set_state", ) from err - await self.coordinator.async_refresh() diff --git a/homeassistant/components/duco/select.py b/homeassistant/components/duco/select.py index e55a7ae3365f..e4acb6c67a92 100644 --- a/homeassistant/components/duco/select.py +++ b/homeassistant/components/duco/select.py @@ -129,9 +129,7 @@ class DucoVentilationStateSelect(DucoEntity, SelectEntity): try: # SelectEntity exposes string options, and passing the raw API value # through keeps newly added Duco states forward-compatible. - await self.coordinator.client.async_set_ventilation_state( - self._node_id, option - ) + await self.coordinator.async_set_ventilation_state(self._node_id, option) except DucoRateLimitError as err: _LOGGER.warning("Duco write rate limit exceeded for node %s", self._node_id) raise HomeAssistantError( @@ -143,7 +141,3 @@ class DucoVentilationStateSelect(DucoEntity, SelectEntity): translation_domain=DOMAIN, translation_key="failed_to_set_state", ) from err - - # Duco may normalize the requested action on readback, such as - # MAN1x2 -> MAN1 or AUTO -> CNT1, so refresh the authoritative state. - await self.coordinator.async_refresh() diff --git a/tests/components/duco/conftest.py b/tests/components/duco/conftest.py index d82b155ddc87..288c543fc69a 100644 --- a/tests/components/duco/conftest.py +++ b/tests/components/duco/conftest.py @@ -299,10 +299,19 @@ def mock_duco_client( ), ): client = mock_class.return_value + + def get_node_info(node_id: int) -> Node: + return next( + node + for node in client.async_get_nodes.return_value + if node.node_id == node_id + ) + client.async_get_api_info.return_value = mock_api_info client.async_get_board_info.return_value = mock_board_info client.async_get_lan_info.return_value = mock_lan_info client.async_get_nodes.return_value = mock_nodes + client.async_get_node_info.side_effect = get_node_info client.async_get_node_configs.return_value = node_configs_from_nodes(mock_nodes) client.async_get_node_actions.return_value = mock_node_actions client.async_get_time_filter_remaining.return_value = 180 diff --git a/tests/components/duco/test_fan.py b/tests/components/duco/test_fan.py index d5953deec559..2db0a6b49f6a 100644 --- a/tests/components/duco/test_fan.py +++ b/tests/components/duco/test_fan.py @@ -68,8 +68,6 @@ async def test_fan_set_state( expected_duco_state: str, ) -> None: """Test that fan service calls map to the correct Duco ventilation state.""" - mock_duco_client.async_set_ventilation_state = AsyncMock() - await hass.services.async_call( FAN_DOMAIN, service, @@ -77,9 +75,11 @@ async def test_fan_set_state( blocking=True, ) - mock_duco_client.async_set_ventilation_state.assert_called_once_with( + mock_duco_client.async_set_ventilation_state.assert_awaited_once_with( 1, expected_duco_state ) + mock_duco_client.async_get_node_info.assert_awaited_once_with(1) + assert mock_duco_client.async_get_nodes.await_count == 1 @pytest.mark.usefixtures("init_integration") diff --git a/tests/components/duco/test_select.py b/tests/components/duco/test_select.py index a20d281c57cb..9bfebe7e49bd 100644 --- a/tests/components/duco/test_select.py +++ b/tests/components/duco/test_select.py @@ -1,5 +1,6 @@ """Tests for the Duco select platform.""" +import asyncio from dataclasses import replace from unittest.mock import AsyncMock @@ -16,21 +17,33 @@ from duco_connectivity import ( NodeType, VentilationState, ) +from freezegun.api import FrozenDateTimeFactory import pytest +from homeassistant.components.duco.const import SCAN_INTERVAL +from homeassistant.components.fan import ( + ATTR_PERCENTAGE, + DOMAIN as FAN_DOMAIN, + SERVICE_SET_PERCENTAGE, +) from homeassistant.components.select import ( ATTR_OPTION, ATTR_OPTIONS, DOMAIN as SELECT_DOMAIN, SERVICE_SELECT_OPTION, ) -from homeassistant.const import ATTR_ENTITY_ID, STATE_UNKNOWN, Platform +from homeassistant.const import ( + ATTR_ENTITY_ID, + STATE_UNAVAILABLE, + STATE_UNKNOWN, + Platform, +) from homeassistant.core import HomeAssistant from homeassistant.exceptions import HomeAssistantError from . import setup_platform_integration -from tests.common import MockConfigEntry +from tests.common import MockConfigEntry, async_fire_time_changed _SELECT_ENTITY = "select.living_ventilation_state" _VALVE_SELECT_ENTITY = "select.bedroom_valve_ventilation_state" @@ -93,6 +106,35 @@ def _replace_node_state(node: Node, state: str | VentilationState | None) -> Nod return replace(node, ventilation=replace(node.ventilation, state=state)) +def _assert_select_state(hass: HomeAssistant, expected_state: str) -> None: + """Assert the ventilation select state.""" + state = hass.states.get(_SELECT_ENTITY) + assert state is not None + assert state.state == expected_state + + +async def _async_select_option( + hass: HomeAssistant, option: str, entity_id: str = _SELECT_ENTITY +) -> None: + """Select a ventilation option.""" + await hass.services.async_call( + SELECT_DOMAIN, + SERVICE_SELECT_OPTION, + {ATTR_ENTITY_ID: entity_id, ATTR_OPTION: option}, + blocking=True, + ) + + +async def _async_set_fan_percentage(hass: HomeAssistant) -> None: + """Set the ventilation fan percentage.""" + await hass.services.async_call( + FAN_DOMAIN, + SERVICE_SET_PERCENTAGE, + {ATTR_ENTITY_ID: "fan.living", ATTR_PERCENTAGE: 33}, + blocking=True, + ) + + @pytest.fixture async def init_integration( hass: HomeAssistant, @@ -187,14 +229,28 @@ async def test_select_option_calls_ventilation_state_library_method( """Test that selecting an option uses the typed ventilation state helper.""" mock_duco_client.async_set_ventilation_state = AsyncMock() - await hass.services.async_call( - SELECT_DOMAIN, - SERVICE_SELECT_OPTION, - {ATTR_ENTITY_ID: _SELECT_ENTITY, ATTR_OPTION: "CNT2"}, - blocking=True, - ) + await _async_select_option(hass, "CNT2") - mock_duco_client.async_set_ventilation_state.assert_called_once_with(1, "CNT2") + mock_duco_client.async_set_ventilation_state.assert_awaited_once_with(1, "CNT2") + mock_duco_client.async_get_node_info.assert_awaited_once_with(1) + assert mock_duco_client.async_get_nodes.await_count == 1 + + +@pytest.mark.usefixtures("init_integration") +async def test_targeted_readback_keeps_coordinator_poll_schedule( + hass: HomeAssistant, + mock_duco_client: AsyncMock, + freezer: FrozenDateTimeFactory, +) -> None: + """Test a targeted readback does not postpone the full coordinator poll.""" + freezer.tick(SCAN_INTERVAL / 2) + await _async_select_option(hass, "CNT2") + + freezer.tick(SCAN_INTERVAL / 2) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + assert mock_duco_client.async_get_nodes.await_count == 2 @pytest.mark.usefixtures("init_integration") @@ -215,12 +271,7 @@ async def test_select_option_error( mock_duco_client.async_set_ventilation_state = AsyncMock(side_effect=exception) with pytest.raises(HomeAssistantError, match=match): - await hass.services.async_call( - SELECT_DOMAIN, - SERVICE_SELECT_OPTION, - {ATTR_ENTITY_ID: _SELECT_ENTITY, ATTR_OPTION: "CNT2"}, - blocking=True, - ) + await _async_select_option(hass, "CNT2") async def test_select_extended_manual_options_allow_normalized_readback( @@ -238,7 +289,6 @@ async def test_select_extended_manual_options_allow_normalized_readback( state = hass.states.get(_SELECT_ENTITY) assert state is not None assert state.attributes[ATTR_OPTIONS] == ["AUTO", "MAN1", "MAN1x2", "MAN1x3"] - box_node = mock_nodes[0] mock_duco_client.async_set_ventilation_state = AsyncMock() mock_duco_client.async_get_nodes.return_value = [ @@ -246,12 +296,7 @@ async def test_select_extended_manual_options_allow_normalized_readback( *mock_nodes[1:], ] - await hass.services.async_call( - SELECT_DOMAIN, - SERVICE_SELECT_OPTION, - {ATTR_ENTITY_ID: _SELECT_ENTITY, ATTR_OPTION: "MAN1x2"}, - blocking=True, - ) + await _async_select_option(hass, "MAN1x2") mock_duco_client.async_set_ventilation_state.assert_called_once_with(1, "MAN1x2") state = hass.states.get(_SELECT_ENTITY) @@ -270,7 +315,6 @@ async def test_select_auto_option_allows_cnt1_readback( options=["AUTO", "CNT1", "CNT2"] ) await setup_platform_integration(hass, mock_config_entry, [Platform.SELECT]) - box_node = mock_nodes[0] mock_duco_client.async_set_ventilation_state = AsyncMock() mock_duco_client.async_get_nodes.return_value = [ @@ -278,12 +322,7 @@ async def test_select_auto_option_allows_cnt1_readback( *mock_nodes[1:], ] - await hass.services.async_call( - SELECT_DOMAIN, - SERVICE_SELECT_OPTION, - {ATTR_ENTITY_ID: _SELECT_ENTITY, ATTR_OPTION: "AUTO"}, - blocking=True, - ) + await _async_select_option(hass, "AUTO") mock_duco_client.async_set_ventilation_state.assert_called_once_with(1, "AUTO") state = hass.states.get(_SELECT_ENTITY) @@ -291,10 +330,230 @@ async def test_select_auto_option_allows_cnt1_readback( assert state.state == "CNT1" +@pytest.mark.parametrize( + ("failed_entity_id", "expected_state"), + [ + pytest.param(_SELECT_ENTITY, "CNT1", id="same-node"), + pytest.param(_VALVE_SELECT_ENTITY, STATE_UNAVAILABLE, id="other-node"), + ], +) +async def test_targeted_readback_failure_and_recovery( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + mock_duco_client: AsyncMock, + mock_sensor_nodes: list[Node], + failed_entity_id: str, + expected_state: str, +) -> None: + """Test a targeted success only clears its own node's failure.""" + mock_duco_client.async_get_nodes.return_value = mock_sensor_nodes + mock_duco_client.async_get_node_actions.return_value = _build_multi_node_actions( + [1, 60], options=["AUTO", "CNT1", "MAN3"] + ) + await setup_platform_integration( + hass, mock_config_entry, [Platform.FAN, Platform.SELECT] + ) + readback_started = asyncio.Event() + release_readback = asyncio.Event() + readback_count = 0 + + async def get_node_info(node_id: int) -> Node: + nonlocal readback_count + readback_count += 1 + if readback_count == 1: + readback_started.set() + await release_readback.wait() + raise DucoError("Readback failed") + return _replace_node_state(mock_sensor_nodes[0], "CNT1") + + mock_duco_client.async_get_node_info.side_effect = get_node_info + failed_task = asyncio.create_task( + _async_select_option(hass, "MAN3", failed_entity_id) + ) + await readback_started.wait() + + fan_task = asyncio.create_task(_async_set_fan_percentage(hass)) + release_readback.set() + await asyncio.gather(failed_task, fan_task) + + _assert_select_state(hass, expected_state) + + +async def test_targeted_readback_restores_node_omitted_by_older_poll( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + mock_duco_client: AsyncMock, + mock_nodes: list[Node], + freezer: FrozenDateTimeFactory, +) -> None: + """Test a targeted readback restores a node omitted by an older poll.""" + await setup_platform_integration(hass, mock_config_entry, [Platform.SELECT]) + poll_started = asyncio.Event() + release_poll = asyncio.Event() + write_started = asyncio.Event() + + async def get_nodes() -> list[Node]: + poll_started.set() + await release_poll.wait() + return mock_nodes[1:] + + def set_ventilation_state(node_id: int, state: str | VentilationState) -> None: + write_started.set() + + mock_duco_client.async_get_nodes.side_effect = get_nodes + mock_duco_client.async_set_ventilation_state.side_effect = set_ventilation_state + mock_duco_client.async_get_node_info.return_value = _replace_node_state( + mock_nodes[0], "MAN3" + ) + mock_duco_client.async_get_node_info.side_effect = None + + freezer.tick(SCAN_INTERVAL) + async_fire_time_changed(hass) + await poll_started.wait() + + write_task = asyncio.create_task(_async_select_option(hass, "MAN3")) + await asyncio.sleep(0) + assert not write_started.is_set() + + release_poll.set() + await write_task + + _assert_select_state(hass, "MAN3") + + +async def test_targeted_readback_does_not_recover_failed_coordinator_poll( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + mock_duco_client: AsyncMock, + mock_nodes: list[Node], + freezer: FrozenDateTimeFactory, +) -> None: + """Test a node readback does not recover a failed full coordinator poll.""" + await setup_platform_integration(hass, mock_config_entry, [Platform.SELECT]) + poll_started = asyncio.Event() + release_poll = asyncio.Event() + + async def get_nodes() -> list[Node]: + poll_started.set() + await release_poll.wait() + raise DucoConnectionError("Connection failed") + + mock_duco_client.async_get_nodes.side_effect = get_nodes + mock_duco_client.async_get_node_info.return_value = _replace_node_state( + mock_nodes[0], "MAN3" + ) + mock_duco_client.async_get_node_info.side_effect = None + + freezer.tick(SCAN_INTERVAL) + async_fire_time_changed(hass) + await poll_started.wait() + + write_task = asyncio.create_task(_async_select_option(hass, "MAN3")) + await asyncio.sleep(0) + release_poll.set() + await write_task + await hass.async_block_till_done() + + mock_duco_client.async_set_ventilation_state.assert_awaited_once_with(1, "MAN3") + mock_duco_client.async_get_node_info.assert_awaited_once_with(1) + _assert_select_state(hass, STATE_UNAVAILABLE) + + +async def test_latest_targeted_readback_wins_when_fan_and_select_overlap( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + mock_duco_client: AsyncMock, + mock_nodes: list[Node], +) -> None: + """Test an older fan readback cannot overwrite a newer select readback.""" + await setup_platform_integration( + hass, mock_config_entry, [Platform.FAN, Platform.SELECT] + ) + first_readback_started = asyncio.Event() + release_first_readback = asyncio.Event() + select_write_started = asyncio.Event() + readback_count = 0 + + def set_ventilation_state(node_id: int, state: str | VentilationState) -> None: + if state == "MAN3": + select_write_started.set() + + async def get_node_info(node_id: int) -> Node: + nonlocal readback_count + readback_count += 1 + if readback_count == 1: + first_readback_started.set() + await release_first_readback.wait() + return _replace_node_state(mock_nodes[0], "CNT1") + return _replace_node_state(mock_nodes[0], "MAN3") + + mock_duco_client.async_set_ventilation_state.side_effect = set_ventilation_state + mock_duco_client.async_get_node_info.side_effect = get_node_info + fan_task = asyncio.create_task(_async_set_fan_percentage(hass)) + await first_readback_started.wait() + + select_task = asyncio.create_task(_async_select_option(hass, "MAN3")) + await asyncio.sleep(0) + assert not select_write_started.is_set() + + release_first_readback.set() + await asyncio.gather(fan_task, select_task) + + _assert_select_state(hass, "MAN3") + + +@pytest.mark.usefixtures("init_integration") +@pytest.mark.parametrize( + "readback_error", + [ + pytest.param(None, id="success"), + pytest.param( + DucoError("Readback failed"), + id="failure", + ), + ], +) +async def test_newer_coordinator_poll_supersedes_targeted_outcome( + hass: HomeAssistant, + mock_duco_client: AsyncMock, + mock_nodes: list[Node], + freezer: FrozenDateTimeFactory, + readback_error: DucoError | None, +) -> None: + """Test a newer coordinator poll supersedes an older targeted outcome.""" + readback_started = asyncio.Event() + release_readback = asyncio.Event() + + async def get_node_info(node_id: int) -> Node: + readback_started.set() + await release_readback.wait() + if readback_error: + raise readback_error + return _replace_node_state(mock_nodes[0], "MAN3") + + mock_duco_client.async_get_node_info.side_effect = get_node_info + write_task = asyncio.create_task(_async_select_option(hass, "MAN3")) + await readback_started.wait() + + mock_duco_client.async_get_nodes.return_value = [ + _replace_node_state(mock_nodes[0], "CNT2"), + *mock_nodes[1:], + ] + freezer.tick(SCAN_INTERVAL) + async_fire_time_changed(hass) + + release_readback.set() + await write_task + await hass.async_block_till_done() + + _assert_select_state(hass, "CNT2") + + async def test_select_entity_is_added_when_action_discovery_succeeds_later( hass: HomeAssistant, mock_config_entry: MockConfigEntry, mock_duco_client: AsyncMock, + freezer: FrozenDateTimeFactory, ) -> None: """Test select entities are added when action discovery becomes available later.""" mock_duco_client.async_get_node_actions.side_effect = [ @@ -304,13 +563,12 @@ async def test_select_entity_is_added_when_action_discovery_succeeds_later( ), ] - config_entry = await setup_platform_integration( - hass, mock_config_entry, [Platform.SELECT] - ) + await setup_platform_integration(hass, mock_config_entry, [Platform.SELECT]) assert hass.states.get(_SELECT_ENTITY) is None - await config_entry.runtime_data.async_refresh() + freezer.tick(SCAN_INTERVAL) + async_fire_time_changed(hass) await hass.async_block_till_done() state = hass.states.get(_SELECT_ENTITY)