From 99a866d48884c45da63e997de8c162c57cd6c20e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 18 Sep 2024 12:10:05 -0400 Subject: [PATCH] Controller suspend/resume should stop/start operational advertising as needed. (#35635) --- .../CHIP/MTRDeviceControllerFactory.mm | 16 +++ .../MTRDeviceControllerFactory_Internal.h | 8 ++ .../CHIP/MTRDeviceController_Concrete.mm | 24 +++- .../CHIPTests/MTRPerControllerStorageTests.m | 134 +++++++++++++++++- 4 files changed, 179 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index e47415880cdb13..ee0f330b26cfc2 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1244,6 +1244,22 @@ - (MTROperationalBrowser *)operationalBrowser return _operationalBrowser.get(); } +- (FabricTable * _Nullable)fabricTable +{ + assertChipStackLockedByCurrentThread(); + + if (_controllerFactory == nullptr) { + return nullptr; + } + + auto systemState = _controllerFactory->GetSystemState(); + if (systemState == nullptr) { + return nullptr; + } + + return systemState->Fabrics(); +} + @end @interface MTRDummyStorage : NSObject diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index 58592b6ed4b670..7c59a65dba42c2 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -32,6 +32,7 @@ #import "MTRDeviceControllerFactory.h" #import "MTROperationalBrowser.h" +#include #include #include #include @@ -111,6 +112,13 @@ MTR_DIRECT_MEMBERS @property (readonly) chip::Credentials::GroupDataProvider * groupDataProvider; @property (readonly, assign) MTROperationalBrowser * operationalBrowser; +// fabricTable must be gotten on the Matter queue. May return null if there are +// no controllers running. +@property (readonly, nullable, assign) chip::FabricTable * fabricTable; + +// resetOperationalAdvertising must happen on the Matter queue. +- (void)resetOperationalAdvertising; + @end MTR_DIRECT_MEMBERS diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 0cbb8b7e95d8c4..97f47a50faca06 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -113,6 +113,9 @@ @interface MTRDeviceController_Concrete () @property (nonatomic, readonly) MTRDeviceStorageBehaviorConfiguration * storageBehaviorConfiguration; +// Whether we should be advertising our operational identity when we are not suspended. +@property (nonatomic, readonly) BOOL shouldAdvertiseOperational; + @end @implementation MTRDeviceController_Concrete { @@ -358,6 +361,15 @@ - (void)_controllerSuspended MTRDeviceControllerFactory * factory = _factory; dispatch_async(_chipWorkQueue, ^{ factory.operationalBrowser->ControllerDeactivated(); + + if (self.shouldAdvertiseOperational) { + auto * fabricTable = factory.fabricTable; + if (fabricTable) { + // We don't care about errors here. If our fabric is gone, nothing to do. + fabricTable->SetShouldAdvertiseIdentity(self->_storedFabricIndex, chip::FabricTable::AdvertiseIdentity::No); + [factory resetOperationalAdvertising]; + } + } }); } @@ -366,6 +378,15 @@ - (void)_controllerResumed MTRDeviceControllerFactory * factory = _factory; dispatch_async(_chipWorkQueue, ^{ factory.operationalBrowser->ControllerActivated(); + + if (self.shouldAdvertiseOperational) { + auto * fabricTable = factory.fabricTable; + if (fabricTable) { + // We don't care about errors here. If our fabric is gone, nothing to do. + fabricTable->SetShouldAdvertiseIdentity(self->_storedFabricIndex, chip::FabricTable::AdvertiseIdentity::Yes); + [factory resetOperationalAdvertising]; + } + } }); } @@ -668,7 +689,8 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams commissionerParams.controllerNOC = noc; } commissionerParams.controllerVendorId = static_cast([startupParams.vendorID unsignedShortValue]); - commissionerParams.enableServerInteractions = startupParams.advertiseOperational; + _shouldAdvertiseOperational = startupParams.advertiseOperational; + commissionerParams.enableServerInteractions = !self.suspended && self.shouldAdvertiseOperational; // We never want plain "removal" from the fabric table since this leaves // the in-memory state out of sync with what's in storage. In per-controller diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 096349c985b84f..e94863d4f50080 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -16,6 +16,7 @@ #import +#import #import #import "MTRDeviceControllerLocalTestStorage.h" @@ -207,6 +208,109 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo @end +@interface MTRPerControllerStorageTestsOperationalBrowser : NSObject + +// expectedNodeID should be a 16-char uppercase hex string encoding the 64-bit +// node ID. +- (instancetype)initWithNodeID:(NSString *)expectedNodeID; +- (void)shutdown; + +@property (nonatomic, readwrite) NSString * expectedNodeID; +@property (nonatomic, readwrite, nullable) XCTestExpectation * addedExpectation; +@property (nonatomic, readwrite, nullable) XCTestExpectation * removedExpectation; + +- (void)onBrowse:(DNSServiceFlags)flags error:(DNSServiceErrorType)error instanceName:(const char *)instanceName; + +@end + +static void OnBrowse(DNSServiceRef serviceRef, DNSServiceFlags flags, uint32_t interfaceId, + DNSServiceErrorType error, const char * name, const char * type, const char * domain, void * context) +{ + __auto_type * self = (__bridge MTRPerControllerStorageTestsOperationalBrowser *) context; + [self onBrowse:flags error:error instanceName:name]; +} + +@implementation MTRPerControllerStorageTestsOperationalBrowser { + DNSServiceRef _browseRef; +} + +- (instancetype)initWithNodeID:(NSString *)expectedNodeID +{ + if (!(self = [super init])) { + return nil; + } + + _expectedNodeID = expectedNodeID; + + __auto_type queue = dispatch_get_main_queue(); + + __auto_type err = DNSServiceBrowse(&_browseRef, /* DNSServiceFlags = */ 0, kDNSServiceInterfaceIndexAny, "_matter._tcp", "local.", OnBrowse, (__bridge void *) self); + XCTAssertEqual(err, kDNSServiceErr_NoError); + if (err != kDNSServiceErr_NoError) { + return nil; + } + + err = DNSServiceSetDispatchQueue(_browseRef, queue); + XCTAssertEqual(err, kDNSServiceErr_NoError); + if (err != kDNSServiceErr_NoError) { + DNSServiceRefDeallocate(_browseRef); + _browseRef = nil; + return nil; + } + + return self; +} + +- (void)shutdown +{ + if (_browseRef) { + DNSServiceRefDeallocate(_browseRef); + _browseRef = nil; + } +} + +- (void)onBrowse:(DNSServiceFlags)flags error:(DNSServiceErrorType)error instanceName:(const char *)instanceName +{ + XCTAssertEqual(error, kDNSServiceErr_NoError); + if (error != kDNSServiceErr_NoError) { + DNSServiceRefDeallocate(_browseRef); + _browseRef = nil; + return; + } + + __auto_type len = strlen(instanceName); + XCTAssertEqual(len, 33); // Matter instance names are 33 chars. + + if (len != 33) { + return; + } + + // Skip over compressed fabric id and dash. + // TODO: Consider checking the compressed fabric ID? That's a bit hard to + // do, in general, since it depends on our keys. + const char * nodeID = &instanceName[17]; + NSString * browsedNode = [NSString stringWithUTF8String:nodeID]; + if (![browsedNode isEqual:self.expectedNodeID]) { + return; + } + + if (flags & kDNSServiceFlagsAdd) { + if (self.addedExpectation) { + XCTestExpectation * expectation = self.addedExpectation; + self.addedExpectation = nil; + [expectation fulfill]; + } + } else { + if (self.removedExpectation) { + XCTestExpectation * expectation = self.removedExpectation; + self.removedExpectation = nil; + [expectation fulfill]; + } + } +} + +@end + @interface MTRPerControllerStorageTests : MTRTestCase @end @@ -1665,6 +1769,12 @@ - (void)test011_TestDataStoreMTRDeviceWithStorageBehaviorOptimizationDisabled // suspension tests to a different file. - (void)test012_startSuspended { + // Needs to match the 888 == 0x378 for the node ID below. + __auto_type * operationalBrowser = [[MTRPerControllerStorageTestsOperationalBrowser alloc] initWithNodeID:@"0000000000000378"]; + XCTestExpectation * initialAdvertisingExpectation = [self expectationWithDescription:@"Controller advertising initially"]; + operationalBrowser.addedExpectation = initialAdvertisingExpectation; + initialAdvertisingExpectation.inverted = YES; // We should not in fact advertise, since we are suspended. + NSError * error; __auto_type * storageDelegate = [[MTRTestPerControllerStorage alloc] initWithControllerID:[NSUUID UUID]]; __auto_type * controller = [self startControllerWithRootKeys:[[MTRTestKeys alloc] init] @@ -1675,6 +1785,7 @@ - (void)test012_startSuspended caseAuthenticatedTags:nil paramsModifier:^(MTRDeviceControllerExternalCertificateParameters * params) { params.startSuspended = YES; + params.shouldAdvertiseOperational = YES; } error:&error]; @@ -1691,17 +1802,28 @@ - (void)test012_startSuspended [controller setupCommissioningSessionWithPayload:payload newNodeID:@(17) error:&error]; XCTAssertNotNil(error); + [self waitForExpectations:@[ initialAdvertisingExpectation ] timeout:kTimeoutInSeconds]; + [controller shutdown]; + + [operationalBrowser shutdown]; } - (void)test013_suspendDevices { + // getMTRDevice uses "123" for the node ID of the controller, which is hex 0x7B + __auto_type * operationalBrowser = [[MTRPerControllerStorageTestsOperationalBrowser alloc] initWithNodeID:@"000000000000007B"]; + XCTestExpectation * initialAdvertisingExpectation = [self expectationWithDescription:@"Controller advertising initially"]; + operationalBrowser.addedExpectation = initialAdvertisingExpectation; + NSNumber * deviceID = @(17); __auto_type * device = [self getMTRDevice:deviceID]; __auto_type * controller = device.deviceController; XCTAssertFalse(controller.suspended); + [self waitForExpectations:@[ initialAdvertisingExpectation ] timeout:kTimeoutInSeconds]; + __auto_type queue = dispatch_get_main_queue(); __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init]; @@ -1757,6 +1879,9 @@ - (void)test013_suspendDevices } }); + XCTestExpectation * advertisingStoppedExpectation = [self expectationWithDescription:@"Controller stopped advertising"]; + operationalBrowser.removedExpectation = advertisingStoppedExpectation; + [controller suspend]; XCTAssertTrue(controller.suspended); @@ -1767,7 +1892,7 @@ - (void)test013_suspendDevices [toggle2Expectation fulfill]; }]; - [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation ] timeout:kTimeoutInSeconds]; + [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation, advertisingStoppedExpectation ] timeout:kTimeoutInSeconds]; XCTestExpectation * newSubscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up again"]; XCTestExpectation * newReachableExpectation = [self expectationWithDescription:@"Device became reachable again"]; @@ -1790,10 +1915,13 @@ - (void)test013_suspendDevices } }); + XCTestExpectation * advertisingResumedExpectation = [self expectationWithDescription:@"Controller resumed advertising"]; + operationalBrowser.addedExpectation = advertisingResumedExpectation; + [controller resume]; XCTAssertFalse(controller.suspended); - [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation ] timeout:kSubscriptionTimeoutInSeconds]; + [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation, advertisingResumedExpectation ] timeout:kSubscriptionTimeoutInSeconds]; MTRSetLogCallback(MTRLogTypeProgress, nil); @@ -1814,6 +1942,8 @@ - (void)test013_suspendDevices ResetCommissionee(baseDevice, queue, self, kTimeoutInSeconds); [controller shutdown]; + + [operationalBrowser shutdown]; } // TODO: This might want to go in a separate test file, with some shared setup