Address API review issues in MTRDeviceController. (#22596)

* 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.

examples/darwin-framework-tool/commands/clusters/ModelCommandBridge.mm

@@ -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;
 }
 

examples/darwin-framework-tool/commands/pairing/Commands.h

@@ -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:
@@ -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);

examples/darwin-framework-tool/commands/pairing/OpenCommissioningWindowCommand.mm

@@ -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) {

examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.h

@@ -24,7 +24,6 @@ enum class PairingMode
 {
  None,
  Code,
- Ethernet,
  Ble,
 };
 
@@ -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);
@@ -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();
 
@@ -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;
 };

examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.mm

@@ -66,9 +66,6 @@
  case PairingMode::Code:
  PairWithPayload(&error);
  break;
- case PairingMode::Ethernet:
- PairWithIPAddress(&error);
- break;
  case PairingMode::Ble:
  PairWithCode(&error);
  break;
@@ -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);
+ }];
+ }];
 }

examples/darwin-framework-tool/commands/pairing/PairingDelegateBridge.mm

@@ -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;

examples/darwin-framework-tool/commands/tests/TestCommandBridge.h

@@ -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;
  }
 
@@ -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;
  }
@@ -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);

scripts/tests/chiptest/lsan-mac-suppressions.txt

@@ -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:]

src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.h

@@ -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);
@@ -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);
 

src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.m

@@ -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)