[IM] Record LastReportTick and DirtyTick in ReadHandler

.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) &

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))

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.

src/app/AttributePathParams.h

@@ -86,5 +86,6 @@ struct AttributePathParams
     EndpointId mEndpointId   = kInvalidEndpointId;  // uint16
     ListIndex mListIndex     = kInvalidListIndex;   // uint16
 };
+
 } // namespace app
 } // namespace chip

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<ReadHandler *>(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

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<AttributePathParams> * GetAttributePathList() { return mpAttributePathList; }
     ObjectList<EventPathParams> * 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.