RFC: Cleaner Quirk V2 Building for Tuya Devices

[prairiesnpr]

Proposed change

This extends QuirkBuilder to add add_tuya_dp this allows DPs and converters to be added in the following manner.

.tuya_dp(
        3,
        TuyaSoilMoisture.ep_attribute,
        "measured_value",
        converter=lambda x: x * 100,
    )

Or if the if it's known attribute, such as battery, temperature, or soil_moisture.

.tuya_temperature(dp_id=5, scale=10)

I've implemented replacement V2 Quirks for the existing soil quirks and tested with a _TZE284_aao3yzhs.

Additional information

Full Quirk Example:
Orginal V2: #3411
With TuyaQuirkBuilder: https://gist.github.com/prairiesnpr/e32e10b86a0736f5ff09693f6006ab3b

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.83%. Comparing base (a29ad2f) to head (576b8e6).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3417      +/-   ##
==========================================
+ Coverage   88.71%   88.83%   +0.11%     
==========================================
  Files         306      307       +1     
  Lines        9820     9923     +103     
==========================================
+ Hits         8712     8815     +103     
  Misses       1108     1108              

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

[TheJulianJES]

Nice work!

Currently, we have a bit of an issue with duplicating parts of a v2 quirk. There's no proper way to share them, so a downside with this approach is that there'll be somewhat of a duplication for basic datapoints like temperature and so on.
Personally, I'd probably keep the Tuya datapoint mappings in the temperature/soil clusters and rather only define Tuya datapoints in a v2 quirk (with this approach) for specific/custom entities that are unlikely to be common across devices.

That would allow us to easily re-use common clusters like temperature/soil (in v1 and v2 quirks), but adds the flexibility to just tack on custom v2 entities with Tuya datapoints in a v2 quirk if needed.

In the long term, we should still take a look at how we can better share parts of a v2 quirk across different quirks, but I think common datapoint mappings just make more sense to have in a cluster compared to a in a device/quirk. What do you think?
EDIT: See my newer comment.

[dmulcahey]

Could we add methods to this for the known datapoints / clusters that are shared? Either by device type (I think they have required dp’s for each device type) or by functionality? Maybe something like .temperature, .onoff, .fan etc or if device type specific something like .smart_switch, .relay, .plug, etc? And have that pull in the dp’s (or clusters) depending on which way we decide to go?

[TheJulianJES]

Ah, regarding my previous comment. I somewhat forgot that the Tuya datapoints are obviously defined on the Tuya manufacturer cluster and not on the virtual "temperature cluster". I had in my mind that you just do .replaces(TuyaTempCluster) (and have the datapoints defined in that cluster), but that's obviously not the case.
So, you can kind of disregard my previous comment (except the duplication part).

Some methods like .tuya_temperature (with optional multiplication and optional other datapoint kwargs) in the TuyaQuirkBuilder would make sense. It could add a virtual ZCL temperature cluster and create the datapoint mapping.
I think I really like that approach. Should be easy to implement as well with the current TuyaQuirkBuilder.

This would also cause a lot less duplication across devices, as it's only like a single line (maybe with a differing datapoint or multiplication value). I really like this idea 😄

[prairiesnpr]

I think I prefer methods by functionality but still pass the DP. We could even have them go ahead and add the appropriate entity, a switch if it's onoff etc.

I'd like to be a bit careful that we don't abstract too much. A typical user building a quirk will go pull a list of DPs using something like https://www.zigbee2mqtt.io/advanced/support-new-devices/03_find_tuya_data_points.html. I'd like to keep it straightforward so that they can quickly scan a quirk and see that the quirk matches their device and that they can just add the manufacturer and model. I think having the list of DPs in the quirk will help with that even at the cost of some duplication.

[dmulcahey]

I think I prefer methods by functionality but still pass the DP. We could even have them go ahead and add the appropriate entity, a switch if it's onoff etc.

I'd like to be a bit careful that we don't abstract too much. A typical user building a quirk will go pull a list of DPs using something like https://www.zigbee2mqtt.io/advanced/support-new-devices/03_find_tuya_data_points.html. I'd like to keep it straightforward so that they can quickly scan a quirk and see that the quirk matches their device and that they can just add the manufacturer and model. I think having the list of DPs in the quirk will help with that even at the cost of some duplication.

Couldn’t we have our own version of this that also determines if the quirk exists and maybe even creates the PR if it doesn’t 🤔

[TheJulianJES]

I'm not set on this yet, but would it make sense to append a tuya_ prefix to all Tuya related methods?
Just so there aren't any conflicts down the road and it's clear this is for Tuya datapoints?

[prairiesnpr]

I'm leaning towards adding the tuya prefix. I'm working on adding support for adding attributes, switches, and enums now, and if we add the prefix, then you can still call the original methods if needed. I think it adds clarity and keeps functionality that we may need in the future if we run into something that doesn't fit the normal approach.

[prairiesnpr]

b83c29f adds methods corresponding to switch, sensor, binary_sensor, enum, and number from QuirkBuilder. This allows for a quirks such as this #3411 to be simplified to https://gist.github.com/prairiesnpr/e32e10b86a0736f5ff09693f6006ab3b

[prairiesnpr]

Will need a zigpy release for tests to pass, zigpy/zigpy@0707ff3 added entity_type for switches.

[TheJulianJES]

Also, would it make sense to move everything Tuya quirk builder related (including the small local clusters) into something like tuya/builder.py, tuya/mcu/builder.py, or tuya/builder/__init__.py?

[prairiesnpr]

Do we have any other Tuya methods we would want added to this initial PR?

[TheJulianJES]

Not sure we want to do this, but I think we could replace all default arguments (passed down to the normal .switch() method) with *args, **kwargs. Then, just pass those in.
It should contain all arguments that we did not explicitly define in our method definition.

This means we could introduce additional parameters in zigpy, without having to modify this code.

[prairiesnpr]

I have no strong preference on it, I had started with *args and **kwargs, but looking through the repo it looked like that wasn't the typical pattern, so I went the other way. Happy to change it, if that's the consensus.