Skip to content

Commit

Permalink
Address API review issues in MTRDeviceController. (#22596)
Browse files Browse the repository at this point in the history 
* Address API review issues in MTRDeviceController.

* Make MTRBaseDevice creation synchronous.  This requires updates to
  MTRBaseDeviceOverXPC to do the possible async getting of the controller id it
  needs during its async operations, not when getting the device object.
* Rename "pairDevice" to "setupCommissioningSessionWithPayload", fix its
  signature, improve documentation.
* Rename "commissionDevice" to "commissionNodeWithID".
* Rename "stopDevicePairing" to "cancelCommissioningForNodeID" and document.
* Rename "getDeviceBeingCommissioned" to "getDeviceBeingCommissionedWithNodeID".
* Various documentation improvements.
* Add a way to generate a QR code from an MTRSetupPayload to allow correct
  recovery of long discriminators in setupCommissioningSessionWithPayload.
* Fix signature of computePaseVerifier.
* Fix a leak when we failed to start a controller because it wanted a fabric
  that does not exist, or wanted a new fabric and a matching one existed.  This
  used to not show up in LSan before, maybe because we did not have an
  autoreleasepool in place.

Fixes #22533

Addresses part of #22420

* Address review comments.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Sep 14, 2022
1 parent 3a3b1db commit 5757068
Show file tree
Hide file tree
Showing 28 changed files with 1,110 additions and 1,141 deletions.
28 changes: 6 additions & 22 deletions examples/darwin-framework-tool/commands/clusters/ModelCommandBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,31 +25,15 @@

CHIP_ERROR ModelCommand::RunCommand()
{
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip-tool.command", DISPATCH_QUEUE_SERIAL);

MTRDeviceController * commissioner = CurrentCommissioner();
ChipLogProgress(chipTool, "Sending command to node 0x" ChipLogFormatX64, ChipLogValueX64(mNodeId));
[commissioner getBaseDevice:mNodeId
queue:callbackQueue
completion:^(MTRBaseDevice * _Nullable device, NSError * _Nullable error) {
if (error != nil) {
SetCommandExitStatus(error, "Error getting connected device");
return;
}

CHIP_ERROR err;
if (device == nil) {
err = CHIP_ERROR_INTERNAL;
} else {
err = SendCommand(device, mEndPointId);
}
auto * device = [MTRBaseDevice deviceWithNodeID:@(mNodeId) controller:commissioner];
CHIP_ERROR err = SendCommand(device, mEndPointId);

if (err != CHIP_NO_ERROR) {
ChipLogError(chipTool, "Error: %s", chip::ErrorStr(err));
SetCommandExitStatus(err);
return;
}
}];
if (err != CHIP_NO_ERROR) {
ChipLogError(chipTool, "Error: %s", chip::ErrorStr(err));
return err;
}
return CHIP_NO_ERROR;
}

Expand Down
17 changes: 7 additions & 10 deletions examples/darwin-framework-tool/commands/pairing/Commands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ class PairCodeThread : public PairingCommandBridge
PairCodeThread() : PairingCommandBridge("code-thread", PairingMode::Code, PairingNetworkType::Thread) {}
};

class PairWithIPAddress : public PairingCommandBridge
{
public:
PairWithIPAddress() : PairingCommandBridge("ethernet", PairingMode::Ethernet, PairingNetworkType::Ethernet) {}
};

