Migrate Window Covering handling to Matter Switch #2714
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 484 suites 0s ⏱️ Results for commit ca9c2e7. ♻️ 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 ca9c2e7 |
drivers/SmartThings/matter-switch/src/sub_drivers/closures/init.lua
Outdated
Show resolved
Hide resolved
| device:set_field(closure_fields.REVERSE_POLARITY, false, { persist = true }) | ||
| end | ||
|
|
||
| function ClosureLifecycleHandlers.info_changed(driver, device, event, args) |
There was a problem hiding this comment.
The software version update logic has been lost here, as well as the expanded subscribe logic. I am unsure how much we want to repeat this kind of logic in a subdriver, or if we want to conditionally move the preference logic into the main driver.
There was a problem hiding this comment.
I moved the preference logic to the main driver in the latest commit.
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.
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.
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
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.