Skip to content

[RFC] Explicit entity object discovery, lifetimes, and registration #365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
Feb 26, 2025

Conversation

puddly
Copy link
Contributor

@puddly puddly commented Jan 28, 2025

PlatformEntity objects, among others, rely on implicit "registration" inside of __init__ by mutating a global dictionary that ZHA flushes when creating new devices. This is a bit difficult to work with, in my opinion, and is somewhat preventing zigpy/zigpy#1535 from being implemented.

This PR:

  1. Construct entity objects early.
  2. Implement an on_add interface to mirror on_remove and allow for entity object creation to not rely on any sort of global state.
  3. Allow entities to explicitly indicate if they are not supported by the current device via Entity.is_supported (this is mostly a copy/paste of the create_platform_entity logic).
  4. Funnel all device entity discovery explicitly via gateway.py.

@puddly
Copy link
Contributor Author

puddly commented Jan 28, 2025

This PR is "done" architecturally, for the most part.

Main changes:

  • All entities must compute capability bitmasks and other feature flags in Entity.recompute_capabilities().
    This is required because we effectively perform multiple passes when setting up entities: an initial pass to discover entities and claim cluster handlers, a second pass to initialize the cluster handlers and possibly populate the attribute cache, and a third pass to actually validate whether or not the discovered entities are applicable to the device. In the future, it would be nice to consolidate this functionality entirely into the Entity subclass.
  • Entities indicate device applicability via Entity._is_supported().
    For the same reason as above, since entity objects are created at runtime, we need a way to signal applicability to a device independent of creation. I've also added a new _attr_is_always_supported to allow entities that do not abide by these rules to be easily flagged (quirks v2, group, RSSI/LQI, and device counters).
  • All entity objects' listeners, callbacks, and tasks must be registered in Entity.on_add() and deregistered with Entity.on_remove().
    For simplicity, there is also a Entity._on_remove_callbacks list that will be iterated over by the default Entity.on_remove() callback.

That about covers everything. This is a very, very rough first pass PR and is in not in a state for review beyond architecture changes.

@puddly puddly force-pushed the puddly/explicit-entity-registry branch from 0f4faef to fb3dfd6 Compare January 30, 2025 21:51
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 99.35484% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.40%. Comparing base (c23285f) to head (832fc62).
Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
zha/application/gateway.py 90.00% 1 Missing ⚠️
zha/application/platforms/light/__init__.py 97.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #365      +/-   ##
==========================================
- Coverage   96.58%   96.40%   -0.19%     
==========================================
  Files          61       61              
  Lines        9526     9584      +58     
==========================================
+ Hits         9201     9239      +38     
- Misses        325      345      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@puddly puddly marked this pull request as ready for review February 3, 2025 19:15
@puddly puddly merged commit 7ac35c9 into zigpy:dev Feb 26, 2025
8 of 9 checks passed
jeverley added a commit to jeverley/zha that referenced this pull request Mar 13, 2025
puddly pushed a commit that referenced this pull request Mar 25, 2025
* Improvements to Cover PlatformEntity status methods

This aims to address issues in correctly representing the current cover status.

 - Fixes issue where the movement state instigated by 'go to' commands is not cleared upon completion
 - Implements movement timeout and previous sate comparison to determine device instigated changes
 - Removed target position entity attributes (we don't need to restore these)
 - Fix tests for tilt axis

* Test coverage for timeout and device instigated movement

* Use common functions for instigating target tracking

* Calculate axis states only if their position/target has changed

More efficient on devices that frequently report position updates.

* Improve debug logging outputs

* Use CoverState StrEnum in const for states rather than literals

* Readability improvement for _determine_state method

* Simplify stop_cover methods

* Fix _attr_device_class for WindowCoveringType.Rollershade 

The previous logic was failing to populate the `_attr_device_class` because WindowCoveringType.Rollershade == 0x00.

We should instead check for `is not None`.

* Amend tests following shade device_class fix

* Include missing ATTR_CURRENT_TILT_POSITION in state property

* Docstring updates and use of converter functions for Shade PlatformEntity

* Guard against state change when target is equal to current position

This prevents ZHA from incorrectly marking the state as opening/closing if the cover is already at its target.

This could occur if the cover immediately reported an attribute update after receiving the command.

* Add test for dynamic movement timeout

* Full test coverage for tilt axis

* Remove unused seconds arg from start_movement_timer

* Only debug log timer cancellation if timer existed

* Refinements to cover property handling

 - Properties return None when the value not known (per https://developers.home-assistant.io/docs/core/entity/cover/)
 - Move shade _attr_supported_features out of init
 - Improvements to common zcl/ha value conversion functions

* Remove position tolerance

This scenario can be handled using the timeout

* Apply pre-commit auto fixes

* Handle concurrent movement of lift and tilt axis

- Use separate timers for lift and tilt
- Add test for concurrent movement state handling

* Simplify async_update_state

* Robustness and clarity improvements

- Clear active transition when setting a new target position
- Use properties to abstract from previous position deque mechanism
- Consider whether a transition is active when determining state

* Clean up determine_state method

* Implement recompute_capabilities

Per #365

* Apply pre-commit auto fixes

* Ensure supported_features is not cached

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants