Skip to content

Commit

Permalink
[Darwin] MTRDevice writes and commands should be serialized along wit…
Browse files Browse the repository at this point in the history 
…h reads (#23141)

* [Darwin] MTRDevice writes and commands should be serialized along with reads

* Add _Nullable in implementation to match header for better documentation

* Added back MTRDevice -newBaseDevice

* Added header doc changes about nullable expected values and expected values interval

* Implementation of clamping timeout values according to header intention and spec

* Unlinked timed invoke and expected value interval. Unrelated, but kept the range clamping since having NSNumber values exceeding ranges would result in undefined / unexpected values

* Address review comments

* Clarification on expectedValues and expectedValueInterval

* Typo

* Update src/darwin/Framework/CHIP/MTRDevice.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/darwin/Framework/CHIP/MTRDevice.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
@jtung-apple @bzbarsky-apple @pull
2 people authored and pull[bot] committed Nov 19, 2022
1 parent 17586cf commit 2219508
Show file tree
Hide file tree
Showing 5 changed files with 8,805 additions and 5,612 deletions.
14 changes: 10 additions & 4 deletions src/darwin/Framework/CHIP/MTRDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ typedef NS_ENUM(NSUInteger, MTRDeviceState) {
* MTRDeviceResponseHandler.
*
* @param expectedValueInterval maximum interval in milliseconds during which reads of the attribute will return the value being
* written. This value will be clamped to timeoutMs
* written. This value must be within [1, UINT32_MAX], and will be clamped to this range.
*
* TODO: document that -readAttribute... will return the expected value for the [endpoint,cluster,attribute] until one of the
* following:
Expand All @@ -96,7 +96,8 @@ typedef NS_ENUM(NSUInteger, MTRDeviceState) {
* 3. We succeed at writing the attribute.
* 4. We fail at writing the attribute and give up on the write
*
* @param timeout timeout in milliseconds for timed write, or nil.
* @param timeout timeout in milliseconds for timed write, or nil. This value must be within [1, UINT16_MAX], and will be clamped
* to this range.
* TODO: make timeout arguments uniform
*/
- (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
Expand All @@ -117,13 +118,18 @@ typedef NS_ENUM(NSUInteger, MTRDeviceState) {
* @param expectedValues array of dictionaries containing the expected values in the same format as
* attribute read completion handler. Requires MTRAttributePathKey values.
* See MTRDeviceResponseHandler definition for dictionary details.
* The expectedValues and expectedValueInterval arguments need to be both
* nil or both non-nil, or both will be both ignored.
*
* TODO: document better the expectedValues is how this command is expected to change attributes when read, and that the next
* readAttribute will get these values
*
* @param expectedValueInterval maximum interval in milliseconds during which reads of the attribute will return the value being
* written. This value will be clamped to timeout
* written. If the value is less than 1, both this value and expectedValues will be ignored.
If this value is greater than UINT32_MAX, it will be clamped to UINT32_MAX.
*
* @param timeout timeout in milliseconds for timed invoke, or nil.
* @param timeout timeout in milliseconds for timed invoke, or nil. This value must be within [1, UINT16_MAX], and will be clamped
* to this range.
*
* @param completion response handler will receive either values or error.
*/
Expand Down
111 changes: 74 additions & 37 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ - (id)strongObject
}
@end

NSNumber * MTRClampedNumber(NSNumber * aNumber, NSNumber * min, NSNumber * max)
{
if ([aNumber compare:min] == NSOrderedAscending) {
return min;
} else if ([aNumber compare:max] == NSOrderedDescending) {
return max;
}
return aNumber;
}

#pragma mark - SubscriptionCallback class declaration
using namespace chip;
using namespace chip::app;
Expand Down Expand Up @@ -136,8 +146,6 @@ @interface MTRDevice ()
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, MTRPair<NSDate *, NSDictionary *> *> * expectedValueCache;

@property (nonatomic) BOOL expirationCheckScheduled;

@property (nonatomic) MTRAsyncCallbackWorkQueue * asyncCallbackWorkQueue;
@end

@implementation MTRDevice
Expand Down Expand Up @@ -382,7 +390,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
readAttributeWithEndpointId:endpointID
Expand All @@ -398,7 +406,7 @@ - (void)subscribeWithMinInterval:(uint16_t)minInterval maxInterval:(uint16_t)max
}

// TODO: better retry logic
if (retryCount < 2) {
if (error && (retryCount < 2)) {
[workItem retryWork];
} else {
[workItem endWork];
Expand All @@ -424,20 +432,29 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
expectedValueInterval:(NSNumber *)expectedValueInterval
timedWriteTimeout:(NSNumber * _Nullable)timeout
{
// Start the asynchronous operation
MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.nodeID controller:self.deviceController];
[baseDevice
writeAttributeWithEndpointId:endpointID
clusterId:clusterID
attributeId:attributeID
value:value
timedWriteTimeout:timeout
clientQueue:self.queue
completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
if (values) {
[self _handleAttributeReport:values];
}
}];
if (timeout) {
timeout = MTRClampedNumber(timeout, @(1), @(UINT16_MAX));
}
expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTRBaseDevice * baseDevice = [self newBaseDevice];
[baseDevice
writeAttributeWithEndpointId:endpointID
clusterId:clusterID
attributeId:attributeID
value:value
timedWriteTimeout:timeout
clientQueue:self.queue
completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
if (values) {
[self _handleAttributeReport:values];
}
[workItem endWork];
}];
};
workItem.readyHandler = readyHandler;
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

// Commit change into expected value cache
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointId:endpointID
Expand All @@ -452,28 +469,43 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
clusterID:(NSNumber *)clusterID
commandID:(NSNumber *)commandID
commandFields:(id)commandFields
expectedValues:(NSArray<NSDictionary<NSString *, id> *> *)expectedValues
expectedValueInterval:(NSNumber *)expectedValueInterval
expectedValues:(NSArray<NSDictionary<NSString *, id> *> * _Nullable)expectedValues
expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval
timedInvokeTimeout:(NSNumber * _Nullable)timeout
clientQueue:(dispatch_queue_t)clientQueue
completion:(MTRDeviceResponseHandler)completion
{
// Perform this operation
MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.nodeID controller:self.deviceController];
[baseDevice
invokeCommandWithEndpointId:endpointID
clusterId:clusterID
commandId:commandID
commandFields:commandFields
timedInvokeTimeout:timeout
clientQueue:self.queue
completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
dispatch_async(clientQueue, ^{
completion(values, error);
});
}];

[self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval];
if (timeout) {
timeout = MTRClampedNumber(timeout, @(1), @(UINT16_MAX));
}
if (!expectedValueInterval || ([expectedValueInterval compare:@(0)] == NSOrderedAscending)) {
expectedValues = nil;
} else {
expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
}
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTRBaseDevice * baseDevice = [self newBaseDevice];
[baseDevice
invokeCommandWithEndpointId:endpointID
clusterId:clusterID
commandId:commandID
commandFields:commandFields
timedInvokeTimeout:timeout
clientQueue:self.queue
completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
dispatch_async(clientQueue, ^{
completion(values, error);
});
[workItem endWork];
}];
};
workItem.readyHandler = readyHandler;
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

