Repository navigation
zigbee lock: use structured utils to uniformly handle zigbee code responses - #3331
hcarter-775 wants to merge 6 commits into
Conversation
|
Invitation URL: |
|
zigbee-lock_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 1eab2cd |
Test Results 73 files 550 suites 0s ⏱️ Results for commit 1eab2cd. ♻️ This comment has been updated with latest results. |
hcarter-775
changed the title
| if not credential then | ||
| -- check what user slot we have to associate this with and add a default user | ||
| local next_index = tables.next_index(device, "users") | ||
| if next_index and next_index <= tables.get_max_entries(device, "users") then |
There was a problem hiding this comment.
This sanity check did not get carried over to the library function, but I think we should keep it
There was a problem hiding this comment.
would you clarify- which sanity check? If you mean the check on number of users, I disagree. users do not actually map to anything on the device, so I do not think we should limit the number of users we can create, at least via the direct APIS. If someone wants to limit it somewhere else (like in oneapp), they are free to do so.
There was a problem hiding this comment.
Yeah, I am referring to the line
if next_index and next_index <= tables.get_max_entries(device, "users") then
which doesn't move forward with making a user if we are at the max entries for the user table. I still think we should limit the number of users that can be created, even via the direct APIs. I believe we should respect the maximums that are provided via the capability definition, which is something that the driver itself provides via a capability event. Is there a benefit to not limiting the users to the max number that the capability reports? It seems like it would just result in unnecessary extra unusable users for the app to have to deal with, and could potentially cause confusion to users.
There was a problem hiding this comment.
Following up from this comment: #3331 (comment), the device does not even tell us the number of users in this context it supports- it tells us the number of credentials. I think we'd be making a jump to say that the user count needs to match what value we are matching with- I am not convinced that the value we match against is valid in this context.
| capabilities.lockUsers.users( | ||
| { { userIndex = 20, userName = "Guest 20", userType = "guest" } }, | ||
| capabilities.lockCredentials.credentials( | ||
| { { userIndex = 1, credentialIndex = 20, credentialType = "pin", credentialName = "Guest 1" } }, |
There was a problem hiding this comment.
The PR #3333 introduces the concept of being able to set a credentialIndex directly by tying it to the userIndex provided in the addCredential command. However, this seems to contradict what is being done here, since the userIndex no longer has parity with the credentialIndex.
I might be confused on what is being gained by setting the users in order rather than having the index match the credential index where possible. I know that is already a fragile mapping to begin with, but I feel that at least trying to make userIndex == credentialIndex where possible makes the changes that are proposed in #3333 make more sense. What is the driver for making the userIndexes increase incrementally regardless of the credentialIndex?
There was a problem hiding this comment.
At present, User index is a completely self-defined feature that is irrelevant to the device-level understanding of the device. For example, index 0 is an index that can exist on the device, but not on the users table. So what should we do- just break the system in that single case? If we do that, and set it to "5" for example, what do we do with credential index 5? Set it according to 6? And then, if we're just doing a best guess thing here, why would we do that at all?
This is the reasoning. Even though it is "different", the previous handling is not set in stone in my opinion, and is fairly arbitrary itself (I was the one who wrote it, after all).
There was a problem hiding this comment.
Thanks for the explanation! Most of my context on this was just based on the current convention so I was not sure if the the driver was originally set up this way for a specific reason or not (it sounds like it was arbitrary)
Just a quick check to make sure I understand correctly - multiple users can share a credential, right? i.e the credential in slot 5 could be used by users with index 5 and index 6, is that right? Or will #users == #credentials?
There was a problem hiding this comment.
yeah, technically multiple users can share a credential. But the more primary concept would be that one user can share multiple credentials. However, this is mostly a Matter thing (the whole capability schema was defined in the context of Matter), so that's why I'm eschewing that in this work to create a more explicit 1-1 mapping by default. If one wants to specify different things, they can, but I imagine they should do that outside of the addCredential function, for better and for worse.
addCredential has no credentialIndex argument, so its userIndex argument now selects the credential slot (0 auto-assigns). The user is always new and takes the next free user slot. Reject a requested slot that is already occupied. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Description of Change
Introduces a couple new helper functions that have the opportunity to improve the handling here slightly, by issuing users in order, rather than issuing them according to a possibly arbitrary order of read credentials. It uses this new handling in both locations where a user may be reported to us without us having explicitly sent a create command, and in locations where we explicitly set a code on the device.
Summary of Completed Tests
Tested on-device. Unit tests updated