Repository navigation
Conversation
|
Invitation URL: |
|
zwave-lock_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against ccfcada |
6d08930 to
e1f498a
Compare
ctowns
changed the title
|
|
||
| -- Returns the credentialIndex of the device's master code slot, or nil if the device does not have one. | ||
| -- Only Yale locks have one. | ||
| function table_utils.master_credential_index(device) |
There was a problem hiding this comment.
I do not think we should put Yale-specific code in this tables file, which is for capability contract stuff. This also seems like it should just be in a yale subdriver?
There was a problem hiding this comment.
cool! I will say, the specific function I mentioned, function table_utils.master_credential_index(device), still exists at the tables level, which I think is maybe a mistake. I see how it's used in your new is_full table util, but I think my issue is still that this capability-level wrapper and its associated functions shouldn't have manufacturer-specific logic. I think is_full is probably either 1. obfuscating a manufacturer-specific override, or 2. this handling of the master credential index is not done generically enough at present.
This is kinda what I was getting at here: #3312 (comment) basically, rather than add this complex idea of how to not count some credentials than might be allowed, we could instead just bump the limit if we set an admin, by like 1 per admin. This would keep our internal max in line with what we actually think the "max" is, rather than defining some non-rule following type of credential
There was a problem hiding this comment.
I am just documenting here that we discussed this offline and plan to take another look at how to organize the split between generic and specific handling so that the +1 to user max if one user is a programming user is more easily maintainable. Currently, I think the method of checking if any current user is a programming user and then excluding t hem from the total is the most "generic" way to do it, but it is not the most efficient or necessary. This is something we will follow up on after addressing necessary functional changes.
|
Generally, my opinion here is that things look good, but that there is a lot of Yale-specific stuff in otherwise generic handling. I think we should make efforts to distinguish these things, and pull Yale stuff into its own subdriver wherever possible. |
|
|
1316281 to
cc51717
Compare
|
|
||
| -- Returns the credentialIndex of the device's master code slot, or nil if the device does not have one. | ||
| -- Only Yale locks have one. | ||
| function table_utils.master_credential_index(device) |
There was a problem hiding this comment.
cool! I will say, the specific function I mentioned, function table_utils.master_credential_index(device), still exists at the tables level, which I think is maybe a mistake. I see how it's used in your new is_full table util, but I think my issue is still that this capability-level wrapper and its associated functions shouldn't have manufacturer-specific logic. I think is_full is probably either 1. obfuscating a manufacturer-specific override, or 2. this handling of the master credential index is not done generically enough at present.
This is kinda what I was getting at here: #3312 (comment) basically, rather than add this complex idea of how to not count some credentials than might be allowed, we could instead just bump the limit if we set an admin, by like 1 per admin. This would keep our internal max in line with what we actually think the "max" is, rather than defining some non-rule following type of credential
| if lock_utils.is_device_busy(device) then | ||
| lock_utils.emit_command_result(device, capabilities.lockCredentials, consts.LOCK_CREDENTIALS.ADD, consts.COMMAND_RESULT.BUSY) | ||
| return | ||
| end | ||
|
|
||
| lock_utils.set_busy_state(device, consts.LOCK_CREDENTIALS.ADD, { | ||
| userIndex = cred_index, | ||
| credentialIndex = cred_index, | ||
| credentialType = command.args.credentialType, | ||
| credentialName = command.args.credentialName, | ||
| }) | ||
| device:send(UserCode:Set({ | ||
| user_identifier = cred_index, | ||
| user_code = command.args.credentialData, | ||
| user_id_status = user_id_status.ENABLED_GRANT_ACCESS | ||
| })) |
There was a problem hiding this comment.
this all looks identical to the generic addCredential handler? Maybe we should just use the regular add credential command in this case as well, with the bit of extra admin type guarding on the top?
Like basically (idk if this exact thing will work) but maybe do:
local function add_credential(driver, device, command)
-- only an admin code credential may be written to the admin code slot, and it is not limited by the lock's user slots
if is_admin_credential(device, command.args.userIndex) and command.args.userType ~= consts.ADMIN.USER_TYPE then
lock_utils.emit_command_result(device, capabilities.lockCredentials, consts.LOCK_CREDENTIALS.ADD, consts.COMMAND_RESULT.INVALID_COMMAND)
return
end
cap_handlers.add_credential(driver, device, command)
end
Looks like you have this in the update handler, for example. We could just pull this into a helper that wraps the generic functions?
There was a problem hiding this comment.
This is removed entirely now that the generic handler rejects the programmingUser type (same as Zigbee). Removed here: 9d757d3
This whole sub driver should be much simpler now 👍
| zwave_handlers.notification_report(driver, device, cmd) | ||
| end | ||
|
|
||
| local function add_credential(driver, device, command) |
There was a problem hiding this comment.
should programmingUser be a state that anyone calling this can set?
There was a problem hiding this comment.
As discsussed elsewhere, no! So we probably don't need the send_add_credential helper function anymore
There was a problem hiding this comment.
Agreed - I have made it so that programmingUser is rejected in add_credential, and I also added the handling to make sure that the type cannot be changed.
Since this can now be handled generically and doesn't have a Yale sub driver-specific handler, we no longer need the send_add_credential helper. It has been removed.
Deletes were only reported when the lock confirmed them. A lock that refuses a delete (a Yale admin code) ignores the User Code Set and sends nothing, so the command never completed. If a delete is not confirmed within DELETE_CONFIRM_DELAY, the slot is now read back. A report that the slot is still occupied fails the deleteCredential, and the deleteUser that injected it, and leaves the user and credential in place. A timer left from an earlier delete is cancelled when a new one starts. The deleteCredential that deleteUser injects is now handled before the index 0 check, so a stale credential at index 0 is removed locally instead of being sent as user identifier 0. The tests for this are in the Yale admin code commit, which adds the lock that refuses deletes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A lock may keep an admin code in a credential outside its user slots. It is exposed like any other credential, owned by a user with the programmingUser type (consts.ADMIN). Generic lock code only knows that rule; where the admin code lives is left to the sub-driver for the lock. - tables.get_admin_credential finds the programmingUser credential outside the user slots. A programmingUser in an ordinary user slot is an ordinary user. - The admin code and its user do not count toward the credential and user limits (tables.is_full), and add_entry can skip the limit for the admin code itself. - deleteAllCredentials, deleteAllUsers and the all user codes deleted reports (Notification and Alarm v1 type 32) keep the admin code in state, since the lock keeps it. - send_add_credential and is_code_occupied are shared so a sub-driver can reuse them. The tests for this are in the Yale admin code commit, the first lock with an admin code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4819081 to
5586440
Compare
|
I just pushed an update that should address all the feedback below. I also unstaged all the changes and had claude sort them into separate commits for easier review. Major updates with the new push:
|
Yale locks keep their admin (master) code in user code slot 251, outside the lock's user slots. The new yale-lock sub-driver, used for migrated Yale locks, exposes it as a credential at index 251 owned by a programmingUser user with the same index. - addCredential at index 251 writes the admin code. It must have the programmingUser type, is not limited by the lock's user slots, and is rejected as occupied if an admin code is already recorded. The lock confirms it with either NEW_USER_CODE_ADDED (ZW2) or the master code changed notification (ZW3). - A report of slot 251 records the admin code, including one set at the lock, one reported during a sync, and one reported during a command for another credential. While the admin code itself is being updated or deleted, its reports go to the generic handlers. - After a sync, slot 251 is read separately, since it is outside the slots the sync reads. - A programmingUser user can only be the admin code user: addUser rejects the type, and updateUser cannot give it to another user or take it from the admin code user. Adds tests for the admin code, and for the generic delete and admin code behavior it relies on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The lockCodes migration moves to legacy-handlers/migrate.lua and takes the lock's admin code slot, if it has one. That slot becomes a programmingUser user with the slot as its index, and the other codes get sequential user indexes that skip it. A new legacy yale-lock sub-driver migrates Yale locks with slot 251 as the admin code slot. lockCodes only holds the admin code if SmartThings set it and the lock confirmed it as a user code, so otherwise the slot is read from the lock after migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5586440 to
ec5b8fc
Compare
Use the same programming user naming as the Zigbee lock driver: - consts.ADMIN becomes consts.PROGRAMMING_USER, and its default name is now "Programming Code". - get_admin_credential, delete_all_non_admin_entries and read_admin_code become get_programming_credential, delete_all_non_programming_entries and read_programming_code. - yale-lock/admin.lua becomes yale-lock/programming_code.lua, and the Yale handlers, lockCodes migration, tests and comments follow. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ate programming users Match the Zigbee lock driver and move the programming user guards into the generic handlers, so they apply to every lock rather than only Yale: - addUser and addCredential reject the programming user type. - addCredential rejects a userIndex that names an existing programming user, so it cannot be given another credential. - updateUser cannot make a user a programming user, or change a programming user to another type. Add lock_utils.is_programming_user and drop the Yale addUser and updateUser wrappers, which the generic handlers now cover. Yale keeps its addCredential, which owns the programming code slot. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s at index 0 Index 0 is the Z-Wave "all users" wildcard and never a credential, so deleteCredential does not need to handle a stale entry there: - Inline send_credential_delete into delete_credential, which now picks the credential index per case and has a single send and read-back. - Restore the up-front rejection of credentialIndex 0. - Ignore reported credentials below the table minimum in add_reported_credential, the one way an index 0 entry could be added. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| -- an admin code after a "Master Code Changed" response. | ||
| if LockCodesDefaults.get_lock_codes(device)[tostring(yale_admin.SLOT)] == nil then | ||
| yale_admin.read_admin_code(device) | ||
| end |
There was a problem hiding this comment.
I wonder how this is going to interact with the sync logic, which is also doing the same get, but expecting particular data. I suppose it is getting caught at the yale-subdriver, but I wonder if this won't end up causing us to skip an index. I guess it probably won't because the sync doesn't start for 15s, but worth a callout I think
There was a problem hiding this comment.
I added a little extra handling to prevent us from skipping an index as you pointed out - commit a5b7914 covers this case!
Match the Zigbee lock driver and refuse to delete a programming user's credential instead of sending the delete to the lock. A standalone deleteCredential fails, and a deleteUser fails for both lockUsers and lockCredentials. Nothing is sent to the lock. This removes the read-back that checked whether the lock had refused a delete: the delete confirm timer and its constants, is_delete_in_progress, emit_delete_failure and the matching branch in the user code report. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The generic sync handler treats a report for an unexpected index as an empty slot and moves on, which skips an index. The Yale programming code slot is outside the sync's range and is read separately, so a report for it during a sync, such as the read after a migration, must not reach the sync unless the sync asked for that slot. An occupied report is still recorded, and an unrequested empty report is now ignored instead of advancing the sync. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Do not allow addCredential to create the Yale programming code. Yale locks need their programming code to onboard, so the code sync (and the read after a migration) records it, like the Zigbee lock driver, and updateCredential is the only way to change it. - Remove the Yale addCredential wrapper and the handling that only completed an add: the add result in programming_code_reported and the ZW3 "programming code changed" branch. - Check for an update in progress, not "anything but an add", when deciding whether a report belongs to the generic handlers. - Inline send_add_credential back into the generic add_credential, which is now its only caller. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests