Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Device-bound RSSI/LQI discovery currently crashes declarative-quirk devices because quirk matchers still dereference entity.endpoint.
Review effort: Balanced
Findings: 1
What changed in this PR
Separates device-bound entities from ZCL-backed entities by introducing ZclPlatformEntity, enabling non-ZCL entity discovery.
Changes:
- Moves endpoint/cluster behavior into
ZclPlatformEntity. - Migrates existing ZCL entities and device-level diagnostic sensors.
- Updates tests and device snapshots for nullable endpoint IDs.
| File | Description |
|---|---|
zha/application/platforms/__init__.py |
Splits device and ZCL entity bases. |
zha/application/discovery.py |
Types endpoint discovery as ZCL-backed. |
zha/application/platforms/{alarm_control_panel,binary_sensor,button,climate,cover,device_tracker,event,fan,light,lock,number,select,sensor,siren,switch,update,virtual} |
Migrates relevant entity classes. |
zha/zigbee/{cluster_config,device,group}.py |
Handles device-only versus ZCL entities. |
tests/test_{climate,device,platform_event,platform_infrared,platform_valve}.py |
Updates test entity inheritance and typing. |
tests/data/devices/*.json |
Records nullable endpoints for device-bound diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #919 +/- ##
==========================================
- Coverage 97.23% 97.22% -0.01%
==========================================
Files 61 61
Lines 10657 10665 +8
==========================================
+ Hits 10362 10369 +7
- Misses 295 296 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Clean split. I found one small RSSI/LQI unique-ID edge case that's worth closing before merge, plus a note on the compatibility shim. Nothing blocking.
Must-address (small): RSSI/LQI endpoint selection no longer applies the old profile filter. The old ClusterMatch on Basic had the default profile_ids={ZHA, ZLL} filter (and skipped renamed clusters). Discovery went endpoint by endpoint, and is_supported_in_list kept the first match. Device.basic_cluster takes the first Basic in-cluster on any profile. On a device whose first Basic endpoint has a non-HA/ZLL profile and whose later endpoint has an HA Basic, the unique IDs move from <ieee>-2-0-rssi/lqi to <ieee>-1-0-rssi/lqi, and the existing entities are orphaned. I reproduced this with a two-endpoint mock (ep 1 profile 0xC105 + Basic, ep 2 profile 0x0104 + Basic): dev gives -2-0-…, this branch gives -1-0-…. None of the 873 snapshots hit it: digi-xbee3 is the only device whose first Basic is on a non-HA/ZLL endpoint, and it has no other Basic, so it only gains the two new sensors. Real-world impact is small, but the fix is cheap. See the inline suggestion.
Note: the RSSI/LQI endpoint shim is also load-bearing for HA Core, not just for zha-quirks. websocket_get_groupable_devices (homeassistant/components/zha/websocket_api.py:278) evaluates entity_ref.entity_data.entity.endpoint.id for every entity of a groupable device. If the shim is removed as the TODO describes, or another device-bound entity without endpoint is added to a regular device, zha/devices/groupable raises AttributeError for any device with a Groups server cluster where such an entity is enabled. HA only registers entity refs in async_added_to_hass, so the disabled-by-default RSSI/LQI only trigger it once a user enables them. Please name that consumer in the TODO so it gets updated (e.g. an isinstance/getattr guard there) before the shim goes. (DeviceCounterSensor is not affected today. The active coordinator is always groupable, but zigpy's default coordinator endpoints have no Groups in-cluster, so the comprehension never runs for it.)
Minor payload change, probably intended: GroupMember.associated_entities switched from hasattr(entity, "endpoint") to isinstance(entity, ZclPlatformEntity). RSSI/LQI still have endpoint through the shim, but they are no longer listed in the Basic endpoint member's member_info.entities / all_member_entity_unique_ids. HA Core builds its own list from ha_entity_refs, so nothing breaks there.
Optional: test_quirks_v2_change_entity_metadata dropped endpoint_id=1 from the -lqi metadata change. With the shim in place, endpoint_id=1, cluster_id=Basic.cluster_id still matches the LQI sensor; I checked this locally and the test passes with both filters added back. Keeping them would give the shim's targets_cluster an explicit test, which only the _apply_entity_metadata_changes / _is_entity_removed_by_quirk matchers would otherwise exercise. Either way, the comment at tests/test_device.py:1411 still says "first filter matches by endpoint_id=1" and is now stale.
Verified (7 checks)
- Full test suite on the PR head: 1395 passed. mypy in the worktree venv (with the real zigpy/zhaquirks installed): no issues.
tools.regenerate_diagnosticson the PR head reproduces all committed snapshots byte-for-byte.- Snapshot audit over all 867 changed device files: the entity
(platform, unique_id)sets are identical todev, apart from the two new digi-xbee3 sensors (-232-0-rssi/lqi; the Basic cluster is only on the Digi-profile ep 232, so the old profile filter never matched). The only field change isendpoint_id→nullon RSSISensor/LQISensor. Nothing outsidezha_lib_entitieschanged. - zha-quirks 2.2.2
QuirkV2Device._is_entity_removed_by_quirk/_apply_entity_metadata_changesreadentity.endpoint.idand callentity.targets_cluster(...)on every discovered entity. The shim covers both, andDeviceCounterSensoronly exists on the coordinator, which never gets aQuirkV2Device. - Every remaining
.endpoint/._cluster/ cluster-config access inzha/is eitherZclPlatformEntity-gated (cluster_config.aggregate_cluster_configs,GroupMember.associated_entities,Device.get_platform_entity) or limited toSensorsiblings (AggregatedClusterPoller.async_updateguards withisinstance(entity, Sensor)). - RSSI/LQI no longer subclass
Sensor, so they lose the Basic-cluster attribute listeners andSensor.async_update, which returned early for them because_attribute_nameisNone. Their state still updates through the global updater. - The suggested profile-aware selection (below) restores
dev's IDs in the mock repro. With it,tests/test_device.py+tests/test_sensor.pypass and a full regen changes no snapshots (XBee keeps its new sensors through the fallback).
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Both substantive points from my previous review are addressed in 33f7097, and the delta raises nothing new. Approving, with one leftover nit inline: the optional test point was only half taken, and the updated comment now describes a filter the test doesn't have.
- RSSI/LQI unique IDs:
RSSISensor.legacy_basic_clusterprefers a Basic cluster on a ZHA/ZLL-profile endpoint and falls back to any Basic. Both the unique-ID base inDevice.discover_entitiesand theendpointshim now use it, so they can't disagree. - HA Core consumer: the shim's TODO now names
websocket_get_groupable_devices. - New
DeprecationWarnings on the shim: these fire for RSSI/LQI entities on v2-quirked devices whose quirk filterschange_entity_metadata/prevent_default_entity_creationby endpoint or cluster, because zha-quirks'QuirkV2Devicematchers then readentity.endpoint/ calltargets_clusteron every entity the suffix check didn't already skip. In HA they're silent: Core only callslogging.captureWarnings(True)and keeps Python's default filters, which ignoreDeprecationWarningoutside__main__. They do show up in the warning summary of zha's own test run (test_quirks_v2_prevent_default_entitiesandtest_quirks_v2_change_entity_metadata). Just for tracking: there's no open zha-quirks PR yet for the matcher change the TODO is waiting on.
Verified (5 checks)
- Full test suite at 33f7097: 1395 passed. mypy in the worktree venv: no issues.
tools.regenerate_diagnosticsat 33f7097 reproduces every committed snapshot byte-for-byte, so the endpoint selection change moves no existing unique ID.- Probe of
legacy_basic_cluster: custom-profile ep 1 + HA ep 2 → ep 2 (matchesdev's IDs); Digi-profile-only Basic (XBee) → ep 232 through the fallback; ZLL ep 1 + HA ep 2 → ep 1; no Basic →None.LQISensorinherits it fromRSSISensor. - With
endpoint_id=1, cluster_id=general.Basic.cluster_idadded to the-lqichange_entity_metadatafilter,test_quirks_v2_change_entity_metadatastill passes (matched through the shim). - HA Core (
homeassistant/bootstrap.py) installs no global warning filters, and its onlyentity.endpointread in the zha component iswebsocket_api.py:278, the one the TODO now names.
| @@ -1409,7 +1408,7 @@ def filter_func(entity) -> bool: | |||
|
|
|||
| assert lqi_entity is not None, "LQI sensor entity should exist" | |||
|
|
|||
| # Verify metadata changes were applied - first filter matches by endpoint_id=1 and unique_id_suffix="-lqi" | |||
| # Verify metadata changes were applied - first filter matches by endpoint_id=1, cluster_id=Basic and unique_id_suffix="-lqi" | |||
There was a problem hiding this comment.
Nit: this comment now says the first filter matches by endpoint_id=1, cluster_id=Basic, but that change_entity_metadata(...) call (line 1355) only has unique_id_suffix="-lqi". Either put endpoint_id=1 back and add cluster_id=general.Basic.cluster_id (I checked that the test still passes that way, and it gives the shim's endpoint/targets_cluster an explicit test), or drop that part of the comment.
a9d9347 to
6b25a98
Compare

This PR effectively renames
PlatformEntitytoZclPlatformEntityand creates a realPlatformEntitythat is bound solely to the device. This allows entities to be created that are not tied to the ZCL. I've migrated LQI, RSSI, and coordinator device counter sensors as part of this implementation.Required for #868
Part of OpenHomeFoundation/roadmap#195