Matter Switch: add support for additional switch device types #1376

hcarter-775 · 2024-05-14T17:42:03Z

The Dimmer Switch and Color Dimmer Switch both support the OnOff cluster as client, but some devices in the field support them as server. These changes accomodate that discrepancy. Changes for OnOff Switch device type were originally commited here:
cda431b

https://smartthings.atlassian.net/browse/CHAD-13121

The Dimmer Switch and Color Dimmer Switch both support the OnOff cluster as client, but some devices in the field support them as server. These changes accomodate that discrepancy. Changes for OnOff Switch device type were originally commited here: cda431b

CLAassistant · 2024-05-14T17:42:09Z

All committers have signed the CLA.

github-actions · 2024-05-14T17:43:27Z

Channel deleted.

github-actions · 2024-05-14T17:43:43Z

Test Results

57 files ± 0 367 suites +2 0s ⏱️ ±0s
1 780 tests + 7 1 780 ✅ + 7 0 💤 ±0 0 ❌ ±0
3 095 runs +21 3 095 ✅ +21 0 💤 ±0 0 ❌ ±0

Results for commit 81610a4. ± Comparison against base commit be2f7d9.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-14T17:43:45Z

File	Coverage
All files	`94%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/eve-energy/init.lua	`91%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/init.lua	`96%`	✅

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 81610a4

ctowns

Looks great! Let's test this with the VDA and then we should be good to go

hcarter-775 · 2024-05-15T17:37:01Z

Completed testing using the following VDA devices on my personal v3:

Virtual OnOff Switch
Virtual Dimmer Switch
Virtual Color Dimmer Switch
Virtual Bridge with multiple instances of the devices listed above, as well as instances of various Light devices.
All functionality was preserved for previously working devices, as well as for new functionality for Dimmer and Color Dimmer Switch in the cases of a Bridge and on their own.

gharveymn · 2024-05-21T19:41:23Z

drivers/SmartThings/matter-switch/src/init.lua

+      -- main_endpoint only supports server cluster by definition of get_endpoints()
+      if main_endpoint == ep.endpoint_id then
+        for _, dt in ipairs(ep.device_types) do
+          id = math.max(id, dt.device_type_id)


Will this break if the device also has some other device type with an ID higher than On/Off Light Switch and friends?

No, because below that, we have this check:
if device_type_profile_map[id] ~= nil then device:try_update_metadata({profile = device_type_profile_map[id]}) end
If there were a higher ID, it will not have a mapping in the device_type_profile_map array, and so nothing will be be done in that case.

Maybe a silly question, but how did you clip that code snippet in your comment?

So this code should only run if detect_matter_thing is true, which means a device joined that did not match to any generic fingerprint for a device type that we support. We do not have generic fingerprints for the On/Off Light Switch and friends because we technically do not support them because these are meant to work with bindings. However, we can support these device types if they do not follow the spec and implement their application clusters as server.

If some other device type we are not accounting for comes along, then the check on line 185 will ensure we do not switch to it because it does not have a matching profile in our profile map.

For context, the reason why we check for the largest ID is because endpoints can report that they support superset/subset device types on the same endpoint: https://librarian-regionals.smartthingsgdev.com/matter/index.html#ref_SupersetDeviceTypes

Therefore, an endpoint could support On/Off Light Switch, Dimmer Switch, and Color Dimmer Switch and report all of these as device types since Color Dimmer is a super set of the others. However, it could not also have a OnOff Plug device type on the same endpoint because that is not a subset/superset of the other device types on the endpoint.

Maybe a silly question, but how did you clip that code snippet in your comment?

When you leave a comment on a specific line on a review, it will automatically clip a few lines around where you left your comment so you can see the comment in context in this view.

To leave a comment on specific line, you just need to click the blue + sign next to line number you want to leave a comment on in the "Files changed" view. Here's a little bit more context from the GitHub docs: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request#starting-a-review

