From dd09512b7ac0393c05a4e435427970fd4c65d352 Mon Sep 17 00:00:00 2001 From: Vivien Nicolas Date: Wed, 5 Jul 2023 15:20:24 +0200 Subject: [PATCH] Followup #27318 - Fix review comments that came in after landing (#27483) --- .github/workflows/darwin.yaml | 9 +- .../DiscoverCommissionablesCommand.mm | 39 +-- .../Framework/CHIP/MTRCommissionableBrowser.h | 14 +- .../CHIP/MTRCommissionableBrowser.mm | 128 +++++++--- .../CHIP/MTRCommissionableBrowserDelegate.h | 5 +- .../CHIP/MTRCommissionableBrowserResult.h | 29 ++- .../Framework/CHIP/MTRDeviceController.h | 11 +- .../Framework/CHIP/MTRDeviceController.mm | 34 ++- .../CHIPTests/MTRCommissionableBrowserTests.m | 239 ++++++++++++++++++ .../Matter.xcodeproj/project.pbxproj | 4 + 10 files changed, 437 insertions(+), 75 deletions(-) create mode 100644 src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml index a422c2066a8d98..27e850824bc2c1 100644 --- a/.github/workflows/darwin.yaml +++ b/.github/workflows/darwin.yaml @@ -80,7 +80,10 @@ jobs: # Disable -Wunguarded-availability-new because we internally use # APIs we added after our deployment target version. Maybe we # should change the deployment target version instead? - run: xcodebuild -target "Matter" -sdk macosx OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' + # + # Disable BLE because the app does not have the permission to use + # it and that may crash the CI. + run: xcodebuild -target "Matter" -sdk macosx OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' CHIP_IS_BLE=NO working-directory: src/darwin/Framework - name: Clean Build run: xcodebuild clean @@ -121,10 +124,10 @@ jobs: ../../../out/debug/chip-ota-requestor-app --interface-id -1 --secured-device-port 5543 --discriminator 1112 --KVS /tmp/chip-ota-requestor-kvs2 --otaDownloadPath /tmp/chip-ota-requestor-downloaded-image2 --autoApplyImage > >(tee /tmp/darwin/framework-tests/ota-requestor-app-2.log) 2> >(tee /tmp/darwin/framework-tests/ota-requestor-app-err-2.log >&2) & # -enableUndefinedBehaviorSanitizer instruments the code in Matter.framework, # but to instrument the code in the underlying libCHIP we need to pass CHIP_IS_UBSAN=YES - TEST_RUNNER_ASAN_OPTIONS=__CURRENT_VALUE__:detect_stack_use_after_return=1 xcodebuild test -target "Matter" -scheme "Matter Framework Tests" -sdk macosx -enableAddressSanitizer YES -enableUndefinedBehaviorSanitizer YES OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' CHIP_IS_UBSAN=YES > >(tee /tmp/darwin/framework-tests/darwin-tests-asan.log) 2> >(tee /tmp/darwin/framework-tests/darwin-tests-asan-err.log >&2) + TEST_RUNNER_ASAN_OPTIONS=__CURRENT_VALUE__:detect_stack_use_after_return=1 xcodebuild test -target "Matter" -scheme "Matter Framework Tests" -sdk macosx -enableAddressSanitizer YES -enableUndefinedBehaviorSanitizer YES OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' CHIP_IS_UBSAN=YES CHIP_IS_BLE=NO > >(tee /tmp/darwin/framework-tests/darwin-tests-asan.log) 2> >(tee /tmp/darwin/framework-tests/darwin-tests-asan-err.log >&2) # -enableThreadSanitizer instruments the code in Matter.framework, # but to instrument the code in the underlying libCHIP we need to pass CHIP_IS_TSAN=YES - xcodebuild test -target "Matter" -scheme "Matter Framework Tests" -sdk macosx -enableThreadSanitizer YES OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' CHIP_IS_TSAN=YES > >(tee /tmp/darwin/framework-tests/darwin-tests-tsan.log) 2> >(tee /tmp/darwin/framework-tests/darwin-tests-tsan-err.log >&2) + xcodebuild test -target "Matter" -scheme "Matter Framework Tests" -sdk macosx -enableThreadSanitizer YES OTHER_CFLAGS='${inherited} -Werror -Wconversion -Wno-unguarded-availability-new' CHIP_IS_TSAN=YES CHIP_IS_BLE=NO > >(tee /tmp/darwin/framework-tests/darwin-tests-tsan.log) 2> >(tee /tmp/darwin/framework-tests/darwin-tests-tsan-err.log >&2) working-directory: src/darwin/Framework - name: Build Matter TV Casting Bridge run: | diff --git a/examples/darwin-framework-tool/commands/discover/DiscoverCommissionablesCommand.mm b/examples/darwin-framework-tool/commands/discover/DiscoverCommissionablesCommand.mm index beaf43a954aea0..aef3f3969bee1b 100644 --- a/examples/darwin-framework-tool/commands/discover/DiscoverCommissionablesCommand.mm +++ b/examples/darwin-framework-tool/commands/discover/DiscoverCommissionablesCommand.mm @@ -21,31 +21,32 @@ auto gDispatchQueue = dispatch_queue_create("com.chip.discover", DISPATCH_QUEUE_SERIAL); @interface DeviceScannerDelegate : NSObject -- (void)didDiscoverCommissionable:(MTRCommissionableBrowserResult *)device; -- (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device; +- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device; +- (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevice:(MTRCommissionableBrowserResult *)device; @end @implementation DeviceScannerDelegate -- (void)didDiscoverCommissionable:(MTRCommissionableBrowserResult *)device +- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device { - auto serviceName = device.serviceName; - auto vendorId = device.vendorId; - auto productId = device.productId; + auto instanceName = device.instanceName; + auto vendorId = device.vendorID; + auto productId = device.productID; auto discriminator = device.discriminator; [gDiscoveredDevices addObject:device]; - NSLog(@"Found Device (%@) with discriminator: %@ (vendor: %@, product: %@)", serviceName, discriminator, vendorId, productId); + NSLog(@"Found Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId); } -- (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device +- (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevice:(MTRCommissionableBrowserResult *)device { - auto serviceName = device.serviceName; - auto vendorId = device.vendorId; - auto productId = device.productId; + auto instanceName = device.instanceName; + auto vendorId = device.vendorID; + auto productId = device.productID; auto discriminator = device.discriminator; [gDiscoveredDevices removeObjectIdenticalTo:device]; - NSLog(@"Removed Device (%@) with discriminator: %@ (vendor: %@, product: %@)", serviceName, discriminator, vendorId, productId); + NSLog( + @"Removed Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId); } @end @@ -58,7 +59,7 @@ - (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device }); auto delegate = [[DeviceScannerDelegate alloc] init]; - auto success = [CurrentCommissioner() startScan:delegate queue:gDispatchQueue]; + auto success = [CurrentCommissioner() startBrowseForCommissionables:delegate queue:gDispatchQueue]; VerifyOrReturnError(success, CHIP_ERROR_INTERNAL); SetCommandExitStatus(CHIP_NO_ERROR); @@ -69,7 +70,7 @@ - (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device { VerifyOrReturnError(IsInteractive(), CHIP_ERROR_INCORRECT_STATE); - auto success = [CurrentCommissioner() stopScan]; + auto success = [CurrentCommissioner() stopBrowseForCommissionables]; VerifyOrReturnError(success, CHIP_ERROR_INTERNAL); SetCommandExitStatus(CHIP_NO_ERROR); @@ -86,13 +87,13 @@ - (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device uint16_t index = 0; for (id device in gDiscoveredDevices) { - auto serviceName = [device serviceName]; - auto vendorId = [device vendorId]; - auto productId = [device productId]; + auto instanceName = [device instanceName]; + auto vendorId = [device vendorID]; + auto productId = [device productID]; auto discriminator = [device discriminator]; - NSLog( - @"\t %u %@ - Discriminator: %@ - Vendor: %@ - Product: %@", index, serviceName, discriminator, vendorId, productId); + NSLog(@"\t %u %@ - Discriminator: %@ - Vendor: %@ - Product: %@", index, instanceName, discriminator, vendorId, + productId); index++; } diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.h b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.h index 4ab4b9559b1f2d..1a05c13a59021a 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.h +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.h @@ -21,13 +21,25 @@ NS_ASSUME_NONNULL_BEGIN @protocol MTRCommissionableBrowserDelegate; +@class MTRDeviceController; MTR_HIDDEN @interface MTRCommissionableBrowser : NSObject - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; -- (instancetype)initWithDelegate:(id)delegate queue:(dispatch_queue_t)queue; +- (instancetype)initWithDelegate:(id)delegate + controller:(MTRDeviceController *)controller + queue:(dispatch_queue_t)queue; +/** + * Start browsing the available networks (e.g IP, BLE) for commissionable nodes. + * + * If a browse is already ongoing this will not start a new browse and the return value will be NO. + */ - (BOOL)start; + +/** + * Stop browsing the network for commissionable nodes. + */ - (BOOL)stop; @end diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm index 3d1359895e23c6..c6fd8d8fe6d559 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm @@ -18,6 +18,7 @@ #import "MTRCommissionableBrowser.h" #import "MTRCommissionableBrowserDelegate.h" #import "MTRCommissionableBrowserResult_Internal.h" +#import "MTRDeviceController.h" #import "MTRLogging_Internal.h" #include @@ -30,17 +31,19 @@ #if CONFIG_NETWORK_LAYER_BLE #include using namespace chip::Ble; -constexpr const char * kBleKey = "BLE"; #endif // CONFIG_NETWORK_LAYER_BLE +constexpr const char * kBleKey = "BLE"; + @implementation MTRCommissionableBrowserResultInterfaces @end @interface MTRCommissionableBrowserResult () -@property (nonatomic) NSString * serviceName; -@property (nonatomic) NSNumber * vendorId; -@property (nonatomic) NSNumber * productId; +@property (nonatomic) NSString * instanceName; +@property (nonatomic) NSNumber * vendorID; +@property (nonatomic) NSNumber * productID; @property (nonatomic) NSNumber * discriminator; +@property (nonatomic) BOOL commissioningMode; @end @implementation MTRCommissionableBrowserResult @@ -54,13 +57,17 @@ @implementation MTRCommissionableBrowserResult #endif // CONFIG_NETWORK_LAYER_BLE { public: - CHIP_ERROR Start(id delegate, dispatch_queue_t queue) + CHIP_ERROR Start(id delegate, MTRDeviceController * controller, dispatch_queue_t queue) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mDelegate == nil, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mController == nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDispatchQueue == nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDiscoveredResults == nil, CHIP_ERROR_INCORRECT_STATE); mDelegate = delegate; + mController = controller; mDispatchQueue = queue; mDiscoveredResults = [[NSMutableDictionary alloc] init]; @@ -80,12 +87,19 @@ CHIP_ERROR Start(id delegate, dispatch_queue_t CHIP_ERROR Stop() { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mController != nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDispatchQueue != nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDiscoveredResults != nil, CHIP_ERROR_INCORRECT_STATE); mDelegate = nil; + mController = nil; mDispatchQueue = nil; + + ClearBleDiscoveredDevices(); + ClearDnssdDiscoveredDevices(); mDiscoveredResults = nil; #if CONFIG_NETWORK_LAYER_BLE @@ -95,9 +109,46 @@ CHIP_ERROR Stop() return ChipDnssdStopBrowse(this); } + void ClearBleDiscoveredDevices() + { + NSMutableDictionary * discoveredResultsCopy = @ {}.mutableCopy; + + for (NSString * key in mDiscoveredResults) { + if (![mDiscoveredResults[key].instanceName isEqual:[NSString stringWithUTF8String:kBleKey]]) { + discoveredResultsCopy[key] = mDiscoveredResults[key]; + } + } + + mDiscoveredResults = discoveredResultsCopy; + } + + void ClearDnssdDiscoveredDevices() + { + NSMutableDictionary * discoveredResultsCopy = @ {}.mutableCopy; + + for (NSString * key in mDiscoveredResults) { + auto * interfaces = mDiscoveredResults[key].interfaces; + for (id interfaceKey in interfaces) { + // Check if the interface data has been resolved already, otherwise, just inform the + // back end that we may not need it anymore. + if (!interfaces[interfaceKey].resolutionData.HasValue()) { + ChipDnssdResolveNoLongerNeeded(key.UTF8String); + } + } + + if ([mDiscoveredResults[key].instanceName isEqual:[NSString stringWithUTF8String:kBleKey]]) { + discoveredResultsCopy[key] = mDiscoveredResults[key]; + } + } + + mDiscoveredResults = discoveredResultsCopy; + } + /////////// CommissioningResolveDelegate Interface ///////// void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override { + assertChipStackLockedByCurrentThread(); + auto & commissionData = nodeData.commissionData; auto key = [NSString stringWithUTF8String:commissionData.instanceName]; if ([mDiscoveredResults objectForKey:key] == nil) { @@ -106,13 +157,14 @@ void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override } auto result = mDiscoveredResults[key]; - result.serviceName = key; - result.vendorId = @(static_cast(commissionData.vendorId)); - result.productId = @(commissionData.productId); + result.instanceName = key; + result.vendorID = @(commissionData.vendorId); + result.productID = @(commissionData.productId); result.discriminator = @(commissionData.longDiscriminator); + result.commissioningMode = commissionData.commissioningMode != 0; auto & resolutionData = nodeData.resolutionData; - auto interfaces = result.interfaces; + auto * interfaces = result.interfaces; interfaces[@(resolutionData.interfaceId.GetPlatformInterface())].resolutionData = chip::MakeOptional(resolutionData); // Check if any interface for the advertised service has been resolved already. If so, @@ -132,20 +184,26 @@ void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override } dispatch_async(mDispatchQueue, ^{ - [mDelegate didDiscoverCommissionable:result]; + [mDelegate controller:mController didFindCommissionableDevice:result]; }); } /////////// DnssdBrowseDelegate Interface ///////// void OnBrowseAdd(DnssdService service) override { + assertChipStackLockedByCurrentThread(); + + VerifyOrReturn(mDelegate != nil); + VerifyOrReturn(mController != nil); + VerifyOrReturn(mDispatchQueue != nil); + auto key = [NSString stringWithUTF8String:service.mName]; if ([mDiscoveredResults objectForKey:key] == nil) { mDiscoveredResults[key] = [[MTRCommissionableBrowserResult alloc] init]; mDiscoveredResults[key].interfaces = [[NSMutableDictionary alloc] init]; } - auto interfaces = mDiscoveredResults[key].interfaces; + auto * interfaces = mDiscoveredResults[key].interfaces; auto interfaceKey = @(service.mInterface.GetPlatformInterface()); interfaces[interfaceKey] = [[MTRCommissionableBrowserResultInterfaces alloc] init]; @@ -154,6 +212,12 @@ void OnBrowseAdd(DnssdService service) override void OnBrowseRemove(DnssdService service) override { + assertChipStackLockedByCurrentThread(); + + VerifyOrReturn(mDelegate != nil); + VerifyOrReturn(mController != nil); + VerifyOrReturn(mDispatchQueue != nil); + auto key = [NSString stringWithUTF8String:service.mName]; if ([mDiscoveredResults objectForKey:key] == nil) { // It should not happens. @@ -161,7 +225,7 @@ void OnBrowseRemove(DnssdService service) override } auto result = mDiscoveredResults[key]; - auto interfaces = result.interfaces; + auto * interfaces = result.interfaces; auto interfaceKey = @(service.mInterface.GetPlatformInterface()); // Check if the interface data has been resolved already, otherwise, just inform the @@ -177,7 +241,7 @@ void OnBrowseRemove(DnssdService service) override // too and informs the delegate that it is gone. if ([interfaces count] == 0) { dispatch_async(mDispatchQueue, ^{ - [mDelegate commissionableUnavailable:result]; + [mDelegate controller:mController didRemoveCommissionableDevice:result]; }); mDiscoveredResults[key] = nil; @@ -186,39 +250,37 @@ void OnBrowseRemove(DnssdService service) override void OnBrowseStop(CHIP_ERROR error) override { - for (id key in mDiscoveredResults) { - auto interfaces = mDiscoveredResults[key].interfaces; - for (id interfaceKey in interfaces) { - // Check if the interface data has been resolved already, otherwise, just inform the - // back end that we may not need it anymore. - if (!interfaces[interfaceKey].resolutionData.HasValue()) { - ChipDnssdResolveNoLongerNeeded([key UTF8String]); - } - } - } + assertChipStackLockedByCurrentThread(); + + ClearDnssdDiscoveredDevices(); } #if CONFIG_NETWORK_LAYER_BLE /////////// BleScannerDelegate Interface ///////// void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificationInfo & info) override { + assertChipStackLockedByCurrentThread(); + auto result = [[MTRCommissionableBrowserResult alloc] init]; - result.serviceName = [NSString stringWithUTF8String:kBleKey]; - result.vendorId = @(static_cast(info.GetVendorId())); - result.productId = @(info.GetProductId()); + result.instanceName = [NSString stringWithUTF8String:kBleKey]; + result.vendorID = @(info.GetVendorId()); + result.productID = @(info.GetProductId()); result.discriminator = @(info.GetDeviceDiscriminator()); + result.commissioningMode = YES; result.params = chip::MakeOptional(chip::Controller::SetUpCodePairerParameters(connObj, false /* connected */)); auto key = [NSString stringWithFormat:@"%@", connObj]; mDiscoveredResults[key] = result; dispatch_async(mDispatchQueue, ^{ - [mDelegate didDiscoverCommissionable:result]; + [mDelegate controller:mController didFindCommissionableDevice:result]; }); } void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override { + assertChipStackLockedByCurrentThread(); + auto key = [NSString stringWithFormat:@"%@", connObj]; if ([mDiscoveredResults objectForKey:key] == nil) { // It should not happens. @@ -229,7 +291,7 @@ void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override mDiscoveredResults[key] = nil; dispatch_async(mDispatchQueue, ^{ - [mDelegate commissionableUnavailable:result]; + [mDelegate controller:mController didFindCommissionableDevice:result]; }); } #endif // CONFIG_NETWORK_LAYER_BLE @@ -237,20 +299,25 @@ void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override private: dispatch_queue_t mDispatchQueue; id mDelegate; + MTRDeviceController * mController; NSMutableDictionary * mDiscoveredResults; }; @interface MTRCommissionableBrowser () @property (strong, nonatomic) dispatch_queue_t queue; @property (nonatomic, readonly) id delegate; +@property (nonatomic, readonly) MTRDeviceController * controller; @property (unsafe_unretained, nonatomic) CommissionableBrowserInternal browser; @end @implementation MTRCommissionableBrowser -- (instancetype)initWithDelegate:(id)delegate queue:(dispatch_queue_t)queue +- (instancetype)initWithDelegate:(id)delegate + controller:(MTRDeviceController *)controller + queue:(dispatch_queue_t)queue { if (self = [super init]) { _delegate = delegate; + _controller = controller; _queue = queue; } return self; @@ -258,7 +325,7 @@ - (instancetype)initWithDelegate:(id)delegate - (BOOL)start { - VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(_delegate, _queue), NO); + VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(_delegate, _controller, _queue), NO); return YES; } @@ -266,6 +333,7 @@ - (BOOL)stop { VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(), NO); _delegate = nil; + _controller = nil; _queue = nil; return YES; } diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowserDelegate.h b/src/darwin/Framework/CHIP/MTRCommissionableBrowserDelegate.h index 38ed6d3ad8bda6..6dbc04ca5a481c 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowserDelegate.h +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowserDelegate.h @@ -18,6 +18,7 @@ #import @class MTRCommissionableBrowserResult; +@class MTRDeviceController; NS_ASSUME_NONNULL_BEGIN @@ -26,12 +27,12 @@ MTR_NEWLY_AVAILABLE /** * Tells the delegate the commissionable manager discovered a device while scanning for devices. */ -- (void)didDiscoverCommissionable:(MTRCommissionableBrowserResult *)device; +- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device; /** * Tells the delegate a previously discovered device is is no longer available. */ -- (void)commissionableUnavailable:(MTRCommissionableBrowserResult *)device; +- (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevice:(MTRCommissionableBrowserResult *)device; @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowserResult.h b/src/darwin/Framework/CHIP/MTRCommissionableBrowserResult.h index b83a314aa4c8cd..c62e6ea3477654 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowserResult.h +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowserResult.h @@ -22,11 +22,34 @@ NS_ASSUME_NONNULL_BEGIN MTR_NEWLY_AVAILABLE @interface MTRCommissionableBrowserResult : NSObject -@property (readonly, nonatomic) NSString * serviceName; -@property (readonly, nonatomic) NSNumber * vendorId; -@property (readonly, nonatomic) NSNumber * productId; +/** + * For a node advertising over DNS-SD, the instance name is a dynamic, pseudo-randomly selected, 64-bit temporary unique identifier, + * expressed as a fixed-length sixteen-character hexadecimal string, encoded as ASCII text using capital letters. + * + * For a node advertising over Bluetooth Low Energy, the instance name is always "BLE". + */ +@property (readonly, nonatomic) NSString * instanceName; + +/** + * A 16-bit unsigned value identifying the device manufacturer. + */ +@property (readonly, nonatomic) NSNumber * vendorID; + +/** + * A 16-bit unsigned value identifying the product. + */ +@property (readonly, nonatomic) NSNumber * productID; + +/** + * A 12-bit value matching the field of the same name in MTRSetupPayload. + */ @property (readonly, nonatomic) NSNumber * discriminator; +/** + * A boolean indicating whether the device has a commissioning window open. + */ +@property (readonly, nonatomic) BOOL commissioningMode; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h index 84882c65257638..683e584bca5666 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController.h @@ -97,7 +97,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS * @error error indication if the commissioning session establishment can't start at all. * * The connection information for the device will be retrieved from the discovered device. - * A device discovered over MDNS will use the discovered IPs/ports, while a device discovered + * A device discovered over DNS-SD will use the discovered IPs/ports, while a device discovered * over BLE will use the underlying CBPeripheral. * * Then a PASE session will be established with the device, unless an error @@ -177,16 +177,17 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS /** * Start scanning for commissionable devices. * - * This method will fail if the controller factory is not running. + * This method will fail if the controller factory is not running or the browse has already been started. */ -- (BOOL)startScan:(id)delegate queue:(dispatch_queue_t)queue MTR_NEWLY_AVAILABLE; +- (BOOL)startBrowseForCommissionables:(id)delegate + queue:(dispatch_queue_t)queue MTR_NEWLY_AVAILABLE; /** * Stop scanning for commissionable devices. * - * This method will fail if the controller factory is not running or the scan has not been started. + * This method will fail if the controller factory is not running or the browse has not been started. */ -- (BOOL)stopScan MTR_NEWLY_AVAILABLE; +- (BOOL)stopBrowseForCommissionables MTR_NEWLY_AVAILABLE; /** * Return the attestation challenge for the secure session of the device being commissioned. diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 4b7f7fcc7f0f57..59a4c338fc4af6 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -120,6 +120,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dis _factory = factory; _deviceMapLock = OS_UNFAIR_LOCK_INIT; _nodeIDToDeviceMap = [NSMutableDictionary dictionary]; + _commissionableBrowser = nil; _deviceControllerDelegateBridge = new MTRDeviceControllerDelegateBridge(); if ([self checkForInitError:(_deviceControllerDelegateBridge != nullptr) logMsg:kErrorPairingInit]) { @@ -166,7 +167,7 @@ - (void)cleanupAfterStartup [device invalidate]; } [self.nodeIDToDeviceMap removeAllObjects]; - [self stopScan]; + [self stopBrowseForCommissionables]; [_factory controllerShuttingDown:self]; } @@ -465,12 +466,12 @@ - (BOOL)setupCommissioningSessionWithDiscoveredDevice:(MTRCommissionableBrowserR self->_operationalCredentialsDelegate->SetDeviceID(nodeId); auto errorCode = CHIP_ERROR_INVALID_ARGUMENT; - if (discoveredDevice.params.HasValue()) { - auto params = discoveredDevice.params.Value(); - auto pinCode = static_cast([[payload setupPasscode] unsignedLongValue]); - params.SetSetupPINCode(pinCode); + chip::Optional params = discoveredDevice.params; + if (params.HasValue()) { + auto pinCode = static_cast(payload.setupPasscode.unsignedLongValue); + params.Value().SetSetupPINCode(pinCode); - errorCode = self.cppCommissioner->EstablishPASEConnection(nodeId, params); + errorCode = self.cppCommissioner->EstablishPASEConnection(nodeId, params.Value()); } else { // Try to get a QR code if possible (because it has a better // discriminator, etc), then fall back to manual code if that fails. @@ -588,23 +589,31 @@ - (BOOL)cancelCommissioningForNodeID:(NSNumber *)nodeID error:(NSError * __autor return [self syncRunOnWorkQueueWithBoolReturnValue:block error:error]; } -- (BOOL)startScan:(id)delegate queue:(dispatch_queue_t)queue +- (BOOL)startBrowseForCommissionables:(id)delegate queue:(dispatch_queue_t)queue { auto block = ^BOOL { - self->_commissionableBrowser = [[MTRCommissionableBrowser alloc] initWithDelegate:delegate queue:queue]; - return [self.commissionableBrowser start]; + VerifyOrReturnValue(self.commissionableBrowser == nil, NO); + + auto commissionableBrowser = [[MTRCommissionableBrowser alloc] initWithDelegate:delegate controller:self queue:queue]; + VerifyOrReturnValue([commissionableBrowser start], NO); + + self->_commissionableBrowser = commissionableBrowser; + return YES; }; return [self syncRunOnWorkQueueWithBoolReturnValue:block error:nil]; } -- (BOOL)stopScan +- (BOOL)stopBrowseForCommissionables { auto block = ^BOOL { + VerifyOrReturnValue(self.commissionableBrowser != nil, NO); + auto commissionableBrowser = self.commissionableBrowser; - VerifyOrReturnValue(commissionableBrowser, NO); + VerifyOrReturnValue([commissionableBrowser stop], NO); + self->_commissionableBrowser = nil; - return [commissionableBrowser stop]; + return YES; }; return [self syncRunOnWorkQueueWithBoolReturnValue:block error:nil]; @@ -911,6 +920,7 @@ - (void)asyncDispatchToMatterQueue:(dispatch_block_t)block errorHandler:(nullabl - (void)syncRunOnWorkQueue:(SyncWorkQueueBlock)block error:(NSError * __autoreleasing *)error { + VerifyOrDie(!chip::DeviceLayer::PlatformMgrImpl().IsWorkQueueCurrentQueue()); VerifyOrReturn([self checkIsRunning:error]); dispatch_sync(_chipWorkQueue, ^{ diff --git a/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m new file mode 100644 index 00000000000000..9c542b6a996ea7 --- /dev/null +++ b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m @@ -0,0 +1,239 @@ +/** + * + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import + +// system dependencies +#import + +#import "MTRTestKeys.h" +#import "MTRTestStorage.h" + +static const uint16_t kLocalPort = 5541; +static const uint16_t kTestVendorId = 0xFFF1u; +static const uint16_t kTestProductId = 0x8001u; +static const uint16_t kTestDiscriminator1 = 1111u; +static const uint16_t kTestDiscriminator2 = 1112u; +static const uint16_t kTestDiscriminator3 = 3840u; +static const uint16_t kDiscoverDeviceTimeoutInSeconds = 10; +static const uint16_t kExpectedDiscoveredDevicesCount = 3; + +// Singleton controller we use. +static MTRDeviceController * sController = nil; + +@interface DeviceScannerDelegate : NSObject +@property (nonatomic) XCTestExpectation * expectation; +@property (nonatomic) NSNumber * resultsCount; + +- (instancetype)initWithExpectation:(XCTestExpectation *)expectation; +- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device; +- (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevice:(MTRCommissionableBrowserResult *)device; +@end + +@implementation DeviceScannerDelegate +- (instancetype)initWithExpectation:(XCTestExpectation *)expectation +{ + if (!(self = [super init])) { + return nil; + } + + _resultsCount = 0; + _expectation = expectation; + return self; +} + +- (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device +{ + _resultsCount = @(_resultsCount.unsignedLongValue + 1); + if ([_resultsCount isEqual:@(kExpectedDiscoveredDevicesCount)]) { + [self.expectation fulfill]; + } + + XCTAssertLessThanOrEqual(_resultsCount.unsignedLongValue, kExpectedDiscoveredDevicesCount); + + __auto_type instanceName = device.instanceName; + __auto_type vendorId = device.vendorID; + __auto_type productId = device.productID; + __auto_type discriminator = device.discriminator; + __auto_type commissioningMode = device.commissioningMode; + + XCTAssertEqual(instanceName.length, 16); // The instance name is random, so just ensure the len is right. + XCTAssertEqualObjects(vendorId, @(kTestVendorId)); + XCTAssertEqualObjects(productId, @(kTestProductId)); + XCTAssertTrue([discriminator isEqual:@(kTestDiscriminator1)] || [discriminator isEqual:@(kTestDiscriminator2)] || + [discriminator isEqual:@(kTestDiscriminator3)]); + XCTAssertEqual(commissioningMode, YES); + + NSLog(@"Found Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId); +} + +- (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevice:(MTRCommissionableBrowserResult *)device +{ + __auto_type instanceName = device.instanceName; + __auto_type vendorId = device.vendorID; + __auto_type productId = device.productID; + __auto_type discriminator = device.discriminator; + + NSLog( + @"Removed Device (%@) with discriminator: %@ (vendor: %@, product: %@)", instanceName, discriminator, vendorId, productId); +} +@end + +@interface MTRCommissionableBrowserTests : XCTestCase +@end + +static BOOL sStackInitRan = NO; +static BOOL sNeedsStackShutdown = YES; + +@implementation MTRCommissionableBrowserTests + ++ (void)tearDown +{ + // Global teardown, runs once + if (sNeedsStackShutdown) { + [self shutdownStack]; + } +} + +- (void)setUp +{ + // Per-test setup, runs before each test. + [super setUp]; + [self setContinueAfterFailure:NO]; + + if (sStackInitRan == NO) { + [self initStack]; + } +} + +- (void)tearDown +{ + // Per-test teardown, runs after each test. + [super tearDown]; +} + +- (void)initStack +{ + sStackInitRan = YES; + + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type * storage = [[MTRTestStorage alloc] init]; + __auto_type * factoryParams = [[MTRDeviceControllerFactoryParams alloc] initWithStorage:storage]; + factoryParams.port = @(kLocalPort); + + BOOL ok = [factory startControllerFactory:factoryParams error:nil]; + XCTAssertTrue(ok); + + __auto_type * testKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(testKeys); + + __auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithIPK:testKeys.ipk fabricID:@(1) nocSigner:testKeys]; + params.vendorID = @(kTestVendorId); + + MTRDeviceController * controller = [factory createControllerOnNewFabric:params error:nil]; + XCTAssertNotNil(controller); + + sController = controller; +} + ++ (void)shutdownStack +{ + sNeedsStackShutdown = NO; + + MTRDeviceController * controller = sController; + XCTAssertNotNil(controller); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); + + [[MTRDeviceControllerFactory sharedInstance] stopControllerFactory]; +} + +- (void)test001_StartBrowseAndStopBrowse +{ + __auto_type delegate = [[DeviceScannerDelegate alloc] init]; + dispatch_queue_t dispatchQueue = dispatch_queue_create("com.chip.discover", DISPATCH_QUEUE_SERIAL); + + // Start browsing + XCTAssertTrue([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + // Stop browsing + XCTAssertTrue([sController stopBrowseForCommissionables]); +} + +- (void)test002_StartBrowseAndStopBrowseMultipleTimes +{ + __auto_type delegate = [[DeviceScannerDelegate alloc] init]; + dispatch_queue_t dispatchQueue = dispatch_queue_create("com.chip.discover", DISPATCH_QUEUE_SERIAL); + + // Start browsing + XCTAssertTrue([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + // Stop browsing + XCTAssertTrue([sController stopBrowseForCommissionables]); + + // Start browsing a second time + XCTAssertTrue([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + // Stop browsing a second time + XCTAssertTrue([sController stopBrowseForCommissionables]); +} + +- (void)test003_StopBrowseWhileNotBrowsing +{ + // Stop browsing while there is no browse ongoing + XCTAssertFalse([sController stopBrowseForCommissionables]); +} + +- (void)test004_StartBrowseWhileBrowsing +{ + __auto_type delegate = [[DeviceScannerDelegate alloc] init]; + dispatch_queue_t dispatchQueue = dispatch_queue_create("com.chip.discover", DISPATCH_QUEUE_SERIAL); + + // Start browsing + XCTAssertTrue([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + // Start browsing a second time while a browse is ongoing + XCTAssertFalse([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + // Properly stop browsing + XCTAssertTrue([sController stopBrowseForCommissionables]); +} + +- (void)test005_StartBrowseGetCommissionableOverMdns +{ + __auto_type expectation = [self expectationWithDescription:@"Commissionable devices Found"]; + __auto_type delegate = [[DeviceScannerDelegate alloc] initWithExpectation:expectation]; + dispatch_queue_t dispatchQueue = dispatch_queue_create("com.chip.discover", DISPATCH_QUEUE_SERIAL); + + // Start browsing + XCTAssertTrue([sController startBrowseForCommissionables:delegate queue:dispatchQueue]); + + [self waitForExpectations:@[ expectation ] timeout:kDiscoverDeviceTimeoutInSeconds]; + + // Properly stop browsing + XCTAssertTrue([sController stopBrowseForCommissionables]); +} + +- (void)test999_TearDown +{ + [[self class] shutdownStack]; +} + +@end diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj index 03c94bc395960b..7b97bb8c4861e2 100644 --- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj +++ b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj @@ -95,6 +95,7 @@ 1ED276E426C5832500547A89 /* MTRCluster.h in Headers */ = {isa = PBXBuildFile; fileRef = 1ED276E326C5832500547A89 /* MTRCluster.h */; settings = {ATTRIBUTES = (Public, ); }; }; 1EDCE545289049A100E41EC9 /* MTROTAHeader.h in Headers */ = {isa = PBXBuildFile; fileRef = 1EDCE543289049A100E41EC9 /* MTROTAHeader.h */; settings = {ATTRIBUTES = (Public, ); }; }; 1EDCE546289049A100E41EC9 /* MTROTAHeader.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1EDCE544289049A100E41EC9 /* MTROTAHeader.mm */; }; + 1EE0805E2A44875E008A03C2 /* MTRCommissionableBrowserTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 1EE0805C2A448756008A03C2 /* MTRCommissionableBrowserTests.m */; }; 27A53C1727FBC6920053F131 /* MTRAttestationTrustStoreBridge.h in Headers */ = {isa = PBXBuildFile; fileRef = 27A53C1527FBC6920053F131 /* MTRAttestationTrustStoreBridge.h */; }; 27A53C1827FBC6920053F131 /* MTRAttestationTrustStoreBridge.mm in Sources */ = {isa = PBXBuildFile; fileRef = 27A53C1627FBC6920053F131 /* MTRAttestationTrustStoreBridge.mm */; }; 2C1B027A2641DB4E00780EF1 /* MTROperationalCredentialsDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2C1B02782641DB4E00780EF1 /* MTROperationalCredentialsDelegate.mm */; }; @@ -371,6 +372,7 @@ 1ED276E326C5832500547A89 /* MTRCluster.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRCluster.h; sourceTree = ""; }; 1EDCE543289049A100E41EC9 /* MTROTAHeader.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTROTAHeader.h; sourceTree = ""; }; 1EDCE544289049A100E41EC9 /* MTROTAHeader.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTROTAHeader.mm; sourceTree = ""; }; + 1EE0805C2A448756008A03C2 /* MTRCommissionableBrowserTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRCommissionableBrowserTests.m; sourceTree = ""; }; 27A53C1527FBC6920053F131 /* MTRAttestationTrustStoreBridge.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRAttestationTrustStoreBridge.h; sourceTree = ""; }; 27A53C1627FBC6920053F131 /* MTRAttestationTrustStoreBridge.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRAttestationTrustStoreBridge.mm; sourceTree = ""; }; 2C1B02782641DB4E00780EF1 /* MTROperationalCredentialsDelegate.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTROperationalCredentialsDelegate.mm; sourceTree = ""; }; @@ -1098,6 +1100,7 @@ B202529A2459E34F00F97062 /* CHIPTests */ = { isa = PBXGroup; children = ( + 1EE0805C2A448756008A03C2 /* MTRCommissionableBrowserTests.m */, 51189FC72A33ACE900184508 /* TestHelpers */, 3DFCB3282966684500332B35 /* MTRCertificateInfoTests.m */, 99C65E0F267282F1003402F6 /* MTRControllerTests.m */, @@ -1526,6 +1529,7 @@ 99C65E10267282F1003402F6 /* MTRControllerTests.m in Sources */, 1E5801C328941C050033A199 /* MTRTestOTAProvider.m in Sources */, 5A6FEC9D27B5E48900F25F42 /* MTRXPCProtocolTests.m in Sources */, + 1EE0805E2A44875E008A03C2 /* MTRCommissionableBrowserTests.m in Sources */, 51339B1F2A0DA64D00C798C1 /* MTRCertificateValidityTests.m in Sources */, 5173A47929C0E82300F67F48 /* MTRFabricInfoTests.m in Sources */, 3D0C484B29DA4FA0006D811F /* MTRErrorTests.m in Sources */,