Skip to content

Commit

Permalink
[Darwin] more XPC service tweaks (#35048)
Browse files Browse the repository at this point in the history 
* return `MTRDevice_XPC`s from XPC controller

* more logging

* move shadow property declarations to internal header

* declare `_setupDeviceForNodeID` as common internal device controller method

* prefetchedClusterData is nullable

* fix a few properties that needed raising to base class

* you get a log and you get a log EVERYONE GETS A LOG

* convert device map lock for use in subclasses

* check for optional delegate method impl before calling

* ivar no longer necessary with accessor method

underlying lock is the only state needed

* Restyled by clang-format

* remove more obsolete lock bits from `MTRDeviceController_XPC`

---------

Co-authored-by: Restyled.io <commits@restyled.io>
  • Loading branch information
@kiel-apple @restyled-commits @pull
2 people authored and pull[bot] committed Sep 28, 2024
1 parent 5f87153 commit 7760261
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 42 deletions.
29 changes: 17 additions & 12 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ @implementation MTRDeviceController {
MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
MTRDeviceControllerFactory * _factory;
NSMapTable * _nodeIDToDeviceMap;
os_unfair_lock _deviceMapLock; // protects nodeIDToDeviceMap
os_unfair_lock _underlyingDeviceMapLock;
MTRCommissionableBrowser * _commissionableBrowser;
MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;

Expand All @@ -134,12 +134,17 @@ @implementation MTRDeviceController {
MTRP256KeypairBridge _operationalKeypairBridge;
}

- (os_unfair_lock_t)deviceMapLock
{
return &_underlyingDeviceMapLock;
}

- (instancetype)initForSubclasses
{
if (self = [super init]) {
// nothing, as superclass of MTRDeviceController is NSObject
}

_underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT;
return self;
}

Expand Down Expand Up @@ -250,7 +255,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory

_chipWorkQueue = queue;
_factory = factory;
_deviceMapLock = OS_UNFAIR_LOCK_INIT;
_underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT;
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
_serverEndpoints = [[NSMutableArray alloc] init];
_commissionableBrowser = nil;
Expand Down Expand Up @@ -330,10 +335,10 @@ - (void)cleanupAfterStartup
// while calling out into arbitrary invalidation code, snapshot the list of
// devices before we start invalidating.
MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self);
os_unfair_lock_lock(&_deviceMapLock);
os_unfair_lock_lock(self.deviceMapLock);
NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator];
[_nodeIDToDeviceMap removeAllObjects];
os_unfair_lock_unlock(&_deviceMapLock);
os_unfair_lock_unlock(self.deviceMapLock);

