Matter Switch: update subscribe logic #2667
Conversation
hcarter-775
requested review from
ctowns,
gharveymn,
greens and
nickolas-deboom
|
Invitation URL: |
hcarter-775
changed the title
Test Results 71 files 480 suites 0s ⏱️ Results for commit 3de35fd. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 3de35fd |
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
Some of the test changes need a bit of explanation, but otherwise LGTM.
| -- @param matter_elements_seen table a list of attribute and event ids that have been added already | ||
| --- @param subscribed_attributes table key-value pairs mapping capability ids to subscribed attributes | ||
| --- @param subscribed_events table key-value pairs mapping capability ids to subscribed events | ||
| function utils.populate_subscribe_request_for_device(checked_device, subscribe_request, capabilities_seen, attributes_seen, events_seen, subscribed_attributes, subscribed_events) |
drivers/SmartThings/matter-switch/src/test/test_matter_multi_button_switch_mcd.lua
Outdated
Show resolved
Hide resolved
| test.socket.device_lifecycle:__queue_receive(unsup_mock_device:generate_info_changed({ profile = updated_device_profile })) | ||
|
|
||
| local cluster_subscribe_list = { | ||
| -- clusters.OnOff.attributes.OnOff, |
There was a problem hiding this comment.
Why is this no longer expected?
There was a problem hiding this comment.
This is no longer expected because now, the subscription is based on the capability->attribute/event maps for devices that currently exist.
Before, we were doing a device type check on all endpoints, which was catching that this device would need to subscribe to onOff eventually, when its child device was created. Now that logic has been pushed to when the child device is actually created (which is not now). Thus the attribute is no longer subscribed to at this point
There was a problem hiding this comment.
Also, I reworked this file a bit to get the structure tested more thoroughly by only initializing the mock_child in the tests where it's directly needed. This way, I showed that before the device is created, only some of the subscriptions are handled, but after the device is initialized, the more complete subscribe table is used.
drivers/SmartThings/matter-switch/src/sub_drivers/camera/camera_utils/utils.lua
Show resolved
Hide resolved
hcarter-775
force-pushed
the
update/matter-switch-subscribe-logic-no-fields
branch
from
ddaf944 to
1972787
Compare
hcarter-775
force-pushed
the
update/matter-switch-subscribe-logic-no-fields
branch
3 times, most recently
from
8f71143 to
79cea27
Compare
hcarter-775
force-pushed
the
update/matter-switch-subscribe-logic-no-fields
branch
from
79cea27 to
3de35fd
Compare
| }, | ||
| } | ||
| function() | ||
| test.mock_device.add_test_device(mock_child) |
There was a problem hiding this comment.
I'm a little confused on how these tests work. This test for instance looks like it initially subscribed to CLUSTER_SUBSCRIBE_LIST_NO_CHILD, and then wait to add the child device until right here. When do the additional attributes from CLUSTER_SUBSCRIBE_LIST_WITH_CHILD get subscribed to?
There was a problem hiding this comment.
for ease, the main test path doesn't actually test the child subscription logic. That path is tested in the particular test that uses CLUSTER_SUBSCRIBE_LIST_WITH_CHILD
| end | ||
| end | ||
| end | ||
| local im = require "st.matter.interaction_model" |
There was a problem hiding this comment.
Could we replace the block below with a call to the subscribe function implemented in switch_utils?
There was a problem hiding this comment.
the issue there is how do we add the subscription to clusters.CameraAvStreamManagement.attributes.AttributeList.ID if we do it like that? I couldn't really find a way around that at this time, so I left it like this
Description of Change
Override generic matter switch handling of device:subscribe() to more properly and generically handle child devices.
Summary of Completed Tests
Tested multiple parent/child devices where the children had unique subscriptions from the parent device.