Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down
24 changes: 23 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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];
}
}
});
}

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

Expand Down Expand Up @@ -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
Expand Down
134 changes: 132 additions & 2 deletions src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#import <Matter/Matter.h>

#import <dns_sd.h>
#import <os/lock.h>

#import "MTRDeviceControllerLocalTestStorage.h"
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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]
Expand All @@ -1675,6 +1785,7 @@ - (void)test012_startSuspended
caseAuthenticatedTags:nil
paramsModifier:^(MTRDeviceControllerExternalCertificateParameters * params) {
params.startSuspended = YES;
params.shouldAdvertiseOperational = YES;
}
error:&error];

Expand All @@ -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];

Expand Down Expand Up @@ -1757,6 +1879,9 @@ - (void)test013_suspendDevices
}
});

XCTestExpectation * advertisingStoppedExpectation = [self expectationWithDescription:@"Controller stopped advertising"];
operationalBrowser.removedExpectation = advertisingStoppedExpectation;

[controller suspend];
XCTAssertTrue(controller.suspended);

Expand All @@ -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"];
Expand All @@ -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);

Expand All @@ -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
Expand Down
Loading