Read ViCare entity values in the executor (#181379)

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

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."""

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:

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)

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"

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