Matter Switch: fix endpoint to component mapping #986
Conversation
The matter switch driver was initially written with the assumption that for multi-endpoint devices the endpoints would be sequentially numbered starting from 1. However, this is not a requirement in the matter spec, and devices like composed bridged devices often do not follow this assumption. To address this, endpoints are now queried on device add and then mapped to the corresponding component.
|
Channel deleted. |
ctowns
force-pushed
the
matter-switch-fix-endpoint-mapping
branch
from
534b645 to
b637d9c
Compare
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against f8aa183 |
ctowns
requested review from
greens,
tpmanley,
z-michel,
samadDotDev,
gharveymn,
Con-Tejus and
hubTab
| )) | ||
| end | ||
| if num_switch_eps > 1 then | ||
| device:try_update_metadata({profile = string.format("switch-%d", math.min(num_switch_eps, MAX_MULTI_SWITCH_EPS))}) |
There was a problem hiding this comment.
As it was used as such in the past, it could have been useful to define switch-%d as a constant, to be shared across all drivers, to ensure the format would be consistent across.
|
@Con-Tejus the CI generates a channel invite automatically for all PRs: https://bestow-regional.api.smartthings.com/invite/oDM8dkQVkYML |
| end | ||
| return "main" |
There was a problem hiding this comment.
I think what we probably want to do here is, if we have any endpoint to component mapping done at all, to ignore messages from a non-mapped endpoint, rather than emit them from main
I'm thinking of a situation where you have more switches than we have profiles for. Anything from endpoint > X will change the state of "main" which is also a real component that we have mapped to.
There was a problem hiding this comment.
The emit_event_for_endpoint function expects the endpoint_to_component function to return something, or else we would run into an issue in emit_component_event when we try to use the nil value for the component. So, I think there needs to be some kind of default value, even if it is not ideal.
I'm thinking of a situation where you have more switches than we have profiles for. Anything from endpoint > X will change the state of "main" which is also a real component that we have mapped to.
I think this is a valid problem that would be caused by this. The solution would be to add another profile to account for the number of switches. Ideally we'd be able to quickly see the error in the logs since it is logged if a device has more than our supported number of switches
There was a problem hiding this comment.
if you feel like digging into it, what I did with buttons was use parent-child in the case that I didn't have a profile configured for that number of components
There was a problem hiding this comment.
Is this intended to handle the base case or the unexpected case? I would have expected that all expected cases, including the base case with just one endpoint, would be handled by the map.
If this is handling the unexpected case, we need to be louder here, and should avoid affecting the operation of other components. Regardless of the answer to the above, I think that this is how we need to be handling the unexpected case.
There was a problem hiding this comment.
if you feel like digging into it, what I did with buttons was use parent-child in the case that I didn't have a profile configured for that number of components
@greens What are the tradeoffs between using parent-child vs. making a new profile? My understanding on how we would handle this is that we support up to 7 switches, but if a device comes along that has more we'd add another profile for number of switches and it would work. We'd know when a device needs a new profile because of the error() in initialize_switch()when there are more than the max number of switches. I think adding parent-child could be done in a separate PR and I think it could be a good idea given the limitation of our static profiles, but would that mean we'd rely on parent-child instead of creating a new static profile for devices with >7 switches?
I am also not sure what the market looks like, but a cursory glance seems like bumping our support up to 8 might cover most use cases.
Is this intended to handle the base case or the unexpected case? I would have expected that all expected cases, including the base case with just one endpoint, would be handled by the map.
If this is handling the unexpected case, we need to be louder here, and should avoid affecting the operation of other components. Regardless of the answer to the above, I think that this is how we need to be handling the unexpected case.
@gharveymn The endpoint map will be created for all devices, regardless of how many endpoints they have (including base case with just one endpoint). For all expected cases (up to 7 switches), each endpoint will be mapped to its own component. Mapping excess endpoints to main is not ideal, but they must be mapped to something (see my comment above). I could add a log here for cases where an excess endpoint is mapped to main so it is clear that we are hitting this unexpected case.
We could potentially make some changes to the lua libs to allow us to ignore unsupported "excess endpoints", but they would be another PR outside of the drivers. Depending on how we decide to handle the unexpected cases (i.e. with only static profiles, or with the parent-child relationship Steven mentioned above), I think the lua lib changes to allow for ignored endpoints could be worthwhile.
drivers/SmartThings/matter-switch/src/test/test_multi_switch.lua
Outdated
Show resolved
Hide resolved
| end | ||
| return "main" |
There was a problem hiding this comment.
Is this intended to handle the base case or the unexpected case? I would have expected that all expected cases, including the base case with just one endpoint, would be handled by the map.
If this is handling the unexpected case, we need to be louder here, and should avoid affecting the operation of other components. Regardless of the answer to the above, I think that this is how we need to be handling the unexpected case.
The matter switch driver was initially written with the assumption that for multi-endpoint devices the endpoints would be sequentially numbered starting from 1. However, this is not a requirement in the matter spec, and devices like composed bridged devices often do not follow this assumption. To address this, endpoints are now queried on device add and then mapped to the corresponding component.