diff --git a/src/app/InteractionModelDelegatePointers.cpp b/src/app/InteractionModelDelegatePointers.cpp index af4c85ee1bb1c2..56583888f3bc81 100644 --- a/src/app/InteractionModelDelegatePointers.cpp +++ b/src/app/InteractionModelDelegatePointers.cpp @@ -31,6 +31,12 @@ app::TimedHandlerDelegate * GlobalInstanceProvider::I return app::InteractionModelEngine::GetInstance(); } +template <> +app::WriteHandlerDelegate * GlobalInstanceProvider::InstancePointer() +{ + return app::InteractionModelEngine::GetInstance(); +} + } // namespace chip #endif diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 97528768cb2561..6ee2a1134594b5 100644 --- 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 index 87c497c9e9955d..4f61cb4e464a27 100644 --- 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 index f7f3ee1c8ae7ae..068f1f6b5c3004 100644 --- 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 index 9a004d9b0e5e60..907aa043561eed 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -19,6 +19,7 @@ #pragma once #include #include +#include #include #include #include @@ -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 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 mDelegate; }; } // namespace app } // namespace chip diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 25c9106f2986a0..795c52a9030f99 100644 --- 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 index 896073a9b15801..76035e48d33841 100644 --- a/src/lib/support/static_support_smart_ptr.h +++ b/src/lib/support/static_support_smart_ptr.h @@ -54,6 +54,7 @@ template class CheckedGlobalInstanceReference { public: + CheckedGlobalInstanceReference() = default; CheckedGlobalInstanceReference(T * e) { VerifyOrDie(e == GlobalInstanceProvider::InstancePointer()); } CheckedGlobalInstanceReference & operator=(T * value) { @@ -63,6 +64,7 @@ class CheckedGlobalInstanceReference inline T * operator->() { return GlobalInstanceProvider::InstancePointer(); } inline const T * operator->() const { return GlobalInstanceProvider::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 SimpleInstanceReference { public: + SimpleInstanceReference() = 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 index b51afc15729ad8..5ffe1cee4d284b 100644 --- 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 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 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 ref_default; + ASSERT_FALSE(ref_default); } } // namespace