Adding request-commissioning CHIPTool command #7729
Conversation
ReturnErrorOnFailure(mCommissionableNodeController.SendUserDirectedCommissioningRequest(args...));This comment was generated by todo based on a
|
integrate with Server:SendUserDirectedCommissioningRequest()connectedhomeip/src/controller/CHIPCommissionableNodeController.cpp Lines 57 to 63 in 621d6e8 This comment was generated by todo based on a
|
pullapprove
bot
requested review from
andy31415,
chrisdecenzo,
jelderton,
jmartinez-silabs,
LuDuda,
mspang,
saurabhst and
woody-apple
|
|
||
| constexpr uint16_t kWaitDurationInSeconds = 30; | ||
|
|
||
| CHIP_ERROR RequestCommissioningCommand::Run(NodeId localId, NodeId remoteId) |
There was a problem hiding this comment.
I'm surprised that localId and remoteId are the right arguments for this. The UDC command sends the instance name (which is not the nodeId) to the IP and port specified in the SRV record of the selected commissioner. Is there code to derive those values from the remoteId in the "SendUserDirectedCommissioningRequest" command?
There was a problem hiding this comment.
I'm surprised that localId and remoteId are the right arguments for this.
Those are the arguments that currently all commands get; the virtual Run method has those arguments. Some commands completely ignore them (and indeed don't get useful ones); the QR code parsing commands are a good example. In addition to these args commands can have whatever members are initialized via the command line. The args are just a way to avoid having to keep passing in node ids on the command line for various IM bits once you've set them up with one of the "pairing" commands....
There was a problem hiding this comment.
I did not really have a use for the passed down localId or remoteId, but didn't have a choice to stop them from being passed.
As for deriving the instance name, looking at https://github.com/project-chip/connectedhomeip/pull/6790/files I believe the GetCommissionableInstanceName() function could be used?
| ChipLogProgress(chipTool, "Waiting for %d sec", kWaitDurationInSeconds); | ||
| } | ||
|
|
||
| WaitForResponse(kWaitDurationInSeconds); |
There was a problem hiding this comment.
Where will the response come from, exactly? This looks like it'll just sit here for 30 seconds and then time out...
There was a problem hiding this comment.
The response would be expected from a Device Commissioner on the network that receives the User Directed commissioning request as described in step # 9 in https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/rendezvous/InitiatingSetup.adoc#242-high-level-flow - the call to sendUDC method would be included in a follow-up PR (trying to keep diffs for review manageable).
examples/chip-tool/commands/pairing/RequestCommissioningCommand.cpp
Outdated
Show resolved
Hide resolved
| #endif | ||
| } | ||
|
|
||
| CHIP_ERROR CommissionableNodeController::SendUserDirectedCommissioningRequest(chip::Inet::IPAddress commissioner, uint16_t port) |
There was a problem hiding this comment.
Where would the input to this function come from?
There was a problem hiding this comment.
It would come from the user in the form of a command line argument to this command as per step # 5 in https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/rendezvous/InitiatingSetup.adoc#242-high-level-flow (in a follow-up PR).
|
This generally looks good to me. I believe, however, it will need a slight refactor to conform to #7829. |
|
Size increase report for "esp32-example-build" from 039dfbd
Full report output |
| @@ -18,6 +18,9 @@ static_library("controller") { | |||
| output_name = "libChipController" | |||
|
|
|||
| sources = [ | |||
| "${chip_root}/src/app/server/Mdns.cpp", | |||
There was a problem hiding this comment.
Don't do this.
A source file should belong to one library at a time which determines its options (defines, include dirs, etc). If you want to share code, create a new library and make it a dependency.
)" (project-chip#8095) This reverts commit 355a991.
Problem
Support for Initializing setup via "commissioner-discovery-from-an-on-network-device" is pending implementation of commands for Steps 7-12.
Note: Steps 3-5 were previously implemented as a discover-commissioner command in the CHIPTool (see PR)
What is being fixed?
Change overview
Testing
Tested by running the new request-commissioning command on the CHIPTool and verifying using minimal-mdns-client that a commissionable node is advertised. See outputs below from each command: