Matter-Switch: Integrate matter-button driver #1423
Conversation
This PR addresses CHAD-13270 to integrate the functionality from matter-button into the matter-switch driver. Additionally, a 5 button profile was created to support the IKEA 5 button Tradfri remote.
|
Duplicate profile check: Passed - no duplicate profiles detected. |
nickolas-deboom
requested review from
tpmanley,
gharveymn,
Con-Tejus,
z-michel and
hcarter-775
|
Channel deleted. |
Test Results 60 files 380 suites 0s ⏱️ Results for commit 36a696d. ♻️ This comment has been updated with latest results. |
|
matter-button_coverage.xml
matter-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 36a696d |
nickolas-deboom
force-pushed
the
integrate-matter-switch-and-matter-button
branch
4 times, most recently
from
47b7caf
to
3c8a2f5
Compare
nickolas-deboom
force-pushed
the
integrate-matter-switch-and-matter-button
branch
from
3c8a2f5
to
06f7131
Compare
Revert earlier change that moved subscription event in the driver to before the initialize_switch is called.
This commit adds supports for subscribing to attributes supported by child devices.
Now that the fingerprints for button device types have been moved into the switch driver, they should be removed from the button driver so that new devices join profiles in the switch driver.
This commit ports the MCD functionality from the button driver into the switch driver. The multi-button profiles were brought over as well. Test case updates were also made and a new test case file was created to verify the functionality of MCD for a combination button/switch device.
nickolas-deboom
force-pushed
the
integrate-matter-switch-and-matter-button
branch
from
202e855
to
064d575
Compare
…m:SmartThingsCommunity/SmartThingsEdgeDrivers into integrate-matter-switch-and-matter-button
After merging main into this branch, device:subscribe() is now called two times and therefore a second expected subscription request needed to be added to the test cases brought over from the matter-button driver.
New profiles were created to support odd-numbered button devices being initialized as MCD. Buttons are no longer initialized as parent-child (unless the device contains an unsupported number of buttons, > 8).
Multi press was sending an extra capability event because SUPPORTS_MULTI_PRESS was not being set for multi component devices.
Fix the matter button and switch MCD unit tests, which were failing during the initialization due to previous changes.
|
@nickolas-deboom - I would encourage you to add a few additional folks like Steven or Z Varberg as reviewers as they might have additional context when trying to "depreciate a driver". |
| -- driver. All functionality and test cases present in this driver were carried over into matter-switch. Note that this | ||
| -- won't affect devices using the button driver unless they are re-onboarded, in which case they would onboard to the | ||
| -- switch driver, as all button fingerprints were removed from this driver and moved to the switch driver. | ||
|
|
There was a problem hiding this comment.
I would add additionally here that we are no longer accepting bug fixes, feature enhancements such as fingerprint adds to this driver.
Something along those lines to make it really clear - there are no planned changes to folks who might be reading this.
There was a problem hiding this comment.
Updated the comment as suggested!
| component_map["main"] = ep | ||
| end | ||
| component_map_used = true | ||
| else -- Create child devices for non-main endpoints |
| -- no device type that is not in the switch subset should be considered. | ||
| if (ON_OFF_SWITCH_ID <= dt.device_type_id and dt.device_type_id <= ON_OFF_COLOR_DIMMER_SWITCH_ID) then | ||
| id = math.max(id, dt.device_type_id) | ||
| if num_server_eps > 1 then |
There was a problem hiding this comment.
The assumptions that num_server_eps > 1 means that the device is parent-child is no longer true since a n-button device would have 2 server eps but be represented with an MCD profile. You can just exchange this for a boolean that is set to true when try_create_device is called in the conditional above
|
|
||
| device:set_field(SWITCH_INITIALIZED, true) | ||
|
|
||
| if #button_eps > 0 then |
There was a problem hiding this comment.
What if the device has a mixture of buttons and switch device types? For example, a button with an LED light on it? I think we need to determine how that kind of device type should be represented
There was a problem hiding this comment.
My thinking for this would be that if there are any button endpoints, the device will use an MCD configuration, and you can see here that a switch endpoint would use switch%d for its component name. I created an example profile of a 2 button, 1 switch device that would use this configuration. This line would append "-switch" to the profile name if there are switch endpoints in addition to button endpoints. If devices come out that need multiple switches or other configurations, we could update this profile selection logic and create new profiles for those devices, like we decided to do in the thermostat and sensor drivers. Do you think this approach would make sense?
| end | ||
|
|
||
| if new_profile then | ||
| if #switch_eps > 0 then |
There was a problem hiding this comment.
This would only work in the case that there is just one switch EP and it is an On/Off plug/switch, it doesn't account for more complex device types like color bulbs. This might be out of the scope of this PR, but we may just want to remove this part until we have a more comprehensive solution for these complex device types and just limit this PR to supporting the devices that are already supported by the matter-button driver.
There was a problem hiding this comment.
I saw this comment after the previous one, and I do think that would make sense. My only question is, I created test cases for this example 2-button 1-switch device and so if we remove this then those test cases obviously wouldn't work. Should we just go ahead and remove this for now, and also remove the test cases until we actually have a more complex device like this?
| -- do not have a generic fingerprint and will join as a matter-thing. However, we have seen some devices | ||
| -- claim to be Light Switch device types and still implement their clusters as server, so this is a | ||
| -- workaround for those devices. | ||
| if num_server_eps > 0 and detect_matter_thing(device) == true then |
There was a problem hiding this comment.
You'll what to make this num_switch_server_eps so that we are not counting button server eps also. This logic should only affect Light Switch device types as described in the comment above, so we only want to count server eps for the light switch types when determining if we need to go down this codepath.
| if device:supports_server_cluster(clusters.OnOff.ID, ep) or device:supports_server_cluster(clusters.Switch.ID, ep) then | ||
| num_server_eps = num_server_eps + 1 | ||
| -- Configure MCD for button endpoints | ||
| if tbl_contains(STATIC_PROFILE_SUPPORTED, #button_eps) then |
There was a problem hiding this comment.
I think this would break in the event that we have a more complex device type such that
ep1 = switch
ep2 = button
ep3 = button
Because then the first switch EP would be assigned to main button component, and the ep3 button would be created as a child of the first two endpoints.
Overall, I don't think we have a robust plan for handling combined switch + button device types yet. If the scope of these PR is just meant to "add support for button devices to the matter switch driver, but NOT add support for combination switch + button devices" then maybe we could simplify this by handling the button and switch eps separately and not assuming that there would be a device that supports both.
However, if the goal is to add support for switch + button combo devices, then I think we need to reconsider how we would handle that. Right now, buttons are mostly be handled as hard-coded MCD profiles. Switch devices types (plugs, bulbs, etc.) are being handled as parent-child devices. Would a combo device have to be MCD by default? Could we mix parent child with MCD (I'm not sure if this is even possible) and have an MCD 3-button parent with a child light device (or vice versa)? Right now, I don't think this could handle devices like that properly and I think this is worth starting a discussion about - unless I have already missed this somewhere!
| @@ -226,6 +510,8 @@ end | |||
|
|
|||
| local function device_init(driver, device) | |||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER then | |||
| device:set_component_to_endpoint_fn(component_to_endpoint) | |||
There was a problem hiding this comment.
Do these functions still need to be moved here before the initialize_switch call, or do they work where they were before now that we have the deferred configure for the buttons in place?
I don't think it matters where these are called in the init function, I just want to make sure I understand why they needed to be moved and if they still need to be for some reason.
Adjust logic in initialize_switch based on review feedback
nickolas-deboom
changed the base branch from
main
to
merge-switch-and-button-drivers
nickolas-deboom
changed the title
nickolas-deboom
changed the title
nickolas-deboom
changed the base branch from
merge-switch-and-button-drivers
to
main
nickolas-deboom
changed the title
This PR was split into three separate PRs in order to simplify reviewing and also to allow more discussion and testing before supporting combination button/switch device types.
The newly created PRs are: