Implement DataModel based read support in the interaction model engin…

…e. (project-chip#34419) * Implement DataModel based read support in the interacton model engine. This implements reads via ember and via codegen as well as a checked-mode where reads are validated as returning the same status codes and data size (because data content is not available) * Comment update and logic update: we MAY get different sizes if data size changes for numbers * Fix typo for ember implementation while renaming things * Fix override markers * run_tv_casting_test should be executable * Do not report errors on chunking * Typo fix * move chipDie for tests only * move all chipdie to unit test only * fix comment and formatting * Update encoderstate logic a bit - code is cleaner, will have to see if CI tests pass * Restyle * Enable tracing of messages for tv tests, to see what is going on in CI * Restyle * Start adding some log line processing for the tv tests, to have propper timeouts and not block on IO buffers * Significant reformat and start refactoring the tv casting test * TV tests pass * Restyle * Fix ruff * Review comment update: set state in all cases * Added a TODO regarding the awkward "callback on success only" * Merge fix * Update src/app/reporting/Read-Checked.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Review updates * Fix placement of dm state * Restyle * Code review comments: double-check that IM not active when setting model, explain why we have the ifdef * Code review: comment why we did not re-use code * Code review feedback: warn if running in checked mode * Restyle * Avoid loop of err/out empty output * Support a log directory argument for the casting tests, so I can debug their content * Better debuggability and error reporting support for shell - this is to debug cast failures --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
nivi-apple · Jul 24, 2024 · c88d5cf · c88d5cf
1 parent 8ad0bdf
commit c88d5cf
Show file tree

Hide file tree

Showing 28 changed files with 953 additions and 64 deletions.
diff --git a/scripts/tests/linux/log_line_processing.py b/scripts/tests/linux/log_line_processing.py
@@ -17,6 +17,7 @@
 import select
 import subprocess
 import threading
+import time
 from typing import List
 
 
@@ -57,23 +58,31 @@ def _io_thread(self):
         reading
         """
         out_wait = select.poll()
-        out_wait.register(self.process.stdout, select.POLLIN)
+        out_wait.register(self.process.stdout, select.POLLIN | select.POLLHUP)
 
         err_wait = select.poll()
-        err_wait.register(self.process.stderr, select.POLLIN)
+        err_wait.register(self.process.stderr, select.POLLIN | select.POLLHUP)
 
         with open(self.output_path, "wt") as f:
+            f.write("PROCESS START: %s\n" % time.ctime())
             while not self.done:
                 changes = out_wait.poll(0.1)
                 if changes:
                     out_line = self.process.stdout.readline()
+                    if not out_line:
+                        # stdout closed (otherwise readline should have at least \n)
+                        continue
                     f.write(out_line)
                     self.output_lines.put(out_line)
 
                 changes = err_wait.poll(0)
                 if changes:
                     err_line = self.process.stderr.readline()
+                    if not err_line:
+                        # stderr closed (otherwise readline should have at least \n)
+                        continue
                     f.write(f"!!STDERR!! : {err_line}")
+            f.write("PROCESS END: %s\n" % time.ctime())
 
     def __enter__(self):
         self.done = False

diff --git a/scripts/tests/run_tv_casting_test.py b/scripts/tests/run_tv_casting_test.py
@@ -295,8 +295,14 @@ def cmd_execute_list(app_path):
     default=False,
     help="Enable the commissioner generated passcode test flow.",
 )
+@click.option(
+    "--log-directory",
+    type=str,
+    default=None,
+    help="Where to place output logs",
+)
 def test_casting_fn(
-    tv_app_rel_path, tv_casting_app_rel_path, commissioner_generated_passcode
+    tv_app_rel_path, tv_casting_app_rel_path, commissioner_generated_passcode, log_directory
 ):
     """Test if the casting experience between the Linux tv-casting-app and the Linux tv-app continues to work.
 
@@ -320,10 +326,16 @@ def test_casting_fn(
 
     # Store the log files to a temporary directory.
     with tempfile.TemporaryDirectory() as temp_dir:
-        linux_tv_app_log_path = os.path.join(temp_dir, LINUX_TV_APP_LOGS)
-        linux_tv_casting_app_log_path = os.path.join(
-            temp_dir, LINUX_TV_CASTING_APP_LOGS
-        )
+        if log_directory:
+            linux_tv_app_log_path = os.path.join(log_directory, LINUX_TV_APP_LOGS)
+            linux_tv_casting_app_log_path = os.path.join(
+                log_directory, LINUX_TV_CASTING_APP_LOGS
+            )
+        else:
+            linux_tv_app_log_path = os.path.join(temp_dir, LINUX_TV_APP_LOGS)
+            linux_tv_casting_app_log_path = os.path.join(
+                temp_dir, LINUX_TV_CASTING_APP_LOGS
+            )
 
         # Get all the test sequences.
         test_sequences = Sequence.get_test_sequences()

diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h
@@ -162,6 +162,7 @@ class AttributeValueEncoder
 private:
     // We made EncodeListItem() private, and ListEncoderHelper will expose it by Encode()
     friend class ListEncodeHelper;
+    friend class TestOnlyAttributeValueEncoderAccessor;
 
     template <typename... Ts>
     CHIP_ERROR EncodeListItem(Ts &&... aArgs)

diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
@@ -58,16 +58,6 @@ declare_args() {
   chip_im_static_global_interaction_model_engine =
       current_os != "linux" && current_os != "mac" && current_os != "ios" &&
       current_os != "android"
-
-  # Data model interface usage:
-  #   - disabled: does not use data model interface at all
-  #   - check:    runs BOTH datamodel and non-data-model (if possible) functionality and compares results
-  #   - enabled:  runs only the data model interface (does not use the legacy code)
-  if (current_os == "linux") {
-    chip_use_data_model_interface = "check"
-  } else {
-    chip_use_data_model_interface = "disabled"
-  }
 }
 
 buildconfig_header("app_buildconfig") {
@@ -219,6 +209,7 @@ static_library("interaction-model") {
     "WriteClient.h",
     "reporting/Engine.cpp",
     "reporting/Engine.h",
+    "reporting/Read.h",
     "reporting/ReportScheduler.h",
     "reporting/ReportSchedulerImpl.cpp",
     "reporting/ReportSchedulerImpl.h",
@@ -260,6 +251,29 @@ static_library("interaction-model") {
 
   public_configs = [ "${chip_root}/src:includes" ]
 
+  if (chip_use_data_model_interface == "disabled") {
+    sources += [
+      "reporting/Read-Ember.cpp",
+      "reporting/Read-Ember.h",
+    ]
+  } else if (chip_use_data_model_interface == "check") {
+    sources += [
+      "reporting/Read-Checked.cpp",
+      "reporting/Read-Checked.h",
+      "reporting/Read-DataModel.cpp",
+      "reporting/Read-DataModel.h",
+      "reporting/Read-Ember.cpp",
+      "reporting/Read-Ember.h",
+    ]
+    public_deps += [ "${chip_root}/src/app/data-model-interface" ]
+  } else {  # enabled
+    sources += [
+      "reporting/Read-DataModel.cpp",
+      "reporting/Read-DataModel.h",
+    ]
+    public_deps += [ "${chip_root}/src/app/data-model-interface" ]
+  }
+
   if (chip_enable_read_client) {
     sources += [ "ReadClient.cpp" ]
   }

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -93,6 +93,15 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM
 
     StatusIB::RegisterErrorFormatter();
 
+#if CHIP_CONFIG_USE_EMBER_DATA_MODEL && CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+    ChipLogError(InteractionModel, "WARNING ┌────────────────────────────────────────────────────");
+    ChipLogError(InteractionModel, "WARNING │ Interaction Model Engine running in 'Checked' mode.");
+    ChipLogError(InteractionModel, "WARNING │ This executes BOTH ember and data-model code paths.");
+    ChipLogError(InteractionModel, "WARNING │ which is inefficient and consumes more flash space.");
+    ChipLogError(InteractionModel, "WARNING │ This should be done for testing only.");
+    ChipLogError(InteractionModel, "WARNING └────────────────────────────────────────────────────");
+#endif
+
     return CHIP_NO_ERROR;
 }
 
@@ -1697,6 +1706,16 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const
     return ServerClusterCommandExists(aCommandPath);
 }
 
+InteractionModel::DataModel * InteractionModelEngine::SetDataModel(InteractionModel::DataModel * model)
+{
+    // Alternting data model should not be done while IM is actively handling requests.
+    VerifyOrDie(mReadHandlers.begin() == mReadHandlers.end());
+
+    InteractionModel::DataModel * oldModel = GetDataModel();
+    mDataModel                             = model;
+    return oldModel;
+}
+
 InteractionModel::DataModel * InteractionModelEngine::GetDataModel() const
 {
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -125,10 +125,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      *  @param[in]    apFabricTable    A pointer to the FabricTable object.
      *  @param[in]    apCASESessionMgr An optional pointer to a CASESessionManager (used for re-subscriptions).
      *
-     *  @retval #CHIP_ERROR_INCORRECT_STATE If the state is not equal to
-     *          kState_NotInitialized.
-     *  @retval #CHIP_NO_ERROR On success.
-     *
      */
     CHIP_ERROR Init(Messaging::ExchangeManager * apExchangeMgr, FabricTable * apFabricTable,
                     reporting::ReportScheduler * reportScheduler, CASESessionManager * apCASESessionMgr = nullptr,
@@ -408,6 +404,13 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 
     InteractionModel::DataModel * GetDataModel() const;
 
+    // MUST NOT be used while the interaction model engine is running as interaction
+    // model functionality (e.g. active reads/writes/subscriptions) rely on data model
+    // state
+    //
+    // Returns the old data model value.
+    InteractionModel::DataModel * SetDataModel(InteractionModel::DataModel * model);
+
 private:
     friend class reporting::Engine;
     friend class TestCommandInteraction;

diff --git a/src/app/common_flags.gni b/src/app/common_flags.gni
@@ -21,4 +21,14 @@ declare_args() {
   # Flag that controls whether the time-to-wait from BUSY responses is
   # communicated to OperationalSessionSetup API consumers.
   chip_enable_busy_handling_for_operational_session_setup = true
+
+  # Data model interface usage:
+  #   - disabled: does not use data model interface at all
+  #   - check:    runs BOTH datamodel and non-data-model (if possible) functionality and compares results
+  #   - enabled:  runs only the data model interface (does not use the legacy code)
+  if (current_os == "linux") {
+    chip_use_data_model_interface = "check"
+  } else {
+    chip_use_data_model_interface = "disabled"
+  }
 }
diff --git a/src/app/data-model-interface/OperationTypes.h b/src/app/data-model-interface/OperationTypes.h
@@ -65,7 +65,6 @@ enum class ReadFlags : uint32_t
 struct ReadAttributeRequest : OperationRequest
 {
     ConcreteAttributePath path;
-    std::optional<DataVersion> dataVersion;
     BitFlags<ReadFlags> readFlags;
 };
 

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -31,6 +31,7 @@
 #include <app/InteractionModelEngine.h>
 #include <app/RequiredPrivilege.h>
 #include <app/reporting/Engine.h>
+#include <app/reporting/Read.h>
 #include <app/util/MatterCallbacks.h>
 #include <app/util/ember-compatibility-functions.h>
 
@@ -79,25 +80,6 @@ bool Engine::IsClusterDataVersionMatch(const SingleLinkedListNode<DataVersionFil
     return existPathMatch && !existVersionMismatch;
 }
 
-CHIP_ERROR
-Engine::RetrieveClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
-                            AttributeReportIBs::Builder & aAttributeReportIBs, const ConcreteReadAttributePath & aPath,
-                            AttributeEncodeState * aEncoderState)
-{
-    ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId,
-                  aPath.mAttributeId);
-
-    DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
-                                                          DataModelCallbacks::OperationOrder::Pre, aPath);
-
-    ReturnErrorOnFailure(ReadSingleClusterData(aSubjectDescriptor, aIsFabricFiltered, aPath, aAttributeReportIBs, aEncoderState));
-
-    DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
-                                                          DataModelCallbacks::OperationOrder::Post, aPath);
-
-    return CHIP_NO_ERROR;
-}
-
 static bool IsOutOfWriterSpaceError(CHIP_ERROR err)
 {
     return err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL;
@@ -200,8 +182,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
             ConcreteReadAttributePath pathForRetrieval(readPath);
             // Load the saved state from previous encoding session for chunking of one single attribute (list chunking).
             AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState();
-            err = RetrieveClusterData(apReadHandler->GetSubjectDescriptor(), apReadHandler->IsFabricFiltered(), attributeReportIBs,
-                                      pathForRetrieval, &encodeState);
+            err = Impl::RetrieveClusterData(mpImEngine->GetDataModel(), apReadHandler->GetSubjectDescriptor(),
+                                            apReadHandler->IsFabricFiltered(), attributeReportIBs, pathForRetrieval, &encodeState);
             if (err != CHIP_NO_ERROR)
             {
                 // If error is not an "out of writer space" error, rollback and encode status.

diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h
@@ -168,9 +168,6 @@ class Engine
                                                        bool * apHasMoreChunks, bool * apHasEncodedData);
     CHIP_ERROR BuildSingleReportDataEventReports(ReportDataMessage::Builder & reportDataBuilder, ReadHandler * apReadHandler,
                                                  bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData);
-    CHIP_ERROR RetrieveClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
-                                   AttributeReportIBs::Builder & aAttributeReportIBs,
-                                   const ConcreteReadAttributePath & aClusterInfo, AttributeEncodeState * apEncoderState);
     CHIP_ERROR CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler);
 
     // If version match, it means don't send, if version mismatch, it means send.