for (MTRDevice * device in devices) {
[device invalidate];
Expand Down Expand Up @@ -634,7 +639,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
[_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary<NSNumber *, NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *> * _Nonnull clusterDataByNode) {
MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast<unsigned long>(clusterDataByNode.count), self->_uniqueIdentifier);

std::lock_guard lock(self->_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
NSMutableArray * deviceList = [NSMutableArray array];
for (NSNumber * nodeID in clusterDataByNode) {
NSDictionary * clusterData = clusterDataByNode[nodeID];
Expand Down Expand Up @@ -1004,7 +1009,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
// If prefetchedClusterData is not provided, load attributes individually from controller data store
- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
{
os_unfair_lock_assert_owner(&_deviceMapLock);
os_unfair_lock_assert_owner(self.deviceMapLock);

MTRDevice * deviceToReturn = [[MTRDevice_Concrete alloc] initWithNodeID:nodeID controller:self];
// If we're not running, don't add the device to our map. That would
Expand Down Expand Up @@ -1043,7 +1048,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N

- (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
{
std::lock_guard lock(_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID];
if (!deviceToReturn) {
deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil];
Expand All @@ -1054,7 +1059,7 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID

- (void)removeDevice:(MTRDevice *)device
{
std::lock_guard lock(_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
auto * nodeID = device.nodeID;
MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID];
if (deviceToRemove == device) {
Expand All @@ -1068,7 +1073,7 @@ - (void)removeDevice:(MTRDevice *)device
#ifdef DEBUG
- (NSDictionary<NSNumber *, NSNumber *> *)unitTestGetDeviceAttributeCounts
{
std::lock_guard lock(_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
NSMutableDictionary<NSNumber *, NSNumber *> * deviceAttributeCounts = [NSMutableDictionary dictionary];
for (NSNumber * nodeID in _nodeIDToDeviceMap) {
deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
Expand Down Expand Up @@ -1508,9 +1513,9 @@ - (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);
os_unfair_lock_lock(self.deviceMapLock);
MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)];
os_unfair_lock_unlock(&_deviceMapLock);
os_unfair_lock_unlock(self.deviceMapLock);

if (device == nil) {
return;
Expand Down
44 changes: 19 additions & 25 deletions src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@
#include <dns_sd.h>
#include <string>

#import <os/lock.h>

typedef void (^SyncWorkQueueBlock)(void);
typedef id (^SyncWorkQueueBlockWithReturnValue)(void);
typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void);
Expand All @@ -103,8 +101,6 @@ @interface MTRDeviceController_Concrete ()
@property (nonatomic, readwrite) NSUUID * uniqueIdentifier;
@property (nonatomic, readonly) dispatch_queue_t chipWorkQueue;
@property (nonatomic, readonly, nullable) MTRDeviceControllerFactory * factory;
@property (nonatomic, readonly, nullable) NSMapTable * nodeIDToDeviceMap;
@property (nonatomic, readonly) os_unfair_lock deviceMapLock;
@property (nonatomic, readonly, nullable) id<MTROTAProviderDelegate> otaProviderDelegate;
@property (nonatomic, readonly, nullable) dispatch_queue_t otaProviderDelegateQueue;
@property (nonatomic, readonly, nullable) MTRCommissionableBrowser * commissionableBrowser;
Expand Down Expand Up @@ -270,11 +266,9 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory

_otaProviderDelegate = otaProviderDelegate;
_otaProviderDelegateQueue = otaProviderDelegateQueue;

_chipWorkQueue = queue;
_factory = factory;
_deviceMapLock = OS_UNFAIR_LOCK_INIT;
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
_serverEndpoints = [[NSMutableArray alloc] init];
_commissionableBrowser = nil;

Expand Down Expand Up @@ -352,10 +346,10 @@ - (void)cleanupAfterStartup
// while calling out into arbitrary invalidation code, snapshot the list of
// devices before we start invalidating.
MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self);
os_unfair_lock_lock(&_deviceMapLock);
NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator];
[_nodeIDToDeviceMap removeAllObjects];
os_unfair_lock_unlock(&_deviceMapLock);
os_unfair_lock_lock(self.deviceMapLock);
NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
[self.nodeIDToDeviceMap removeAllObjects];
os_unfair_lock_unlock(self.deviceMapLock);

for (MTRDevice * device in devices) {
[device invalidate];
Expand Down Expand Up @@ -654,7 +648,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
[_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary<NSNumber *, NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *> * _Nonnull clusterDataByNode) {
MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast<unsigned long>(clusterDataByNode.count), self->_uniqueIdentifier);

std::lock_guard lock(self->_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
NSMutableArray * deviceList = [NSMutableArray array];
for (NSNumber * nodeID in clusterDataByNode) {
NSDictionary * clusterData = clusterDataByNode[nodeID];
Expand Down Expand Up @@ -1024,15 +1018,15 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
// If prefetchedClusterData is not provided, load attributes individually from controller data store
- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
{
os_unfair_lock_assert_owner(&_deviceMapLock);
os_unfair_lock_assert_owner(self.deviceMapLock);

MTRDevice * deviceToReturn = [[MTRDevice alloc] initWithNodeID:nodeID controller:self];
// If we're not running, don't add the device to our map. That would
// create a cycle that nothing would break. Just return the device,
// which will be in exactly the state it would be in if it were created
// while we were running and then we got shut down.
if ([self isRunning]) {
[_nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
[self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
}

if (prefetchedClusterData) {
Expand Down Expand Up @@ -1063,8 +1057,8 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N

- (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
{
std::lock_guard lock(_deviceMapLock);
MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID];
std::lock_guard lock(*self.deviceMapLock);
MTRDevice * deviceToReturn = [self.nodeIDToDeviceMap objectForKey:nodeID];
if (!deviceToReturn) {
deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil];
}
Expand All @@ -1074,12 +1068,12 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID

- (void)removeDevice:(MTRDevice *)device
{
std::lock_guard lock(_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
auto * nodeID = device.nodeID;
MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID];
MTRDevice * deviceToRemove = [self.nodeIDToDeviceMap objectForKey:nodeID];
if (deviceToRemove == device) {
[deviceToRemove invalidate];
[_nodeIDToDeviceMap removeObjectForKey:nodeID];
[self.nodeIDToDeviceMap removeObjectForKey:nodeID];
} else {
MTR_LOG_ERROR("%@ Error: Cannot remove device %p with nodeID %llu", self, device, nodeID.unsignedLongLongValue);
}
Expand All @@ -1088,10 +1082,10 @@ - (void)removeDevice:(MTRDevice *)device
#ifdef DEBUG
- (NSDictionary<NSNumber *, NSNumber *> *)unitTestGetDeviceAttributeCounts
{
std::lock_guard lock(_deviceMapLock);
std::lock_guard lock(*self.deviceMapLock);
NSMutableDictionary<NSNumber *, NSNumber *> * deviceAttributeCounts = [NSMutableDictionary dictionary];
for (NSNumber * nodeID in _nodeIDToDeviceMap) {
deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
for (NSNumber * nodeID in self.nodeIDToDeviceMap) {
deviceAttributeCounts[nodeID] = @([[self.nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]);
}
return deviceAttributeCounts;
}
Expand Down Expand Up @@ -1540,9 +1534,9 @@ - (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 = [_nodeIDToDeviceMap objectForKey:@(nodeID)];
os_unfair_lock_unlock(&_deviceMapLock);
os_unfair_lock_lock(self.deviceMapLock);
MTRDevice * device = [self.nodeIDToDeviceMap objectForKey:@(nodeID)];
os_unfair_lock_unlock(self.deviceMapLock);

if (device == nil) {
return;
Expand Down
6 changes: 6 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
#include <lib/core/CHIPError.h>
#include <lib/core/DataModelTypes.h>

#import <os/lock.h>

#import "MTRBaseDevice.h"
#import "MTRDeviceController.h"
#import "MTRDeviceControllerDataStore.h"
Expand Down Expand Up @@ -63,6 +65,9 @@ NS_ASSUME_NONNULL_BEGIN

@interface MTRDeviceController ()

@property (nonatomic, readwrite, nullable) NSMapTable * nodeIDToDeviceMap;
@property (readonly, assign) os_unfair_lock_t deviceMapLock;

- (instancetype)initForSubclasses;

#pragma mark - MTRDeviceControllerFactory methods
Expand Down Expand Up @@ -270,6 +275,7 @@ NS_ASSUME_NONNULL_BEGIN
#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;
- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData;
- (void)removeDevice:(MTRDevice *)device;

/**
Expand Down
21 changes: 20 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

#import "MTRDeviceController_XPC.h"

#import "MTRDefines_Internal.h"
#import "MTRDeviceController_Internal.h"
#import "MTRDevice_XPC.h"
Expand Down Expand Up @@ -45,7 +46,7 @@ @implementation MTRDeviceController_XPC
- (id)initWithUniqueIdentifier:(NSUUID *)UUID xpConnectionBlock:(NSXPCConnection * (^)(void) )connectionBlock
{
if (self = [super initForSubclasses]) {
MTR_LOG("Setting up XPC Controller for UUID: %@ with connection block: %p", UUID, connectionBlock);
MTR_LOG("Setting up XPC Controller for UUID: %@ with connection block: %p", UUID, connectionBlock);

if (UUID == nil) {
MTR_LOG_ERROR("MTRDeviceController_XPC initWithUniqueIdentifier failed, nil UUID");
Expand Down Expand Up @@ -111,6 +112,24 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete
return nil;
}

// If prefetchedClusterData is not provided, load attributes individually from controller data store
- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)prefetchedClusterData
{
MTR_LOG("%s", __PRETTY_FUNCTION__);
os_unfair_lock_assert_owner(self.deviceMapLock);

MTRDevice * deviceToReturn = [[MTRDevice_XPC alloc] initWithNodeID:nodeID controller:self];
// If we're not running, don't add the device to our map. That would
// create a cycle that nothing would break. Just return the device,
// which will be in exactly the state it would be in if it were created
// while we were running and then we got shut down.
if ([self isRunning]) {
[self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID];
}
MTR_LOG("%s: returning XPC device for node id %@", __PRETTY_FUNCTION__, nodeID);
return deviceToReturn;
}

#pragma mark - XPC Action Overrides

MTR_DEVICECONTROLLER_SIMPLE_REMOTE_XPC_GETTER(isRunning, BOOL, NO, getIsRunningWithReply)
Expand Down
26 changes: 22 additions & 4 deletions src/darwin/Framework/CHIP/MTRDevice_XPC.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,41 +82,59 @@

@implementation MTRDevice_XPC

#pragma mark - Client Callbacks
#pragma mark - Client Callbacks (MTRDeviceDelegate)

// required methods for MTRDeviceDelegates
- (oneway void)device:(NSNumber *)nodeID stateChanged:(MTRDeviceState)state
{
MTR_LOG("%s", __PRETTY_FUNCTION__);
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate device:self stateChanged:state];
}];
}

- (oneway void)device:(NSNumber *)nodeID receivedAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport
{
MTR_LOG("%s", __PRETTY_FUNCTION__);
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate device:self receivedAttributeReport:attributeReport];
}];
}

- (oneway void)device:(NSNumber *)nodeID receivedEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport
{
MTR_LOG("%s", __PRETTY_FUNCTION__);
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate device:self receivedEventReport:eventReport];
}];
}

// optional methods for MTRDeviceDelegates - check for implementation before calling
- (oneway void)deviceBecameActive:(NSNumber *)nodeID
{
MTR_LOG("%s", __PRETTY_FUNCTION__);
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate deviceBecameActive:self];
if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) {
[delegate deviceBecameActive:self];
}
}];
}

- (oneway void)deviceCachePrimed:(NSNumber *)nodeID
{
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate deviceCachePrimed:self];
if ([delegate respondsToSelector:@selector(deviceCachePrimed:)]) {
[delegate deviceCachePrimed:self];
}
}];
}

- (oneway void)deviceConfigurationChanged:(NSNumber *)nodeID
{
[self _callDelegatesWithBlock:^(id<MTRDeviceDelegate> delegate) {
[delegate deviceConfigurationChanged:self];
if ([delegate respondsToSelector:@selector(deviceConfigurationChanged:)]) {
[delegate deviceConfigurationChanged:self];
}
}];
}

Expand Down

0 comments on commit 7760261

Please sign in to comment.