Skip to content

Commit 9a7921b

Browse files
Controller suspend/resume should stop/start operational advertising as needed.
1 parent aa69085 commit 9a7921b

4 files changed

+179
-3
lines changed

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

+16
Original file line numberDiff line numberDiff line change
@@ -1244,6 +1244,22 @@ - (MTROperationalBrowser *)operationalBrowser
12441244
return _operationalBrowser.get();
12451245
}
12461246

1247+
- (FabricTable * _Nullable)fabricTable
1248+
{
1249+
assertChipStackLockedByCurrentThread();
1250+
1251+
if (_controllerFactory == nullptr) {
1252+
return nullptr;
1253+
}
1254+
1255+
auto systemState = _controllerFactory->GetSystemState();
1256+
if (systemState == nullptr) {
1257+
return nullptr;
1258+
}
1259+
1260+
return systemState->Fabrics();
1261+
}
1262+
12471263
@end
12481264

12491265
@interface MTRDummyStorage : NSObject <MTRStorage>

src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h

+8
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#import "MTRDeviceControllerFactory.h"
3333
#import "MTROperationalBrowser.h"
3434

35+
#include <credentials/FabricTable.h>
3536
#include <lib/core/CHIPPersistentStorageDelegate.h>
3637
#include <lib/core/DataModelTypes.h>
3738
#include <lib/core/PeerId.h>
@@ -111,6 +112,13 @@ MTR_DIRECT_MEMBERS
111112
@property (readonly) chip::Credentials::GroupDataProvider * groupDataProvider;
112113
@property (readonly, assign) MTROperationalBrowser * operationalBrowser;
113114

115+
// fabricTable must be gotten on the Matter queue. May return null if there are
116+
// no controllers running.
117+
@property (readonly, nullable, assign) chip::FabricTable * fabricTable;
118+
119+
// resetOperationalAdvertising must happen on the Matter queue.
120+
- (void)resetOperationalAdvertising;
121+
114122
@end
115123

116124
MTR_DIRECT_MEMBERS

src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm

+23-1
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ @interface MTRDeviceController_Concrete ()
113113

114114
@property (nonatomic, readonly) MTRDeviceStorageBehaviorConfiguration * storageBehaviorConfiguration;
115115

116+
// Whether we should be advertising our operational identity when we are not suspended.
117+
@property (nonatomic, readonly) BOOL shouldAdvertiseOperational;
118+
116119
@end
117120

118121
@implementation MTRDeviceController_Concrete {
@@ -358,6 +361,15 @@ - (void)_controllerSuspended
358361
MTRDeviceControllerFactory * factory = _factory;
359362
dispatch_async(_chipWorkQueue, ^{
360363
factory.operationalBrowser->ControllerDeactivated();
364+
365+
if (self.shouldAdvertiseOperational) {
366+
auto * fabricTable = factory.fabricTable;
367+
if (fabricTable) {
368+
// We don't care about errors here. If our fabric is gone, nothing to do.
369+
fabricTable->SetShouldAdvertiseIdentity(self->_storedFabricIndex, chip::FabricTable::AdvertiseIdentity::No);
370+
[factory resetOperationalAdvertising];
371+
}
372+
}
361373
});
362374
}
363375

@@ -366,6 +378,15 @@ - (void)_controllerResumed
366378
MTRDeviceControllerFactory * factory = _factory;
367379
dispatch_async(_chipWorkQueue, ^{
368380
factory.operationalBrowser->ControllerActivated();
381+
382+
if (self.shouldAdvertiseOperational) {
383+
auto * fabricTable = factory.fabricTable;
384+
if (fabricTable) {
385+
// We don't care about errors here. If our fabric is gone, nothing to do.
386+
fabricTable->SetShouldAdvertiseIdentity(self->_storedFabricIndex, chip::FabricTable::AdvertiseIdentity::Yes);
387+
[factory resetOperationalAdvertising];
388+
}
389+
}
369390
});
370391
}
371392

@@ -668,7 +689,8 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
668689
commissionerParams.controllerNOC = noc;
669690
}
670691
commissionerParams.controllerVendorId = static_cast<chip::VendorId>([startupParams.vendorID unsignedShortValue]);
671-
commissionerParams.enableServerInteractions = startupParams.advertiseOperational;
692+
_shouldAdvertiseOperational = startupParams.advertiseOperational;
693+
commissionerParams.enableServerInteractions = !self.suspended && self.shouldAdvertiseOperational;
672694

673695
// We never want plain "removal" from the fabric table since this leaves
674696
// the in-memory state out of sync with what's in storage. In per-controller

src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

+132-2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#import <Matter/Matter.h>
1818

19+
#import <dns_sd.h>
1920
#import <os/lock.h>
2021

