Match ZHA Custom ClusterHandler on a Custom Cluster using a unique id for the quirk

[Caius-Bonus]

Because some devices implement Standard Zigbee Clusters differently than the standard prescribes (examples: Xiaomi Vibration AQ1 and Danfoss Ally TRV), there is a need to match ClusterHandlers on a Specific Quirk. Additionally it is conceivable that at some point certain manufacturers use the same Manufacturer Specific Cluster IDs.

This is now done for Vibration AQ1 using an "Ugly Hack" as it is called in the comment. The code for the Danfoss Ally TRV has not been merged as of yet, but an approach explored there overwrites the ClusterHandler in the ZIGBEE_CLUSTER_HANDLER_REGISTRY. Then it only performs certain actions if the ClusterHandler has a specific quirk_class. This has however as a consequence that all Standard Clusters have a custom type, which I consider to be ugly. This PR would implement that functionality without hacks.

Proposed change

Change ZIGBEE_CLUSTER_HANDLER_REGISTRY to type: dict[ClusterId, dict[QuirkId, ClusterHandler]].

Change ProbeEndpoint.discover_by_cluster_id(self, endpoint: Endpoint) and Endpoint.add_all_cluster_handlers(self) to get a CustomCluster from the new ZIGBEE_CLUSTER_HANDLER_REGISTRY.

Remove "Ugly Hack" from ProbeEndpoint.discover_by_cluster_id(self, endpoint: Endpoint) and fulfill that functionality with the new registry.

Currently it creates a new decorator (DictOfDictRegistry) and a new registry (CUSTOM_CLUSTER_HANDLER_REGISTRY), an alternative would be to merge the CUSTOM_CLUSTER_HANDLER_REGISTRY and ZIGBEE_CLUSTER_HANDLER_REGISTRY and have all Standard Clusters be quirk_name=None. This would reduce LOC Added, but increase Lines Changed in this PR.
Edit: It does now change the original registry to accomodate the custom cluster handlers.

Because this PR requires a unique ID for a quirk requiring a Custom ClusterHandler, this PR would depend on the addition of this id to zigpy.quirks.__init__.CustomDevice and zhaquirks.xiaomi.aqara.vibration_aq1.VibrationAQ1. It would also be possible to simply match on the quirk_class as is done in the entity matching code (this is however to my knowledge unused and as TheJulianJES said, not very resistant against refactoring). Edit: Quirk ID matching is now merged.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• This PR is related to pr: Add Danfoss Ally thermostat and derivatives to ZHA #86907, Replace ZHA quirk class matching with quirk ID matching #102482
• This PR is dependent on PR: Add quirk id for Xiaomi Aqara Vibration AQ1 zigpy/zha-device-handlers#2681
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• I have followed the perfect PR recommendations
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[home-assistant]

Hey there @dmulcahey, @Adminiuga, @puddly, mind taking a look at this pull request as it has been labeled with an integration (zha) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of zha can trigger bot actions by commenting:

• @home-assistant close Closes the pull request.
• @home-assistant rename Awesome new title Renames the pull request.
• @home-assistant reopen Reopen the pull request.
• @home-assistant unassign zha Removes the current integration label and assignees on the pull request, add the integration domain after the command.

[TheJulianJES]

