Fix Lyric room sensor/priority data being silently skipped for T9/T10 thermostats - #177022
Fix Lyric room sensor/priority data being silently skipped for T9/T10 thermostats#177022clutch2sft wants to merge 8 commits into
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @clutch2sft,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
f597d0dfa9f24f551846739db53fd11d4648c22f- This commit has something that looks like an email address (greggc@trip-g.com). Maybe try linking that to GitHub?. -
9bf76ba5a7124485f95c842b11c7e6864823d501- This commit has something that looks like an email address (greggc@trip-g.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"email@address.com" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
home-assistant
Bot
added
bugfix
cla-error
has-tests
integration: lyric
small-pr
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @timmo001, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Replaces Lyric’s device-ID heuristic with per-device room-priority handling for T9/T10 thermostats.
Changes:
- Fetches room data for every thermostat.
- Removes the LCC-only select restriction.
- Adds coordinator error-handling tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
coordinator.py |
Adds per-device room fetching and error handling. |
select.py |
Allows room-priority selects for non-LCC devices. |
test_coordinator.py |
Tests room-fetch outcomes. |
| except ClientResponseError as exception: | ||
| if exception.status != HTTPStatus.BAD_REQUEST: | ||
| raise | ||
| _LOGGER.debug("Device %s does not support room priority data", device_id) |
| request_info=MagicMock(), history=(), status=400 | ||
| ) | ||
|
|
||
| await coordinator._get_thermostat_rooms("location1", "device1") |
| """A 400 GetPriorityFailed for one device shouldn't fail the update. | ||
|
|
||
| Devices that don't support the room priority endpoint (e.g. older | ||
| thermostats) return a 400 here; that's expected and must not be | ||
| conflated with devices that do support it but got skipped by a | ||
| device ID heuristic. | ||
| """ |
| """Fetch room/priority data for a single thermostat. | ||
|
|
||
| Not all thermostat models support this endpoint; Honeywell returns | ||
| a 400 (GetPriorityFailed) for those. That's expected and shouldn't | ||
| fail the whole coordinator update, so it's handled per-device | ||
| instead of relying on device ID heuristics to predict support. | ||
| """ |
recheck |
… thermostats The "LCC" device ID prefix check added in home-assistant#116876 to avoid a 400 GetPriorityFailed error was gating on the wrong signal: the prefix reflects which Honeywell cloud backend a device is registered under, not whether it supports the room priority endpoint. This silently skipped room sensor/priority data for supported T9/T10 thermostats whose device IDs don't start with "LCC". Instead, attempt the room/priority fetch for every thermostat and handle the expected per-device 400 response individually, so an unsupported device no longer fails the whole coordinator update (the original home-assistant#116665/home-assistant#116668 issue) without excluding devices that do support it. Also drops the same broken prefix check from the select platform, which already gates correctly on rooms_dict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Hi @clutch2sft
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
homeassistant/components/lyric/coordinator.py:103
- Catch aiolyric’s
LyricExceptionand verify its payload is a 400GetPriorityFailedresponse. The pinned aiolyric 2.1.1 raisesLyricException, notClientResponseError, for this HTTP 400, so unsupported thermostats still reach the outer handler asUpdateFailedand the change reintroduces #116668 for every previously skipped non-LCC device; the current status-only check would also suppress unrelated 400 responses.
except ClientResponseError as exception:
if exception.status != HTTPStatus.BAD_REQUEST:
raise
_LOGGER.debug("Device %s does not support room priority data", device_id)
tests/components/lyric/test_coordinator.py:37
- Exercise a coordinator refresh with a non-LCC thermostat instead of invoking only the private helper. These tests still pass if the
startswith("LCC")gate remains or is reintroduced in_run_update, so they do not cover the reported regression; an update-level test should also verify that one unsupported thermostat does not fail the aggregate refresh.
await coordinator._get_thermostat_rooms("location1", "device1")
aiolyric's LyricClient.request() never raises aiohttp's ClientResponseError - for any non-200 response it raises its own LyricException (or LyricAuthenticationException for 401), with the HTTP status embedded in exception.args[0]["status"] rather than a .status attribute. The previous except ClientResponseError clause in _get_thermostat_rooms never matched anything real, so a 400 GetPriorityFailed from an unsupported device fell through to the outer except (LyricException, ClientResponseError) handler and failed the whole coordinator update - reintroducing the exact home-assistant#116665/home-assistant#116668 regression this fix was meant to prevent. Also: LyricAuthenticationException is a LyricException subclass, so it must be caught and re-raised before the generic LyricException handler, or auth failures would be silently swallowed as "unsupported device" instead of triggering the token-refresh retry in _run_update. Rewrote the coordinator tests to construct LyricException with aiolyric's real payload shape instead of a fabricated ClientResponseError, added a test asserting authentication errors still propagate, and added an aggregate-level test that exercises _run_update itself (not just the private helper) with a mix of a supported and unsupported device, verifying one unsupported thermostat no longer fails the whole refresh. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Checking HTTP status alone meant any 400 from the priority endpoint - not just "device doesn't support this" - was silently treated as an unsupported device, which could mask an unrelated bad-request error (a bug in our own request, or a new/different failure from Honeywell) with no visibility. Now also requires the response payload's "code" field to equal "GetPriorityFailed" before suppressing; any other 400 re-raises and fails loudly via UpdateFailed, same as before this fix existed. Also condensed a docstring that restated the exception-routing history already covered in commit messages.
| self._get_thermostat_rooms( | ||
| location.location_id, device.device_id | ||
| ) |
There was a problem hiding this comment.
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.
| 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" | ||
| ): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We don't want to directly test the coordinator, instead mock the library and have HA set up the integration
There was a problem hiding this comment.
Reworked — dropped test_coordinator.py. Tests now mock aiolyric.Lyric and go through hass.config_entries.async_setup(), asserting on entity states / ConfigEntryState instead of coordinator internals. Coverage moved into test_init.py (failure paths) and test_sensor.py (unsupported device doesn't block setup). eworked — dropped test_coordinator.py. Tests now mock aiolyric.Lyric and go through hass.config_entries.async_setup(), asserting on entity states / ConfigEntryState instead of coordinator internals. Coverage moved into test_init.py (failure paths) and test_sensor.py (unsupported device doesn't block setup).
| thermostat_json("AABBCC000001", "LCC-AABBCC000001", "Living Room"), | ||
| thermostat_json("AABBCC000002", "LCC-AABBCC000002", "Bedroom"), |
| raise UpdateFailed(exception) from exception | ||
| return self.lyric | ||
|
|
||
| async def _get_thermostat_rooms(self, location_id: str, device_id: str) -> None: |
… setup Replace direct coordinator unit tests with integration-level tests that mock aiolyric and drive setup through hass.config_entries.async_setup(), asserting on entity states and config entry state instead of coordinator internals. Fold the coverage into test_init.py (setup failure modes) and test_sensor.py (entity creation), and drop test_coordinator.py.
The sensor test used two LCC-prefixed device IDs and only loaded the sensor platform, so it wouldn't fail if the removed device ID prefix gates were reintroduced in coordinator.py or select.py. Add a non-LCC supported device to the shared mock fixture and a new select platform test asserting its room priority entity.
thermostat_json() rebuilt its whole payload from scratch in Python on every call. Move the boilerplate (deviceClass, units, changeableValues, etc.) into fixtures/thermostat.json and just override the per-call fields (deviceID, macID, name) in Python, matching the "JSON fixtures over inline dicts" convention - while keeping the parameterization this fixture actually needs (it's called with different IDs across several tests, so a single static JSON file alone wouldn't fit).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
homeassistant/components/lyric/coordinator.py:90
- Match aiolyric’s
get_thermostat_roomscontract by typinglocation_idasint;LyricLocation.location_idand the library method use numeric location IDs, so the current annotation misstates this wrapper’s input.
async def _get_thermostat_rooms(self, location_id: str, device_id: str) -> None:
tests/components/lyric/test_sensor.py:41
- Apply the unused
mock_lyric_mixed_devicesfixture throughusefixturesinstead of injecting it; this keeps the signature limited to values the test reads.
mock_lyric_mixed_devices: MagicMock,
tests/components/lyric/test_select.py:23
- Apply the unused
mock_lyric_mixed_devicesfixture throughusefixturesinstead of injecting it; this keeps the signature limited to values the test reads.
mock_lyric_mixed_devices: MagicMock,
- _get_thermostat_rooms's location_id was typed str, but Resideo's own API docs document locationID as an Integer, and aiolyric's get_thermostat_rooms already types it int - our test fixtures had been quoting it as a string since early in this branch's history, which is what made the str annotation look consistent. Fixed the annotation to match the documented contract. - Apply the unused mock_lyric_mixed_devices fixture via usefixtures in test_sensor.py/test_select.py instead of injecting it as an unused parameter, per our own "don't inject unused fixture arguments" rule.
Proposed change
Room sensor accessories (Honeywell RCHTSENSOR paired to a T9/T10 thermostat) never show up as
entities, even when the Honeywell/Resideo cloud confirms they're paired. The Room Priority
selectentity and Room Temperature/Humiditysensorentities added in #104343 are silentlynever created for these devices, with no error or warning logged.
The cause is in the fetch gate added by #116876. That PR restricted the
/devices/thermostats/{id}/prioritycall to devices whosedevice_idstarts with"LCC", tostop one unsupported device (#116668) from throwing a
ClientResponseErrorthat failed theentire coordinator update every poll. But the
LCC/TCCdevice ID prefix reflects whichHoneywell cloud backend a device happens to be registered under, not which thermostat models
support the priority endpoint — a T9/T10 can have either prefix depending on account/pairing
history. So the fix over-corrected: it also silently excludes supported T9/T10 devices whose IDs
don't happen to start with
LCC.This PR removes the prefix heuristic and instead attempts the room/priority fetch for every
thermostat, catching the specific
400(GetPriorityFailed) response per-device so anunsupported device is skipped without failing the whole coordinator update — fixing the original
#116665/#116668 problem without excluding devices that do support the endpoint. The same broken
prefix check is also removed from
select.py, which already gates correctly on whetherrooms_dicthas data for the device.Note: this makes Home Assistant correctly attempt and parse the priority fetch. On its own it's
also enough to stop the coordinator failing outright for devices that don't support the endpoint.
Full end-to-end room sensor/select data additionally depends on a separate bug I found in the
aiolyriclibrary itself (it reads JSON keys that don't match Resideo's current live responseschema, e.g.
currentPriorityvs. the actualpriority) — PR open here:timmo001/aiolyric@master...clutch2sft:aiolyric:fix/priority-endpoint-field-names
Type of change
Additional information
This PR fixes or closes issue: fixes Room sensor accessories (Honeywell Home RCHTSENSOR) paired to a T9/T10 thermostat are not being surfaced as entities #177010
This PR is related to issue: Finished fetching lyric_coordinator data in 0.380 seconds (success: False) After 2024.5.0 update #116665, Honeywell lyric integration on getpriority error if device does not support this API #116668
Companion library fix (required for room sensor data to actually populate once this is merged): Fix priority endpoint field names to match Resideo's live API schema timmo001/aiolyric#165
This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:
Link to developer documentation pull request:
Link to frontend pull request:
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: