Skip to content

Commit 1283995

Browse files
bzbarsky-applepull[bot]
bzbarsky-apple
authored and
pull[bot]
committed
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.
1 parent 6204b0d commit 1283995

File tree

4 files changed

+95
-41
lines changed

4 files changed

+95
-41
lines changed

‎src/app/ReadClient.cpp

+6-1
Original file line numberDiff line numberDiff line change
@@ -1186,7 +1186,12 @@ CHIP_ERROR ReadClient::GetMinEventNumber(const ReadPrepareParams & aReadPrepareP
11861186
}
11871187
else
11881188
{
1189-
return mpCallback.GetHighestReceivedEventNumber(aEventMin);
1189+
ReturnErrorOnFailure(mpCallback.GetHighestReceivedEventNumber(aEventMin));
1190+
if (aEventMin.HasValue())
1191+
{
1192+
// We want to start with the first event _after_ the last one we received.
1193+
aEventMin.SetValue(aEventMin.Value() + 1);
1194+
}
11901195
}
11911196
return CHIP_NO_ERROR;
11921197
}

‎src/controller/tests/TestEventCaching.cpp

+56-23
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,16 @@ class TestReadCallback : public app::ClusterStateCache::Callback
128128
{
129129
public:
130130
TestReadCallback() : mClusterCacheAdapter(*this) {}
131-
void OnDone(app::ReadClient *) {}
131+
void OnDone(app::ReadClient *) override {}
132+
133+
void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
134+
{
135+
++mEventsSeen;
136+
}
132137

133138
app::ClusterStateCache mClusterCacheAdapter;
139+
140+
size_t mEventsSeen = 0;
134141
};
135142

136143
namespace {
@@ -181,6 +188,7 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
181188
chip::EventNumber lastEventNumber;
182189

183190
GenerateEvents(apSuite, firstEventNumber, lastEventNumber);
191+
NL_TEST_ASSERT(apSuite, lastEventNumber > firstEventNumber);
184192

185193
app::EventPathParams eventPath;
186194
eventPath.mEndpointId = kTestEndpointId;
@@ -203,10 +211,12 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
203211

204212
uint8_t generationCount = 0;
205213
readCallback.mClusterCacheAdapter.ForEachEventData(
206-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
214+
[&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) {
207215
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
208216
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
209217
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
218+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber);
219+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
210220

211221
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
212222
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -216,21 +226,23 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
216226
return CHIP_NO_ERROR;
217227
});
218228

219-
NL_TEST_ASSERT(apSuite, generationCount == 5);
229+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1);
220230

221231
Optional<EventNumber> highestEventNumber;
222232
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
223-
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4);
233+
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber);
224234

225235
//
226236
// Re-run the iterator but pass in a path filter: EP*/TestCluster/EID*
227237
//
228238
generationCount = 0;
229239
readCallback.mClusterCacheAdapter.ForEachEventData(
230-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
240+
[&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) {
231241
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
232242
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
233243
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
244+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber);
245+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
234246

235247
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
236248
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -241,17 +253,19 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
241253
},
242254
app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId));
243255

244-
NL_TEST_ASSERT(apSuite, generationCount == 5);
256+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1);
245257

246258
//
247259
// Re-run the iterator but pass in a path filter: EP*/TestCluster/TestEvent
248260
//
249261
generationCount = 0;
250262
readCallback.mClusterCacheAdapter.ForEachEventData(
251-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
263+
[&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) {
252264
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
253265
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
254266
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
267+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber);
268+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
255269

256270
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
257271
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -262,17 +276,20 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
262276
},
263277
app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, Clusters::UnitTesting::Events::TestEvent::Id));
264278

265-
NL_TEST_ASSERT(apSuite, generationCount == 5);
279+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1);
266280

267281
//
268-
// Re-run the iterator but pass in a min event number filter (EventNumber = 1). We should only receive 4 events.
282+
// Re-run the iterator but pass in a min event number filter
283+
// (EventNumber = firstEventNumber + 1). We should only receive 4 events.
269284
//
270285
generationCount = 1;
271286
readCallback.mClusterCacheAdapter.ForEachEventData(
272-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
287+
[&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) {
273288
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
274289
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
275290
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
291+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber + 1);
292+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
276293

277294
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
278295
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -281,20 +298,23 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
281298
generationCount++;
282299
return CHIP_NO_ERROR;
283300
},
284-
app::EventPathParams(), 1);
301+
app::EventPathParams(), firstEventNumber + 1);
285302

286-
NL_TEST_ASSERT(apSuite, generationCount == 5);
303+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1);
287304

