From cc9fd1ed36f07f8a0310ad6faa58eead723bef96 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Wed, 14 Jun 2023 15:01:31 -0400 Subject: [PATCH 1/8] Updated function names and description related to the scheduling of subcription reports in the ReadHandler to explicit its functionnality --- examples/all-clusters-app/nxp/mw320/main.cpp | 12 ++-- src/app/ReadHandler.cpp | 76 ++++++++++---------- src/app/ReadHandler.h | 68 +++++++++++------- src/app/reporting/Engine.cpp | 14 ++-- src/app/tests/TestReadInteraction.cpp | 62 ++++++++-------- 5 files changed, 125 insertions(+), 107 deletions(-) diff --git a/examples/all-clusters-app/nxp/mw320/main.cpp b/examples/all-clusters-app/nxp/mw320/main.cpp index 23675050c73052..b8b1fe85bfabdd 100644 --- a/examples/all-clusters-app/nxp/mw320/main.cpp +++ b/examples/all-clusters-app/nxp/mw320/main.cpp @@ -14,8 +14,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -//#include "FreeRTOS.h" -//#include "task.h" +// #include "FreeRTOS.h" +// #include "task.h" #include @@ -28,7 +28,7 @@ #include #include -//#include //==> rm from TE7.5 +// #include //==> rm from TE7.5 #include #include #include @@ -59,9 +59,9 @@ #include "app/clusters/ota-requestor/DefaultOTARequestor.h" #include "app/clusters/ota-requestor/DefaultOTARequestorDriver.h" #include "app/clusters/ota-requestor/DefaultOTARequestorStorage.h" -//#include +// #include #include "platform/nxp/mw320/OTAImageProcessorImpl.h" -//#include "app/clusters/ota-requestor/OTARequestorDriver.h" +// #include "app/clusters/ota-requestor/OTARequestorDriver.h" // for ota module test #include "mw320_ota.h" @@ -1528,7 +1528,7 @@ static void OnSwitchAttributeChangeCallback(EndpointId endpointId, AttributeId a ReadHandler * phandler = pimEngine->ActiveHandlerAt(i); if (phandler->IsType(chip::app::ReadHandler::InteractionType::Subscribe) && (phandler->IsGeneratingReports() || phandler->IsAwaitingReportResponse())) { - phandler->UnblockUrgentEventDelivery(); + phandler->ForceDirtyState(); do_sendrpt = true; break; } diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 1c2511b497b0f2..af4c4b8dfed73e 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -60,7 +60,7 @@ ReadHandler::ReadHandler(ManagementCallback & apCallback, Messaging::ExchangeCon mLastWrittenEventsBytes = 0; mTransactionStartGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); mFlags.ClearAll(); - SetStateFlag(ReadHandlerFlags::PrimingReports); + SetStateFlagAndScheduleReport(ReadHandlerFlags::PrimingReports); mSessionHandle.Grab(mExchangeCtx->GetSessionHandle()); } @@ -80,7 +80,7 @@ void ReadHandler::ResumeSubscription(CASESessionManager & caseSessionManager, mSubscriptionId = subscriptionInfo.mSubscriptionId; mMinIntervalFloorSeconds = subscriptionInfo.mMinInterval; mMaxInterval = subscriptionInfo.mMaxInterval; - SetStateFlag(ReadHandlerFlags::FabricFiltered, subscriptionInfo.mFabricFiltered); + SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, subscriptionInfo.mFabricFiltered); // Move dynamically allocated attributes and events from the SubscriptionInfo struct into // the object pool managed by the IM engine @@ -124,10 +124,10 @@ ReadHandler::~ReadHandler() if (IsType(InteractionType::Subscribe)) { InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( - OnUnblockHoldReportCallback, this); + MinIntervalExpiredCallback, this); InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( - OnRefreshSubscribeTimerSyncCallback, this); + MaxIntervalExpiredCallback, this); } if (IsAwaitingReportResponse()) @@ -187,7 +187,7 @@ void ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload) else { // Force us to be in a dirty state so we get processed by the reporting - SetStateFlag(ReadHandlerFlags::ForceDirty); + SetStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); } } @@ -214,7 +214,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange { err = SendSubscribeResponse(); - SetStateFlag(ReadHandlerFlags::ActiveSubscription); + SetStateFlagAndScheduleReport(ReadHandlerFlags::ActiveSubscription); auto * appCallback = mManagementCallback.GetAppCallback(); if (appCallback) @@ -248,7 +248,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aStatus) { - VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnLogError(IsReportableNow(), CHIP_ERROR_INCORRECT_STATE); if (IsPriming() || IsChunkedReport()) { mSessionHandle.Grab(mExchangeCtx->GetSessionHandle()); @@ -272,7 +272,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks) { - VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnLogError(IsReportableNow(), CHIP_ERROR_INCORRECT_STATE); VerifyOrDie(!IsAwaitingReportResponse()); // Should not be reportable! if (IsPriming() || IsChunkedReport()) { @@ -297,7 +297,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b { mCurrentReportsBeginGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); } - SetStateFlag(ReadHandlerFlags::ChunkedReport, aMoreChunks); + SetStateFlagAndScheduleReport(ReadHandlerFlags::ChunkedReport, aMoreChunks); bool responseExpected = IsType(InteractionType::Subscribe) || aMoreChunks; mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime); @@ -319,10 +319,10 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b if (IsType(InteractionType::Subscribe) && !IsPriming()) { - // Ignore the error from RefreshSubscribeSyncTimer. If we've + // Ignore the error from UpdateReportTimer. If we've // successfully sent the message, we need to return success from // this method. - RefreshSubscribeSyncTimer(); + UpdateReportTimer(); } } if (!aMoreChunks) @@ -442,7 +442,7 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa bool isFabricFiltered; ReturnErrorOnFailure(readRequestParser.GetIsFabricFiltered(&isFabricFiltered)); - SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered); + SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, isFabricFiltered); ReturnErrorOnFailure(readRequestParser.ExitContainer()); MoveToState(HandlerState::GeneratingReports); @@ -591,7 +591,7 @@ void ReadHandler::MoveToState(const HandlerState aTargetState) // If we just unblocked sending reports, let's go ahead and schedule the reporting // engine to run to kick that off. // - if (aTargetState == HandlerState::GeneratingReports && IsReportable()) + if (aTargetState == HandlerState::GeneratingReports && IsReportableNow()) { InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); } @@ -634,9 +634,9 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse() ReturnErrorOnFailure(writer.Finalize(&packet)); VerifyOrReturnLogError(mExchangeCtx, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(RefreshSubscribeSyncTimer()); + ReturnErrorOnFailure(UpdateReportTimer()); - ClearStateFlag(ReadHandlerFlags::PrimingReports); + ClearStateFlagAndScheduleReport(ReadHandlerFlags::PrimingReports); return mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeResponse, std::move(packet)); } @@ -718,7 +718,7 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP bool isFabricFiltered; ReturnErrorOnFailure(subscribeRequestParser.GetIsFabricFiltered(&isFabricFiltered)); - SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered); + SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, isFabricFiltered); ReturnErrorOnFailure(Crypto::DRBG_get_bytes(reinterpret_cast(&mSubscriptionId), sizeof(mSubscriptionId))); ReturnErrorOnFailure(subscribeRequestParser.ExitContainer()); MoveToState(HandlerState::GeneratingReports); @@ -753,42 +753,42 @@ void ReadHandler::PersistSubscription() } } -void ReadHandler::OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState) +void ReadHandler::MinIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState) { VerifyOrReturn(apAppState != nullptr); ReadHandler * readHandler = static_cast(apAppState); ChipLogDetail(DataManagement, "Unblock report hold after min %d seconds", readHandler->mMinIntervalFloorSeconds); - readHandler->ClearStateFlag(ReadHandlerFlags::HoldReport); + readHandler->ClearStateFlagAndScheduleReport(ReadHandlerFlags::HoldMinInterval); InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( - System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds), - OnRefreshSubscribeTimerSyncCallback, readHandler); + System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds), MaxIntervalExpiredCallback, + readHandler); } -void ReadHandler::OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState) +void ReadHandler::MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState) { VerifyOrReturn(apAppState != nullptr); ReadHandler * readHandler = static_cast(apAppState); - readHandler->ClearStateFlag(ReadHandlerFlags::HoldSync); + readHandler->ClearStateFlagAndScheduleReport(ReadHandlerFlags::HoldMaxInterval); ChipLogProgress(DataManagement, "Refresh subscribe timer sync after %d seconds", readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds); } -CHIP_ERROR ReadHandler::RefreshSubscribeSyncTimer() +CHIP_ERROR ReadHandler::UpdateReportTimer() { InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( - OnUnblockHoldReportCallback, this); + MinIntervalExpiredCallback, this); InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( - OnRefreshSubscribeTimerSyncCallback, this); + MaxIntervalExpiredCallback, this); if (!IsChunkedReport()) { ChipLogProgress(DataManagement, "Refresh Subscribe Sync Timer with min %d seconds and max %d seconds", mMinIntervalFloorSeconds, mMaxInterval); - SetStateFlag(ReadHandlerFlags::HoldReport); - SetStateFlag(ReadHandlerFlags::HoldSync); + SetStateFlagAndScheduleReport(ReadHandlerFlags::HoldMinInterval); + SetStateFlagAndScheduleReport(ReadHandlerFlags::HoldMaxInterval); ReturnErrorOnFailure( InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( - System::Clock::Seconds16(mMinIntervalFloorSeconds), OnUnblockHoldReportCallback, this)); + System::Clock::Seconds16(mMinIntervalFloorSeconds), MinIntervalExpiredCallback, this)); } return CHIP_NO_ERROR; @@ -800,13 +800,13 @@ void ReadHandler::ResetPathIterator() mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); } -void ReadHandler::SetDirty(const AttributePathParams & aAttributeChanged) +void ReadHandler::UpdateDirtyGeneration(const AttributePathParams & aAttributeChanged) { ConcreteAttributePath path; mDirtyGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); - // We won't reset the path iterator for every SetDirty call to reduce the number of full data reports. + // We won't reset the path iterator for every UpdateDirtyGeneration call to reduce the number of full data reports. // The iterator will be reset after finishing each report session. // // Here we just reset the iterator to the beginning of the current cluster, if the dirty path affects it. @@ -829,7 +829,7 @@ void ReadHandler::SetDirty(const AttributePathParams & aAttributeChanged) mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); } - if (IsReportable()) + if (IsReportableNow()) { InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); } @@ -844,25 +844,25 @@ Transport::SecureSession * ReadHandler::GetSession() const return mSessionHandle->AsSecureSession(); } -void ReadHandler::UnblockUrgentEventDelivery() +void ReadHandler::ForceDirtyState() { - SetStateFlag(ReadHandlerFlags::ForceDirty); + SetStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); } -void ReadHandler::SetStateFlag(ReadHandlerFlags aFlag, bool aValue) +void ReadHandler::SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aValue) { - bool oldReportable = IsReportable(); + bool oldReportable = IsReportableNow(); mFlags.Set(aFlag, aValue); // If we became reportable, schedule a reporting run. - if (!oldReportable && IsReportable()) + if (!oldReportable && IsReportableNow()) { InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); } } -void ReadHandler::ClearStateFlag(ReadHandlerFlags aFlag) +void ReadHandler::ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag) { - SetStateFlag(aFlag, false); + SetStateFlagAndScheduleReport(aFlag, false); } void ReadHandler::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index a933aa76e51389..a94fa87f6f06a3 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -212,15 +212,15 @@ class ReadHandler : public Messaging::ExchangeDelegate enum class ReadHandlerFlags : uint8_t { - // mHoldReport is used to prevent subscription data delivery while we are + // mHoldMinInterval is used to prevent subscription data delivery while we are // waiting for the min reporting interval to elapse. - HoldReport = (1 << 0), + HoldMinInterval = (1 << 0), - // mHoldSync is used to prevent subscription empty report delivery while we - // are waiting for the max reporting interval to elaps. When mHoldSync + // mHoldMaxInterval is used to prevent subscription empty report delivery while we + // are waiting for the max reporting interval to elaps. When mHoldMaxInterval // becomes false, we are allowed to send an empty report to keep the // subscription alive on the client. - HoldSync = (1 << 1), + HoldMaxInterval = (1 << 1), // The flag indicating we are in the middle of a series of chunked report messages, this flag will be cleared during // sending last chunked message. @@ -290,13 +290,19 @@ class ReadHandler : public Messaging::ExchangeDelegate bool IsFromSubscriber(Messaging::ExchangeContext & apExchangeContext) const; bool IsIdle() const { return mState == HandlerState::Idle; } - bool IsReportable() const + + /// @brief Returns whether the ReadHandler is in a state where it can send a report immediately. The main conditions for that + /// are that the handler is in a generating reports state and that the, meaning it has been subscribe to or is processing a read + /// request, that the min interval has elapsed and that either max interval has elapsed, forcing an empty report, or that the + /// handler is dirty. This function is used to determine whether a handler should be scheduled for a run. + /// @return + bool IsReportableNow() const { - // Important: Anything that changes the state IsReportable depends on in - // a way that causes IsReportable to become true must call ScheduleRun + // Important: Anything that changes the state IsReportableNow depends on in + // a way that causes IsReportableNow to become true must call ScheduleRun // on the reporting engine. - return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::HoldReport) && - (IsDirty() || !mFlags.Has(ReadHandlerFlags::HoldSync)); + return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::HoldMinInterval) && + (IsDirty() || !mFlags.Has(ReadHandlerFlags::HoldMaxInterval)); } bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; } bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; } @@ -323,15 +329,15 @@ class ReadHandler : public Messaging::ExchangeDelegate void GetSubscriptionId(SubscriptionId & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; } AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; } - /** - * Notify the read handler that a set of attribute paths has been marked dirty. - */ - void SetDirty(const AttributePathParams & aAttributeChanged); + /// @brief Update mDirtyGeneration to the latest DirtySetGeneration of the InteractionModelEngine. As this might make the + /// readhandler dirty, this function checks if the readhandler is now reportable and schedules a run if it is the case. + /// @param aAttributeChanged + void UpdateDirtyGeneration(const AttributePathParams & aAttributeChanged); bool IsDirty() const { return (mDirtyGeneration > mPreviousReportsBeginGeneration) || mFlags.Has(ReadHandlerFlags::ForceDirty); } - void ClearForceDirtyFlag() { ClearStateFlag(ReadHandlerFlags::ForceDirty); } + void ClearForceDirtyFlag() { ClearStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); } NodeId GetInitiatorNodeId() const { auto session = GetSession(); @@ -349,7 +355,9 @@ class ReadHandler : public Messaging::ExchangeDelegate auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; } - void UnblockUrgentEventDelivery(); + /// @brief This allows the read handler to schedule a run as soon as the min interval has elapsed by setting the read handler to + /// a dirty state regardless of the attributes change, thus being reportable as soon as the min interval callback happends. + void ForceDirtyState(); const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } void SetAttributeEncodeState(const AttributeValueEncoder::AttributeEncodeState & aState) { mAttributeEncoderState = aState; } @@ -396,9 +404,12 @@ class ReadHandler : public Messaging::ExchangeDelegate */ void Close(CloseOptions options = CloseOptions::kDropPersistedSubscription); - static void OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState); - static void OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState); - CHIP_ERROR RefreshSubscribeSyncTimer(); + /// @brief This function is called when the min interval timer has expired, it restarts the timer on a timeout equalt to the + /// difference between the max interval and the min interval. + static void MinIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState); + static void MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState); + /// @brief This function is called when a report is sent and it restarts the min interval timer. + CHIP_ERROR UpdateReportTimer(); CHIP_ERROR SendSubscribeResponse(); CHIP_ERROR ProcessSubscribeRequest(System::PacketBufferHandle && aPayload); CHIP_ERROR ProcessReadRequest(System::PacketBufferHandle && aPayload); @@ -416,9 +427,16 @@ class ReadHandler : public Messaging::ExchangeDelegate void PersistSubscription(); - // Helpers for managing our state flags properly. - void SetStateFlag(ReadHandlerFlags aFlag, bool aValue = true); - void ClearStateFlag(ReadHandlerFlags aFlag); + /// @brief This function modifies at state Flag in the read handler and schedules an engine run if the read handler want from a + /// non reportable state to a reportable state, resulting in the emission of a report. + /// @param aFlag Flag to set + /// @param aValue Flag new value + void SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aValue = true); + + /// @brief This function calls the SetStateFlagAndScheduleReport with the flag value to false, thus possibly emitting a report + /// generation. + /// @param aFlag + void ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag); // Helpers for continuing the subscription resumption static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, @@ -436,7 +454,7 @@ class ReadHandler : public Messaging::ExchangeDelegate // current generation when we started sending the last set reports that we completed. // // This allows us to reset the iterator to the beginning of the current - // cluster instead of the beginning of the whole report in SetDirty, without + // cluster instead of the beginning of the whole report in UpdateDirtyGeneration, without // permanently missing dirty any paths. uint64_t mDirtyGeneration = 0; @@ -450,14 +468,14 @@ class ReadHandler : public Messaging::ExchangeDelegate /* * (mDirtyGeneration = b > a, this is a dirty read handler) * +- Start Report -> mCurrentReportsBeginGeneration = c - * | +- SetDirty (Attribute Y) -> mDirtyGeneration = d + * | +- UpdateDirtyGeneration (Attribute Y) -> mDirtyGeneration = d * | | +- Last Chunk -> mPreviousReportsBeginGeneration = mCurrentReportsBeginGeneration = c * | | | +- (mDirtyGeneration = d) > (mPreviousReportsBeginGeneration = c), this is a dirty read handler * | | | | Attribute X has a dirty generation less than c, Attribute Y has a dirty generation larger than c * | | | | So Y will be included in the report but X will not be inclued in this report. * -a--b--c------d-----e---f---> Generation * | | - * | +- SetDirty (Attribute X) (mDirtyGeneration = b) + * | +- UpdateDirtyGeneration (Attribute X) (mDirtyGeneration = b) * +- mPreviousReportsBeginGeneration * For read handler, if mDirtyGeneration > mPreviousReportsBeginGeneration, then we regard it as a dirty read handler, and it * should generate report on timeout reached. diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 0f9d592737ce79..8f449e313f28d0 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -636,7 +636,7 @@ void Engine::Run() ReadHandler * readHandler = imEngine->ActiveHandlerAt(mCurReadHandlerIdx % (uint32_t) imEngine->mReadHandlers.Allocated()); VerifyOrDie(readHandler != nullptr); - if (readHandler->IsReportable()) + if (readHandler->IsReportableNow()) { mRunningReadHandler = readHandler; CHIP_ERROR err = BuildAndSendSingleReportData(readHandler); @@ -826,16 +826,16 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) bool intersectsInterestPath = false; InteractionModelEngine::GetInstance()->mReadHandlers.ForEachActiveObject( [&aAttributePath, &intersectsInterestPath](ReadHandler * handler) { - // We call SetDirty for both read interactions and subscribe interactions, since we may send inconsistent attribute data - // between two chunks. SetDirty will be ignored automatically by read handlers which are waiting for a response to the - // last message chunk for read interactions. + // We call UpdateDirtyGeneration for both read interactions and subscribe interactions, since we may send inconsistent + // attribute data between two chunks. UpdateDirtyGeneration will not schedule a new run for read handlers which are + // waiting for a response to the last message chunk for read interactions. if (handler->IsGeneratingReports() || handler->IsAwaitingReportResponse()) { for (auto object = handler->GetAttributePathList(); object != nullptr; object = object->mpNext) { if (object->mValue.Intersects(aAttributePath)) { - handler->SetDirty(aAttributePath); + handler->UpdateDirtyGeneration(aAttributePath); intersectsInterestPath = true; break; } @@ -937,7 +937,7 @@ CHIP_ERROR Engine::ScheduleEventDelivery(ConcreteEventPath & aPath, uint32_t aBy if (interestedPath->mValue.IsEventPathSupersetOf(aPath) && interestedPath->mValue.mIsUrgentEvent) { isUrgentEvent = true; - handler->UnblockUrgentEventDelivery(); + handler->ForceDirtyState(); break; } } @@ -967,7 +967,7 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) return Loop::Continue; } - handler->UnblockUrgentEventDelivery(); + handler->ForceDirtyState(); return Loop::Continue; }); diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index fe0d2a04e39731..f6a689a0c2d0d7 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1587,7 +1587,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a dirtyPath5.mAttributeId = 4; // Test report with 2 different path - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mGotEventResponse = false; delegate.mNumAttributeResponse = 0; @@ -1604,7 +1604,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 2 different path, and 1 same path - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1620,7 +1620,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 3 different path, and one path is overlapped with another - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1636,7 +1636,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 3 different path, all are not overlapped, one path is not interested for current subscription - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1652,8 +1652,8 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test empty report - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldSync, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled()); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -1740,12 +1740,12 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); delegate.mGotEventResponse = false; delegate.mGotReport = false; - NL_TEST_ASSERT(apSuite, nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); nonUrgentDelegate.mGotEventResponse = false; nonUrgentDelegate.mGotReport = false; @@ -1781,13 +1781,13 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite // Since we just sent a report for our urgent subscription, we should have our min interval timer // running again. - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty()); delegate.mGotEventResponse = false; // For our non-urgent subscription, we did not send anything, so we // should not have a min interval timer running there. - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); // Wait for the min interval timer to fire. @@ -1807,15 +1807,15 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite // min-interval timer should have fired, and our handler should still // not be dirty or even reportable. - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty()); - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsReportable()); + NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsReportableNow()); // And the non-urgent one should not have changed state either, since // it's waiting for the max-interval. - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldReport)); + NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportable()); + NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportableNow()); // There should be no reporting run scheduled. This is very important; // otherwise we can get a false-positive pass below because the run was @@ -1827,11 +1827,11 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite // Urgent read handler should now be dirty, and reportable. NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsReportable()); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsReportableNow()); // Non-urgent read handler should not be reportable. NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportable()); + NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportableNow()); // Still no reporting should have happened. NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); @@ -1914,7 +1914,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap // Set a concrete path dirty { - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -1935,7 +1935,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap // Set a endpoint dirty { - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; delegate.mNumArrayItems = 0; @@ -2027,7 +2027,7 @@ void TestReadInteraction::TestSubscribePartialOverlap(nlTestSuite * apSuite, voi // Set a partial overlapped path dirty { - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2104,7 +2104,7 @@ void TestReadInteraction::TestSubscribeSetDirtyFullyOverlap(nlTestSuite * apSuit // Set a full overlapped path dirty and expect to receive one E2C3A1 { - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2225,8 +2225,8 @@ void TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip(nlTestSuite NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); delegate.mpReadHandler = engine->ActiveHandlerAt(0); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldSync, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled()); ctx.DrainAndServiceIO(); @@ -2401,7 +2401,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu dirtyPath2.mAttributeId = 2; // Test report with 2 different path - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2415,8 +2415,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu NL_TEST_ASSERT(apSuite, delegate.mGotReport); NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldSync, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; ctx.ExpireSessionBobToAlice(); @@ -2763,8 +2763,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT dirtyPath1.mEndpointId = Test::kMockEndpoint3; dirtyPath1.mAttributeId = Test::MockAttributeId(4); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldSync, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); err = engine->GetReportingEngine().SetDirty(dirtyPath1); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); delegate.mGotReport = false; @@ -2865,8 +2865,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui dirtyPath1.mEndpointId = Test::kMockEndpoint3; dirtyPath1.mAttributeId = Test::MockAttributeId(4); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); - delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldSync, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); err = engine->GetReportingEngine().SetDirty(dirtyPath1); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); delegate.mGotReport = false; @@ -4246,7 +4246,7 @@ void TestReadInteraction::TestSubscriptionReportWithDefunctSession(nlTestSuite * NL_TEST_ASSERT(apSuite, SessionHandle(*readHandler->GetSession()) == ctx.GetSessionAliceToBob()); // Test that we send reports as needed. - readHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + readHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; engine->GetReportingEngine().SetDirty(subscribePath); @@ -4262,7 +4262,7 @@ void TestReadInteraction::TestSubscriptionReportWithDefunctSession(nlTestSuite * // Test that if the session is defunct we don't send reports and clean // up properly. readHandler->GetSession()->MarkAsDefunct(); - readHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::HoldReport, false); + readHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; engine->GetReportingEngine().SetDirty(subscribePath); From 7ef6dad5a93ab184447bbbcb8a3dd21f0e85bdcc Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Thu, 15 Jun 2023 14:36:07 -0400 Subject: [PATCH 2/8] Apply suggestions from code review Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> --- src/app/ReadHandler.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index a94fa87f6f06a3..dfff8364182030 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -356,7 +356,7 @@ class ReadHandler : public Messaging::ExchangeDelegate auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; } /// @brief This allows the read handler to schedule a run as soon as the min interval has elapsed by setting the read handler to - /// a dirty state regardless of the attributes change, thus being reportable as soon as the min interval callback happends. + /// a dirty state regardless of the attributes change, thus being reportable as soon as the min interval callback happens. void ForceDirtyState(); const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } @@ -404,7 +404,7 @@ class ReadHandler : public Messaging::ExchangeDelegate */ void Close(CloseOptions options = CloseOptions::kDropPersistedSubscription); - /// @brief This function is called when the min interval timer has expired, it restarts the timer on a timeout equalt to the + /// @brief This function is called when the min interval timer has expired, it restarts the timer on a timeout equal to the /// difference between the max interval and the min interval. static void MinIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState); static void MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState); @@ -427,7 +427,7 @@ class ReadHandler : public Messaging::ExchangeDelegate void PersistSubscription(); - /// @brief This function modifies at state Flag in the read handler and schedules an engine run if the read handler want from a + /// @brief This function modifies a state Flag in the read handler and schedules an engine run if the read handler want from a /// non reportable state to a reportable state, resulting in the emission of a report. /// @param aFlag Flag to set /// @param aValue Flag new value From be4967823872473eeadf63e389ce1b02923f1aa4 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Thu, 15 Jun 2023 14:43:09 -0400 Subject: [PATCH 3/8] Fixed the syntax for the IsReportableNow description --- src/app/ReadHandler.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index dfff8364182030..e36c23dfaaffd8 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -291,10 +291,10 @@ class ReadHandler : public Messaging::ExchangeDelegate bool IsIdle() const { return mState == HandlerState::Idle; } - /// @brief Returns whether the ReadHandler is in a state where it can send a report immediately. The main conditions for that - /// are that the handler is in a generating reports state and that the, meaning it has been subscribe to or is processing a read - /// request, that the min interval has elapsed and that either max interval has elapsed, forcing an empty report, or that the - /// handler is dirty. This function is used to determine whether a handler should be scheduled for a run. + /// @brief Returns whether the ReadHandler is in a state where it can immediately send a report. The main conditions for this + /// are that the handler is in a report generating state, meaning it is subscribed to or processing a read request, that the min + /// interval has expired and the max interval has expired, forcing an empty report, or that the handler is dirty. This function + /// is used to determine whether a report generation should be scheduled for the handler. /// @return bool IsReportableNow() const { From 42e112f573b7c853296a348c922645d3617811df Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Thu, 15 Jun 2023 15:33:00 -0400 Subject: [PATCH 4/8] Added missing parameter and return description in comments --- src/app/ReadHandler.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index e36c23dfaaffd8..d83191e7ec04b3 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -295,7 +295,7 @@ class ReadHandler : public Messaging::ExchangeDelegate /// are that the handler is in a report generating state, meaning it is subscribed to or processing a read request, that the min /// interval has expired and the max interval has expired, forcing an empty report, or that the handler is dirty. This function /// is used to determine whether a report generation should be scheduled for the handler. - /// @return + /// @return Whether the ReadHandler is in a state where it can immediately send a report. bool IsReportableNow() const { // Important: Anything that changes the state IsReportableNow depends on in @@ -331,7 +331,7 @@ class ReadHandler : public Messaging::ExchangeDelegate /// @brief Update mDirtyGeneration to the latest DirtySetGeneration of the InteractionModelEngine. As this might make the /// readhandler dirty, this function checks if the readhandler is now reportable and schedules a run if it is the case. - /// @param aAttributeChanged + /// @param aAttributeChanged Path to the attribute that was changed. void UpdateDirtyGeneration(const AttributePathParams & aAttributeChanged); bool IsDirty() const { @@ -435,7 +435,7 @@ class ReadHandler : public Messaging::ExchangeDelegate /// @brief This function calls the SetStateFlagAndScheduleReport with the flag value to false, thus possibly emitting a report /// generation. - /// @param aFlag + /// @param aFlag Flag to clear void ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag); // Helpers for continuing the subscription resumption From d9f617f9850773210025878742302ae07cceb6c6 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Fri, 16 Jun 2023 10:49:12 -0400 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/ReadHandler.h | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index d83191e7ec04b3..e795a844ab145c 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -214,13 +214,13 @@ class ReadHandler : public Messaging::ExchangeDelegate { // mHoldMinInterval is used to prevent subscription data delivery while we are // waiting for the min reporting interval to elapse. - HoldMinInterval = (1 << 0), + WaitingUntilMinInterval = (1 << 0), // mHoldMaxInterval is used to prevent subscription empty report delivery while we // are waiting for the max reporting interval to elaps. When mHoldMaxInterval // becomes false, we are allowed to send an empty report to keep the // subscription alive on the client. - HoldMaxInterval = (1 << 1), + WaitingUntilMaxInterval = (1 << 1), // The flag indicating we are in the middle of a series of chunked report messages, this flag will be cleared during // sending last chunked message. @@ -355,8 +355,9 @@ class ReadHandler : public Messaging::ExchangeDelegate auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; } - /// @brief This allows the read handler to schedule a run as soon as the min interval has elapsed by setting the read handler to - /// a dirty state regardless of the attributes change, thus being reportable as soon as the min interval callback happens. + /// @brief Forces the read handler into a dirty state, regardless of what's going on with attributes. + /// This can lead to scheduling of a reporting run immediately, if the min interval has been reached, + /// or after the min interval is reached if it has not yet been reached. void ForceDirtyState(); const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } @@ -427,13 +428,13 @@ class ReadHandler : public Messaging::ExchangeDelegate void PersistSubscription(); - /// @brief This function modifies a state Flag in the read handler and schedules an engine run if the read handler want from a - /// non reportable state to a reportable state, resulting in the emission of a report. + /// @brief Modifies a state flag in the read handler and schedules an engine run if the read handler want from a + /// non-reportable state to a reportable state. /// @param aFlag Flag to set /// @param aValue Flag new value void SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aValue = true); - /// @brief This function calls the SetStateFlagAndScheduleReport with the flag value to false, thus possibly emitting a report + /// @brief This function calls the SetStateFlagAndScheduleReport with the flag value set to false, thus possibly emitting a report /// generation. /// @param aFlag Flag to clear void ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag); From e963a902cdd5ad2d7edd13badb9b688f814139f9 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Fri, 16 Jun 2023 11:12:06 -0400 Subject: [PATCH 6/8] Reverted name changes for flag set and clear, reworded comments as suggested --- src/app/ReadHandler.cpp | 36 ++++++++--------- src/app/ReadHandler.h | 41 +++++++++---------- src/app/reporting/Engine.cpp | 6 +-- src/app/tests/TestReadInteraction.cpp | 57 ++++++++++++++------------- 4 files changed, 70 insertions(+), 70 deletions(-) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index af4c4b8dfed73e..4a92fcfac91f9c 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -60,7 +60,7 @@ ReadHandler::ReadHandler(ManagementCallback & apCallback, Messaging::ExchangeCon mLastWrittenEventsBytes = 0; mTransactionStartGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); mFlags.ClearAll(); - SetStateFlagAndScheduleReport(ReadHandlerFlags::PrimingReports); + SetStateFlag(ReadHandlerFlags::PrimingReports); mSessionHandle.Grab(mExchangeCtx->GetSessionHandle()); } @@ -80,7 +80,7 @@ void ReadHandler::ResumeSubscription(CASESessionManager & caseSessionManager, mSubscriptionId = subscriptionInfo.mSubscriptionId; mMinIntervalFloorSeconds = subscriptionInfo.mMinInterval; mMaxInterval = subscriptionInfo.mMaxInterval; - SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, subscriptionInfo.mFabricFiltered); + SetStateFlag(ReadHandlerFlags::FabricFiltered, subscriptionInfo.mFabricFiltered); // Move dynamically allocated attributes and events from the SubscriptionInfo struct into // the object pool managed by the IM engine @@ -187,7 +187,7 @@ void ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload) else { // Force us to be in a dirty state so we get processed by the reporting - SetStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); + SetStateFlag(ReadHandlerFlags::ForceDirty); } } @@ -214,7 +214,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange { err = SendSubscribeResponse(); - SetStateFlagAndScheduleReport(ReadHandlerFlags::ActiveSubscription); + SetStateFlag(ReadHandlerFlags::ActiveSubscription); auto * appCallback = mManagementCallback.GetAppCallback(); if (appCallback) @@ -297,7 +297,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b { mCurrentReportsBeginGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); } - SetStateFlagAndScheduleReport(ReadHandlerFlags::ChunkedReport, aMoreChunks); + SetStateFlag(ReadHandlerFlags::ChunkedReport, aMoreChunks); bool responseExpected = IsType(InteractionType::Subscribe) || aMoreChunks; mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime); @@ -442,7 +442,7 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa bool isFabricFiltered; ReturnErrorOnFailure(readRequestParser.GetIsFabricFiltered(&isFabricFiltered)); - SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, isFabricFiltered); + SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered); ReturnErrorOnFailure(readRequestParser.ExitContainer()); MoveToState(HandlerState::GeneratingReports); @@ -636,7 +636,7 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse() ReturnErrorOnFailure(UpdateReportTimer()); - ClearStateFlagAndScheduleReport(ReadHandlerFlags::PrimingReports); + ClearStateFlag(ReadHandlerFlags::PrimingReports); return mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeResponse, std::move(packet)); } @@ -718,7 +718,7 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP bool isFabricFiltered; ReturnErrorOnFailure(subscribeRequestParser.GetIsFabricFiltered(&isFabricFiltered)); - SetStateFlagAndScheduleReport(ReadHandlerFlags::FabricFiltered, isFabricFiltered); + SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered); ReturnErrorOnFailure(Crypto::DRBG_get_bytes(reinterpret_cast(&mSubscriptionId), sizeof(mSubscriptionId))); ReturnErrorOnFailure(subscribeRequestParser.ExitContainer()); MoveToState(HandlerState::GeneratingReports); @@ -758,7 +758,7 @@ void ReadHandler::MinIntervalExpiredCallback(System::Layer * apSystemLayer, void VerifyOrReturn(apAppState != nullptr); ReadHandler * readHandler = static_cast(apAppState); ChipLogDetail(DataManagement, "Unblock report hold after min %d seconds", readHandler->mMinIntervalFloorSeconds); - readHandler->ClearStateFlagAndScheduleReport(ReadHandlerFlags::HoldMinInterval); + readHandler->ClearStateFlag(ReadHandlerFlags::WaitingUntilMinInterval); InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds), MaxIntervalExpiredCallback, readHandler); @@ -768,7 +768,7 @@ void ReadHandler::MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void { VerifyOrReturn(apAppState != nullptr); ReadHandler * readHandler = static_cast(apAppState); - readHandler->ClearStateFlagAndScheduleReport(ReadHandlerFlags::HoldMaxInterval); + readHandler->ClearStateFlag(ReadHandlerFlags::WaitingUntilMaxInterval); ChipLogProgress(DataManagement, "Refresh subscribe timer sync after %d seconds", readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds); } @@ -784,8 +784,8 @@ CHIP_ERROR ReadHandler::UpdateReportTimer() { ChipLogProgress(DataManagement, "Refresh Subscribe Sync Timer with min %d seconds and max %d seconds", mMinIntervalFloorSeconds, mMaxInterval); - SetStateFlagAndScheduleReport(ReadHandlerFlags::HoldMinInterval); - SetStateFlagAndScheduleReport(ReadHandlerFlags::HoldMaxInterval); + SetStateFlag(ReadHandlerFlags::WaitingUntilMinInterval); + SetStateFlag(ReadHandlerFlags::WaitingUntilMaxInterval); ReturnErrorOnFailure( InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( System::Clock::Seconds16(mMinIntervalFloorSeconds), MinIntervalExpiredCallback, this)); @@ -800,13 +800,13 @@ void ReadHandler::ResetPathIterator() mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); } -void ReadHandler::UpdateDirtyGeneration(const AttributePathParams & aAttributeChanged) +void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeChanged) { ConcreteAttributePath path; mDirtyGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); - // We won't reset the path iterator for every UpdateDirtyGeneration call to reduce the number of full data reports. + // We won't reset the path iterator for every AttributePathIsDirty call to reduce the number of full data reports. // The iterator will be reset after finishing each report session. // // Here we just reset the iterator to the beginning of the current cluster, if the dirty path affects it. @@ -846,10 +846,10 @@ Transport::SecureSession * ReadHandler::GetSession() const void ReadHandler::ForceDirtyState() { - SetStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); + SetStateFlag(ReadHandlerFlags::ForceDirty); } -void ReadHandler::SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aValue) +void ReadHandler::SetStateFlag(ReadHandlerFlags aFlag, bool aValue) { bool oldReportable = IsReportableNow(); mFlags.Set(aFlag, aValue); @@ -860,9 +860,9 @@ void ReadHandler::SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aVa } } -void ReadHandler::ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag) +void ReadHandler::ClearStateFlag(ReadHandlerFlags aFlag) { - SetStateFlagAndScheduleReport(aFlag, false); + SetStateFlag(aFlag, false); } void ReadHandler::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index e795a844ab145c..3998de49ca739b 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -212,12 +212,12 @@ class ReadHandler : public Messaging::ExchangeDelegate enum class ReadHandlerFlags : uint8_t { - // mHoldMinInterval is used to prevent subscription data delivery while we are + // WaitingUntilMinInterval is used to prevent subscription data delivery while we are // waiting for the min reporting interval to elapse. WaitingUntilMinInterval = (1 << 0), - // mHoldMaxInterval is used to prevent subscription empty report delivery while we - // are waiting for the max reporting interval to elaps. When mHoldMaxInterval + // WaitingUntilMaxInterval is used to prevent subscription empty report delivery while we + // are waiting for the max reporting interval to elaps. When WaitingUntilMaxInterval // becomes false, we are allowed to send an empty report to keep the // subscription alive on the client. WaitingUntilMaxInterval = (1 << 1), @@ -291,18 +291,15 @@ class ReadHandler : public Messaging::ExchangeDelegate bool IsIdle() const { return mState == HandlerState::Idle; } - /// @brief Returns whether the ReadHandler is in a state where it can immediately send a report. The main conditions for this - /// are that the handler is in a report generating state, meaning it is subscribed to or processing a read request, that the min - /// interval has expired and the max interval has expired, forcing an empty report, or that the handler is dirty. This function + /// @brief Returns whether the ReadHandler is in a state where it can immediately send a report. This function /// is used to determine whether a report generation should be scheduled for the handler. - /// @return Whether the ReadHandler is in a state where it can immediately send a report. bool IsReportableNow() const { // Important: Anything that changes the state IsReportableNow depends on in // a way that causes IsReportableNow to become true must call ScheduleRun // on the reporting engine. - return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::HoldMinInterval) && - (IsDirty() || !mFlags.Has(ReadHandlerFlags::HoldMaxInterval)); + return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::WaitingUntilMinInterval) && + (IsDirty() || !mFlags.Has(ReadHandlerFlags::WaitingUntilMaxInterval)); } bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; } bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; } @@ -329,15 +326,15 @@ class ReadHandler : public Messaging::ExchangeDelegate void GetSubscriptionId(SubscriptionId & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; } AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; } - /// @brief Update mDirtyGeneration to the latest DirtySetGeneration of the InteractionModelEngine. As this might make the - /// readhandler dirty, this function checks if the readhandler is now reportable and schedules a run if it is the case. + /// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will scehdule an engine run if + /// the change to the attribute path makes the ReadHandler reportable. /// @param aAttributeChanged Path to the attribute that was changed. - void UpdateDirtyGeneration(const AttributePathParams & aAttributeChanged); + void AttributePathIsDirty(const AttributePathParams & aAttributeChanged); bool IsDirty() const { return (mDirtyGeneration > mPreviousReportsBeginGeneration) || mFlags.Has(ReadHandlerFlags::ForceDirty); } - void ClearForceDirtyFlag() { ClearStateFlagAndScheduleReport(ReadHandlerFlags::ForceDirty); } + void ClearForceDirtyFlag() { ClearStateFlag(ReadHandlerFlags::ForceDirty); } NodeId GetInitiatorNodeId() const { auto session = GetSession(); @@ -356,7 +353,7 @@ class ReadHandler : public Messaging::ExchangeDelegate auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; } /// @brief Forces the read handler into a dirty state, regardless of what's going on with attributes. - /// This can lead to scheduling of a reporting run immediately, if the min interval has been reached, + /// This can lead to scheduling of a reporting run immediately, if the min interval has been reached, /// or after the min interval is reached if it has not yet been reached. void ForceDirtyState(); @@ -428,16 +425,16 @@ class ReadHandler : public Messaging::ExchangeDelegate void PersistSubscription(); - /// @brief Modifies a state flag in the read handler and schedules an engine run if the read handler want from a - /// non-reportable state to a reportable state. + /// @brief Modifies a state flag in the read handler. If the flag went from a + /// non-reportable state to a reportable state, schedules an engine run. /// @param aFlag Flag to set /// @param aValue Flag new value - void SetStateFlagAndScheduleReport(ReadHandlerFlags aFlag, bool aValue = true); + void SetStateFlag(ReadHandlerFlags aFlag, bool aValue = true); - /// @brief This function calls the SetStateFlagAndScheduleReport with the flag value set to false, thus possibly emitting a report + /// @brief This function calls the SetStateFlag with the flag value set to false, thus possibly emitting a report /// generation. /// @param aFlag Flag to clear - void ClearStateFlagAndScheduleReport(ReadHandlerFlags aFlag); + void ClearStateFlag(ReadHandlerFlags aFlag); // Helpers for continuing the subscription resumption static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, @@ -455,7 +452,7 @@ class ReadHandler : public Messaging::ExchangeDelegate // current generation when we started sending the last set reports that we completed. // // This allows us to reset the iterator to the beginning of the current - // cluster instead of the beginning of the whole report in UpdateDirtyGeneration, without + // cluster instead of the beginning of the whole report in AttributePathIsDirty, without // permanently missing dirty any paths. uint64_t mDirtyGeneration = 0; @@ -469,14 +466,14 @@ class ReadHandler : public Messaging::ExchangeDelegate /* * (mDirtyGeneration = b > a, this is a dirty read handler) * +- Start Report -> mCurrentReportsBeginGeneration = c - * | +- UpdateDirtyGeneration (Attribute Y) -> mDirtyGeneration = d + * | +- AttributePathIsDirty (Attribute Y) -> mDirtyGeneration = d * | | +- Last Chunk -> mPreviousReportsBeginGeneration = mCurrentReportsBeginGeneration = c * | | | +- (mDirtyGeneration = d) > (mPreviousReportsBeginGeneration = c), this is a dirty read handler * | | | | Attribute X has a dirty generation less than c, Attribute Y has a dirty generation larger than c * | | | | So Y will be included in the report but X will not be inclued in this report. * -a--b--c------d-----e---f---> Generation * | | - * | +- UpdateDirtyGeneration (Attribute X) (mDirtyGeneration = b) + * | +- AttributePathIsDirty (Attribute X) (mDirtyGeneration = b) * +- mPreviousReportsBeginGeneration * For read handler, if mDirtyGeneration > mPreviousReportsBeginGeneration, then we regard it as a dirty read handler, and it * should generate report on timeout reached. diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 8f449e313f28d0..a9853215332fd5 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -826,8 +826,8 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) bool intersectsInterestPath = false; InteractionModelEngine::GetInstance()->mReadHandlers.ForEachActiveObject( [&aAttributePath, &intersectsInterestPath](ReadHandler * handler) { - // We call UpdateDirtyGeneration for both read interactions and subscribe interactions, since we may send inconsistent - // attribute data between two chunks. UpdateDirtyGeneration will not schedule a new run for read handlers which are + // We call AttributePathIsDirty for both read interactions and subscribe interactions, since we may send inconsistent + // attribute data between two chunks. AttributePathIsDirty will not schedule a new run for read handlers which are // waiting for a response to the last message chunk for read interactions. if (handler->IsGeneratingReports() || handler->IsAwaitingReportResponse()) { @@ -835,7 +835,7 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) { if (object->mValue.Intersects(aAttributePath)) { - handler->UpdateDirtyGeneration(aAttributePath); + handler->AttributePathIsDirty(aAttributePath); intersectsInterestPath = true; break; } diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index f6a689a0c2d0d7..9ec3d59330c216 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1587,7 +1587,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a dirtyPath5.mAttributeId = 4; // Test report with 2 different path - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mGotEventResponse = false; delegate.mNumAttributeResponse = 0; @@ -1604,7 +1604,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 2 different path, and 1 same path - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1620,7 +1620,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 3 different path, and one path is overlapped with another - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1636,7 +1636,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test report with 3 different path, all are not overlapped, one path is not interested for current subscription - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; err = engine->GetReportingEngine().SetDirty(dirtyPath1); @@ -1652,8 +1652,8 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); // Test empty report - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMaxInterval, false); NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled()); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -1740,12 +1740,13 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite GenerateEvents(apSuite, apContext); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); delegate.mGotEventResponse = false; delegate.mGotReport = false; - NL_TEST_ASSERT(apSuite, nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, + nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); nonUrgentDelegate.mGotEventResponse = false; nonUrgentDelegate.mGotReport = false; @@ -1781,13 +1782,14 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite // Since we just sent a report for our urgent subscription, we should have our min interval timer // running again. - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty()); delegate.mGotEventResponse = false; // For our non-urgent subscription, we did not send anything, so we // should not have a min interval timer running there. - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, + !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); // Wait for the min interval timer to fire. @@ -1807,13 +1809,14 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite // min-interval timer should have fired, and our handler should still // not be dirty or even reportable. - NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty()); NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsReportableNow()); // And the non-urgent one should not have changed state either, since // it's waiting for the max-interval. - NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::HoldMinInterval)); + NL_TEST_ASSERT(apSuite, + !nonUrgentDelegate.mpReadHandler->mFlags.Has(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval)); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty()); NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportableNow()); @@ -1914,7 +1917,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap // Set a concrete path dirty { - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -1935,7 +1938,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap // Set a endpoint dirty { - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; delegate.mNumArrayItems = 0; @@ -2027,7 +2030,7 @@ void TestReadInteraction::TestSubscribePartialOverlap(nlTestSuite * apSuite, voi // Set a partial overlapped path dirty { - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2104,7 +2107,7 @@ void TestReadInteraction::TestSubscribeSetDirtyFullyOverlap(nlTestSuite * apSuit // Set a full overlapped path dirty and expect to receive one E2C3A1 { - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2225,8 +2228,8 @@ void TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip(nlTestSuite NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); delegate.mpReadHandler = engine->ActiveHandlerAt(0); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMaxInterval, false); NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled()); ctx.DrainAndServiceIO(); @@ -2401,7 +2404,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu dirtyPath2.mAttributeId = 2; // Test report with 2 different path - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; @@ -2415,8 +2418,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu NL_TEST_ASSERT(apSuite, delegate.mGotReport); NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMaxInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; ctx.ExpireSessionBobToAlice(); @@ -2763,8 +2766,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT dirtyPath1.mEndpointId = Test::kMockEndpoint3; dirtyPath1.mAttributeId = Test::MockAttributeId(4); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMaxInterval, false); err = engine->GetReportingEngine().SetDirty(dirtyPath1); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); delegate.mGotReport = false; @@ -2865,8 +2868,8 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui dirtyPath1.mEndpointId = Test::kMockEndpoint3; dirtyPath1.mAttributeId = Test::MockAttributeId(4); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); - delegate.mpReadHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMaxInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); + delegate.mpReadHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMaxInterval, false); err = engine->GetReportingEngine().SetDirty(dirtyPath1); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); delegate.mGotReport = false; @@ -4246,7 +4249,7 @@ void TestReadInteraction::TestSubscriptionReportWithDefunctSession(nlTestSuite * NL_TEST_ASSERT(apSuite, SessionHandle(*readHandler->GetSession()) == ctx.GetSessionAliceToBob()); // Test that we send reports as needed. - readHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + readHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; engine->GetReportingEngine().SetDirty(subscribePath); @@ -4262,7 +4265,7 @@ void TestReadInteraction::TestSubscriptionReportWithDefunctSession(nlTestSuite * // Test that if the session is defunct we don't send reports and clean // up properly. readHandler->GetSession()->MarkAsDefunct(); - readHandler->SetStateFlagAndScheduleReport(ReadHandler::ReadHandlerFlags::HoldMinInterval, false); + readHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::WaitingUntilMinInterval, false); delegate.mGotReport = false; delegate.mNumAttributeResponse = 0; engine->GetReportingEngine().SetDirty(subscribePath); From b72536de643094e0c17cb67ffcd3359ae7a15396 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Mon, 19 Jun 2023 07:59:29 -0400 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/ReadHandler.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 3998de49ca739b..27113757412af2 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -326,7 +326,7 @@ class ReadHandler : public Messaging::ExchangeDelegate void GetSubscriptionId(SubscriptionId & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; } AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; } - /// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will scehdule an engine run if + /// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will schedule a reporting engine run if /// the change to the attribute path makes the ReadHandler reportable. /// @param aAttributeChanged Path to the attribute that was changed. void AttributePathIsDirty(const AttributePathParams & aAttributeChanged); @@ -425,13 +425,13 @@ class ReadHandler : public Messaging::ExchangeDelegate void PersistSubscription(); - /// @brief Modifies a state flag in the read handler. If the flag went from a - /// non-reportable state to a reportable state, schedules an engine run. + /// @brief Modifies a state flag in the read handler. If the read handler went from a + /// non-reportable state to a reportable state, schedules a reporting engine run. /// @param aFlag Flag to set /// @param aValue Flag new value void SetStateFlag(ReadHandlerFlags aFlag, bool aValue = true); - /// @brief This function calls the SetStateFlag with the flag value set to false, thus possibly emitting a report + /// @brief This function call SetStateFlag with the flag value set to false, thus possibly emitting a report /// generation. /// @param aFlag Flag to clear void ClearStateFlag(ReadHandlerFlags aFlag); From 7a37b57176e6f6f7b046bd149c0ee19b573c1183 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Mon, 19 Jun 2023 11:59:49 +0000 Subject: [PATCH 8/8] Restyled by clang-format --- src/app/ReadHandler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 27113757412af2..f460188935223e 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -326,8 +326,8 @@ class ReadHandler : public Messaging::ExchangeDelegate void GetSubscriptionId(SubscriptionId & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; } AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; } - /// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will schedule a reporting engine run if - /// the change to the attribute path makes the ReadHandler reportable. + /// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will schedule a reporting engine + /// run if the change to the attribute path makes the ReadHandler reportable. /// @param aAttributeChanged Path to the attribute that was changed. void AttributePathIsDirty(const AttributePathParams & aAttributeChanged); bool IsDirty() const