Add initial support for ZHADoorLock sensor. #4540

Smanar · 2021-03-08T16:44:30Z

This code create ZHAdoorlock sensor for new doorlock, older one are not impacted and still use "light" type.
The JSON look like this one

{
  "config": {
    "battery": 100,
    "lock": false,
    "on": true,
    "reachable": true
  },
  "ep": 11,
  "etag": "a43862f76b7fa48b0fbb9107df123b0e",
  "lastseen": "2021-03-06T22:25Z",
  "manufacturername": "Onesti Products AS",
  "modelid": "easyCodeTouch_v1",
  "name": "easyCodeTouch_v1",
  "state": {
    "lastupdated": "2021-03-06T21:25:45.624",
    "lockstate": "unlocked"
  },
  "swversion": "20201211",
  "type": "ZHADoorLock",
  "uniqueid": "xx:xx:xx:xx:xx:xx:xx:xx-xx-0101"
}

The code add too some attribute and fonction to the GUI for doorlock cluster 0x0101

It add support for thoses devices.

Yale YRD226 TSDB
Yale YRD226/246 TSDB
Yale YRD256L TSDB SL
Yale YRD220/240 TSDB
Yale YRD256 TSDB
Kwikset 914 ZigBee smart lock
Datek ID Lock 150

Not all are tested, have some issue for the EasyAccess EasyCodeTouch #4253

To unlock/lock the device, use config/lock.
State/lockstate give the device state (locked/unlocked/undefined/not fully locked)

Lasted issue used #3750

Update

Smanar · 2021-03-08T16:51:31Z

To test the code, you need to use direclty the API, as third app don't use this sensor yet #3750 (comment)

ebaauw · 2021-03-08T17:38:49Z

Nice, thanks @Smanar !

Some remarks:

I think the capitalisation should be ZHADoorLock
I would rather see state.locked with values true and false instead of state.lockstate with a string value;
Not sure if we need the whitelist in addSensorNode(). I think it's OK to expose the door locks already supported as /lights also as ZHADoorLock /sensors. That would enable a smooth migration of API clients to the /sensors resources. Make sure to blacklist the lumi.vibration.aq1 (and maybe some other) sensor, that (ab)uses a Door Lock cluster;
For consistency we would need a CLIPDoorLock as well.

gabriellanata · 2021-03-08T17:42:03Z

Does this work with YRD256-NR-619 from https://smile.amazon.com/dp/B077YRHFHQ/ref=cm_sw_r_cp_api_glt_fabc_EKH4YHMMTZA847B9S1EN?_encoding=UTF8&psc=1 ?

ebaauw · 2021-03-08T17:42:46Z

Sorry, I though it was requesting me to review, but apparently this was only a suggestion. Not sure how I can mark this as "reviewed".

Smanar · 2021-03-08T18:15:12Z

* I think the capitalisation should be `ZHADoorLock`

Yes ( ZHALightLevel)

* I would rather see `state.locked` with values `true` and `false` instead of `state.lockstate` with a string value;

For that it will be config/lock, the state/lockstate can have more values locked/unlocked/undefined/not fully locked

* Not sure if we need the whitelist in `addSensorNode()`.  I think it's OK to expose the door locks already supported as `/lights` also as ZHADoorLock `/sensors`.  That would enable a smooth migration of API clients to the `/sensors` resources.

Yep, the whitelist is exactly for that

Make sure to blacklist the lumi.vibration.aq1 (and maybe some other) sensor, that (ab)uses a Door Lock cluster;

I m using this kind of check

            // For the moment reserved to doorlock device
            if ((*i == DOOR_LOCK_CLUSTER_ID) && (sensor->type() != QLatin1String("ZHADoorLock")))
            {
                break;
            }

So the code is only used for this kind of sensor, I m making this check at line 3286 binding.cpp and line 9495 in de_web_plugin.cpp, not sure it will be enought ...

