Skip to content
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

Add Tuya cover _TZE200_eevqq1uv #3114

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Conversation

matt-sullivan
Copy link

@matt-sullivan matt-sullivan commented Apr 17, 2024

Similar to other Tuya window cover devices, but reports multiple tuya data points in set_data_response, so this implementation is based on the newer TuyaMCUCluster rather than TuyaManufCluster.

Sold as Zemismart ZM25R3 roller blind motor.

Proposed change

Additional information

Fixes #3068

Once merged it might make sense to change other existing tuya covers to use the new cluster implementations added in this PR. I think they could be replaced/consolidated with very little change, but I'm not sure how to how to test that sort of refactoring.

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 Apr 17, 2024

Codecov Report

Attention: Patch coverage is 96.12403% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 88.29%. Comparing base (bfbfb48) to head (44c5d9b).

Files Patch % Lines
zhaquirks/tuya/mcu/__init__.py 96.77% 3 Missing ⚠️
zhaquirks/tuya/__init__.py 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3114      +/-   ##
==========================================
+ Coverage   88.14%   88.29%   +0.14%     
==========================================
  Files         299      299              
  Lines        9389     9498     +109     
==========================================
+ Hits         8276     8386     +110     
+ Misses       1113     1112       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

attributes = WindowCovering.attributes.copy()
# cover direction and lift percent control attributes are not very useful to HA, but needed to
# allow TuyaMCUCluster to map to the data point.
attributes.update({ATTR_COVER_DIRECTION: (ATTR_COVER_DIRECTION_NAME, t.enum8)})
Copy link
Author

Choose a reason for hiding this comment

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

I'm surprised I need a new attribute to track motor status (i.e. opening, closing, stopped) but I couldn't find anything in the standard zigbee WindowCovering.

@matt-sullivan matt-sullivan marked this pull request as ready for review April 17, 2024 22:23
@TheJulianJES TheJulianJES added Tuya Request/PR regarding a Tuya device has tests PR has tests labels Apr 21, 2024
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.

Nice job, especially on the test coverage!
I'll have a closer look at this soon.

Comment on lines 573 to 575
def __init__(self, *args, **kwargs):
"""Init."""
super().__init__(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Although it's a bit awkward if the DPToAttributeMapping converter needs to refer to self. I worked around it by instead using data_point_handlers, which finds a function by name and will call an instance method.

@matt-sullivan
Copy link
Author

Nice job, especially on the test coverage! I'll have a closer look at this soon.

Thanks @TheJulianJES. Since raising the PR I've figured out some more tuya data points, and I've started changing it to use the quirks v2 registration. If you'd prefer to wait a few days it might be easier to review it all at once?

@TheJulianJES
Copy link
Collaborator

Yep, feel free to change the PR.

Btw., if you're exposing custom entities to ZHA via quirks v2, the translation keys are added with the "quirks bump PR" in HA. Just let me know what translation keys I should add to the file when bumping quirks then. (only applicable if you're using custom entities)

@stast1
Copy link

stast1 commented Apr 24, 2024

I tested with _TZE200_68nvbio9 and it works great! I propose to add _TZE200_68nvbio9 here

Similar to other Tuya window cover devices, but reports multiple tuya data points in set_data_response, so this implementation is based on the newer TuyaMCUCluster rather than TuyaManufCluster.

Sold as Zemismart ZM25R3 roller blind motor.
TuyaManufCluster has a TUYA_MCU_COMMAND listener that is passed a command to send. Many existing calls to TUYA_MCU_COMMAND were calling the TuyaMCUCluster variant which takes a TuyaClusterData structure. I've renamed those calls to TUYA_MCU_SET_CLUSTER_DATA so sit alongside TUYA_MCU_SET_DATAPOINTS and distinguish the data type they accept.
@matt-sullivan
Copy link
Author

@TheJulianJES , this PR is now ready for review again.

It turned out to be much bigger than I expected, including some refactoring and a new base class. I'm happy for feedback to adjust the design, but thought it best to publish a proof of concept to show you.

The new base class (TuyaCommandCluster) is used to provide generic processing of zigbee commands into tuya set_data commands, without needing to define cluster attributes for tuya data points that don't have sensible values to show. It does this similar to how TuyaMCUCluster maps cluster attributes to tuya set_data commands with dp_to_attribute.

entity_type=EntityType.STANDARD,
translation_key="small_step_open",
).command_button(
WINDOW_COVER_COMMAND_SMALL_STEP_NAME,
Copy link
Author

Choose a reason for hiding this comment

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

I think these command definitions are correct, but they currently cause an error 'Platform zha does not generate unique IDs'

I think that's because the zha component uses the command id to generate an unique id, but does not expect multiple instances of the same command or take the command parameters into account. I've created a proof of concept change to zha matt-sullivan/home-assistant-core@e7bdec2 that works by adding a simple index to each unique id.

I'm not sure if that's a good way to do it. Maybe the quirk should specify an id, or zha should include the translation_key in the id?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh, good that you noticed this!
There may be some bigger ZHA refactor changes coming soon. Hoping I don't forget about this, so this can be addressed soon-ish.

entity_type=EntityType.STANDARD,
translation_key="small_step_open",
).command_button(
WINDOW_COVER_COMMAND_SMALL_STEP_NAME,
Copy link
Author

Choose a reason for hiding this comment

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

These command definitions create HA entities with ids like button.[device name]none[int]. I.e. there doesn't seem to be any sensible naming applied to the entity. is that expected behaviour, or something wrong with these definitions?

It's not really a problem, but feels like it could be better. Could the zha component use the translation key, command name, or another parameter to set the entity id?

@matt-sullivan
Copy link
Author

@TheJulianJES, I created a zha commit with the translation keys this needs: matt-sullivan/home-assistant-core@1dff881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has tests PR has tests Tuya Request/PR regarding a Tuya device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Device Support Request] Tuya roller blind _TZE200_eevqq1uv TS0601
3 participants