Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-23: Feature door lock get all pin codes #23

Closed

Conversation

nkljajic
Copy link
Collaborator

@nkljajic nkljajic commented Nov 29, 2023

Number of possible PIN codes can be up to 255 and probing all of them can last for up to 5 minutes.

Change

  • Defer probing all possible PIN codes from join/interview phase for CC user code v1, and create dedicated GetAllPINCodes command for DoorLock Cluster, similar how configuration parameters V1 can be discovered at a later time on demand.
  • Add missing UAM rules for Door Lock generated Notification events.

Checklist

@nkljajic
Copy link
Collaborator Author

nkljajic commented Nov 29, 2023

@rzr I had not implemented unit test for this new command. Another issue is that this also affects ZigPC.

@nkljajic nkljajic changed the title Feature door lock get all pin codes GH-23: Feature door lock get all pin codes Nov 29, 2023
@nkljajic nkljajic marked this pull request as ready for review November 29, 2023 13:41
@nkljajic
Copy link
Collaborator Author

Implementing Z-Wave specific commands, not available in Zigbee Cluster Library can be done by extending existing ZCL xml definitions or creating new ones. In DoorLock case I opted for the former, which was easier path.

@nkljajic nkljajic force-pushed the feature-DoorLock-GetAllPINCodes branch from 68005b4 to e7f155e Compare November 29, 2023 14:29
sl_log_debug(LOG_TAG, "Skip creating User ID nodes for CC UserCode V1.");
return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason behind this change ? I think CC UserCode V1 support multiple user ids.

Copy link
Collaborator Author

@nkljajic nkljajic Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described above, we do not want to create all 255 User IDs during Interview phase and wait 5 minutes for it to complete.

Copy link

@Thomasdjb Thomasdjb Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we never create user ids for User Code v1 unless using GetAllPin command. This is not good for certification as this should be done automatically. To workaround we could update the user ids (triggering a User Number Get) when using User Code Set on a user id which is not registered. Either way, it has to be done somewhere.

```

To see supported generated commands for DoorLock cluster under the by-unid topic space:

```console
mosquitto_sub -t 'ucl/by-unid/<UNID>/<EP>/DoorLock/SupportedGeneratedCommands'
# Example output
ucl/by-unid/<UNID>/<EP>/DoorLock/SupportedGeneratedCommands { "value": ["LockDoorResponse","UnlockDoorResponse","ToggleResponse","UnlockWithTimeoutResponse","GetLogRecordResponse","SetPINCodeResponse","GetPINCodeResponse","ClearPINCodeResponse","ClearAllPINCodesResponse","SetUserStatusResponse","GetUserStatusResponse","SetWeekdayScheduleResponse","GetWeekdayScheduleResponse","ClearWeekdayScheduleResponse","SetYearDayScheduleResponse","GetYearDayScheduleResponse","ClearYearDayScheduleResponse","SetHolidayScheduleResponse","GetHolidayScheduleResponse","ClearHolidayScheduleResponse","SetUserTypeResponse","GetUserTypeResponse","SetRFIDCodeResponse","GetRFIDCodeResponse","ClearRFIDCodeResponse","ClearAllRFIDCodesResponse","OperatingEventNotification","ProgrammingEventNotification"] }
ucl/by-unid/<UNID>/<EP>/DoorLock/SupportedGeneratedCommands { "value": ["LockDoorResponse","UnlockDoorResponse","ToggleResponse","UnlockWithTimeoutResponse","GetLogRecordResponse","SetPINCodeResponse","GetPINCodeResponse","ClearPINCodeResponse","ClearAllPINCodesResponse","SetUserStatusResponse","GetUserStatusResponse","SetWeekdayScheduleResponse","GetWeekdayScheduleResponse","ClearWeekdayScheduleResponse","SetYearDayScheduleResponse","GetYearDayScheduleResponse","ClearYearDayScheduleResponse","SetHolidayScheduleResponse","GetHolidayScheduleResponse","ClearHolidayScheduleResponse","SetUserTypeResponse","GetUserTypeResponse","SetRFIDCodeResponse","GetRFIDCodeResponse","ClearRFIDCodeResponse","ClearAllRFIDCodesResponse","OperatingEventNotification","ProgrammingEventNotification",] }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma at the end typo ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was generated by zap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ucl/by-unid/<UNID>{{#unless (clusterWithoutEndpoints label)}}/<EP>{{/unless}}/{{asPublicationName label}}/SupportedGeneratedCommands { "value": [{{#zcl_commands}}{{#if (isEqual source "client")}}"{{label}}"{{listComma this}}{{/if}}{{/zcl_commands}}] }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's acceptable to me, I would keep the generated file this has to be fixed in zap :)

not a big deal anyway

@rzr
Copy link
Collaborator

rzr commented Nov 30, 2023

@rzr I had not implemented unit test for this new command. Another issue is that this also affects ZigPC.

Thanks for sharing this info, maybe I should enable zigbee support in CI , if you think it's not read to be merged you can set this PR as draft, as suggested at:

#18

your feedback is valuable

@nkljajic nkljajic marked this pull request as draft November 30, 2023 15:49
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…tAllPINCodes to DoorLock.xml

This a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit f9425c8c48fc290c07d6ff9e290d68675cba30c8)
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…p-generated files for command GetAllPINCodes

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit 674e23edc5d29903b052b197047ebee2f898a578)
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…s in ZPC

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit db74b5f2abe795d3ee1f54c8387dec9d4ded81be)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit 954cdcf4b91776e21760921e9c4f0f086328cc6c)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
… Code Set.

