Matter Switch: Add more reliability to device type checking #2564
Conversation
hcarter-775
requested review from
ctowns,
gharveymn,
greens and
nickolas-deboom
|
Channel deleted. |
| --- Some devices report multiple device types which are a subset of | ||
| --- a superset device type (Ex. Dimmable Light is a superset of On/Off Light). | ||
| --- We should map to the largest superset device type supported. | ||
| --- This can be done by matching to the device type with the highest ID |
There was a problem hiding this comment.
Sorry, I wasn't being clear. I was actually referring to the assertion that the superset IDs are always greater than the IDs of subsets. I think what you had before for the rest is actually more clear than after the change.
There was a problem hiding this comment.
If it's not explicitly specified, and it isn't feasible to switch to an alternative algorithm, we should explicitly point out that this is just a heuristic, and could break in the future.
There was a problem hiding this comment.
revrerted, and added a note
Test Results 71 files 469 suites 0s ⏱️ Results for commit 1e4fcde. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 98d0d61 |
| --- Lights and Switches are Device Types that have Superset-style functionality | ||
| --- For all other device types, this function should be used to identify the primary device type | ||
| function utils.find_primary_device_type(ep_info) | ||
| for _, dt in ipairs(ep_info.device_types) do |
There was a problem hiding this comment.
Sort this list first maybe?
There was a problem hiding this comment.
what would it be sorting? Further, the device type ids aren't necessarily "ordered" in a meaningful way, adding to my question of what "sorting" would be wanted per se?
There was a problem hiding this comment.
Is there anything special about the 'first' device type in ep_info.device_types that would be returned by this function in that case?
There was a problem hiding this comment.
well, the idea is we need to return something. Generally, there should only be one. And before, it was just the first one. Now it's the first one that isn't bridged node. It's kinda jank cause it handles a spec-breaking system.
There was a problem hiding this comment.
the device type ids aren't necessarily "ordered" in a meaningful way
does that fact add additional context to this discussion, or is this referring to something else?
There was a problem hiding this comment.
@gharveymn kinda, but not necessarily. It does speak to the fact that for "superset" scoped device types, the fact that the ids increment is heuristic and may not always be true (though at present it is). On the other hand, this comment also speaks to the fact that the ordering of device types on an endpoint is arbitrary as well
Description of Change
Because not all devices match spec exactly, device type checking should not directly call the "first" device type, since even Application scoped endpoints may have other non-spec compliant device types attached as well (such as bridged node). This has been seen in the field.
Summary of Completed Tests
Tested on a device breaking spec by putting Bridged Node on all endpoints