SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
269 stars 462 forks source link

Add Dynamic Battery Support for Base-Lock Profiled Door Lock Devices #1660

Closed hcarter-775 closed 1 month ago

hcarter-775 commented 1 month ago

Check all that apply

Type of Change

Checklist

Description of Change

Jira ticket showing mis-profiling of a bridged door lock: https://smartthings.atlassian.net/browse/MTR-821 Further, there is a general ticket on adding dynamic profiling to this driver: https://smartthings.atlassian.net/browse/CHAD-13948

This logic will permit dynamic profiling on the base-lock profile depending on the Power Source cluster, and will ignore devices that do not auto-profile as base-lock.

Summary of Completed Tests

Unit tests Bridged Switchbot device - Passed VDA Door Lock - Passed VDA Door Lock (Lock Codes) - Passed Yale Assure Matter Lock - Passed

github-actions[bot] commented 1 month ago

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions[bot] commented 1 month ago

Channel deleted.

github-actions[bot] commented 1 month ago

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

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against e0179b24d3abe851cf16a81a25de581e48ce2829

github-actions[bot] commented 1 month ago

Test Results

   64 files    400 suites   0s :stopwatch: 1 988 tests 1 988 :white_check_mark: 0 :zzz: 0 :x: 3 429 runs  3 429 :white_check_mark: 0 :zzz: 0 :x:

Results for commit e0179b24.

:recycle: This comment has been updated with latest results.

nickolas-deboom commented 1 month ago

This looks good to me, but I was just thinking that one potential drawback with this approach is that each PR for new WWST device fingerprints would also have to include an update to the list of fingerprinted devices in do_configure (the one I mentioned in my other comment for example). Maybe that is no big deal but I thought I'd mention it

hcarter-775 commented 1 month ago

Yeah @nickolas-deboom I agree, that's definitely a downside. Unfortunately, there's not really any better method in place of solving this issue atm. I figured we would inform Alissa of this in the next Risk Analysis meeting and then you and I can help monitor this as well until we put a better solution in place.

hcarter-775 commented 1 month ago

@nickolas-deboom I also think this example might get some more proactive desire to fix this situation in a larger way

hcarter-775 commented 1 month ago

And I just thought of this, but another point of note here is that there is a new and improved matter Lock driver coming soon, so this issue may be extremely temporary.

nickolas-deboom commented 1 month ago

Those are good points, and I can't think of a better way to handle this either, so I think this is a good solution for now, especially given it might be temporary since there are improvements coming to this driver as you mentioned.

ctowns commented 1 month ago

Also before merging, this change should be tested with both the new bridged the device as well as regression tested against an existing lock.

hcarter-775 commented 1 month ago

Yes, I agreee Cooper. I will add the dynamic battery profiling logic to those places as well. And I have already reached out on the affected ticket to get some testing with the new device.