[RFC] Move from centralized discovery to entity-driven discovery

[puddly]

As part of a medium-term goal of getting rid of cluster handlers and eventually rewriting entity classes themselves to request attribute reporting config/binding/attribute values, I think a first stepping stone would be to move away from implicit entity registration decorators and have entity objects themselves decide what cluster handlers they want (or if they are not applicable).

This is first draft and has not been runtime tested but I have most device entity tests passing. The few that do not may actually be current bugs with ZHA that are accidentally fixed by this PR.

Basically, instead of this:

@STRICT_MATCH(
    cluster_handler_names=CLUSTER_HANDLER_ON_OFF,
    aux_cluster_handlers={CLUSTER_HANDLER_COLOR, CLUSTER_HANDLER_LEVEL},
)
class Light(BaseClusterHandlerLight, PlatformEntity):
    ...

We do this:

@register_entity
class Light(BaseClusterHandlerLight, PlatformEntity):
    ...
    @classmethod
    def match_cluster_handlers(cls, endpoint: Endpoint) -> ClusterHandlerMatch | None:
        """Match cluster handlers for this entity."""
        # Only create an on/off light if the device type is correct
        if (
            endpoint.zigpy_endpoint.profile_id,
            endpoint.zigpy_endpoint.device_type,
        ) not in {
            # ZHA
            (zha.PROFILE_ID, zha.DeviceType.COLOR_DIMMABLE_LIGHT),
            ...
        }:
            return None

        return ClusterHandlerMatch(
            cluster_handlers=frozenset({CLUSTER_HANDLER_ON_OFF}),
            optional_cluster_handlers=frozenset(
                {CLUSTER_HANDLER_COLOR, CLUSTER_HANDLER_LEVEL}
            ),
            legacy_discovery_unique_id=f"{endpoint.device.ieee}-{endpoint.id}",
        )

There is no more complexity in ClusterHandlerMatch. It lists what cluster handlers are required, what cluster handlers are optional, and what unique ID format will be used for the entity. There are no stop groups or anything else, entity classes are expected to be explicitly coordinated to be mutually exclusive.

This has a few benefits:

1. It moves unique ID calculation logic into the entity itself and makes it 100% explicit, allowing us to more easily migrate.
2. Allows us to fine-tune cluster handler matching per-entity without adding more matching rules or even relying on a weighting system at all.
3. Will allow us to move cluster handler ZCL attribute reporting and binding config into the ZCL entity objects themselves, allowing ZHA to granularly merge binding/reporting config when setting up a device.

[codecov]

Codecov Report

❌ Patch coverage is 99.26380% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.13%. Comparing base (c6ac89e) to head (ae26b14).

Files with missing lines	Patch %	Lines
zha/application/discovery.py	96.94%	4 Missing ⚠️
zha/application/platforms/cover/__init__.py	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #615      +/-   ##
==========================================
+ Coverage   97.02%   97.13%   +0.11%     
==========================================
  Files          63       62       -1     
  Lines       10573    10671      +98     
==========================================
+ Hits        10258    10365     +107     
+ Misses        315      306       -9     

☔ View full report in Codecov by Sentry.
📢 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.

[TheJulianJES]

The test would already always pass if this patch is removed before. It no longer tests that the quirks v2 entities claim the OppleRemoteClusterHandler then. The default ZHA entities will always claim that, hence discovery for those was patched out.

I'll need to have a proper look at this later, but I think these tests essentially test nothing (correctly) now.

[puddly]

The entire unit test could actually be removed, I think. This PR removes the concept of claiming cluster handlers: we claim them to tell ZHA to "use" them, nothing more.

[TheJulianJES]

Ok, I'll still need to take a look at this. Previously, claiming them determined setting up attribute reporting and if they're bound to the coordinator or not.

[TheJulianJES]

This doesn't scale really well, as we have to keep repeating ourselves if we add another entity that's more specific compared to the base one, with the intention of replacing the base one.

The discovery system we had before with the weighting and stop_on_match_group was a bit awkward, but I think it's closer to the new discovery schemas used in Matter (and slowly in Z-Wave).

