-
Notifications
You must be signed in to change notification settings - Fork 902
Add Sonoff smart plug S60ZBTPF quirk #4075
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
zhaquirks/sonoff/s60zbtpf.py
Outdated
| async def _read_attributes( | ||
| self, | ||
| attribute_ids: list[t.uint16_t], | ||
| *args, | ||
| manufacturer: int | t.uint16_t | None = None, | ||
| **kwargs, | ||
| ): | ||
| """Read attributes ZCL foundation command.""" | ||
| return await super()._read_attributes( | ||
| attribute_ids, | ||
| *args, | ||
| manufacturer=foundation.ZCLHeader.NO_MANUFACTURER_ID, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| @property | ||
| def _is_manuf_specific(self): | ||
| return False |
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 have manufacturer_id_override = foundation.ZCLHeader.NO_MANUFACTURER_ID at the top.
Are you sure this is needed too?
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.
Do you mean that I should delete lines 82 and 75
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.
Lines 71 to 88. So, does the quirk still work fine if you delete the _read_attributes method and _is_manuf_specific property?
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.
Yes, it still works after deletion.I have deleted the code you mentioned
|
Also, keeping the old PR would have been fine. No need to create a new one after making changes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #4075 +/- ##
==========================================
+ Coverage 91.20% 92.18% +0.98%
==========================================
Files 335 362 +27
Lines 10889 12009 +1120
==========================================
+ Hits 9931 11071 +1140
+ Misses 958 938 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I got some of these myself and will test them against this PR. |
zhaquirks/sonoff/s60zbtpf.py
Outdated
| ) | ||
| .number( | ||
| SonoffCluster.AttributeDefs.ac_current_max_overload.name, | ||
| 0xFC11, |
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.
Can this and all instances of 0xFC11 use SonoffCluster.cluster_id?
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
zhaquirks/sonoff/s60zbtpf.py
Outdated
| @property | ||
| def _is_manuf_specific(self): | ||
| return False |
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 not needed now that manufacturer_id_override: t.uint16_t = .. is used.
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. I'll revise it together after you reply to me
zhaquirks/sonoff/s60zbtpg.py
Outdated
| @property | ||
| def _is_manuf_specific(self): | ||
| return False |
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.
Same here.
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. I'll revise it together after you reply to me
| .number( | ||
| SonoffCluster.AttributeDefs.ac_current_max_overload.name, | ||
| 0xFC11, | ||
| ClusterType.Server, | ||
| 1, | ||
| 0.1, | ||
| 14.0, | ||
| 0.1, | ||
| unit=UnitOfElectricCurrent.AMPERE, | ||
| multiplier=0.001, | ||
| translation_key="ac_current_max_overload", | ||
| device_class=NumberDeviceClass.CURRENT, | ||
| fallback_name="AC current max overload", | ||
| ) |
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 might be missing something, but from a quick look, the values in https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/src/devices/sonoff.ts#L1866 seem to be shared between the two variants of this device.
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.
Yes, some are the same, but some are different, such as overloadProtection
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.
Ah, right. The wattage/amp is different for the F and G variants.
| .number( | ||
| SonoffCluster.AttributeDefs.ac_current_max_overload.name, | ||
| SonoffCluster.cluster_id, | ||
| ClusterType.Server, | ||
| 1, | ||
| 0.1, | ||
| 17.0, | ||
| 0.1, | ||
| unit=UnitOfElectricCurrent.AMPERE, | ||
| multiplier=0.001, | ||
| translation_key="ac_current_max_overload", | ||
| device_class=NumberDeviceClass.CURRENT, | ||
| fallback_name="AC current max overload", | ||
| ) |
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 explicitly specify what parameter we provide by using keyword arguments, so:
| .number( | |
| SonoffCluster.AttributeDefs.ac_current_max_overload.name, | |
| SonoffCluster.cluster_id, | |
| ClusterType.Server, | |
| 1, | |
| 0.1, | |
| 17.0, | |
| 0.1, | |
| unit=UnitOfElectricCurrent.AMPERE, | |
| multiplier=0.001, | |
| translation_key="ac_current_max_overload", | |
| device_class=NumberDeviceClass.CURRENT, | |
| fallback_name="AC current max overload", | |
| ) | |
| .number( | |
| SonoffCluster.AttributeDefs.ac_current_max_overload.name, | |
| SonoffCluster.cluster_id, | |
| cluster_type=ClusterType.Server, | |
| endpoint_id=1, | |
| min_value=0.1, | |
| max_value=17.0, | |
| step=0.1, | |
| unit=UnitOfElectricCurrent.AMPERE, | |
| multiplier=0.001, | |
| translation_key="ac_current_max_overload", | |
| device_class=NumberDeviceClass.CURRENT, | |
| fallback_name="AC current max overload", | |
| ) |
Please also apply that to all other quirks v2 methods.
Omitting the parameter name for attribute_name and cluster_id is fine (first two parameters), as that's a common pattern across all our v2 quirks. We should explicitly provide all other names though.
In the future, we'll hopefully enforce this in the zigpy quirks v2 API.
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,I have added it
zhaquirks/sonoff/s60zbtpf.py
Outdated
| """Attribute definitions.""" | ||
|
|
||
| outlet_control_protect_setting = ZCLAttributeDef( | ||
| name="outlet_control_protect_setting", |
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 name doesn't need to be provided here, since we're already using the new-style AttributeDefs.
We'll just use the actual attribute ("value") name.
Please remove it for all ZCLAttributeDef.
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,I have removed
|
@TheJulianJES @fgsch Is there anything else that needs to be modified |
You may want to remove |
|
@fgsch You mean that I'm going to delete cluster_type=ClusterType.Server and endpoint_id=1 |
Yes, those are the default values, so no need to specify them. |
|
@fgsch I have deleted the default value |
|
LGTM. Thank you! |
|
@TheJulianJES @fgsch Hi,When can you help me merge |
|
@Gatsby-2024 I'm afraid I don't have permissions to merge, so we'll need to wait for @TheJulianJES or someone else to do so. |
|
@TheJulianJES May I ask if there are any other questions? If not, could you help me merge? |
|
@Gatsby-2024 please don't add more changes, and in particular other files. Those should go in a separate PR. |
Proposed change
Additional information
Checklist
pre-commitchecks pass / the code has been formatted using Black