From ece1e93e9ae0d65d67782aac1172bcf836f41fd9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 Jul 2024 15:55:20 -0400 Subject: [PATCH 1/5] Remove no-longer-used MTRDevice logic for truncating data version lists After https://github.com/project-chip/connectedhomeip/pull/34111, ReadClient handles this logic itself, so the attempted truncation in MTRDevice was now dead code. --- src/app/ReadClient.cpp | 5 + src/darwin/Framework/CHIP/MTRDevice.mm | 127 ++++++++++--------------- 2 files changed, 57 insertions(+), 75 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 470c875fbaac30..d8fdc0517f3562 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -403,6 +403,7 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder const Span & aDataVersionFilters, bool & aEncodedDataVersionList) { + ChipLogProgress(DataManagement, "Attempting to encode %lu data version filters", static_cast(aDataVersionFilters.size())); for (auto & filter : aDataVersionFilters) { VerifyOrReturnError(filter.IsValidDataVersionFilter(), CHIP_ERROR_INVALID_ARGUMENT); @@ -434,6 +435,10 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder { // Packet is full, ignore the rest of the list aDataVersionFilterIBsBuilder.Rollback(backup); + ssize_t nonSkippedFilters = &filter - aDataVersionFilters.data(); + size_t skippedFilters = aDataVersionFilters.size() - static_cast(nonSkippedFilters); + ChipLogProgress(DataManagement, "Skipped encoding %lu out of %lu data version filters due to lack of space", + static_cast(skippedFilters), static_cast(aDataVersionFilters.size())); return CHIP_NO_ERROR; } else diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 05bc9f0ee6b0ac..759491439c2dcf 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -2277,17 +2277,16 @@ - (void)_removeCachedAttribute:(NSNumber *)attributeID fromCluster:(MTRClusterPa [clusterData removeValueForAttribute:attributeID]; } -- (void)_createDataVersionFilterListFromDictionary:(NSDictionary *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count sizeReduction:(size_t)sizeReduction +- (void)_createDataVersionFilterListFromDictionary:(NSDictionary *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count { size_t maxDataVersionFilterSize = dataVersions.count; // Check if any filter list should be generated - if (!dataVersions.count || (maxDataVersionFilterSize <= sizeReduction)) { + if (!dataVersions.count) { *count = 0; *dataVersionFilterList = nullptr; return; } - maxDataVersionFilterSize -= sizeReduction; DataVersionFilter * dataVersionFilterArray = new DataVersionFilter[maxDataVersionFilterSize]; size_t i = 0; @@ -2296,13 +2295,13 @@ - (void)_createDataVersionFilterListFromDictionary:(NSDictionary(path.endpoint.unsignedShortValue), static_cast(path.cluster.unsignedLongValue), static_cast(dataVersionNumber.unsignedLongValue)); } - if (i == maxDataVersionFilterSize) { - break; - } } *dataVersionFilterList = dataVersionFilterArray; - *count = maxDataVersionFilterSize; + // Note that we might have i < maxDataVersionFilterSize here if some of the + // dictionary entries had a null dataVersionNumber. The correct size of the + // valid entried in our array is "i". + *count = i; } - (void)_setupConnectivityMonitoring @@ -2530,75 +2529,55 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason auto readClient = std::make_unique(InteractionModelEngine::GetInstance(), exchangeManager, clusterStateCache->GetBufferedCallback(), ReadClient::InteractionType::Subscribe); - // Subscribe with data version filter list and retry with smaller list if out of packet space - CHIP_ERROR err; - NSDictionary * dataVersions = [self _getCachedDataVersions]; - size_t dataVersionFilterListSizeReduction = 0; - for (;;) { - // Wildcard endpoint, cluster, attribute, event. - auto attributePath = std::make_unique(); - auto eventPath = std::make_unique(); - // We want to get event reports at the minInterval, not the maxInterval. - eventPath->mIsUrgentEvent = true; - ReadPrepareParams readParams(session.Value()); - - readParams.mMinIntervalFloorSeconds = 0; - // Select a max interval based on the device's claimed idle sleep interval. - auto idleSleepInterval = std::chrono::duration_cast( - session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout); - - auto maxIntervalCeilingMin = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN); - if (idleSleepInterval < maxIntervalCeilingMin) { - idleSleepInterval = maxIntervalCeilingMin; - } - - auto maxIntervalCeilingMax = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX); - if (idleSleepInterval > maxIntervalCeilingMax) { - idleSleepInterval = maxIntervalCeilingMax; - } + // Wildcard endpoint, cluster, attribute, event. + auto attributePath = std::make_unique(); + auto eventPath = std::make_unique(); + // We want to get event reports at the minInterval, not the maxInterval. + eventPath->mIsUrgentEvent = true; + ReadPrepareParams readParams(session.Value()); + + readParams.mMinIntervalFloorSeconds = 0; + // Select a max interval based on the device's claimed idle sleep interval. + auto idleSleepInterval = std::chrono::duration_cast( + session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout); + + auto maxIntervalCeilingMin = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN); + if (idleSleepInterval < maxIntervalCeilingMin) { + idleSleepInterval = maxIntervalCeilingMin; + } + + auto maxIntervalCeilingMax = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX); + if (idleSleepInterval > maxIntervalCeilingMax) { + idleSleepInterval = maxIntervalCeilingMax; + } #ifdef DEBUG - if (maxIntervalOverride.HasValue()) { - idleSleepInterval = maxIntervalOverride.Value(); - } -#endif - readParams.mMaxIntervalCeilingSeconds = static_cast(idleSleepInterval.count()); - - readParams.mpAttributePathParamsList = attributePath.get(); - readParams.mAttributePathParamsListSize = 1; - readParams.mpEventPathParamsList = eventPath.get(); - readParams.mEventPathParamsListSize = 1; - readParams.mKeepSubscriptions = true; - readParams.mIsFabricFiltered = false; - size_t dataVersionFilterListSize = 0; - DataVersionFilter * dataVersionFilterList; - [self _createDataVersionFilterListFromDictionary:dataVersions dataVersionFilterList:&dataVersionFilterList count:&dataVersionFilterListSize sizeReduction:dataVersionFilterListSizeReduction]; - readParams.mDataVersionFilterListSize = dataVersionFilterListSize; - readParams.mpDataVersionFilterList = dataVersionFilterList; - attributePath.release(); - eventPath.release(); - - // TODO: Change from local filter list generation to rehydrating ClusterStateCache ot take advantage of existing filter list sorting algorithm - - // SendAutoResubscribeRequest cleans up the params, even on failure. - err = readClient->SendAutoResubscribeRequest(std::move(readParams)); - if (err == CHIP_NO_ERROR) { - break; - } - - // If error is not a "no memory" issue, then break and go through regular resubscribe logic - if (err != CHIP_ERROR_NO_MEMORY) { - break; - } - - // If "no memory" error is not caused by data version filter list, break as well - if (!dataVersionFilterListSize) { - break; - } - - // Now "no memory" could mean subscribe request packet space ran out. Reduce size and try again immediately - dataVersionFilterListSizeReduction++; + if (maxIntervalOverride.HasValue()) { + idleSleepInterval = maxIntervalOverride.Value(); } +#endif + readParams.mMaxIntervalCeilingSeconds = static_cast(idleSleepInterval.count()); + readParams.mpAttributePathParamsList = attributePath.get(); + readParams.mAttributePathParamsListSize = 1; + readParams.mpEventPathParamsList = eventPath.get(); + readParams.mEventPathParamsListSize = 1; + readParams.mKeepSubscriptions = true; + readParams.mIsFabricFiltered = false; + + // Subscribe with data version filter list from our cache. + size_t dataVersionFilterListSize = 0; + DataVersionFilter * dataVersionFilterList; + [self _createDataVersionFilterListFromDictionary:[self _getCachedDataVersions] dataVersionFilterList:&dataVersionFilterList count:&dataVersionFilterListSize]; + + readParams.mDataVersionFilterListSize = dataVersionFilterListSize; + readParams.mpDataVersionFilterList = dataVersionFilterList; + attributePath.release(); + eventPath.release(); + + // TODO: Change from local filter list generation to rehydrating ClusterStateCache to take advantage of existing filter list sorting algorithm + + // SendAutoResubscribeRequest cleans up the params, even on failure. + CHIP_ERROR err = readClient->SendAutoResubscribeRequest(std::move(readParams)); if (err != CHIP_NO_ERROR) { NSError * error = [MTRError errorForCHIPErrorCode:err logContext:self]; MTR_LOG_ERROR("%@ SendAutoResubscribeRequest error %@", self, error); @@ -2610,8 +2589,6 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason return; } - MTR_LOG("%@ Subscribe with data version list size %lu, reduced by %lu", self, static_cast(dataVersions.count), static_cast(dataVersionFilterListSizeReduction)); - // Callback and ClusterStateCache and ReadClient will be deleted // when OnDone is called. os_unfair_lock_lock(&self->_lock); From e289970941a93a2942e9782c518a0c4df8083cd0 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 Jul 2024 16:49:12 -0400 Subject: [PATCH 2/5] Address review comment. --- src/darwin/Framework/CHIP/MTRDevice.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 759491439c2dcf..d100f4a467f7b4 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -2589,6 +2589,8 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason return; } + MTR_LOG("%@ Subscribe with data version list size %lu", self, static_cast(dataVersionFilterListSize)); + // Callback and ClusterStateCache and ReadClient will be deleted // when OnDone is called. os_unfair_lock_lock(&self->_lock); From 3b7d7cf2920f79194c3ddcdc727f709797870c3a Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 Jul 2024 16:51:41 -0400 Subject: [PATCH 3/5] Fix compile issues. --- src/app/ReadClient.cpp | 2 ++ src/darwin/Framework/CHIP/MTRDevice.mm | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index d8fdc0517f3562..80e326f0aeaac3 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -435,10 +435,12 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder { // Packet is full, ignore the rest of the list aDataVersionFilterIBsBuilder.Rollback(backup); +#if CHIP_PROGRESS_LOGGING ssize_t nonSkippedFilters = &filter - aDataVersionFilters.data(); size_t skippedFilters = aDataVersionFilters.size() - static_cast(nonSkippedFilters); ChipLogProgress(DataManagement, "Skipped encoding %lu out of %lu data version filters due to lack of space", static_cast(skippedFilters), static_cast(aDataVersionFilters.size())); +#endif // CHIP_PROGRESS_LOGGING return CHIP_NO_ERROR; } else diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index d100f4a467f7b4..ab1306bf269dce 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -2539,7 +2539,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason readParams.mMinIntervalFloorSeconds = 0; // Select a max interval based on the device's claimed idle sleep interval. auto idleSleepInterval = std::chrono::duration_cast( - session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout); + session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout); auto maxIntervalCeilingMin = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN); if (idleSleepInterval < maxIntervalCeilingMin) { From f23754ebc4850ffdb2b8d79aeb29213489e49b36 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 3 Jul 2024 16:55:31 -0400 Subject: [PATCH 4/5] Address another review comment. --- src/darwin/Framework/CHIP/MTRDevice.mm | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index ab1306bf269dce..9bb846dde2911c 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -2279,29 +2279,24 @@ - (void)_removeCachedAttribute:(NSNumber *)attributeID fromCluster:(MTRClusterPa - (void)_createDataVersionFilterListFromDictionary:(NSDictionary *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count { - size_t maxDataVersionFilterSize = dataVersions.count; + size_t dataVersionFilterSize = dataVersions.count; // Check if any filter list should be generated - if (!dataVersions.count) { + if (dataVersionFilterSize == 0) { *count = 0; *dataVersionFilterList = nullptr; return; } - DataVersionFilter * dataVersionFilterArray = new DataVersionFilter[maxDataVersionFilterSize]; + DataVersionFilter * dataVersionFilterArray = new DataVersionFilter[dataVersionFilterSize]; size_t i = 0; for (MTRClusterPath * path in dataVersions) { NSNumber * dataVersionNumber = dataVersions[path]; - if (dataVersionNumber) { - dataVersionFilterArray[i++] = DataVersionFilter(static_cast(path.endpoint.unsignedShortValue), static_cast(path.cluster.unsignedLongValue), static_cast(dataVersionNumber.unsignedLongValue)); - } + dataVersionFilterArray[i++] = DataVersionFilter(static_cast(path.endpoint.unsignedShortValue), static_cast(path.cluster.unsignedLongValue), static_cast(dataVersionNumber.unsignedLongValue)); } *dataVersionFilterList = dataVersionFilterArray; - // Note that we might have i < maxDataVersionFilterSize here if some of the - // dictionary entries had a null dataVersionNumber. The correct size of the - // valid entried in our array is "i". - *count = i; + *count = dataVersionFilterSize; } - (void)_setupConnectivityMonitoring From e9cd5d1a69b7b572d9bca8eeb1ead7ec53ff16c2 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 23 Jul 2024 13:18:13 -0400 Subject: [PATCH 5/5] Address review comment. --- src/app/ReadClient.cpp | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 80e326f0aeaac3..51adf6e8ddc143 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -403,7 +403,11 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder const Span & aDataVersionFilters, bool & aEncodedDataVersionList) { - ChipLogProgress(DataManagement, "Attempting to encode %lu data version filters", static_cast(aDataVersionFilters.size())); +#if CHIP_PROGRESS_LOGGING + size_t encodedFilterCount = 0; + size_t irrelevantFilterCount = 0; + size_t skippedFilterCount = 0; +#endif for (auto & filter : aDataVersionFilters) { VerifyOrReturnError(filter.IsValidDataVersionFilter(), CHIP_ERROR_INVALID_ARGUMENT); @@ -421,6 +425,9 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder if (!intersected) { +#if CHIP_PROGRESS_LOGGING + ++irrelevantFilterCount; +#endif continue; } @@ -429,6 +436,9 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder CHIP_ERROR err = EncodeDataVersionFilter(aDataVersionFilterIBsBuilder, filter); if (err == CHIP_NO_ERROR) { +#if CHIP_PROGRESS_LOGGING + ++encodedFilterCount; +#endif aEncodedDataVersionList = true; } else if (err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL) @@ -436,18 +446,21 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder // Packet is full, ignore the rest of the list aDataVersionFilterIBsBuilder.Rollback(backup); #if CHIP_PROGRESS_LOGGING - ssize_t nonSkippedFilters = &filter - aDataVersionFilters.data(); - size_t skippedFilters = aDataVersionFilters.size() - static_cast(nonSkippedFilters); - ChipLogProgress(DataManagement, "Skipped encoding %lu out of %lu data version filters due to lack of space", - static_cast(skippedFilters), static_cast(aDataVersionFilters.size())); + ssize_t nonSkippedFilterCount = &filter - aDataVersionFilters.data(); + skippedFilterCount = aDataVersionFilters.size() - static_cast(nonSkippedFilterCount); #endif // CHIP_PROGRESS_LOGGING - return CHIP_NO_ERROR; + break; } else { return err; } } + + ChipLogProgress(DataManagement, + "%lu data version filters provided, %lu not relevant, %lu encoded, %lu skipped due to lack of space", + static_cast(aDataVersionFilters.size()), static_cast(irrelevantFilterCount), + static_cast(encodedFilterCount), static_cast(skippedFilterCount)); return CHIP_NO_ERROR; }