For reference, see Matter's discovery here and an example of a specific binary sensor replacement here. (The Matter key is used as a unique ID suffix.)
By default, a single Matter entity claims an attribute, adding that to discovered_attributes.
Unless allow_multi=True is set, a second entity will not be discovered for that attribute.

As far as I can see, the Hue sensor replacement only works, as it's first in the list (before the generic/fallback one) (and the default is allow_multi=False). I do think that's not a perfect solution and I'd have switched the default the other way around, as that can be easier seen in tests, but our weighting system sorting the discovered entities based on specificity was actually a nice solution for that IMO.

---

Maybe we should talk about this a bit. What do you think of introducing something similar to allow_multi but in the opposite direction, e.g. exclusive_claim? This would only be set on the more specific entities (e.g. vendor-specific Hue sensor replacing generic one for the Matter example, or, in our case, ForceOnLight replacing the generic one, for example).

If we need to match in a much more specific way, we can simply do it manually, like done here, but in most cases, we should be able to set exclusive_claim=True, only for the more specific entities.
The more specific entities would automatically get higher priority compared to ones that do not have exclusive_claim set, so these entities would be discovered first. For the exclusive_claim=True entities, the primary attribute (or a key, same for both entities) would be added to a list/set of "claimed attributes/keys".
Later, all normal entities with exclusive_claim=False check if their attribute/key is in in the claimed attributes/keys. If not, they're created normally. But if it is, they would skip creation.

This would be a much more simple system than our previous weighting system, and we should be able to keep all benefits you mentioned in the PR description, at least from a quick look, but also simplify and clean this up a lot IMO.

[puddly]

I completely agree! The PR in its current state is still very much a WIP (I haven't actually run it) and the entity-based discovery needs to be completely rewritten; I was curious to see how far I can get with making all of the matching logic painfully verbose and entirely isolated to individual entities.

In this branch, 99% of all entities are simply created based on the presence of a cluster with no further filtering. The 1% of exceptions are now very explicit:

• The OnOff server cluster can be turned into a Light or a Switch based on the endpoint device type.
• The OnOff server cluster can also be turned into a Shade if Level and Shade clusters also exist and the device type is correct.
• The Fan server cluster is not created if a Thermostat entity is currently claiming it, to prevent creating a duplicate entity for fan functionality that's currently part of the complex thermostat entity.

I think that's actually all of them. If we migrate all of the device-specific quirks out of the ZHA codebase and into quirks, the ZCL-only functionality becomes super straightforward. Something like:

from zha.platforms.climate import BaseThermostat

class MyWeirdThermostat(BaseThermostat):
    def __init__(self, device) -> None:
        self._device = device

    async def async_turn_on(self) -> None:
        await self._device.send_packet(...)

(
    QuirkBuilder()
    .applies_to("manuf", "model")
    # Prevent ZCL entity auto-discovery
    .prevent_default_entity_creation(endpoint_id=1)
    # And substitute our own
    .exposes_entity(MyWeirdThermostat)
    .add_to_registry()
)

What do you think of introducing something similar to allow_multi but in the opposite direction, e.g. exclusive_claim?

Having an entity prevent future entities from doing something is an API that I'd like to avoid. Once you move the device-specific "quirks" out of core ZHA, we may not even need claiming/exclusivity as part of the entity matching API. We could eventually use static discovery schemas:

class EntityDiscovery:
    # Mandatory clusters
    server_clusters: frozenset[type[Cluster]]
    client_clusters: frozenset[type[Cluster]]

    # Missing clusters
    not_server_clusters: frozenset[type[Cluster]]
    not_client_clusters: frozenset[type[Cluster]]

    # A set of (profile, device_type)s
    device_types: frozenset[tuple[int, int]]

    # Some way to specify attributes to peek at??

    # Optional clusters will be pulled from the endpoint during initialization

[puddly]

I've added a feature group weighting system nearly identical to the above. It slightly increases discovery complexity but removes the need for all filtering logic in match_cluster_handlers().

At this point, all but a few def match_cluster_handlers functions are entirely static. The only dynamic logic is to determine the unique_id format based on the endpoint profile and device type, which I think we can get rid of.

[puddly]

I'm successfully running this branch locally, as a final test. No entity issues to report.