-
Notifications
You must be signed in to change notification settings - Fork 878
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
base: dev
Are you sure you want to change the base?
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
@puddly @TheJulianJES Please help to check |
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.
.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.
Proposed change
This PR added adaptation code for dual_plug and added 2 additional private clusters
Additional information
Checklist
pre-commit
checks pass / the code has been formatted using Black