Add support for Tuya DPs mapped to multiple cluster attributes

[abmantis]

Proposed change

Some Tuya devices expose multiple atributes (like current+voltage+power) in a single DP. This PR adds support for easilly mapping those, including Builder support.

Additional information

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

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.93%. Comparing base (730f0a8) to head (965a3ac).

Files with missing lines	Patch %	Lines
zhaquirks/tuya/__init__.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3643      +/-   ##
==========================================
+ Coverage   90.91%   90.93%   +0.01%     
==========================================
  Files         325      325              
  Lines       10567    10575       +8     
==========================================
+ Hits         9607     9616       +9     
+ Misses        960      959       -1     

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

[TheJulianJES]

Just went through the existing DP mapping code and noticed this was merged at some point: #1989
The example at the bottom is interesting. It seems like we already support mapping one datapoint to multiple attributes if they're on the same cluster.

I generally prefer this implementation though, as it's more explicit/clear than the older one. In a follow-up PR, we can probably convert old quirks and remove the old parsing code, unless we find that a bunch of custom quirks use it..

---

Overall, I think this is fine. We should see if we can optimize how often _dp_to_attributes is generated though. It might be enough to just do that once in the cluster __init__.

[TheJulianJES]

Hmm, is there any benefit to also having this method instead of just calling tuya_dp twice for the same endpoint?

If so, we may want to add a test for this, though I'd need to check if tuya_dp is tested explicitly (or just by one of the other Tuya methods).

[abmantis]

Part of the reason was that calling tuya_dp multiple times would allow passing a different dp_handler, which will not work properly. It would still handle multiple DPToAttributeMapping, but just overwrite the dp_handler.
Having a separate method prevents that and makes it clearer to who uses it. I can drop it if you think it would be better that way.

[abmantis]

Oh, I just noticed that I missed the tests when opening this PR from the original one: https://github.com/zigpy/zha-device-handlers/pull/3644/files#diff-d01eb6338c5bfa4635fb433e6a8fc5b9cf3de58e784218f8930c5ccaedd927f5
🤦‍♂️

Copied them to this PR now.

[TheJulianJES]

Ok, one more thing regarding this. Can we either block subsequent calls to tuya_dp if the dp_id is already in tuya_dp_to_attribute or just have it update the existing list of DPToAttributeMapping per id?
For the latter, both tuya_dp_multi and subsequent calls to tuya_dp would work as expected then.

[abmantis]

Added that in a new PR #3827 since it required changing 2 quirks that were invalid.

[TheJulianJES]

Well, this code also never expected a tuple, so the "old multi DP code".
Needs updating in a future PR if we keep that code around.

[abmantis]

Personally I would prefer to drop the tuple from DPToAttributeMapping. Even if a few custom quirks break, I think it is worth it to have the code a bit clean. Except if there are a lot of custom quirks using it.

[TheJulianJES]

Ok, we probably should remove it soon then.
I guess we could also convert the old tuple-style to the new _dp_to_attributes format with the list in __init__ if we really wanted to. Then, we wouldn't need to care about handling it again in _dp_2_attr_update or for the "mark attributes as valid". I think that could be nice. Otherwise, we can just remove it soon.

[abmantis]

I am fine with both removing or converting it to _dp_to_attributes. Personally, I prefer to remove it just to keep the code simpler and easier to maintain.

But let me know which approach you prefer and I'll do that. Is it fine if we leave that to a follow-up PR? This one is not breaking it anyway.

[TheJulianJES]

yeah, that's fine.

[jeverley]

I was under the impression that this is already supported in v1 quirks, see the DPToAttributeMapping class:

@dataclasses.dataclass
class DPToAttributeMapping:
    """Container for datapoint to cluster attribute update mapping."""

    ep_attribute: str
    attribute_name: Union[str, tuple]
    converter: Optional[
        Callable[
            [
                Any,
            ],
            Any,
        ]
    ] = None
    endpoint_id: Optional[int] = None

attribute_name can be provided as a tuple containing multiple attribute names, this is then used in conjunction with a converter that returns a value for each attribute.

Below is an example that I used previously that calls a function in the TuyaPowerPhase class to extract voltage, current and power:

    dp_to_attribute: Dict[int, DPToAttributeMapping] = {
        TUYA_DP_POWER_PHASE: DPToAttributeMapping(
            TuyaElectricalMeasurement.ep_attribute,
            (
                "rms_voltage",
                "rms_current",
                "active_power",
            ),
            converter=lambda x: TuyaPowerPhase.variant_3(x),
        ),
    }

class TuyaPowerPhase:
    """Extract values from a Tuya power phase datapoint."""

    @staticmethod
    def variant_3(value) -> Tuple[t.uint_t, t.uint_t, int]:
        voltage = (value[0] << 8) | value[1]
        current = (value[2] << 16) | (value[3] << 8) | value[4]
        power = (value[5] << 16) | (value[6] << 8) | value[7]
        return voltage, current, power * 10

This would look like this in a v2 quirk:

    .tuya_dp(
        dp_id=6,
        ep_attribute=TuyaElectricalMeasurement.ep_attribute,
        attribute_name=(
            TuyaElectricalMeasurement.AttributeDefs.rms_voltage.name,
            TuyaElectricalMeasurement.AttributeDefs.rms_current.name,
            TuyaElectricalMeasurement.AttributeDefs.active_power.name,
        ),
        converter=lambda x: TuyaPowerPhase.variant_3(x),
    )

Before seeing this PR I as about to raise my own (linked #3811) to amend the v2 tuya builder methods tuya_dp and tuya_dp_attribute to support str | tuple for attribute_name to match the underlying DPToAttributeMapping class (the functions currently only use str as the type).

Are we overcomplicating things here?

[abmantis]

@jeverley TheJulianJES pointed that out too here: #3643 (review)

I have missed that when I opened this. Still, this PR makes it a bit more explicit, and allows having a single DP mapped to different clusters, which is not possible with the old DPToAttributeMapping tuple approach.

Also, not all code is currently properly handling the tuple. See #3643 (comment)

The plan is to check if we can drop the old tuple after this is merge (and migrate any quirks using it).