Skip to content

Make PlatformEntity device bound and add ZclPlatformEntity - #919

Open
puddly wants to merge 12 commits into
zigpy:devfrom
puddly:puddly/zcl-less-entites
Open

puddly wants to merge 12 commits into
zigpy:devfrom
puddly:puddly/zcl-less-entites

Conversation

@puddly

@puddly puddly commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

This PR effectively renames PlatformEntity to ZclPlatformEntity and creates a real PlatformEntity that 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

Copilot AI balanced review requested due to automatic review settings October 2, 2026 17:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity

Open (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.

Comment thread zha/zigbee/device.py
@codecov

codecov Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.22%. Comparing base (5eb118c) to head (e238a57).
⚠️ Report is 3 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_diagnostics on 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 to dev, 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 is endpoint_id → null on RSSISensor/LQISensor. Nothing outside zha_lib_entities changed.
  • zha-quirks 2.2.2 QuirkV2Device._is_entity_removed_by_quirk / _apply_entity_metadata_changes read entity.endpoint.id and call entity.targets_cluster(...) on every discovered entity. The shim covers both, and DeviceCounterSensor only exists on the coordinator, which never gets a QuirkV2Device.
  • Every remaining .endpoint / ._cluster / cluster-config access in zha/ is either ZclPlatformEntity-gated (cluster_config.aggregate_cluster_configs, GroupMember.associated_entities, Device.get_platform_entity) or limited to Sensor siblings (AggregatedClusterPoller.async_update guards with isinstance(entity, Sensor)).
  • RSSI/LQI no longer subclass Sensor, so they lose the Basic-cluster attribute listeners and Sensor.async_update, which returned early for them because _attribute_name is None. 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.py pass and a full regen changes no snapshots (XBee keeps its new sensors through the fallback).

Comment thread zha/zigbee/device.py Outdated
Comment thread zha/application/platforms/sensor/__init__.py

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_cluster prefers a Basic cluster on a ZHA/ZLL-profile endpoint and falls back to any Basic. Both the unique-ID base in Device.discover_entities and the endpoint shim 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 filters change_entity_metadata / prevent_default_entity_creation by endpoint or cluster, because zha-quirks' QuirkV2Device matchers then read entity.endpoint / call targets_cluster on every entity the suffix check didn't already skip. In HA they're silent: Core only calls logging.captureWarnings(True) and keeps Python's default filters, which ignore DeprecationWarning outside __main__. They do show up in the warning summary of zha's own test run (test_quirks_v2_prevent_default_entities and test_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_diagnostics at 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 (matches dev's IDs); Digi-profile-only Basic (XBee) → ep 232 through the fallback; ZLL ep 1 + HA ep 2 → ep 1; no Basic → None. LQISensor inherits it from RSSISensor.
  • With endpoint_id=1, cluster_id=general.Basic.cluster_id added to the -lqi change_entity_metadata filter, test_quirks_v2_change_entity_metadata still passes (matched through the shim).
  • HA Core (homeassistant/bootstrap.py) installs no global warning filters, and its only entity.endpoint read in the zha component is websocket_api.py:278, the one the TODO now names.

Comment thread tests/test_device.py
@@ -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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@puddly
puddly force-pushed the puddly/zcl-less-entites branch 2 times, most recently from a9d9347 to 6b25a98 Compare October 5, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Breaking change (mainly for PRs) code quality Improvement to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants