Repository navigation
Zigbee Lock: Support master codes and validate migration indices - #3321
hcarter-775 wants to merge 3 commits into
Conversation
Add master code support to the zigbee lock driver, allow any positive slot index to be tried, add explicit index minimums to the capability definitions, and move Yale-specific handling into a yale sub-driver. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Channel deleted. |
Test Results 73 files 548 suites 0s ⏱️ Results for commit e1b270c. ♻️ This comment has been updated with latest results. |
|
zigbee-lock_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against e1b270c |
| end | ||
| end | ||
|
|
||
| -- Some Yale locks may report a successful ClearPINCode for the master code (slot 0) but do not actually delete it, |
There was a problem hiding this comment.
Do we have multiple Yale Zigbee locks that we have tested with? Is there any distinction between which ones report this but don't actually delete the code vs. those that do?
There was a problem hiding this comment.
We don't- this is the safe option in my opinion, between letting devices accidentally delete it, and not allowing them the option at all.
| local occupied = {} | ||
| for _, entry in ipairs(t) do occupied[entry[def.match_key]] = true end | ||
| local idx = 1 | ||
| local idx = def.minimum |
There was a problem hiding this comment.
Won't this make it so that all locks will start at index 0? Even ones where we aren't tracking the master code like we are for Yale? It feels like if this slot could be used for master codes on other locks, we should leave it reserved for that.
There was a problem hiding this comment.
We always sync codes starting at 0 on device initialization, and this commit (e1b270c) strengthens that binding a good bit.
That commit also changes the user type from guest to master if a MASTER_CODE_CHANGED event ever comes in for a code we already store.
What I'm trying to say is that all locks, if they have a code at index 0, should tell us that prior to us doing anything. I don't think we should block index 0 from getting used just because there might be a master code on it, since we explicitly check that.
| -- addCredential has no credentialIndex argument, so its userIndex argument selects the credential slot. A value | ||
| -- of 0 means "auto-assign the next available slot". The user is always new, and takes the next available user slot. | ||
| local cred_index = command.args.userIndex == 0 and tables.next_index(device, "credentials") or command.args.userIndex | ||
| local user_index = tables.next_index(device, "users") |
There was a problem hiding this comment.
As you noted, with this change calling addCredential will create a new user. Previously one could call addUser to create a user and then addCredential to add a PIN for that user, so with this change that no longer works. It's not clear to me this is the right thing to do, but I also don't think it's related to the support for master code. Could we take this change out of this PR and discuss it separately?
| end | ||
|
|
||
| if user_id >= tables.get_max_entries(device, "credentials") or | ||
| if #(tables.get_state(device, "credentials") or {}) >= tables.get_max_entries(device, "credentials") or |
There was a problem hiding this comment.
I think this change may allow it to keep reading past the number of supported slots because it no longer checks to see if the user_id is >= the max entries.
|
|
|
Closing this PR. It has been replaced by a series of smaller PRs, including: |
Description of Change
The primary thing this PR does is add master code support to the zigbee lock driver. For now, it uses the
remoteOnlyUsertype to specify which slots are master codes, but that should be updated soon.Besides that, it validates all indices from the lockCodes before migrating them- this is especially useful for ensuring no 0-slotted codes are matched to a users entry, since that has a minimum of 1. It also allows any positive slot index to be tried, adds explicit index minimums to the capability definitions and moves Yale-specific handling into a yale sub-driver.
A more subtle update is that we create a more explicit 1-1 mapping between users and credentials. If a credential is added, a new user will be created, if that credential creation succeeds. The chosen user indices will follow a simple sequential process, adding the lowest user entry possible in all cases. Credentials, on the other hand, are now specifiable via setting a particular userIndex in the addCredential function. If 0 is chosen, the next available credential index will be tried- else, the specified index will be tried. This alters the current idea from being a user-first style of code creation to a credential-first style, speaking generically.
Summary of Completed Tests
Many unit tests have been updated to verify the new functionality, and many unit tests have been added to verify all new concepts. Add/migrate/crud testing has been completed on a Yale device (and soon a Kwikset device) to ensure all functionality remains intact.