* For consistency we would need a `CLIPDoorLock` as well.

Hu ? here I have not understand what i need to do ?

@gabriellanata if you give me the model id visible in deconz, I can add it, all yale are working for the moment, realy standard zigbee use.

ebaauw · 2021-03-08T18:19:04Z

For that it will be config/lock, the state/lockstate can have more values locked/unlocked/undefined/not fully locked

That's a good reason!

gabriellanata · 2021-03-08T23:06:43Z

@Smanar Thanks! Unsure which one is the model id though:

Ah I think I found where it should be but it's blank, even after asking it to read the configuration :/ I'll try a few things

Got it! Had to completely remove it and readd it:

gabriellanata · 2021-03-09T01:20:59Z

Created a PR to your fork to maybe save you some time. Thanks again! Smanar#22

Add Yale YRD256 model ID

Smanar · 2021-03-09T17:41:20Z

Nice perfect ^^, thx.

So the code is working too for this device ?

gabriellanata · 2021-03-09T18:52:55Z

rest_sensors.cpp

+                                //if (item->lastChanged() == item->lastSet())
+                                //{


Is this expected to be commented out?

IDK ^^.
I have commented it during my test, but I don't see the utility of this code. If the state don't change, you will not have notification, and the state don't change every hour, so not sure it s usefull ....

I'd suggest to remove commented out/dead code, otherwise it will sit there for ages ;)

gabriellanata · 2021-03-09T18:54:12Z

Nice perfect ^^, thx.

So the code is working too for this device ?

I haven't tried it honestly. It should be the same as the others though. I verified the state values and commands for lock/unlock match in code.

SwoopX

Some small comments

SwoopX · 2021-03-09T13:21:02Z

rest_sensors.cpp

@@ -789,6 +789,42 @@ int DeRestPluginPrivate::changeSensorConfig(const ApiRequest &req, ApiResponse &
                else if (rid.suffix == RConfigTempThreshold || rid.suffix == RConfigHumiThreshold)
                {
                }
+                else if (rid.suffix == RConfigLock)
+                {
+                    bool val = map[pi.key()].toBool();


Could you please add a type check here, as we have it further down below in that function (e.g. for thermostats)? Please also don't forget about the error message returned in case it's not a bool value ;)

SwoopX · 2021-03-09T13:22:49Z

rest_sensors.cpp

+                                rspItemState[QString("/sensors/%1/config/lock").arg(id)] = map["lock"];
+                                rspItem["success"] = rspItemState;
+                                //if (item->lastChanged() == item->lastSet())
+                                //{
+                                    updated = true;
+                                //}


I guess we only require the updated part here ;)

Good question why was for the code I commented ?
I don't see the utility of it ...

Do I need to use it or remove it ?

SwoopX · 2021-03-09T13:24:17Z

rest_sensors.cpp

+                            rsp.list.append(errorToMap(ERR_INVALID_VALUE, QString("/sensors/%1/config/lock").arg(id), QString("Command error, %1, for parameter, lock").arg(map[pi.key()].toString())));
+                            rsp.httpStatus = HttpStatusBadRequest;
+                            return REQ_READY_SEND;


I guess we should have the error here homogeneously? So just the part which contains ERR_ACTION_ERROR as further down below.

SwoopX · 2021-03-09T13:25:16Z

de_web_plugin.cpp

@@ -9432,6 +9492,57 @@ void DeRestPluginPrivate::updateSensorNode(const deCONZ::NodeEvent &event)
                                    }
                                }
                            }
+                            else if (i->type() == QLatin1String("ZHADoorLock")) // Door lock


General question to all: do we want to have/leave it here or have an extra file for it?

I need one.
Because of notifications, need too much code, it will be in next version, WIP

+1 splitting up door lock in separate file sounds clean like it is done for window covering.
But I'm fine when this is done in a extra PR.

