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 write should reset expected value on error #25966

Merged
merged 5 commits into from
Apr 14, 2023
Merged

[Darwin] MTRDevice write should reset expected value on error #25966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f767c3
[Darwin] MTRDevice write should reset expected value on error
jtung-apple Apr 4, 2023
24c40ad
Added logic to identify expected values to avoid race
jtung-apple Apr 13, 2023
d6fa772
Fixed expectedValueID for commands
jtung-apple Apr 13, 2023
9cf7283
Better comment doc for expected value cache property
jtung-apple Apr 13, 2023
7eca7bf
More fixes for review comments
jtung-apple Apr 13, 2023
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
239 changes: 172 additions & 67 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,7 @@

// Consider moving utility classes to their own file
#pragma mark - Utility Classes
@interface MTRPair<FirstObjectType, SecondObjectType> : NSObject
+ (instancetype)pairWithFirst:(FirstObjectType)first second:(SecondObjectType)second;
@property (nonatomic, readonly) FirstObjectType first;
@property (nonatomic, readonly) SecondObjectType second;
@end

@implementation MTRPair
- (instancetype)initWithFirst:(id)first second:(id)second
{
if (self = [super init]) {
_first = first;
_second = second;
}
return self;
}
+ (instancetype)pairWithFirst:(id)first second:(id)second
{
return [[self alloc] initWithFirst:first second:second];
}
@end

// This class is for storing weak references in a container
@interface MTRWeakReference<ObjectType> : NSObject
+ (instancetype)weakReferenceWithObject:(ObjectType)object;
- (instancetype)initWithObject:(ObjectType)object;
Expand Down Expand Up @@ -171,6 +151,12 @@ MTREventPriority MTREventPriorityForValidPriorityLevel(chip::app::PriorityLevel
} // anonymous namespace

#pragma mark - MTRDevice
typedef NS_ENUM(NSUInteger, MTRDeviceExpectedValueFieldIndex) {
MTRDeviceExpectedValueFieldExpirationTimeIndex = 0,
MTRDeviceExpectedValueFieldValueIndex = 1,
MTRDeviceExpectedValueFieldIDIndex = 2
};

@interface MTRDevice ()
@property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state
@property (nonatomic) chip::FabricIndex fabricIndex;
Expand Down Expand Up @@ -200,9 +186,14 @@ @interface MTRDevice ()
// See MTRDeviceResponseHandler definition for value dictionary details.
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, NSDictionary *> * readCache;

// Expected value cache is attributePath => MTRPair of [NSDate of expiration time, NSDictionary of value]
// Expected value cache is attributePath => NSArray of [NSDate of expiration time, NSDictionary of value, expected value ID]
// - See MTRDeviceExpectedValueFieldIndex for the definitions of indices into this array.
// See MTRDeviceResponseHandler definition for value dictionary details.
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, MTRPair<NSDate *, NSDictionary *> *> * expectedValueCache;
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, NSArray *> * expectedValueCache;

// This is a monotonically increasing value used when adding entries to expectedValueCache
// Currently used/updated only in _getAttributesToReportWithNewExpectedValues:expirationTime:expectedValueID:
@property (nonatomic) uint64_t expectedValueNextID;
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved

@property (nonatomic) BOOL expirationCheckScheduled;

Expand Down Expand Up @@ -716,6 +707,16 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
timeout = MTRClampedNumber(timeout, @(1), @(UINT16_MAX));
}
expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID
clusterID:clusterID
attributeID:attributeID];
// Commit change into expected value cache
NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value };
uint64_t expectedValueID;
[self setExpectedValues:@[ newExpectedValueDictionary ]
expectedValueInterval:expectedValueInterval
expectedValueID:&expectedValueID];

MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
Expand All @@ -730,19 +731,14 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
MTR_LOG_INFO("%@ completion error %@ endWork", logPrefix, error);
[workItem endWork];
if (error) {
[self removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID];
}
}];
};
workItem.readyHandler = readyHandler;
MTR_LOG_INFO("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue);
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

// Commit change into expected value cache
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID
clusterID:clusterID
attributeID:attributeID];
NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value };

