Skip to content
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

Rename quirk_id to exposes_features and allow multiple features in quirks #311

Open
TheJulianJES opened this issue Nov 26, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@TheJulianJES
Copy link
Contributor

TheJulianJES commented Nov 26, 2024

Idea

We should consider renaming quirk_id to exposes_features and allowing the possibility for one quirk to define more than one "exposed feature".

This issue is mostly here to draw up the plan and to make sure we consider everything affected (like cluster handler matching per quirk id).

Additional information

Currently, a quirk (CustomDevice) can only have one quirk_id. There might be cases where we want to match multiple independent entities in ZHA, without having to add new quirks to the entities in ZHA.

For this use-case, we'd want exposes_features to be able to expose multiple features, like, for example, both the "IAS tamper bit" feature and a "Tuya config plugs" feature.
This would allow the (theoretical) example of a Tuya plug with an IAS tamper sensor matching both the special treatment in the cluster handler for Tuya plugs + creating the Tuya config entities and exposing the IAS tamper bit as an entity.

For new devices, this would require no changes to ZHA then, as we can just add all exposed features of a device to the quirk (instead of needing to give devices that support multiple features a new quirk id each and modify the entities in ZHA to also match those). This is how a (v1) quirk with multiple exposed features would set them:
exposes_features={IAS_TAMPER_BIT, TUYA_PLUG_ONOFF}

Tamper bit entity example

This would be somewhat relevant for PRs like this:

Currently, we could already make that PR work if a device only needs a one quirk_id.
We could allow the tamper entity to match on a quirk id defined in zha-quirks (e.g. IAS_TAMPER_BIT).
For devices that support the tamper bit, we'd just need to set the quirk_id attribute on the CustomDevice class in zha-quirks.

An example of the match decorator for the tamper entity matching on an IAS tamper bit quirk id.

@MULTI_MATCH(
    cluster_handler_names=CLUSTER_HANDLER_ON_OFF, quirk_ids={IAS_TAMPER_BIT}  # can be a list or a string
)

This would be renamed to exposes_features if we want to do this.

Quirks v2

We'll also want to expose quirk_id / the new exposes_features via the quirk v2 API, so a quirk can be really compact to expose a feature for supporting devices.

Considerations for cluster handler matching

home-assistant/core#101709 added support for matching a cluster handler per quirk id, so the cluster id can be ignored.
This was used to get rid of an "ugly hack" that hard-coded a different cluster handler for the Xiaomi vibration sensor's DoorLock cluster (that's actually a MultistateInput cluster). It's also used for Danfoss devices to allow them to match to different cluster handler, based on the ZCL Thermostat cluster handler. It avoids putting manufacturer-related checks in the ZCL cluster handlers.

Obviously, we can only match a single cluster handler per cluster. exposes_features would allow for one device/quirk to have multiple exposed features.
We could just match the first exposed feature for the cluster handler (and have it be the standard that the first exposed feature in the list of a quirk is the one to define the matched cluster handler).
Since there are so few devices that actually need this, I think that approach would be fine (and still clean enough).

Another approach would be to have both quirk_id, just for the custom cluster handler matching, and exposes_features for the rest (entities + custom initialization in existing cluster handlers if needed).
Personally, I think I'd prefer the above variant with using the first "exposed feature" for cluster matching though.
The Xiaomi vibration sensor and Danfoss sensor would only have a single exposed feature, so there's nothing that can be confused.

Related:

@registries.CLUSTER_HANDLER_REGISTRY.register(
DoorLock.cluster_id, XIAOMI_AQARA_VIBRATION_AQ1
)
class XiaomiVibrationAQ1ClusterHandler(MultistateInputClusterHandler):
"""Xiaomi DoorLock Cluster is in fact a MultiStateInput Cluster."""

Backwards compatibility

We could also consider being backwards compatible to quirk_id.
I don't think a lot of custom quirks use quirk_id though, so I likely wouldn't implement this. If we want/need to, it would be relatively simple at least.

@abmantis
Copy link

An extra use-case is for 3 phase metering devices. In addition to adding entities for ph_b and ph_c, we could also have a better naming for the "phase a". Since phase a uses the same attribute as single phase devices, it shows as "Current", while the other two phases would show as "Current B" and "Current C".
Using exposes_features would allow us to have a proper name for "Current A".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants