Add Device Pairing Enabled binary sensor to Lyric integration - #177062
Conversation
Resideo's priority endpoint reports per-accessory motion detection (detectMotion) for paired IndoorAirSensor room accessories, which the integration parsed but never surfaced as an entity. Adds a new binary_sensor platform following the same per-accessory pattern already used for Room Temperature/Humidity in sensor.py.
Resideo's device response includes vacationHold.enabled, which was parsed but never surfaced. Adds a device-level binary sensor alongside the existing per-accessory Room Motion sensor, introducing a DEVICE_BINARY_SENSORS list mirroring the DEVICE_SENSORS pattern already used in sensor.py.
Resideo's device response includes settings.devicePairingEnabled, which was parsed but never surfaced. Adds it as a diagnostic binary sensor alongside Vacation Hold, using the same DEVICE_BINARY_SENSORS list.
|
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
Adds Lyric binary sensors for room motion, vacation hold, and device pairing status.
Changes:
- Registers the binary sensor platform.
- Adds device and accessory binary sensor entities.
- Adds translated entity names.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
homeassistant/components/lyric/__init__.py |
Registers the platform. |
homeassistant/components/lyric/binary_sensor.py |
Implements three binary sensors. |
homeassistant/components/lyric/strings.json |
Adds entity names. |
| LyricBinarySensorEntityDescription( | ||
| key="vacation_hold", | ||
| translation_key="vacation_hold", | ||
| value_fn=lambda device: device.vacation_hold.enabled, |
There was a problem hiding this comment.
Confirmed — good catch. vacationHold.Enabled (capital E) is what Resideo actually returns; I have a live captured payload showing it. Fixed in timmo001/aiolyric here: clutch2sft/aiolyric#170 (branch fix-vacation-hold-key).
| translation_key="room_motion", | ||
| device_class=BinarySensorDeviceClass.MOTION, | ||
| value_fn=lambda _, accessory: accessory.detect_motion, | ||
| suitable_fn=lambda _, accessory: accessory.type == "IndoorAirSensor", |
There was a problem hiding this comment.
Correct, and already tracked — this needs timmo001/aiolyric#165 (the currentPriority/priority and type/sensorType field fix) merged and released before Room Motion populates for real accounts. Filing it as a dependency rather than blocking on it here, since #165 is a separate project on its own release cycle, and this PR is still correct/complete for what core can control. Once #165 releases, the follow-up is a one-line manifest.json version bump — tracked separately.
| async def async_setup_entry( | ||
| hass: HomeAssistant, | ||
| entry: LyricConfigEntry, | ||
| async_add_entities: AddConfigEntryEntitiesCallback, | ||
| ) -> None: |
| } | ||
| }, | ||
| "entity": { | ||
| "binary_sensor": { |
There was a problem hiding this comment.
Link to documentation pull request: home-assistant/home-assistant.io#46986
Covers entity creation (async_setup_entry), the accessory-type filter that gates Room Motion to IndoorAirSensor accessories only, the Vacation Hold/Device Pairing Enabled value mappings, and diagnostic entity_category metadata. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
homeassistant/components/lyric/binary_sensor.py:44
- Fix the upstream vacation-hold key mapping and bump
aiolyricbefore exposing this entity. The pinned 2.1.1 readsvacationHold["enabled"], but the live/locationspayload in #177010 uses{"Enabled": ...}andLyricBaseObjectperforms no normalization, so this value always falls back toFalseand the entity cannot report an active vacation hold.
value_fn=lambda device: device.vacation_hold.enabled,
homeassistant/components/lyric/binary_sensor.py:62
- Depend on an
aiolyricrelease that mapssensorTypebefore filtering room accessories. In the pinned 2.1.1,LyricAccessory.typereads the nonexistent live-response keytype; live priority responses usesensorType(timmo001/aiolyric#165), so.typeis empty and every room-motion entity is filtered out.
suitable_fn=lambda _, accessory: accessory.type == "IndoorAirSensor",
| side_effect=lambda entities: added.append(list(entities)) | ||
| ) | ||
|
|
||
| await async_setup_entry(MagicMock(), entry, async_add_entities) |
There was a problem hiding this comment.
Fair — the previous version built entities from MagicMocks with already-parsed properties set directly, so it would pass regardless of real payload/key mismatches. Reworked to construct real LyricDevice/LyricRoom/LyricAccessory objects from realistic payloads and assert through entity.is_on, exercising the actual aiolyric parsing boundary. For vacation_hold and room_motion specifically, the currently-pinned aiolyric==2.1.1 still has the exact key mismatches this PR's description already discloses (vacationHold.Enabled vs .enabled, sensorType vs type) — so those two are now xfail(strict=True) against the real live payload shape: they fail today for the same reason the real entities don't populate correctly yet, and strict=True means they'll turn into hard failures (forcing the marker's removal) the moment the companion aiolyric fixes are released and the manifest pin is bumped, rather than silently staying green. device_pairing_enabled has no known mismatch, so it gets a real end-to-end passing test today.
Addresses two review comments on the previous version of this test: - Copilot: tests built entities from MagicMocks with pre-set already- parsed properties, so they'd pass even with the exact live payload/ key mismatches this session found (e.g. vacationHold.Enabled vs .enabled). Now builds real LyricDevice/LyricRoom/LyricAccessory objects from realistic payloads and asserts through entity.is_on, exercising the actual parsing boundary. - Maintainer (@Samielakkad): setup test only checked entity_description.key, not the generated unique_id - the part most likely to collide/drift as more room/accessory sensors are added. Added a dedicated test asserting exact unique_id values across multiple rooms/accessories. Since aiolyric 2.1.1 (the currently pinned release) still has the vacationHold.Enabled and accessory sensorType mismatches, two new tests use xfail(strict=True) against the real live payload shape: they currently fail for the same reason the real entities don't work yet, and will start passing (forcing marker removal, since strict=True turns an unexpected pass into a failure) once the companion aiolyric fixes are released and the manifest pin is bumped. This documents the known gap explicitly instead of hiding it behind mocks. device_pairing_enabled has no known aiolyric mismatch, so it gets a real end-to-end pass today, proving the pattern isn't blanket broken. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Good catch, added. There's now a dedicated test (test_async_setup_entry_generates_correct_unique_ids) using real LyricDevice/LyricRoom/LyricAccessory objects across two rooms (one with a non-qualifying Thermostat-type accessory mixed in) that asserts the exact unique_id set for both device-level and accessory-level entities — confirming the room-id/accessory-id components of the accessory unique_id don't collide. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/components/lyric/test_binary_sensor.py:74
- Set up the config entry through
hass.config_entries.async_setup(...)instead of invoking the platform directly. The tests pylint job rejects this call withhome-assistant-tests-direct-platform-async-setup-entry, so the current branch cannot pass CI.
await async_setup_entry(MagicMock(), entry, async_add_entities)
tests/components/lyric/test_binary_sensor.py:134
- Shorten this docstring and avoid describing this mocked platform-level test as end-to-end. The parsing distinction is already evident from the payload and assertions.
Unlike vacation_hold and room_motion, this field isn't affected by any
known aiolyric field-name mismatch, so this exercises the full
integration boundary (real LyricDevice -> entity.is_on) end-to-end.
tests/components/lyric/test_binary_sensor.py:206
- Remove the duplicated failure narrative from this docstring. The
xfailreason immediately above already documents the upstream schema mismatch.
Built from the actual /priority response shape captured from a live
T9-T10 account - currently fails because accessory.type never matches
"IndoorAirSensor" under the pending key-name fix, so no entity is
created at all.
| """Entity unique_ids are correctly formed and don't collide across rooms. | ||
|
|
||
| Uses real LyricDevice/LyricRoom/LyricAccessory objects (constructed with | ||
| field names aiolyric 2.1.1 already parses correctly) to verify the ID | ||
| formula itself: device-level IDs key off the device MAC, accessory-level | ||
| IDs additionally key off room id and accessory id - the parts most | ||
| likely to collide when more room/accessory sensors are added later. | ||
| """ |
| """Vacation Hold should resolve True given a live-shaped payload. | ||
|
|
||
| Built from the actual API response shape captured from a live account, | ||
| not a synthetic/pre-parsed mock - currently fails because of the | ||
| pending key-name fix, by design. | ||
| """ |
CI enforces this as a hard rule (pylint plugin check home-assistant-tests-direct-platform-async-setup-entry, W7420): a platform's async_setup_entry must not be called directly in tests; use hass.config_entries.async_setup(entry.entry_id) instead. The previous version called binary_sensor.async_setup_entry directly with mocked hass/entry/async_add_entities, which this now-enforced rule flags. Adds tests/components/lyric/conftest.py with fixtures for a fully authenticated config entry (registered via application_credentials, matching test_config_flow.py's proven pattern) and HTTP-level mocks for the /locations and /priority endpoints using the actual live payload shape captured from a real T9-T10 account this session - including the real key names aiolyric 2.1.1 gets wrong (vacationHold .Enabled, accessories[].sensorType, currentPriority vs priority). Entities are looked up via entity_registry.async_get_entity_id() with their known unique_id format rather than guessing generated entity_id slugs, which is deterministic regardless of naming/translation quirks. device_pairing_enabled has no known field-name mismatch, so its test passes for real against the currently-pinned aiolyric release. vacation_hold and room_motion are xfail(strict=True) against the real live payload shape for the same reasons already disclosed in this PR's description; strict=True means they'll turn into hard failures (forcing marker removal) once the companion aiolyric fixes are released and the manifest pin is bumped, rather than silently staying green. Note: I could not run this against a real pytest+hass fixture harness locally (blocked by a Windows/fcntl limitation unrelated to this integration) - built carefully against the proven patterns in this repo's own test_config_flow.py and a structurally similar OAuth2 + DataUpdateCoordinator integration (iotty), but this needs a real pytest run to confirm before considering it fully verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| # Deliberately "LCC-"-prefixed: this branch is intentionally cut from a | ||
| # clean base without the coordinator fix from home-assistant/core#177022 | ||
| # (which removes a device-ID-prefix heuristic gating the /priority fetch). | ||
| # Using a non-"LCC-" ID here would make room-level entities fail to be | ||
| # created for that unrelated, separately-tracked reason, muddying what | ||
| # these tests are actually checking. |
Only the LCC- prefix requirement matters to the fixture; the branch/PR context it referenced would rot as the codebase evolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
joostlek flagged the same conftest.py pattern on PR home-assistant#177067 (this branch's conftest.py was cut from the same base): a bespoke "cred" credential name instead of the DOMAIN default, setting up the lyric domain directly instead of application_credentials, and mocking HTTP responses instead of the aiolyric client itself. Bringing this branch in line proactively rather than waiting for the same comments here. - Register the test credential under DOMAIN, matching every other OAuth2 integration's conftest. - Set up application_credentials directly instead of relying on it being pulled in as a side effect of lyric's manifest dependency. - Patch Lyric.get_locations/get_thermostat_rooms to build real LyricLocation/LyricPriority objects instead of mocking HTTP at the aiohttp transport level. Field-name parsing still runs for real (the properties read straight off the same attributes dicts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 (2)
homeassistant/components/lyric/binary_sensor.py:37
- Align the implementation with the advertised binary sensors. The PR description and linked documentation say this platform adds Room Motion, Vacation Hold, and Device Pairing Enabled, but
DEVICE_BINARY_SENSORSnow defines only Device Pairing Enabled, so the first two entities are never created.
DEVICE_BINARY_SENSORS: list[LyricBinarySensorEntityDescription] = [
LyricBinarySensorEntityDescription(
key="device_pairing_enabled",
translation_key="device_pairing_enabled",
entity_category=EntityCategory.DIAGNOSTIC,
value_fn=lambda device: device.settings.device_pairing_enabled,
suitable_fn=lambda device: True,
),
tests/components/lyric/conftest.py:63
- Shorten this fixture docstring to describe only its behavior. The autospec/Mealie implementation narrative duplicates the body and will become stale when the mock setup changes.
"""Mock the aiolyric client, backed by a real Location parsed from a live-shaped fixture.
Patches Lyric where the integration imports it (autospec, like the
mealie client mock) rather than mocking HTTP responses, so tests
exercise real aiolyric parsing - the same LyricLocation code reading
…ract Resideo's own API docs document locationID as an Integer, and aiolyric's get_thermostat_rooms already types it int, but this fixture quoted it as a string - inherited from early in this session before checking the real docs. No behavior change (the value is only ever interpolated into a URL), but the fixture now matches reality.
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/binary_sensor.py:34
- Implement the advertised Room Motion and Vacation Hold entities, or narrow the PR title and description to the pairing diagnostic.
DEVICE_BINARY_SENSORScurrently defines only Device Pairing Enabled, so the code does not deliver two of the three entities described as this PR's purpose.
DEVICE_BINARY_SENSORS: list[LyricBinarySensorEntityDescription] = [
LyricBinarySensorEntityDescription(
key="device_pairing_enabled",
translation_key="device_pairing_enabled",
entity_category=EntityCategory.DIAGNOSTIC,
tests/components/lyric/conftest.py:65
- Condense this fixture docstring to a single purpose-focused sentence; the patch-location and Mealie comparison narrate implementation details that callers do not need.
Patches Lyric where the integration imports it (autospec, like the
mealie client mock) rather than mocking HTTP responses, so tests
exercise real aiolyric parsing - the same LyricLocation code reading
the actual field names Resideo returns.
"""
tests/components/lyric/test_binary_sensor.py:21
- Activate the unused fixtures with
pytest.mark.usefixturesinstead of injecting them as unused parameters, following the repository's test guidance.
setup_credentials: None,
mock_lyric_api: MagicMock,
- Condense mock_lyric_api's docstring to a single purpose-focused sentence; drop the patch-location/Mealie-comparison narration. - Apply setup_credentials/mock_lyric_api via usefixtures in test_binary_sensor instead of injecting them as unused parameters.
clutch2sft
changed the title
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.
Suppressed comments (1)
homeassistant/components/lyric/binary_sensor.py:32
- Remove the stale Room Motion and Vacation Hold sections from the PR description. The description says these entities were held back, but then lists them again and claims this list introduces Vacation Hold even though the implementation contains only Device Pairing Enabled.
DEVICE_BINARY_SENSORS: list[LyricBinarySensorEntityDescription] = [
LyricBinarySensorEntityDescription(
key="device_pairing_enabled",
|
Rebased forward via a merge commit (merged upstream/dev in, not rebased) — no conflicts this time, and the full tests/components/lyric suite (9 tests) passes against real aiolyric==2.1.2, which landed on dev upstream (#178670) while this was open. @Samielakkad @joostlek — could you take another look when you get a chance? Let me know if there's anything still outstanding beyond what's already been addressed in the review threads. |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Merge conflicts, otherwise looks ok
…otion conftest.py was an add/add conflict against home-assistant#177067's now-merged identical-pattern conftest.py (same setup_credentials/mock_config_entry/ mock_lyric_api fixtures both branches independently wrote). Resolved by keeping upstream's version, minus this branch's dead LOCATION_ID/ DEVICE_ID/MAC_ID constants - unused outside their own definition.
|
Apologies for the conflict churn — in trying to keep each of these as a "smallest possible PR," I didn't think through how much they'd step on each other once any one of them landed. Should've expected that upstreaming five PRs that all touch the same sensor.py/strings.json regions was going to mean repeated resolves every time one merges. This pass I merged latest dev into all four open ones together and confirmed none of them reconflict with each other before pushing, so hopefully this holds a bit better going forward. Thanks for staying on top of the reviews. |
Proposed change
Adds a new
binary_sensorplatform to thelyricintegration, exposing afield that was already being parsed by the coordinator but never surfaced
as an entity:
device.settings.device_pairing_enabledDevice Pairing Enabled introduces a
DEVICE_BINARY_SENSORSlist mirroringthe existing
DEVICE_SENSORSpattern.Room Motion and Vacation Hold were originally part of this PR too, but both
depend on upstream aiolyric fixes that haven't released yet (field-name
mismatches tracked in timmo001/aiolyric#165 and the vacation-hold-key fix),
so they've been split out to a held-back branch to avoid shipping entities
that can't work correctly yet. They'll come back as their own PR once the
dependency chain lands.
IndoorAirSensorroom accessory) —accessory.detect_motiondevice.vacation_hold.enableddevice.settings.device_pairing_enabledRoom Motion follows the same per-accessory pattern already used for Room
Temperature/Humidity in
sensor.py. Vacation Hold and Device Pairing Enabledintroduce a
DEVICE_BINARY_SENSORSlist mirroring the existingDEVICE_SENSORSpattern.
Dependency note: Room Motion only populates once room/accessory data is being
fetched at all, which depends on #177022 (fixes the coordinator silently skipping
this fetch for some thermostats). Vacation Hold and Device Pairing Enabled are
device-level fields from the base
/locationsresponse, which is always fetched —they work independently of #177022.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Documentation added/updated for www.home-assistant.io
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: