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.

Sign up for GitHub

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

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

Merged
merged 11 commits into from
Oct 26, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b2ebdc1
[Darwin] MTRDevice writes and commands should be serialized along wit…
jtung-apple Oct 20, 2022
72b491c
Add _Nullable in implementation to match header for better documentation
jtung-apple Oct 20, 2022
e834221
Added back MTRDevice -newBaseDevice
jtung-apple Oct 20, 2022
da55472
Added header doc changes about nullable expected values and expected …
jtung-apple Oct 24, 2022
4601a1b
Implementation of clamping timeout values according to header intenti…
jtung-apple Oct 24, 2022
3da7b35
Unlinked timed invoke and expected value interval. Unrelated, but kep…
jtung-apple Oct 24, 2022
75a0b17
Address review comments
jtung-apple Oct 24, 2022
77eae9f
Clarification on expectedValues and expectedValueInterval
jtung-apple Oct 26, 2022
331e1b7
Typo
jtung-apple Oct 26, 2022
582f4ba
Update src/darwin/Framework/CHIP/MTRDevice.h
jtung-apple Oct 26, 2022
19edc82
Update src/darwin/Framework/CHIP/MTRDevice.h
jtung-apple Oct 26, 2022
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
13 changes: 9 additions & 4 deletions src/darwin/Framework/CHIP/MTRDevice.h
View file
Open in desktop
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,17 @@ 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.
* This argument is ignored if expectedValueInterval is <= 0.
*
* 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. This value must be within [1, UINT32_MAX], and will be clamped to this range. This argument is ignored if expectedValues
* is nil.
*
* @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
113 changes: 76 additions & 37 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Open in desktop
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];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
[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];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
}
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
[workItem endWork];
}];
};
workItem.readyHandler = readyHandler;
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

// Commit change into expected value cache
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointId:endpointID
Expand All @@ -452,28 +469,45 @@ - (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
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
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) {
if ([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);
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
});
[workItem endWork];
}];
};
workItem.readyHandler = readyHandler;
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

if (expectedValues) {
[self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
}
}

- (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
Expand All @@ -482,7 +516,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 +736,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
Open in desktop
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
Open in desktop
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;
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
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];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved

if ([expectedValueIntervalMs compare:@(0)] == NSOrderedAscending) {
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
expectedValues = nil;
} else {
expectedValueIntervalMs = MTRClampedNumber(expectedValueIntervalMs, @(1), @(UINT32_MAX));
}
if (expectedValues) {
[self.device setExpectedValues:expectedValues expectedValueInterval:expectedValueIntervalMs];
}
}
{{/chip_cluster_commands}}

Expand Down
Loading