Skip to content

Conversation

abmantis
Copy link
Contributor

@abmantis abmantis commented Dec 25, 2024

Proposed change

Device in Z2M: https://www.zigbee2mqtt.io/devices/TS0601_3_phase_clamp_meter.html

Additional information

Requires: #3643

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

Copy link

codecov bot commented Dec 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.06%. Comparing base (62765a4) to head (687dbe8).
Report is 6 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheJulianJES TheJulianJES added Tuya Request/PR regarding a Tuya device needs review This PR should be reviewed soon, as it generally looks good. labels Dec 26, 2024
@jeverley
Copy link
Contributor

jeverley commented Feb 4, 2025

@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.

@abmantis
Copy link
Contributor Author

abmantis commented Feb 6, 2025

@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.
Also, I am not sure we should have virtual channels, as those can be added by the users on HA side, if they want to. That would add extra complexity to the quirks, making it harder for people to contribute to them.

Personally, given the added complexity, I would prefer to keep it simple unless there is something it is really missing.

@jeverley
Copy link
Contributor

jeverley commented Feb 7, 2025

@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. Also, I am not sure we should have virtual channels, as those can be added by the users on HA side, if they want to. That would add extra complexity to the quirks, making it harder for people to contribute to them.

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 intent being it would keep things easier to maintain than having per device implementations scattered accross multiple files.

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.

@abmantis
Copy link
Contributor Author

abmantis commented Feb 8, 2025

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 intent being it would keep things easier to maintain than having per device implementations scattered accross multiple files.

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.

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.
But I'll leave that for the maintainers to decide, as I am also ok with doing it all in one big PR if that is preferred.

@rhakbari
Copy link
Contributor

rhakbari commented Feb 15, 2025

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.

image

@abmantis
Copy link
Contributor Author

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.

That does not seem to be the same device as the one in this PR. Are the data points the same?

@rhakbari
Copy link
Contributor

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.

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.

@abmantis
Copy link
Contributor Author

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.

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)

@abmantis abmantis marked this pull request as ready for review February 23, 2025 16:38
@TheJulianJES TheJulianJES added the smash This PR is close to be merged soon label Feb 26, 2025
@abmantis abmantis marked this pull request as ready for review March 19, 2025 16:50
Copy link
Collaborator

@TheJulianJES TheJulianJES left a 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.

@abmantis abmantis requested a review from TheJulianJES March 25, 2025 15:54
@abmantis abmantis marked this pull request as draft March 25, 2025 15:56
@abmantis abmantis marked this pull request as ready for review March 25, 2025 15:59
Copy link
Collaborator

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines +214 to +215
.removes(LevelControl.cluster_id)
.removes(OnOff.cluster_id)
Copy link
Collaborator

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).

Copy link
Contributor Author

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?

Comment on lines +62 to +68
.tuya_sensor(
dp_id=134,
attribute_name="device_status",
type=t.int32s,
fallback_name="Device status",
translation_key="device_status",
)
Copy link
Collaborator

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..

Copy link
Contributor Author

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?

@TheJulianJES TheJulianJES merged commit 4c8875e into zigpy:dev Mar 25, 2025
9 checks passed
@cdce8p cdce8p mentioned this pull request Jun 10, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review This PR should be reviewed soon, as it generally looks good. smash This PR is close to be merged soon Tuya Request/PR regarding a Tuya device

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants