Skip to content

Commit ba949bf

Browse files
Remove no-longer-used MTRDevice logic for truncating data version lists (#34183)
* Remove no-longer-used MTRDevice logic for truncating data version lists After #34111, ReadClient handles this logic itself, so the attempted truncation in MTRDevice was now dead code. * Address review comment. * Fix compile issues. * Address another review comment. * Address review comment.
1 parent b74057e commit ba949bf

File tree

2 files changed

+74
-80
lines changed

2 files changed

+74
-80
lines changed

src/app/ReadClient.cpp

+21-1
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,11 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder
403403
const Span<DataVersionFilter> & aDataVersionFilters,
404404
bool & aEncodedDataVersionList)
405405
{
406+
#if CHIP_PROGRESS_LOGGING
407+
size_t encodedFilterCount = 0;
408+
size_t irrelevantFilterCount = 0;
409+
size_t skippedFilterCount = 0;
410+
#endif
406411
for (auto & filter : aDataVersionFilters)
407412
{
408413
VerifyOrReturnError(filter.IsValidDataVersionFilter(), CHIP_ERROR_INVALID_ARGUMENT);
@@ -420,6 +425,9 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder
420425

421426
if (!intersected)
422427
{
428+
#if CHIP_PROGRESS_LOGGING
429+
++irrelevantFilterCount;
430+
#endif
423431
continue;
424432
}
425433

@@ -428,19 +436,31 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder
428436
CHIP_ERROR err = EncodeDataVersionFilter(aDataVersionFilterIBsBuilder, filter);
429437
if (err == CHIP_NO_ERROR)
430438
{
439+
#if CHIP_PROGRESS_LOGGING
440+
++encodedFilterCount;
441+
#endif
431442
aEncodedDataVersionList = true;
432443
}
433444
else if (err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL)
434445
{
435446
// Packet is full, ignore the rest of the list
436447
aDataVersionFilterIBsBuilder.Rollback(backup);
437-
return CHIP_NO_ERROR;
448+
#if CHIP_PROGRESS_LOGGING
449+
ssize_t nonSkippedFilterCount = &filter - aDataVersionFilters.data();
450+
skippedFilterCount = aDataVersionFilters.size() - static_cast<size_t>(nonSkippedFilterCount);
451+
#endif // CHIP_PROGRESS_LOGGING
452+
break;
438453
}
439454
else
440455
{
441456
return err;
442457
}
443458
}
459+
460+
ChipLogProgress(DataManagement,
461+
"%lu data version filters provided, %lu not relevant, %lu encoded, %lu skipped due to lack of space",
462+
static_cast<unsigned long>(aDataVersionFilters.size()), static_cast<unsigned long>(irrelevantFilterCount),
463+
static_cast<unsigned long>(encodedFilterCount), static_cast<unsigned long>(skippedFilterCount));
444464
return CHIP_NO_ERROR;
445465
}
446466

src/darwin/Framework/CHIP/MTRDevice.mm

+53-79
Original file line numberDiff line numberDiff line change
@@ -2277,32 +2277,26 @@ - (void)_removeCachedAttribute:(NSNumber *)attributeID fromCluster:(MTRClusterPa
22772277
[clusterData removeValueForAttribute:attributeID];
22782278
}
22792279