class PairBleWiFi : public PairingCommandBridge
{
public:
Expand All @@ -70,10 +64,13 @@ void registerCommandsPairing(Commands & commands)
const char * clusterName = "Pairing";

commands_list clusterCommands = {
make_unique<PairCode>(), make_unique<PairWithIPAddress>(),
make_unique<PairCodeWifi>(), make_unique<PairCodeThread>(),
make_unique<PairBleWiFi>(), make_unique<PairBleThread>(),
make_unique<Unpair>(), make_unique<OpenCommissioningWindowCommand>(),
make_unique<PairCode>(),
make_unique<PairCodeWifi>(),
make_unique<PairCodeThread>(),
make_unique<PairBleWiFi>(),
make_unique<PairBleThread>(),
make_unique<Unpair>(),
make_unique<OpenCommissioningWindowCommand>(),
};

commands.Register(clusterName, clusterCommands);
Expand Down
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/commands/pairing/OpenCommissioningWindowCommand.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
{
mWorkQueue = dispatch_queue_create("com.chip.open_commissioning_window", DISPATCH_QUEUE_SERIAL);
auto * controller = CurrentCommissioner();
auto * device = [[MTRBaseDevice alloc] initWithNodeID:@(mNodeId) controller:controller];
auto * device = [MTRBaseDevice deviceWithNodeID:@(mNodeId) controller:controller];

auto * self = this;
if (mCommissioningWindowOption == 0) {
Expand Down
10 changes: 0 additions & 10 deletions examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ enum class PairingMode
{
None,
Code,
Ethernet,
Ble,
};

Expand Down Expand Up @@ -64,12 +63,6 @@ class PairingCommandBridge : public CHIPCommandBridge
case PairingMode::Code:
AddArgument("payload", &mOnboardingPayload);
break;
case PairingMode::Ethernet:
AddArgument("setup-pin-code", 0, 134217727, &mSetupPINCode);
AddArgument("discriminator", 0, 4096, &mDiscriminator);
AddArgument("device-remote-ip", &ipAddress);
AddArgument("device-remote-port", 0, UINT16_MAX, &mRemotePort);
break;
case PairingMode::Ble:
AddArgument("setup-pin-code", 0, 134217727, &mSetupPINCode);
AddArgument("discriminator", 0, 4096, &mDiscriminator);
Expand All @@ -84,7 +77,6 @@ class PairingCommandBridge : public CHIPCommandBridge
private:
void PairWithCode(NSError * __autoreleasing * error);
void PairWithPayload(NSError * __autoreleasing * error);
void PairWithIPAddress(NSError * __autoreleasing * error);
void Unpair();
void SetUpPairingDelegate();

Expand All @@ -94,9 +86,7 @@ class PairingCommandBridge : public CHIPCommandBridge
chip::ByteSpan mSSID;
chip::ByteSpan mPassword;
chip::NodeId mNodeId;
uint16_t mRemotePort;
uint16_t mDiscriminator;
uint32_t mSetupPINCode;
char * mOnboardingPayload;
char * ipAddress;
};
97 changes: 36 additions & 61 deletions examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@
case PairingMode::Code:
PairWithPayload(&error);
break;
case PairingMode::Ethernet:
PairWithIPAddress(&error);
break;
case PairingMode::Ble:
PairWithCode(&error);
break;
Expand All @@ -83,75 +80,53 @@
void PairingCommandBridge::PairWithCode(NSError * __autoreleasing * error)
{
SetUpPairingDelegate();
auto * payload = [[MTRSetupPayload alloc] initWithSetupPasscode:@(mSetupPINCode) discriminator:@(mDiscriminator)];
MTRDeviceController * commissioner = CurrentCommissioner();
[commissioner pairDevice:mNodeId discriminator:mDiscriminator setupPINCode:mSetupPINCode error:error];
[commissioner setupCommissioningSessionWithPayload:payload newNodeID:@(mNodeId) error:error];
}

void PairingCommandBridge::PairWithPayload(NSError * __autoreleasing * error)
{
NSString * payload = [NSString stringWithUTF8String:mOnboardingPayload];

SetUpPairingDelegate();
MTRDeviceController * commissioner = CurrentCommissioner();
[commissioner pairDevice:mNodeId onboardingPayload:payload error:error];
}

void PairingCommandBridge::PairWithIPAddress(NSError * __autoreleasing * error)
{
NSString * onboardingPayload = [NSString stringWithUTF8String:mOnboardingPayload];
SetUpPairingDelegate();
auto * payload = [MTRSetupPayload setupPayloadWithOnboardingPayload:onboardingPayload error:error];
if (payload == nil) {
return;
}
MTRDeviceController * commissioner = CurrentCommissioner();
[commissioner pairDevice:mNodeId
address:[NSString stringWithUTF8String:ipAddress]
port:mRemotePort
setupPINCode:mSetupPINCode
error:error];
[commissioner setupCommissioningSessionWithPayload:payload newNodeID:@(mNodeId) error:error];
}

void PairingCommandBridge::Unpair()
{
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip-tool.command", DISPATCH_QUEUE_SERIAL);
MTRDeviceController * commissioner = CurrentCommissioner();
[commissioner
getBaseDevice:mNodeId
queue:callbackQueue
completion:^(MTRBaseDevice * _Nullable device, NSError * _Nullable error) {
CHIP_ERROR err = CHIP_NO_ERROR;
if (error) {
err = MTRErrorToCHIPErrorCode(error);
LogNSError("Error: ", error);
SetCommandExitStatus(err);
} else if (device == nil) {
ChipLogError(chipTool, "Error: %s", chip::ErrorStr(CHIP_ERROR_INTERNAL));
SetCommandExitStatus(CHIP_ERROR_INTERNAL);
} else {
ChipLogProgress(chipTool, "Attempting to unpair device %llu", mNodeId);
MTRBaseClusterOperationalCredentials * opCredsCluster =
[[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device endpoint:@(0) queue:callbackQueue];
[opCredsCluster
readAttributeCurrentFabricIndexWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable readError) {
if (readError) {
CHIP_ERROR readErr = MTRErrorToCHIPErrorCode(readError);
LogNSError("Failed to get current fabric: ", readError);
SetCommandExitStatus(readErr);
return;
}
MTROperationalCredentialsClusterRemoveFabricParams * params =
[[MTROperationalCredentialsClusterRemoveFabricParams alloc] init];
params.fabricIndex = value;
[opCredsCluster
removeFabricWithParams:params
completion:^(MTROperationalCredentialsClusterNOCResponseParams * _Nullable data,
NSError * _Nullable removeError) {
CHIP_ERROR removeErr = CHIP_NO_ERROR;
if (removeError) {
removeErr = MTRErrorToCHIPErrorCode(removeError);
LogNSError("Failed to remove current fabric: ", removeError);
} else {
ChipLogProgress(chipTool, "Successfully unpaired deviceId %llu", mNodeId);
}
SetCommandExitStatus(removeErr);
}];
}];
}
}];
auto * device = [MTRBaseDevice deviceWithNodeID:@(mNodeId) controller:commissioner];

ChipLogProgress(chipTool, "Attempting to unpair device %llu", mNodeId);
MTRBaseClusterOperationalCredentials * opCredsCluster =
[[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device endpoint:@(0) queue:callbackQueue];
[opCredsCluster readAttributeCurrentFabricIndexWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable readError) {
if (readError) {
CHIP_ERROR readErr = MTRErrorToCHIPErrorCode(readError);
LogNSError("Failed to get current fabric: ", readError);
SetCommandExitStatus(readErr);
return;
}
MTROperationalCredentialsClusterRemoveFabricParams * params =
[[MTROperationalCredentialsClusterRemoveFabricParams alloc] init];
params.fabricIndex = value;
[opCredsCluster removeFabricWithParams:params
completion:^(MTROperationalCredentialsClusterNOCResponseParams * _Nullable data,
NSError * _Nullable removeError) {
CHIP_ERROR removeErr = CHIP_NO_ERROR;
if (removeError) {
removeErr = MTRErrorToCHIPErrorCode(removeError);
LogNSError("Failed to remove current fabric: ", removeError);
} else {
ChipLogProgress(chipTool, "Successfully unpaired deviceId %llu", mNodeId);
}
SetCommandExitStatus(removeErr);
}];
}];
}
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/commands/pairing/PairingDelegateBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ - (void)onPairingComplete:(NSError *)error
ChipLogProgress(chipTool, "Pairing Success");
ChipLogProgress(chipTool, "PASE establishment successful");
NSError * commissionError;
[_commissioner commissionDevice:_deviceID commissioningParams:_params error:&commissionError];
[_commissioner commissionNodeWithID:@(_deviceID) commissioningParams:_params error:&commissionError];
if (commissionError != nil) {
_commandBridge->SetCommandExitStatus(commissionError);
return;
Expand Down
32 changes: 16 additions & 16 deletions examples/darwin-framework-tool/commands/tests/TestCommandBridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,27 +156,21 @@ class TestCommandBridge : public CHIPCommandBridge,

SetIdentity(identity);

// Invalidate our existing CASE session; otherwise getConnectedDevice
// will just hand it right back to us without establishing a new CASE
// session when a reboot is done on the server.
// Invalidate our existing CASE session; otherwise trying to work with
// our device will just reuse it without establishing a new CASE
// session when a reboot is done on the server, and then our next
// interaction will time out.
if (value.expireExistingSession.ValueOr(true)) {
if (GetDevice(identity) != nil) {
[GetDevice(identity) invalidateCASESession];
mConnectedDevices[identity] = nil;
}
}

[controller getBaseDevice:value.nodeId
queue:mCallbackQueue
completion:^(MTRBaseDevice * _Nullable device, NSError * _Nullable error) {
if (error != nil) {
SetCommandExitStatus(error);
return;
}

mConnectedDevices[identity] = device;
NextTest();
}];
mConnectedDevices[identity] = [MTRBaseDevice deviceWithNodeID:@(value.nodeId) controller:controller];
dispatch_async(mCallbackQueue, ^{
NextTest();
});
return CHIP_NO_ERROR;
}

Expand All @@ -197,7 +191,11 @@ class TestCommandBridge : public CHIPCommandBridge,
length:value.payload.size()
encoding:NSUTF8StringEncoding];
NSError * err;
BOOL ok = [controller pairDevice:value.nodeId onboardingPayload:payloadStr error:&err];
auto * payload = [MTRSetupPayload setupPayloadWithOnboardingPayload:payloadStr error:&err];
if (err != nil) {
return MTRErrorToCHIPErrorCode(err);
}
BOOL ok = [controller setupCommissioningSessionWithPayload:payload newNodeID:@(value.nodeId) error:&err];
if (ok == YES) {
return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -234,7 +232,9 @@ class TestCommandBridge : public CHIPCommandBridge,
VerifyOrReturn(commissioner != nil, Exit("No current commissioner"));

NSError * commissionError = nil;
[commissioner commissionDevice:nodeId commissioningParams:[[MTRCommissioningParameters alloc] init] error:&commissionError];
[commissioner commissionNodeWithID:@(nodeId)
commissioningParams:[[MTRCommissioningParameters alloc] init]
error:&commissionError];
CHIP_ERROR err = MTRErrorToCHIPErrorCode(commissionError);
if (err != CHIP_NO_ERROR) {
Exit("Failed to kick off commissioning", err);
Expand Down
11 changes: 0 additions & 11 deletions scripts/tests/chiptest/lsan-mac-suppressions.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,5 @@ leak:drbg_bytes
# TODO: OpenSSL ERR_get_state seems to leak.
leak:ERR_get_state

# TODO: BLE initialization allocates some UUIDs and strings that seem to leak.
leak:[BleConnection initWithDiscriminator:]
leak:[CBXpcConnection initWithDelegate:queue:options:sessionType:]

# TODO: Figure out how we are managing to leak NSData while using ARC!
leak:[CHIPToolKeypair signMessageECDSA_RAW:]

# TODO: Figure out how we are managing to leak NSData while using ARC, though
# this may just be a leak deep inside the CFPreferences stuff somewhere.
leak:[CHIPToolPersistentStorageDelegate storageDataForKey:]

# TODO: https://github.com/project-chip/connectedhomeip/issues/22333
leak:[MTRBaseCluster* subscribeAttribute*WithParams:subscriptionEstablished:reportHandler:]
6 changes: 4 additions & 2 deletions src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ extern NSString * const kNetworkSSIDDefaultsKey;
extern NSString * const kNetworkPasswordDefaultsKey;
extern NSString * const kFabricIdKey;

typedef void (^DeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NSError * _Nullable error);

MTRDeviceController * _Nullable InitializeMTR(void);
MTRDeviceController * _Nullable MTRRestartController(MTRDeviceController * controller);
id _Nullable MTRGetDomainValueForKey(NSString * domain, NSString * key);
Expand All @@ -36,8 +38,8 @@ uint64_t MTRGetLastPairedDeviceId(void);
void MTRSetNextAvailableDeviceID(uint64_t id);
void MTRSetDevicePaired(uint64_t id, BOOL paired);
BOOL MTRIsDevicePaired(uint64_t id);
BOOL MTRGetConnectedDevice(MTRDeviceConnectionCallback completionHandler);
BOOL MTRGetConnectedDeviceWithID(uint64_t deviceId, MTRDeviceConnectionCallback completionHandler);
BOOL MTRGetConnectedDevice(DeviceConnectionCallback completionHandler);
BOOL MTRGetConnectedDeviceWithID(uint64_t deviceId, DeviceConnectionCallback completionHandler);
void MTRUnpairDeviceWithID(uint64_t deviceId);
MTRBaseDevice * _Nullable MTRGetDeviceBeingCommissioned(void);

Expand Down
18 changes: 12 additions & 6 deletions src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,32 +129,38 @@ uint64_t MTRGetLastPairedDeviceId(void)
return deviceId;
}

BOOL MTRGetConnectedDevice(MTRDeviceConnectionCallback completionHandler)
BOOL MTRGetConnectedDevice(DeviceConnectionCallback completionHandler)
{
MTRDeviceController * controller = InitializeMTR();
InitializeMTR();

// Let's use the last device that was paired
uint64_t deviceId = MTRGetLastPairedDeviceId();
return [controller getBaseDevice:deviceId queue:dispatch_get_main_queue() completion:completionHandler];

return MTRGetConnectedDeviceWithID(deviceId, completionHandler);
}

MTRBaseDevice * MTRGetDeviceBeingCommissioned(void)
{
NSError * error;
MTRDeviceController * controller = InitializeMTR();
MTRBaseDevice * device = [controller getDeviceBeingCommissioned:MTRGetLastPairedDeviceId() error:&error];
MTRBaseDevice * device = [controller deviceBeingCommissionedWithNodeID:@(MTRGetLastPairedDeviceId()) error:&error];
if (error) {
NSLog(@"Error retrieving device being commissioned for deviceId %llu", MTRGetLastPairedDeviceId());
return nil;
}
return device;
}

BOOL MTRGetConnectedDeviceWithID(uint64_t deviceId, MTRDeviceConnectionCallback completionHandler)
BOOL MTRGetConnectedDeviceWithID(uint64_t deviceId, DeviceConnectionCallback completionHandler)
{
MTRDeviceController * controller = InitializeMTR();

return [controller getBaseDevice:deviceId queue:dispatch_get_main_queue() completion:completionHandler];
// We can simplify this now that devices can be gotten sync, but for now just do the async dispatch.
dispatch_async(dispatch_get_main_queue(), ^{
__auto_type * device = [MTRBaseDevice deviceWithNodeID:@(deviceId) controller:controller];
completionHandler(device, nil);
});
return YES;
}

BOOL MTRIsDevicePaired(uint64_t deviceId)
Expand Down
Loading

0 comments on commit 5757068

Please sign in to comment.