Matter Switch: Simplify component to endpoint mapping logic #2560
Conversation
hcarter-775
requested review from
ctowns,
gharveymn,
greens and
nickolas-deboom
|
Channel deleted. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 574fd58 |
| local formatted_info = {} | ||
| if type(ep_info) == "number" then | ||
| ep_info = { endpoint_id = ep_info } | ||
| formatted_info = { endpoint_id = ep_info } | ||
| elseif type(ep_info) == "table" then | ||
| ep_info = { | ||
| endpoint_id = ep_info.endpoint_id, | ||
| cluster_id = ep_info.cluster_id, | ||
| attribute_id = ep_info.attribute_id | ||
| } | ||
| formatted_info = ep_info |
There was a problem hiding this comment.
Effectively, can't this just be
if type(ep_info) == "number" then
ep_info = { endpoint_id = ep_info }
end? There isn't any transform done to the table form, and other types are not transformed.
There was a problem hiding this comment.
oh it can. I just thought that might be weird/wrong cause the capability event id is going to get appended later and that's not technically "ep info"
There was a problem hiding this comment.
but I suppose it's kinda equally weird to break it apart like I did, so I'll switch it
There was a problem hiding this comment.
ended up removing capabilities from the mapping, so this makes even more sense now
| --- to route events to. | ||
| --- | ||
| --- @param device any a Matter device object | ||
| --- @param ep_info number|table endpoint_id or ib (includes endpoint_id, cluster_id, attribute_id) |
There was a problem hiding this comment.
Are cluster_id and attribute_id required when in table form? Is capability_id required as well?
There was a problem hiding this comment.
again, they are at the time but I suppose don't need to be. changed the wording in the upcoming commit
There was a problem hiding this comment.
well, it actually reads endpoint_id or ib, and I note that the ib includes that data. I agree this wording is a little confused
| --- single endpoint. | ||
| --- | ||
| --- @param device any a Matter device object | ||
| --- @param opts number|table either is an ep_id or a table { endpoint_id, capability_id } |
There was a problem hiding this comment.
I think this doc should include mention of the required fields of cluster_id and attribute_id when in table form.
| and (not map_info.cluster_id or (map_info.cluster_id == opts.cluster_id | ||
| and utils.tbl_contains(map_info.attribute_ids, opts.attribute_id))) |
There was a problem hiding this comment.
We are using the logic that existence of the cluster_id field implies the existence of a non-nil attribute_id. Is this always the case?
There was a problem hiding this comment.
It is at the time (and I can't see when/why it wouldn't be), though I can see how it can seem a little arbitrary. fixed in next commit (cannot push now due to github outage)
| --- | ||
| --- @param device any a Matter device object | ||
| --- @param opts number|table either is an ep_id or a table { endpoint_id, capability_id } | ||
| --- @param ep_info number|table either an ep_id or a table { endpoint_id, optional(cluster_id), optional(attribute_id) } |
There was a problem hiding this comment.
Can we choose a single parameter type and make the the caller provide the additional info? I know lua doesn't have strong parameter typing but I think this is a practice we should try to avoid. If you need to handle additional optional values, they can be passed as optional parameters:
function utils.endpoint_to_component(device, ep, cluster_id, attribute_id)
There was a problem hiding this comment.
We cannot. Since we want to have this be compatible with the lua libs, we cannot add extra arguments. This was a compromise I figured was relatively clear given the limitation.
There was a problem hiding this comment.
I don't understand the reasoning for why we cannot add extra arguments - why is backwards compatibility a concern if we are creating a new function that overrides the lua libs? I think we should only use this override when needed, and it shouldn't be a blanket override for all devices in the switch driver, unless they specifically require this advanced mapping, right?
If this is a function override, I assume we can make it whatever we want. Even using extend_device allows us to override the function pointer from the lua libs and just point to this new function, even though I don't neccessarily think using extend_device in this case is the way to go.
There was a problem hiding this comment.
Also just a note that this discussion isn't a blocker if this needs to go in urgently, but I think it's worth having more discussion around how we should be designing these APIs for this complex mapping.
nickolas-deboom
left a comment
nickolas-deboom
left a comment
There was a problem hiding this comment.
LGTM, I tested with the camera example app and things are looking good with these changes.
Description of Change
This cuts down the amount of checks and reformatting handled in the
endpoint_to_componentandemit_event_for_endpointfunctions.This logic was just added by me in the recent Camera PR and it was a bit rushed. This cleans up some unnecessary logic that was introduced.
Summary of Completed Tests
Tested by Unit Test and with VDA. Expanded functionality is currently only utilized by unreleased Camera subdriver