I get what you're trying to do with the max wrt the superset device type, but

However, it could not also have a OnOff Plug device type on the same endpoint because that is not a subset/superset of the other device types on the endpoint.

I think I am missing where the spec is saying this—I don't see it in the link provided. Could you help me out?

And, to elucidate, this is the device type list I'm wondering about:

0x0103 On/Off Light Switch 0x0104 Dimmer Switch 0x0105 Color Dimmer Switch 0x010A On/Off Plug-in Unit

Then, this code will detect 0x010A and stuff will be broken, or am I missing something? Personally, if the spec isn't absolutely clear that this isn't allowed, I think we will have fewer potential problems in the future if we only do the max of the ones we are targeting.

Thank you! Wtf does this even mean though:

The endpoint MAY support additional device types (application, utility or node device types) as defined by each additional device type.

Yeah, the explanation makes sense, but yeah let's just make it explicit so we have less to worry about.

Yeah, this is not terribly clear... I believe this is referring to cases where you might have a bridged device that has a bridged node device type and an application cluster, or a case where you have an application cluster and a utility cluster (power source for example). As of right now, I do not see any device types that include as a part of their device types requirements defining another application cluster on the same endpoint. I believe this line is just here in case that situation should ever arise, but right now in cases where any additional device types dependencies are included in the spec for a device type, those additional device types are included on separate endpoints.

Here is a somewhat relevant thread from the CSA channel that helps provide a little more context. https://csamembers.slack.com/archives/C01B01GH64E/p1684356219272049?thread_ts=1684272999.049459&cid=C01B01GH64E

However, I still agree that making this explicit seems less error prone, so I think we should make the change that Gene proposed @hcarter-775

Thanks for the great question @gharveymn !

@ctowns @gharveymn I have committed a change in line 180 with a corresponding comment as to this discussion. Tests continue to pass. I have not retried VDA devices with this driver change; do you think that is necessary in this case?

In general, I lean on the side of caution and try to test as much as I can. You are right that this is a small change that shouldn't affect functionality given our test cases, but I'd still recommend using the VDA to join a few devices (you can join all these devices in one test by adding them as bridged devices on the virtual bridge in the VDA) and just doing a smoke test to make sure we didn't overlook anything.

I added a VDA bridge device (on/off switch, on/off dimmer switch, on/off color dimmer switch) last night using the edited drivers- everything worked as expected.

…into CHAD-13121-switch-device-types

ctowns · 2024-05-23T01:48:37Z

drivers/SmartThings/matter-switch/src/init.lua

+      if main_endpoint == ep.endpoint_id then
+        for _, dt in ipairs(ep.device_types) do
+          -- no device type that is not in the switch subset should be considered.
+          if (dt.device_type_id <= ON_OFF_COLOR_DIMMER_SWITCH_ID) then


This check will still include these device types because they have a lower value than ON_OFF_COLOR_DIMMER_SWITCH_ID = 0x0105

local ON_OFF_LIGHT_DEVICE_TYPE_ID = 0x0100 local DIMMABLE_LIGHT_DEVICE_TYPE_ID = 0x0101

So you'll probably want to add a lower bounds on this as well.

This has been added in the most recent push

…into CHAD-13121-switch-device-types

gharveymn · 2024-05-23T17:26:10Z

drivers/SmartThings/matter-switch/src/init.lua

  if num_server_eps > 0 and detect_matter_thing(device) == true then
-    device:try_update_metadata({profile = "switch-binary"})
+    local id = 0
+    for _,ep in ipairs(device.endpoints) do


super nit:

Suggested change

for _,ep in ipairs(device.endpoints) do

for _, ep in ipairs(device.endpoints) do

made that change 👍

…into CHAD-13121-switch-device-types

ctowns

Look good to me, great work @hcarter-775 ! 🎉

