[Aqara/Locks] Remove unnecessary sequence number increment check#2931
[Aqara/Locks] Remove unnecessary sequence number increment check#2931haedo-doo wants to merge 2 commits intoSmartThingsCommunity:mainfrom
Conversation
Test Results 72 files 506 suites 0s ⏱️ Results for commit b02a6de. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against b02a6de |
|
Avoid redundant key exchanges triggered by out-of-order Zigbee messages under poor signal conditions. |
| end | ||
| else | ||
| request_generate_shared_key(device) | ||
| device:set_field(SERIAL_NUM_RX, serial_num) |
There was a problem hiding this comment.
Do you anticipate a future use of this or could be considered for removal? In the current code I don't see anything else reading the SERIAL_NUM_RX field.
There was a problem hiding this comment.
Thanks for the feedback. I've removed the unnecessary fields.
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
There currently are not any tests for how an old serial number in a message is handled. Please add a test to show what events the driver will emit if such a message is received.
| local serial_num = toValue(msg, 3, 2) | ||
| local last_serial_num = device:get_field(SERIAL_NUM_RX) or 0 | ||
|
|
||
| if serial_num > last_serial_num then |
There was a problem hiding this comment.
With this check removed, how will duplicate/stale messages that are sent by a device be detected? It seems like this could result in extra/stale lock events being emitted.
There was a problem hiding this comment.
Thanks for the feedback. The main reason for this change is to prevent an infinite loop of shared key regeneration caused by out-of-order messages in unstable Zigbee networks.
In the current logic, if a message (e.g., N+1) is lost but subsequent messages (N+2 to N+8) arrive first, the driver expects the next sequence to be higher than N+8. When the door lock later retries the lost N+1, the driver treats it as an invalid or stale packet and immediately triggers the shared key regeneration process. In poor network conditions, this cycle repeats indefinitely, leading to a deadlock where the device becomes unusable.
| toValue(payload, 6, string.byte(payload, 5))) | ||
| end | ||
| else | ||
| request_generate_shared_key(device) |
There was a problem hiding this comment.
What is the initial reason for having this here? are there cases where a key does need to be generated if the message serial numbers start getting out of order?
There was a problem hiding this comment.
While this validation was initially implemented following the manufacturer's guide, it causes severe side effects in real-world environments. Therefore, I decided to remove it to ensure connectivity and stability.
Regarding the test cases, the SmartThings Edge Driver framework does not support simulating the encryption key generation process or assigning arbitrary keys. Since the door lock communicates all data with the hub exclusively through encrypted messages, it is currently impossible to implement meaningful test cases that incorporate this encryption logic. We ask for your understanding regarding these platform-level constraints.
3 participants
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests