Controller suspend/resume should stop/start operational advertising as needed.

Author: bzbarsky-apple

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 <MTRStorage>

src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h

@@ -32,6 +32,7 @@
 #import "MTRDeviceControllerFactory.h"
 #import "MTROperationalBrowser.h"
 
+#include <credentials/FabricTable.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
 #include <lib/core/DataModelTypes.h>
 #include <lib/core/PeerId.h>
@@ -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

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<chip::VendorId>([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

src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

@@ -16,6 +16,7 @@
 
 #import <Matter/Matter.h>
 
+#import <dns_sd.h>
 #import <os/lock.h>
 
 #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,26 @@ - (void)test012_startSuspended
     [controller setupCommissioningSessionWithPayload:payload newNodeID:@(17) error:&error];
     XCTAssertNotNil(error);
 
+    [self waitForExpectations:@[ initialAdvertisingExpectation ] timeout:kTimeoutInSeconds];
+
     [controller 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 +1877,9 @@ - (void)test013_suspendDevices
         }
     });
 
+    XCTestExpectation * advertisingStoppedExpectation = [self expectationWithDescription:@"Controller stopped advertising"];
+    operationalBrowser.removedExpectation = advertisingStoppedExpectation;
+
     [controller suspend];
     XCTAssertTrue(controller.suspended);
 
@@ -1767,7 +1890,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 +1913,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 +1940,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