Update matter-lock driver for U-tec #1812

HunsupJung · 2024-12-10T07:55:30Z

Check all that apply

Type of Change

WWST Certification Request
- If this is your first time contributing code:
  - I have reviewed the README.md file
  - I have reviewed the CODE_OF_CONDUCT.md file
  - I have signed the CLA
- I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
Bug fix
New feature
Refactor

Checklist

I have performed a self-review of my code
I have commented my code in hard-to-understand areas
I have verified my changes by testing with a device or have communicated a plan for testing
I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

https://smartthings.atlassian.net/browse/IOTE-4705

Summary of Completed Tests

github-actions · 2024-12-10T07:55:59Z

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions · 2024-12-10T07:56:58Z

Invitation URL:
https://bestow-regional.api.smartthings.com/invite/Bxlr8exwxxMP

Signed-off-by: Hunsup Jung <[email protected]>

github-actions · 2024-12-10T07:58:51Z

File	Coverage
All files	`90%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/lock_utils.lua	`98%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/init.lua	`91%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/new-matter-lock/init.lua	`90%`	✅

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against fa76eda

github-actions · 2024-12-10T07:59:55Z

Test Results

64 files 403 suites 0s ⏱️
2 005 tests 2 005 ✅ 0 💤 0 ❌
3 459 runs 3 459 ✅ 0 💤 0 ❌

Results for commit fa76eda.

♻️ This comment has been updated with latest results.

nickolas-deboom · 2024-12-12T15:37:58Z

After an initial review, the changes look good to me overall. I have two comments -
(1) are there additional profiles needed for the possible combinations of lock/user/pin/schedule + battery/batteryLevel? Or are we just providing profiles for known configurations at this time?

(2) Would you be able to create a few test cases similar to what I have here? https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1796/files#diff-188bbf519a245b9967a0a86552cf7b20bef0a2b5ce6598196cca7e6591acc56d

(In order to show that the BatPercentRemaining attribute being present in the AttributeList results in the correct profile being chosen.)

HunsupJung · 2024-12-16T11:44:25Z

(1) are there additional profiles needed for the possible combinations of lock/user/pin/schedule + battery/batteryLevel? Or are we just providing profiles for known configurations at this time?

There may be other possible combinations. But I added lock-user-pin-batteryLevel, lock-user-pin-battery and lock-user-pin-scheduled-battery which are highly likely. For U-tec, the lock-user-pin-batteryLevel profile is enough.
Do you want to consider all edge cases?

(2) Would you be able to create a few test cases similar to what I have here?

Okay, I will update it.

nickolas-deboom · 2024-12-17T21:30:59Z

(1) are there additional profiles needed for the possible combinations of lock/user/pin/schedule + battery/batteryLevel? Or are we just providing profiles for known configurations at this time?

There may be other possible combinations. But I added lock-user-pin-batteryLevel, lock-user-pin-battery and lock-user-pin-scheduled-battery which are highly likely. For U-tec, the lock-user-pin-batteryLevel profile is enough. Do you want to consider all edge cases?

(2) Would you be able to create a few test cases similar to what I have here?

Okay, I will update it.

I think it's ok to include just the likely ones for now rather than every combination.

Signed-off-by: Hunsup Jung <[email protected]>

ctowns · 2024-12-20T20:46:58Z

drivers/SmartThings/matter-lock/src/new-matter-lock/init.lua

@@ -1685,7 +1752,10 @@ local new_matter_lock_handler = {
    capabilities.lock,
    capabilities.lockUsers,
    capabilities.lockCredentials,
-    capabilities.lockSchedules
+    capabilities.lockSchedules,
+    capabilities.remoteControlStatus,


There is no handler for remoteControlStatus, should there be or should this be removed?

I don't think it's needed. I will remove it.

ctowns · 2024-12-20T20:58:21Z

drivers/SmartThings/matter-lock/src/test/test_new_matter_lock_battery.lua

+end
+test.set_test_init_function(test_init)
+
+test.register_coroutine_test(


Could you create a test case for the following cases also?

battery percent remaining is available ✅

battery charge level only is available ❌

battery charge level and battery percent remaining are available ❌

You will just need to modify the attributes list in the AttributeList:build_test_report_data function you are using to test these different combinations of attributes in the new test cases.

I thought the second case was added, but it wasn't. I will modify it.

For reference, it is impossible to support only Battery percent remaining. Because if server supports BAT feature, Battery change level is mandatory. I will create as follows.

Attributes related to BAT feature is not available.

battery charge level only is available.

battery charge level and battery percent remaining are available.

ctowns · 2024-12-20T21:04:19Z

drivers/SmartThings/matter-lock/src/new-matter-lock/init.lua

+  local support_battery_percentage = false
+  for _, attr in ipairs(ib.data.elements) do
+    -- Re-profile the device if BatPercentRemaining (Attribute ID 0x0C) is present.
+    if attr.value == 0x0C then


Can we add an explicit check for the BatChargeLevel attribute here?

if attr.value == 0x0C then support_battery_percentage = true end if attr.value == 0x0E then support_battery_level = true end ------then later add this :----------- if support_battery_percentage then profile_name = profile_name .. "-battery" elseif support_battery_level profile_name = profile_name .. "-batteryLevel" end

Okay, I will

ctowns

This looks pretty good to me! After adding the additional unit tests and making the small change in the handler, I think this will be good to go - thank you @HunsupJung !

Also, has this been regression tested with the other locks that use this driver?

Kwang-Hui · 2024-12-23T05:55:08Z

I tested this PR with Aqara U200/U300 door lock - it was fine. Thank you.

Signed-off-by: Hunsup Jung <[email protected]>

Update matter-lock driver for U-tec

3c60f45

Signed-off-by: Hunsup Jung <[email protected]>

HunsupJung force-pushed the update-matter-lock-for-u-tec branch from e386b18 to 3c60f45 Compare December 10, 2024 07:58

lelandblue requested review from ctowns, z-michel, hcarter-775 and nickolas-deboom December 10, 2024 13:19

Update test cases for matter-lock

aa47454

Signed-off-by: Hunsup Jung <[email protected]>

HunsupJung force-pushed the update-matter-lock-for-u-tec branch from 2d7e508 to aa47454 Compare December 20, 2024 07:52

ctowns reviewed Dec 20, 2024

View reviewed changes

HunsupJung added 2 commits December 24, 2024 14:02

Update handle_power_source_attribute_list function

4592168

Signed-off-by: Hunsup Jung <[email protected]>

Add test cases for matter-lock driver

fa76eda

Signed-off-by: Hunsup Jung <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update matter-lock driver for U-tec #1812

Update matter-lock driver for U-tec #1812

HunsupJung commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

nickolas-deboom commented Dec 12, 2024 •

edited

Loading

HunsupJung commented Dec 16, 2024

nickolas-deboom commented Dec 17, 2024

ctowns Dec 20, 2024

HunsupJung Dec 24, 2024

ctowns Dec 20, 2024

HunsupJung Dec 24, 2024

HunsupJung Dec 24, 2024

ctowns Dec 20, 2024

HunsupJung Dec 24, 2024

ctowns left a comment

Kwang-Hui commented Dec 23, 2024

Update matter-lock driver for U-tec #1812

Are you sure you want to change the base?

Update matter-lock driver for U-tec #1812

Conversation

HunsupJung commented Dec 10, 2024 • edited Loading

Type of Change

Checklist

Description of Change

Summary of Completed Tests

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 • edited Loading

github-actions bot commented Dec 10, 2024 • edited Loading

Test Results

nickolas-deboom commented Dec 12, 2024 • edited Loading

HunsupJung commented Dec 16, 2024

nickolas-deboom commented Dec 17, 2024

ctowns Dec 20, 2024

Choose a reason for hiding this comment

HunsupJung Dec 24, 2024

Choose a reason for hiding this comment

ctowns Dec 20, 2024

Choose a reason for hiding this comment

HunsupJung Dec 24, 2024

Choose a reason for hiding this comment

HunsupJung Dec 24, 2024

Choose a reason for hiding this comment

ctowns Dec 20, 2024

Choose a reason for hiding this comment

HunsupJung Dec 24, 2024

Choose a reason for hiding this comment

ctowns left a comment

Choose a reason for hiding this comment

Kwang-Hui commented Dec 23, 2024

HunsupJung commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

nickolas-deboom commented Dec 12, 2024 •

edited

Loading