Repository navigation
Support multi-endpoint window covering - #3191
Open
HunsupJung wants to merge 5 commits into
Open
HunsupJung wants to merge 5 commits into
HunsupJung wants to merge 5 commits into
Conversation
|
Invitation URL: |
Test Results 73 files 545 suites 0s ⏱️ Results for commit c3849ad. ♻️ This comment has been updated with latest results. |
|
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against c3849ad |
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
2 times, most recently
from
August 21, 2026 07:20
e7b42a3 to
04ec853
Compare
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
August 28, 2026 10:30
04ec853 to
e2d4adc
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
September 7, 2026 06:34
e2d4adc to
fa58718
Compare
Contributor
|
Hi @HunsupJung , Before reviewing, can you please provide:
Thanks! |
hcarter-775
reviewed
Oct 12, 2026
Comment on lines
+278
to
280
| if #wc_cluster_eps > 1 then | ||
| create_or_update_multi_devices(driver, device, wc_cluster_eps) | ||
| else |
Contributor
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.
Collaborator
Author
There was a problem hiding this comment.
So for Endpoint 1, you’re suggesting we go with a battery check — is that right?
Collaborator
Author
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. |
Collaborator
Author
|
@hcarter-775 |
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
October 19, 2026 06:17
67e3370 to
87da271
Compare
ctowns
requested changes
Oct 19, 2026
ctowns
left a comment
ctowns
left a comment
Contributor
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", |
Contributor
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.
Collaborator
Author
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?
Contributor
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.
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
October 20, 2026 06:59
b2bbb23 to
c81a781
Compare
Signed-off-by: Hunsup Jung <hunsup.jung@samsung.com>
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
October 20, 2026 07:14
e2a29e9 to
b971722
Compare
Collaborator
Author
|
@ctowns |
ctowns
requested changes
Oct 20, 2026
ctowns
left a comment
ctowns
left a comment
Contributor
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", |
Contributor
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) |
Contributor
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( |
Contributor
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) |
Contributor
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
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
October 22, 2026 04:51
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>
HunsupJung
force-pushed
the
feature/multi-endpoint-window-covering
branch
from
October 22, 2026 04:54
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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