Entity recomputation and add/remove at runtime#517
Conversation
8af5782 to
983586a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #517 +/- ##
==========================================
+ Coverage 97.59% 97.60% +0.01%
==========================================
Files 62 62
Lines 10712 10761 +49
==========================================
+ Hits 10454 10503 +49
Misses 258 258 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d4118a6 to
8662227
Compare
|
When HA receives a HA does keep metadata, like names, icon, and so on for deleted entities since a year or so. So if it's re-added, it will be re-applied by HA. I guess it kind of depends on when this would happen? E.g. after a firmware update or re-interview? entity_rediscovery.movLast month, someone showed me how they reinterviewed a Z-Wave keypad which had 100+ entities. The old names were all "Value 1" and so on, but "worked". After the reinterview, all those entities became unavailable (likely "no longer provided") and completely new entities with proper names (and different unique IDs) were added. I guess we may want to not completely remove the entity from the registry, for now? I'm not really sure... HA does seem to prefer the pattern of completely removing the entity from the registry in these cases.. |
| # TODO: allow all entity information to be serialized and include it here | ||
| unique_id: str |
There was a problem hiding this comment.
Why would we need to do that?
There was a problem hiding this comment.
It's been a while 😅.
For the server+client refactor, I think so that ZHA in Core can react to the "added" event and have all of the entity state ready to go. We then would send state changes as separate events, completing the entire entity lifecycle. Right now, ZHA in Core needs to peek into the entity object's state after it receives the "added" event.
There was a problem hiding this comment.
Ok, that makes sense. I think we can do that in a future PR with the refactor (and especially the .state stuff from #663). For now, I'd just leave this as is.
zha/zigbee/device.py
Outdated
| # TODO: To avoid unnecessary traffic during shutdown, we don't | ||
| # need to emit an event for every entity, just the device | ||
| await self._remove_entity(platform_entity, emit_event=False) |
There was a problem hiding this comment.
Is there still something to do here?
There was a problem hiding this comment.
I don't believe so, no. The original issue was event churn during reconfiguration and startup/shutdown (one event per entity, which was excessive). I think emit_event=False (or a event-catching context manager) is enough.
In the eventual client+server split, we maybe should see if it's worth consolidating these into a single "device added" message that includes all entity state or if it's not really a problem to really send state piecemeal with TCP socket buffering.
|
This PR rebased + adding
Very experimental implementation in HA:
|
Co-authored-by: TheJulianJES <[email protected]> # Conflicts: # tests/test_device.py # zha/zigbee/device.py
|
@TheJulianJES Sure, feel free to force push |
8662227 to
189ed5e
Compare
There was a problem hiding this comment.
Pull request overview
Adds runtime entity lifecycle support to ZHA Device by enabling entity set recomputation and emitting explicit add/remove events when the computed entity set changes.
Changes:
- Introduces
DeviceEntityAddedEvent/DeviceEntityRemovedEventand corresponding const event names. - Adds
Device.recompute_entities()plus internal helpers to add/remove entities and emit lifecycle events. - Extends tests to validate idempotent recomputation and correct add/remove event emission.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| zha/zigbee/device.py | Adds recomputation logic, entity add/remove helpers, and new device entity lifecycle event dataclasses. |
| zha/application/const.py | Defines new event name constants for entity added/removed. |
| zha/application/platforms/sensor/init.py | Adjusts is_supported_in_list to ignore self (needed for recomputation checks). |
| zha/application/platforms/button/init.py | Adjusts is_supported_in_list to ignore self (needed for recomputation checks). |
| tests/test_discover.py | Calls recompute_entities() during device-file discovery test to assert idempotency. |
| tests/test_device.py | Adds coverage for recomputation-driven add/remove behavior and duplicate/nonexistent add/remove error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
521e5bd to
5f81896
Compare
43833fa to
0b0845f
Compare
…th a value `remove_unsupported_attribute` was removed in recent zigpy methods. Calling `update_attribute` with a value also marks the attribute as supported again.
Depending on if this is set, HA can either just unload the entity, or fully remove it from the entity registry.
4dd2e51 to
5123a77
Compare
|
I think an initial version of this should be ready to merge, so we can get the HA part ready soon and I can put up my re-interview PR for ZHA, so we may be able to get that all in for next month. 😅 One change I've made (which might change again in the future) is to not emit the individual entity events on startup (first initialization) of the device, as HA already gets a signal to add entities from the "device fully initialized event". |
There was a problem hiding this comment.
Pull request overview
This PR adds runtime entity set reconciliation to ZHA devices, including explicit “entity added/removed” events, and extends tests to validate idempotency and event emission behavior across initialization and recomputation.
Changes:
- Introduces
DeviceEntityAddedEvent/DeviceEntityRemovedEventand emits them when entities are added/removed at runtime. - Adds
Device.recompute_entities()to remove unsupported entities and discover/add new ones. - Updates entity list support checks (sensor/button) and expands tests to cover initialization vs re-initialization event behavior and recomputation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
zha/zigbee/device.py |
Adds new entity add/remove events, refactors entity add/remove bookkeeping, and introduces recompute_entities() and initialization event gating. |
zha/application/platforms/sensor/__init__.py |
Adjusts is_supported_in_list() to avoid self-matching during recomputation. |
zha/application/platforms/button/__init__.py |
Adjusts is_supported_in_list() to avoid self-matching during recomputation. |
zha/application/const.py |
Adds constants for the new device entity add/remove event types. |
tests/test_discover.py |
Adds a recomputation idempotency check during device discovery tests and reorders cleanup. |
tests/test_device.py |
Adds coverage for initial init vs re-init event emission, recomputation add/remove behavior, and duplicate/nonexistent entity error cases. |
Comments suppressed due to low confidence (1)
zha/zigbee/device.py:1531
- Primary entity recomputation can get stuck after entity removal:
_compute_primary_entity()sets all non-winning candidates toprimary=False, and later excludes those from candidacy viae._attr_primary is not False. If the current primary entity is removed and no new entities are discovered, the remaining entities may all be permanently ineligible, leaving the device with no primary entity. Consider not setting non-winners toFalse(e.g., leave asNone), or otherwise resetting previously weight-computedprimaryflags before re-running the selection logic.
candidates = [
e
for e in entities
if e.enabled and hasattr(e, "info_object") and e._attr_primary is not False
]
candidates.sort(reverse=True, key=lambda e: e.primary_weight)
if not candidates:
return
winner = candidates[0]
others = candidates[1:]
# We have a clear winner
if not others or winner.primary_weight > others[0].primary_weight:
winner.primary = True
del winner.info_object
for entity in others:
entity.primary = False
del entity.info_object
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entity.on_add() | ||
| self._pending_entities.append(entity) | ||
|
|
||
| async def async_initialize(self, from_cache: bool = False) -> None: | ||
| """Initialize cluster handlers.""" | ||
| self.debug("started initialization") | ||
| def _add_entity(self, entity: PlatformEntity, *, emit_event: bool = True) -> None: | ||
| """Add an entity to the device.""" |
There was a problem hiding this comment.
_discover_new_entities() calls entity.on_add() and queues the entity in _pending_entities. Since async_configure() also calls _discover_new_entities() only to claim cluster handlers (and does not subsequently call _add_pending_entities()), this can leave newly-created entities “half-added” for the lifetime of the device, leaking any side effects performed in on_add() (e.g., RSSI/LQI sensors register global updater listeners in on_add()). Consider separating “claim cluster handlers” discovery from “entity add” discovery (or adding a flag to _discover_new_entities() to skip on_add()/pending-queueing when used by async_configure()), and ensure pending entities are always either finalized via _add_pending_entities() or explicitly cleaned up immediately.
There was a problem hiding this comment.
See comment below and this issue:
This will be investigated at a later point.
|
The "Comment[s] suppressed due to low confidence" from here is valid IMO: #517 (review) But I think we can't really fix unless we can determine whether the Pre-existing issues flagged by Copilot (not introduced by this PR)
|
|
Everything seems to work fine (without any HA changes – just not getting new functionality then). I think we can merge this for now. I'll create an issue for the discovery issues mentioned above. Issue: |
This PR introduces two new events to the ZHA device object:
DeviceEntityAddedEvent(unique_id: str)DeviceEntityRemovedEvent(unique_id: str)And a new method:
recompute_entities()recompute_entities()will emit the above events when entities are added/removed at runtime. I've added it to the main device discovery unit test to make sure that it's idempotent.Future TODOs:
recompute_entitieson device rejoin to deal with devices that change modes.recompute_entitiesafter device reconfiguration button is clicked.