This allows to get confirmation of the User Code Set through Get Pin Code Response

Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit e7debde26fce76a30db3626a8c553348dfb877de)
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…mmand GetAllPINCodes to DoorLock.xml

This a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit f9425c8c48fc290c07d6ff9e290d68675cba30c8)
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…p-generated files for command GetAllPINCodes

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit 674e23edc5d29903b052b197047ebee2f898a578)
Thomasdjb pushed a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
…s in ZPC

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit db74b5f2abe795d3ee1f54c8387dec9d4ded81be)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit 954cdcf4b91776e21760921e9c4f0f086328cc6c)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
… Code Set.

This allows to get confirmation of the User Code Set through Get Pin Code Response

Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit e7debde26fce76a30db3626a8c553348dfb877de)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 18, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwared-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit 5a445787e735313f67e7755a25950d08fe090096)
@rzr
Copy link
Collaborator

rzr commented Jan 18, 2024

Please rebase branch to enable tests :

a3fa1c8

Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-19
(cherry picked from commit 954cdcf4b91776e21760921e9c4f0f086328cc6c)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
… Code Set.

This allows to get confirmation of the User Code Set through Get Pin Code Response

Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit e7debde26fce76a30db3626a8c553348dfb877de)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwared-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit 5a445787e735313f67e7755a25950d08fe090096)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-19
(cherry picked from commit 954cdcf4b91776e21760921e9c4f0f086328cc6c)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
… Code Set.

This allows to get confirmation of the User Code Set through Get Pin Code Response

Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit e7debde26fce76a30db3626a8c553348dfb877de)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 19, 2024
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwared-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit 5a445787e735313f67e7755a25950d08fe090096)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 23, 2024
Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit 66c6bb11ab1bbeeed1307ff9316efcc70b89a476)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 23, 2024
… creation.

Previously we were using ADD_IF_MISSING which creates a GET/REPORT sequence for each
UserID. This was slowing down the interview process.
User Code and User Id Status are now set to default value, which can be
updated using GetAllPinCodes command.

Bug-SiliconLabs: UIC-3062
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit ad72e34eb291502c60940a59e5db7d52cd744e41)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 23, 2024
… creation.

Previously we were using ADD_IF_MISSING which creates a GET/REPORT sequence for each
UserID. This was slowing down the interview process.
User Code and User Id Status are now set to default value, which can be
updated using GetAllPinCodes command.

Bug-SiliconLabs: UIC-3062
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit ad72e34eb291502c60940a59e5db7d52cd744e41)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 23, 2024
Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit 66c6bb11ab1bbeeed1307ff9316efcc70b89a476)
Thomasdjb added a commit to Thomasdjb/UnifySDK that referenced this pull request Jan 23, 2024
… creation.

Previously we were using ADD_IF_MISSING which creates a GET/REPORT sequence for each
UserID. This was slowing down the interview process.
User Code and User Id Status are now set to default value, which can be
updated using GetAllPinCodes command.

Bug-SiliconLabs: UIC-3062
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit ad72e34eb291502c60940a59e5db7d52cd744e41)
nkljajic and others added 11 commits January 23, 2024 16:22
This is a contribution per the CLA

Signed-off-by: Nenad Kljajic <[email protected]>
…NCodes

This is a contribution per the CLA

Signed-off-by: Nenad Kljajic <[email protected]>
This is a contribution per the CLA

Signed-off-by: Nenad Kljajic <[email protected]>
This is a contribution per the CLA

Signed-off-by: Nenad Kljajic <[email protected]>
…mmand GetAllPINCodes to DoorLock.xml

