-
Notifications
You must be signed in to change notification settings - Fork 707
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
Refactor Aqara remote switch H1 quirk to v2 format to support all variants #3564
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #3564 +/- ##
==========================================
- Coverage 89.79% 89.78% -0.01%
==========================================
Files 323 323
Lines 10414 10409 -5
==========================================
- Hits 9351 9346 -5
Misses 1063 1063 ☔ View full report in Codecov by Sentry. |
I'm re-implementing this as a v2 quirk to expose the configuration options as well as exploring whether this resolves the unknown device bug home-assistant/core#127874 - we shall see. |
This is to cover all device variants
zhaquirks/xiaomi/aqara/remote_h1.py
Outdated
.adds(Identify) | ||
.adds(Identify, cluster_type=ClusterType.Client) | ||
.adds(Identify, endpoint_id=2) | ||
.adds(Identify, endpoint_id=2, cluster_type=ClusterType.Client) | ||
.adds(Identify, endpoint_id=3) | ||
.adds(Identify, endpoint_id=3, cluster_type=ClusterType.Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we adding all these Identify clusters because the signature reported by the device is sometimes incomplete?
Is there any real function for these on endpoint 2 and 3 (or the client clusters)? If not, I wonder if we can just not add them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified this in 76158d1 to just add the first on endpoint 1 (it seems to be the only one that actually responds).
zhaquirks/xiaomi/aqara/remote_h1.py
Outdated
.friendly_name( | ||
manufacturer="Aqara", model="Wireless Remote Switch H1 (Double Rocker)" | ||
) | ||
.adds(AqaraRemoteManuSpecificCluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, if a cluster is ever reported by the device, we should use .replaces
. It also removes the existing cluster with the same cluster id, should there be one already. It should not error out if there is no cluster with that ID.
Then, replaces
also adds the custom cluster.
I don't see it in the signature
from the v1 quirks, but I think we should still consider using replaces
here, as it's a real cluster present on the device, just not exposed in the signature (by the device).
I guess that could theoretically change. With adds
, we only add the custom cluster if one doesn't exist with the same cluster ID already (again, IIRC). In that case, we would not add the attribute definitions.
I think we should just use replaces
for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, using replace should shield us from future variants which do expose the cluster.
Amended in 76158d1
zhaquirks/xiaomi/aqara/remote_h1.py
Outdated
.adds(MultistateInputCluster) | ||
.adds(MultistateInputCluster, endpoint_id=2) | ||
.adds(MultistateInputCluster, endpoint_id=3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess the same might apply here. These are real clusters, just not exposed in the signature by the current device firmware.
I'd also consider using replaces
for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some variants to expose the cluster in their signature, definitely correct to use replace.
Amended in 76158d1
.adds(OnOff, cluster_type=ClusterType.Client) | ||
.adds(OnOff, endpoint_id=2, cluster_type=ClusterType.Client) | ||
.adds(OnOff, endpoint_id=3, cluster_type=ClusterType.Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are probably fine to leave as-is. If they're not exposed by the device signature, we add them. If they are, we don't add them, but it's still the same cluster, as it's the default ZCL OnOff
one.
I believe these clusters are also used if bound to another device when the remote is in that mode.
zhaquirks/xiaomi/aqara/remote_h1.py
Outdated
( | ||
QuirkBuilder(LUMI, "lumi.remote.b18ac1") | ||
.friendly_name( | ||
manufacturer="Aqara", model="Wireless Remote Switch H1 (Single Rocker)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the current implementation we have for friendly_name
, this might break existing blueprints.
I wonder if we should comment it out for now (until the ZHA implementation is changed?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have commented these out for now in 76158d1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Proposed change
This PR refactors the existing Aqara H1 quirk into the new v2 structure (originally undertaken as I'd encountered a new device variant for the double switch):
Additional information
I've two switches, one of which seems to be intermittently affected by the issue home-assistant/core#127874 whereby it's recognised and works correctly after initial pairing, but following a restart of the ZHA integration or Home Assistant it's recognised as unk_manufacturer, unk_model and no longer functions.
Update:
I haven't encountered the issue above in the last week with the new v2 quirk (following multiple reboots and zha reloads).
Checklist
pre-commit
checks pass / the code has been formatted using Black