Skip to content

Commit

Permalink
Fix handling of cluster state cache min event number.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Jul 25, 2023
1 parent 88f9a44 commit 6388938
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 41 deletions.
7 changes: 6 additions & 1 deletion src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
79 changes: 56 additions & 23 deletions src/controller/tests/TestEventCaching.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);

{
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}

//
Expand Down
49 changes: 32 additions & 17 deletions src/controller/tests/TestEventNumberCaching.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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(),
Expand All @@ -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);

Expand Down
1 change: 1 addition & 0 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1484,6 +1484,7 @@ - (void)test017_TestMTRDeviceBasics
[self waitForExpectations:@[ subscriptionExpectation ] timeout:60];

XCTAssertNotEqual(attributeReportsReceived, 0);
XCTAssertNotEqual(eventReportsReceived, 0);

attributeReportsReceived = 0;
eventReportsReceived = 0;
Expand Down

0 comments on commit 6388938

Please sign in to comment.