Migrate Window Covering handling to Matter Switch #2714
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
|
matter-switch_coverage.xml
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 91917f1 |
| @@ -0,0 +1,20 @@ | |||
| name: window-covering | |||
There was a problem hiding this comment.
If we're moving the window logic into switch, let's make this modular, no?
There was a problem hiding this comment.
I would definitely like to, but might not work given that window coverings can be created as child devices
There was a problem hiding this comment.
Modular profiles can be used on child devices. It just adds some complexity in that it has to first be created before being updated. So the create event will not be able to include the modular metadata, but a following update will work as expected.
There was a problem hiding this comment.
Ok nice, I implemented modular profiles for parent and child window coverings in the previous 2 commits
There was a problem hiding this comment.
Note that I left a few static profiles because there are existing fingerprints pointing to them. Devices will profile switch to the modular profile though
There was a problem hiding this comment.
Updated fingerprints in latest commit
| device:emit_event(capabilities.windowShadePreset.position(preset_position, {visibility = {displayed = false}})) | ||
| device:set_field(closure_fields.PRESET_LEVEL_KEY, preset_position, {persist = true}) | ||
| end | ||
| device:subscribe() |
There was a problem hiding this comment.
We should probably use the extended subscribe logic, right? Since it is possible this will be a part of a parent/child device?
There was a problem hiding this comment.
Yes definitely. I'll update this
There was a problem hiding this comment.
Done in latest. As discussed, we will need to add a generic refresh test case and come up with a solution for that
There was a problem hiding this comment.
as a POC I made a fix in the latest commit, but we can discuss if that's the solution we want to go with or not (and let's make it a separate PR ofc)
| -- These should only ever be nil once (and at the same time) for already-installed devices | ||
| -- It can be removed after migration is complete |
There was a problem hiding this comment.
We don't need logic related to this, correct? There is no migration for devices who will now be using the switch driver?
There was a problem hiding this comment.
I kept this because I wasn't sure if the comment is accurate or not, but I will check
There was a problem hiding this comment.
Similar to the discussion from #2723, I think we will want to emit these capabilities once per device, which the current check should be doing. So I just removed this comment.
| 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
| local device_info_copy = utils.deep_copy(aqara_mock_device.raw_st_data) | ||
| device_info_copy.profile.id = "3-button-battery-temperature-humidity" | ||
| local device_info_json = dkjson.encode(device_info_copy) | ||
| test.socket.device_lifecycle:__queue_receive({ aqara_mock_device.id, "infoChanged", device_info_json }) |
There was a problem hiding this comment.
We might want to keep logic testing this infoChanged event, no?
There was a problem hiding this comment.
I'm pretty sure this wasn't doing anything, since the profile didn't change. So all this was doing was testing that infoChanged causes a new subscribe and configure_buttons call, which didn't seem very necessary...
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
from
29fe667 to
81c998e
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
235859c to
2ca640d
Compare
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
from
71f1947 to
509df2c
Compare
use modular profile for child devices and fix onboarding flow
nickolas-deboom
force-pushed
the
migrate-window-covering-driver-to-matter-switch
branch
from
ef48a90 to
475172b
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.