2122
#import "MTRDeviceControllerLocalTestStorage.h"
@@ -207,6 +208,109 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo
207208

208209
@end
209210

211+
@interface MTRPerControllerStorageTestsOperationalBrowser : NSObject
212+
213+
// expectedNodeID should be a 16-char uppercase hex string encoding the 64-bit
214+
// node ID.
215+
- (instancetype)initWithNodeID:(NSString *)expectedNodeID;
216+
- (void)shutdown;
217+
218+
@property (nonatomic, readwrite) NSString * expectedNodeID;
219+
@property (nonatomic, readwrite, nullable) XCTestExpectation * addedExpectation;
220+
@property (nonatomic, readwrite, nullable) XCTestExpectation * removedExpectation;
221+
222+
- (void)onBrowse:(DNSServiceFlags)flags error:(DNSServiceErrorType)error instanceName:(const char *)instanceName;
223+
224+
@end
225+
226+
static void OnBrowse(DNSServiceRef serviceRef, DNSServiceFlags flags, uint32_t interfaceId,
227+
DNSServiceErrorType error, const char * name, const char * type, const char * domain, void * context)
228+
{
229+
__auto_type * self = (__bridge MTRPerControllerStorageTestsOperationalBrowser *) context;
230+
[self onBrowse:flags error:error instanceName:name];
231+
}
232+
233+
@implementation MTRPerControllerStorageTestsOperationalBrowser {
234+
DNSServiceRef _browseRef;
235+
}
236+
237+
- (instancetype)initWithNodeID:(NSString *)expectedNodeID
238+
{
239+
if (!(self = [super init])) {
240+
return nil;
241+
}
242+
243+
_expectedNodeID = expectedNodeID;
244+
245+
__auto_type queue = dispatch_get_main_queue();
246+
247+
__auto_type err = DNSServiceBrowse(&_browseRef, /* DNSServiceFlags = */ 0, kDNSServiceInterfaceIndexAny, "_matter._tcp", "local.", OnBrowse, (__bridge void *) self);
248+
XCTAssertEqual(err, kDNSServiceErr_NoError);
249+
if (err != kDNSServiceErr_NoError) {
250+
return nil;
251+
}
252+
253+
err = DNSServiceSetDispatchQueue(_browseRef, queue);
254+
XCTAssertEqual(err, kDNSServiceErr_NoError);
255+
if (err != kDNSServiceErr_NoError) {
256+
DNSServiceRefDeallocate(_browseRef);
257+
_browseRef = nil;
258+
return nil;
259+
}
260+
261+
return self;
262+
}
263+
264+
- (void)shutdown
265+
{
266+
if (_browseRef) {
267+
DNSServiceRefDeallocate(_browseRef);
268+
_browseRef = nil;
269+
}
270+
}
271+
272+
- (void)onBrowse:(DNSServiceFlags)flags error:(DNSServiceErrorType)error instanceName:(const char *)instanceName
273+
{
274+
XCTAssertEqual(error, kDNSServiceErr_NoError);
275+
if (error != kDNSServiceErr_NoError) {
276+
DNSServiceRefDeallocate(_browseRef);
277+
_browseRef = nil;
278+
return;
279+
}
280+
281+
__auto_type len = strlen(instanceName);
282+
XCTAssertEqual(len, 33); // Matter instance names are 33 chars.
283+
284+
if (len != 33) {
285+
return;
286+
}
287+
288+
// Skip over compressed fabric id and dash.
289+
// TODO: Consider checking the compressed fabric ID? That's a bit hard to
290+
// do, in general, since it depends on our keys.
291+
const char * nodeID = &instanceName[17];
292+
NSString * browsedNode = [NSString stringWithUTF8String:nodeID];
293+
if (![browsedNode isEqual:self.expectedNodeID]) {
294+
return;
295+
}
296+
297+
if (flags & kDNSServiceFlagsAdd) {
298+
if (self.addedExpectation) {
299+
XCTestExpectation * expectation = self.addedExpectation;
300+
self.addedExpectation = nil;
301+
[expectation fulfill];
302+
}
303+
} else {
304+
if (self.removedExpectation) {
305+
XCTestExpectation * expectation = self.removedExpectation;
306+
self.removedExpectation = nil;
307+
[expectation fulfill];
308+
}
309+
}
310+
}
311+
312+
@end
313+
210314
@interface MTRPerControllerStorageTests : MTRTestCase
211315
@end
212316