2280-
- (void)_createDataVersionFilterListFromDictionary:(NSDictionary<MTRClusterPath *, NSNumber *> *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count sizeReduction:(size_t)sizeReduction
2280+
- (void)_createDataVersionFilterListFromDictionary:(NSDictionary<MTRClusterPath *, NSNumber *> *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count
22812281
{
2282-
size_t maxDataVersionFilterSize = dataVersions.count;
2282+
size_t dataVersionFilterSize = dataVersions.count;
22832283

22842284
// Check if any filter list should be generated
2285-
if (!dataVersions.count || (maxDataVersionFilterSize <= sizeReduction)) {
2285+
if (dataVersionFilterSize == 0) {
22862286
*count = 0;
22872287
*dataVersionFilterList = nullptr;
22882288
return;
22892289
}
2290-
maxDataVersionFilterSize -= sizeReduction;
22912290

2292-
DataVersionFilter * dataVersionFilterArray = new DataVersionFilter[maxDataVersionFilterSize];
2291+
DataVersionFilter * dataVersionFilterArray = new DataVersionFilter[dataVersionFilterSize];
22932292
size_t i = 0;
22942293
for (MTRClusterPath * path in dataVersions) {
22952294
NSNumber * dataVersionNumber = dataVersions[path];
2296-
if (dataVersionNumber) {
2297-
dataVersionFilterArray[i++] = DataVersionFilter(static_cast<chip::EndpointId>(path.endpoint.unsignedShortValue), static_cast<chip::ClusterId>(path.cluster.unsignedLongValue), static_cast<chip::DataVersion>(dataVersionNumber.unsignedLongValue));
2298-
}
2299-
if (i == maxDataVersionFilterSize) {
2300-
break;
2301-
}
2295+
dataVersionFilterArray[i++] = DataVersionFilter(static_cast<chip::EndpointId>(path.endpoint.unsignedShortValue), static_cast<chip::ClusterId>(path.cluster.unsignedLongValue), static_cast<chip::DataVersion>(dataVersionNumber.unsignedLongValue));
23022296
}
23032297

23042298
*dataVersionFilterList = dataVersionFilterArray;
2305-
*count = maxDataVersionFilterSize;
2299+
*count = dataVersionFilterSize;
23062300
}
23072301

23082302
- (void)_setupConnectivityMonitoring
@@ -2530,75 +2524,55 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
25302524
auto readClient = std::make_unique<ReadClient>(InteractionModelEngine::GetInstance(), exchangeManager,
25312525
clusterStateCache->GetBufferedCallback(), ReadClient::InteractionType::Subscribe);
25322526

2533-
// Subscribe with data version filter list and retry with smaller list if out of packet space
2534-
CHIP_ERROR err;
2535-
NSDictionary<MTRClusterPath *, NSNumber *> * dataVersions = [self _getCachedDataVersions];
2536-
size_t dataVersionFilterListSizeReduction = 0;
2537-
for (;;) {
2538-
// Wildcard endpoint, cluster, attribute, event.
2539-
auto attributePath = std::make_unique<AttributePathParams>();
2540-
auto eventPath = std::make_unique<EventPathParams>();
2541-
// We want to get event reports at the minInterval, not the maxInterval.
2542-
eventPath->mIsUrgentEvent = true;
2543-
ReadPrepareParams readParams(session.Value());
2544-
2545-
readParams.mMinIntervalFloorSeconds = 0;
2546-
// Select a max interval based on the device's claimed idle sleep interval.
2547-
auto idleSleepInterval = std::chrono::duration_cast<System::Clock::Seconds32>(
2548-
session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout);
2549-
2550-
auto maxIntervalCeilingMin = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN);
2551-
if (idleSleepInterval < maxIntervalCeilingMin) {
2552-
idleSleepInterval = maxIntervalCeilingMin;
2553-
}
2554-
2555-
auto maxIntervalCeilingMax = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX);
2556-
if (idleSleepInterval > maxIntervalCeilingMax) {
2557-
idleSleepInterval = maxIntervalCeilingMax;
2558-
}
2527+
// Wildcard endpoint, cluster, attribute, event.
2528+
auto attributePath = std::make_unique<AttributePathParams>();
2529+
auto eventPath = std::make_unique<EventPathParams>();
2530+
// We want to get event reports at the minInterval, not the maxInterval.
2531+
eventPath->mIsUrgentEvent = true;
2532+
ReadPrepareParams readParams(session.Value());
2533+
2534+
readParams.mMinIntervalFloorSeconds = 0;
2535+
// Select a max interval based on the device's claimed idle sleep interval.
2536+
auto idleSleepInterval = std::chrono::duration_cast<System::Clock::Seconds32>(
2537+
session.Value()->GetRemoteMRPConfig().mIdleRetransTimeout);
2538+
2539+
auto maxIntervalCeilingMin = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN);
2540+
if (idleSleepInterval < maxIntervalCeilingMin) {
2541+
idleSleepInterval = maxIntervalCeilingMin;
2542+
}
2543+
2544+
auto maxIntervalCeilingMax = System::Clock::Seconds32(MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX);
2545+
if (idleSleepInterval > maxIntervalCeilingMax) {
2546+
idleSleepInterval = maxIntervalCeilingMax;
2547+
}
25592548
#ifdef DEBUG
2560-
if (maxIntervalOverride.HasValue()) {
2561-
idleSleepInterval = maxIntervalOverride.Value();
2562-
}
2563-
#endif
2564-
readParams.mMaxIntervalCeilingSeconds = static_cast<uint16_t>(idleSleepInterval.count());
2565-
2566-
readParams.mpAttributePathParamsList = attributePath.get();
2567-
readParams.mAttributePathParamsListSize = 1;
2568-
readParams.mpEventPathParamsList = eventPath.get();
2569-
readParams.mEventPathParamsListSize = 1;
2570-
readParams.mKeepSubscriptions = true;
2571-
readParams.mIsFabricFiltered = false;
2572-
size_t dataVersionFilterListSize = 0;
2573-
DataVersionFilter * dataVersionFilterList;
2574-
[self _createDataVersionFilterListFromDictionary:dataVersions dataVersionFilterList:&dataVersionFilterList count:&dataVersionFilterListSize sizeReduction:dataVersionFilterListSizeReduction];
2575-
readParams.mDataVersionFilterListSize = dataVersionFilterListSize;
2576-
readParams.mpDataVersionFilterList = dataVersionFilterList;
2577-
attributePath.release();
2578-
eventPath.release();
2579-
2580-
// TODO: Change from local filter list generation to rehydrating ClusterStateCache ot take advantage of existing filter list sorting algorithm
2581-
2582-
// SendAutoResubscribeRequest cleans up the params, even on failure.
2583-
err = readClient->SendAutoResubscribeRequest(std::move(readParams));
2584-
if (err == CHIP_NO_ERROR) {
2585-
break;
2586-
}
2587-
2588-
// If error is not a "no memory" issue, then break and go through regular resubscribe logic
2589-
if (err != CHIP_ERROR_NO_MEMORY) {
2590-
break;
2591-
}
2592-
2593-
// If "no memory" error is not caused by data version filter list, break as well
2594-
if (!dataVersionFilterListSize) {
2595-
break;
2596-
}
2597-
2598-
// Now "no memory" could mean subscribe request packet space ran out. Reduce size and try again immediately
2599-
dataVersionFilterListSizeReduction++;
2549+
if (maxIntervalOverride.HasValue()) {
2550+
idleSleepInterval = maxIntervalOverride.Value();
26002551
}
2552+
#endif
2553+
readParams.mMaxIntervalCeilingSeconds = static_cast<uint16_t>(idleSleepInterval.count());
2554+
2555+
readParams.mpAttributePathParamsList = attributePath.get();
2556+
readParams.mAttributePathParamsListSize = 1;
2557+
readParams.mpEventPathParamsList = eventPath.get();
2558+
readParams.mEventPathParamsListSize = 1;
2559+
readParams.mKeepSubscriptions = true;
2560+
readParams.mIsFabricFiltered = false;
2561+
2562+
// Subscribe with data version filter list from our cache.
2563+
size_t dataVersionFilterListSize = 0;
2564+
DataVersionFilter * dataVersionFilterList;
2565+
[self _createDataVersionFilterListFromDictionary:[self _getCachedDataVersions] dataVersionFilterList:&dataVersionFilterList count:&dataVersionFilterListSize];
2566+
2567+
readParams.mDataVersionFilterListSize = dataVersionFilterListSize;
2568+
readParams.mpDataVersionFilterList = dataVersionFilterList;
2569+
attributePath.release();
2570+
eventPath.release();
2571+
2572+
// TODO: Change from local filter list generation to rehydrating ClusterStateCache to take advantage of existing filter list sorting algorithm
26012573

2574+
// SendAutoResubscribeRequest cleans up the params, even on failure.
2575+
CHIP_ERROR err = readClient->SendAutoResubscribeRequest(std::move(readParams));
26022576
if (err != CHIP_NO_ERROR) {
26032577
NSError * error = [MTRError errorForCHIPErrorCode:err logContext:self];
26042578
MTR_LOG_ERROR("%@ SendAutoResubscribeRequest error %@", self, error);
@@ -2610,7 +2584,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
26102584
return;
26112585
}
26122586

2613-
MTR_LOG("%@ Subscribe with data version list size %lu, reduced by %lu", self, static_cast<unsigned long>(dataVersions.count), static_cast<unsigned long>(dataVersionFilterListSizeReduction));
2587+
MTR_LOG("%@ Subscribe with data version list size %lu", self, static_cast<unsigned long>(dataVersionFilterListSize));
26142588

26152589
// Callback and ClusterStateCache and ReadClient will be deleted
26162590
// when OnDone is called.

0 commit comments

Comments
 (0)