Check descriptor clusters during commissioning #14410
Conversation
pullapprove
bot
requested review from
anush-apple,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
LuDuda,
lzgrablic02,
mlepage-google,
mrjerryjohns and
msandstedt
pullapprove
bot
requested review from
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
|
PR #14410: Size comparison from 63d0c37 to eb8b946 Increases (1 build for linux)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
mlepage-google
changed the title
| @@ -1602,11 +1680,50 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio | |||
|
|
|||
| switch (step) | |||
| { | |||
| case CommissioningStage::kReadVendorId: { | |||
| ChipLogProgress(Controller, "Reading vendor ID"); | |||
There was a problem hiding this comment.
This is a lot of round-trips.... Do we actually want to read these attributes one at a time? Why do we want to walk the list of all endpoints starting from the end looking for network commissioning things, instead of starting at the front and stopping when we find the first network commissioning thing which supports a network technology we support?
There was a problem hiding this comment.
No, but as far as I can tell, we don't have support for bulk reading attributes right now.
There was a problem hiding this comment.
They're all checked in order to address the request for supporting network commissioning clusters on other endpoints (multiple radios). There is always a NetworkCommissioning cluster on EP0, but you had a request in the network tecnology PR (#13829) to support network commissioning clusters that could be present on endpoints with manufacturer specific device types.
see #14412
There was a problem hiding this comment.
we don't have support for bulk reading attributes right now
We certainly do. You just have to use a slightly lower-level API to do it, because the results of such a read can't be expressed as a single C++ value.... Doing this can be a followup, though.
They're all checked in order to address the request for supporting network commissioning clusters on other endpoints
This just seems like an odd approach to addressing that request. What I would have expected is we start with the network commissioning on EP0. If we can't commission using that (rare case), we look for another one.
It feels like this is something where people are going to want to do a variety of things and UX flows, and reading all endpoints can be quite costly (latency) for devices with lots of endpoints....
There was a problem hiding this comment.
There's always going to be a network commissioning cluster on EP0, so if we're starting there, we're ending there. If we want to allow devices to specify more than one networking technology, we have to read them all.
There was a problem hiding this comment.
I'm not following. If what's on EP0 is a WiFi cluster but we don't have any WiFi credentials, but do have Thread ones, then we would want to look for other network commissioning clusters to see if the device supports Thread. But if EP0 has a network technology we're happy to commission with, why would we spend time looking at the other endpoints?
There was a problem hiding this comment.
I'd actually argue that dual network commissioning clusters is dubiously supported anyway and unlikely to happen in a real device. The only place where this would be really beneficial would be for a border router, and the network commissioning clusters are insufficient there anyway because we there aren't sufficient commands to support creating a thread network.
The alternate is to assume EP0 for the networking cluster allow devices that require a more complicated setup use a custom commissioning flow. If we want to be able to commission border routers, the network commissioning cluster needs some additional commands, and we could then add a parts list to make the endpoint walk easier.
There was a problem hiding this comment.
How would you know to send in only thread credentials if you didn't know what the device supported in the first place? That assumes that the commissioning parameters are the source of truth, whereas what we really want is for the device to be the source of truth.
I suppose we could cut out some of the back and forth if we only have one set of credentials and it happens to match the feature map for the network cluster on EP0.
I wrote it this way originally because I was going to double-purpose this for a convenience function to allow devs to get device information before calling the commission command. Let me see if I can maybe disentangle this use case and short-circut the full walk during commissioning if we only get one set of network credentials that matches EP0.
There was a problem hiding this comment.
OK, I've updated the PR to just go with the first networking cluster it finds. Working with jerry on a follow up to just query all the network cluster feature maps using wild card - I didn't realize that was implemented now, but it's a bit more of a change.
If we check the feature map right after we check for the existance
of the networking cluster, we can just bail out once we find a
networking cluster that matches
Test: on-network with lighting app on linux
wifi on all-clusters app on M5
tested thread using all clusters app - saw reads of endpoints,
found network cluster on different endpoint.
|
PR #14410: Size comparison from 08d8fd8 to 420142b Increases (11 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
Full report (16 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
|
Alternate proposal: #14567 Jerry showed me how to do wildcard reads. I wasn't aware they were supported yet, but it greatly simplifies the logic on the commissioning delegate because we can just query all the network cluster feature maps at once and directly parse which endpoints have which clusters. The logic on the DeviceCommissioner is a bit more complex, but that seems more reasonable to me. In theory I can do that to the general commissioning cluster too (just not in the PR). If I can get consensus to go with that option, I will drop this PR. |
|
#14567 has landed, so this PR is officially obsolete. Closing. |
Problem
Adding some additional checks to ensure that the device has the appropriate clusters for commissioning.
Change overview
Testing
printed values and tested with M5 and linux lighting app. Commissioning completes successfully.