SwoopX · 2021-03-09T13:26:02Z

de_web_plugin.cpp

+            else
+            {
+                checkSensorNodeReachable(sensor);
+                checkIasEnrollmentStatus(sensor);


Just wandering if the IAS check is required here, persumably not?

No, at all, removed

Update

manup

Added my usual code style requests, can you please have a look. :)

In future I hope to automate these checks with clang-tidy to get a more consistent style across the code base.

bindings.cpp

de_web_plugin.cpp

manup · 2021-03-25T11:56:10Z

de_web_plugin.cpp

@@ -9432,6 +9492,57 @@ void DeRestPluginPrivate::updateSensorNode(const deCONZ::NodeEvent &event)
                                    }
                                }
                            }
+                            else if (i->type() == QLatin1String("ZHADoorLock")) // Door lock


+1 splitting up door lock in separate file sounds clean like it is done for window covering.
But I'm fine when this is done in a extra PR.

de_web_plugin.cpp

manup · 2021-03-25T12:10:12Z

rest_sensors.cpp

+                                //if (item->lastChanged() == item->lastSet())
+                                //{


I'd suggest to remove commented out/dead code, otherwise it will sit there for ages ;)

Smanar · 2021-03-25T15:29:46Z

Ok so I m not able to resolve all, but all is resolved.

Just 2 comments.

Someone can explain me the utility of this code (visible in the code)

                            if (item->lastChanged() == item->lastSet())
                            {
                                updated = true;
                            }

On my side worked better just with
updated = true;

splitting up door lock in separate file is in WIP https://github.com/Smanar/deconz-rest-plugin/blob/doorlock_v2/doorlock.cpp

manup · 2021-03-27T09:39:00Z

Someone can explain me the utility of this code (visible in the code)
                            if (item->lastChanged() == item->lastSet())
                            {
                                updated = true;
                            }
On my side worked better just with
updated = true;

This is used to set updated only if the value was indeed be updated and is different from the previous set value. It depends on use case and code which follows. For example for button events they should trigger an event even if the value hasn't been changed but the timestamp since the same button was pressed multiple times.

In short, we use if (item->lastChanged() == item->lastSet()) only if events should be triggered on actual value change, not only on setting a value.

manup

Thanks, looking better now. I found a missing error handling can you please check.

rest_sensors.cpp

Smanar added 23 commits January 16, 2021 17:25

First test

3089d2a

Update bindings.cpp

c93b533

Update rest_sensors.cpp

039d114

Update de_web_plugin.cpp

dc1f53c

Update de_web_plugin.cpp

8df2034

Add new device

5890ca7

New test

60736f4

Add new device

4041dad

Update de_web_plugin.cpp

2f1cadc

Update bindings.cpp

7c51b92

Merge pull request #12 from dresden-elektronik/master

e687221

Update

Merge pull request #15 from dresden-elektronik/master

68cbbb8

Update

Merge pull request #17 from dresden-elektronik/master

5d7e81f

Update

Merge branch 'doorlock' into to_delete4

0b9be3d

Merge pull request #18 from Smanar/to_delete4

a5abc96

Update

test

57a6274

Update de_web_plugin.cpp

d9b484a

Merge pull request #19 from dresden-elektronik/master

51586b4

Update

corrective

6debedb

Merge branch 'doorlock' into tmpbranch7

c0d72c2

Merge pull request #20 from Smanar/tmpbranch7

b783645

Update

Code clean

920f81c

Update bindings.cpp

0a2c515

This was referenced Mar 8, 2021

Yale YRD226 ZigBee keypad door lock #3750

Closed

ID-Lock #3827

Closed

Kwikset 914 ZigBee smart lock #3152

Closed

ebaauw requested review from ebaauw and removed request for ebaauw March 8, 2021 17:39

gabriellanata added 2 commits March 8, 2021 17:18

Add Yale YRD256

229df06

Update de_web_plugin.cpp

