-
Notifications
You must be signed in to change notification settings - Fork 882
Add Tuya PC321-Z-TY 3 phase power meter #3644
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #3644 +/- ##
==========================================
+ Coverage 91.00% 91.06% +0.05%
==========================================
Files 328 332 +4
Lines 10656 10723 +67
==========================================
+ Hits 9698 9765 +67
Misses 958 958 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…1_TZE200_nslr42tt
@abmantis, @prairiesnpr I've been working on a v2 quirk specifically for supporting multiple Tuya energy meter devices (#3824) - it already has mappings for 4 devices. It would be pretty straightforward to support this device in that quirk by adding the relevant datapoints, I'll add those in this evening if you're happy to go that route (I'm conscious we want to avoid duplication of effort & code). Tagging #3764 too for visibility. |
@jeverley Thanks for the suggestion, but I don't think this device would benefit from that. As far as I know, this device only measures current in one direction, so no need for direction calculation/mitigation. Personally, given the added complexity, I would prefer to keep it simple unless there is something it is really missing. |
I was aiming to create a single file in which we could maintain Tuya energy meter devices consistently (utilising native zigbee clusters rather than custom sensors - this comes with the benefit of translations being available out of the box for the entities). The POC b566139 commit excludes this device from the virtual channel + directionality logic (this is optionally be used for a device if required). I suppose an alternative approach might be to split my PR into two files, one with 'simple' energy meter devices which don't require the energy direction logic to sign values, and those that do. |
…lers into TS0601_TZE200_nslr42tt
I just updated this PR because it had a lot of outdated stuff from the multi_dp branch. You can see from https://github.com/zigpy/zha-device-handlers/pull/3644/files#diff-5da7f6ba0921c69699fbf6b68fb97058fef0d107cbb4193173309e0894833f05 that it doesn't have much that could be reused by other quirks. Regarding using standard zigbee attributes, I tried to do it for most attributes, except for energy since there is no 3 phase energy stuff in the standard. I tried using different endpoints, but ended up going for custom tuya attributes (I don't remember why I decided on that, maybe because of the naming?). I'll check that again before moving this to ready just to be sure. Personally, I prefer to keep the PRs as small as possible instead of bundling multiple changes in one big PR. So I would add this one as a simple quirk, and then add shared stuff later, as a different PR. |
Support for Tuya Zemismart 3 Phase Power Meter _TZE200_v9hkz2yn variant needs to be added as I tested your current quirk and it is not working with the Zemismart 3 phase device. Kindly check this PR [https://github.com//pull/3764] in which I have worked on. This custom quirk is working but it is on v1 quirk and the PR wasn't merged due to the versioning. |
That does not seem to be the same device as the one in this PR. Are the data points the same? |
Yeah, it's not the same device as in your PR, which is why we would need to add it to the PR with v2 quirks. My PR was created using v1 quirks since I wasn't aware of v2 at the time. |
In that case it should be on a new separate PR, not on this one (except it shares the same endpoints and has just a different model id) |
…to TS0601_TZE200_nslr42tt
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, one small (hopefully) last thing. Looks good otherwise.
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.
Thanks!
.removes(LevelControl.cluster_id) | ||
.removes(OnOff.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.
Not sure if these clusters have any use at all, but we can now use prevent_default_entity_creation(endpoint_id=1, cluster_id=OnOff.cluster_id)
as well (instead of completely "removing" the clusters).
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.
They did not seem to do anything at all. Should prevent_default_entity_creation
be used instead for these cases?
.tuya_sensor( | ||
dp_id=134, | ||
attribute_name="device_status", | ||
type=t.int32s, | ||
fallback_name="Device status", | ||
translation_key="device_status", | ||
) |
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.
What does this sensor show? Since it's only numerical..
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 have no ideia, I just added it since it is also on Z2M :/ On mine it is always 0.
Should I remove it?
Proposed change
Device in Z2M: https://www.zigbee2mqtt.io/devices/TS0601_3_phase_clamp_meter.html
Additional information
Requires: #3643
Checklist
pre-commit
checks pass / the code has been formatted using Black