288305
//
289-
// Re-run the iterator but pass in a min event number filter (EventNumber = 1) AND a path filter. We should only receive 4
306+
// Re-run the iterator but pass in a min event number filter
307+
// (EventNumber = firstEventNumber + 1) AND a path filter. We should only receive 4
290308
// events.
291309
//
292310
generationCount = 1;
293311
readCallback.mClusterCacheAdapter.ForEachEventData(
294-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
312+
[&apSuite, &readCallback, &generationCount, firstEventNumber, lastEventNumber](const app::EventHeader & header) {
295313
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
296314
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
297315
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
316+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= firstEventNumber + 1);
317+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
298318

299319
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
300320
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -303,14 +323,15 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
303323
generationCount++;
304324
return CHIP_NO_ERROR;
305325
},
306-
app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId), 1);
326+
app::EventPathParams(kInvalidEndpointId, Clusters::UnitTesting::Id, kInvalidEventId), firstEventNumber + 1);
307327

308-
NL_TEST_ASSERT(apSuite, generationCount == 5);
328+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - firstEventNumber + 1);
309329
}
310330

311331
//
312332
// Generate more events.
313333
//
334+
const EventNumber oldFirstEventNumber = firstEventNumber;
314335
GenerateEvents(apSuite, firstEventNumber, lastEventNumber);
315336

316337
{
@@ -327,10 +348,12 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
327348
//
328349
uint8_t generationCount = 0;
329350
readCallback.mClusterCacheAdapter.ForEachEventData(
330-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
351+
[&apSuite, &readCallback, &generationCount, oldFirstEventNumber, lastEventNumber](const app::EventHeader & header) {
331352
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
332353
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
333354
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
355+
NL_TEST_ASSERT(apSuite, header.mEventNumber >= oldFirstEventNumber);
356+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
334357

335358
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
336359
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -341,7 +364,7 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
341364
return CHIP_NO_ERROR;
342365
});
343366

344-
NL_TEST_ASSERT(apSuite, generationCount == 10);
367+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - oldFirstEventNumber + 1);
345368

346369
Optional<EventNumber> highestEventNumber;
347370
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
@@ -368,18 +391,28 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
368391
app::ReadClient::InteractionType::Read);
369392

370393
readCallback.mClusterCacheAdapter.ClearEventCache();
371-
readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(3);
394+
constexpr EventNumber kLastSeenEventNumber = 3;
395+
NL_TEST_ASSERT(apSuite, kLastSeenEventNumber < lastEventNumber);
396+
readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(kLastSeenEventNumber);
397+
readParams.mEventNumber.ClearValue();
398+
399+
readCallback.mEventsSeen = 0;
372400

373401
NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR);
374402

375403
ctx.DrainAndServiceIO();
376404

377-
uint8_t generationCount = 4;
405+
// We should only get events with event numbers larger than kHighestEventNumberSeen.
406+
NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - kLastSeenEventNumber);
407+
408+
uint8_t generationCount = kLastSeenEventNumber + 1;
378409
readCallback.mClusterCacheAdapter.ForEachEventData(
379-
[&apSuite, &readCallback, &generationCount](const app::EventHeader & header) {
410+
[&apSuite, &readCallback, &generationCount, lastEventNumber](const app::EventHeader & header) {
380411
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
381412
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
382413
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
414+
NL_TEST_ASSERT(apSuite, header.mEventNumber > kLastSeenEventNumber);
415+
NL_TEST_ASSERT(apSuite, header.mEventNumber <= lastEventNumber);
383416

384417
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
385418
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) == CHIP_NO_ERROR);
@@ -390,10 +423,10 @@ void TestReadEvents::TestBasicCaching(nlTestSuite * apSuite, void * apContext)
390423
return CHIP_NO_ERROR;
391424
});
392425

393-
NL_TEST_ASSERT(apSuite, generationCount == 10);
426+
NL_TEST_ASSERT(apSuite, generationCount == lastEventNumber - oldFirstEventNumber + 1);
394427
Optional<EventNumber> highestEventNumber;
395428
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
396-
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 9);
429+
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber);
397430
}
398431

399432
//

‎src/controller/tests/TestEventNumberCaching.cpp

+32-17
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,16 @@ class TestReadCallback : public app::ClusterStateCache::Callback
128128
{
129129
public:
130130
TestReadCallback() : mClusterCacheAdapter(*this, Optional<EventNumber>::Missing(), false /*cacheData*/) {}
131-
void OnDone(app::ReadClient *) {}
131+
void OnDone(app::ReadClient *) override {}
132+
133+
void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
134+
{
135+
++mEventsSeen;
136+
}
132137

133138
app::ClusterStateCache mClusterCacheAdapter;
139+
140+
size_t mEventsSeen = 0;
134141
};
135142

