Matter Switch move button and switch initialization to doConfigure #2041
Conversation
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7b62645 |
nickolas-deboom
force-pushed
the
matter-switch-update-field-names
branch
from
951333e to
d2843e6
Compare
|
I found this line: -- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP_BUTTON field |
| -- 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) |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Thank you, that's a great call out.
There was a problem hiding this comment.
I updated this in the latest commit
| @@ -760,6 +722,47 @@ local function device_init(driver, device) | |||
| device:subscribe() | |||
| end | |||
|
|
|||
| local function do_configure(driver, device) | |||
There was a problem hiding this comment.
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 was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Adding to this: another reason to put this handling in doConfigure is because it truly is a one time configuration function. added for example will be re-run in the standard case of a hub failover, since the device will be put in a new driver.
| 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) |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
Good catch! Fixed in latest commit |
nickolas-deboom
force-pushed
the
matter-switch-update-field-names
branch
6 times, most recently
from
2879761 to
7b5daf1
Compare
| @@ -473,6 +469,17 @@ local function endpoint_to_component(device, ep) | |||
| return "main" | |||
| end | |||
|
|
|||
| local function check_field_name_updates(device) | |||
| for _, field in ipairs(updated_fields) do | |||
| if device:get_field(field.field_name) then | |||
There was a problem hiding this comment.
lets call field_name current_field_name for clarity. also, I don't think we need the if field.updated_field_name ~= nil, since this is already handled in the set_field definition in lua libs. We also remove the data either way, so it doesn't change the handling if we set data to a nil field that doesn't exist.
There was a problem hiding this comment.
I updated the name, but think we should leave the check because I tried it without and get an error if the field is nil:
Received unexpected error: ...eboom/Documents/dev/scripting-engine/lua_libs/cosock.lua:252: ...ocuments/dev/scripting-engine/lua_libs/st/dispatcher.lua:270: Error encountered while processing event for <MatterDevice: 00000000-1111-2222-3333-000000000001 [nil] (Matter Switch)>:
arg1: init
"./init.lua:475: Field key cannot be nil"
stack traceback:
[C]: in function 'error'
...om/Documents/dev/scripting-engine/lua_libs/st/thread.lua:88: in function <...om/Documents/dev/scripting-engine/lua_libs/st/thread.lua:55>
stack traceback:
[C]: in function 'error'
...eboom/Documents/dev/scripting-engine/lua_libs/cosock.lua:252: in upvalue 'step_thread'
...eboom/Documents/dev/scripting-engine/lua_libs/cosock.lua:360: in function 'cosock.run'
...om/Documents/dev/scripting-engine/lua_libs/st/driver.lua:1144: in function 'st.driver.run'
./init.lua:1654: in main chunk
[C]: in function 'require'
[C]: in function 'xpcall'
.../dev/scripting-engine/lua_libs/integration_test/init.lua:242: in upvalue 'run_configured_test'
.../dev/scripting-engine/lua_libs/integration_test/init.lua:376: in function 'integration_test.run_registered_tests'
test/test_multi_switch_parent_child_plugs.lua:357: in main chunk
[C]: in ?
There was a problem hiding this comment.
ah I see, they mark it as an error. That's fine then, thanks for checking 👍
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
force-pushed
the
matter-switch-update-field-names
branch
from
8482368 to
7b62645
Compare
Type of Change
Checklist
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 fromdevice_initthat 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_mapfield can now be utilized by buttons and other devices becauseinitialize_buttons_and_switchesis ensured to only run one time, meaning that old MCD switch devices will not be affected. Also, the__switch_intializedfield can now be deleted from devices.Summary of Completed Tests
Tested with various matter buttons and lights to ensure no change in behavior.