Skip to content

Commit

Permalink
Add a continuous browse for Matter operational advertisements on Darw…
Browse files Browse the repository at this point in the history
…in. (#25317)

The information is propagated to MTRDevice, but we're not making use of it there
yet.

The fabricIndex changes were due to a pre-existing issue: we are in fact getting
it from the wrong thread/queue in various places (MTRDevice init, MTRDevice
command invocation), such that an assertChipStackLockedByCurrentThread() in the
getter failed.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Oct 23, 2023
1 parent c47dbca commit 2548789
Show file tree
Hide file tree
Showing 9 changed files with 268 additions and 4 deletions.
11 changes: 11 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,17 @@ - (void)invalidate
os_unfair_lock_unlock(&self->_lock);
}

- (void)nodeMayBeAdvertisingOperational
{
// TODO: Figure out what to do with that information. If we're not waiting
// to subscribe/resubscribe, do nothing, otherwise perhaps trigger the
// subscribe/resubscribe immediately? We need to have much better tracking
// of our internal state for that, and may need to add something on
// ReadClient to cancel its outstanding timer and try to resubscribe
// immediately....
MTR_LOG_DEFAULT("%@ saw new operational advertisement", self);
}

// assume lock is held
- (void)_changeState:(MTRDeviceState)state
{
Expand Down
43 changes: 40 additions & 3 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,13 @@
#include <credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h>
#include <credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h>
#include <lib/core/CHIPVendorIdentifiers.hpp>
#include <platform/LockTracker.h>
#include <platform/PlatformManager.h>
#include <setup_payload/ManualSetupPayloadGenerator.h>
#include <system/SystemClock.h>

#include <atomic>

#import <os/lock.h>

static NSString * const kErrorCommissionerInit = @"Init failure while initializing a commissioner";
Expand Down Expand Up @@ -82,7 +85,10 @@
typedef id (^SyncWorkQueueBlockWithReturnValue)(void);
typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void);

@interface MTRDeviceController ()
@interface MTRDeviceController () {
// Atomic because it can be touched from multiple threads.
std::atomic<chip::FabricIndex> _storedFabricIndex;
}

// queue used to serialize all work performed by the MTRDeviceController
@property (atomic, readonly) dispatch_queue_t chipWorkQueue;
Expand Down Expand Up @@ -123,6 +129,8 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dis
if ([self checkForInitError:(_operationalCredentialsDelegate != nullptr) logMsg:kErrorOperationalCredentialsInit]) {
return nil;
}

_storedFabricIndex = chip::kUndefinedFabricIndex;
}
return self;
}
Expand Down Expand Up @@ -152,12 +160,15 @@ - (void)cleanupAfterStartup
// in a very specific way that only MTRDeviceControllerFactory knows about.
- (void)shutDownCppController
{
assertChipStackLockedByCurrentThread();

if (_cppCommissioner) {
auto * commissionerToShutDown = _cppCommissioner;
// Flag ourselves as not running before we start shutting down
// _cppCommissioner, so we're not in a state where we claim to be
// running but are actually partially shut down.
_cppCommissioner = nullptr;
_storedFabricIndex = chip::kUndefinedFabricIndex;
commissionerToShutDown->Shutdown();
delete commissionerToShutDown;
if (_operationalCredentialsDelegate != nil) {
Expand Down Expand Up @@ -345,6 +356,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
return;
}

self->_storedFabricIndex = fabricIdx;
commissionerInitialized = YES;
});