This a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit f9425c8c48fc290c07d6ff9e290d68675cba30c8)
…p-generated files for command GetAllPINCodes

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit 674e23edc5d29903b052b197047ebee2f898a578)
…s in ZPC

This is a patch integration into the Unify codebase.
This may be squashed into the original commit.

Origin: SiliconLabs#23
Bug-SiliconLabs: UIC-3201
Signed-off-by: Nenad Kljajic <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-18
(cherry picked from commit db74b5f2abe795d3ee1f54c8387dec9d4ded81be)
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
Last-Update: 2024-01-19
(cherry picked from commit 954cdcf4b91776e21760921e9c4f0f086328cc6c)
… Code Set.

This allows to get confirmation of the User Code Set through Get Pin Code Response

Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit e7debde26fce76a30db3626a8c553348dfb877de)
Bug-SiliconLabs: UIC-3201
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwared-SiliconLabs: thdubois/UIC-3072/c4/develop
(cherry picked from commit 5a445787e735313f67e7755a25950d08fe090096)
Bug-SiliconLabs: UIC-3072
Signed-off-by: Thomas du Boisrouvray <[email protected]>
Forwarded-SiliconLabs: thdubois/UIC-3072/c4/develop

(cherry picked from commit 66c6bb11ab1bbeeed1307ff9316efcc70b89a476)
@nkljajic nkljajic force-pushed the feature-DoorLock-GetAllPINCodes branch from 6985a1d to 3931a0d Compare January 23, 2024 15:22
@Thomasdjb
Copy link

Hi Nenad,
Regarding your issue with the inclusion of 255 UserIDs device.
Unfortunately, the interview process for command class User Code is specified by the AWG specification (CL:0063.01.21.01.2).
For nodes supporting the v1 of UserCode command class, the controller must ask for the PIN code with an User Code Get for each User Identifier.
Bypassing this process will cause certification to fail.
On my setup interviewing a node with ~150 UserID takes less than 30 seconds. Maybe there is another problem with your setup ?

@nkljajic
Copy link
Collaborator Author

nkljajic commented Jan 23, 2024

@Thomasdjb you are right, as per
Z-Wave Command Class Control Specification
CL:0063.01.21.02.1
We must ask for all pin codes during Interview stage.

However, per CL:0000.00.13.04.1 and CL:0000.00.21.02.1 we can do it later (before sending first command), and not automatically right after joining stage?
CL:0000.00.13.04.1

If a Command Class is partially controlled, the controlling node MAY skip steps or perform them in a
different order for the mandatory node interview detailed in this document.

CL:0000.00.21.02.1

This interview MUST be performed prior to issuing any control command to the supporting node.
Preferably, it should be done during the commissioning phase of the supporting node or shortly after
the inclusion of the controlling node

My emphasis is on "shortly after the inclusion"
My proposal is to delay, not skip, get all pin codes step. Perhaps device should step into "Online non-functional" after joining?

Legacy Z-Wave S0 Door Locks are slow, S2 do not have that issue.
Problem is elevated when we want to add several locks at once, and all of them start sending all possible pin codes at the same time.

@rzr
Copy link
Collaborator

rzr commented Feb 14, 2024

Please rebase on

https://github.com/SiliconLabs/UnifySDK/releases/tag/ver_1.5.0

feedback welcome

@Thomasdjb
Copy link

@Thomasdjb you are right, as per Z-Wave Command Class Control Specification CL:0063.01.21.02.1 We must ask for all pin codes during Interview stage.

However, per CL:0000.00.13.04.1 and CL:0000.00.21.02.1 we can do it later (before sending first command), and not automatically right after joining stage? CL:0000.00.13.04.1

If a Command Class is partially controlled, the controlling node MAY skip steps or perform them in a
different order for the mandatory node interview detailed in this document.

CL:0000.00.21.02.1

This interview MUST be performed prior to issuing any control command to the supporting node.
Preferably, it should be done during the commissioning phase of the supporting node or shortly after
the inclusion of the controlling node

My emphasis is on "shortly after the inclusion" My proposal is to delay, not skip, get all pin codes step. Perhaps device should step into "Online non-functional" after joining?

Legacy Z-Wave S0 Door Locks are slow, S2 do not have that issue. Problem is elevated when we want to add several locks at once, and all of them start sending all possible pin codes at the same time.

I can provide a patch to delay the probing of user code with timer but it is too device specific to be added into the UnifySDK.
My advice would be to propose a change in the specification to clear up the inclusion delay. Then we can work on integrating a solution into the Unify SDK.

@rzr
Copy link
Collaborator

rzr commented Jun 13, 2024

Please rebase this PR if you want to move this change forward or close

@nkljajic nkljajic closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants