Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DeviceController::PairDevice(NodeId remoteId, const char * setupCode) API that looks for the device over supported networks resolves #9343 #9847

Conversation

vivien-apple
Copy link
Contributor

Problem

As of today, CHIPDeviceController::PairDevice API requires the user to pass the ip, port and interfaceId of the device to connect to.

This PR introduce an additional API, that takes a const char * setupCode and search over supported networks for a device (currently BLE/IP).
The current CHIPDeviceController is already complex, and I would expect the new code to grow as more edge cases are coming in. As a result I have added a separate class for the mentioned work.

As a separate issue I think it will be helpful to spend some times on how the mdns records are retrieved. Currently AbstractMdnsDiscoveryController is used to look over the network for commissionable nodes. It is not very practical because under the hood, there is a “Browse” operation which ends up beeing broken into multiples “Resolve”. AbstractMdnsDiscoveryController does not have a way to know when the multiple resolves are done for a single browse, and so there is no easy way for the user of the API to know when it is time to retrieve commissionable nodes results.

Change overview

  • Update src/platform/Darwin/MdnsImpl.cpp to support browsing for subtypes
  • Update src/setup_payload/ headers to use #pragma once since that’s missing and it was causing conflicts…
  • Add a new class chip::Controller::SetUpCodeParser which parses a setup code and extract the relevant rendezvousInformation in order to search for the target peripheral
  • Update chip-tool to have a few additional commands for browsing mdns
  • Update chip-tool pairing qrcode and pairing manualcode APIs to use the new SetupCodeParser::PairDevice interface

Testing

scripts/tests/test_suites.sh has been changed so it uses the new API in order to find which device it needs to connect to. This should exercise the API, as well as the minimal and Darwin mdns backends.

@cecille
Copy link
Contributor

cecille commented Sep 21, 2021

General comment - in the python controller, this is handled for QR by having the python layer parse the code, discover, then connect. I'm not sure bunching all this together in the SetupParser is the correct place for this. To me, SetupParser seems like it should be exclusively for parsing the setup code - that way it's contained and testable. This all seems like it should be implemented a layer up in a convenience function layer where we put all the steps together for those that wish to do that.

src/controller/SetUpCodeParser.h Outdated Show resolved Hide resolved
src/controller/SetUpCodeParser.h Outdated Show resolved Hide resolved
@vivien-apple vivien-apple force-pushed the Controller_PairDeviceAllSupportedNetwork branch from a158dd0 to 676adf0 Compare September 21, 2021 16:26
@vivien-apple
Copy link
Contributor Author

General comment - in the python controller, this is handled for QR by having the python layer parse the code, discover, then connect. I'm not sure bunching all this together in the SetupParser is the correct place for this. To me, SetupParser seems like it should be exclusively for parsing the setup code - that way it's contained and testable. This all seems like it should be implemented a layer up in a convenience function layer where we put all the steps together for those that wish to do that.

I have renamed the class so (I hope) it is a bit clearer. The idea behind having a separate class is to be able to add more logic to it without having to pollute the DeviceController.

I understand that the python controller parses the QR code manually before using those informations. The issue is that this is also done (before this PR) inside chip-tool, and then inside iOS ChipTool, and likely inside the Java controller....
It does sounds like there is a need for a single API that does this simple operation for you, while if you want to do it your way there is still an API that let's you do so.

@pan-apple
Copy link
Contributor

Fixes #9343

@franck-apple franck-apple added this to the Test Event 7 milestone Sep 24, 2021
@woody-apple woody-apple linked an issue Sep 29, 2021 that may be closed by this pull request
@woody-apple woody-apple changed the title Add DeviceController::PairDevice(NodeId remoteId, const char * setupCode) API that looks for the device over supported networks Add DeviceController::PairDevice(NodeId remoteId, const char * setupCode) API that looks for the device over supported networks resolves #9343 Sep 29, 2021
@vivien-apple vivien-apple force-pushed the Controller_PairDeviceAllSupportedNetwork branch from 676adf0 to 857f421 Compare September 30, 2021 09:37
@vivien-apple vivien-apple force-pushed the Controller_PairDeviceAllSupportedNetwork branch from 857f421 to e6186fe Compare October 1, 2021 15:08
@vivien-apple vivien-apple merged commit 02d8900 into project-chip:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants