From 638893891110d9819dbf1860b3c1bb20f9434868 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 25 Jul 2023 14:04:18 -0400 Subject: [PATCH] Fix handling of cluster state cache min event number. When initializing a new ReadClient with an existing ClusterStateCache, we should be setting the min event number to 1 more than the last event the cache saw. ReadClient was not doing this correctly. --- src/app/ReadClient.cpp | 7 +- src/controller/tests/TestEventCaching.cpp | 79 +++++++++++++------ .../tests/TestEventNumberCaching.cpp | 49 ++++++++---- .../Framework/CHIPTests/MTRDeviceTests.m | 1 + 4 files changed, 95 insertions(+), 41 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 23b57df99ddbcf..c9df6638b708ef 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -1186,7 +1186,12 @@ CHIP_ERROR ReadClient::GetMinEventNumber(const ReadPrepareParams & aReadPrepareP } else { - return mpCallback.GetHighestReceivedEventNumber(aEventMin); + ReturnErrorOnFailure(mpCallback.GetHighestReceivedEventNumber(aEventMin)); + if (aEventMin.HasValue()) + { + // We want to start with the first event _after_ the last one we received. + aEventMin.SetValue(aEventMin.Value() + 1); + } } return CHIP_NO_ERROR; } diff --git a/src/controller/tests/TestEventCaching.cpp b/src/controller/tests/TestEventCaching.cpp index 06100250bdc2f4..24a101aeeb95b8 100644 --- a/src/controller/tests/TestEventCaching.cpp +++ b/src/controller/tests/TestEventCaching.cpp @@ -128,9 +128,16 @@ class TestReadCallback : public app::ClusterStateCache::Callback { public: TestReadCallback() : mClusterCacheAdapter(*this) {} - void OnDone(app::ReadClient *) {} + void OnDone(app::ReadClient *) override {} + + void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override + { + ++mEventsSeen; + } app::ClusterStateCache mClusterCacheAdapter; + + size_t mEventsSeen = 0; }; namespace { @@ -181,6 +188,7 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) chip::EventNumber lastEventNumber; GenerateEvents(apSuite, firstEventNumber, lastEventNumber); + NL_TEST_ASSERT(apSuite, lastEventNumber > firstEventNumber); app::EventPathParams eventPath; eventPath.mEndpointId = kTestEndpointId; @@ -203,10 +211,12 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) uint8_t generationCount = 0; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -216,21 +226,23 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) return CHIP_NO_ERROR; }); - NL_TEST_ASSERT(apSuite, generationCount == 5); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1); Optional highestEventNumber; readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); - NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4); + NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber); // // Re-run the iterator but pass in a path filter: EP*/TestCluster/EID* // generationCount = 0; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -241,17 +253,19 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) }, app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId)); - NL_TEST_ASSERT(apSuite, generationCount == 5); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1); // // Re-run the iterator but pass in a path filter: EP*/TestCluster/TestEvent // generationCount = 0; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -262,17 +276,20 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) }, app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, Clusters::UnitTesting::Events::TestEvent::Id)); - NL_TEST_ASSERT(apSuite, generationCount == 5); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1); // - // Re-run the iterator but pass in a min event number filter (EventNumber = 1). We should only receive 4 events. + // Re-run the iterator but pass in a min event number filter + // (EventNumber = firstEventNumber + 1). We should only receive 4 events. // generationCount = 1; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber + 1); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -281,20 +298,23 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) generationCount++; return CHIP_NO_ERROR; }, - app::EventPathParams(), 1); + app::EventPathParams(), firstEventNumber + 1); - NL_TEST_ASSERT(apSuite, generationCount == 5); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1); // - // Re-run the iterator but pass in a min event number filter (EventNumber = 1) AND a path filter. We should only receive 4 + // Re-run the iterator but pass in a min event number filter + // (EventNumber = firstEventNumber + 1) AND a path filter. We should only receive 4 // events. // generationCount = 1; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber + 1); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -303,14 +323,15 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) generationCount++; return CHIP_NO_ERROR; }, - app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId), 1); + app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId), firstEventNumber + 1); - NL_TEST_ASSERT(apSuite, generationCount == 5); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1); } // // Generate more events. // + const EventNumber oldFirstEventNumber = firstEventNumber; GenerateEvents(apSuite, firstEventNumber, lastEventNumber); { @@ -327,10 +348,12 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) // uint8_t generationCount = 0; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, oldFirstEventNumber, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber >= oldFirstEventNumber); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -341,7 +364,7 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) return CHIP_NO_ERROR; }); - NL_TEST_ASSERT(apSuite, generationCount == 10); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - oldFirstEventNumber + 1); Optional highestEventNumber; readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); @@ -368,18 +391,28 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) app::ReadClient::InteractionType::Read); readCallback.mClusterCacheAdapter.ClearEventCache(); - readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(3); + constexpr EventNumber kLastSeenEventNumber = 3; + NL_TEST_ASSERT(apSuite, kLastSeenEventNumber < lastEventNumber); + readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(kLastSeenEventNumber); + readParams.mEventNumber.ClearValue(); + + readCallback.mEventsSeen = 0; NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); - uint8_t generationCount = 4; + // We should only get events with event numbers larger than kHighestEventNumberSeen. + NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - kLastSeenEventNumber); + + uint8_t generationCount = kLastSeenEventNumber + 1; readCallback.mClusterCacheAdapter.ForEachEventData( - [&apSuite, &readCallback, &generationCount](const app::EventHeader & header) { + [&apSuite, &readCallback, &generationCount, lastEventNumber](const app::EventHeader & header) { NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, header.mEventNumber > kLastSeenEventNumber); + NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber); Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR); @@ -390,10 +423,10 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext) return CHIP_NO_ERROR; }); - NL_TEST_ASSERT(apSuite, generationCount == 10); + NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - oldFirstEventNumber + 1); Optional highestEventNumber; readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); - NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 9); + NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber); } // diff --git a/src/controller/tests/TestEventNumberCaching.cpp b/src/controller/tests/TestEventNumberCaching.cpp index 8c291013e874c5..982e98fcc2cc05 100644 --- a/src/controller/tests/TestEventNumberCaching.cpp +++ b/src/controller/tests/TestEventNumberCaching.cpp @@ -128,9 +128,16 @@ class TestReadCallback : public app::ClusterStateCache::Callback { public: TestReadCallback() : mClusterCacheAdapter(*this, Optional::Missing(), false /*cacheData*/) {} - void OnDone(app::ReadClient *) {} + void OnDone(app::ReadClient *) override {} + + void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override + { + ++mEventsSeen; + } app::ClusterStateCache mClusterCacheAdapter; + + size_t mEventsSeen = 0; }; namespace { @@ -178,6 +185,7 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont chip::EventNumber lastEventNumber; GenerateEvents(apSuite, firstEventNumber, lastEventNumber); + NL_TEST_ASSERT(apSuite, lastEventNumber > firstEventNumber); app::EventPathParams eventPath; eventPath.mEndpointId = kTestEndpointId; @@ -201,23 +209,22 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont ctx.DrainAndServiceIO(); - readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite, &readCallback](const app::EventHeader & header) { - NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); - NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); - NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - firstEventNumber + 1); + + readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite](const app::EventHeader & header) { + // We are not caching data. + NL_TEST_ASSERT(apSuite, false); - Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; - NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) != CHIP_NO_ERROR); return CHIP_NO_ERROR; }); readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); - NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4); + NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber); } // // Clear out the event cache and set its highest received event number to a non zero value. Validate that - // we don't receive events lower than that value. + // we don't receive events except ones larger than that value. // { app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mClusterCacheAdapter.GetBufferedCallback(), @@ -227,24 +234,32 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont Optional highestEventNumber; readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); NL_TEST_ASSERT(apSuite, !highestEventNumber.HasValue()); - readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(3); + const EventNumber kHighestEventNumberSeen = lastEventNumber - 1; + NL_TEST_ASSERT(apSuite, kHighestEventNumberSeen < lastEventNumber); + + readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(kHighestEventNumberSeen); + + readCallback.mEventsSeen = 0; + + readParams.mEventNumber.ClearValue(); + NL_TEST_ASSERT(apSuite, !readParams.mEventNumber.HasValue()); NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); - readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite, &readCallback](const app::EventHeader & header) { - NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id); - NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id); - NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId); + // We should only get events with event numbers larger than kHighestEventNumberSeen. + NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - kHighestEventNumberSeen); + + readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite](const app::EventHeader & header) { + // We are not caching data. + NL_TEST_ASSERT(apSuite, false); - Clusters::UnitTesting::Events::TestEvent::DecodableType eventData; - NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) != CHIP_NO_ERROR); return CHIP_NO_ERROR; }); readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber); - NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4); + NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber); } NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index a4e0577f8f92ef..0d9b693296bc2d 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1484,6 +1484,7 @@ - (void)test017_TestMTRDeviceBasics [self waitForExpectations:@[ subscriptionExpectation ] timeout:60]; XCTAssertNotEqual(attributeReportsReceived, 0); + XCTAssertNotEqual(eventReportsReceived, 0); attributeReportsReceived = 0; eventReportsReceived = 0;