Skip to content

[Aqara/Locks] Remove unnecessary sequence number increment check #2931

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.

Sign up for GitHub

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

Open
haedo-doo wants to merge 2 commits into
base: main
Choose a base branch
from
+4 −14
Open

[Aqara/Locks] Remove unnecessary sequence number increment check #2931

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fcf608
Remove unnecessary sequence number increment check
haedo-doo Apr 29, 2026
b02a6de
Refactor: Remove unnecessary field values
haedo-doo May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions drivers/Aqara/aqara-lock/src/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ local CLOUD_PUBLIC_KEY = "__cloud_public_key"
local SUPPORTED_ALARM_VALUES = { "damaged", "forcedOpeningAttempt", "unableToLockTheDoor", "notClosedForALongTime",
"highTemperature", "attemptsExceeded" }
local SERIAL_NUM_TX = "serial_num_tx"
local SERIAL_NUM_RX = "serial_num_rx"
local SEQ_NUM = "seq_num"
local THRESHOLD_BATTERY = {
["aqara.lock.akr011"] = { low = 47, dryout = 28 }, -- K100
Expand Down Expand Up @@ -67,7 +66,6 @@ local function comp_supported_alarm_values(last_alarm_values)
end

local function device_init(self, device)
device:set_field(SERIAL_NUM_RX, 0)
device:set_field(SERIAL_NUM_TX, 1)
device:set_field(SEQ_NUM, 0)
local last_alarm_values = device:get_latest_state("main", LockAlarm.ID, LockAlarm.supportedAlarmValues.NAME) or {}
Expand Down Expand Up @@ -240,7 +238,6 @@ local function lock_state_handler(driver, device, value, zb_rx)
if res then
print(res)
end
device:set_field(SERIAL_NUM_RX, 0)
device:set_field(SERIAL_NUM_TX, 1)
elseif shared_key == nil then
request_generate_shared_key(device)
Expand All @@ -252,17 +249,10 @@ local function lock_state_handler(driver, device, value, zb_rx)
local text = string.sub(msg, 5, string.len(msg))
local payload = string.sub(text, 4, string.len(text))
local func_id = toValue(payload, 1, 1) .. "." .. toValue(payload, 2, 1) .. "." .. toValue(payload, 3, 2)
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
Copy link
Copy Markdown
Contributor

@cjswedes cjswedes Apr 29, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@haedo-doo haedo-doo May 6, 2026

Choose a reason for hiding this comment

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

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.

device:set_field(SERIAL_NUM_RX, serial_num)
if resource_id[func_id] then
resource_id[func_id].event_handler(driver, device, resource_id[func_id].event_name,
toValue(payload, 6, string.byte(payload, 5)))
end
else
request_generate_shared_key(device)
Copy link
Copy Markdown
Contributor

@cjswedes cjswedes Apr 29, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@haedo-doo haedo-doo May 6, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@cjswedes cjswedes May 6, 2026

Choose a reason for hiding this comment

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

Given the lack of ability to add unit tests, I assume this is tested manually.


if resource_id[func_id] then
resource_id[func_id].event_handler(driver, device, resource_id[func_id].event_name,
toValue(payload, 6, string.byte(payload, 5)))
end
end
end
Expand Down
Loading