[IM] Notify report engine when adding or enabling endpoints (#15505)

* [IM] Notify report engine when adding or enabling endpoints

* Update to tot

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

src/app/reporting/reporting.h

@@ -61,3 +61,8 @@ void MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::Clu
  * Same but with a nicer attribute path.
  */
 void MatterReportingAttributeChangeCallback(const chip::app::ConcreteAttributePath & aPath);
+
+/*
+ * Same but only with an EndpointId, this is used when adding / enabling an endpoint during runtime.
+ */
+void MatterReportingAttributeChangeCallback(chip::EndpointId endpoint);

src/app/util/attribute-storage.cpp

@@ -910,6 +910,7 @@ bool emberAfEndpointEnableDisable(EndpointId endpoint, bool enable)
         if (enable)
         {
             initializeEndpoint(&(emAfEndpoints[index]));
+            MatterReportingAttributeChangeCallback(endpoint);
         }
         else
         {
@@ -964,10 +965,6 @@ bool emberAfEndpointEnableDisable(EndpointId endpoint, bool enable)
             }
         }
 
-        // TODO: We should notify about the fact that all the attributes for
-        // this endpoint have appeared/disappeared, but the reporting engine has
-        // no way to do that right now.
-
         // TODO: Once endpoints are in parts lists other than that of endpoint
         // 0, something more complicated might need to happen here.
 

src/app/util/ember-compatibility-functions.cpp

@@ -1034,3 +1034,17 @@ void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath)
 {
     return MatterReportingAttributeChangeCallback(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId);
 }
+
+void MatterReportingAttributeChangeCallback(EndpointId endpoint)
+{
+    // Attribute writes have asserted this already, but this assert should catch
+    // applications notifying about changes from their end.
+    assertChipStackLockedByCurrentThread();
+
+    ClusterInfo info;
+    info.mEndpointId = endpoint;
+
+    // We are adding or enabling a whole endpoint, in this case, we do not touch the cluster data version.
+
+    InteractionModelEngine::GetInstance()->GetReportingEngine().SetDirty(info);
+}

src/controller/tests/TestReadChunking.cpp

@@ -32,7 +32,9 @@
 #include <app/util/attribute-storage.h>
 #include <controller/InvokeInteraction.h>
 #include <lib/support/ErrorStr.h>
+#include <lib/support/TimeUtils.h>
 #include <lib/support/UnitTestRegistration.h>
+#include <lib/support/UnitTestUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <messaging/tests/MessagingContext.h>
 #include <nlunit-test.h>
@@ -53,7 +55,9 @@ nlTestSuite * gSuite     = nullptr;
 //
 constexpr EndpointId kTestEndpointId = 2;
 // Another endpoint, with a list attribute only.
-constexpr EndpointId kTestEndpointId3    = 3;
+constexpr EndpointId kTestEndpointId3 = 3;
+// Another endpoint, for adding / enabling during running.
+constexpr EndpointId kTestEndpointId4    = 4;
 constexpr AttributeId kTestListAttribute = 6;
 constexpr AttributeId kTestBadAttribute  = 7; // Reading this attribute will return CHIP_NO_MEMORY but nothing is actually encoded.
 
@@ -64,6 +68,7 @@ class TestCommandInteraction
     static void TestChunking(nlTestSuite * apSuite, void * apContext);
     static void TestListChunking(nlTestSuite * apSuite, void * apContext);
     static void TestBadChunking(nlTestSuite * apSuite, void * apContext);
+    static void TestDynamicEndpoint(nlTestSuite * apSuite, void * apContext);
 
 private:
 };
@@ -88,6 +93,14 @@ DECLARE_DYNAMIC_CLUSTER(TestCluster::Id, testClusterAttrsOnEndpoint3, nullptr, n
 
 DECLARE_DYNAMIC_ENDPOINT(testEndpoint3, testEndpoint3Clusters);
 
+DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(testClusterAttrsOnEndpoint4)
+DECLARE_DYNAMIC_ATTRIBUTE(0x00000001, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
+
+DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpoint4Clusters)
+DECLARE_DYNAMIC_CLUSTER(TestCluster::Id, testClusterAttrsOnEndpoint4, nullptr, nullptr), DECLARE_DYNAMIC_CLUSTER_LIST_END;
+
+DECLARE_DYNAMIC_ENDPOINT(testEndpoint4, testEndpoint4Clusters);
+
 //clang-format on
 
 uint8_t sAnStringThatCanNeverFitIntoTheMTU[4096] = { 0 };
@@ -103,8 +116,11 @@ class TestReadCallback : public app::ReadClient::Callback
 
     void OnReportEnd() override { mOnReportEnd = true; }
 
-    uint32_t mAttributeCount = 0;
-    bool mOnReportEnd        = false;
+    void OnSubscriptionEstablished(uint64_t aSubscriptionId) override { mOnSubscriptionEstablished = true; }
+
+    uint32_t mAttributeCount        = 0;
+    bool mOnReportEnd               = false;
+    bool mOnSubscriptionEstablished = false;
     app::BufferedReadCallback mBufferedCallback;
 };
 