Expand Down Expand Up @@ -813,17 +825,26 @@ - (BOOL)syncRunOnWorkQueueWithBoolReturnValue:(SyncWorkQueueBlockWithBoolReturnV

- (chip::FabricIndex)fabricIndex
{
return _storedFabricIndex;
}

- (nullable NSNumber *)compressedFabricID
{
assertChipStackLockedByCurrentThread();

if (!_cppCommissioner) {
return chip::kUndefinedFabricIndex;
return nil;
}

return _cppCommissioner->GetFabricIndex();
return @(_cppCommissioner->GetCompressedFabricId());
}

- (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
fabricIndex:(chip::FabricIndex)fabricIndex
isRunning:(BOOL *)isRunning
{
assertChipStackLockedByCurrentThread();

if (![self isRunning]) {
*isRunning = NO;
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -861,6 +882,22 @@ - (void)invalidateCASESessionForNode:(chip::NodeId)nodeID;
[self syncRunOnWorkQueue:block error:nil];
}

- (void)operationalInstanceAdded:(chip::NodeId)nodeID
{
// Don't use deviceForNodeID here, because we don't want to create the
// device if it does not already exist.
os_unfair_lock_lock(&_deviceMapLock);
MTRDevice * device = self.nodeIDToDeviceMap[@(nodeID)];
os_unfair_lock_unlock(&_deviceMapLock);

if (device == nil) {
return;
}

ChipLogProgress(Controller, "Notifying device about node 0x" ChipLogFormatX64 " advertising", ChipLogValueX64(nodeID));
[device nodeMayBeAdvertisingOperational];
}

@end

/**
Expand Down
24 changes: 24 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#import "MTRFramework.h"
#import "MTRLogging_Internal.h"
#import "MTROTAProviderDelegateBridge.h"
#import "MTROperationalBrowser.h"
#import "MTRP256KeypairBridge.h"
#import "MTRPersistentStorageDelegateBridge.h"
#import "NSDataSpanConversion.h"
Expand Down Expand Up @@ -81,6 +82,7 @@ @interface MTRDeviceControllerFactory ()
@property (readonly) NSMutableArray<MTRDeviceController *> * controllers;
@property (readonly) PersistentStorageOperationalKeystore * keystore;
@property (readonly) Credentials::PersistentStorageOpCertStore * opCertStore;
@property (readonly) MTROperationalBrowser * operationalBrowser;
@property () chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier;

- (BOOL)findMatchingFabric:(FabricTable &)fabricTable
Expand Down Expand Up @@ -643,6 +645,9 @@ - (MTRDeviceController * _Nullable)createController
// Bringing up the first controller. Start the event loop now. If we
// fail to bring it up, its cleanup will stop the event loop again.
chip::DeviceLayer::PlatformMgrImpl().StartEventLoopTask();
dispatch_sync(_chipWorkQueue, ^{
self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue);
});
}

// Add the controller to _controllers now, so if we fail partway through its
Expand Down Expand Up @@ -742,6 +747,10 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller
[_controllers removeObject:controller];

if ([_controllers count] == 0) {
dispatch_sync(_chipWorkQueue, ^{
delete self->_operationalBrowser;
self->_operationalBrowser = nullptr;
});
// That was our last controller. Stop the event loop before it
// shuts down, because shutdown of the last controller will tear
// down most of the world.
Expand Down Expand Up @@ -777,6 +786,21 @@ - (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricI
return nil;
}

- (void)operationalInstanceAdded:(chip::PeerId &)operationalID
{
for (MTRDeviceController * controller in _controllers) {
auto * compressedFabricId = controller.compressedFabricID;
if (compressedFabricId != nil && compressedFabricId.unsignedLongLongValue == operationalID.GetCompressedFabricId()) {
ChipLogProgress(Controller, "Notifying controller at fabric index %u about new operational node 0x" ChipLogFormatX64,
controller.fabricIndex, ChipLogValueX64(operationalID.GetNodeId()));
[controller operationalInstanceAdded:operationalID.GetNodeId()];
}

// Keep going: more than one controller might match a given compressed
// fabric id, though the chances are low.
}
}

- (MTRPersistentStorageDelegateBridge *)storageDelegateBridge
{
return _persistentStorageDelegateBridge;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#import "MTRDeviceControllerFactory.h"

#include <lib/core/DataModelTypes.h>
#include <lib/core/PeerId.h>

class MTRPersistentStorageDelegateBridge;

Expand All @@ -47,6 +48,12 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricIndex)fabricIndex;

/**
* Notify the controller factory that a new operational instance with the given
* compressed fabric id and node id has been observed.
*/
- (void)operationalInstanceAdded:(chip::PeerId &)operationalID;

@property (readonly) MTRPersistentStorageDelegateBridge * storageDelegateBridge;
@property (readonly) chip::Credentials::GroupDataProvider * groupData;
@property (readonly) chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier;
Expand Down
15 changes: 14 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,17 @@ NS_ASSUME_NONNULL_BEGIN

/**
* Will return chip::kUndefinedFabricIndex if we do not have a fabric index.
* This property MUST be gotten from the Matter work queue.
*/
@property (readonly) chip::FabricIndex fabricIndex;

/**
* Will return the compressed fabric id of the fabric if the controller is
* running, else nil.
*
* This property MUST be gotten from the Matter work queue.
*/
@property (readonly, nullable) NSNumber * compressedFabricID;

/**
* Init a newly created controller.
*
Expand Down Expand Up @@ -185,6 +192,12 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID;

/**
* Notify the controller that a new operational instance with the given node id
* and a compressed fabric id that matches this controller has been observed.
*/
- (void)operationalInstanceAdded:(chip::NodeId)nodeID;

#pragma mark - Device-specific data and SDK access
// DeviceController will act as a central repository for this opaque dictionary that MTRDevice manages
- (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID;
Expand Down
5 changes: 5 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);
// called by controller to clean up and shutdown
- (void)invalidate;

// Called by controller when a new operational advertisement for what we think
// is this device's identity has been observed. This could have
// false-positives, for example due to compressed fabric id collisions.
- (void)nodeMayBeAdvertisingOperational;

@property (nonatomic, readonly) MTRDeviceController * deviceController;
@property (nonatomic, readonly, copy) NSNumber * nodeID;
// Queue used for various internal bookkeeping work. In general endWork calls
Expand Down
47 changes: 47 additions & 0 deletions src/darwin/Framework/CHIP/MTROperationalBrowser.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* 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.
*/

#pragma once

#import <Foundation/Foundation.h>
#import <Matter/MTRDeviceControllerFactory.h>
#import <dns_sd.h>

class MTROperationalBrowser
{
public:
// Should be created at a point when the factory starts up the event loop,
// and destroyed when the event loop is stopped.
MTROperationalBrowser(MTRDeviceControllerFactory * aFactory, dispatch_queue_t aQueue);

~MTROperationalBrowser();

private:
static void OnBrowse(DNSServiceRef aServiceRef, DNSServiceFlags aFlags, uint32_t aInterfaceId, DNSServiceErrorType aError,
const char * aName, const char * aType, const char * aDomain, void * aContext);

void TryToStartBrowse();

MTRDeviceControllerFactory * const mDeviceControllerFactory;
dispatch_queue_t mQueue;
DNSServiceRef mBrowseRef;

// If mInitialized is true, mBrowseRef is valid.
bool mInitialized = false;

// If mIsDestroying is true, we're in our destructor, shutting things down.
bool mIsDestroying = false;
};
Loading

0 comments on commit 2548789

Please sign in to comment.