From 589164382f0e174186256b1633bfd4646db8b4a3 Mon Sep 17 00:00:00 2001 From: Song GUO Date: Mon, 28 Mar 2022 17:59:35 +0800 Subject: [PATCH] [IM] Record LastReportTick and DirtyTick in ReadHandler (#16060) * [IM] Record LastRecordTimestamp and DirtyTimestamp in ReadHandler * Update naming and comments * Add more tests * Fix * Update tests * Address comments * Resolve compile error * Drive IO for a bit longer time * Address comments * Lift timelimit for darwin * Tick -> Generation --- .github/workflows/darwin.yaml | 6 +- src/app/AttributePathExpandIterator.cpp | 21 ++ src/app/AttributePathExpandIterator.h | 9 + src/app/AttributePathParams.h | 1 + src/app/ReadHandler.cpp | 47 ++- src/app/ReadHandler.h | 75 ++++- src/app/reporting/Engine.cpp | 50 ++- src/app/reporting/Engine.h | 33 +- src/app/tests/TestReadInteraction.cpp | 15 +- src/controller/tests/TestReadChunking.cpp | 382 ++++++++++++++++++++++ 10 files changed, 603 insertions(+), 36 deletions(-) diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml index f47b1587e46b20..38de448ba5146f 100644 --- a/.github/workflows/darwin.yaml +++ b/.github/workflows/darwin.yaml @@ -91,11 +91,11 @@ jobs: run: xcodebuild clean working-directory: src/darwin/Framework - name: Build example chip-tool-darwin - timeout-minutes: 10 + timeout-minutes: 15 run: | scripts/examples/gn_build_example.sh examples/chip-tool-darwin out/debug chip_config_network_layer_ble=false is_asan=true - name: Build example All Clusters Server - timeout-minutes: 10 + timeout-minutes: 15 run: | scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug chip_config_network_layer_ble=false - name: Build example OTA Provider @@ -110,7 +110,7 @@ jobs: run: defaults delete com.apple.dt.xctest.tool continue-on-error: true - name: Run Framework Tests - timeout-minutes: 10 + timeout-minutes: 15 run: | mkdir -p /tmp/darwin/framework-tests ../../../out/debug/chip-all-clusters-app > >(tee /tmp/darwin/framework-tests/all-cluster-app.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-err.log >&2) & diff --git a/src/app/AttributePathExpandIterator.cpp b/src/app/AttributePathExpandIterator.cpp index 3409808ff7c8b5..013cd2fac62652 100644 --- a/src/app/AttributePathExpandIterator.cpp +++ b/src/app/AttributePathExpandIterator.cpp @@ -140,6 +140,27 @@ void AttributePathExpandIterator::PrepareAttributeIndexRange(const AttributePath } } +void AttributePathExpandIterator::ResetCurrentCluster() +{ + // If this is a null iterator, or the attribute id of current cluster info is not a wildcard attribute id, then this function + // will do nothing, since we won't be expanding the wildcard attribute ids under a cluster. + VerifyOrReturn(mpAttributePath != nullptr && mpAttributePath->mValue.HasWildcardAttributeId()); + + // Otherwise, we will reset the index for iterating the attributes, so we report the attributes for this cluster again. This + // will ensure that the client sees a coherent view of the cluster from the reports generated by a single (wildcard) attribute + // path in the request. + // + // Note that when Next() returns, we must be in one of the following states: + // - This is not a wildcard path + // - We just expanded some attribute id field + // - We have exhausted all paths + // Only the second case will happen here since the above check will fail for 1 and 3, so the following Next() call must result + // in a valid path, which is the first attribute id we will emit for the current cluster. + mAttributeIndex = UINT16_MAX; + mGlobalAttributeIndex = UINT8_MAX; + Next(); +} + bool AttributePathExpandIterator::Next() { for (; mpAttributePath != nullptr; (mpAttributePath = mpAttributePath->mpNext, mEndpointIndex = UINT16_MAX)) diff --git a/src/app/AttributePathExpandIterator.h b/src/app/AttributePathExpandIterator.h index 693c20cb024ba9..9b5e170c72552e 100644 --- a/src/app/AttributePathExpandIterator.h +++ b/src/app/AttributePathExpandIterator.h @@ -88,6 +88,15 @@ class AttributePathExpandIterator return Valid(); } + /** + * Reset the iterator to the beginning of current cluster if we are in the middle of expanding a wildcard attribute id for some + * cluster. + * + * When attributes are changed in the middle of expanding a wildcard attribute, we need to reset the iterator, to provide the + * client with a consistent state of the cluster. + */ + void ResetCurrentCluster(); + /** * Returns if the iterator is valid (not exhausted). An iterator is exhausted if and only if: * - Next() is called after iterating last path. diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index 6dd9b135ca9f5d..515c2f2b322cde 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -86,5 +86,6 @@ struct AttributePathParams EndpointId mEndpointId = kInvalidEndpointId; // uint16 ListIndex mListIndex = kInvalidListIndex; // uint16 }; + } // namespace app } // namespace chip diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 7e15f22b708c9d..b5e7ba7d033872 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -132,8 +132,8 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload) } else { - // Mark read handler dirty for read/subscribe priming stage - mDirty = true; + // Force us to be in a dirty state so we get processed by the reporting + mForceDirty = true; } return err; @@ -226,6 +226,10 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b } VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); + if (!IsReporting()) + { + mCurrentReportsBeginGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration(); + } mIsChunkedReport = aMoreChunks; bool noResponseExpected = IsType(InteractionType::Read) && !mIsChunkedReport; if (!noResponseExpected) @@ -252,6 +256,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b } if (!aMoreChunks) { + mPreviousReportsBeginGeneration = mCurrentReportsBeginGeneration; ClearDirty(); InteractionModelEngine::GetInstance()->ReleaseDataVersionFilterList(mpDataVersionFilterList); } @@ -710,7 +715,7 @@ void ReadHandler::OnUnblockHoldReportCallback(System::Layer * apSystemLayer, voi ReadHandler * readHandler = static_cast(apAppState); ChipLogDetail(DataManagement, "Unblock report hold after min %d seconds", readHandler->mMinIntervalFloorSeconds); readHandler->mHoldReport = false; - if (readHandler->mDirty) + if (readHandler->IsDirty()) { InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); } @@ -744,5 +749,41 @@ CHIP_ERROR ReadHandler::RefreshSubscribeSyncTimer() return CHIP_NO_ERROR; } + +void ReadHandler::ResetPathIterator() +{ + mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList); + mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); +} + +void ReadHandler::SetDirty(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. + // 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. + // This will ensure the reports are consistent within a single cluster generated from a single path in the request. + + // TODO (#16699): Currently we can only gurentee the reports generated from a single path in the request are consistent. The + // data might be inconsistent if the user send a request with two paths from the same cluster. We need to clearify the behavior + // or make it consistent. + if (mAttributePathExpandIterator.Get(path) && + (aAttributeChanged.HasWildcardEndpointId() || aAttributeChanged.mEndpointId == path.mEndpointId) && + (aAttributeChanged.HasWildcardClusterId() || aAttributeChanged.mClusterId == path.mClusterId)) + { + ChipLogDetail(DataManagement, + "The dirty path intersects the cluster we are currently reporting; reset the iterator to the beginning of " + "that cluster"); + // If we're currently in the middle of generating reports for a given cluster and that in turn is marked dirty, let's reset + // our iterator to point back to the beginning of that cluster. This ensures that the receiver will get a coherent view of + // the state of the cluster as present on the server + mAttributePathExpandIterator.ResetCurrentCluster(); + mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); + } +} } // namespace app } // namespace chip diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 8259c455a5a9f5..78062a16f84777 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -131,10 +131,13 @@ class ReadHandler : public Messaging::ExchangeDelegate */ bool IsFromSubscriber(Messaging::ExchangeContext & apExchangeContext) const; - bool IsReportable() const { return mState == HandlerState::GeneratingReports && !mHoldReport && (mDirty || !mHoldSync); } + bool IsReportable() const { return mState == HandlerState::GeneratingReports && !mHoldReport && (IsDirty() || !mHoldSync); } bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; } bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; } + // Resets the path iterator to the beginning of the whole report for generating a series of new reports. + void ResetPathIterator(); + CHIP_ERROR ProcessDataVersionFilterList(DataVersionFilterIBs::Parser & aDataVersionFilterListParser); ObjectList * GetAttributePathList() { return mpAttributePathList; } ObjectList * GetEventPathList() { return mpEventPathList; } @@ -149,22 +152,23 @@ class ReadHandler : public Messaging::ExchangeDelegate bool IsType(InteractionType type) const { return (mInteractionType == type); } bool IsChunkedReport() const { return mIsChunkedReport; } + // Is reporting indicates whether we are in the middle of a series chunks. As we will set mIsChunkedReport on the first chunk + // and clear that flag on the last chunk, we can use mIsChunkedReport to indicate this state. + bool IsReporting() const { return mIsChunkedReport; } bool IsPriming() const { return mIsPrimingReports; } bool IsActiveSubscription() const { return mActiveSubscription; } bool IsFabricFiltered() const { return mIsFabricFiltered; } CHIP_ERROR OnSubscribeRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload); void GetSubscriptionId(uint64_t & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; } AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; } - void SetDirty() - { - mDirty = true; - // If the contents of the global dirty set have changed, we need to reset the iterator since the paths - // we've sent up till now are no longer valid and need to be invalidated. - mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList); - mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); - } - void ClearDirty() { mDirty = false; } - bool IsDirty() const { return mDirty; } + + /** + * Notify the read handler that a set of attribute paths has been marked dirty. + */ + void SetDirty(const AttributePathParams & aAttributeChanged); + bool IsDirty() const { return (mDirtyGeneration > mPreviousReportsBeginGeneration) || mForceDirty; } + void ClearDirty() { mForceDirty = false; } + NodeId GetInitiatorNodeId() const { return mInitiatorNodeId; } FabricIndex GetAccessingFabricIndex() const { return mSubjectDescriptor.fabricIndex; } @@ -173,7 +177,7 @@ class ReadHandler : public Messaging::ExchangeDelegate void UnblockUrgentEventDelivery() { mHoldReport = false; - mDirty = true; + mForceDirty = true; } const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } @@ -271,7 +275,6 @@ class ReadHandler : public Messaging::ExchangeDelegate // report immediately due to an urgent event being queued, // UnblockUrgentEventDelivery can be used to force mHoldReport to false. bool mHoldReport = false; - bool mDirty = false; bool mActiveSubscription = false; // 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. @@ -283,7 +286,51 @@ class ReadHandler : public Messaging::ExchangeDelegate // are waiting for the max reporting interval to elaps. When mHoldSync // becomes false, we are allowed to send an empty report to keep the // subscription alive on the client. - bool mHoldSync = false; + bool mHoldSync = false; + + // The current generation of the reporting engine dirty set the last time we were notified that a path we're interested in was + // marked dirty. + // + // This allows us to detemine whether any paths we care about might have + // been marked dirty after we had already sent reports for them, which would + // mean we should report those paths again, by comparing this generation to the + // 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 + // permanently missing dirty any paths. + uint64_t mDirtyGeneration = 0; + // 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. + // + // 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. + bool mForceDirty = false; + // For subscriptions, we record the timestamp when we started to generate the last report. + // The mCurrentReportsBeginGeneration records the timestamp for the current report, which won;t be used for checking if this + // ReadHandler is dirty. + // mPreviousReportsBeginGeneration will be set to mCurrentReportsBeginGeneration after we sent the last chunk of the current + // report. + uint64_t mPreviousReportsBeginGeneration = 0; + uint64_t mCurrentReportsBeginGeneration = 0; + /* + * (mDirtyGeneration = b > a, this is a dirty read handler) + * +- Start Report -> mCurrentReportsBeginGeneration = c + * | +- SetDirty (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) + * +- mPreviousReportsBeginGeneration + * For read handler, if mDirtyGeneration > mPreviousReportsBeginGeneration, then we regard it as a dirty read handler, and it + * should generate report on timeout reached. + */ + uint32_t mLastWrittenEventsBytes = 0; SubjectDescriptor mSubjectDescriptor; // The detailed encoding state for a single attribute, used by list chunking feature. diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 6dff530476d39e..3b681230c47930 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -111,6 +111,20 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu // vs write paths. ConcreteAttributePath readPath; + ChipLogDetail(DataManagement, + "Building Reports for ReadHandler with LastReportGeneration = %" PRIu64 " DirtyGeneration = %" PRIu64, + apReadHandler->mPreviousReportsBeginGeneration, apReadHandler->mDirtyGeneration); + + // This ReadHandler is not generating reports, so we reset the iterator for a clean start. + if (!apReadHandler->IsReporting()) + { + apReadHandler->ResetPathIterator(); + } + +#if CONFIG_IM_BUILD_FOR_UNIT_TEST + uint32_t attributesRead = 0; +#endif + // For each path included in the interested path of the read handler... for (; apReadHandler->GetAttributePathExpandIterator()->Get(readPath); apReadHandler->GetAttributePathExpandIterator()->Next()) @@ -122,8 +136,13 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu mGlobalDirtySet.ForEachActiveObject([&](auto * dirtyPath) { if (dirtyPath->IsAttributePathSupersetOf(readPath)) { - concretePathDirty = true; - return Loop::Break; + // We don't need to worry about paths that were already marked dirty before the last time this read handler + // started a report that it completed: those paths already got reported. + if (dirtyPath->mGeneration > apReadHandler->mPreviousReportsBeginGeneration) + { + concretePathDirty = true; + return Loop::Break; + } } return Loop::Continue; }); @@ -142,7 +161,16 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu } } - // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths. +#if CONFIG_IM_BUILD_FOR_UNIT_TEST + attributesRead++; + if (attributesRead > mMaxAttributesPerChunk) + { + ExitNow(err = CHIP_ERROR_BUFFER_TOO_SMALL); + } +#endif + + // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty + // paths. TLV::TLVWriter attributeBackup; attributeReportIBs.Checkpoint(attributeBackup); ConcreteReadAttributePath pathForRetrieval(readPath); @@ -580,6 +608,8 @@ void Engine::Run() if (allReadClean) { + ChipLogDetail(DataManagement, "All ReadHandler-s are clean, clear GlobalDirtySet"); + mGlobalDirtySet.ReleaseAll(); } } @@ -589,10 +619,12 @@ bool Engine::MergeOverlappedAttributePath(AttributePathParams & aAttributePath) return Loop::Break == mGlobalDirtySet.ForEachActiveObject([&](auto * path) { if (path->IsAttributePathSupersetOf(aAttributePath)) { + path->mGeneration = GetDirtySetGeneration(); return Loop::Break; } if (aAttributePath.IsAttributePathSupersetOf(*path)) { + path->mGeneration = GetDirtySetGeneration(); path->mListIndex = aAttributePath.mListIndex; path->mAttributeId = aAttributePath.mAttributeId; return Loop::Break; @@ -603,6 +635,8 @@ bool Engine::MergeOverlappedAttributePath(AttributePathParams & aAttributePath) CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) { + BumpDirtySetGeneration(); + InteractionModelEngine::GetInstance()->mReadHandlers.ForEachActiveObject([&aAttributePath](ReadHandler * handler) { // We call SetDirty for both read interactions and subscribe interactions, since we may sent inconsistent attribute data // between two chunks. SetDirty will be ignored automatically by read handlers which is waiting for response to last message @@ -614,7 +648,7 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) if (aAttributePath.IsAttributePathSupersetOf(object->mValue) || object->mValue.IsAttributePathSupersetOf(aAttributePath)) { - handler->SetDirty(); + handler->SetDirty(aAttributePath); break; } } @@ -632,7 +666,8 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath) ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!"); return CHIP_ERROR_NO_MEMORY; } - *object = aAttributePath; + *object = aAttributePath; + object->mGeneration = GetDirtySetGeneration(); } // Schedule work to run asynchronously on the CHIP thread. The scheduled @@ -662,7 +697,8 @@ void Engine::UpdateReadHandlerDirty(ReadHandler & aReadHandler) for (auto object = aReadHandler.GetAttributePathList(); object != nullptr; object = object->mpNext) { mGlobalDirtySet.ForEachActiveObject([&](auto * path) { - if (path->IsAttributePathSupersetOf(object->mValue) || object->mValue.IsAttributePathSupersetOf(*path)) + if ((path->IsAttributePathSupersetOf(object->mValue) || object->mValue.IsAttributePathSupersetOf(*path)) && + path->mGeneration > aReadHandler.mPreviousReportsBeginGeneration) { intersected = true; return Loop::Break; @@ -676,8 +712,8 @@ void Engine::UpdateReadHandlerDirty(ReadHandler & aReadHandler) } if (!intersected) { - ChipLogDetail(InteractionModel, "clear read handler dirty in UpdateReadHandlerDirty!"); aReadHandler.ClearDirty(); + ChipLogDetail(InteractionModel, "clear read handler dirty in UpdateReadHandlerDirty!"); } } diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index 3a26abba3ef040..d0fb3d57adda76 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -65,6 +65,8 @@ class Engine #if CONFIG_IM_BUILD_FOR_UNIT_TEST void SetWriterReserved(uint32_t aReservedSize) { mReservedSize = aReservedSize; } + + void SetMaxAttributesPerChunk(uint32_t aMaxAttributesPerChunk) { mMaxAttributesPerChunk = aMaxAttributesPerChunk; } #endif /** @@ -120,10 +122,20 @@ class Engine uint32_t GetNumReportsInFlight() const { return mNumReportsInFlight; } + uint64_t GetDirtySetGeneration() const { return mDirtyGeneration; } + void ScheduleUrgentEventDeliverySync(); private: friend class TestReportingEngine; + + struct AttributePathParamsWithGeneration : public AttributePathParams + { + AttributePathParamsWithGeneration() {} + AttributePathParamsWithGeneration(const AttributePathParams aPath) : AttributePathParams(aPath) {} + uint64_t mGeneration = 0; + }; + /** * Build Single Report Data including attribute changes and event data stream, and send out * @@ -175,6 +187,8 @@ class Engine */ bool MergeOverlappedAttributePath(AttributePathParams & aAttributePath); + inline void BumpDirtySetGeneration() { mDirtyGeneration++; } + /** * Boolean to indicate if ScheduleRun is pending. This flag is used to prevent calling ScheduleRun multiple times * within the same execution context to avoid applying too much pressure on platforms that use small, fixed size event queues. @@ -203,10 +217,25 @@ class Engine * mGlobalDirtySet is used to track the set of attribute/event paths marked dirty for reporting purposes. * */ - ObjectPool mGlobalDirtySet; + ObjectPool mGlobalDirtySet; + + /** + * A generation counter for the dirty attrbute set. + * ReadHandlers can save the generation value when generating reports. + * + * Then we can tell whether they might have missed reporting an attribute by + * comparing its generation counter to the saved one. + * + * mDirtySetGeneration will increase by one when SetDirty is called. + * + * Count it from 1, so 0 can be used in ReadHandler to indicate "the read handler has never + * completed a report". + */ + uint64_t mDirtyGeneration = 1; #if CONFIG_IM_BUILD_FOR_UNIT_TEST - uint32_t mReservedSize = 0; + uint32_t mReservedSize = 0; + uint32_t mMaxAttributesPerChunk = UINT32_MAX; #endif }; diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index e9ed676ec99b69..7cf1032ae3d175 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1319,10 +1319,11 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void ctx.DrainAndServiceIO(); - // We should receive another 2 * (6 + 1) = 14 attribute reports since the underlying path iterator should be reset. + // We should receive another (6 + 1) = 7 attribute reports since the underlying path iterator should be reset to the + // beginning of the cluster it is currently iterating. ChipLogError(DataManagement, "OLD: %d\n", currentAttributeResponsesWhenSetDirty); ChipLogError(DataManagement, "NEW: %d\n", delegate.mNumAttributeResponse); - NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + 14); + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + 7); NL_TEST_ASSERT(apSuite, delegate.mGotReport); NL_TEST_ASSERT(apSuite, !delegate.mReadError); // By now we should have closed all exchanges and sent all pending acks, so @@ -1523,7 +1524,7 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a GenerateEvents(apSuite, apContext); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mHoldReport == false); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mDirty == true); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = kTestClusterId; dirtyPath1.mEndpointId = kTestEndpointId; @@ -1702,7 +1703,7 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite GenerateEvents(apSuite, apContext); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mHoldReport == false); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mDirty == true); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty() == true); delegate.mGotEventResponse = false; delegate.mGotReport = false; ctx.DrainAndServiceIO(); @@ -2083,7 +2084,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu GenerateEvents(apSuite, apContext); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mHoldReport == false); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mDirty == true); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = kTestClusterId; dirtyPath1.mEndpointId = kTestEndpointId; @@ -2402,7 +2403,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT GenerateEvents(apSuite, apContext); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mHoldReport == false); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mDirty == true); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = Test::MockClusterId(2); dirtyPath1.mEndpointId = Test::kMockEndpoint3; @@ -2499,7 +2500,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui GenerateEvents(apSuite, apContext); NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mHoldReport == false); - NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->mDirty == true); + NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty()); chip::app::AttributePathParams dirtyPath1; dirtyPath1.mClusterId = Test::MockClusterId(2); dirtyPath1.mEndpointId = Test::kMockEndpoint3; diff --git a/src/controller/tests/TestReadChunking.cpp b/src/controller/tests/TestReadChunking.cpp index a3020ff5e4872e..32ece2aceadf09 100644 --- a/src/controller/tests/TestReadChunking.cpp +++ b/src/controller/tests/TestReadChunking.cpp @@ -31,13 +31,16 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include +#include using TestContext = chip::Test::AppContext; using namespace chip; @@ -47,6 +50,7 @@ namespace { uint32_t gIterationCount = 0; nlTestSuite * gSuite = nullptr; +TestContext * gCtx = nullptr; // // The generated endpoint_config for the controller app has Endpoint 1 @@ -58,6 +62,7 @@ constexpr EndpointId kTestEndpointId = 2; constexpr EndpointId kTestEndpointId3 = 3; // Another endpoint, for adding / enabling during running. constexpr EndpointId kTestEndpointId4 = 4; +constexpr EndpointId kTestEndpointId5 = 5; constexpr AttributeId kTestListAttribute = 6; constexpr AttributeId kTestBadAttribute = 7; // Reading this attribute will return CHIP_NO_MEMORY but nothing is actually encoded. @@ -69,6 +74,7 @@ class TestCommandInteraction static void TestListChunking(nlTestSuite * apSuite, void * apContext); static void TestBadChunking(nlTestSuite * apSuite, void * apContext); static void TestDynamicEndpoint(nlTestSuite * apSuite, void * apContext); + static void TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void * apContext); private: }; @@ -101,6 +107,16 @@ DECLARE_DYNAMIC_CLUSTER(TestCluster::Id, testClusterAttrsOnEndpoint4, nullptr, n DECLARE_DYNAMIC_ENDPOINT(testEndpoint4, testEndpoint4Clusters); +// Unlike endpoint 1, we can modify the values for values in endpoint 5 +DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(testClusterAttrsOnEndpoint5) +DECLARE_DYNAMIC_ATTRIBUTE(0x00000001, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE(0x00000002, INT8U, 1, 0), + DECLARE_DYNAMIC_ATTRIBUTE(0x00000003, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); + +DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpoint5Clusters) +DECLARE_DYNAMIC_CLUSTER(TestCluster::Id, testClusterAttrsOnEndpoint5, nullptr, nullptr), DECLARE_DYNAMIC_CLUSTER_LIST_END; + +DECLARE_DYNAMIC_ENDPOINT(testEndpoint5, testEndpoint5Clusters); + //clang-format on uint8_t sAnStringThatCanNeverFitIntoTheMTU[4096] = { 0 }; @@ -184,6 +200,45 @@ void TestReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aP void TestReadCallback::OnDone() {} +class TestMutableAttrAccess +{ +public: + CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder); + + void SetDirty(AttributeId attr) + { + app::AttributePathParams path; + path.mEndpointId = kTestEndpointId5; + path.mClusterId = TestCluster::Id; + path.mAttributeId = attr; + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetDirty(path); + } + + // These setters + void SetVal(uint8_t attribute, uint8_t newVal) + { + uint8_t index = static_cast(attribute - 1); + if (index < ArraySize(val) && val[index] != newVal) + { + val[index] = newVal; + SetDirty(attribute); + } + } + + void Reset() { val[0] = val[1] = val[2] = 0; } + + uint8_t val[3] = { 0, 0, 0 }; +}; + +CHIP_ERROR TestMutableAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) +{ + uint8_t index = static_cast(aPath.mAttributeId - 1); + VerifyOrReturnError(aPath.mEndpointId == kTestEndpointId5 && index < ArraySize(val), CHIP_ERROR_NOT_FOUND); + return aEncoder.Encode(val[index]); +} + +TestMutableAttrAccess gMutableAttrAccess; + class TestAttrAccess : public app::AttributeAccessInterface { public: @@ -201,6 +256,12 @@ TestAttrAccess gAttrAccess; CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) { + CHIP_ERROR err = gMutableAttrAccess.Read(aPath, aEncoder); + if (err != CHIP_ERROR_NOT_FOUND) + { + return err; + } + switch (aPath.mAttributeId) { case kTestListAttribute: @@ -227,6 +288,58 @@ CHIP_ERROR TestAttrAccess::Write(const app::ConcreteDataAttributePath & aPath, a return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; } +class TestMutableReadCallback : public app::ReadClient::Callback +{ +public: + TestMutableReadCallback() : mBufferedCallback(*this) {} + void OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, + const app::StatusIB & aStatus) override; + + void OnDone() override {} + + void OnReportBegin() override { mAttributeCount = 0; } + + void OnReportEnd() override { mOnReportEnd = true; } + + void OnSubscriptionEstablished(uint64_t aSubscriptionId) override { mOnSubscriptionEstablished = true; } + + uint32_t mAttributeCount = 0; + // We record every dataversion field from every attribute IB. + std::map, DataVersion> mDataVersions; + std::map, uint8_t> mValues; + std::map, std::function> mActionOn; + bool mOnReportEnd = false; + bool mOnSubscriptionEstablished = false; + app::BufferedReadCallback mBufferedCallback; +}; + +void TestMutableReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, + const app::StatusIB & aStatus) +{ + VerifyOrReturn(apData != nullptr); + NL_TEST_ASSERT(gSuite, aPath.mClusterId == TestCluster::Id); + + mAttributeCount++; + if (aPath.mAttributeId <= 5) + { + uint8_t v; + NL_TEST_ASSERT(gSuite, app::DataModel::Decode(*apData, v) == CHIP_NO_ERROR); + mValues[std::make_pair(aPath.mEndpointId, aPath.mAttributeId)] = v; + + auto action = mActionOn.find(std::make_pair(aPath.mEndpointId, aPath.mAttributeId)); + if (action != mActionOn.end() && action->second) + { + action->second(); + } + } + + if (aPath.mDataVersion.HasValue()) + { + mDataVersions[std::make_pair(aPath.mEndpointId, aPath.mAttributeId)] = aPath.mDataVersion.Value(); + } + // Ignore all other attributes, we don't care above the global attributes. +} + /* * This validates all the various corner cases encountered during chunking by * artificially reducing the size of a packet buffer used to encode attribute data @@ -509,6 +622,274 @@ void TestCommandInteraction::TestDynamicEndpoint(nlTestSuite * apSuite, void * a emberAfClearDynamicEndpoint(0); } +/* + * The tests below are for testing deatiled bwhavior when the attributes are modified between two chunks. In this test, we only care + * above whether we will receive correct attribute values in reasonable messages with reduced reporting traffic. + */ + +namespace TestSetDirtyBetweenChunksUtil { + +using AttributeIdWithEndpointId = std::pair; + +template +constexpr AttributeIdWithEndpointId AttrOnEp1 = AttributeIdWithEndpointId(kTestEndpointId, id); + +template +constexpr AttributeIdWithEndpointId AttrOnEp5 = AttributeIdWithEndpointId(kTestEndpointId5, id); + +auto WriteAttrOp(AttributeIdWithEndpointId attr, uint8_t val) +{ + return [=]() { gMutableAttrAccess.SetVal(static_cast(attr.second), val); }; +} + +auto TouchAttrOp(AttributeIdWithEndpointId attr) +{ + return [=]() { + app::AttributePathParams path; + path.mEndpointId = attr.first; + path.mClusterId = TestCluster::Id; + path.mAttributeId = attr.second; + gIterationCount++; + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetDirty(path); + }; +} + +enum AttrIds +{ + Attr1 = 1, + Attr2 = 2, + Attr3 = 3, +}; + +using AttributeWithValue = std::pair; +using AttributesList = std::vector; + +struct Instruction +{ + // The maximum number of attributes should be iterated in a single report chunk. + uint32_t chunksize; + // A list of functions that will be executed before driving the main loop. + std::vector> preworks; + // A list of pair for attributes and their expected values in the report. + std::vector expectedValues; + // A list of list of various attributes which should have the same data version in the report. + std::vector attributesWithSameDataVersion; +}; + +void DriveIOUntilSubscriptionEstablished(TestMutableReadCallback * callback) +{ + callback->mOnReportEnd = false; + gCtx->GetIOContext().DriveIOUntil(System::Clock::Seconds16(5), [&]() { return callback->mOnSubscriptionEstablished; }); + NL_TEST_ASSERT(gSuite, callback->mOnReportEnd); + NL_TEST_ASSERT(gSuite, callback->mOnSubscriptionEstablished); + callback->mActionOn.clear(); +} + +void DriveIOUntilEndOfReport(TestMutableReadCallback * callback) +{ + callback->mOnReportEnd = false; + gCtx->GetIOContext().DriveIOUntil(System::Clock::Seconds16(5), [&]() { return callback->mOnReportEnd; }); + NL_TEST_ASSERT(gSuite, callback->mOnReportEnd); + callback->mActionOn.clear(); +} + +void CheckValues(TestMutableReadCallback * callback, std::vector expectedValues = {}) +{ + for (const auto & vals : expectedValues) + { + NL_TEST_ASSERT(gSuite, callback->mValues[vals.first] == vals.second); + } +} + +void ExpectSameDataVersions(TestMutableReadCallback * callback, AttributesList attrList) +{ + if (attrList.size() == 0) + { + return; + } + DataVersion expectedVersion = callback->mDataVersions[attrList[0]]; + for (const auto & attr : attrList) + { + NL_TEST_ASSERT(gSuite, callback->mDataVersions[attr] == expectedVersion); + } +} + +void DoTest(TestMutableReadCallback * callback, Instruction instruction) +{ + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetMaxAttributesPerChunk(instruction.chunksize); + + for (const auto & act : instruction.preworks) + { + act(); + } + + DriveIOUntilEndOfReport(callback); + + CheckValues(callback, instruction.expectedValues); + + for (const auto & attrList : instruction.attributesWithSameDataVersion) + { + ExpectSameDataVersions(callback, attrList); + } +} + +}; // namespace TestSetDirtyBetweenChunksUtil + +void TestCommandInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void * apContext) +{ + using namespace TestSetDirtyBetweenChunksUtil; + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); + + gCtx = &ctx; + gSuite = apSuite; + + // Initialize the ember side server logic + InitDataModelHandler(&ctx.GetExchangeManager()); + + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(0); + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetMaxAttributesPerChunk(2); + + DataVersion dataVersionStorage1[ArraySize(testEndpointClusters)]; + DataVersion dataVersionStorage5[ArraySize(testEndpoint5Clusters)]; + + gMutableAttrAccess.Reset(); + + // Register our fake dynamic endpoint. + emberAfSetDynamicEndpoint(0, kTestEndpointId, &testEndpoint, 0, 0, Span(dataVersionStorage1)); + emberAfSetDynamicEndpoint(1, kTestEndpointId5, &testEndpoint5, 0, 0, Span(dataVersionStorage5)); + + { + app::AttributePathParams attributePath; + app::ReadPrepareParams readParams(sessionHandle); + + readParams.mpAttributePathParamsList = &attributePath; + readParams.mAttributePathParamsListSize = 1; + readParams.mMinIntervalFloorSeconds = 0; + readParams.mMaxIntervalCeilingSeconds = 2; + + // TEST 1 -- Read using wildcard paths + ChipLogProgress(DataManagement, "Test 1: Read using wildcard paths."); + { + TestMutableReadCallback readCallback; + + gIterationCount = 1; + + app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback, + app::ReadClient::InteractionType::Subscribe); + + NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR); + + // CASE 1 -- Touch an attribute during priming report, then verify it is included in first report after priming report. + { + // When the report engine starts to report attributes in endpoint 5, mark cluster 1 as dirty. + // The report engine should NOT include it in initial report to reduce traffic. + // We are expected to miss attributes on kTestEndpointId during initial reports. + ChipLogProgress(DataManagement, "Case 1-1: Set dirty during priming report."); + readCallback.mActionOn[AttrOnEp5] = TouchAttrOp(AttrOnEp1); + DriveIOUntilSubscriptionEstablished(&readCallback); + CheckValues(&readCallback, { { AttrOnEp1, 1 } }); + + ChipLogProgress(DataManagement, "Case 1-2: Check for attributes missed last report."); + DoTest(&readCallback, Instruction{ .chunksize = 2, .expectedValues = { { AttrOnEp1, 2 } } }); + } + + // CASE 2 -- Set dirty during chunked report, the attribute is already dirty. + { + ChipLogProgress(DataManagement, "Case 2: Set dirty during chunked report by wildcard path."); + readCallback.mActionOn[AttrOnEp5] = WriteAttrOp(AttrOnEp5, 3); + DoTest( + &readCallback, + Instruction{ .chunksize = 2, + .preworks = { WriteAttrOp(AttrOnEp5, 2), WriteAttrOp(AttrOnEp5, 2), + WriteAttrOp(AttrOnEp5, 2) }, + .expectedValues = { { AttrOnEp5, 2 }, { AttrOnEp5, 2 }, { AttrOnEp5, 3 } }, + .attributesWithSameDataVersion = { { AttrOnEp5, AttrOnEp5, AttrOnEp5 } } }); + } + + // CASE 3 -- Set dirty during chunked report, the attribute is not dirty, and it may catch / missed the current report. + { + ChipLogProgress(DataManagement, + "Case 3-1: Set dirty during chunked report by wildcard path -- new dirty attribute."); + readCallback.mActionOn[AttrOnEp5] = WriteAttrOp(AttrOnEp5, 4); + DoTest( + &readCallback, + Instruction{ .chunksize = 1, + .preworks = { WriteAttrOp(AttrOnEp5, 4), WriteAttrOp(AttrOnEp5, 4) }, + .expectedValues = { { AttrOnEp5, 4 }, { AttrOnEp5, 4 }, { AttrOnEp5, 4 } }, + .attributesWithSameDataVersion = { { AttrOnEp5, AttrOnEp5, AttrOnEp5 } } }); + + ChipLogProgress(DataManagement, + "Case 3-2: Set dirty during chunked report by wildcard path -- new dirty attribute."); + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetMaxAttributesPerChunk(1); + readCallback.mActionOn[AttrOnEp5] = WriteAttrOp(AttrOnEp5, 5); + DoTest( + &readCallback, + Instruction{ .chunksize = 1, + .preworks = { WriteAttrOp(AttrOnEp5, 5), WriteAttrOp(AttrOnEp5, 5) }, + .expectedValues = { { AttrOnEp5, 5 }, { AttrOnEp5, 5 }, { AttrOnEp5, 5 } }, + .attributesWithSameDataVersion = { { AttrOnEp5, AttrOnEp5, AttrOnEp5 } } }); + } + } + } + // The read client is destructed, server will shutdown the corresponding subscription later. + + // TEST 2 -- Read using concrete paths. + ChipLogProgress(DataManagement, "Test 2: Read using concrete paths."); + { + app::AttributePathParams attributePath[3]; + app::ReadPrepareParams readParams(sessionHandle); + + attributePath[0] = app::AttributePathParams(kTestEndpointId5, TestCluster::Id, Attr1); + attributePath[1] = app::AttributePathParams(kTestEndpointId5, TestCluster::Id, Attr2); + attributePath[2] = app::AttributePathParams(kTestEndpointId5, TestCluster::Id, Attr3); + + readParams.mpAttributePathParamsList = attributePath; + readParams.mAttributePathParamsListSize = 3; + readParams.mMinIntervalFloorSeconds = 0; + readParams.mMaxIntervalCeilingSeconds = 2; + gMutableAttrAccess.Reset(); + + // CASE 1 -- Touch an attribute during priming report, then verify it is included in first report after priming report. + { + TestMutableReadCallback readCallback; + + app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback, + app::ReadClient::InteractionType::Subscribe); + + NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR); + + DriveIOUntilSubscriptionEstablished(&readCallback); + + // Note, although the two attributes comes from the same cluster, they are generated by different interested paths. + // In this case, we won't reset the path iterator. + ChipLogProgress(DataManagement, "Case 1-1: Test set dirty during reports generated by concrete paths."); + readCallback.mActionOn[AttrOnEp5] = WriteAttrOp(AttrOnEp5, 4); + DoTest(&readCallback, + Instruction{ .chunksize = 1, + .preworks = { WriteAttrOp(AttrOnEp5, 3), WriteAttrOp(AttrOnEp5, 3), + WriteAttrOp(AttrOnEp5, 3) }, + .expectedValues = { { AttrOnEp5, 3 }, { AttrOnEp5, 3 }, { AttrOnEp5, 3 } } }); + + // The attribute failed to catch last report will be picked by this report. + ChipLogProgress(DataManagement, "Case 1-2: Check for attributes missed last report."); + DoTest(&readCallback, { .chunksize = 1, .expectedValues = { { AttrOnEp5, 4 } } }); + } + } + + chip::test_utils::SleepMillis(secondsToMilliseconds(3)); + + // Destroying the read client will terminate the subscription transaction. + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + + emberAfClearDynamicEndpoint(1); + emberAfClearDynamicEndpoint(0); + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetMaxAttributesPerChunk(UINT32_MAX); +} + // clang-format off const nlTest sTests[] = { @@ -516,6 +897,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestListChunking", TestCommandInteraction::TestListChunking), NL_TEST_DEF("TestBadChunking", TestCommandInteraction::TestBadChunking), NL_TEST_DEF("TestDynamicEndpoint", TestCommandInteraction::TestDynamicEndpoint), + NL_TEST_DEF("TestSetDirtyBetweenChunks", TestCommandInteraction::TestSetDirtyBetweenChunks), NL_TEST_SENTINEL() };