One thing we may want to do is squash some of these commits together before merging. We can essentially squash all these changes into one single commit that covers all the changes. Using separate commits as you have done here is helpful for review, but now that we are ready to merge, it can be useful from a readability and maintenence perspective to squash these commits into one commit since most of the extra commits are small fixes rather than new additions to the original commit.

devhari02 · 2024-05-24T06:16:29Z

Hi @hcarter-775 .
Actually I need help regarding PR against smartthings drivers.
It will be helpful if we connect together. Ping me through main id: [email protected]

hcarter-775 requested review from samadDotDev, gharveymn, Con-Tejus, ctowns, z-michel and nickolas-deboom May 14, 2024 17:42

ctowns and others added 2 commits May 15, 2024 11:11

Merge branch 'main' into CHAD-13121-switch-device-types

4798dac

Fixup minor issues from lua check, as well as typos

01b8228

ctowns reviewed May 15, 2024

View reviewed changes

gharveymn reviewed May 21, 2024

View reviewed changes

hcarter-775 requested a review from gharveymn May 22, 2024 15:13

hcarter-775 added 2 commits May 22, 2024 12:09

Fixup minor issues from lua check, as well as typos.

0c78592

Merge remote-tracking branch 'origin/CHAD-13121-switch-device-types' …

6b8abd5

…into CHAD-13121-switch-device-types

ctowns reviewed May 23, 2024

View reviewed changes

hcarter-775 added 2 commits May 23, 2024 11:46

Merge remote-tracking branch 'origin/CHAD-13121-switch-device-types' …

179854d

…into CHAD-13121-switch-device-types

Merge remote-tracking branch 'origin/CHAD-13121-switch-device-types' …

f002886

…into CHAD-13121-switch-device-types

gharveymn approved these changes May 23, 2024

View reviewed changes

hcarter-775 added 2 commits May 23, 2024 12:32

Merge remote-tracking branch 'origin/CHAD-13121-switch-device-types' …

06a3fba

…into CHAD-13121-switch-device-types

Merge remote-tracking branch 'origin/CHAD-13121-switch-device-types' …

81610a4

…into CHAD-13121-switch-device-types

ctowns approved these changes May 23, 2024

View reviewed changes

hcarter-775 merged commit ea55bb9 into main May 23, 2024
11 checks passed

hcarter-775 deleted the CHAD-13121-switch-device-types branch May 23, 2024 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matter Switch: add support for additional switch device types #1376

Matter Switch: add support for additional switch device types #1376

hcarter-775 commented May 14, 2024

CLAassistant commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

ctowns left a comment

hcarter-775 commented May 15, 2024

gharveymn May 21, 2024

hcarter-775 May 22, 2024

ctowns May 22, 2024

ctowns May 22, 2024

gharveymn May 22, 2024 •

edited

Loading

gharveymn May 22, 2024

ctowns May 22, 2024

hcarter-775 May 22, 2024

ctowns May 23, 2024

hcarter-775 May 23, 2024

ctowns May 23, 2024

hcarter-775 May 23, 2024

gharveymn May 23, 2024

hcarter-775 May 23, 2024

ctowns left a comment

devhari02 commented May 24, 2024

	for _,ep in ipairs(device.endpoints) do
	for _, ep in ipairs(device.endpoints) do

Matter Switch: add support for additional switch device types #1376

Matter Switch: add support for additional switch device types #1376

Conversation

hcarter-775 commented May 14, 2024

CLAassistant commented May 14, 2024 • edited Loading

github-actions bot commented May 14, 2024 • edited Loading

github-actions bot commented May 14, 2024 • edited Loading

Test Results

github-actions bot commented May 14, 2024 • edited Loading

ctowns left a comment

Choose a reason for hiding this comment

hcarter-775 commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gharveymn May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctowns left a comment

Choose a reason for hiding this comment

devhari02 commented May 24, 2024

CLAassistant commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

gharveymn May 22, 2024 •

edited

Loading