[self setExpectedValues:@[ newExpectedValueDictionary ] expectedValueInterval:expectedValueInterval];
}

- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
Expand All @@ -764,6 +760,16 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
} else {
expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
}

uint64_t expectedValueID = 0;
NSMutableArray<MTRAttributePath *> * attributePaths = nil;
if (expectedValues) {
[self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval expectedValueID:&expectedValueID];
attributePaths = [NSMutableArray array];
for (NSDictionary<NSString *, id> * expectedValue in expectedValues) {
[attributePaths addObject:expectedValue[MTRAttributePathKey]];
}
}
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
Expand All @@ -781,15 +787,14 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
completion(values, error);
});
[workItem endWork];
if (error && expectedValues) {
[self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID];
}
}];
};
workItem.readyHandler = readyHandler;
MTR_LOG_INFO("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue);
[_asyncCallbackWorkQueue enqueueWorkItem:workItem];

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

- (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
Expand Down Expand Up @@ -825,14 +830,15 @@ - (void)_checkExpiredExpectedValues
// find expired attributes, and calculate next timer fire date
NSDate * now = [NSDate date];
NSDate * nextExpirationDate = nil;
NSMutableSet<MTRPair<MTRAttributePath *, NSDictionary *> *> * attributeInfoToRemove = [NSMutableSet set];
// Set of NSArray with 2 elements [path, value] - this is used in this method only
NSMutableSet<NSArray *> * attributeInfoToRemove = [NSMutableSet set];
for (MTRAttributePath * attributePath in _expectedValueCache) {
MTRPair<NSDate *, NSDictionary *> * expectedValue = _expectedValueCache[attributePath];
NSDate * attributeExpirationDate = expectedValue.first;
NSArray * expectedValue = _expectedValueCache[attributePath];
NSDate * attributeExpirationDate = expectedValue[MTRDeviceExpectedValueFieldExpirationTimeIndex];
if (expectedValue) {
if ([now compare:attributeExpirationDate] == NSOrderedDescending) {
// expired - save [path, values] pair to attributeToRemove
[attributeInfoToRemove addObject:[MTRPair pairWithFirst:attributePath second:expectedValue.second]];
[attributeInfoToRemove addObject:@[ attributePath, expectedValue[MTRDeviceExpectedValueFieldValueIndex] ]];
} else {
// get the next expiration date
if (!nextExpirationDate || [nextExpirationDate compare:attributeExpirationDate] == NSOrderedDescending) {
Expand All @@ -845,10 +851,10 @@ - (void)_checkExpiredExpectedValues
// remove from expected value cache and report attributes as needed
NSMutableArray * attributesToReport = [NSMutableArray array];
NSMutableArray * attributePathsToReport = [NSMutableArray array];
for (MTRPair<MTRAttributePath *, NSDictionary *> * attributeInfo in attributeInfoToRemove) {
for (NSArray * attributeInfo in attributeInfoToRemove) {
// compare with known value and mark for report if different
MTRAttributePath * attributePath = attributeInfo.first;
NSDictionary * attributeDataValue = attributeInfo.second;
MTRAttributePath * attributePath = attributeInfo[0];
NSDictionary * attributeDataValue = attributeInfo[1];
NSDictionary * cachedAttributeDataValue = _readCache[attributePath];
if (cachedAttributeDataValue
&& ![self _attributeDataValue:attributeDataValue isEqualToDataValue:cachedAttributeDataValue]) {
Expand Down Expand Up @@ -895,17 +901,17 @@ - (void)_performScheduledExpirationCheck
os_unfair_lock_lock(&self->_lock);

// First check expected value cache
MTRPair<NSDate *, NSDictionary *> * expectedValue = _expectedValueCache[attributePath];
NSArray * expectedValue = _expectedValueCache[attributePath];
if (expectedValue) {
NSDate * now = [NSDate date];
if ([now compare:expectedValue.first] == NSOrderedDescending) {
if ([now compare:expectedValue[MTRDeviceExpectedValueFieldExpirationTimeIndex]] == NSOrderedDescending) {
// expired - purge and fall through
_expectedValueCache[attributePath] = nil;
} else {
os_unfair_lock_unlock(&self->_lock);

// not yet expired - return result
return expectedValue.second;
return expectedValue[MTRDeviceExpectedValueFieldValueIndex];
}
}

Expand Down Expand Up @@ -962,9 +968,10 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
_readCache[attributePath] = nil;
} else {
// if expected values exists, purge and update read cache
MTRPair<NSDate *, NSDictionary *> * expectedValue = _expectedValueCache[attributePath];
NSArray * expectedValue = _expectedValueCache[attributePath];
if (expectedValue) {
if (![self _attributeDataValue:attributeDataValue isEqualToDataValue:expectedValue.second]) {
if (![self _attributeDataValue:attributeDataValue
isEqualToDataValue:expectedValue[MTRDeviceExpectedValueFieldValueIndex]]) {
shouldReportAttribute = YES;
}
_expectedValueCache[attributePath] = nil;
Expand Down Expand Up @@ -995,47 +1002,102 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
return attributesToReport;
}

// If value is non-nil, associate with expectedValueID
// If value is nil, remove only if expectedValueID matches
- (void)_setExpectedValue:(NSDictionary<NSString *, id> *)expectedAttributeValue
attributePath:(MTRAttributePath *)attributePath
expirationTime:(NSDate *)expirationTime
shouldReportValue:(BOOL *)shouldReportValue
attributeValueToReport:(NSDictionary<NSString *, id> **)attributeValueToReport
expectedValueID:(uint64_t)expectedValueID
{
os_unfair_lock_assert_owner(&self->_lock);

*shouldReportValue = NO;
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved

NSArray * previousExpectedValue = _expectedValueCache[attributePath];
if (previousExpectedValue) {
if (expectedAttributeValue
&& ![self _attributeDataValue:expectedAttributeValue
isEqualToDataValue:previousExpectedValue[MTRDeviceExpectedValueFieldValueIndex]]) {
// Case where new expected value overrides previous expected value - report new expected value
*shouldReportValue = YES;
*attributeValueToReport = expectedAttributeValue;
} else if (!expectedAttributeValue) {
// Remove previous expected value only if it's from the same setExpectedValues operation
NSNumber * previousExpectedValueID = previousExpectedValue[MTRDeviceExpectedValueFieldIDIndex];
if (previousExpectedValueID.unsignedLongLongValue == expectedValueID) {
if (![self _attributeDataValue:previousExpectedValue[MTRDeviceExpectedValueFieldValueIndex]
isEqualToDataValue:_readCache[attributePath]]) {
// Case of removing expected value that is different than read cache - report read cache value
*shouldReportValue = YES;
*attributeValueToReport = _readCache[attributePath];
_expectedValueCache[attributePath] = nil;
}
}
}
} else {
if (expectedAttributeValue
&& ![self _attributeDataValue:expectedAttributeValue isEqualToDataValue:_readCache[attributePath]]) {
// Case where new expected value is different than read cache - report new expected value
*shouldReportValue = YES;
*attributeValueToReport = expectedAttributeValue;
}

// No need to report if new and previous expected value are both nil
}

if (expectedAttributeValue) {
_expectedValueCache[attributePath] = @[ expirationTime, expectedAttributeValue, @(expectedValueID) ];
}
}

// assume lock is held
- (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)expectedAttributeValues
expirationTime:(NSDate *)expirationTime
expectedValueID:(uint64_t *)expectedValueID
{
os_unfair_lock_assert_owner(&self->_lock);
uint64_t expectedValueIDToReturn = _expectedValueNextID++;

NSMutableArray * attributesToReport = [NSMutableArray array];
NSMutableArray * attributePathsToReport = [NSMutableArray array];
for (NSDictionary<NSString *, id> * attributeReponseValue in expectedAttributeValues) {
MTRAttributePath * attributePath = attributeReponseValue[MTRAttributePathKey];
NSDictionary * attributeDataValue = attributeReponseValue[MTRDataKey];

// check if value is different than cache, and report if needed
BOOL shouldReportAttribute = NO;

// first check write cache and purge / update
MTRPair<NSDate *, NSDictionary *> * previousExpectedValue = _expectedValueCache[attributePath];
if (previousExpectedValue) {
if (![self _attributeDataValue:attributeDataValue isEqualToDataValue:previousExpectedValue.second]) {
shouldReportAttribute = YES;
}
} else {
// if new expected value then compare with read cache and report if needed
if (![self _attributeDataValue:attributeDataValue isEqualToDataValue:_readCache[attributePath]]) {
shouldReportAttribute = YES;
}
}
_expectedValueCache[attributePath] = [MTRPair pairWithFirst:expirationTime second:attributeDataValue];

if (shouldReportAttribute) {
[attributesToReport addObject:attributeReponseValue];
BOOL shouldReportValue = NO;
NSDictionary<NSString *, id> * attributeValueToReport;
[self _setExpectedValue:attributeDataValue
attributePath:attributePath
expirationTime:expirationTime
shouldReportValue:&shouldReportValue
attributeValueToReport:&attributeValueToReport
expectedValueID:expectedValueIDToReturn];

if (shouldReportValue) {
[attributesToReport addObject:@{ MTRAttributePathKey : attributePath, MTRDataKey : attributeValueToReport }];
[attributePathsToReport addObject:attributePath];
}
}
if (expectedValueID) {
*expectedValueID = expectedValueIDToReturn;
}

MTR_LOG_INFO("%@ report from new expected values %@", self, attributePathsToReport);

return attributesToReport;
}

- (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values expectedValueInterval:(NSNumber *)expectedValueInterval
{
[self setExpectedValues:values expectedValueInterval:expectedValueInterval expectedValueID:nil];
}

// expectedValueID is an out-argument that returns an identifier to be used when removing expected values
- (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values
expectedValueInterval:(NSNumber *)expectedValueInterval
expectedValueID:(uint64_t *)expectedValueID
{
// since NSTimeInterval is in seconds, convert ms into seconds in double
NSDate * expirationTime = [NSDate dateWithTimeIntervalSinceNow:expectedValueInterval.doubleValue / 1000];
Expand All @@ -1046,13 +1108,56 @@ - (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values expe
os_unfair_lock_lock(&self->_lock);

// _getAttributesToReportWithNewExpectedValues will log attribute paths reported
NSArray * attributesToReport = [self _getAttributesToReportWithNewExpectedValues:values expirationTime:expirationTime];
NSArray * attributesToReport = [self _getAttributesToReportWithNewExpectedValues:values
expirationTime:expirationTime
expectedValueID:expectedValueID];
[self _reportAttributes:attributesToReport];

[self _checkExpiredExpectedValues];
os_unfair_lock_unlock(&self->_lock);
}

- (void)removeExpectedValuesForAttributePaths:(NSArray<MTRAttributePath *> *)attributePaths
expectedValueID:(uint64_t)expectedValueID
{
os_unfair_lock_lock(&self->_lock);
for (MTRAttributePath * attributePath in attributePaths) {
[self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID];
}
os_unfair_lock_unlock(&self->_lock);
}

- (void)removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID
{
os_unfair_lock_lock(&self->_lock);
[self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID];
os_unfair_lock_unlock(&self->_lock);
}

- (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID
{
os_unfair_lock_assert_owner(&self->_lock);

BOOL shouldReportValue;
NSDictionary<NSString *, id> * attributeValueToReport;
[self _setExpectedValue:nil
attributePath:attributePath
expirationTime:nil
shouldReportValue:&shouldReportValue
attributeValueToReport:&attributeValueToReport
expectedValueID:expectedValueID];

MTR_LOG_INFO("%@ remove expected value for path %@ should report %@", self, attributePath, shouldReportValue ? @"YES" : @"NO");

if (shouldReportValue) {
if (attributeValueToReport) {
[self _reportAttributes:@[ @{ MTRAttributePathKey : attributePath, MTRDataKey : attributeValueToReport } ]];
} else {
[self _reportAttributes:@[ @{ MTRAttributePathKey : attributePath } ]];
}
}
}

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