@@ -1665,6 +1769,12 @@ - (void)test011_TestDataStoreMTRDeviceWithStorageBehaviorOptimizationDisabled
16651769
// suspension tests to a different file.
16661770
- (void)test012_startSuspended
16671771
{
1772+
// Needs to match the 888 == 0x378 for the node ID below.
1773+
__auto_type * operationalBrowser = [[MTRPerControllerStorageTestsOperationalBrowser alloc] initWithNodeID:@"0000000000000378"];
1774+
XCTestExpectation * initialAdvertisingExpectation = [self expectationWithDescription:@"Controller advertising initially"];
1775+
operationalBrowser.addedExpectation = initialAdvertisingExpectation;
1776+
initialAdvertisingExpectation.inverted = YES; // We should not in fact advertise, since we are suspended.
1777+
16681778
NSError * error;
16691779
__auto_type * storageDelegate = [[MTRTestPerControllerStorage alloc] initWithControllerID:[NSUUID UUID]];
16701780
__auto_type * controller = [self startControllerWithRootKeys:[[MTRTestKeys alloc] init]
@@ -1675,6 +1785,7 @@ - (void)test012_startSuspended
16751785
caseAuthenticatedTags:nil
16761786
paramsModifier:^(MTRDeviceControllerExternalCertificateParameters * params) {
16771787
params.startSuspended = YES;
1788+
params.shouldAdvertiseOperational = YES;
16781789
}
16791790
error:&error];
16801791

@@ -1691,17 +1802,28 @@ - (void)test012_startSuspended
16911802
[controller setupCommissioningSessionWithPayload:payload newNodeID:@(17) error:&error];
16921803
XCTAssertNotNil(error);
16931804

1805+
[self waitForExpectations:@[ initialAdvertisingExpectation ] timeout:kTimeoutInSeconds];
1806+
16941807
[controller shutdown];
1808+
1809+
[operationalBrowser shutdown];
16951810
}
16961811

16971812
- (void)test013_suspendDevices
16981813
{
1814+
// getMTRDevice uses "123" for the node ID of the controller, which is hex 0x7B
1815+
__auto_type * operationalBrowser = [[MTRPerControllerStorageTestsOperationalBrowser alloc] initWithNodeID:@"000000000000007B"];
1816+
XCTestExpectation * initialAdvertisingExpectation = [self expectationWithDescription:@"Controller advertising initially"];
1817+
operationalBrowser.addedExpectation = initialAdvertisingExpectation;
1818+
16991819
NSNumber * deviceID = @(17);
17001820
__auto_type * device = [self getMTRDevice:deviceID];
17011821
__auto_type * controller = device.deviceController;
17021822

17031823
XCTAssertFalse(controller.suspended);
17041824

1825+
[self waitForExpectations:@[ initialAdvertisingExpectation ] timeout:kTimeoutInSeconds];
1826+
17051827
__auto_type queue = dispatch_get_main_queue();
17061828
__auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];
17071829

@@ -1757,6 +1879,9 @@ - (void)test013_suspendDevices
17571879
}
17581880
});
17591881

1882+
XCTestExpectation * advertisingStoppedExpectation = [self expectationWithDescription:@"Controller stopped advertising"];
1883+
operationalBrowser.removedExpectation = advertisingStoppedExpectation;
1884+
17601885
[controller suspend];
17611886
XCTAssertTrue(controller.suspended);
17621887

@@ -1767,7 +1892,7 @@ - (void)test013_suspendDevices
17671892
[toggle2Expectation fulfill];
17681893
}];
17691894

1770-
[self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation ] timeout:kTimeoutInSeconds];
1895+
[self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation, advertisingStoppedExpectation ] timeout:kTimeoutInSeconds];
17711896

17721897
XCTestExpectation * newSubscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up again"];
17731898
XCTestExpectation * newReachableExpectation = [self expectationWithDescription:@"Device became reachable again"];
@@ -1790,10 +1915,13 @@ - (void)test013_suspendDevices
17901915
}
17911916
});
17921917

1918+
XCTestExpectation * advertisingResumedExpectation = [self expectationWithDescription:@"Controller resumed advertising"];
1919+
operationalBrowser.addedExpectation = advertisingResumedExpectation;
1920+
17931921
[controller resume];
17941922
XCTAssertFalse(controller.suspended);
17951923

1796-
[self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation ] timeout:kSubscriptionTimeoutInSeconds];
1924+
[self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation, advertisingResumedExpectation ] timeout:kSubscriptionTimeoutInSeconds];
17971925

17981926
MTRSetLogCallback(MTRLogTypeProgress, nil);
17991927

@@ -1814,6 +1942,8 @@ - (void)test013_suspendDevices
18141942
ResetCommissionee(baseDevice, queue, self, kTimeoutInSeconds);
18151943

18161944
[controller shutdown];
1945+
1946+
[operationalBrowser shutdown];
18171947
}
18181948

18191949
// TODO: This might want to go in a separate test file, with some shared setup

0 commit comments

Comments
 (0)