Fix handling of cluster state cache min event number. (#28273)

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.
project-chip · Feb 12, 2024 · 3675724 · 3675724
1 parent 147056c
commit 3675724
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 41 deletions.
diff --git 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
@@ -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<EventNumber> 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<EventNumber> 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<EventNumber> 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
@@ -128,9 +128,16 @@ class TestReadCallback : public app::ClusterStateCache::Callback
 {
 public:
     TestReadCallback() : mClusterCacheAdapter(*this, Optional<EventNumber>::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<EventNumber> 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
@@ -1484,6 +1484,7 @@ - (void)test017_TestMTRDeviceBasics
     [self waitForExpectations:@[ subscriptionExpectation ] timeout:60];
 
     XCTAssertNotEqual(attributeReportsReceived, 0);
+    XCTAssertNotEqual(eventReportsReceived, 0);
 
     attributeReportsReceived = 0;
     eventReportsReceived = 0;