Add support for additional data point values in Tuya payloads #483

gmbnomis · 2021-12-22T20:02:13Z

Although most TuYa devices send only one data point value in a data
report/command, it is possible to include values for multiple data points in
such a payload.

Extend the definitions of those receiving commands to decode all values
present. Store them in the dpValues array. Additionally, remove the fn
field: It is just the high byte of the length of the data buffer and isn't
used by converters.

Change the definitions of the sending commands to use an array of DP values
as well.

NOTE:

This is a breaking change to the tuya library functions in zigbee-herdsman-converters.
This breaks all existing Tuya from Zigbee converters using dataResponse,
dataReport, or activeStatusReport. They need to be adapted to use the
first entry of dpValues

Closes #478

src/zcl/definition/cluster.ts

Koenkk · 2021-12-22T20:33:03Z

@hacker-cb could you review this?

hacker-cb · 2021-12-22T20:40:45Z

@gmbnomis where you found about multiply dp in one command?

There is no such information in official tuya document.

gmbnomis · 2021-12-22T21:53:02Z

@gmbnomis where you found about multiply dp in one command?

Please have a look into the corresponding issue #478. I have a water leakage sensor that sends such a payload.

You are right, the Zigbee part does not mention it explicitly, but if you look at the serial protocol it is possible to send multiple DP values back-to-back. ("Sending multiple commands are supported in obj type data points").

I reckon that in my case, these back-to-back data point values are just put into the Zcl "DP data"

hacker-cb · 2021-12-23T04:41:43Z

@gmbnomis seems you are right, I can accept version that MCU DPs data is transfered transparently via Zigbee.

I think right way is completely replace single-dp behaviour with multi-dp because may be other devices also use multi-dp format and currently we just ignore this.

Of cause, need to refactor converters in this case.
I think it will be better to use class-based dp everywhere.

After brief lookup, for incoming dp:

Just need to change fromZigbee.js and lidl.js.
...?

For outgoing dp:

Add sendMultiDataPoints() method to lib/tuya.js, based on sendDataPoint().
Call it from sendDataPoint() for backward-compatibility.
Completely replace sendDataPoint[Value|sendDataPointBool|sendDataPointEnum|...]() with class-based dp.
...?

So, @Koenkk how do you think?

gmbnomis · 2021-12-23T09:11:27Z

Of cause, need to refactor converters in this case. I think it will be better to use class-based dp everywhere.

@hacker-cb Makes sense. Essentially, we would convert these by changing msg.data.... into msg.data.dpValues[0]...?

After brief lookup, for incoming dp:

Just need to change fromZigbee.js and lidl.js.

There is also tuya_thermostat_weekly_schedule in legacy.js

...?

I would propose:

Get rid of the fnfield. According to the documentation, it is just the high byte of the length and it's used nowhere but in tuya_data_point_dump.
Adapt tuya_data_point_dump to be able to dump multiple data points. Adapt read_tuya_dump.py (I have already done this)
Adapt the documentation https://www.zigbee2mqtt.io/advanced/support-new-devices/02_support_new_tuya_devices.html

Koenkk · 2021-12-23T18:19:55Z

Sounds good to me, I think such change has quite a risk so once ready I will merge it after the next release on 1 January.

gmbnomis · 2021-12-23T21:09:44Z

Great!

@hacker-cb As you have looked into the outgoing part in more detail, should we split up the work? (I can continue to work on the incoming part, which will mostly happen in other repositories I suppose)

hacker-cb · 2021-12-24T05:17:47Z

@gmbnomis I think at least this things of the outgoing part should be merged together with incoming to avoid double refactoring of the zigbee-herdsman project:

Add sendMultiDataPoints() method to lib/tuya.js, based on sendDataPoint().

Call it from sendDataPoint() for backward-compatibility.

@Koenkk this PR should be merged very quickly to avoid situations like this.

Koenkk · 2021-12-24T09:45:35Z

Does this pr also require any changes to zigbee-herdman-converters?

gmbnomis · 2021-12-24T14:01:39Z

Does this pr also require any changes to zigbee-herdman-converters?

@Koenkk Yes, this PR totally breaks all "from Zigbee" Tuya converters. The necessary adaptations are at Koenkk/zigbee-herdsman-converters#3572.

