Remove Interaction Model Engine dependency in WriteHandler (#33426)

* Remove Interaction Model Engine dependency in WriteHandler * Add test * Add reference * Restyled by whitespace * Restyled by clang-format * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky <[email protected]> --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Sep 23, 2024 · 711324b · 711324b
1 parent 1eb6be3
commit 711324b
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 16 deletions.
diff --git a/src/app/InteractionModelDelegatePointers.cpp b/src/app/InteractionModelDelegatePointers.cpp
@@ -31,6 +31,12 @@ app::TimedHandlerDelegate * GlobalInstanceProvider<app::TimedHandlerDelegate>::I
     return app::InteractionModelEngine::GetInstance();
 }
 
+template <>
+app::WriteHandlerDelegate * GlobalInstanceProvider<app::WriteHandlerDelegate>::InstancePointer()
+{
+    return app::InteractionModelEngine::GetInstance();
+}
+
 } // namespace chip
 
 #endif
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -869,7 +869,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messa
     {
         if (writeHandler.IsFree())
         {
-            VerifyOrReturnError(writeHandler.Init() == CHIP_NO_ERROR, Status::Busy);
+            VerifyOrReturnError(writeHandler.Init(this) == CHIP_NO_ERROR, Status::Busy);
             return writeHandler.OnWriteRequest(apExchangeContext, std::move(aPayload), aIsTimedWrite);
         }
     }

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -91,7 +91,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
                                public ReadHandler::ManagementCallback,
                                public FabricTable::Delegate,
                                public SubscriptionsInfoProvider,
-                               public TimedHandlerDelegate
+                               public TimedHandlerDelegate,
+                               public WriteHandlerDelegate
 {
 public:
     /**
@@ -235,6 +236,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     void OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
                       const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override;
 
+    // WriteHandlerDelegate implementation
+    bool HasConflictWriteRequests(const WriteHandler * apWriteHandler, const ConcreteAttributePath & apath) override;
+
 #if CHIP_CONFIG_ENABLE_READ_CLIENT
     /**
      *  Activate the idle subscriptions.
@@ -279,12 +283,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      */
     size_t GetNumDirtySubscriptions() const;
 
-    /**
-     * Returns whether the write operation to the given path is conflict with another write operations. (i.e. another write
-     * transaction is in the middle of processing the chunked value of the given path.)
-     */
-    bool HasConflictWriteRequests(const WriteHandler * apWriteHandler, const ConcreteAttributePath & aPath);
-
     /**
      * Select the oldest (and the one that exceeds the per subscription resource minimum if there are any) read handler on the
      * fabric with the given fabric index. Evict it when the fabric uses more resources than the per fabric quota or aForceEvict is

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -36,10 +36,12 @@ using namespace Protocols::InteractionModel;
 using Status                         = Protocols::InteractionModel::Status;
 constexpr uint8_t kListAttributeType = 0x48;
 
-CHIP_ERROR WriteHandler::Init()
+CHIP_ERROR WriteHandler::Init(WriteHandlerDelegate * apWriteHandlerDelegate)
 {
     VerifyOrReturnError(!mExchangeCtx, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(apWriteHandlerDelegate, CHIP_ERROR_INVALID_ARGUMENT);
 
+    mDelegate = apWriteHandlerDelegate;
     MoveToState(State::Initialized);
 
     mACLCheckCache.ClearValue();
@@ -241,7 +243,8 @@ CHIP_ERROR WriteHandler::DeliverFinalListWriteEndForGroupWrite(bool writeWasSucc
 
         processingConcreteAttributePath.mEndpointId = mapping.endpoint_id;
 
-        if (!InteractionModelEngine::GetInstance()->HasConflictWriteRequests(this, processingConcreteAttributePath))
+        VerifyOrReturnError(mDelegate, CHIP_ERROR_INCORRECT_STATE);
+        if (!mDelegate->HasConflictWriteRequests(this, processingConcreteAttributePath))
         {
             DeliverListWriteEnd(processingConcreteAttributePath, writeWasSuccessful);
         }
@@ -306,7 +309,8 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
             dataAttributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
         }
 
-        if (InteractionModelEngine::GetInstance()->HasConflictWriteRequests(this, dataAttributePath) ||
+        VerifyOrExit(mDelegate, err = CHIP_ERROR_INCORRECT_STATE);
+        if (mDelegate->HasConflictWriteRequests(this, dataAttributePath) ||
             // Per chunking protocol, we are processing the list entries, but the initial empty list is not processed, so we reject
             // it with Busy status code.
             (dataAttributePath.IsListItemOperation() && !IsSameAttribute(mProcessingAttributePath, dataAttributePath)))
@@ -458,13 +462,15 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
             {
                 auto processingConcreteAttributePath        = mProcessingAttributePath.Value();
                 processingConcreteAttributePath.mEndpointId = mapping.endpoint_id;
-                if (!InteractionModelEngine::GetInstance()->HasConflictWriteRequests(this, processingConcreteAttributePath))
+                VerifyOrExit(mDelegate, err = CHIP_ERROR_INCORRECT_STATE);
+                if (mDelegate->HasConflictWriteRequests(this, processingConcreteAttributePath))
                 {
                     DeliverListWriteEnd(processingConcreteAttributePath, true /* writeWasSuccessful */);
                 }
             }
 
