Repository navigation
Support multi-endpoint window covering - #3191
HunsupJung wants to merge 7 commits into
Conversation
|
Invitation URL: |
Test Results 73 files 546 suites 0s ⏱️ Results for commit 75181de. ♻️ This comment has been updated with latest results. |
|
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 75181de |
e7b42a3 to
04ec853
Compare
04ec853 to
e2d4adc
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
e2d4adc to
fa58718
Compare
|
Hi @HunsupJung , Before reviewing, can you please provide:
Thanks! |
| if #wc_cluster_eps > 1 then | ||
| create_or_update_multi_devices(driver, device, wc_cluster_eps) | ||
| else |
There was a problem hiding this comment.
This section will make a multi-ep device skip the battery checks in the main do_configure pathing. I think we should ensure there is still always 1 battery check (as there is now), and that these should be integrated to make that check continue to always occur.
There was a problem hiding this comment.
So for Endpoint 1, you’re suggesting we go with a battery check — is that right?
By the way, could you share what the testing criteria should be? Since there’s no defined test checklist, it’s always a bit unclear what standard to go by when I share test results. |
|
@hcarter-775 |
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
67e3370 to
87da271
Compare
ctowns
left a comment
ctowns
left a comment
There was a problem hiding this comment.
Hi @HunsupJung !
I have left a couple comments below. Currently, I found a bug where the driver would not work correctly after a restart. Could you address the bug based on my comment below, and then complete testing again? Make sure you test that the device continues to work correctly after a driver restart (or a hub restart). Also, could you rebase these changes on the latest main branch? It looks like these are currently branching off an older commit, so we just want to make sure we are testing against the latest.
Could you add unit tests for this new multi-endpoint device in a new test file in the matter-window-covering folder? Thank you!
| driver:try_create_device({ | ||
| type = "EDGE_CHILD", | ||
| label = label_and_name, | ||
| profile = "window-covering", |
There was a problem hiding this comment.
Just confirming - will all the child endpoints always have the window-covering profile? For example, none of them support tilt, right?
In the future we might want to make this dynamic similar to how we handle child devices in the matter-switch driver, but if this is the only multi-endpoint device we know of for window coverings, we can leave it as is for now and plan to make this dynamic in a follow up.
There was a problem hiding this comment.
I don't know how the child lifecycle works, but from the logs it looks like do_configure gets called for each child once it’s created as an EDGE_CHILD. So it seems we don’t need to check the feature map here.
Could you take a look at how the lifecycle is currently set up and let me know what you think?
There was a problem hiding this comment.
Yes, child devices do get the do_configure event. And currently it looks like all child devices would go through the same match_profile as the parent device because of this.
When I look at the testing you provided in the linked confluence page, it looks like all these child devices do have tilt support.
b2bbb23 to
c81a781
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
e2a29e9 to
b971722
Compare
|
@ctowns |
ctowns
left a comment
ctowns
left a comment
There was a problem hiding this comment.
Thanks for adding the new tests @HunsupJung ! I have a few more comments and some potential bugs that need to be fixed. Also, could you update your device testing confluence page to include checking for the following cases?
- confirm that device continues to work after a driver/hub restart
- confirm that the idle state of the shades is correctly displayed in the app (I have suggested a fix for this below)
Also, I have added some suggestions for additional unit testing. Let me know if you have any questions!
| driver:try_create_device({ | ||
| type = "EDGE_CHILD", | ||
| label = label_and_name, | ||
| profile = "window-covering", |
There was a problem hiding this comment.
Yes, child devices do get the do_configure event. And currently it looks like all child devices would go through the same match_profile as the parent device because of this.
When I look at the testing you provided in the linked confluence page, it looks like all these child devices do have tilt support.
| end | ||
|
|
||
| local function create_or_update_multi_devices(driver, device, wc_cluster_eps) | ||
| local default_endpoint_id = find_default_endpoint(device) |
There was a problem hiding this comment.
nit: this should probably be find_default_endpoint(device, clusters.WindowCovering.ID) for consistency
|
|
||
| test.set_test_init_function(test_init) | ||
|
|
||
| test.register_coroutine_test( |
There was a problem hiding this comment.
Please add test coverage for capability commands such as open, close, setShadeLevel and others to confirm these are working as expected. These tests currently only cover attribute reports from the device.
| --- Tries to emit the idle window shade status if no target percentages are set. | ||
| --- @param device any a Matter device object | ||
| local function try_emit_idle_window_shade_status(device) | ||
| local function try_emit_idle_window_shade_status(device, ep) |
There was a problem hiding this comment.
I noticed in the test video that you shared that the window shade state sometimes appears to be wrong when idle (i.e. after the window is done closing, it might show "Open" in the app). This because we are always reading the get_idle_window_shade_status() from the parent device rather than using the child device when updating a child endpoint.
This could be fixed by adding something like this below:
if not device:get_field(TARGET_LIFT_PERCENT.STATE) and
not device:get_field(TARGET_TILT_PERCENT.STATE) then
--- gets child device if one exists for this endpoint
local status_device = ep and find_child(device, ep) or device
status_device:emit_event(get_idle_window_shade_status(status_device))
end
aebd822 to
a9596b4
Compare
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
Duplicate profile check: Passed - no duplicate profiles detected. |
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
a9596b4 to
418e8f4
Compare
|
zigbee-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-water-leak-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zwave-window-treatment_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
matter-lock_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
matter-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zwave-electric-meter_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zwave-garage-door-opener_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zwave-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-smoke-detector_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
matter-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zwave-virtual-momentary-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-humidity-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-valve_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-button_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
matter-thermostat_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
|
zigbee-thermostat_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
496f328 to
c1ef45c
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
c1ef45c to
05fa702
Compare
|
@ctowns |
ctowns
left a comment
ctowns
left a comment
There was a problem hiding this comment.
Thank you @HunsupJung for making those updates and providing some new testing! I have a few more comments but I think this is close!
| end | ||
|
|
||
| local function device_init(driver, device) | ||
| if device:get_field(IS_PARENT_CHILD_DEVICE) then |
There was a problem hiding this comment.
In the matter-switch driver we only call subscribe for the parent device in init and skip it for child devices to prevent us from sending duplicate and unneccessary subscribe requests for each child at startup. We should do the same here.
There was a problem hiding this comment.
I made the change as shown below. Let me know if anything else needs adjusting.
- device:subscribe()
+ if device.network_type ~= device_lib.NETWORK_TYPE_CHILD then
+ device:subscribe()
+ endThere was a problem hiding this comment.
This will always be the parent device, so we need to update this to account for the child device if one is being used for that endpoint:
local state_device = find_child(device, ib.endpoint_id) or device
local latest_lift_percentage = state_device:get_latest_state(
"main",
capabilities.windowShadeLevel.ID,
capabilities.windowShadeLevel.shadeLevel.NAME
)
The same change needs to be made for the target_position_lift_percent_100ths_handler function as well
There was a problem hiding this comment.
I have made the change.
| if not ib.data.value then return end | ||
| local global_op_status = ib.data.value & clusters.WindowCovering.types.OperationalStatus.GLOBAL | ||
|
|
||
| local reverse = device.preferences.reverse |
There was a problem hiding this comment.
This reverse is also based on the preference of the parent rather than the child. If we want to be able to configure the reverse preference on a per-child basis, we would need to do something like this:
local reverse = (find_child(device, ib.endpoint_id) or device).preferences.reverse
There was a problem hiding this comment.
I have made the change.
| --- If the ASSIGNED_CHILD_KEY field is populated for an endpoint, it should be | ||
| --- used as the key in the get_child_by_parent_assigned_key() function. This allows | ||
| --- multiple endpoints to associate with the same child device, though right now child | ||
| local ASSIGNED_CHILD_KEY = "__assigned_child_key" |
There was a problem hiding this comment.
Is this still needed? The comment mentions that this is only needed for cases where multiple endpoints associate with the same child. Currently, that functionality is not supported in this driver, and we do not set this field anywhere below
There was a problem hiding this comment.
It’s used in the find_child function. It doesn’t look like it needs to be changed.
a47ac9b to
ddbeecc
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
ddbeecc to
75181de
Compare
Type of Change
Checklist
Description of Change
https://smartthings.atlassian.net/wiki/spaces/STHK/pages/5632132591/Matter+Wintec+Blind+-+Support+Direction
Summary of Completed Tests
https://smartthings.atlassian.net/wiki/spaces/STHK/pages/5913215186/Matter+Wintec+Blind+-+Test+Result