diff --git a/homeassistant/components/vicare/coordinator.py b/homeassistant/components/vicare/coordinator.py index 534c8ff8054c..93109846ec4c 100644 --- a/homeassistant/components/vicare/coordinator.py +++ b/homeassistant/components/vicare/coordinator.py @@ -1,5 +1,6 @@ """DataUpdateCoordinator for the ViCare integration.""" +from collections.abc import Callable from datetime import timedelta import logging from typing import override @@ -16,7 +17,7 @@ from PyViCare.PyViCareUtils import ( ) import requests -from homeassistant.core import HomeAssistant +from homeassistant.core import HomeAssistant, callback from homeassistant.exceptions import ConfigEntryAuthFailed from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed @@ -56,6 +57,17 @@ class ViCareCoordinator(DataUpdateCoordinator[None]): ) self._device = device self._accessor = accessor + self._value_readers: list[Callable[[], None]] = [] + + @callback + def async_add_value_reader(self, reader: Callable[[], None]) -> Callable[[], None]: + """Register a reader to run in the executor after each fetch.""" + self._value_readers.append(reader) + + def remove_value_reader() -> None: + self._value_readers.remove(reader) + + return remove_value_reader @override async def _async_update_data(self) -> None: @@ -89,3 +101,13 @@ class ViCareCoordinator(DataUpdateCoordinator[None]): requests.RequestException, ) as err: raise UpdateFailed(str(err)) from err + else: + # Only after a successful fetch: the cache was emptied above, and a + # reader must not be the one to refill it from the event loop. + # Iterate a copy, entities deregister from the event loop thread. + for reader in list(self._value_readers): + try: + reader() + except Exception: + # One unreadable value must not take the whole device down. + _LOGGER.exception("Error reading a ViCare entity value") diff --git a/homeassistant/components/vicare/entity.py b/homeassistant/components/vicare/entity.py index ddcb2658b081..01fbbbbd2eff 100644 --- a/homeassistant/components/vicare/entity.py +++ b/homeassistant/components/vicare/entity.py @@ -123,7 +123,13 @@ class ViCareEntity(Entity): class ViCareCoordinatorEntity(CoordinatorEntity[ViCareCoordinator], ViCareEntity): - """Base class for ViCare entities backed by the update coordinator.""" + """Base class for ViCare entities backed by the update coordinator. + + Values are read in ``_read_value``, which the coordinator calls from its + executor job. Properties must only return what was read there: a PyViCare + getter takes the library cache lock and does blocking I/O when that cache + is cold. + """ def __init__( self, @@ -139,3 +145,14 @@ class ViCareCoordinatorEntity(CoordinatorEntity[ViCareCoordinator], ViCareEntity ViCareEntity.__init__( self, unique_id_suffix, device_serial, device_config, device, component ) + + @override + async def async_added_to_hass(self) -> None: + """Register the value reader and prime the first value.""" + await super().async_added_to_hass() + self.async_on_remove(self.coordinator.async_add_value_reader(self._read_value)) + # The coordinator's first refresh ran before this entity existed. + await self.hass.async_add_executor_job(self._read_value) + + def _read_value(self) -> None: + """Read this entity's value from the API. Runs in the executor.""" diff --git a/homeassistant/components/vicare/fan.py b/homeassistant/components/vicare/fan.py index 966e0df738f5..ec568827dfcb 100644 --- a/homeassistant/components/vicare/fan.py +++ b/homeassistant/components/vicare/fan.py @@ -120,6 +120,7 @@ class ViCareFan(ViCareEntity, FanEntity): _attr_speed_count = len(ORDERED_NAMED_FAN_SPEEDS) _attr_translation_key = "ventilation" + _standby: bool = False def __init__( self, @@ -160,6 +161,11 @@ class ViCareFan(ViCareEntity, FanEntity): ) if VentilationQuickmode.STANDBY in quickmodes: self._attr_supported_features |= FanEntityFeature.TURN_OFF + # The first state is published before the first poll. + with suppress(PyViCareNotSupportedFeatureError): + self._standby = device.getVentilationQuickmode( + VentilationQuickmode.STANDBY + ) def update(self) -> None: """Update state of fan.""" @@ -170,6 +176,15 @@ class ViCareFan(ViCareEntity, FanEntity): self._api.getActiveVentilationMode() ) + if FanEntityFeature.TURN_OFF in self._attr_supported_features: + # Clear before the guarded read, a suppressed error would + # otherwise keep reporting the fan as off. + self._standby = False + with suppress(PyViCareNotSupportedFeatureError): + self._standby = self._api.getVentilationQuickmode( + VentilationQuickmode.STANDBY + ) + with suppress(PyViCareNotSupportedFeatureError): level = filter_state(self._api.getVentilationLevel()) if level is not None and level in ORDERED_NAMED_FAN_SPEEDS: @@ -183,9 +198,7 @@ class ViCareFan(ViCareEntity, FanEntity): @override def is_on(self) -> bool | None: """Return true if the entity is on.""" - if VentilationQuickmode.STANDBY in self._attributes[ - "vicare_quickmodes" - ] and self._api.getVentilationQuickmode(VentilationQuickmode.STANDBY): + if self._standby: return False return self.percentage is not None and self.percentage > 0 @@ -199,9 +212,7 @@ class ViCareFan(ViCareEntity, FanEntity): @override def icon(self) -> str | None: """Return the icon to use in the frontend.""" - if VentilationQuickmode.STANDBY in self._attributes[ - "vicare_quickmodes" - ] and self._api.getVentilationQuickmode(VentilationQuickmode.STANDBY): + if self._standby: return "mdi:fan-off" if hasattr(self, "_attr_preset_mode"): if self._attr_preset_mode == VentilationMode.VENTILATION: diff --git a/homeassistant/components/vicare/sensor.py b/homeassistant/components/vicare/sensor.py index 5b23d3e7179f..edf1e0481092 100644 --- a/homeassistant/components/vicare/sensor.py +++ b/homeassistant/components/vicare/sensor.py @@ -38,7 +38,6 @@ from homeassistant.const import ( ) from homeassistant.core import HomeAssistant from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback -from homeassistant.helpers.typing import StateType from .const import ( VICARE_BAR, @@ -1643,10 +1642,11 @@ class ViCareSensor(ViCareCoordinatorEntity, SensorEntity): vicare_unit ] - @property @override - def native_value(self) -> StateType: - """Return the state of the sensor.""" - with suppress(PyViCareNotSupportedFeatureError): - return self.entity_description.value_getter(self._api) - return None + def _read_value(self) -> None: + """Read the sensor value from the API.""" + # Both context managers swallow read failures, so clear first rather + # than republish the previous value. + self._attr_native_value = None + with self.vicare_api_handler(), suppress(PyViCareNotSupportedFeatureError): + self._attr_native_value = self.entity_description.value_getter(self._api) diff --git a/tests/components/vicare/test_fan.py b/tests/components/vicare/test_fan.py index 0200c3d61303..2d89c454927c 100644 --- a/tests/components/vicare/test_fan.py +++ b/tests/components/vicare/test_fan.py @@ -3,11 +3,14 @@ from unittest.mock import patch import pytest +from PyViCare.PyViCareUtils import PyViCareNotSupportedFeatureError from syrupy.assertion import SnapshotAssertion -from homeassistant.const import Platform +from homeassistant.components.fan import DOMAIN as FAN_DOMAIN +from homeassistant.const import ATTR_ICON, Platform from homeassistant.core import HomeAssistant from homeassistant.helpers import entity_registry as er +from homeassistant.helpers.entity_component import async_update_entity from . import MODULE, setup_integration from .conftest import Fixture, MockPyViCare @@ -41,3 +44,83 @@ async def test_all_entities( await setup_integration(hass, mock_config_entry) await snapshot_platform(hass, entity_registry, snapshot, mock_config_entry.entry_id) + + +@pytest.mark.usefixtures("entity_registry_enabled_by_default") +async def test_standby_quickmode( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, +) -> None: + """Test that the fan follows the standby quickmode. + + The value is now read in the executor and cached, so it has to be primed + before the first state is published, and a read that stops being supported + must not leave the fan reported as in standby. The recorded level of this + device is unknown, which is why the icon and not the state carries the + difference here. + """ + fixtures: list[Fixture] = [Fixture({"type:ventilation"}, "vicare/VitoPure.json")] + vicare_data = MockPyViCare(fixtures).as_vicare_data() + api = vicare_data.devices[0].api + + with ( + patch( + "homeassistant.helpers.config_entry_oauth2_flow.OAuth2Session.async_ensure_token_valid", + ), + patch(f"{MODULE}._setup_vicare_api", return_value=vicare_data), + patch(f"{MODULE}.PLATFORMS", [Platform.FAN]), + patch.object(api, "getVentilationQuickmode", return_value=True), + ): + await setup_integration(hass, mock_config_entry) + + entity_id = hass.states.async_entity_ids(FAN_DOMAIN)[0] + assert "standby" in hass.states.get(entity_id).attributes["vicare_quickmodes"] + # The first state is published before the first poll. + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-off" + + # The fixture runs sensor driven and reports the quickmode as inactive. + await async_update_entity(hass, entity_id) + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-auto" + + with patch.object(api, "getVentilationQuickmode", return_value=True): + await async_update_entity(hass, entity_id) + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-off" + + with patch.object( + api, + "getVentilationQuickmode", + side_effect=PyViCareNotSupportedFeatureError("standby"), + ): + await async_update_entity(hass, entity_id) + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-auto" + + +@pytest.mark.usefixtures("entity_registry_enabled_by_default") +async def test_standby_quickmode_with_a_second_fan( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, +) -> None: + """Test that a second fan without the quickmode does not stop the refresh.""" + fixtures: list[Fixture] = [ + Fixture({"type:ventilation"}, "vicare/VitoPure.json"), + Fixture({"type:ventilation"}, "vicare/ViAir300F.json"), + ] + vicare_data = MockPyViCare(fixtures).as_vicare_data() + api = vicare_data.devices[0].api + + with ( + patch( + "homeassistant.helpers.config_entry_oauth2_flow.OAuth2Session.async_ensure_token_valid", + ), + patch(f"{MODULE}._setup_vicare_api", return_value=vicare_data), + patch(f"{MODULE}.PLATFORMS", [Platform.FAN]), + patch.object(api, "getVentilationQuickmode", return_value=True), + ): + await setup_integration(hass, mock_config_entry) + + entity_id = hass.states.async_entity_ids(FAN_DOMAIN)[0] + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-off" + + # The fixture itself reports the quickmode as inactive. + await async_update_entity(hass, entity_id) + assert hass.states.get(entity_id).attributes[ATTR_ICON] == "mdi:fan-auto" diff --git a/tests/components/vicare/test_sensor.py b/tests/components/vicare/test_sensor.py index 0d35c1f5660f..99bab0689f8a 100644 --- a/tests/components/vicare/test_sensor.py +++ b/tests/components/vicare/test_sensor.py @@ -1,18 +1,28 @@ """Test ViCare sensor entity.""" +from contextlib import ExitStack +from datetime import timedelta +import threading +from typing import Any from unittest.mock import patch +from freezegun.api import FrozenDateTimeFactory import pytest +from PyViCare.PyViCareService import ViCareDeviceAccessor from syrupy.assertion import SnapshotAssertion +from homeassistant.components.fan import DOMAIN as FAN_DOMAIN +from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN +from homeassistant.components.vicare.const import DEFAULT_CACHE_DURATION from homeassistant.const import Platform from homeassistant.core import HomeAssistant from homeassistant.helpers import entity_registry as er +from homeassistant.helpers.entity_component import async_update_entity from . import MODULE, setup_integration from .conftest import Fixture, MockPyViCare -from tests.common import MockConfigEntry, snapshot_platform +from tests.common import MockConfigEntry, async_fire_time_changed, snapshot_platform @pytest.mark.usefixtures("entity_registry_enabled_by_default") @@ -55,3 +65,73 @@ async def test_all_entities( await setup_integration(hass, mock_config_entry) await snapshot_platform(hass, entity_registry, snapshot, mock_config_entry.entry_id) + + +@pytest.mark.usefixtures("entity_registry_enabled_by_default") +async def test_no_api_read_on_event_loop( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, +) -> None: + """Entity values must be read in the executor, never on the event loop. + + A PyViCare getter takes the library's cache lock and does blocking I/O when + that cache is cold, so reading a value from a property freezes the loop for + the length of an HTTP request. + """ + fixtures: list[Fixture] = [ + Fixture({"type:heatpump"}, "vicare/Vitocal250A.json"), + Fixture({"type:ventilation"}, "vicare/VitoPure.json"), + ] + mock_vicare = MockPyViCare(fixtures) + services = {id(d.service): d.service for d in mock_vicare.devices} + + loop_thread_id = threading.get_ident() + reads_on_loop: list[str] = [] + reads_in_executor: list[str] = [] + + def guard(service): + read_property = service.getProperty + + def guarded_get_property( + accessor: ViCareDeviceAccessor, property_name: str + ) -> Any: + if threading.get_ident() == loop_thread_id: + reads_on_loop.append(property_name) + else: + reads_in_executor.append(property_name) + return read_property(accessor, property_name) + + return patch.object(service, "getProperty", guarded_get_property) + + with ( + patch( + "homeassistant.helpers.config_entry_oauth2_flow.OAuth2Session.async_ensure_token_valid", + ), + patch( + f"{MODULE}._setup_vicare_api", + return_value=mock_vicare.as_vicare_data(), + ), + patch(f"{MODULE}.PLATFORMS", [Platform.SENSOR, Platform.FAN]), + ): + await setup_integration(hass, mock_config_entry) + + entity_ids = hass.states.async_entity_ids(SENSOR_DOMAIN) + fan_ids = hass.states.async_entity_ids(FAN_DOMAIN) + assert entity_ids + assert fan_ids + + with ExitStack() as stack: + for service in services.values(): + stack.enter_context(guard(service)) + + freezer.tick(timedelta(seconds=DEFAULT_CACHE_DURATION * 2)) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + for entity_id in (entity_ids[0], fan_ids[0]): + await async_update_entity(hass, entity_id) + + assert not reads_on_loop + # Without this the test would also pass if nothing read a value at all. + assert reads_in_executor