From 37d549dc18030014e38e88185ad11cb25c8aa895 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 25 Oct 2024 13:44:11 -0400 Subject: [PATCH] Address review comments. --- src/darwin/Framework/CHIP/MTRDevice.h | 5 +- src/darwin/Framework/CHIP/MTRDevice.mm | 104 ++++++++++++------ .../Framework/CHIPTests/MTRDeviceTests.m | 12 +- 3 files changed, 81 insertions(+), 40 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.h b/src/darwin/Framework/CHIP/MTRDevice.h index 728fda93b9496c..b9429025ddaf04 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.h +++ b/src/darwin/Framework/CHIP/MTRDevice.h @@ -348,11 +348,10 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) * The values to wait for are represented as an array of response-values which * have an MTRAttributePathKey and MTRDataKey. * - * The timeout argument can be nil to indicate no timeout. Otherwise it must - * be an NSTimeInterval wrapped in NSNumber. + * If the provided timeout is larger than 5 minutes, it will be capped to 5 minutes. */ - (void)waitForAttributeValues:(NSArray *> *)values - timeout:(NSNumber * _Nullable)timeout + timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue completion:(void (^)(NSError * _Nullable error))completion MTR_NEWLY_AVAILABLE; diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 0221c33b7b2f29..947a446caf8db6 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -80,25 +80,43 @@ - (BOOL)callDelegateSynchronouslyWithBlock:(void (^)(id))bloc #pragma mark - MTRAttributeValueWaiter -static NSString * const MTRValueSatisfiedKey = @"valueSatisfied"; +// Represents the state of a single attribute being waited for: has the path and +// value being waited for in the response-value and whether the value has been reached. +@interface MTRAwaitedAttributeState : NSObject +@property (nonatomic, assign, readwrite) BOOL valueSatisfied; +@property (nonatomic, retain, readonly) MTRDeviceResponseValueDictionary value; + +- (instancetype)initWithValue:(MTRDeviceResponseValueDictionary)value; +@end + +@implementation MTRAwaitedAttributeState +- (instancetype)initWithValue:(MTRDeviceResponseValueDictionary)value +{ + if (self = [super init]) { + _valueSatisfied = NO; + _value = value; + } + + return self; +} +@end // Represents the data passed to waitForAttributeValues: @interface MTRAttributeValueWaiter : NSObject -// Each entry of "valueExpectations" has two keys: -// * MTRValueSatisfiedKey: whether that value has been satisfied by observed -// attribute values. -// * MTRValueKey: the response-value we are trying to wait for. -@property (nonatomic, retain) NSArray *> * valueExpectations; +@property (nonatomic, retain) NSArray * valueExpectations; @property (nonatomic, retain) dispatch_queue_t queue; @property (nonatomic) MTRStatusCompletion completion; @property (nonatomic, readonly) BOOL allValuesSatisfied; +@property (nonatomic, retain, readwrite, nullable) dispatch_source_t expirationTimer; - (instancetype)initWithValues:(NSArray *> *)values queue:(dispatch_queue_t)queue completion:(MTRStatusCompletion)completion; // Returns YES if after this report the waiter might be done waiting, NO otherwise. - (BOOL)attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRAttributePath *)path byDevice:(MTRDevice *)device; +- (void)cancelTimerIfNeeded; + @end @implementation MTRAttributeValueWaiter @@ -107,9 +125,7 @@ - (instancetype)initWithValues:(NSArray *> *)values if (self = [super init]) { auto * valueExpectations = [NSMutableArray arrayWithCapacity:values.count]; for (NSDictionary * value in values) { - auto * valueExpectation = [NSMutableDictionary dictionaryWithCapacity:2]; - valueExpectation[MTRValueSatisfiedKey] = @(NO); - valueExpectation[MTRValueKey] = value; + auto * valueExpectation = [[MTRAwaitedAttributeState alloc] initWithValue:value]; [valueExpectations addObject:valueExpectation]; } _valueExpectations = valueExpectations; @@ -122,12 +138,11 @@ - (instancetype)initWithValues:(NSArray *> *)values - (BOOL)attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRAttributePath *)path byDevice:(MTRDevice *)device { - for (NSMutableDictionary * valueExpectation in self.valueExpectations) { - NSDictionary * expectedValue = valueExpectation[MTRValueKey]; + for (MTRAwaitedAttributeState * valueExpectation in self.valueExpectations) { + MTRDeviceResponseValueDictionary expectedValue = valueExpectation.value; if ([path isEqual:expectedValue[MTRAttributePathKey]]) { - BOOL valueSatisfiesExpectation = [device _attributeDataValue:value satisfiesValueExpectation:expectedValue[MTRDataKey]]; - valueExpectation[MTRValueSatisfiedKey] = @(valueSatisfiesExpectation); - return valueSatisfiesExpectation; + valueExpectation.valueSatisfied = [device _attributeDataValue:value satisfiesValueExpectation:expectedValue[MTRDataKey]]; + return valueExpectation.valueSatisfied; } } @@ -136,8 +151,8 @@ - (BOOL)attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRA - (BOOL)allValuesSatisfied { - for (NSMutableDictionary * valueExpectation in self.valueExpectations) { - if (![valueExpectation[MTRValueSatisfiedKey] boolValue]) { + for (MTRAwaitedAttributeState * valueExpectation in self.valueExpectations) { + if (!valueExpectation.valueSatisfied) { return NO; } } @@ -145,6 +160,14 @@ - (BOOL)allValuesSatisfied return YES; } +- (void)cancelTimerIfNeeded +{ + if (self.expirationTimer != nil) { + dispatch_source_cancel(self.expirationTimer); + self.expirationTimer = nil; + } +} + @end #pragma mark - MTRDevice @@ -794,10 +817,10 @@ - (BOOL)_attributeDataValue:(MTRDeviceDataValueDictionary)observed satisfiesValu #pragma mark - Handling of waits for attribute values -- (void)waitForAttributeValues:(NSArray *> *)values timeout:(NSNumber * _Nullable)timeout queue:(dispatch_queue_t)queue completion:(void (^)(NSError * _Nullable error))completion +- (void)waitForAttributeValues:(NSArray *)values timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue completion:(void (^)(NSError * _Nullable error))completion { // Check that all the values are in fact attribute response-values. - for (NSDictionary * value in values) { + for (MTRDeviceResponseValueDictionary value in values) { if (!value[MTRAttributePathKey] || !value[MTRDataKey]) { MTR_LOG_ERROR("%@ waitForAttributeValues passed a non-attribute value %@", self, value); dispatch_async(queue, ^{ @@ -814,12 +837,12 @@ - (void)waitForAttributeValues:(NSArray *> *)values [requestPaths addObject:[MTRAttributeRequestPath requestPathWithEndpointID:path.endpoint clusterID:path.cluster attributeID:path.attribute]]; } - NSArray *> * currentValues = [self readAttributePaths:requestPaths]; + NSArray * currentValues = [self readAttributePaths:requestPaths]; std::lock_guard lock(_lock); auto * attributeWaiter = [[MTRAttributeValueWaiter alloc] initWithValues:values queue:queue completion:completion]; - for (NSDictionary * currentValue in currentValues) { + for (MTRDeviceResponseValueDictionary currentValue in currentValues) { // Pretend as if this got reported, for purposes of the attribute // waiter. [attributeWaiter attributeValue:currentValue[MTRDataKey] reportedForPath:currentValue[MTRAttributePathKey] byDevice:self]; @@ -836,17 +859,33 @@ - (void)waitForAttributeValues:(NSArray *> *)values // Otherwise, wait for the values to arrive or our timeout. [self.attributeValueWaiters addObject:attributeWaiter]; - if (timeout != nil) { - mtr_weakify(attributeWaiter); - mtr_weakify(self); - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, static_cast(timeout.doubleValue * NSEC_PER_SEC)), self.queue, ^{ - mtr_strongify(self); - mtr_strongify(attributeWaiter); - if (self != nil && attributeWaiter != nil) { - [self _attributeWaitTimedOut:attributeWaiter]; - } - }); - } + // Clamp timeout to 5 minutes. + timeout = std::min(timeout, 5.0 * 60); + dispatch_source_t timerSource = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self.queue); + attributeWaiter.expirationTimer = timerSource; + + // Set a timer to go off after timeout, and not repeat. + dispatch_source_set_timer(timerSource, dispatch_time(DISPATCH_TIME_NOW, static_cast(timeout * static_cast(NSEC_PER_SEC))), DISPATCH_TIME_FOREVER, + // Allow .5 seconds of leeway; should be plenty, + // in practice. + static_cast(0.5 * static_cast(NSEC_PER_SEC))); + + mtr_weakify(attributeWaiter); + mtr_weakify(self); + mtr_weakify(timerSource); + dispatch_source_set_event_handler(timerSource, ^{ + mtr_strongify(self); + mtr_strongify(attributeWaiter); + mtr_strongify(timerSource); + if (self != nil && attributeWaiter != nil) { + [self _attributeWaitTimedOut:attributeWaiter]; + } else if (timerSource != nil) { + // Make sure to cancel the timer ourselves, if we can't call _attributeWaitTimedOut. + dispatch_source_cancel(timerSource); + } + }); + + dispatch_resume(timerSource); } - (void)_attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRAttributePath *)path @@ -877,6 +916,7 @@ - (void)_attributeWaitTimedOut:(MTRAttributeValueWaiter *)attributeValueWaiter if (![self.attributeValueWaiters containsObject:attributeValueWaiter]) { // Nothing to do here anymore. + [attributeValueWaiter cancelTimerIfNeeded]; return; } @@ -887,6 +927,8 @@ - (void)_attributeWaitTimedOut:(MTRAttributeValueWaiter *)attributeValueWaiter - (void)_notifyAttributeValueWaiter:(MTRAttributeValueWaiter *)attributeValueWaiter withError:(NSError * _Nullable)error { + [attributeValueWaiter cancelTimerIfNeeded]; + [self.attributeValueWaiters removeObject:attributeValueWaiter]; auto completion = attributeValueWaiter.completion; diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 81c577fc571b1e..ef9c783b40fd9c 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1589,7 +1589,7 @@ - (void)test017_TestMTRDeviceBasics // Some quick tests for waitForAttributeValues. First, values that we know // are already there: XCTestExpectation * deviceTypesWaitExpectation = [self expectationWithDescription:@"deviceTypes is already the value we expect"]; - [device waitForAttributeValues:deviceTypes timeout:nil queue:queue completion:^(NSError * _Nullable error) { + [device waitForAttributeValues:deviceTypes timeout:200 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNil(error); [deviceTypesWaitExpectation fulfill]; }]; @@ -1606,7 +1606,7 @@ - (void)test017_TestMTRDeviceBasics }, ]; XCTestExpectation * bogusDeviceTypesWaitExpectation = [self expectationWithDescription:@"bogusDeviceTypes wait should time out"]; - [device waitForAttributeValues:bogusDeviceTypes timeout:@(0.5) queue:queue completion:^(NSError * _Nullable error) { + [device waitForAttributeValues:bogusDeviceTypes timeout:0.5 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNotNil(error); XCTAssertEqual(error.domain, MTRErrorDomain); XCTAssertEqual(error.code, MTRErrorCodeTimeout); @@ -1648,7 +1648,7 @@ - (void)test017_TestMTRDeviceBasics MTRDataKey : writeValue, } ] - timeout:@(0.5) + timeout:0.5 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNotNil(error); XCTAssertEqual(error.domain, MTRErrorDomain); @@ -1709,19 +1709,19 @@ - (void)test017_TestMTRDeviceBasics }; XCTestExpectation * waitingForOnTimeValue1Expectation = [self expectationWithDescription:@"OnTime value is now the expected value"]; - [device waitForAttributeValues:@[ onTimeValueToWaitFor ] timeout:nil queue:queue completion:^(NSError * _Nullable error) { + [device waitForAttributeValues:@[ onTimeValueToWaitFor ] timeout:200 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNil(error); [waitingForOnTimeValue1Expectation fulfill]; }]; XCTestExpectation * waitingForOnTimeValue2Expectation = [self expectationWithDescription:@"OnTime value is now the expected value and first device type is the expected value"]; - [device waitForAttributeValues:@[ onTimeValueToWaitFor, deviceTypes[0] ] timeout:nil queue:queue completion:^(NSError * _Nullable error) { + [device waitForAttributeValues:@[ onTimeValueToWaitFor, deviceTypes[0] ] timeout:200 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNil(error); [waitingForOnTimeValue2Expectation fulfill]; }]; XCTestExpectation * waitingForOnTimeValue3Expectation = [self expectationWithDescription:@"OnTime value is now the expected value and first device type is bogus, or we timed out"]; - [device waitForAttributeValues:@[ onTimeValueToWaitFor, bogusDeviceTypes[0] ] timeout:@(0.5) queue:queue completion:^(NSError * _Nullable error) { + [device waitForAttributeValues:@[ onTimeValueToWaitFor, bogusDeviceTypes[0] ] timeout:0.5 queue:queue completion:^(NSError * _Nullable error) { XCTAssertNotNil(error); XCTAssertEqual(error.domain, MTRErrorDomain); XCTAssertEqual(error.code, MTRErrorCodeTimeout);