diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 4422edaf6d0e35..6b609098a87cf4 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -243,7 +243,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b if (!aMoreChunks) { mPreviousReportsBeginGeneration = mCurrentReportsBeginGeneration; - ClearDirty(); + ClearForceDirtyFlag(); InteractionModelEngine::GetInstance()->ReleaseDataVersionFilterList(mpDataVersionFilterList); } diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 88b8ad8e13f2b5..b07ef01b2c956a 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -198,9 +198,7 @@ class ReadHandler : public Messaging::ExchangeDelegate enum class ReadHandlerFlags : uint8_t { // mHoldReport is used to prevent subscription data delivery while we are - // waiting for the min reporting interval to elapse. If we have to send a - // report immediately due to an urgent event being queued, - // UnblockUrgentEventDelivery can be used to force mHoldReport to false. + // waiting for the min reporting interval to elapse. HoldReport = (1 << 0), // mHoldSync is used to prevent subscription empty report delivery while we @@ -219,7 +217,6 @@ class ReadHandler : public Messaging::ExchangeDelegate PrimingReports = (1 << 3), ActiveSubscription = (1 << 4), FabricFiltered = (1 << 5), - // For subscriptions, we record the dirty set generation when we started to generate the last report. // The mCurrentReportsBeginGeneration records the generation at the start of the current report. This only/ // has a meaningful value while IsReporting() is true. @@ -227,6 +224,8 @@ class ReadHandler : public Messaging::ExchangeDelegate // mPreviousReportsBeginGeneration will be set to mCurrentReportsBeginGeneration after we send the last // chunk of the current report. Anything that was dirty with a generation earlier than // mPreviousReportsBeginGeneration has had its value sent to the client. + // when receiving initial request, it needs mark current handler as dirty. + // when there is urgent event, it needs mark current handler as dirty. ForceDirty = (1 << 6), // Don't need the response for report data if true @@ -300,8 +299,7 @@ class ReadHandler : public Messaging::ExchangeDelegate { return (mDirtyGeneration > mPreviousReportsBeginGeneration) || mFlags.Has(ReadHandlerFlags::ForceDirty); } - void ClearDirty() { mFlags.Clear(ReadHandlerFlags::ForceDirty); } - + void ClearForceDirtyFlag() { mFlags.Clear(ReadHandlerFlags::ForceDirty); } NodeId GetInitiatorNodeId() const { auto session = GetSession(); @@ -319,11 +317,7 @@ class ReadHandler : public Messaging::ExchangeDelegate auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; } - void UnblockUrgentEventDelivery() - { - mFlags.Clear(ReadHandlerFlags::HoldReport); - mFlags.Set(ReadHandlerFlags::ForceDirty); - } + void UnblockUrgentEventDelivery() { mFlags.Set(ReadHandlerFlags::ForceDirty); } const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } void SetAttributeEncodeState(const AttributeValueEncoder::AttributeEncodeState & aState) { mAttributeEncoderState = aState; } diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 792631f58acdac..b1c2f5a3db54ea 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -653,8 +653,7 @@ void Engine::Run() bool allReadClean = true; - imEngine->mReadHandlers.ForEachActiveObject([this, &allReadClean](ReadHandler * handler) { - UpdateReadHandlerDirty(*handler); + imEngine->mReadHandlers.ForEachActiveObject([&allReadClean](ReadHandler * handler) { if (handler->IsDirty()) { allReadClean = false; @@ -850,41 +849,6 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) return CHIP_NO_ERROR; } -void Engine::UpdateReadHandlerDirty(ReadHandler & aReadHandler) -{ - if (!aReadHandler.IsDirty()) - { - return; - } - - if (!aReadHandler.IsType(ReadHandler::InteractionType::Subscribe)) - { - return; - } - - bool intersected = false; - for (auto object = aReadHandler.GetAttributePathList(); object != nullptr; object = object->mpNext) - { - mGlobalDirtySet.ForEachActiveObject([&](auto * path) { - if (path->Intersects(object->mValue) && path->mGeneration > aReadHandler.mPreviousReportsBeginGeneration) - { - intersected = true; - return Loop::Break; - } - return Loop::Continue; - }); - if (intersected) - { - break; - } - } - if (!intersected) - { - aReadHandler.ClearDirty(); - ChipLogDetail(InteractionModel, "clear read handler dirty in UpdateReadHandlerDirty!"); - } -} - CHIP_ERROR Engine::SendReport(ReadHandler * apReadHandler, System::PacketBufferHandle && aPayload, bool aHasMoreChunks) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -974,8 +938,8 @@ CHIP_ERROR Engine::ScheduleEventDelivery(ConcreteEventPath & aPath, uint32_t aBy if (isUrgentEvent) { - ChipLogDetail(DataManagement, "urgent event schedule run"); - return ScheduleRun(); + ChipLogDetail(DataManagement, "urgent event would be sent after min interval"); + return CHIP_NO_ERROR; } return ScheduleBufferPressureEventDelivery(aBytesWritten); diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index dc6b6f5ea920a5..45754b490ee7c8 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -170,12 +170,6 @@ class Engine bool IsClusterDataVersionMatch(const ObjectList * aDataVersionFilterList, const ConcreteReadAttributePath & aPath); - /** - * Check all active subscription, if the subscription has no paths that intersect with global dirty set, - * it would clear dirty flag for that subscription - * - */ - void UpdateReadHandlerDirty(ReadHandler & aReadHandler); /** * Send Report via ReadHandler * diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index d97b11f3213a8d..f96f5279e75023 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1515,9 +1515,8 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a { app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, chip::app::ReadClient::InteractionType::Subscribe); - readPrepareParams.mpEventPathParamsList[0].mIsUrgentEvent = true; - delegate.mGotReport = false; - err = readClient.SendRequest(readPrepareParams); + delegate.mGotReport = false; + err = readClient.SendRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); @@ -1532,8 +1531,6 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = kTestClusterId; dirtyPath1.mEndpointId = kTestEndpointId; @@ -1563,6 +1560,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a // Test report with 2 different path delegate.mpReadHandler->mFlags.Set(ReadHandler::ReadHandlerFlags::HoldReport, false); delegate.mGotReport = false; + delegate.mGotEventResponse = false; delegate.mNumAttributeResponse = 0; printf("HereHere\n"); @@ -1575,6 +1573,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a ctx.DrainAndServiceIO(); NL_TEST_ASSERT(apSuite, delegate.mGotReport); + NL_TEST_ASSERT(apSuite, delegate.mGotEventResponse == true); NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 2 different path, and 1 same path @@ -1701,6 +1700,8 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite ctx.DrainAndServiceIO(); + System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp(); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); delegate.mpReadHandler = engine->ActiveHandlerAt(0); @@ -1711,12 +1712,37 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty() == true); delegate.mGotEventResponse = false; delegate.mGotReport = false; - ctx.DrainAndServiceIO(); - NL_TEST_ASSERT(apSuite, delegate.mGotEventResponse); + + // wait for min interval 2 seconds(in test, we use 1.9second considering the time variation), expect no event is received, + // then wait for 0.5 seconds, then the urgent event would be sent out + // currently DriveIOUntil will call `DriveIO` at least once, which means that if there is any CPU scheduling issues, + // there's a chance 1.9s will already have elapsed by the time we get there, which will result in DriveIO being called when + // it shouldn't. Better fix could happen inside DriveIOUntil, not sure the sideeffect there. + while (true) + { + if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(1900)) + { + break; + } + ctx.GetIOContext().DriveIO(); // at least one IO loop is guaranteed + } + + NL_TEST_ASSERT(apSuite, delegate.mGotEventResponse != true); + + startTime = System::SystemClock().GetMonotonicTimestamp(); + while (true) + { + if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(500)) + { + break; + } + ctx.GetIOContext().DriveIO(); // at least one IO loop is guaranteed + } + NL_TEST_ASSERT(apSuite, delegate.mGotEventResponse == true); } // By now we should have closed all exchanges and sent all pending acks, so @@ -2228,7 +2254,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu { app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, chip::app::ReadClient::InteractionType::Subscribe); - readPrepareParams.mpEventPathParamsList[0].mIsUrgentEvent = true; printf("\nSend first subscribe request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); delegate.mGotReport = false; err = readClient.SendRequest(readPrepareParams); @@ -2246,8 +2271,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = kTestClusterId; dirtyPath1.mEndpointId = kTestEndpointId; @@ -2548,7 +2571,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT { app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, chip::app::ReadClient::InteractionType::Subscribe); - readPrepareParams.mpEventPathParamsList[0].mIsUrgentEvent = true; printf("\nSend first subscribe request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); delegate.mGotReport = false; err = readClient.SendRequest(readPrepareParams); @@ -2565,8 +2587,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = Test::MockClusterId(2); dirtyPath1.mEndpointId = Test::kMockEndpoint3; @@ -2645,7 +2665,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui { app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, chip::app::ReadClient::InteractionType::Subscribe); - readPrepareParams.mpEventPathParamsList[0].mIsUrgentEvent = true; printf("\nSend first subscribe request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); delegate.mGotReport = false; err = readClient.SendRequest(readPrepareParams); @@ -2662,8 +2681,6 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = Test::MockClusterId(2); dirtyPath1.mEndpointId = Test::kMockEndpoint3;