From ae5d75c69f6cec4fb62b991e57b895b6f4812291 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 22 Aug 2024 13:13:04 -0400 Subject: [PATCH 1/2] Remove unnecessary invokeCommand overrides from MTRDevice_Concrete. (#35151) The one part that is not shared with the XPC implementation is _invokeCommandWithEndpointID:.... Everything else is just generic argument massaging and forwarding that can keep living in the base MTRDevice. --- .../Framework/CHIP/MTRDevice_Concrete.mm | 126 ------------------ 1 file changed, 126 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 120d29aff122d4..12614ea4091b0c 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -2835,59 +2835,6 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"write %@ 0x%llx 0x%llx", endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue]; } -- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID - clusterID:(NSNumber *)clusterID - commandID:(NSNumber *)commandID - commandFields:(NSDictionary * _Nullable)commandFields - expectedValues:(NSArray *> * _Nullable)expectedValues - expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval - queue:(dispatch_queue_t)queue - completion:(MTRDeviceResponseHandler)completion -{ - if (commandFields == nil) { - commandFields = @{ - MTRTypeKey : MTRStructureValueType, - MTRValueKey : @[], - }; - } - - [self invokeCommandWithEndpointID:endpointID - clusterID:clusterID - commandID:commandID - commandFields:commandFields - expectedValues:expectedValues - expectedValueInterval:expectedValueInterval - timedInvokeTimeout:nil - queue:queue - completion:completion]; -} - -- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID - clusterID:(NSNumber *)clusterID - commandID:(NSNumber *)commandID - commandFields:(id)commandFields - expectedValues:(NSArray *> * _Nullable)expectedValues - expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval - timedInvokeTimeout:(NSNumber * _Nullable)timeout - queue:(dispatch_queue_t)queue - completion:(MTRDeviceResponseHandler)completion -{ - // We don't have a way to communicate a non-default invoke timeout - // here for now. - // TODO: https://github.com/project-chip/connectedhomeip/issues/24563 - - [self _invokeCommandWithEndpointID:endpointID - clusterID:clusterID - commandID:commandID - commandFields:commandFields - expectedValues:expectedValues - expectedValueInterval:expectedValueInterval - timedInvokeTimeout:timeout - serverSideProcessingTimeout:nil - queue:queue - completion:completion]; -} - - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID commandID:(NSNumber *)commandID @@ -2985,58 +2932,6 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"invoke %@ 0x%llx 0x%llx", endpointID, clusterID.unsignedLongLongValue, commandID.unsignedLongLongValue]; } -- (void)_invokeKnownCommandWithEndpointID:(NSNumber *)endpointID - clusterID:(NSNumber *)clusterID - commandID:(NSNumber *)commandID - commandPayload:(id)commandPayload - expectedValues:(NSArray *> * _Nullable)expectedValues - expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval - timedInvokeTimeout:(NSNumber * _Nullable)timeout - serverSideProcessingTimeout:(NSNumber * _Nullable)serverSideProcessingTimeout - responseClass:(Class _Nullable)responseClass - queue:(dispatch_queue_t)queue - completion:(void (^)(id _Nullable response, NSError * _Nullable error))completion -{ - if (![commandPayload respondsToSelector:@selector(_encodeAsDataValue:)]) { - dispatch_async(queue, ^{ - completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]); - }); - return; - } - - NSError * encodingError; - auto * commandFields = [commandPayload _encodeAsDataValue:&encodingError]; - if (commandFields == nil) { - dispatch_async(queue, ^{ - completion(nil, encodingError); - }); - return; - } - - auto responseHandler = ^(NSArray *> * _Nullable values, NSError * _Nullable error) { - id _Nullable response = nil; - if (error == nil) { - if (values.count != 1) { - error = [NSError errorWithDomain:MTRErrorDomain code:MTRErrorCodeSchemaMismatch userInfo:nil]; - } else if (responseClass != nil) { - response = [[responseClass alloc] initWithResponseValue:values[0] error:&error]; - } - } - completion(response, error); - }; - - [self _invokeCommandWithEndpointID:endpointID - clusterID:clusterID - commandID:commandID - commandFields:commandFields - expectedValues:expectedValues - expectedValueInterval:expectedValueInterval - timedInvokeTimeout:timeout - serverSideProcessingTimeout:serverSideProcessingTimeout - queue:queue - completion:responseHandler]; -} - - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode discriminator:(NSNumber *)discriminator duration:(NSNumber *)duration @@ -4072,27 +3967,6 @@ + (MTRDevice *)deviceWithNodeID:(uint64_t)nodeID deviceController:(MTRDeviceCont return [self deviceWithNodeID:@(nodeID) controller:deviceController]; } -- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID - clusterID:(NSNumber *)clusterID - commandID:(NSNumber *)commandID - commandFields:(id)commandFields - expectedValues:(NSArray *> * _Nullable)expectedValues - expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval - timedInvokeTimeout:(NSNumber * _Nullable)timeout - clientQueue:(dispatch_queue_t)queue - completion:(MTRDeviceResponseHandler)completion -{ - [self invokeCommandWithEndpointID:endpointID - clusterID:clusterID - commandID:commandID - commandFields:commandFields - expectedValues:expectedValues - expectedValueInterval:expectedValueInterval - timedInvokeTimeout:timeout - queue:queue - completion:completion]; -} - @end #pragma mark - SubscriptionCallback From 26956012d4d6d6a14796a2894ffe4af0a3c88ff9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 22 Aug 2024 13:16:50 -0400 Subject: [PATCH 2/2] Remove readAttributeWithEndpointID implementation from MTRDevice. (#35150) This is implemented (differently) by the different subclasses. Once this implementation is removed, the following become unreachable and can be removed: * _attributeValueDictionaryForAttributePath * _subscriptionAbleToReport * _readThroughSkipped At that point _subscriptionsAllowed becomes unreachable and can be removed. --- src/darwin/Framework/CHIP/MTRDevice.mm | 229 +------------------------ 1 file changed, 7 insertions(+), 222 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index d4027f7725ddeb..3f2acce5f5587e 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -649,21 +649,7 @@ - (void)_setDSTOffsets:(NSArray return outputArray; } -#pragma mark Subscription and delegate handling - -// subscription intervals are in seconds -#define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN (10 * 60) // 10 minutes (for now) -#define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX (60 * 60) // 60 minutes - -- (BOOL)_subscriptionsAllowed -{ - os_unfair_lock_assert_owner(&self->_lock); - - // TODO: XPC: This function and all its callsites should go away from this class. - - // We should not allow a subscription for device controllers over XPC. - return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; -} +#pragma mark Delegate handling - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queue { @@ -757,41 +743,6 @@ - (void)nodeMayBeAdvertisingOperational MTR_LOG("%@ saw new operational advertisement", self); } -// Return YES if we are in a state where, apart from communication issues with -// the device, we will be able to get reports via our subscription. -- (BOOL)_subscriptionAbleToReport -{ - std::lock_guard lock(_lock); - if (![self _delegateExists]) { - // No delegate definitely means no subscription. - return NO; - } - - // For unit testing only, matching logic in setDelegate -#ifdef DEBUG - __block BOOL useTestDelegateOverride = NO; - __block BOOL testDelegateShouldSetUpSubscriptionForDevice = NO; - [self _callFirstDelegateSynchronouslyWithBlock:^(id testDelegate) { - if ([testDelegate respondsToSelector:@selector(unitTestShouldSetUpSubscriptionForDevice:)]) { - useTestDelegateOverride = YES; - testDelegateShouldSetUpSubscriptionForDevice = [testDelegate unitTestShouldSetUpSubscriptionForDevice:self]; - } - }]; - if (useTestDelegateOverride && !testDelegateShouldSetUpSubscriptionForDevice) { - return NO; - } - -#endif - - // Subscriptions are not able to report if they are not allowed. - return [self _subscriptionsAllowed]; -} - -// Notification that read-through was skipped for an attribute read. -- (void)_readThroughSkipped -{ -} - - (BOOL)_delegateExists { os_unfair_lock_assert_owner(&self->_lock); @@ -1880,147 +1831,12 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) attributeID:(NSNumber *)attributeID params:(MTRReadParams * _Nullable)params { - MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID - clusterID:clusterID - attributeID:attributeID]; - - BOOL attributeIsSpecified = MTRAttributeIsSpecified(clusterID.unsignedIntValue, attributeID.unsignedIntValue); - BOOL hasChangesOmittedQuality; - if (attributeIsSpecified) { - hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath); - } else { - if (params == nil) { - hasChangesOmittedQuality = NO; - } else { - hasChangesOmittedQuality = !params.assumeUnknownAttributesReportable; - } - } - - // Return current known / expected value right away - NSDictionary * attributeValueToReturn = [self _attributeValueDictionaryForAttributePath:attributePath]; - - // Send read request to device if any of the following are true: - // 1. Subscription not in a state we can expect reports - // 2. The attribute has the Changes Omitted quality, so we won't get reports for it. - // 3. The attribute is not in the spec, and the read params asks to assume - // an unknown attribute has the Changes Omitted quality. - if (![self _subscriptionAbleToReport] || hasChangesOmittedQuality) { - // Read requests container will be a mutable array of items, each being an array containing: - // [attribute request path, params] - // Batching handler should only coalesce when params are equal. - - // For this single read API there's only 1 array item. Use NSNull to stand in for nil params for easy comparison. - MTRAttributeRequestPath * readRequestPath = [MTRAttributeRequestPath requestPathWithEndpointID:endpointID - clusterID:clusterID - attributeID:attributeID]; - NSArray * readRequestData = @[ readRequestPath, params ?: [NSNull null] ]; - - // But first, check if a duplicate read request is already queued and return - if ([_asyncWorkQueue hasDuplicateForTypeID:MTRDeviceWorkItemDuplicateReadTypeID workItemData:readRequestData]) { - return attributeValueToReturn; - } - - NSMutableArray * readRequests = [NSMutableArray arrayWithObject:readRequestData]; - - // Create work item, set ready handler to perform task, then enqueue the work - MTRAsyncWorkItem * workItem = [[MTRAsyncWorkItem alloc] initWithQueue:self.queue]; - uint64_t workItemID = workItem.uniqueID; // capture only the ID, not the work item - NSNumber * nodeID = [self nodeID]; - - [workItem setBatchingID:MTRDeviceWorkItemBatchingReadID data:readRequests handler:^(id opaqueDataCurrent, id opaqueDataNext) { - mtr_hide(self); // don't capture self accidentally - NSMutableArray * readRequestsCurrent = opaqueDataCurrent; - NSMutableArray * readRequestsNext = opaqueDataNext; - - MTRBatchingOutcome outcome = MTRNotBatched; - while (readRequestsNext.count) { - // Can only read up to 9 paths at a time, per spec - if (readRequestsCurrent.count >= 9) { - MTR_LOG("Batching read attribute work item [%llu]: cannot add more work, item is full [0x%016llX:%@:0x%llx:0x%llx]", workItemID, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - return outcome; - } - - // if params don't match then they cannot be merged - if (![readRequestsNext[0][MTRDeviceReadRequestFieldParamsIndex] - isEqual:readRequestsCurrent[0][MTRDeviceReadRequestFieldParamsIndex]]) { - MTR_LOG("Batching read attribute work item [%llu]: cannot add more work, parameter mismatch [0x%016llX:%@:0x%llx:0x%llx]", workItemID, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - return outcome; - } - - // merge the next item's first request into the current item's list - auto readItem = readRequestsNext.firstObject; - [readRequestsNext removeObjectAtIndex:0]; - [readRequestsCurrent addObject:readItem]; - MTR_LOG("Batching read attribute work item [%llu]: added %@ (now %lu requests total) [0x%016llX:%@:0x%llx:0x%llx]", - workItemID, readItem, static_cast(readRequestsCurrent.count), nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - outcome = MTRBatchedPartially; - } - NSCAssert(readRequestsNext.count == 0, @"should have batched everything or returned early"); - return MTRBatchedFully; - }]; - [workItem setDuplicateTypeID:MTRDeviceWorkItemDuplicateReadTypeID handler:^(id opaqueItemData, BOOL * isDuplicate, BOOL * stop) { - mtr_hide(self); // don't capture self accidentally - for (NSArray * readItem in readRequests) { - if ([readItem isEqual:opaqueItemData]) { - MTR_LOG("Read attribute work item [%llu] report duplicate %@ [0x%016llX:%@:0x%llx:0x%llx]", workItemID, readItem, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - *isDuplicate = YES; - *stop = YES; - return; - } - } - *stop = NO; - }]; - [workItem setReadyHandler:^(MTRDevice * self, NSInteger retryCount, MTRAsyncWorkCompletionBlock completion) { - // Sanity check - if (readRequests.count == 0) { - MTR_LOG_ERROR("Read attribute work item [%llu] contained no read requests", workItemID); - completion(MTRAsyncWorkComplete); - return; - } - - // Build the attribute paths from the read requests - NSMutableArray * attributePaths = [NSMutableArray array]; - for (NSArray * readItem in readRequests) { - NSAssert(readItem.count == 2, @"invalid read attribute item"); - [attributePaths addObject:readItem[MTRDeviceReadRequestFieldPathIndex]]; - } - // If param is the NSNull stand-in, then just use nil - id readParamObject = readRequests[0][MTRDeviceReadRequestFieldParamsIndex]; - MTRReadParams * readParams = (![readParamObject isEqual:[NSNull null]]) ? readParamObject : nil; - - MTRBaseDevice * baseDevice = [self newBaseDevice]; - [baseDevice - readAttributePaths:attributePaths - eventPaths:nil - params:readParams - includeDataVersion:YES - queue:self.queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - if (values) { - // Since the format is the same data-value dictionary, this looks like an - // attribute report - MTR_LOG("Read attribute work item [%llu] result: %@ [0x%016llX:%@:0x%llX:0x%llX]", workItemID, values, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - [self _handleAttributeReport:values fromSubscription:NO]; - } - - // TODO: better retry logic - if (error && (retryCount < 2)) { - MTR_LOG_ERROR("Read attribute work item [%llu] failed (will retry): %@ [0x%016llX:%@:0x%llx:0x%llx]", workItemID, error, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - completion(MTRAsyncWorkNeedsRetry); - } else { - if (error) { - MTR_LOG("Read attribute work item [%llu] failed (giving up): %@ [0x%016llX:%@:0x%llx:0x%llx]", workItemID, error, nodeID.unsignedLongLongValue, endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue); - } - completion(MTRAsyncWorkComplete); - } - }]; - }]; - [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"read %@ 0x%llx 0x%llx", endpointID, clusterID.unsignedLongLongValue, attributeID.unsignedLongLongValue]; - } else { - [self _readThroughSkipped]; - } - - return attributeValueToReturn; +#define ErrorStr "MTRDevice readAttributeWithEndpointID:clusterID:attributeID:params: must be handled by subclasses" + MTR_LOG_ERROR(ErrorStr); +#ifdef DEBUG + NSAssert(NO, @ErrorStr); +#endif // DEBUG + return nil; } - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID @@ -2456,37 +2272,6 @@ - (void)_performScheduledExpirationCheck [self _checkExpiredExpectedValues]; } -// Get attribute value dictionary for an attribute path from the right cache -- (NSDictionary *)_attributeValueDictionaryForAttributePath:(MTRAttributePath *)attributePath -{ - std::lock_guard lock(_lock); - - // First check expected value cache - NSArray * expectedValue = _expectedValueCache[attributePath]; - if (expectedValue) { - NSDate * now = [NSDate date]; - if ([now compare:expectedValue[MTRDeviceExpectedValueFieldExpirationTimeIndex]] == NSOrderedDescending) { - // expired - purge and fall through - _expectedValueCache[attributePath] = nil; - } else { - // not yet expired - return result - return expectedValue[MTRDeviceExpectedValueFieldValueIndex]; - } - } - - // Then check read cache - NSDictionary * cachedAttributeValue = [self _cachedAttributeValueForPath:attributePath]; - if (cachedAttributeValue) { - return cachedAttributeValue; - } else { - // TODO: when not found in cache, generated default values should be used - MTR_LOG("%@ _attributeValueDictionaryForAttributePath: could not find cached attribute values for attribute %@", self, - attributePath); - } - - return nil; -} - - (BOOL)_attributeDataValue:(NSDictionary *)one isEqualToDataValue:(NSDictionary *)theOther { // Sanity check for nil cases