Migrate Window Covering handling to Matter Switch #2714
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 485 suites 0s ⏱️ Results for commit a5d0ea1. ♻️ This comment has been updated with latest results. |
|
matter-switch_coverage.xml
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against a5d0ea1 |
drivers/SmartThings/matter-switch/src/sub_drivers/closures/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/closures/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/test/test_matter_multi_button.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/test/test_aqara_climate_sensor_w100.lua
Outdated
Show resolved
Hide resolved
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
from
38edd0c to
29b30bb
Compare
nickolas-deboom
changed the base branch from
main
to
matter-switch-improve-battery-profiling
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
4 times, most recently
from
ef48a90 to
475172b
Compare
| local window_covering_ep_ids = switch_utils.get_endpoints_by_device_type(device:get_parent_device(), fields.DEVICE_TYPE_ID.WINDOW_COVERING) | ||
| if #window_covering_ep_ids > 0 then | ||
| local default_endpoint_id = switch_utils.find_default_endpoint(device:get_parent_device()) | ||
| child_cfg.create_or_update_child_devices(driver, device:get_parent_device(), window_covering_ep_ids, default_endpoint_id, window_covering_cfg.assign_profile_for_window_covering_ep) | ||
| end |
There was a problem hiding this comment.
can't this be done directly in match_profile?
There was a problem hiding this comment.
or rather, isn't it kind of already defined there?
There was a problem hiding this comment.
This is meant to run create_or_update_child_devices a second time to update the child devices with optional capabilities since aren't included by try_create_device, since match_profile won't be called a second time. Did you have something else in mind for updating the child profiles with the optional capabilities metadata?
There was a problem hiding this comment.
Ah, I see. Yeah this makes sense then. One other small nit that I'll mention here, now that we're pulling create_or_update_child_devices() into other places, I kinda regret putting it into its own ChildConfiguration table. I think it would be more natural to just put that function in DeviceConfiguration, so we don't have to pull this table just for the single function.
There was a problem hiding this comment.
Thinking some more about this, I'm not this will run as you're expecting, because the child device won't necessarily have experienced an info_changed event on create, it will have just been created so it will do the added, init, doConfigure flow. Also, this logic would have the create_or_update logic occur on every infoChanged event, irrespective of whether that should be happening.
There was a problem hiding this comment.
I'm a little bit confused- why would the child device not necessarily having an info_changed event matter exactly? This is implemented in doConfigure, which a child device should always go through, right?
(Did I used to have this logic in infoChanged maybe? I can't remember 😅)
nickolas-deboom
force-pushed
the
matter-switch-improve-battery-profiling
branch
2 times, most recently
from
14b331e to
303b21a
Compare
| function ClosureLifecycleHandlers.device_added(driver, device) | ||
| device:emit_event(capabilities.windowShade.supportedWindowShadeCommands({"open", "close", "pause"}, {visibility = {displayed = false}})) | ||
| device:set_field(closure_fields.REVERSE_POLARITY, false, { persist = true }) | ||
| switch_utils.handle_electrical_sensor_info(device) |
There was a problem hiding this comment.
I don't think a window covering needs to handle this at the moment.
There was a problem hiding this comment.
It's needed to set fields.profiling_data.POWER_TOPOLOGY so that match_profile can proceed. We could do something like
local electrical_sensor_eps = utils.get_endpoints_by_device_type(device, fields.DEVICE_TYPE_ID.ELECTRICAL_SENSOR, { with_info = true })
if #electrical_sensor_eps == 0 then
device:set_field(fields.profiling_data.POWER_TOPOLOGY, false, {persist=true})
end
in device_init if you prefer? Similar to setting profiling_data.BATTERY _SUPPORT
There was a problem hiding this comment.
ah, I see! My bad, forgot about that. Yeah this is fine then. It is a little confusing though... maybe it would be better to port this logic into an if block of the main driver, rather than pulling the main driver logic into a subdriver? That, or maybe we should move this function to doConfigure so we don't have to worry about overrides at all?
Thinking a little more, we could maybe even do both if that makes sense... I know this is a 1-1 port from the old window driver but do these steps really have to occur in device_added rather than init for example? Of course, with init it would require a little extra logic to avoid extra calls but that is relatively common and may be worth it here.
There was a problem hiding this comment.
I tried a few different things out, but realized that we need to override the added handler in the subdriver anyways, since the added handler in the main driver calls the main driver's device_init, which we want to avoid. I do think it makes sense to move the capability initialization to device_init to be consistent with the init's for the preset capability, which I did in the last commit.
I could move the remaining code out of the added handler, but since we have to override it anyway we might as well make use of it I guess? Lemme know what you think about that, I'm good either way
nickolas-deboom
force-pushed
the
matter-switch-improve-battery-profiling
branch
3 times, most recently
from
316d0e0 to
b17cb98
Compare
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
from
91917f1 to
bcbf2bb
Compare
To support devices using the WindowCovering cluster in addition to light/switch/button endpoints, this moves the handling for window coverings into a subdriver within the switch driver. Note that the subdriver is called `closures`, since it will be expanded to cover more Matter 1.5 closure types.
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
from
bcbf2bb to
ca9c2e7
Compare
| @@ -0,0 +1,68 @@ | |||
| -- Copyright © 2026 SmartThings, Inc. | |||
There was a problem hiding this comment.
nit: For these "generic" sub-drivers, I think the new directory name meta should be the generic words "utils" and "handlers" instead of names like "closure_handlers" and "closure_utils" for example.
The reason is that the pathing should already be unique, since these are in a subdriver, so the require would look like require "sub_drivers.closures.utils.fields". Therefore, no information is really lost (the subdriver name is still visible), and we don't have to worry about any other paths in the driver matching this.
| if device.network_type == device_lib.NETWORK_TYPE_CHILD then | ||
| device = device:get_parent_device() | ||
| end | ||
| if #switch_utils.get_endpoints_by_device_type(device, fields.DEVICE_TYPE_ID.WINDOW_COVERING) > 0 then | ||
| return true, require("sub_drivers.closures") | ||
| end |
There was a problem hiding this comment.
At the moment this is not really an issue I don't think, but what about the case where there is >1 child device and one of them is a window covering? The current logic might overwrite the lifecycle event and capability event handling for the other device, non window covering device.
There was a problem hiding this comment.
Specifically, I think we might want to do a little extra gating based on opts.dispatcher_class.
As in, if the dispatcher class name is MatterMessageDispatcher, we'd likely want to follow something similar to this logic, since all matter messages will come through the parent driver. Therefore, no EDGE_CHILD check would be necessary.
And then in the case of a DeviceLifecycleDispatcher or a CapabilityCommandDispatcher name, it might be safer to do a check based on the particular device's supported capabilities, agnostic of whether it is a parent or a child.
| @@ -0,0 +1,1126 @@ | |||
| -- Copyright © 2026 SmartThings, Inc. | |||
There was a problem hiding this comment.
Can we add some tests for the case that there is a onOff switch as a parent device and a window covering as a child device?
| local FanDeviceConfiguration = {} | ||
| local WindowCoveringDeviceConfiguration = {} | ||
|
|
||
| function ChildConfiguration.create_or_update_child_devices(driver, device, server_cluster_ep_ids, default_endpoint_id, assign_profile_fn) |
There was a problem hiding this comment.
I know I introduced this type so this is my bad... but now that we're returning ChildConfiguration to use elsewhere, I think we should just remove this and use the standard DeviceConfiguration table to house this function.
2 participants
To support devices using the WindowCovering cluster in addition to light/switch/button endpoints, this moves the handling for window coverings into a subdriver within the switch driver. Note that the subdriver is called
closuressince it will be expanded to cover more Matter 1.5 closure types.