if (expectedValues) {
[self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval];
}
}

- (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
Expand All @@ -482,7 +514,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 @@ -702,6 +734,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: 6 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice_Internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);

@property (nonatomic, readonly, strong, nonnull) MTRDeviceController * deviceController;
@property (nonatomic, readonly) uint64_t nodeID;
@property (nonatomic, readonly) MTRAsyncCallbackWorkQueue * asyncCallbackWorkQueue;

@end

#pragma mark - Utility for clamping numbers
// Returns a NSNumber object that is aNumber if it falls within the range [min, max].
// Returns min or max, if it is below or above, respectively.
NSNumber * MTRClampedNumber(NSNumber * aNumber, NSNumber * min, NSNumber * max);

NS_ASSUME_NONNULL_END
114 changes: 65 additions & 49 deletions src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#import <Foundation/Foundation.h>

#import "MTRAttributeCacheContainer_Internal.h"
#import "MTRAsyncCallbackWorkQueue.h"
#import "MTRBaseDevice_internal.h"
#import "MTRClusterConstants.h"
#import "MTRClusters_internal.h"
Expand Down Expand Up @@ -52,56 +53,71 @@ 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];
new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
baseDevice,
{{#if hasSpecificResponse}}
{{! This treats completionHandler as taking an id for the data. This is
not great from a type-safety perspective, of course. }}
completionHandler,
{{else}}
{{! For now, don't change the bridge API; instead just use an adapter
to invoke our completion handler. This is not great from a
type-safety perspective, of course. }}
^(id _Nullable value, NSError * _Nullable error) {
completionHandler(error);
},
{{/if}}
^(ExchangeManager & exchangeManager, const SessionHandle & session, Cancelable * success, Cancelable * failure) {
chip::Optional<uint16_t> timedInvokeTimeoutMs;
ListFreer listFreer;
{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Type request;
if (params != nil) {
if (params.timedInvokeTimeoutMs != nil) {
timedInvokeTimeoutMs.SetValue(params.timedInvokeTimeoutMs.unsignedShortValue);
}
}
{{#if mustUseTimedInvoke}}
if (!timedInvokeTimeoutMs.HasValue()) {
timedInvokeTimeoutMs.SetValue(10000);
}
{{/if}}
{{#chip_cluster_command_arguments}}
{{#first}}
{{#unless (commandHasRequiredField parent)}}
if (params != nil) {
{{/unless}}
{{/first}}
{{>encode_value target=(concat "request." (asLowerCamelCase label)) source=(concat "params." (asStructPropertyName label)) cluster=parent.parent.name errorCode="return CHIP_ERROR_INVALID_ARGUMENT;" depth=0}}
{{#last}}
{{#unless (commandHasRequiredField parent)}}
NSNumber *timedInvokeTimeoutMsParam = params.timedInvokeTimeoutMs;
if (timedInvokeTimeoutMsParam) {
timedInvokeTimeoutMsParam = MTRClampedNumber(timedInvokeTimeoutMsParam, @(1), @(UINT16_MAX));
}
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.callbackQueue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.device.nodeID controller:self.device.deviceController];
new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
baseDevice,
{{#if hasSpecificResponse}}
{{! This treats completionHandler as taking an id for the data. This is
not great from a type-safety perspective, of course. }}
completionHandler,
{{else}}
{{! For now, don't change the bridge API; instead just use an adapter
to invoke our completion handler. This is not great from a
type-safety perspective, of course. }}
^(id _Nullable value, NSError * _Nullable error) {
completionHandler(error);
[workItem endWork];
},
{{/if}}
^(ExchangeManager & exchangeManager, const SessionHandle & session, Cancelable * success, Cancelable * failure) {
chip::Optional<uint16_t> timedInvokeTimeoutMs;
ListFreer listFreer;
{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Type request;
if (timedInvokeTimeoutMsParam != nil) {
timedInvokeTimeoutMs.SetValue(timedInvokeTimeoutMsParam.unsignedShortValue);
}
{{/unless}}
{{/last}}
{{/chip_cluster_command_arguments}}

auto successFn = Callback<{{>callbackName}}CallbackType>::FromCancelable(success);
auto failureFn = Callback<DefaultFailureCallbackType>::FromCancelable(failure);
chip::Controller::{{asUpperCamelCase parent.name}}Cluster cppCluster(exchangeManager, session, self->_endpoint);
return cppCluster.InvokeCommand(request, successFn->mContext, successFn->mCall, failureFn->mCall, timedInvokeTimeoutMs);
});

[self.device setExpectedValues:expectedValues expectedValueInterval:expectedValueIntervalMs];
{{#if mustUseTimedInvoke}}
if (!timedInvokeTimeoutMs.HasValue()) {
timedInvokeTimeoutMs.SetValue(10000);
}
{{/if}}
{{#chip_cluster_command_arguments}}
{{#first}}
{{#unless (commandHasRequiredField parent)}}
if (params != nil) {
{{/unless}}
{{/first}}
{{>encode_value target=(concat "request." (asLowerCamelCase label)) source=(concat "params." (asStructPropertyName label)) cluster=parent.parent.name errorCode="return CHIP_ERROR_INVALID_ARGUMENT;" depth=0}}
{{#last}}
{{#unless (commandHasRequiredField parent)}}
}
{{/unless}}
{{/last}}
{{/chip_cluster_command_arguments}}

auto successFn = Callback<{{>callbackName}}CallbackType>::FromCancelable(success);
auto failureFn = Callback<DefaultFailureCallbackType>::FromCancelable(failure);
chip::Controller::{{asUpperCamelCase parent.name}}Cluster cppCluster(exchangeManager, session, self->_endpoint);
return cppCluster.InvokeCommand(request, successFn->mContext, successFn->mCall, failureFn->mCall, timedInvokeTimeoutMs);
});
};
workItem.readyHandler = readyHandler;
[self.device.asyncCallbackWorkQueue enqueueWorkItem:workItem];

if (!expectedValueIntervalMs || ([expectedValueIntervalMs compare:@(0)] == NSOrderedAscending)) {
expectedValues = nil;
} else {
expectedValueIntervalMs = MTRClampedNumber(expectedValueIntervalMs, @(1), @(UINT32_MAX));
}
if (expectedValues) {
[self.device setExpectedValues:expectedValues expectedValueInterval:expectedValueIntervalMs];
}
}
{{/chip_cluster_commands}}

Expand Down
Loading

0 comments on commit 2219508

Please sign in to comment.