I haven't forgotten about your Danfoss PR. Like you already know, ZHA currently provides "quirk matching" which isn't ideal at all, as it matches per part of a path (which doesn't even have to be unique now) and refactoring can easily change that path. It also doesn't allow two quirks to provide the same entities without registering the quirk twice in ZHA.

I've already worked a bit on what I proposed in your other PR regarding quirk ID matching (which replaces the currently unused quirk path matching):

• https://gist.github.com/TheJulianJES/6de2b225e512a5418b08625930a53b1a ("Design Doc")
• TheJulianJES@7a4234f (first commit for implementing in ZHA)

puddly mentioned that he'd like to allow creating entities from zha-quirks directly, but I'm not sure what the status is on that at the moment.
But as that likely won't solve all problems (registering cluster handlers, initializing attributes in cluster handlers), I'd personally like to move forward with what I mentioned in my gist above.

If that approach is fine, it should unblock your Danfoss PR, as well as allowing us to tidy up stuff like this (one quirk ID for all plugs sharing this functionaltiy):

core/homeassistant/components/zha/core/cluster_handlers/manufacturerspecific.py

Lines 76 to 97 in 66f43eb

	if self.cluster.endpoint.manufacturer in (
	"_TZE200_7tdtqgwv",
	"_TZE200_amp6tsvy",
	"_TZE200_oisqyl4o",
	"_TZE200_vhy3iakz",
	"_TZ3000_uim07oem",
	"_TZE200_wfxuhoea",
	"_TZE200_tviaymwx",
	"_TZE200_g1ib5ldv",
	"_TZE200_wunufsil",
	"_TZE200_7deq70b8",
	"_TZE200_tz32mtza",
	"_TZE200_2hf7x9n3",
	"_TZE200_aqnazj70",
	"_TZE200_1ozguk6x",
	"_TZE200_k6jhsr0q",
	"_TZE200_9mahtqtg",
	):
	self.ZCL_INIT_ATTRS = {
	"backlight_mode": True,
	"power_on_state": True,
	}

core/homeassistant/components/zha/select.py

Lines 255 to 275 in 66f43eb

	@CONFIG_DIAGNOSTIC_MATCH(
	cluster_handler_names="tuya_manufacturer",
	manufacturers={
	"_TZE200_7tdtqgwv",
	"_TZE200_amp6tsvy",
	"_TZE200_oisqyl4o",
	"_TZE200_vhy3iakz",
	"_TZ3000_uim07oem",
	"_TZE200_wfxuhoea",
	"_TZE200_tviaymwx",
	"_TZE200_g1ib5ldv",
	"_TZE200_wunufsil",
	"_TZE200_7deq70b8",
	"_TZE200_tz32mtza",
	"_TZE200_2hf7x9n3",
	"_TZE200_aqnazj70",
	"_TZE200_1ozguk6x",
	"_TZE200_k6jhsr0q",
	"_TZE200_9mahtqtg",
	},
	)

[TheJulianJES]

What I wrote above doesn't invalidate this PR though (from looking at it), as yours allows "overriding cluster handlers matches" for quirks.

[Caius-Bonus]

Looking at TheJulianJES Design Doc and Code, it seems like this PR follows the same format for the quirk_id. That is nice.

[puddly]

Instead of using shared strings between quirks and ZHA, what do you think about directly importing and registering based on the quirk class itself?

import zhaquirks.xiaomi.aqara

...

@registries.CUSTOM_CLUSTER_HANDLER_REGISTRY.register(
    DoorLock.cluster_id, quirk=zhaquirks.xiaomi.aqara.XiaomiVibrationAQ1
)
class XiaomiVibrationAQ1ClusterHandler(MultistateInput):

That way, any breaking changes in zha-quirks will cause HA Core's unit tests to fail. This will also let us more easily, in the future, to migrate entity registration into the same file as the quirk itself.

[Adminiuga]

That way, any breaking changes in zha-quirks will cause HA Core's unit tests to fail. This will also let us more easily, in the future, to migrate entity registration into the same file as the quirk itself.

On the other hand, non breaking changes to quirks, like re-shuffling module structure (e.g. having all tuya trv quirks moved into tuya.trv.sitterwel_trv) becomes a breaking change

[TheJulianJES]

On the other hand, non breaking changes to quirks, like re-shuffling module structure (e.g. having all tuya trv quirks moved into tuya.trv.sitterwel_trv) becomes a breaking change

Yeah. And -- especially common for Tuya -- devices have slightly differing quirk classes that expose the same custom attributes. When matching via quirk class or path, those new quirk classes would also need to be added to HA.

Having a string allows multiple quirks to share the same "ID" (from here):

Furthermore, it would also allow multiple similar quirks (just different signature for example) to share a quirk_id. This would mean the code in ZHA would not have too be duplicated -- unlike with the current "quirk path matching".

Although not all, many of the Tuya plugs expose the same custom attributes:
https://github.com/zigpy/zha-device-handlers/blob/dev/zhaquirks/tuya/ts011f_plug.py
We'd need to add all of them to HA when matching via quirk class or quirk path. When using a unique string, we can use the same quirk_id for multiple quirks. Another small new TS011F plug quirk then just requires a quirks update, not a HA Core update.

[Adminiuga]

We'd need to add all of them to HA when matching via quirk class or quirk path. When using a unique string, we can use the same quirk_id for multiple quirks

I agree and like this idea. My vote goes for this.

[Caius-Bonus]

I think IDs would be the most intuitive as most things right now are matched on IDs/Strings: attribute name with a cluster name.

A drawback to this however is: this requires knowledge of other quirks to not accidentally duplicate an ID and these IDs are completely fictional.

I think it is important to allow multiple CustomDevice definitions to match on a single ClusterHandler or entity. Importing would be too restrictive for this purpose.
If you want to match on the classes itself, it would be possible to do so by refactoring all quirks into strict hierarchies and matching all quirks in a path.

For example: path.startswith("tuya.tuya_plugs_type1"). "tuya/tuya_plugs_type1.py" could contain all CustomDevice definitions for all tuya plugs that have the same attributes exposed to home assistant. If wanting to refactor this, it could become all quirks in the directory "tuya/tuya_plugs_type1/" and all quirks could be defined in different files.

A drawback to this is however that it requires strict hierarchies to be maintained in zha-quirks, while right now it is quite free as far as I can see.

Both would solve the problem in my opinion, but I don't have a very strong preference.

[TheJulianJES]

I think this is mostly fine. This still requires a quirks bump though, as the quirk ID for the Aqara vibration sensor isn't merged into HA yet.

Also, we have tests for custom ZHA entity matches (via quirk ID) to check if the quirk ID exists in any quirk:

core/tests/components/zha/test_registries.py

Lines 552 to 582 in e755dd8

	def test_quirk_classes() -> None:
	"""Make sure that all quirk IDs in components matches exist."""
	def quirk_class_validator(value):
	"""Validate quirk IDs during self test."""
	if callable(value):
	# Callables cannot be tested
	return
	if isinstance(value, (frozenset, set, list)):
	for v in value:
	# Unpack the value if needed
	quirk_class_validator(v)
	return
	if value not in all_quirk_ids:
	raise ValueError(f"Quirk ID '{value}' does not exist.")
	# get all quirk ID from zigpy quirks registry
	all_quirk_ids = []
	for manufacturer in zigpy_quirks._DEVICE_REGISTRY._registry.values():
	for model_quirk_list in manufacturer.values():
	for quirk in model_quirk_list:
	quirk_id = getattr(quirk, ATTR_QUIRK_ID, None)
	if quirk_id is not None and quirk_id not in all_quirk_ids:
	all_quirk_ids.append(quirk_id)
	del quirk, model_quirk_list, manufacturer
	# validate all quirk IDs used in component match rules
	for rule, _ in iter_all_rules():
	quirk_class_validator(rule.quirk_ids)

Should we also do the same for custom cluster handlers?

[Caius-Bonus]

I think this is mostly fine. This still requires a quirks bump though, as the quirk ID for the Aqara vibration sensor isn't merged into HA yet.

There hasn't been a new release yet, or should I assume the new version number is going to be 0.0.107?
Should I do that in this PR or in a separate PR? Separate PR.

[Caius-Bonus]

Should we also do the same for custom cluster handlers?

@TheJulianJES I have added tests for checking whether the quirk ids and cluster ids exist. When the version bump is done, this passes the tests.

[TheJulianJES]

There hasn't been a new release yet, or should I assume the new version number is going to be 0.0.107?

I think the bump will be in anothet PR, as there are more changes unrelated to this PR.
I've waited for now because of the Danfoss stuff, but we could also put that in another release.

For Danfoss, we have to merge the quirks bump before the Danfoss PR, then rebase your Danfoss PR to use the quirk ID constant, then finally merge the Danfoss PR shortly afterwards.

Maybe it would be better if we have another library bump with just the Danfoss quirk changed and then combine that secondary bump with your Danfoss PR?
I'll have a look at this later.

Also, put your PR in draft until the bump is done and it can be merged. (I'll still take a look at the test changes later)

[Caius-Bonus]

Also, put your PR in draft until the bump is done and it can be merged. (I'll still take a look at the test changes later)

I forgot. Done.

Maybe it would be better if we have another library bump with just the Danfoss quirk changed and then combine that secondary bump with your Danfoss PR?

I think it would be fine to merge both the Danfoss quirk PR and the Xiaomi quirk PR in the same version bump. I can test it locally with some hacks and when the version bump is done, I can use "Update Branch" and it will do the online tests too.

[TheJulianJES]

I think it would be fine to merge both the Danfoss quirk PR and the Xiaomi quirk PR in the same version bump.

As time was running a bit short, the quirks bump doesn't include the Danfoss PR for now.
This PR can be rebased/updated, as soon as the quirks bump is merged.

[Caius-Bonus]

I don't quite understand what codecov/patch/required is and what I can do about it.

[TheJulianJES]

I can have a closer look later, but it likely means Codecov thinks your patch isn't covered by tests fully.
There were some changes to ZHA code on dev that aren't in your branch yet.

Try to rebase the PR onto the latest changes from dev (or merge the updated dev branch into your branch). Might even be able to do it using the GitHub UI.

[Caius-Bonus]

Rebase fixed it. Ready for merge.

[dmulcahey]

@TheJulianJES do we wanna get this in the beta?

[TheJulianJES]

@dmulcahey Yeah, should be fine IMO. It's a requirement for a Danfoss-related PR in the future. This can be merged standalone though.

[TheJulianJES]

LGTM.

[dmulcahey]

@balloob can we get @TheJulianJES added as a HA org member please?