-
Notifications
You must be signed in to change notification settings - Fork 902
Add Third Reality dual plug and single plug settings #4109
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
zhaquirks/thirdreality/plug_dual.py
Outdated
| translation_key="on_to_off_delay_ep1", | ||
| fallback_name="Turn off delay", | ||
| ) | ||
| .number( | ||
| attribute_name=ThirdRealityPlugCluster.AttributeDefs.on_to_off_delay.name, | ||
| cluster_id=ThirdRealityPlugCluster.cluster_id, | ||
| endpoint_id=2, | ||
| min_value=0, | ||
| max_value=65535, | ||
| step=1, | ||
| mode="box", | ||
| unit=UnitOfTime.SECONDS, | ||
| device_class=NumberDeviceClass.DURATION, | ||
| translation_key="on_to_off_delay_ep2", | ||
| fallback_name="Turn off delay", | ||
| ) |
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.
The fallback_name will be used as the English translation for the translation_key when this makes its way into HA. Here, the translation keys are different for the two endpoints, but the fallback name is the same.
We should just change the fallback_name to either be "Turn off delay 1" and "Turn off delay 2" or be something like "Turn off delay left" and "Turn off delay right".
In the future, we'll try and add a postfix if there are entities named in the same way on multiple endpoints. For now, it's fine to explicitly make the translation key and fallback name different. Other integrations in HA also do this.
Please also adjust all other occurrences.
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.
zhaquirks/thirdreality/plug_dual.py
Outdated
| .number( | ||
| attribute_name=ThirdRealityPlugCluster.AttributeDefs.on_to_off_delay.name, | ||
| cluster_id=ThirdRealityPlugCluster.cluster_id, | ||
| endpoint_id=1, | ||
| min_value=0, | ||
| max_value=65535, | ||
| step=1, | ||
| mode="box", | ||
| unit=UnitOfTime.SECONDS, | ||
| device_class=NumberDeviceClass.DURATION, | ||
| translation_key="on_to_off_delay_ep1", | ||
| fallback_name="Turn off delay", | ||
| ) | ||
| .number( | ||
| attribute_name=ThirdRealityPlugCluster.AttributeDefs.on_to_off_delay.name, | ||
| cluster_id=ThirdRealityPlugCluster.cluster_id, | ||
| endpoint_id=2, | ||
| min_value=0, | ||
| max_value=65535, | ||
| step=1, | ||
| mode="box", | ||
| unit=UnitOfTime.SECONDS, | ||
| device_class=NumberDeviceClass.DURATION, | ||
| translation_key="on_to_off_delay_ep2", | ||
| fallback_name="Turn off delay", | ||
| ) |
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.
Also, have you checked that the two entities each for the dual plug both show up in HA?
I thought the unique IDs might conflict, as they're just based on the attribute name, but we might also include the endpoint. I'd have to check.
If both appear, this is fine. Otherwise, you can include unique_id_suffix="on_to_off_delay_1" (and _2) for the entities to assign a different UID.
zhaquirks/thirdreality/plug_dual.py
Outdated
|
|
||
| ( | ||
| QuirkBuilder("Third Reality, Inc", "3RDP01072Z") | ||
| .also_applies_to("Third Reality, Inc", "3RWP01073Z") |
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.
Let's use applies_to. Both do the same, but we've switched to just using applies_to in most quirks.
zhaquirks/thirdreality/plug_dual.py
Outdated
| min_value=0, | ||
| max_value=65535, | ||
| step=1, | ||
| mode="box", |
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.
mode=box is fine to keep, but we don't actually set that mode in ZHA yet.
So, it's expected that this does nothing (for now). There's an open issue in the ZHA repo about this.
zhaquirks/thirdreality/plug_dual.py
Outdated
| cluster_id=ThirdRealityPlugCluster.cluster_id, | ||
| endpoint_id=2, | ||
| translation_key="reset_summation_delivered_ep2", | ||
| fallback_name="Reset right summation delivered ep2", # ep2 is right |
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.
This is similar to my previous comment, let's just include either left/right or 1/2, not both and not something abbreviated like "ep2".
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.
Otherwise, I think this looks good. We'll come back to this when the last few things are addressed.
Lastly, there's no reason to ping us every couple of days. We'll try to review PRs as soon as possible.
Sorry, I got it. Also, do I need to @ you every time I make modifications. Or quote your comment |
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.
We'll likely automatically add a postfix number to all entities named the same in the future: zigpy/zha#525
I'll come back to this shortly. For clarity, you can still use different translation keys with left and right though. Those won't be impacted by the change then, since they'll be named differently.
zhaquirks/thirdreality/plug_dual.py
Outdated
| attribute_value=0x01, # 1 reset summation delivered | ||
| cluster_id=ThirdRealityPlugCluster.cluster_id, | ||
| endpoint_id=1, | ||
| translation_key="reset_summation_delivered1", |
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.
Please add an underscore before the 1 at the end. Do this for all translation keys.
In this case, it might make even more sense to rename the translation_key to reset_summation_delivered_left, since that's what the fallback_name (translation value) will be. If the translation were to include 1 or 2, we should mention that in the translation key. But if it's left or right, we should instead mention that.
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.
OK. Modified, please check
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.
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 combined these two quirks into one file/module now, as the manufacturer cluster is exactly the same at the moment.
They can be split apart again if the manufacturer cluster is changed for one device only.
I've also added mode="box", even though that was likely already used due to the big max_value in HA. I've also removed step=1, as that's the default, and we don't really need it with the "box" mode.
Lastly, I changed some names/translations.

Proposed change
This PR added adaptation code for dual_plug and added 2 additional private clusters
Additional information
Checklist
pre-commitchecks pass / the code has been formatted using Black