dresden-elektronik / deconz-rest-plugin

deCONZ REST-API plugin to control ZigBee devices
BSD 3-Clause "New" or "Revised" License
1.9k stars 503 forks source link

Fixes to several Tuya DDF profiles #7633

Closed retrography closed 6 months ago

retrography commented 7 months ago

@Smanar if you accept this, it may resolve some of the new device requests. It has been cumulating for a while.

Smanar commented 7 months ago

No problem for me. Just a comment you can remove"refresh.interval": 300 here

        {
          "name": "state/on",
          "parse": {"fn": "tuya", "dpid": 1, "eval": "Item.val = Attr.val;" },
          "write": {"fn": "tuya", "dpid": 1, "dt": "0x10", "eval": "Item.val == 1 ? 1 : 0;"},
          "read": {"fn": "tuya"},
          "refresh.interval": 300
        },
        {
          "name": "state/bri",
          "parse": {"fn": "tuya", "dpid": 2, "eval": "Item.val = (Attr.val / 1000.0) * 254.0;"},
          "write": {"fn": "tuya", "dpid": 2, "dt": "0x2b", "eval": "(Item.val / 254.0) * 1000.0;"},
          "read": {"fn": "none"},
          "refresh.interval": 300
        },

"refresh.interval" Enable a polling, not used on tuya cluster and "read": {"fn": "none"}, disable it.

retrography commented 7 months ago

@Smanar To be clear, does it need to become like this?

        {
          "name": "state/on",
          "parse": {"fn": "tuya", "dpid": 1, "eval": "Item.val = Attr.val;" },
          "write": {"fn": "tuya", "dpid": 1, "dt": "0x10", "eval": "Item.val == 1 ? 1 : 0;"},
          "read": {"fn": "tuya"}
        },
        {
          "name": "state/bri",
          "parse": {"fn": "tuya", "dpid": 2, "eval": "Item.val = (Attr.val / 1000.0) * 254.0;"},
          "write": {"fn": "tuya", "dpid": 2, "dt": "0x2b", "eval": "(Item.val / 254.0) * 1000.0;"},
          "read": {"fn": "none"}
        },
Smanar commented 7 months ago

Ha, sorry I just remember that https://github.com/dresden-elektronik/deconz-rest-plugin/pull/7585#issuecomment-1937391501

So better to use

        {
          "name": "state/on",
          "parse": {"fn": "tuya", "dpid": 1, "eval": "Item.val = Attr.val;" },
          "write": {"fn": "tuya", "dpid": 1, "dt": "0x10", "eval": "Item.val == 1 ? 1 : 0;"},
          "read": {"fn": "none"}
        },
        {
          "name": "state/bri",
          "parse": {"fn": "tuya", "dpid": 2, "eval": "Item.val = (Attr.val / 1000.0) * 254.0;"},
          "write": {"fn": "tuya", "dpid": 2, "dt": "0x2b", "eval": "(Item.val / 254.0) * 1000.0;"},
          "read": {"fn": "none"}
        },

Like that, no poll at all, all natively by the tuya cluster.

retrography commented 7 months ago

@Smanar Done!

retrography commented 7 months ago

@SwoopX No worries, I can put a bit of time and break it down into individual PRs if needed.

But to be clear, I have all these devices, and I have tested and they are all working with the committed DDFs. The only issue that may arise is if a setting applies to only to one set of manufacturers of a specific device, and not the others. In that case we will need to have multiple DDFs for a single Tuya device, with the distinctive factor being the manufacturer. Is that what you suggest that I do?

Also, which battery resource item are you referring to? The one added to the motion sensors? I have four different variants of this sensor manufactures over the course of approx. three years, and they all report correctly with this mod. But let me know what your suggestion is.

Smanar commented 7 months ago

I m agree, but for this one, it's just 1 DDF and adding some clones. And they are using a common file "button_maps.json" so making many PR may causes conflict.

retrography commented 7 months ago

Whatever you decide, gentlemen. Just let me know.

SwoopX commented 7 months ago

So, given that I haven't overlooked anything, I'd prefer the following (primarily aimed at detangling things):

  1. One PR for switch promotion to gold
  2. One PR for adding the 1 gang dimmer module
  3. One PR for adding the 1 gang remote incl. the button map
  4. One PR for the smart plug

As for the PR regarding the presence sensor battery change, I'm really hesitant, as there are so many different devices in there (>20?). I don't dare to say that your suggestion is universally applicable, as I do not recall any remarks here. Maybe you have a newer/different firmware version, where battery readings are differently. To move on, it might be better to have a seperate DDF tied to the current firmware version?

I'm mentioning that, because we have such a case definitively out there. A sunricher 8 button switch where the DDF would be almost 100% identical, except for the battery readings. Older versions report a max value of 100 whereas newer versions report a max value of 200.

@manup maybe you want to share your view on this?

retrography commented 7 months ago

Regarding the battery level PR, maybe I can bring in some ueful information:

  1. Note that before this PR there was no battery reporting, which is odd.
  2. I have at least two versions of this sensor. _TZ3040_bb6xaihh (Application version 72) and _TZ3000_msl6wxk9 (Application version 70). Only one of my bb6xaihh sensors report version 1.0.8 in Phoscon. The others don't report anything. I am not sure from which cluster that attribute is read. For both these versions the battery reporting code works. At the very least it works better than always reporting 0!
  3. It turns out I have one of the oldest versions of this sensor construction as well, although not called TS0202. It used to be called Konke at the time (Application version 40), and there was a lot of back and forth until it received support. Even that old version supported battery reporting. Support for battery reporting was added in #3347. It reports battery level without DDF. Note that battery reporting for Konke sensor and Tuya TS0202 was added simultaneously in cb985c28585b5efd5ffdd965f4ba2b57fa714da1. I am not well-versed in that pre-DDF legacy code, but maybe it gives you a clue how this battery value has to be reported.
retrography commented 6 months ago

@SwoopX @Smanar @manup Let's continue here: #7716

manup commented 6 months ago

So, given that I haven't overlooked anything, I'd prefer the following (primarily aimed at detangling things):

1. One PR for switch promotion to gold

2. One PR for adding the 1 gang dimmer module

3. One PR for adding the 1 gang remote incl. the button map

4. One PR for the smart plug

As for the PR regarding the presence sensor battery change, I'm really hesitant, as there are so many different devices in there (>20?). I don't dare to say that your suggestion is universally applicable, as I do not recall any remarks here. Maybe you have a newer/different firmware version, where battery readings are differently. To move on, it might be better to have a seperate DDF tied to the current firmware version?

I'm mentioning that, because we have such a case definitively out there. A sunricher 8 button switch where the DDF would be almost 100% identical, except for the battery readings. Older versions report a max value of 100 whereas newer versions report a max value of 200.

@manup maybe you want to share your view on this?

Indeed untangling into multiply PRs is good to better keep track. Also the changelog gets more clear as it's based on the PR headlines :)

The battery differences between firmware versions and devices introduce more complexity than I like. Perhaps we can add a small dynamic thing to figure this out without having to add multiple DDFs and check versions.

Something like a new non public item which tracks the maximum value ever reported. Pseudo code for state/battery and config/battery "parse" handler:

var m = R.item('config/max_battery_reported');
if (m.val < Attr.val) m.val = Attr.val;

if (m.val > 100) Item.val = Attr.val / 2;
else Item.val = Attr.val;

Here the config/max_battery_reported (default: 0) just tracks the maximum reported value, which is also stored in database if changed. And the actual battery value will then be adjusted accordingly.

This requires that the battery value for a 200 firmware is reporting at least once while charge is more than 50 % but that's not unrealistic.