From 0189fb3abf0809c90c6b85784b39082990bb42e9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 23 Aug 2024 01:15:52 -0400 Subject: [PATCH] Remove readAttributeWithEndpointID implementation from MTRDevice. This is implemented (differently) by the different subclasses. Once this implementation is removed, removeExpectedValueForAttributePath becomes unused and can be removed. Also removes the unused setExpectedValues declaration in MTRDevice_Internal.h and the implementations of it. --- src/darwin/Framework/CHIP/MTRDevice.mm | 136 ++---------------- .../Framework/CHIP/MTRDevice_Concrete.mm | 5 - .../Framework/CHIP/MTRDevice_Internal.h | 4 - 3 files changed, 9 insertions(+), 136 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 3f2acce5f5587e..f913f29388f5fb 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -1831,11 +1831,12 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) attributeID:(NSNumber *)attributeID params:(MTRReadParams * _Nullable)params { -#define ErrorStr "MTRDevice readAttributeWithEndpointID:clusterID:attributeID:params: must be handled by subclasses" - MTR_LOG_ERROR(ErrorStr); +#define MTRDeviceErrorStr "MTRDevice readAttributeWithEndpointID:clusterID:attributeID:params: must be handled by subclasses" + MTR_LOG_ERROR(MTRDeviceErrorStr); #ifdef DEBUG - NSAssert(NO, @ErrorStr); + NSAssert(NO, @MTRDeviceErrorStr); #endif // DEBUG +#undef MTRDeviceErrorStr return nil; } @@ -1846,120 +1847,12 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID expectedValueInterval:(NSNumber *)expectedValueInterval timedWriteTimeout:(NSNumber * _Nullable)timeout { - if (timeout) { - timeout = MTRClampedNumber(timeout, @(1), @(UINT16_MAX)); - } - expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX)); - MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID - clusterID:clusterID - - attributeID:attributeID]; - - __block BOOL useValueAsExpectedValue = YES; +#define MTRDeviceErrorStr "MTRDevice writeAttributeWithEndpointID:clusterID:attributeID:value:expectedValueInterval:timedWriteTimeout: must be handled by subclasses" + MTR_LOG_ERROR(MTRDeviceErrorStr); #ifdef DEBUG - os_unfair_lock_lock(&self->_lock); - [self _callFirstDelegateSynchronouslyWithBlock:^(id delegate) { - if ([delegate respondsToSelector:@selector(unitTestShouldSkipExpectedValuesForWrite:)]) { - useValueAsExpectedValue = ![delegate unitTestShouldSkipExpectedValuesForWrite:self]; - } - }]; - os_unfair_lock_unlock(&self->_lock); -#endif - - uint64_t expectedValueID = 0; - if (useValueAsExpectedValue) { - // Commit change into expected value cache - NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value }; - [self setExpectedValues:@[ newExpectedValueDictionary ] - expectedValueInterval:expectedValueInterval - expectedValueID:&expectedValueID]; - } - - MTRAsyncWorkItem * workItem = [[MTRAsyncWorkItem alloc] initWithQueue:self.queue]; - uint64_t workItemID = workItem.uniqueID; // capture only the ID, not the work item - NSNumber * nodeID = _nodeID; - - // Write request data is an array of items (for now always length 1). Each - // item is an array containing: - // - // [ attribute path, value, timedWriteTimeout, expectedValueID ] - // - // where expectedValueID is stored as NSNumber and NSNull represents nil timeouts - auto * writeData = @[ attributePath, [value copy], timeout ?: [NSNull null], @(expectedValueID) ]; - - NSMutableArray * writeRequests = [NSMutableArray arrayWithObject:writeData]; - - [workItem setBatchingID:MTRDeviceWorkItemBatchingWriteID data:writeRequests handler:^(id opaqueDataCurrent, id opaqueDataNext) { - mtr_hide(self); // don't capture self accidentally - NSMutableArray * writeRequestsCurrent = opaqueDataCurrent; - NSMutableArray * writeRequestsNext = opaqueDataNext; - - if (writeRequestsCurrent.count != 1) { - // Very unexpected! - MTR_LOG_ERROR("Batching write attribute work item [%llu]: Unexpected write request count %lu", workItemID, static_cast(writeRequestsCurrent.count)); - return MTRNotBatched; - } - - MTRBatchingOutcome outcome = MTRNotBatched; - while (writeRequestsNext.count) { - // If paths don't match, we cannot replace the earlier write - // with the later one. - if (![writeRequestsNext[0][MTRDeviceWriteRequestFieldPathIndex] - isEqual:writeRequestsCurrent[0][MTRDeviceWriteRequestFieldPathIndex]]) { - MTR_LOG("Batching write attribute work item [%llu]: cannot replace with next work item due to path mismatch", workItemID); - return outcome; - } - - // Replace our one request with the first one from the next item. - auto writeItem = writeRequestsNext.firstObject; - [writeRequestsNext removeObjectAtIndex:0]; - [writeRequestsCurrent replaceObjectAtIndex:0 withObject:writeItem]; - MTR_LOG("Batching write attribute work item [%llu]: replaced with new write value %@ [0x%016llX]", - workItemID, writeItem, nodeID.unsignedLongLongValue); - outcome = MTRBatchedPartially; - } - NSCAssert(writeRequestsNext.count == 0, @"should have batched everything or returned early"); - return MTRBatchedFully; - }]; - // The write operation will install a duplicate check handler, to return NO for "isDuplicate". Since a write operation may - // change values, only read requests after this should be considered for duplicate requests. - [workItem setDuplicateTypeID:MTRDeviceWorkItemDuplicateReadTypeID handler:^(id opaqueItemData, BOOL * isDuplicate, BOOL * stop) { - *isDuplicate = NO; - *stop = YES; - }]; - [workItem setReadyHandler:^(MTRDevice * self, NSInteger retryCount, MTRAsyncWorkCompletionBlock completion) { - MTRBaseDevice * baseDevice = [self newBaseDevice]; - // Make sure to use writeRequests here, because that's what our batching - // handler will modify as needed. - NSCAssert(writeRequests.count == 1, @"Incorrect number of write requests: %lu", static_cast(writeRequests.count)); - - auto * request = writeRequests[0]; - MTRAttributePath * path = request[MTRDeviceWriteRequestFieldPathIndex]; - - id timedWriteTimeout = request[MTRDeviceWriteRequestFieldTimeoutIndex]; - if (timedWriteTimeout == [NSNull null]) { - timedWriteTimeout = nil; - } - - [baseDevice - writeAttributeWithEndpointID:path.endpoint - clusterID:path.cluster - attributeID:path.attribute - value:request[MTRDeviceWriteRequestFieldValueIndex] - timedWriteTimeout:timedWriteTimeout - queue:self.queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - if (error) { - MTR_LOG_ERROR("Write attribute work item [%llu] failed: %@", workItemID, error); - if (useValueAsExpectedValue) { - NSNumber * expectedValueID = request[MTRDeviceWriteRequestFieldExpectedValueIDIndex]; - [self removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID.unsignedLongLongValue]; - } - } - completion(MTRAsyncWorkComplete); - }]; - }]; - [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"write %@ 0x%llx 0x%llx", endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue]; + NSAssert(NO, @MTRDeviceErrorStr); +#endif // DEBUG +#undef MTRDeviceErrorStr } - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID @@ -2796,11 +2689,6 @@ - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray *> *)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 *> *)values expectedValueInterval:(NSNumber *)expectedValueInterval @@ -2833,12 +2721,6 @@ - (void)removeExpectedValuesForAttributePaths:(NSArray *)att } } -- (void)removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID -{ - std::lock_guard lock(_lock); - [self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; -} - - (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID { os_unfair_lock_assert_owner(&self->_lock); diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 12614ea4091b0c..b7e7bf78027b4f 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -3644,11 +3644,6 @@ - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray *> *)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 *> *)values expectedValueInterval:(NSNumber *)expectedValueInterval diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h index b6a59ac9321d42..dae183eec217f6 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Internal.h @@ -125,10 +125,6 @@ MTR_DIRECT_MEMBERS - (instancetype)initForSubclassesWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller; - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller; -// Called from MTRClusters for writes and commands -- (void)setExpectedValues:(NSArray *> *)values - expectedValueInterval:(NSNumber *)expectedValueIntervalMs; - // called by controller to clean up and shutdown - (void)invalidate;