-            if (InteractionModelEngine::GetInstance()->HasConflictWriteRequests(this, dataAttributePath))
+            VerifyOrExit(mDelegate, err = CHIP_ERROR_INCORRECT_STATE);
+            if (mDelegate->HasConflictWriteRequests(this, dataAttributePath))
             {
                 ChipLogDetail(DataManagement,
                               "Writing attribute endpoint=%u Cluster=" ChipLogFormatMEI " attribute=" ChipLogFormatMEI

diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h
@@ -19,6 +19,7 @@
 #pragma once
 #include <app/AttributeAccessToken.h>
 #include <app/AttributePathParams.h>
+#include <app/InteractionModelDelegatePointers.h>
 #include <app/MessageDef/WriteResponseMessage.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/TLVDebug.h>
@@ -35,6 +36,21 @@
 
 namespace chip {
 namespace app {
+
+class WriteHandler;
+
+class WriteHandlerDelegate
+{
+public:
+    virtual ~WriteHandlerDelegate() = default;
+
+    /**
+     * Returns whether the write operation to the given path is in conflict with another write operation.
+     * (i.e. another write transaction is in the middle of processing a chunked write to the given path.)
+     */
+    virtual bool HasConflictWriteRequests(const WriteHandler * apWriteHandler, const ConcreteAttributePath & aPath) = 0;
+};
+
 /**
  *  @brief The write handler is responsible for processing a write request and sending a write reply.
  */
@@ -49,11 +65,13 @@ class WriteHandler : public Messaging::ExchangeDelegate
      *  construction until a call to Close is made to terminate the
      *  instance.
      *
+     *  @param[in] apWriteHandlerDelegate  A Valid pointer to the WriteHandlerDelegate.
+     *
      *  @retval #CHIP_ERROR_INCORRECT_STATE If the state is not equal to
      *          kState_NotInitialized.
      *  @retval #CHIP_NO_ERROR On success.
      */
-    CHIP_ERROR Init();
+    CHIP_ERROR Init(WriteHandlerDelegate * apWriteHandlerDelegate);
 
     /**
      *  Process a write request.  Parts of the processing may end up being asynchronous, but the WriteHandler
@@ -175,6 +193,14 @@ class WriteHandler : public Messaging::ExchangeDelegate
     //  Where (1)-(3) will be consistent among the whole list write request, while (4) and (5) are not appliable to group writes.
     bool mAttributeWriteSuccessful                = false;
     Optional<AttributeAccessToken> mACLCheckCache = NullOptional;
+
+    // This may be a "fake" pointer or a real delegate pointer, depending
+    // on CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE setting.
+    //
+    // When this is not a real pointer, it checks that the value is always
+    // set to the global InteractionModelEngine and the size of this
+    // member is 1 byte.
+    InteractionModelDelegatePointer<WriteHandlerDelegate> mDelegate;
 };
 } // namespace app
 } // namespace chip
diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
@@ -338,7 +338,7 @@ void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apCont
             app::WriteHandler writeHandler;
 
             System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
-            err                            = writeHandler.Init();
+            err                            = writeHandler.Init(chip::app::InteractionModelEngine::GetInstance());
 
             GenerateWriteRequest(apSuite, apContext, messageIsTimed, buf);
 

diff --git a/src/lib/support/static_support_smart_ptr.h b/src/lib/support/static_support_smart_ptr.h
@@ -54,6 +54,7 @@ template <class T>
 class CheckedGlobalInstanceReference
 {
 public:
+    CheckedGlobalInstanceReference<T>() = default;
     CheckedGlobalInstanceReference(T * e) { VerifyOrDie(e == GlobalInstanceProvider<T>::InstancePointer()); }
     CheckedGlobalInstanceReference & operator=(T * value)
     {
@@ -63,6 +64,7 @@ class CheckedGlobalInstanceReference
 
     inline T * operator->() { return GlobalInstanceProvider<T>::InstancePointer(); }
     inline const T * operator->() const { return GlobalInstanceProvider<T>::InstancePointer(); }
+    inline operator bool() const { return true; }
 };
 
 /// A class that acts as a wrapper to a pointer and provides
@@ -87,6 +89,7 @@ template <class T>
 class SimpleInstanceReference
 {
 public:
+    SimpleInstanceReference<T>() = default;
     SimpleInstanceReference(T * e) : mValue(e) {}
     SimpleInstanceReference & operator=(T * value)
     {
@@ -96,9 +99,10 @@ class SimpleInstanceReference
 
     T * operator->() { return mValue; }
     const T * operator->() const { return mValue; }
+    inline operator bool() const { return mValue != nullptr; }
 
 private:
-    T * mValue;
+    T * mValue = nullptr;
 };
 
 } // namespace chip
diff --git a/src/lib/support/tests/TestStaticSupportSmartPtr.cpp b/src/lib/support/tests/TestStaticSupportSmartPtr.cpp
@@ -56,6 +56,7 @@ TEST(TestStaticSupportSmartPtr, TestCheckedGlobalInstanceReference)
     // We expect that sizes of global references is minimal
     EXPECT_EQ(sizeof(ref), 1u);
 
+    ASSERT_TRUE(ref);
     EXPECT_EQ(ref->num, 123);
     EXPECT_STREQ(ref->str, "abc");
 
@@ -69,6 +70,7 @@ TEST(TestStaticSupportSmartPtr, TestCheckedGlobalInstanceReference)
 
     CheckedGlobalInstanceReference<TestClass> ref2(&gTestClass);
 
+    ASSERT_TRUE(ref2);
     EXPECT_EQ(ref->num, ref2->num);
     EXPECT_STREQ(ref->str, ref2->str);
 
@@ -82,6 +84,10 @@ TEST(TestStaticSupportSmartPtr, TestCheckedGlobalInstanceReference)
         EXPECT_EQ(ref2->num, 321);
         EXPECT_STREQ(ref2->str, "test");
     }
+
+    // Check default constructed CheckedGlobalInstanceReference
+    CheckedGlobalInstanceReference<TestClass> ref_default;
+    ASSERT_TRUE(ref_default);
 }
 
 TEST(TestStaticSupportSmartPtr, TestSimpleInstanceReference)
@@ -95,6 +101,9 @@ TEST(TestStaticSupportSmartPtr, TestSimpleInstanceReference)
     // overhead of simple references should be a simple pointer
     EXPECT_LE(sizeof(ref_a), sizeof(void *));
 
+    ASSERT_TRUE(ref_a);
+    ASSERT_TRUE(ref_b);
+
     EXPECT_EQ(ref_a->num, 123);
     EXPECT_EQ(ref_b->num, 100);
 
@@ -103,6 +112,10 @@ TEST(TestStaticSupportSmartPtr, TestSimpleInstanceReference)
 
     EXPECT_EQ(a.num, 99);
     EXPECT_EQ(ref_b->num, 30);
+
+    // Check default constructed SimpleInstanceReference
+    SimpleInstanceReference<TestClass> ref_default;
+    ASSERT_FALSE(ref_default);
 }
 
 } // namespace