Skip to content

Commit

Permalink
Address post-landing review comments from PR 22521. (#22843)
Browse files Browse the repository at this point in the history
The comments came in after that PR merged.

* Store NSNumber as the NodeID in MTRDevice.
* Have a helper in MTRDevice for creating MTRBaseDevice instances.
  • Loading branch information
bzbarsky-apple authored Sep 28, 2022
1 parent 2c8dab2 commit 5d140df
Show file tree
Hide file tree
Showing 5 changed files with 203 additions and 384 deletions.
17 changes: 11 additions & 6 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle
{
if (self = [super init]) {
_lock = OS_UNFAIR_LOCK_INIT;
_nodeID = [nodeID unsignedLongLongValue];
_nodeID = [nodeID copy];
_deviceController = controller;
_queue = dispatch_queue_create("com.apple.matter.framework.xpc.workqueue", DISPATCH_QUEUE_SERIAL);
;
Expand Down Expand Up @@ -286,7 +286,7 @@ - (void)subscribeWithMinInterval:(uint16_t)minInterval maxInterval:(uint16_t)max

_subscriptionActive = YES;

[_deviceController getSessionForNode:_nodeID
[_deviceController getSessionForNode:[_nodeID unsignedLongLongValue]
completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
if (error != nil) {
Expand Down Expand Up @@ -369,7 +369,7 @@ - (void)subscribeWithMinInterval:(uint16_t)minInterval maxInterval:(uint16_t)max
// Create work item, set ready handler to perform task, then enqueue the work
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
MTRBaseDevice * baseDevice = [self newBaseDevice];

[baseDevice readAttributePathWithEndpointID:endpointID
clusterID:clusterID
Expand Down Expand Up @@ -412,7 +412,7 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
timedWriteTimeout:(NSNumber * _Nullable)timeout
{
// Start the asynchronous operation
MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
MTRBaseDevice * baseDevice = [self newBaseDevice];
[baseDevice
writeAttributeWithEndpointID:endpointID
clusterID:clusterID
Expand Down Expand Up @@ -446,7 +446,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
completion:(MTRDeviceResponseHandler)completion
{
// Perform this operation
MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
MTRBaseDevice * baseDevice = [self newBaseDevice];
[baseDevice
invokeCommandWithEndpointID:endpointID
clusterID:clusterID
Expand All @@ -469,7 +469,7 @@ - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
queue:(dispatch_queue_t)queue
completion:(MTRDeviceOpenCommissioningWindowHandler)completion
{
auto * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
auto * baseDevice = [self newBaseDevice];
[baseDevice openCommissioningWindowWithSetupPasscode:setupPasscode
discriminator:discriminator
duration:duration
Expand Down Expand Up @@ -689,6 +689,11 @@ - (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values expe
os_unfair_lock_unlock(&self->_lock);
}

- (MTRBaseDevice *)newBaseDevice
{
return [[MTRBaseDevice alloc] initWithNodeID:self.nodeID controller:self.deviceController];
}

@end

#pragma mark - SubscriptionCallback
Expand Down
6 changes: 3 additions & 3 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -524,11 +524,11 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
- (void)removeDevice:(MTRDevice *)device
{
os_unfair_lock_lock(&_deviceMapLock);
MTRDevice * deviceToRemove = self.nodeIDToDeviceMap[@(device.nodeID)];
MTRDevice * deviceToRemove = self.nodeIDToDeviceMap[device.nodeID];
if (deviceToRemove == device) {
self.nodeIDToDeviceMap[@(device.nodeID)] = nil;
self.nodeIDToDeviceMap[device.nodeID] = nil;
} else {
MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, device.nodeID);
MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, [device.nodeID unsignedLongLongValue]);
}
os_unfair_lock_unlock(&_deviceMapLock);
}
Expand Down
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);
- (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values
expectedValueInterval:(NSNumber *)expectedValueIntervalMs;

@property (nonatomic, readonly, strong, nonnull) MTRDeviceController * deviceController;
@property (nonatomic, readonly) uint64_t nodeID;
@property (nonatomic, readonly) MTRDeviceController * deviceController;
@property (nonatomic, readonly, copy) NSNumber * nodeID;

@end

Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ using chip::SessionHandle;
{
// Make a copy of params before we go async.
params = [params copy];
MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.device.nodeID) controller:self.device.deviceController];
MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.device.nodeID controller:self.device.deviceController];
new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
baseDevice,
{{#if hasSpecificResponse}}
Expand Down
Loading

0 comments on commit 5d140df

Please sign in to comment.