As you said before, this change is quite risky (although I tried to be as conservative as possible), as I can't test any of those converters. So you might want to merge it after releasing.

Additional changes will be necessary for the documentation on how to add TuYa devices. (I plan to provide these as well, but haven't started working on those yet)

This is the input side. Then there is the output (sending) side of things. @hacker-cb I am not sure if I understand your statement on the output side above. In your view, should the changes to the sending methods become part of this PR, or should this PR be merged first to get the output changes on top as quickly as possible?

hacker-cb · 2021-12-24T14:47:21Z

@gmbnomis

I mean that all zigbee-herdsman cluster.ts modifications of the output parts should be together with input modifications in one PR.
zigbee-herdsman-converters sendDataPoint() should be modified to match new cluster and should be in one PR with inputs.

Koenkk · 2021-12-24T15:55:46Z

@gmbnomis

So you might want to merge it after releasing.

I will set a reminder for 2 January, will ping you to update the zigbee-herdsman-converters pr then.

gmbnomis · 2021-12-27T08:52:44Z

I have done the input and output side now, see also Koenkk/zigbee-herdsman-converters#3572.

For outgoing dp:

Add sendMultiDataPoints() method to lib/tuya.js, based on sendDataPoint().

done

Call it from sendDataPoint() for backward-compatibility.

done

Completely replace sendDataPoint[Value|sendDataPointBool|sendDataPointEnum|...]() with class-based dp.

I have not done this: Looking at the existing "to Zigbee" converters, the main use case will still be to send a single DP.
I think we should not make this main use case more complicated and keep the "convenience" functions to send a single DP value.

@hacker-cb Could you have a look? Additionally, if you have a TuYa device that accepts outgoing DP commands, could you test this with a real device? (I verified that the outgoing payloads are the same, but I can't verify with a real device)

hacker-cb · 2021-12-27T09:40:09Z

I have not done this: Looking at the existing "to Zigbee" converters, the main use case will still be to send a single DP.
I think we should not make this main use case more complicated and keep the "convenience" functions to send a single DP value.

I agree with you to keep convenience functions.
Outgoing part looks nice. For the incoming part - I left comment in review.

Additionally, if you have a TuYa device that accepts outgoing DP commands, could you test this with a real device? (I verified that the outgoing payloads are the same, but I can't verify with a real device)

I can't test on real Tuya devices because I'm not in office till end of the NY holidays. May be @Koenkk can ask somebody else...

Koenkk · 2021-12-27T17:13:53Z

@hacker-cb no rush, will merge this after new years eve.

hacker-cb · 2021-12-27T17:48:34Z

Also there is one more thing left to do:

I wrote comment in review of the Processing all dp data in incoming message with for/foreach loop.

So, @gmbnomis - what about that?

gmbnomis · 2021-12-28T12:38:33Z

@hacker-cb Sorry, I can't find a comment at Koenkk/zigbee-herdsman-converters#3572?

hacker-cb · 2021-12-28T13:00:20Z

@gmbnomis

gmbnomis · 2021-12-28T17:01:55Z

@hacker-cb Ah, that looks like a "Pending" comment that is only visible to you. You will need to submit the review to make it visible.

hacker-cb · 2021-12-28T17:10:32Z

@gmbnomis, sorry, submitted, try now :)

Koenkk · 2022-01-01T11:46:02Z

@gmbnomis I plan to merge everything tomorrow:

are you ready?
Could you link all prs I need to merge? (to make sure I don't miss any)

Although most TuYa devices send only one data point value in a data report/command, it is possible to include values for multiple data points in such a payload. Change the definitions of those receiving commands to decode all values present. Store them in the `dpValues` array. Additionally, remove the `fn` field: It is just the high byte of the length of the data buffer and isn't used by converters. **NOTE:** This breaks all existing Tuya converters using `dataResponse`, `dataReport`, or `activeStatusReport`. They need to be adapted to use the first entry of `dpValues`

Change the definitions of the sending commands to use an array of DP values. **NOTE:** This is a breaking change to the tuya library function in zigbee-herdsman-converters.

gmbnomis · 2022-01-01T17:37:00Z

are you ready?

Yes, all PRs are final from my point of view

Could you link all prs I need to merge? (to make sure I don't miss any)

Overall, there are three PRs:

(I will post the changes for the actual device needing this feature after they are merged)

Koenkk · 2022-01-02T09:43:48Z

@gmbnomis merged all, big thanks for working on this!

gmbnomis commented Dec 22, 2021

View reviewed changes

src/zcl/definition/cluster.ts Outdated Show resolved Hide resolved

gmbnomis force-pushed the tuya_multiple_dps branch from be8667d to 1c35f56 Compare December 23, 2021 21:33

gmbnomis mentioned this pull request Dec 24, 2021

Adapt TuYa converters to multiple data point change Koenkk/zigbee-herdsman-converters#3572

Merged

gmbnomis force-pushed the tuya_multiple_dps branch from 1c35f56 to c66ec8b Compare December 27, 2021 08:10

gmbnomis added 2 commits January 1, 2022 18:14

Add support for sending multiple data point values in Tuya payloads

cd133e4

Change the definitions of the sending commands to use an array of DP values. **NOTE:** This is a breaking change to the tuya library function in zigbee-herdsman-converters.

gmbnomis force-pushed the tuya_multiple_dps branch from c66ec8b to cd133e4 Compare January 1, 2022 17:15

Koenkk merged commit edc7187 into Koenkk:master Jan 2, 2022

rforro mentioned this pull request May 16, 2022

[BUG] Tuya data processing - ZCL frame multiple attributes zigpy/zha-device-handlers#1566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional data point values in Tuya payloads #483

Add support for additional data point values in Tuya payloads #483

gmbnomis commented Dec 22, 2021 •

edited

Koenkk commented Dec 22, 2021

hacker-cb commented Dec 22, 2021

gmbnomis commented Dec 22, 2021

hacker-cb commented Dec 23, 2021 •

edited

gmbnomis commented Dec 23, 2021

Koenkk commented Dec 23, 2021

gmbnomis commented Dec 23, 2021

hacker-cb commented Dec 24, 2021 •

edited

Koenkk commented Dec 24, 2021

gmbnomis commented Dec 24, 2021

hacker-cb commented Dec 24, 2021 •

edited

Koenkk commented Dec 24, 2021

gmbnomis commented Dec 27, 2021

hacker-cb commented Dec 27, 2021

Koenkk commented Dec 27, 2021

hacker-cb commented Dec 27, 2021

gmbnomis commented Dec 28, 2021

hacker-cb commented Dec 28, 2021

gmbnomis commented Dec 28, 2021

hacker-cb commented Dec 28, 2021

Koenkk commented Jan 1, 2022

gmbnomis commented Jan 1, 2022

Koenkk commented Jan 2, 2022

Add support for additional data point values in Tuya payloads #483

Add support for additional data point values in Tuya payloads #483

Conversation

gmbnomis commented Dec 22, 2021 • edited

Koenkk commented Dec 22, 2021

hacker-cb commented Dec 22, 2021

gmbnomis commented Dec 22, 2021

hacker-cb commented Dec 23, 2021 • edited

gmbnomis commented Dec 23, 2021

Koenkk commented Dec 23, 2021

gmbnomis commented Dec 23, 2021

hacker-cb commented Dec 24, 2021 • edited

Koenkk commented Dec 24, 2021

gmbnomis commented Dec 24, 2021

hacker-cb commented Dec 24, 2021 • edited

Koenkk commented Dec 24, 2021

gmbnomis commented Dec 27, 2021

hacker-cb commented Dec 27, 2021

Koenkk commented Dec 27, 2021

hacker-cb commented Dec 27, 2021

gmbnomis commented Dec 28, 2021

hacker-cb commented Dec 28, 2021

gmbnomis commented Dec 28, 2021

hacker-cb commented Dec 28, 2021

Koenkk commented Jan 1, 2022

gmbnomis commented Jan 1, 2022

Koenkk commented Jan 2, 2022

gmbnomis commented Dec 22, 2021 •

edited

hacker-cb commented Dec 23, 2021 •

edited

hacker-cb commented Dec 24, 2021 •

edited

hacker-cb commented Dec 24, 2021 •

edited