remove a call of emberAfContainsAttribute when using the data model…

… provider interface (project-chip#35746) * Remove a direct ember call from InteractionModelEngine if data model interface is enabled * Enforce identical DM vs ember logic * Fix up unit tests: because dynamic endpoints are reset in an incompatible manner, the data model needs resetting * Include fixes * Restyled by clang-format * Update src/app/codegen-data-model-provider/CodegenDataModelProvider.h * Comment update to kick CI again * make it more readable based on code review feedback * Restyle --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Restyled.io <[email protected]>
pgregorr-amazon · Sep 26, 2024 · bb9363d · bb9363d
1 parent e6b726f
commit bb9363d
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 6 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -1584,6 +1584,26 @@ CHIP_ERROR InteractionModelEngine::PushFrontAttributePathList(SingleLinkedListNo
     return err;
 }
 
+bool InteractionModelEngine::IsExistingAttributePath(const ConcreteAttributePath & path)
+{
+#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+#if CHIP_CONFIG_USE_EMBER_DATA_MODEL
+    // Ensure that Provider interface and ember are IDENTICAL in attribute location (i.e. "check" mode)
+    VerifyOrDie(GetDataModelProvider()
+                    ->GetAttributeInfo(ConcreteAttributePath(path.mEndpointId, path.mClusterId, path.mAttributeId))
+                    .has_value() == emberAfContainsAttribute(path.mEndpointId, path.mClusterId, path.mAttributeId)
+
+    );
+#endif
+
+    return GetDataModelProvider()
+        ->GetAttributeInfo(ConcreteAttributePath(path.mEndpointId, path.mClusterId, path.mAttributeId))
+        .has_value();
+#else
+    return emberAfContainsAttribute(path.mEndpointId, path.mClusterId, path.mAttributeId);
+#endif
+}
+
 void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(SingleLinkedListNode<AttributePathParams> *& aAttributePaths)
 {
     SingleLinkedListNode<AttributePathParams> * prev = nullptr;
@@ -1592,9 +1612,11 @@ void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(SingleLinkedLi
     while (path1 != nullptr)
     {
         bool duplicate = false;
+
         // skip all wildcard paths and invalid concrete attribute
         if (path1->mValue.IsWildcardPath() ||
-            !emberAfContainsAttribute(path1->mValue.mEndpointId, path1->mValue.mClusterId, path1->mValue.mAttributeId))
+            !IsExistingAttributePath(
+                ConcreteAttributePath(path1->mValue.mEndpointId, path1->mValue.mClusterId, path1->mValue.mAttributeId)))
         {
             prev  = path1;
             path1 = path1->mpNext;

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -612,6 +612,11 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     void ShutdownMatchingSubscriptions(const Optional<FabricIndex> & aFabricIndex = NullOptional,
                                        const Optional<NodeId> & aPeerNodeId       = NullOptional);
 
+    /**
+     * Check if the given attribute path is a valid path in the data model provider.
+     */
+    bool IsExistingAttributePath(const ConcreteAttributePath & path);
+
     static void ResumeSubscriptionsTimerCallback(System::Layer * apSystemLayer, void * apAppState);
 
     template <typename T, size_t N>

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
@@ -63,11 +63,26 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
 
         /// Checks if the given command id exists in the given list
         bool Exists(const CommandId * list, CommandId toCheck);
+
+        void Reset() { mCurrentList = mCurrentHint = nullptr; }
     };
 
 public:
+    /// clears out internal caching. Especially useful in unit tests,
+    /// where path caching does not really apply (the same path may result in different outcomes)
+    void Reset()
+    {
+        mAcceptedCommandsIterator.Reset();
+        mGeneratedCommandsIterator.Reset();
+        mPreviouslyFoundCluster = std::nullopt;
+    }
+
     /// Generic model implementations
-    CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; }
+    CHIP_ERROR Shutdown() override
+    {
+        Reset();
+        return CHIP_NO_ERROR;
+    }
 
     DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
                                                 AttributeValueEncoder & encoder) override;

diff --git a/src/controller/tests/TestEventChunking.cpp b/src/controller/tests/TestEventChunking.cpp
@@ -18,19 +18,19 @@
 
 #include <pw_unit_test/framework.h>
 
-#include "app-common/zap-generated/ids/Attributes.h"
-#include "app-common/zap-generated/ids/Clusters.h"
-#include "app/ConcreteAttributePath.h"
-#include "protocols/interaction_model/Constants.h"
 #include <app-common/zap-generated/cluster-objects.h>
+#include <app-common/zap-generated/ids/Attributes.h>
+#include <app-common/zap-generated/ids/Clusters.h>
 #include <app/AppConfig.h>
 #include <app/AttributeAccessInterface.h>
 #include <app/AttributeAccessInterfaceRegistry.h>
 #include <app/BufferedReadCallback.h>
 #include <app/CommandHandlerInterface.h>
+#include <app/ConcreteAttributePath.h>
 #include <app/EventLogging.h>
 #include <app/GlobalAttributes.h>
 #include <app/InteractionModelEngine.h>
+#include <app/codegen-data-model-provider/Instance.h>
 #include <app/data-model/Decode.h>
 #include <app/tests/AppTestContext.h>
 #include <app/util/DataModelHandler.h>
@@ -42,6 +42,7 @@
 #include <lib/support/CHIPCounter.h>
 #include <lib/support/TimeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
+#include <protocols/interaction_model/Constants.h>
 
 using namespace chip;
 using namespace chip::app;
@@ -293,6 +294,7 @@ TEST_F(TestEventChunking, TestEventChunking)
     app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance();
 
     // Initialize the ember side server logic
+    CodegenDataModelProviderInstance()->Shutdown();
     InitDataModelHandler();
 
     // Register our fake dynamic endpoint.
@@ -359,6 +361,7 @@ TEST_F(TestEventChunking, TestMixedEventsAndAttributesChunking)
     app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance();
 
     // Initialize the ember side server logic
+    CodegenDataModelProviderInstance()->Shutdown();
     InitDataModelHandler();
 
     // Register our fake dynamic endpoint.
@@ -435,6 +438,7 @@ TEST_F(TestEventChunking, TestMixedEventsAndLargeAttributesChunking)
     app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance();
 
     // Initialize the ember side server logic
+    CodegenDataModelProviderInstance()->Shutdown();
     InitDataModelHandler();
 
     // Register our fake dynamic endpoint.