9aa7988

Merge pull request #22 from gabriellanata/patch-1

523db08

Add Yale YRD256 model ID

Smanar changed the title ~~Add initial support for ZHADoorlock sensor.~~ Add initial support for ZHADoorLock sensor. Mar 9, 2021

gabriellanata reviewed Mar 9, 2021

View reviewed changes

Wrong attribute in xml

f52ea8a

SwoopX reviewed Mar 13, 2021

View reviewed changes

Smanar added 5 commits March 17, 2021 17:44

Merge pull request #25 from dresden-elektronik/master

657f71f

Update

Update

743826f

Merge branch 'doorlock' into todelete44

4bbaf09

Merge pull request #27 from Smanar/todelete44

d812fcc

Update

Remove unused debug

e396aab

manup reviewed Mar 25, 2021

View reviewed changes

modifcations requested

6816f41

manup reviewed Mar 27, 2021

View reviewed changes

rest_sensors.cpp Outdated Show resolved Hide resolved

rest_sensors.cpp Outdated Show resolved Hide resolved

Requested change

97a3ca4

manup added this to the v2.11.0-beta milestone Mar 27, 2021

manup merged commit 9cbbae5 into dresden-elektronik:master Mar 27, 2021

Kane610 mentioned this pull request Mar 28, 2021

Add ZHADoorLock support Kane610/deconz#98

Merged

Smanar deleted the doorlock branch June 19, 2021 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for ZHADoorLock sensor. #4540

Add initial support for ZHADoorLock sensor. #4540

Smanar commented Mar 8, 2021 •

edited

Loading

Smanar commented Mar 8, 2021

ebaauw commented Mar 8, 2021

gabriellanata commented Mar 8, 2021

ebaauw commented Mar 8, 2021

Smanar commented Mar 8, 2021

ebaauw commented Mar 8, 2021

gabriellanata commented Mar 8, 2021 •

edited

Loading

gabriellanata commented Mar 9, 2021 •

edited

Loading

Smanar commented Mar 9, 2021

gabriellanata Mar 9, 2021

Smanar Mar 9, 2021 •

edited

Loading

manup Mar 25, 2021

gabriellanata commented Mar 9, 2021

SwoopX left a comment

SwoopX Mar 9, 2021

Smanar Mar 19, 2021

SwoopX Mar 9, 2021

Smanar Mar 19, 2021

SwoopX Mar 9, 2021

Smanar Mar 19, 2021

SwoopX Mar 9, 2021

Smanar Mar 23, 2021

manup Mar 25, 2021

SwoopX Mar 9, 2021

Smanar Mar 19, 2021

manup left a comment

manup Mar 25, 2021

manup Mar 25, 2021

Smanar commented Mar 25, 2021

manup commented Mar 27, 2021

manup left a comment

Add initial support for ZHADoorLock sensor. #4540

Add initial support for ZHADoorLock sensor. #4540

Conversation

Smanar commented Mar 8, 2021 • edited Loading

Smanar commented Mar 8, 2021

ebaauw commented Mar 8, 2021

gabriellanata commented Mar 8, 2021

ebaauw commented Mar 8, 2021

Smanar commented Mar 8, 2021

ebaauw commented Mar 8, 2021

gabriellanata commented Mar 8, 2021 • edited Loading

gabriellanata commented Mar 9, 2021 • edited Loading

Smanar commented Mar 9, 2021

Choose a reason for hiding this comment

Smanar Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriellanata commented Mar 9, 2021

SwoopX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smanar commented Mar 25, 2021

manup commented Mar 27, 2021

manup left a comment

Choose a reason for hiding this comment

Smanar commented Mar 8, 2021 •

edited

Loading

gabriellanata commented Mar 8, 2021 •

edited

Loading

gabriellanata commented Mar 9, 2021 •

edited

Loading

Smanar Mar 9, 2021 •

edited

Loading