@@ -366,6 +382,8 @@ void TestCommandInteraction::TestBadChunking(nlTestSuite * apSuite, void * apCon
     // Initialize the ember side server logic
     InitDataModelHandler(&ctx.GetExchangeManager());
 
+    app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(0);
+
     // Register our fake dynamic endpoint.
     DataVersion dataVersionStorage[ArraySize(testEndpoint3Clusters)];
     emberAfSetDynamicEndpoint(0, kTestEndpointId3, &testEndpoint3, 0, 0, Span<DataVersion>(dataVersionStorage));
@@ -404,12 +422,100 @@ void TestCommandInteraction::TestBadChunking(nlTestSuite * apSuite, void * apCon
     emberAfClearDynamicEndpoint(0);
 }
 
+/*
+ * This test contains two parts, one is to enable a new endpoint on the fly, another is to disable it and re-enable it.
+ */
+void TestCommandInteraction::TestDynamicEndpoint(nlTestSuite * apSuite, void * apContext)
+{
+    TestContext & ctx                    = *static_cast<TestContext *>(apContext);
+    auto sessionHandle                   = ctx.GetSessionBobToAlice();
+    app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance();
+
+    // Initialize the ember side server logic
+    InitDataModelHandler(&ctx.GetExchangeManager());
+
+    // Register our fake dynamic endpoint.
+    DataVersion dataVersionStorage[ArraySize(testEndpoint4Clusters)];
+
+    app::AttributePathParams attributePath;
+    app::ReadPrepareParams readParams(sessionHandle);
+
+    readParams.mpAttributePathParamsList    = &attributePath;
+    readParams.mAttributePathParamsListSize = 1;
+    readParams.mMaxIntervalCeilingSeconds   = 1;
+
+    TestReadCallback readCallback;
+
+    {
+
+        app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback,
+                                   app::ReadClient::InteractionType::Subscribe);
+
+        NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR);
+
+        ctx.DrainAndServiceIO();
+
+        // We should not receive any reports in initial reports, so check mOnSubscriptionEstablished instead.
+        NL_TEST_ASSERT(apSuite, readCallback.mOnSubscriptionEstablished);
+        readCallback.mAttributeCount = 0;
+
+        // Enable the new endpoint
+        emberAfSetDynamicEndpoint(0, kTestEndpointId4, &testEndpoint4, 0, 0, Span<DataVersion>(dataVersionStorage));
+
+        ctx.DrainAndServiceIO();
+
+        // Ensure we have received the report, we do not care about the initial report here.
+        // ClientGeneratedCommandList / ServerGeneratedCommandList / AttributeList attribute are not included in
+        // testClusterAttrsOnEndpoint4.
+        NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == ArraySize(testClusterAttrsOnEndpoint4) + 3);
+
+        // We have received all report data.
+        NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
+
+        readCallback.mAttributeCount = 0;
+        readCallback.mOnReportEnd    = false;
+
+        // Disable the new endpoint
+        emberAfEndpointEnableDisable(kTestEndpointId4, false);
+
+        ctx.DrainAndServiceIO();
+
+        // We may receive some attribute reports for descriptor cluster, but we do not care about it for now.
+
+        // Enable the new endpoint
+
+        readCallback.mAttributeCount = 0;
+        readCallback.mOnReportEnd    = false;
+
+        emberAfEndpointEnableDisable(kTestEndpointId4, true);
+        ctx.DrainAndServiceIO();
+
+        // Ensure we have received the report, we do not care about the initial report here.
+        // ClientGeneratedCommandList / ServerGeneratedCommandList / AttributeList attribute are not include in
+        // testClusterAttrsOnEndpoint4.
+        NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == ArraySize(testClusterAttrsOnEndpoint4) + 3);
+
+        // We have received all report data.
+        NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
+    }
+
+    chip::test_utils::SleepMillis(secondsToMilliseconds(2));
+
+    // Destroying the read client will terminate the subscription transaction.
+    ctx.DrainAndServiceIO();
+
+    NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
+
+    emberAfClearDynamicEndpoint(0);
+}
+
 // clang-format off
 const nlTest sTests[] =
 {
     NL_TEST_DEF("TestChunking", TestCommandInteraction::TestChunking),
     NL_TEST_DEF("TestListChunking", TestCommandInteraction::TestListChunking),
     NL_TEST_DEF("TestBadChunking", TestCommandInteraction::TestBadChunking),
+    NL_TEST_DEF("TestDynamicEndpoint", TestCommandInteraction::TestDynamicEndpoint),
     NL_TEST_SENTINEL()
 };