Enable pairing for multiple devices #3630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
BroderickCarlin,
bzbarsky-apple,
chrisdecenzo,
hawk248,
jelderton,
mspang,
rwalker-apple,
sagar-apple and
saurabhst
jelderton
approved these changes
Nov 5, 2020
|
rebased |
|
rebased |
andy31415
reviewed
Nov 9, 2020
This was referenced Nov 10, 2020
Closed
| * @brief | ||
| * Set the delegate object which will be called when a message is received. | ||
| * The user of this Device object must reset the delegate (by calling | ||
| * SetDelegate(nullptr)) before releasing their delegate object. |
There was a problem hiding this comment.
This implies they control the lifetime of the Device somehow, right? That is, that the device can't go away on them?
Please file a followup to either document ownership here better or add more callbacks to allow a variety of ownership models to be implemented.
src/controller/CHIPDevice.h
Outdated
| * interface etc) are part of the serialized device, so those are not required to be | ||
| * initialized. | ||
| * | ||
| * Note: The lifetime of session manager, and inet layer objects must be longer than |
There was a problem hiding this comment.
Suggested change
| * Note: The lifetime of session manager, and inet layer objects must be longer than | |
| * Note: The lifetime of session manager and inet layer objects must be longer than |
src/controller/CHIPDevice.h
Outdated
|
|
||
| /** | ||
| * @brief | ||
| * Return if the current device object is actively associated with a paired CHIP |
There was a problem hiding this comment.
Suggested change
| * Return if the current device object is actively associated with a paired CHIP | |
| * Return whether the current device object is actively associated with a paired CHIP |
src/controller/CHIPDevice.h
Outdated
| /* IP Address of the CHIP device */ | ||
| Inet::IPAddress mDeviceAddr; | ||
|
|
||
| /* Port on which the CHIP device is receiving message. Typically it is CHIP_PORT */ |
There was a problem hiding this comment.
Suggested change
| /* Port on which the CHIP device is receiving message. Typically it is CHIP_PORT */ | |
| /* Port on which the CHIP device is receiving messages. Typically it is CHIP_PORT */ |
Comment on lines
+106
to
+119
| serializable.mDeviceId = Encoding::LittleEndian::HostSwap64(mDeviceId); | ||
| serializable.mDevicePort = Encoding::LittleEndian::HostSwap16(mDevicePort); |
There was a problem hiding this comment.
Yes, separate PR is fine, but we should make sure it happens....
src/controller/CHIPDevice.cpp
Outdated
| VerifyOrExit(deserializedLen > 0, error = CHIP_ERROR_INVALID_ARGUMENT); | ||
| VerifyOrExit(deserializedLen <= sizeof(serializable), error = CHIP_ERROR_INVALID_ARGUMENT); | ||
|
|
||
| // The second paramter to FromString takes the strlen value. We are subtracting 1 |
There was a problem hiding this comment.
Suggested change
| // The second paramter to FromString takes the strlen value. We are subtracting 1 | |
| // The second parameter to FromString takes the strlen value. We are subtracting 1 |
Comment on lines
+380
to
+383
| mPairingDelegate = nullptr; | ||
| mRendezvousSession = nullptr; | ||
| mDeviceBeingPaired = kNumMaxActiveDevices; | ||
| mPairedDevicesUpdated = false; |
There was a problem hiding this comment.
I vaguely do, but it's not that big a deal.
| { | ||
| mOnComplete.Response(this, mAppReqState, msgBuf); | ||
| // Let's release the device that's being paired. | ||
| // If pairing was successful, it's information is |
There was a problem hiding this comment.
Suggested change
| // If pairing was successful, it's information is | |
| // If pairing was successful, its information is |
| mOnNewConnection(this, nullptr, mAppReqState); | ||
| } | ||
| ChipLogDetail(Controller, "Remote device completed SPAKE2+ handshake\n"); | ||
| mRendezvousSession->GetPairingSession().Serializable(device->GetPairing()); |
There was a problem hiding this comment.
Yes, ToSerializable would be better.
| public PersistentStorageResultDelegate | ||
| /** | ||
| * @brief | ||
| * The controller applications can use this class to commuicate with already paired CHIP devices. The |
There was a problem hiding this comment.
Suggested change
| * The controller applications can use this class to commuicate with already paired CHIP devices. The | |
| * Controller applications can use this class to communicate with already paired CHIP devices. The |
| /** | ||
| * @brief | ||
| * Connect to a CHIP device with the provided Rendezvous connection parameters | ||
| * This function derserializes the provided deviceInfo object, and initialzes and output the |
There was a problem hiding this comment.
Suggested change
| * This function derserializes the provided deviceInfo object, and initialzes and output the | |
| * This function deserializes the provided deviceInfo object, and initializes and outputs the |
| * Connect to a CHIP device with the provided Rendezvous connection parameters | ||
| * This function derserializes the provided deviceInfo object, and initialzes and output the | ||
| * corresponding Device object. The lifetime of the output object is tied to that of DeviceController | ||
| * object. The caller must not use the Device object If they free the DeviceController object. |
There was a problem hiding this comment.
Or after calling ReleaseDevice in the returned Device?
|
|
||
| State mState; | ||
|
|
||
| /* A list of device objects that can be used for commincating with corresponding |
There was a problem hiding this comment.
Suggested change
| /* A list of device objects that can be used for commincating with corresponding | |
| /* A list of device objects that can be used for communicating with corresponding |
| State mState; | ||
|
|
||
| /* A list of device objects that can be used for commincating with corresponding | ||
| CHIP device. The list does not contain all the paired devices, but only the ones |
There was a problem hiding this comment.
Suggested change
| CHIP device. The list does not contain all the paired devices, but only the ones | |
| CHIP devices. The list does not contain all the paired devices, but only the ones |
| void ClearRequestState(); | ||
| void ClearOpState(); | ||
| /* This field is an index in mActiveDevices list. The object at this index in the list | ||
| contains the device object that's tracking the state of the device that's being paired */ |
There was a problem hiding this comment.
Suggested change
| contains the device object that's tracking the state of the device that's being paired */ | |
| contains the device object that's tracking the state of the device that's being paired. | |
| If no device is currently being paired, this value will be kNumMaxPairedDevices. */ |
Right?
| return error; | ||
| } | ||
|
|
||
| CHIP_ERROR SecurePairingSession::Serializable(SecurePairingSessionSerializable & serializable) |
There was a problem hiding this comment.
I'd prefer ToSerializable, yes.
bzbarsky-apple
approved these changes
Nov 11, 2020
|
Size increase report for "nrfconnect-example-build" from 81f0566
Full report output |
|
Size increase report for "esp32-example-build" from 81f0566
Full report output |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
CHIP Controller API doesn't support pairing of multiple devices. The controller class stores information of one paired device.
Summary of Changes
CHIPDeviceclass.ControllerandCommissionerinterfacesFixes #2795