-
-
Notifications
You must be signed in to change notification settings - Fork 38.2k
Fix Lyric room sensor/priority data being silently skipped for T9/T10 thermostats #177022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
a9dd539
90b9dc8
dea9904
a015648
a60d49c
b9bfd69
8cc6a0f
b3870c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,13 +67,12 @@ async def _run_update(self, force_refresh_token: bool) -> Lyric: | |
| await self.lyric.get_locations() | ||
| await asyncio.gather( | ||
| *( | ||
| self.lyric.get_thermostat_rooms( | ||
| self._get_thermostat_rooms( | ||
| location.location_id, device.device_id | ||
| ) | ||
| for location in self.lyric.locations | ||
| for device in location.devices | ||
| if device.device_class == "Thermostat" | ||
| and device.device_id.startswith("LCC") | ||
| ) | ||
| ) | ||
|
|
||
|
|
@@ -87,3 +86,24 @@ async def _run_update(self, force_refresh_token: bool) -> Lyric: | |
| except (LyricException, ClientResponseError) as exception: | ||
| raise UpdateFailed(exception) from exception | ||
| return self.lyric | ||
|
|
||
| async def _get_thermostat_rooms(self, location_id: int, device_id: str) -> None: | ||
| """Fetch room/priority data for a single thermostat. | ||
|
|
||
| Devices that don't support this endpoint return a GetPriorityFailed | ||
| 400, which is expected and shouldn't fail the whole coordinator | ||
| update. Any other error is re-raised. | ||
| """ | ||
| try: | ||
| await self.lyric.get_thermostat_rooms(location_id, device_id) | ||
| except LyricAuthenticationException: | ||
| raise | ||
| except LyricException as exception: | ||
| payload = exception.args[0] if exception.args else {} | ||
| response = payload.get("response") or {} | ||
| if ( | ||
| payload.get("status") != HTTPStatus.BAD_REQUEST | ||
| or response.get("code") != "GetPriorityFailed" | ||
| ): | ||
|
Comment on lines
+101
to
+107
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed in principle — this is exactly the thin-wrapper concern from HA's own integration guidelines, and reaching into exception.args[0]["response"]["code"] is more tightly coupled to aiolyric's raw shape than I'd like. The cleaner fix is what you're describing: a dedicated exception (e.g. LyricPriorityNotSupportedException) raised by aiolyric itself when it sees GetPriorityFailed, with core just catching that type. I'm tracking this as a follow-up in aiolyric rather than folding it into this PR, since it'd add yet another cross-repo release dependency on top of the one already blocking full functionality (see the other comment thread). Once that lands, I'll simplify this handler to catch the typed exception instead of inspecting the payload directly. |
||
| raise | ||
| _LOGGER.debug("Device %s does not support room priority data", device_id) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,13 @@ | ||
| """Tests for the Honeywell Lyric integration.""" | ||
|
|
||
| from homeassistant.core import HomeAssistant | ||
|
|
||
| from tests.common import MockConfigEntry | ||
|
|
||
|
|
||
| async def setup_integration(hass: HomeAssistant, config_entry: MockConfigEntry) -> None: | ||
| """Set up the Lyric integration for tests.""" | ||
| config_entry.add_to_hass(hass) | ||
|
|
||
| await hass.config_entries.async_setup(config_entry.entry_id) | ||
| await hass.async_block_till_done() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| """Fixtures for the Honeywell Lyric integration tests.""" | ||
|
|
||
| from collections.abc import AsyncGenerator | ||
| import time | ||
| from typing import Any | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| from aiolyric import Lyric | ||
| from aiolyric.exceptions import LyricException | ||
| from aiolyric.objects.location import LyricLocation | ||
| from aiolyric.objects.priority import LyricRoom | ||
| import pytest | ||
|
|
||
| from homeassistant.components.application_credentials import ( | ||
| ClientCredential, | ||
| async_import_client_credential, | ||
| ) | ||
| from homeassistant.components.lyric.const import DOMAIN | ||
| from homeassistant.core import HomeAssistant | ||
| from homeassistant.setup import async_setup_component | ||
|
|
||
| from tests.common import MockConfigEntry, load_json_object_fixture | ||
|
|
||
| CLIENT_ID = "1234" | ||
| CLIENT_SECRET = "5678" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def setup_credentials(hass: HomeAssistant) -> None: | ||
| """Set up the application credentials required for the Lyric OAuth2 flow.""" | ||
| assert await async_setup_component(hass, "application_credentials", {}) | ||
| await async_import_client_credential( | ||
| hass, DOMAIN, ClientCredential(CLIENT_ID, CLIENT_SECRET), DOMAIN | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_config_entry() -> MockConfigEntry: | ||
| """Return a Lyric config entry with a valid OAuth2 token.""" | ||
| return MockConfigEntry( | ||
| domain=DOMAIN, | ||
| data={ | ||
| "auth_implementation": DOMAIN, | ||
| "token": { | ||
| "access_token": "mock-access-token", | ||
| "refresh_token": "mock-refresh-token", | ||
| "expires_at": time.time() + 3600, | ||
| "token_type": "Bearer", | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
|
|
||
| def thermostat_json( | ||
| mac_id: str, device_id: str, name: str = "Thermostat" | ||
| ) -> dict[str, Any]: | ||
| """Return a raw aiolyric device payload for a single thermostat.""" | ||
| return { | ||
| **load_json_object_fixture("thermostat.json", DOMAIN), | ||
| "deviceID": device_id, | ||
| "macID": mac_id, | ||
| "name": name, | ||
| } | ||
|
|
||
|
|
||
| def location_json(location_id: int, devices: list[dict[str, Any]]) -> dict[str, Any]: | ||
| """Return a raw aiolyric location payload containing the given devices.""" | ||
| return {"locationID": location_id, "name": "Home", "devices": devices} | ||
|
|
||
|
|
||
| def build_mock_lyric( | ||
| locations: list[LyricLocation], rooms_dict: dict | None = None | ||
| ) -> MagicMock: | ||
| """Build a mocked aiolyric client backed by real aiolyric location/device objects.""" | ||
| lyric = MagicMock(spec=Lyric) | ||
| lyric.locations = locations | ||
| lyric.locations_dict = {location.location_id: location for location in locations} | ||
| lyric.rooms_dict = rooms_dict or {} | ||
| return lyric | ||
|
|
||
|
|
||
| def lyric_exception( | ||
| status: int, code: str | None = "GetPriorityFailed" | ||
| ) -> LyricException: | ||
| """Build a LyricException matching aiolyric's actual payload shape.""" | ||
| return LyricException( | ||
| { | ||
| "request": {"method": "GET", "url": "https://example.com"}, | ||
| "response": {"code": code} if code else {}, | ||
| "status": status, | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| LCC_SUPPORTED_MAC = "AABBCC000001" | ||
| NON_LCC_SUPPORTED_MAC = "AABBCC000002" | ||
| UNSUPPORTED_MAC = "AABBCC000003" | ||
| UNSUPPORTED_DEVICE_ID = "LCC-AABBCC000003" | ||
|
|
||
| LIVING_ROOM_JSON = { | ||
| "id": 0, | ||
| "roomName": "Living Room", | ||
| "roomAvgTemp": 22, | ||
| "roomAvgHumidity": 50, | ||
| "accessories": [ | ||
| {"id": 0, "type": "IndoorAirSensor", "temperature": 22.5, "status": "Ok"} | ||
| ], | ||
| } | ||
|
|
||
| OFFICE_ROOM_JSON = { | ||
| "id": 0, | ||
| "roomName": "Office", | ||
| "roomAvgTemp": 24, | ||
| "roomAvgHumidity": 40, | ||
| "accessories": [ | ||
| {"id": 0, "type": "IndoorAirSensor", "temperature": 24.5, "status": "Ok"} | ||
| ], | ||
| } | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def mock_lyric_mixed_devices() -> AsyncGenerator[MagicMock]: | ||
| """Yield a mocked Lyric client with an LCC device, a non-LCC device, and an unsupported device. | ||
|
|
||
| Room priority support depends on the thermostat model, not on whether | ||
| its device ID happens to start with "LCC" (see the T9/T10 regression | ||
| this integration guards against), so this fixture exercises a | ||
| supported device on each prefix. ``get_thermostat_rooms`` populates | ||
| ``rooms_dict`` as a side effect, mirroring the real library, so a | ||
| device that the coordinator skips calling never gets room data. | ||
| """ | ||
| location = LyricLocation( | ||
| MagicMock(), | ||
| location_json( | ||
| 1, | ||
| [ | ||
| thermostat_json(LCC_SUPPORTED_MAC, "LCC-AABBCC000001", "Living Room"), | ||
| thermostat_json(NON_LCC_SUPPORTED_MAC, "TCC-AABBCC000002", "Office"), | ||
| thermostat_json(UNSUPPORTED_MAC, UNSUPPORTED_DEVICE_ID, "Bedroom"), | ||
| ], | ||
| ), | ||
| ) | ||
| rooms = { | ||
| LCC_SUPPORTED_MAC: LIVING_ROOM_JSON, | ||
| NON_LCC_SUPPORTED_MAC: OFFICE_ROOM_JSON, | ||
| } | ||
| mac_by_device_id = {device.device_id: device.mac_id for device in location.devices} | ||
|
|
||
| lyric = MagicMock(spec=Lyric) | ||
| lyric.locations = [location] | ||
| lyric.locations_dict = {location.location_id: location} | ||
| lyric.rooms_dict = {} | ||
| lyric.priorities_dict = {} | ||
|
|
||
| async def get_thermostat_rooms(location_id: int, device_id: str) -> None: | ||
| if device_id == UNSUPPORTED_DEVICE_ID: | ||
| raise lyric_exception(400) | ||
| mac_id = mac_by_device_id[device_id] | ||
| room_json = rooms[mac_id] | ||
| lyric.rooms_dict[mac_id] = {room_json["id"]: LyricRoom(room_json)} | ||
|
|
||
| lyric.get_thermostat_rooms = AsyncMock(side_effect=get_thermostat_rooms) | ||
|
|
||
| with patch("homeassistant.components.lyric.Lyric", return_value=lyric): | ||
| yield lyric |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "deviceClass": "Thermostat", | ||
| "units": "Celsius", | ||
| "indoorTemperature": 21.5, | ||
| "indoorHumidity": 45, | ||
| "outdoorTemperature": 10, | ||
| "displayedOutdoorHumidity": 60, | ||
| "deviceModel": "T9", | ||
| "changeableValues": { | ||
| "mode": "Heat", | ||
| "heatSetpoint": 20, | ||
| "coolSetpoint": 24, | ||
| "thermostatSetpointStatus": "NoHold" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| """Tests for the Honeywell Lyric select platform.""" | ||
|
|
||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| from homeassistant.components.lyric.const import DOMAIN | ||
| from homeassistant.const import Platform | ||
| from homeassistant.core import HomeAssistant | ||
| from homeassistant.helpers import entity_registry as er | ||
|
|
||
| from . import setup_integration | ||
|
|
||
| from tests.common import MockConfigEntry | ||
|
|
||
|
|
||
| @pytest.mark.usefixtures("setup_credentials", "mock_lyric_mixed_devices") | ||
| async def test_room_priority_select_created_regardless_of_device_id_prefix( | ||
| hass: HomeAssistant, | ||
| mock_config_entry: MockConfigEntry, | ||
| entity_registry: er.EntityRegistry, | ||
| ) -> None: | ||
| """The room priority select is created for supported devices whether or not their ID starts with LCC. | ||
|
|
||
| An unsupported device (GetPriorityFailed 400) shouldn't get a room | ||
| priority select, since it has no room data to choose between. | ||
| """ | ||
| with patch("homeassistant.components.lyric.PLATFORMS", [Platform.SELECT]): | ||
| await setup_integration(hass, mock_config_entry) | ||
|
|
||
| lcc_select = hass.states.get( | ||
| entity_registry.async_get_entity_id( | ||
| Platform.SELECT, DOMAIN, "AABBCC000001_room_priority" | ||
| ) | ||
| ) | ||
| assert lcc_select.attributes["options"] == ["follow_me", "Living Room"] | ||
|
|
||
| non_lcc_select = hass.states.get( | ||
| entity_registry.async_get_entity_id( | ||
| Platform.SELECT, DOMAIN, "AABBCC000002_room_priority" | ||
| ) | ||
| ) | ||
| assert non_lcc_select.attributes["options"] == ["follow_me", "Office"] | ||
|
|
||
| assert ( | ||
| entity_registry.async_get_entity_id( | ||
| Platform.SELECT, DOMAIN, "AABBCC000003_room_priority" | ||
| ) | ||
| is None | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that room entities won't populate from this PR alone — that's a known, disclosed dependency, not an oversight. See the "Additional information" section above: aiolyric's LyricPriority.current_priority currently reads the wrong JSON key (currentPriority vs. the live API's priority), tracked and fixed in timmo001/aiolyric#165, not yet released to PyPI.
This PR and that one fix two separate bugs in two separate projects: this one fixes the coordinator silently skipping the priority fetch entirely for non-LCC devices (and, as of the latest commits, no longer fails the whole coordinator update when one thermostat doesn't support the endpoint — a real regression independent of the room-sensor feature). That's correct and complete on its own merits, for any thermostat, not just T9/T10 room-sensor setups. Once #165 is released, I'll follow up with a manifest version bump to actually wire the two together. Holding this PR until then would mean leaving the #116665/#116668 regression unfixed for everyone in the meantime, for a reason unrelated to what this PR actually does.