Matter Switch move button and switch initialization to doConfigure #2041

nickolas-deboom · 2025-04-01T18:04:15Z

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

This change moves the initialization logic for buttons and switches to do_configure. This consolidates all of the profile selection logic to be within doConfigure and allows the removal of logic gates from device_init that were there to ensure init code only ran one time.

Also added is a new function that runs at init that can rename or delete persisted fields on the device. The original __component_to_endpoint_map field can now be utilized by buttons and other devices because initialize_buttons_and_switches is ensured to only run one time, meaning that old MCD switch devices will not be affected. Also, the __switch_intialized field can now be deleted from devices.

Summary of Completed Tests

Tested with various matter buttons and lights to ensure no change in behavior.

github-actions · 2025-04-01T18:05:37Z

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

github-actions · 2025-04-01T18:06:27Z

Test Results

66 files 422 suites 0s ⏱️
2 171 tests 2 171 ✅ 0 💤 0 ❌
3 694 runs 3 694 ✅ 0 💤 0 ❌

Results for commit 7b5daf1.

♻️ This comment has been updated with latest results.

github-actions · 2025-04-01T18:06:29Z

File	Coverage
All files	`91%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/init.lua	`92%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/aqara-cube/init.lua	`96%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/embedded-cluster-utils.lua	`38%`	❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/eve-energy/init.lua	`91%`	✅

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 7b5daf1

hcarter-775 · 2025-04-02T17:51:04Z

I found this line: -- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP_BUTTON field
Can you update it to the correct field name?

hcarter-775 · 2025-04-02T17:53:52Z

drivers/SmartThings/matter-switch/src/init.lua

@@ -472,6 +467,17 @@ local function endpoint_to_component(device, ep)
  return "main"
 end

+local function check_field_name_updates(device)


this is a very critical function. It definitely looks fine to me, but it may be best to make this functionality as explicit as possible. I think being able to reason about this very clearly is important.

Do you think there is any risk that the command to copy the old field to the new field could fail, and then the next line would set the old field to nil? I kind of doubt that it could happen but I suppose it's possible

I suppose maybe that could happen? but a driver crash wouldn't cause that, so like lua itself and the lua runtime would have to fail. That's not something we're really capable of worrying about I don't think

Yeah, you're probably right.

hcarter-775 · 2025-04-02T18:00:30Z

drivers/SmartThings/matter-switch/src/init.lua

+    if device:get_field(IS_PARENT_CHILD_DEVICE) then
+      device:set_find_child(find_child)
+    end
+    if device:get_field(BUTTON_DEVICE_PROFILED) then


since we've moved this logic, I don't think we need the check to include a field anymore since we're not worried about race conditions? We could have the initialize_buttons_and_switches function return whether or not a profile was found mapped to. What do you think?

Yeah good point, and I like that idea

After trying this out, it felt a bit messy trying to get the profile name from try_build_button_component_map back to doConfigure. It feels easier to just use the field, but now since we only need it a single time we can set it to nil after using it. Do you agree with this approach?

Here's what I'm imagining.

In the do_configure handling:

local profile_found = initialize_buttons_and_switches(driver, device, main_endpoint) if profile_found then return end

Then, in initialize_buttons_and_switches, we could do something like:

local function initialize_buttons_and_switches(driver, device, main_endpoint) ... if #button_eps > 0 then build_button_profile(device, main_endpoint, #button_eps) configure_buttons(device) return true end ... if num_switch_server_eps > 0 and detect_matter_thing(device) then handle_light_switch_with_onOff_server_clusters(device, main_endpoint, num_switch_server_eps) return true end

the button path is actually identical per the current logic, and I added another check for the num_switch_server_eps > 0 case that I think was missed before.

Ah okay, that approach makes sense to me now. I implemented this in the latest commit!

ctowns · 2025-04-03T16:02:15Z

drivers/SmartThings/matter-switch/src/init.lua

+    -- initialize the main device card with buttons if applicable, and create child devices as needed for multi-switch devices.
+    initialize_buttons_and_switches(driver, device, main_endpoint)
+    if device:get_field(IS_PARENT_CHILD_DEVICE) then
+      device:set_find_child(find_child)


device:set_find_child(find_child) will still need to be set on init as it does not persist and should be called each time the driver restarts.

As a good test case for these changes, we should test various device types and ensure that they continue to work after a driver restart. Onboarding a parent-child device and then confirming that the child devices continue to work as expected after a driver restart would be a good test case!

Thank you, that's a great call out.

I updated this in the latest commit

ctowns · 2025-04-03T16:05:31Z

drivers/SmartThings/matter-switch/src/init.lua

-- driver after it was merged into the matter-switch driver. Note that devices
-- containing both button endpoints and switch endpoints will use this field
-- rather than COMPONENT_TO_ENDPOINT_MAP.
-local COMPONENT_TO_ENDPOINT_MAP_BUTTON = "__component_to_endpoint_map_button"


What as the original intent behind using a different field specifically for the button MCD devices? Was it because of the check in init that essentially bypassed MCD devices? That sounds right to me but I just want to make sure there wasn't another intent

Yes that's right. We didn't want to mess with the original field because there are probably still old switch MCD devices out there that we didn't want to re-configure as parent-child. But if the init logic is in doConfigure then we wouldn't need to worry about that.

ctowns · 2025-04-03T16:11:39Z

drivers/SmartThings/matter-switch/src/init.lua

@@ -760,6 +722,47 @@ local function device_init(driver, device)
  device:subscribe()
 end

+local function do_configure(driver, device)


Is there a reason to put this in doConfigure instead of added? I think that we have effectively treated these functions as nearly interchangable "one-time configuration" functions, but it may be worth setting a precedent with this change since this is our biggest and most used driver. Any thoughts on which lifecycle event we should use for this?

There are some calls in device_init that are required to run before initialize_buttons_and_switches (device:set_component_to_endpoint_fn(component_to_endpoint) for example), so I think doConfigure is the best place for this code since it will run after device_init (while added precedes device_init)

Further, since doConfigure is tied to whether or not the provisioning state has been changed to provisioned while added is also related to a driver change, doConfigure is the more appropriate place for a "true" one time handling like device profiling.

ctowns · 2025-04-03T16:14:56Z

drivers/SmartThings/matter-switch/src/init.lua

+  if not detect_bridge(device) then
+    local main_endpoint = find_default_endpoint(device)
+    -- initialize the main device card with buttons if applicable, and create child devices as needed for multi-switch devices.
+    initialize_buttons_and_switches(driver, device, main_endpoint)


Also since doConfigure could be called multiple times, we want to make sure functions like this can be run multiple times without issue in the same lifecycle. I don't suspect any issue from what I see but just wanted to call it out.

There should be no issue but I do see your concern. However, from my understanding doConfigure would only be called multiple times if the device is not set to the provisioned state, and it that case it seems that it would be a good idea to re-run these init functions.

hcarter-775 · 2025-04-03T19:53:57Z

drivers/SmartThings/matter-switch/src/init.lua

@@ -286,8 +286,6 @@ local HELD_THRESHOLD = 1
 -- this is the number of buttons for which we have a static profile already made
 local STATIC_BUTTON_PROFILE_SUPPORTED = {1, 2, 3, 4, 5, 6, 7, 8}

-local BUTTON_DEVICE_PROFILED = "__button_device_profiled"


we should also set this field to nil since we have that handy function to remove these en masse

This field was not persisted so we should be good there 👍

nickolas-deboom · 2025-04-03T20:08:05Z

I found this line: -- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP_BUTTON field Can you update it to the correct field name?

Good catch! Fixed in latest commit

This change moves the initialization logic for buttons and switches to doConfigure. This keeps all of the profile selection logic all within doConfigure and allows the removal of logic gates from device_init that were there to ensure init code only ran one time. Also added is a new function that runs at init that can rename or delete persisted fields on the device.

nickolas-deboom requested review from ctowns, z-michel and hcarter-775 April 1, 2025 18:04

nickolas-deboom force-pushed the matter-switch-update-field-names branch from 951333e to d2843e6 Compare April 1, 2025 19:51

hcarter-775 reviewed Apr 2, 2025

View reviewed changes

ctowns reviewed Apr 3, 2025

View reviewed changes

hcarter-775 reviewed Apr 3, 2025

View reviewed changes

nickolas-deboom force-pushed the matter-switch-update-field-names branch 5 times, most recently from 92942cb to 2879761 Compare April 14, 2025 17:02

nickolas-deboom force-pushed the matter-switch-update-field-names branch from 2879761 to 7b5daf1 Compare April 15, 2025 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matter Switch move button and switch initialization to doConfigure #2041

Matter Switch move button and switch initialization to doConfigure #2041

nickolas-deboom commented Apr 1, 2025 •

edited

Loading

github-actions bot commented Apr 1, 2025

github-actions bot commented Apr 1, 2025 •

edited

Loading

github-actions bot commented Apr 1, 2025 •

edited

Loading

hcarter-775 commented Apr 2, 2025

hcarter-775 Apr 2, 2025

nickolas-deboom Apr 3, 2025

hcarter-775 Apr 3, 2025

nickolas-deboom Apr 3, 2025

hcarter-775 Apr 2, 2025 •

edited

Loading

nickolas-deboom Apr 2, 2025

nickolas-deboom Apr 3, 2025

hcarter-775 Apr 3, 2025

nickolas-deboom Apr 3, 2025

ctowns Apr 3, 2025 •

edited

Loading

nickolas-deboom Apr 3, 2025

nickolas-deboom Apr 3, 2025

ctowns Apr 3, 2025

nickolas-deboom Apr 3, 2025

ctowns Apr 3, 2025

nickolas-deboom Apr 3, 2025

hcarter-775 Apr 3, 2025

ctowns Apr 3, 2025

nickolas-deboom Apr 3, 2025 •

edited

Loading

hcarter-775 Apr 3, 2025

nickolas-deboom Apr 3, 2025 •

edited

Loading

nickolas-deboom commented Apr 3, 2025

Matter Switch move button and switch initialization to doConfigure #2041

Are you sure you want to change the base?

Matter Switch move button and switch initialization to doConfigure #2041

Conversation

nickolas-deboom commented Apr 1, 2025 • edited Loading

Type of Change

Checklist

Description of Change

Summary of Completed Tests

github-actions bot commented Apr 1, 2025

github-actions bot commented Apr 1, 2025 • edited Loading

Test Results

github-actions bot commented Apr 1, 2025 • edited Loading

hcarter-775 commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcarter-775 Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctowns Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickolas-deboom Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickolas-deboom Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

nickolas-deboom commented Apr 3, 2025

nickolas-deboom commented Apr 1, 2025 •

edited

Loading

github-actions bot commented Apr 1, 2025 •

edited

Loading

github-actions bot commented Apr 1, 2025 •

edited

Loading

hcarter-775 Apr 2, 2025 •

edited

Loading

ctowns Apr 3, 2025 •

edited

Loading

nickolas-deboom Apr 3, 2025 •

edited

Loading

nickolas-deboom Apr 3, 2025 •

edited

Loading