136143
namespace {
@@ -178,6 +185,7 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont
178185
chip::EventNumber lastEventNumber;
179186

180187
GenerateEvents(apSuite, firstEventNumber, lastEventNumber);
188+
NL_TEST_ASSERT(apSuite, lastEventNumber > firstEventNumber);
181189

182190
app::EventPathParams eventPath;
183191
eventPath.mEndpointId = kTestEndpointId;
@@ -201,23 +209,22 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont
201209

202210
ctx.DrainAndServiceIO();
203211

204-
readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite, &readCallback](const app::EventHeader & header) {
205-
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
206-
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
207-
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
212+
NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - firstEventNumber + 1);
213+
214+
readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite](const app::EventHeader & header) {
215+
// We are not caching data.
216+
NL_TEST_ASSERT(apSuite, false);
208217

209-
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
210-
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) != CHIP_NO_ERROR);
211218
return CHIP_NO_ERROR;
212219
});
213220

214221
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
215-
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4);
222+
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber);
216223
}
217224

218225
//
219226
// Clear out the event cache and set its highest received event number to a non zero value. Validate that
220-
// we don't receive events lower than that value.
227+
// we don't receive events except ones larger than that value.
221228
//
222229
{
223230
app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mClusterCacheAdapter.GetBufferedCallback(),
@@ -227,24 +234,32 @@ void TestReadEvents::TestEventNumberCaching(nlTestSuite * apSuite, void * apCont
227234
Optional<EventNumber> highestEventNumber;
228235
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
229236
NL_TEST_ASSERT(apSuite, !highestEventNumber.HasValue());
230-
readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(3);
231237

238+
const EventNumber kHighestEventNumberSeen = lastEventNumber - 1;
239+
NL_TEST_ASSERT(apSuite, kHighestEventNumberSeen < lastEventNumber);
240+
241+
readCallback.mClusterCacheAdapter.SetHighestReceivedEventNumber(kHighestEventNumberSeen);
242+
243+
readCallback.mEventsSeen = 0;
244+
245+
readParams.mEventNumber.ClearValue();
246+
NL_TEST_ASSERT(apSuite, !readParams.mEventNumber.HasValue());
232247
NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR);
233248

234249
ctx.DrainAndServiceIO();
235250

236-
readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite, &readCallback](const app::EventHeader & header) {
237-
NL_TEST_ASSERT(apSuite, header.mPath.mClusterId == Clusters::UnitTesting::Id);
238-
NL_TEST_ASSERT(apSuite, header.mPath.mEventId == Clusters::UnitTesting::Events::TestEvent::Id);
239-
NL_TEST_ASSERT(apSuite, header.mPath.mEndpointId == kTestEndpointId);
251+
// We should only get events with event numbers larger than kHighestEventNumberSeen.
252+
NL_TEST_ASSERT(apSuite, readCallback.mEventsSeen == lastEventNumber - kHighestEventNumberSeen);
253+
254+
readCallback.mClusterCacheAdapter.ForEachEventData([&apSuite](const app::EventHeader & header) {
255+
// We are not caching data.
256+
NL_TEST_ASSERT(apSuite, false);
240257

241-
Clusters::UnitTesting::Events::TestEvent::DecodableType eventData;
242-
NL_TEST_ASSERT(apSuite, readCallback.mClusterCacheAdapter.Get(header.mEventNumber, eventData) != CHIP_NO_ERROR);
243258
return CHIP_NO_ERROR;
244259
});
245260

246261
readCallback.mClusterCacheAdapter.GetHighestReceivedEventNumber(highestEventNumber);
247-
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == 4);
262+
NL_TEST_ASSERT(apSuite, highestEventNumber.HasValue() && highestEventNumber.Value() == lastEventNumber);
248263
}
249264
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
250265

‎src/darwin/Framework/CHIPTests/MTRDeviceTests.m

+1
Original file line numberDiff line numberDiff line change
@@ -1484,6 +1484,7 @@ - (void)test017_TestMTRDeviceBasics
14841484
[self waitForExpectations:@[ subscriptionExpectation ] timeout:60];
14851485

14861486
XCTAssertNotEqual(attributeReportsReceived, 0);
1487+
XCTAssertNotEqual(eventReportsReceived, 0);
14871488

14881489
attributeReportsReceived = 0;
14891490
eventReportsReceived = 0;

0 commit comments

Comments
 (0)