From 19a09a055ebeab5be89404ff6421c4f1d56743b8 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Thu, 1 Feb 2024 12:07:53 -0500 Subject: [PATCH 01/16] Remove dependency of InteractionModelEngine in CommandHandler --- src/app/CommandHandler.cpp | 6 +++--- src/app/CommandHandler.h | 5 +++++ src/app/InteractionModelEngine.h | 2 +- src/app/tests/TestCommandInteraction.cpp | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 6831a8440606b2..541c949819082f 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -810,14 +810,14 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber()) ? mpHandler : nullptr; + return (mMagic == mpHandler->mpCallback->GetMagicNumber()) ? mpHandler : nullptr; } void CommandHandler::Handle::Release() { if (mpHandler != nullptr) { - if (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber()) + if (mMagic == mpHandler->mpCallback->GetMagicNumber()) { mpHandler->DecrementHoldOff(); } @@ -832,7 +832,7 @@ CommandHandler::Handle::Handle(CommandHandler * handle) { handle->IncrementHoldOff(); mpHandler = handle; - mMagic = InteractionModelEngine::GetInstance()->GetMagicNumber(); + mMagic = mpHandler->mpCallback->GetMagicNumber(); } } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index b90cb821244336..91fe1973678be2 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -85,6 +85,11 @@ class CommandHandler * fails to exist. */ virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; + + /* + * Get the magic number of the InteractionModelEngine. + */ + virtual uint32_t GetMagicNumber() const = 0; }; /** diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 8ed201a0d934e6..a7f1260dd90706 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -180,7 +180,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, /** * The Magic number of this InteractionModelEngine, the magic number is set during Init() */ - uint32_t GetMagicNumber() const { return mMagic; } + uint32_t GetMagicNumber() const override { return mMagic; } reporting::Engine & GetReportingEngine() { return mReportingEngine; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 02b4b01b116a9f..b608a483de015f 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -285,6 +285,8 @@ class MockCommandHandlerCallback : public CommandHandler::Callback void ResetCounter() { onFinalCalledTimes = 0; } + uint32_t GetMagicNumber() const { return 1; } + int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; From ffd94d30e52bcb7a390654fd0cbb563ecffd07bc Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Thu, 1 Feb 2024 12:50:58 -0500 Subject: [PATCH 02/16] Address comments and fix tizen test --- examples/lighting-app/tizen/src/DBusInterface.cpp | 1 + src/app/CommandHandler.cpp | 6 +++--- src/app/CommandHandler.h | 7 ++++--- src/app/InteractionModelEngine.h | 5 +++-- src/app/tests/TestCommandInteraction.cpp | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 7c6e29d6a1cc38..02a750c6a39adc 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -44,6 +44,7 @@ class CommandHandlerCallback : public CommandHandler::Callback void OnDone(CommandHandler & apCommandObj) {} void DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } + uint32_t GetInteractionModelEngineMagicNumber() const { return InteractionModelEngine::GetInstance()->GetInteractionModelEngineMagicNumber(); } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 541c949819082f..1950faf0f3f163 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -810,14 +810,14 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mMagic == mpHandler->mpCallback->GetMagicNumber()) ? mpHandler : nullptr; + return (mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) ? mpHandler : nullptr; } void CommandHandler::Handle::Release() { if (mpHandler != nullptr) { - if (mMagic == mpHandler->mpCallback->GetMagicNumber()) + if (mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) { mpHandler->DecrementHoldOff(); } @@ -832,7 +832,7 @@ CommandHandler::Handle::Handle(CommandHandler * handle) { handle->IncrementHoldOff(); mpHandler = handle; - mMagic = mpHandler->mpCallback->GetMagicNumber(); + mMagic = mpHandler->mpCallback->GetInteractionModelEngineMagicNumber(); } } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 91fe1973678be2..f8092316a39f4a 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -87,9 +87,10 @@ class CommandHandler virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; /* - * Get the magic number of the InteractionModelEngine. + * Get the magic number of the InteractionModelEngine. A CommandHandler::Handle is valid iff + * its magic number is equals to the InteractionModelEngine's one. */ - virtual uint32_t GetMagicNumber() const = 0; + virtual uint32_t GetInteractionModelEngineMagicNumber() const = 0; }; /** @@ -148,7 +149,7 @@ class CommandHandler } /** - * Get the CommandHandler object it holds. Get() may return a nullptr if the CommandHandler object is holds is no longer + * Get the CommandHandler object it holds. Get() may return a nullptr if the CommandHandler object it holds is no longer * valid. */ CommandHandler * Get(); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index a7f1260dd90706..e04a754c95e970 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -178,9 +178,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex); /** - * The Magic number of this InteractionModelEngine, the magic number is set during Init() + * The Magic number of this InteractionModelEngine, the magic number is set during Init() and Shundown(). + * An CommandHandler::Handle is valid iff. its magic equals to this one. */ - uint32_t GetMagicNumber() const override { return mMagic; } + uint32_t GetInteractionModelEngineMagicNumber() const override { return mMagic; } reporting::Engine & GetReportingEngine() { return mReportingEngine; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index b608a483de015f..4cfdade15c8793 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -285,7 +285,7 @@ class MockCommandHandlerCallback : public CommandHandler::Callback void ResetCounter() { onFinalCalledTimes = 0; } - uint32_t GetMagicNumber() const { return 1; } + uint32_t GetInteractionModelEngineMagicNumber() const { return chip::app::InteractionModelEngine::GetInstance()->GetInteractionModelEngineMagicNumber(); } int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; From b6407ffd2709647969f3298f4b89759b8b9fe6e7 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Thu, 1 Feb 2024 13:14:23 -0500 Subject: [PATCH 03/16] Fix tizen example --- examples/lighting-app/tizen/src/DBusInterface.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 02a750c6a39adc..be5c055a2b3250 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include From e97f98c2635924f276ce28db5a2c5b18d81b2f4a Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Thu, 1 Feb 2024 15:23:43 -0500 Subject: [PATCH 04/16] Fix segfault in CommandHandler::Handle::Get() --- src/app/CommandHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 1950faf0f3f163..e1653bbe3f2abe 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -810,7 +810,7 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) ? mpHandler : nullptr; + return (mpHandler != nullptr && mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) ? mpHandler : nullptr; } void CommandHandler::Handle::Release() From 9d2b95d1d5761a4833a5dd4112b25d99328597e3 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Mon, 5 Feb 2024 11:00:56 -0500 Subject: [PATCH 05/16] Change CommandHandler::Callback function name --- .../lighting-app/tizen/src/DBusInterface.cpp | 2 +- src/app/CommandHandler.cpp | 8 ++++---- src/app/CommandHandler.h | 16 ++++++++-------- src/app/InteractionModelEngine.h | 6 +++--- src/app/tests/TestCommandInteraction.cpp | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index be5c055a2b3250..b8cea6d54d2ca9 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -45,7 +45,7 @@ class CommandHandlerCallback : public CommandHandler::Callback void OnDone(CommandHandler & apCommandObj) {} void DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } - uint32_t GetInteractionModelEngineMagicNumber() const { return InteractionModelEngine::GetInstance()->GetInteractionModelEngineMagicNumber(); } + uint32_t GetInteractionModelEngineGeneration() const { return InteractionModelEngine::GetInstance()->GetInteractionModelEngineGeneration(); } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index e1653bbe3f2abe..1a25550dfae100 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -810,19 +810,19 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mpHandler != nullptr && mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) ? mpHandler : nullptr; + return (mpHandler != nullptr && mImEngineGeneration == mpHandler->mpCallback->GetInteractionModelEngineGeneration()) ? mpHandler : nullptr; } void CommandHandler::Handle::Release() { if (mpHandler != nullptr) { - if (mMagic == mpHandler->mpCallback->GetInteractionModelEngineMagicNumber()) + if (mImEngineGeneration == mpHandler->mpCallback->GetInteractionModelEngineGeneration()) { mpHandler->DecrementHoldOff(); } mpHandler = nullptr; - mMagic = 0; + mImEngineGeneration = 0; } } @@ -832,7 +832,7 @@ CommandHandler::Handle::Handle(CommandHandler * handle) { handle->IncrementHoldOff(); mpHandler = handle; - mMagic = mpHandler->mpCallback->GetInteractionModelEngineMagicNumber(); + mImEngineGeneration = mpHandler->mpCallback->GetInteractionModelEngineGeneration(); } } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index f8092316a39f4a..f6544f2dcd7b62 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -87,10 +87,10 @@ class CommandHandler virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; /* - * Get the magic number of the InteractionModelEngine. A CommandHandler::Handle is valid iff - * its magic number is equals to the InteractionModelEngine's one. + * Get the generation number of the InteractionModelEngine. A CommandHandler::Handle is valid iff + * its number is equals to the InteractionModelEngine's one. */ - virtual uint32_t GetInteractionModelEngineMagicNumber() const = 0; + virtual uint32_t GetInteractionModelEngineGeneration() const = 0; }; /** @@ -124,9 +124,9 @@ class CommandHandler Handle(Handle && handle) { mpHandler = handle.mpHandler; - mMagic = handle.mMagic; + mImEngineGeneration = handle.mImEngineGeneration; handle.mpHandler = nullptr; - handle.mMagic = 0; + handle.mImEngineGeneration = 0; } Handle(decltype(nullptr)) {} Handle(CommandHandler * handle); @@ -136,9 +136,9 @@ class CommandHandler { Release(); mpHandler = handle.mpHandler; - mMagic = handle.mMagic; + mImEngineGeneration = handle.mImEngineGeneration; handle.mpHandler = nullptr; - handle.mMagic = 0; + handle.mImEngineGeneration = 0; return *this; } @@ -158,7 +158,7 @@ class CommandHandler private: CommandHandler * mpHandler = nullptr; - uint32_t mMagic = 0; + uint32_t mImEngineGeneration = 0; }; // Previously we kept adding arguments with default values individually as parameters. This is because there diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index e04a754c95e970..57075950d4a557 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -178,10 +178,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex); /** - * The Magic number of this InteractionModelEngine, the magic number is set during Init() and Shundown(). - * An CommandHandler::Handle is valid iff. its magic equals to this one. + * The generation number of this InteractionModelEngine, the number is updated during Init() and Shundown(). + * An CommandHandler::Handle is valid iff. its number equals to this one. */ - uint32_t GetInteractionModelEngineMagicNumber() const override { return mMagic; } + uint32_t GetInteractionModelEngineGeneration() const override { return mMagic; } reporting::Engine & GetReportingEngine() { return mReportingEngine; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 4cfdade15c8793..a61e3ee4f23a9b 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -285,7 +285,7 @@ class MockCommandHandlerCallback : public CommandHandler::Callback void ResetCounter() { onFinalCalledTimes = 0; } - uint32_t GetInteractionModelEngineMagicNumber() const { return chip::app::InteractionModelEngine::GetInstance()->GetInteractionModelEngineMagicNumber(); } + uint32_t GetInteractionModelEngineGeneration() const { return chip::app::InteractionModelEngine::GetInstance()->GetInteractionModelEngineGeneration(); } int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; From ca93da05ff321265b291382c9ed8c7230f6935cd Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Fri, 26 Apr 2024 15:25:08 -0400 Subject: [PATCH 06/16] Add virtual function in CommandHandler::Callback in CommandResponseSender --- src/app/CommandResponseSender.cpp | 5 +++++ src/app/CommandResponseSender.h | 2 ++ 2 files changed, 7 insertions(+) diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index d7f40caa476c8e..76595985a787e8 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -138,6 +138,11 @@ Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommand return mpCommandHandlerCallback->CommandExists(aCommandPath); } +uint32_t CommandResponseSender::GetInteractionModelEngineGeneration() const { + VerifyOrReturnValue(mpCommandHandlerCallback, 0); + return mpCommandHandlerCallback->GetInteractionModelEngineGeneration(); +} + CHIP_ERROR CommandResponseSender::SendCommandResponse() { VerifyOrReturnError(HasMoreToSend(), CHIP_ERROR_INCORRECT_STATE); diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index a89970ddec6c92..7eabbc40c8628c 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -66,6 +66,8 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + uint32_t GetInteractionModelEngineGeneration() const override; + /** * Gets the inner exchange context object, without ownership. * From 4d3ab1215d01e95e5f270cf09911b902e9b9671b Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Mon, 29 Apr 2024 13:27:09 -0400 Subject: [PATCH 07/16] Add mpCallback null checks --- examples/lighting-app/tizen/src/DBusInterface.cpp | 2 +- src/app/CommandHandler.cpp | 12 +++++++++--- src/app/CommandHandler.h | 8 +++++--- src/app/CommandResponseSender.cpp | 4 ++-- src/app/CommandResponseSender.h | 2 +- src/app/InteractionModelEngine.h | 2 +- src/app/tests/TestCommandInteraction.cpp | 2 +- 7 files changed, 20 insertions(+), 12 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 8837a9b00d96fd..7808b46266b7b1 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -45,7 +45,7 @@ class CommandHandlerCallback : public CommandHandler::Callback void OnDone(CommandHandler & apCommandObj) {} void DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } - uint32_t GetInteractionModelEngineGeneration() const { return InteractionModelEngine::GetInstance()->GetInteractionModelEngineGeneration(); } + uint32_t GetCommandHandlerGeneration() const { return InteractionModelEngine::GetInstance()->GetCommandHandlerGeneration(); } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index de63b84411e377..f21de64a413b38 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -776,14 +776,14 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mpHandler != nullptr && mImEngineGeneration == mpHandler->mpCallback->GetInteractionModelEngineGeneration()) ? mpHandler : nullptr; + return (mpHandler != nullptr && mImEngineGeneration == mpHandler->GetCommandHandlerGeneration()) ? mpHandler : nullptr; } void CommandHandler::Handle::Release() { if (mpHandler != nullptr) { - if (mImEngineGeneration == mpHandler->mpCallback->GetInteractionModelEngineGeneration()) + if (mImEngineGeneration == mpHandler->GetCommandHandlerGeneration()) { mpHandler->DecrementHoldOff(); } @@ -798,7 +798,7 @@ CommandHandler::Handle::Handle(CommandHandler * handle) { handle->IncrementHoldOff(); mpHandler = handle; - mImEngineGeneration = mpHandler->mpCallback->GetInteractionModelEngineGeneration(); + mImEngineGeneration = mpHandler->GetCommandHandlerGeneration(); } } @@ -885,6 +885,12 @@ void CommandHandler::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "Command handler moving to [%10.10s]", GetStateStr()); } + +uint32_t CommandHandler::GetCommandHandlerGeneration() const { + VerifyOrReturnValue(mpCallback != nullptr, 0); + return mpCallback->GetCommandHandlerGeneration(); +} + #if CHIP_WITH_NLFAULTINJECTION namespace { diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index f93979cb2592f6..0b23b1210de001 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -87,10 +87,10 @@ class CommandHandler virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; /* - * Get the generation number of the InteractionModelEngine. A CommandHandler::Handle is valid iff - * its number is equals to the InteractionModelEngine's one. + * Get the generation number of the CommandHandler. A CommandHandler::Handle is valid iff + * its number is equals to the return of this call. */ - virtual uint32_t GetInteractionModelEngineGeneration() const = 0; + virtual uint32_t GetCommandHandlerGeneration() const = 0; }; /** @@ -678,6 +678,8 @@ class CommandHandler size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; } + uint32_t GetCommandHandlerGeneration() const; + bool TestOnlyIsInIdleState() const { return mState == State::Idle; } Callback * mpCallback = nullptr; diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index 76595985a787e8..8a0232cedbac92 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -138,9 +138,9 @@ Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommand return mpCommandHandlerCallback->CommandExists(aCommandPath); } -uint32_t CommandResponseSender::GetInteractionModelEngineGeneration() const { +uint32_t CommandResponseSender::GetCommandHandlerGeneration() const { VerifyOrReturnValue(mpCommandHandlerCallback, 0); - return mpCommandHandlerCallback->GetInteractionModelEngineGeneration(); + return mpCommandHandlerCallback->GetCommandHandlerGeneration(); } CHIP_ERROR CommandResponseSender::SendCommandResponse() diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 7eabbc40c8628c..a890ae6c7cd403 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -66,7 +66,7 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; - uint32_t GetInteractionModelEngineGeneration() const override; + uint32_t GetCommandHandlerGeneration() const override; /** * Gets the inner exchange context object, without ownership. diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index e50ce7b17428c4..41eb56fb2b7944 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -196,7 +196,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, * The generation number of this InteractionModelEngine, the number is updated during Init() and Shundown(). * An CommandHandler::Handle is valid iff. its number equals to this one. */ - uint32_t GetInteractionModelEngineGeneration() const override { return mMagic; } + uint32_t GetCommandHandlerGeneration() const override { return mMagic; } reporting::Engine & GetReportingEngine() { return mReportingEngine; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index f57607fb21f780..7057dd2daf5144 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -316,7 +316,7 @@ class MockCommandHandlerCallback : public CommandHandler::Callback void ResetCounter() { onFinalCalledTimes = 0; } - uint32_t GetInteractionModelEngineGeneration() const { return chip::app::InteractionModelEngine::GetInstance()->GetInteractionModelEngineGeneration(); } + uint32_t GetCommandHandlerGeneration() const { return chip::app::InteractionModelEngine::GetInstance()->GetCommandHandlerGeneration(); } int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; From b06d94e32aa618496e51da95859a0f70a1dc5493 Mon Sep 17 00:00:00 2001 From: "Yuanyao (YY) Zhong" Date: Wed, 1 May 2024 11:14:51 -0400 Subject: [PATCH 08/16] Add weak reference to CommandHandler --- src/app/CommandHandler.cpp | 52 ++++++++++++++++++++++++++++++-------- src/app/CommandHandler.h | 29 +++++++++++++++------ 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index f21de64a413b38..bf057a344e97c9 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -58,6 +58,12 @@ CommandHandler::CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apC } } +CommandHandler::~CommandHandler() { + for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) { + cur->Invalidate(); + } +} + CHIP_ERROR CommandHandler::AllocateBuffer() { // We should only allocate a buffer if we will be sending out a response. @@ -266,6 +272,9 @@ void CommandHandler::Close() // reference is the stack shutting down, in which case Close() is not called. So the below check should always pass. VerifyOrDieWithMsg(mPendingWork == 0, DataManagement, "CommandHandler::Close() called with %u unfinished async work items", static_cast(mPendingWork)); + for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) { + cur->Invalidate(); + } if (mpCallback) { @@ -273,16 +282,44 @@ void CommandHandler::Close() } } -void CommandHandler::IncrementHoldOff() +void CommandHandler::IncrementHoldOff(Handle* apHandle) { + ChipLogDetail(DataManagement, "IncrementHoldOff"); mPendingWork++; + apHandle->SetNext(mpHandlerList); + mpHandlerList = apHandle; } -void CommandHandler::DecrementHoldOff() +void CommandHandler::DecrementHoldOff(Handle* apHandle) { mPendingWork--; ChipLogDetail(DataManagement, "Decreasing reference count for CommandHandler, remaining %u", static_cast(mPendingWork)); + + Handle* prev = nullptr; + if (mpHandlerList != nullptr) { + ChipLogDetail(DataManagement, "mpHandlerList is not null");} else {ChipLogDetail(DataManagement, "cur is null");} + for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) + { + if (cur != nullptr) { + ChipLogDetail(DataManagement, "cur is not null");} else {ChipLogDetail(DataManagement, "cur is null");} + if (cur == apHandle) + { + if (prev == nullptr) + { + mpHandlerList = cur->GetNext(); + } + else + { + prev->SetNext(cur->GetNext()); + } + + cur->SetNext(nullptr); + } + prev = cur; + } + + if (mPendingWork != 0) { return; @@ -776,19 +813,15 @@ CommandHandler * CommandHandler::Handle::Get() // Not safe to work with CommandHandler in parallel with other Matter work. assertChipStackLockedByCurrentThread(); - return (mpHandler != nullptr && mImEngineGeneration == mpHandler->GetCommandHandlerGeneration()) ? mpHandler : nullptr; + return mpHandler; } void CommandHandler::Handle::Release() { if (mpHandler != nullptr) { - if (mImEngineGeneration == mpHandler->GetCommandHandlerGeneration()) - { - mpHandler->DecrementHoldOff(); - } + mpHandler->DecrementHoldOff(this); mpHandler = nullptr; - mImEngineGeneration = 0; } } @@ -796,9 +829,8 @@ CommandHandler::Handle::Handle(CommandHandler * handle) { if (handle != nullptr) { - handle->IncrementHoldOff(); + handle->IncrementHoldOff(this); mpHandler = handle; - mImEngineGeneration = mpHandler->GetCommandHandlerGeneration(); } } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 0b23b1210de001..4fd064b01eb04b 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -124,9 +124,7 @@ class CommandHandler Handle(Handle && handle) { mpHandler = handle.mpHandler; - mImEngineGeneration = handle.mImEngineGeneration; handle.mpHandler = nullptr; - handle.mImEngineGeneration = 0; } Handle(decltype(nullptr)) {} Handle(CommandHandler * handle); @@ -136,9 +134,7 @@ class CommandHandler { Release(); mpHandler = handle.mpHandler; - mImEngineGeneration = handle.mImEngineGeneration; handle.mpHandler = nullptr; - handle.mImEngineGeneration = 0; return *this; } @@ -157,8 +153,16 @@ class CommandHandler void Release(); private: + friend CommandHandler; + /** + * Mechanism for keeping track of a chain of Handle. + */ + void SetNext(Handle * aNext) { mNext = aNext; } + Handle* GetNext() const { return mNext; } + void Invalidate() {mpHandler = nullptr;} + CommandHandler * mpHandler = nullptr; - uint32_t mImEngineGeneration = 0; + Handle* mNext = nullptr; }; // Previously we kept adding arguments with default values individually as parameters. This is because there @@ -197,6 +201,14 @@ class CommandHandler */ CommandHandler(Callback * apCallback); + /* + * Destructor. + * + * The call will also invalidate all Handles created for this CommandHandler. + * + */ + ~CommandHandler(); + /* * Constructor to override the number of supported paths per invoke and command responder. * @@ -460,7 +472,7 @@ class CommandHandler * Tries to add response using lambda. Upon failure to add response, attempts * to rollback the InvokeResponseMessage to a known good state. If failure is due * to insufficient space in the current InvokeResponseMessage: - * - Finalizes the current InvokeResponseMessage. + * - Finalizes the current InvokeResponsfeMessage. * - Allocates a new InvokeResponseMessage. * - Reattempts to add the InvokeResponse to the new InvokeResponseMessage. * @@ -531,13 +543,13 @@ class CommandHandler * Users should use CommandHandler::Handle for management the lifespan of the CommandHandler. * DefRef should be released in reasonable time, and Close() should only be called when the refcount reached 0. */ - void IncrementHoldOff(); + void IncrementHoldOff(Handle* apHandle); /** * DecrementHoldOff is used by CommandHandler::Handle for decreasing the refcount of the CommandHandler. * When refcount reached 0, CommandHandler will send the response to the peer and shutdown. */ - void DecrementHoldOff(); + void DecrementHoldOff(Handle* apHandle); /* * Allocates a packet buffer used for encoding an invoke response payload. @@ -686,6 +698,7 @@ class CommandHandler InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; + Handle* mpHandlerList = nullptr; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; From 2f5d872056c3b21dbada72285e833d15cac90174 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Wed, 1 May 2024 14:45:05 -0400 Subject: [PATCH 09/16] Remove magic number from ImEngine --- .../lighting-app/tizen/src/DBusInterface.cpp | 1 - src/app/CommandHandler.cpp | 75 ++++++++++--------- src/app/CommandHandler.h | 34 +++++---- src/app/CommandResponseSender.cpp | 5 -- src/app/CommandResponseSender.h | 2 - src/app/InteractionModelEngine.cpp | 4 - src/app/InteractionModelEngine.h | 10 --- src/app/tests/TestCommandInteraction.cpp | 2 - 8 files changed, 59 insertions(+), 74 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 7808b46266b7b1..fabdcda800e05f 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -45,7 +45,6 @@ class CommandHandlerCallback : public CommandHandler::Callback void OnDone(CommandHandler & apCommandObj) {} void DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } - uint32_t GetCommandHandlerGeneration() const { return InteractionModelEngine::GetInstance()->GetCommandHandlerGeneration(); } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index bf057a344e97c9..e4faf88dd2f1d2 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -59,9 +59,7 @@ CommandHandler::CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apC } CommandHandler::~CommandHandler() { - for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) { - cur->Invalidate(); - } + InvalidateHandles(); } CHIP_ERROR CommandHandler::AllocateBuffer() @@ -272,9 +270,7 @@ void CommandHandler::Close() // reference is the stack shutting down, in which case Close() is not called. So the below check should always pass. VerifyOrDieWithMsg(mPendingWork == 0, DataManagement, "CommandHandler::Close() called with %u unfinished async work items", static_cast(mPendingWork)); - for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) { - cur->Invalidate(); - } + InvalidateHandles(); if (mpCallback) { @@ -282,32 +278,20 @@ void CommandHandler::Close() } } -void CommandHandler::IncrementHoldOff(Handle* apHandle) -{ - ChipLogDetail(DataManagement, "IncrementHoldOff"); - mPendingWork++; - apHandle->SetNext(mpHandlerList); - mpHandlerList = apHandle; +void CommandHandler::AddToHandleList(Handle* apHandle) { + apHandle->SetNext(mpHandleList); + mpHandleList = apHandle; } -void CommandHandler::DecrementHoldOff(Handle* apHandle) -{ - mPendingWork--; - ChipLogDetail(DataManagement, "Decreasing reference count for CommandHandler, remaining %u", - static_cast(mPendingWork)); - - Handle* prev = nullptr; - if (mpHandlerList != nullptr) { - ChipLogDetail(DataManagement, "mpHandlerList is not null");} else {ChipLogDetail(DataManagement, "cur is null");} - for (Handle* cur = mpHandlerList; cur; cur = cur->GetNext()) +void CommandHandler::RemoveFromHandleList(Handle* apHandle) { + Handle* prev = nullptr; + for (Handle* cur = mpHandleList; cur; cur = cur->GetNext()) { - if (cur != nullptr) { - ChipLogDetail(DataManagement, "cur is not null");} else {ChipLogDetail(DataManagement, "cur is null");} if (cur == apHandle) { if (prev == nullptr) { - mpHandlerList = cur->GetNext(); + mpHandleList = cur->GetNext(); } else { @@ -318,7 +302,28 @@ void CommandHandler::DecrementHoldOff(Handle* apHandle) } prev = cur; } + } + void CommandHandler::InvalidateHandles() { + for (Handle* cur = mpHandleList; cur; cur = cur->GetNext()) { + cur->Invalidate(); + } + } + +void CommandHandler::IncrementHoldOff(Handle* apHandle) +{ + mPendingWork++; + AddToHandleList(apHandle); +} + +void CommandHandler::DecrementHoldOff(Handle* apHandle) +{ + + mPendingWork--; + ChipLogDetail(DataManagement, "Decreasing reference count for CommandHandler, remaining %u", + static_cast(mPendingWork)); + + RemoveFromHandleList(apHandle); if (mPendingWork != 0) { @@ -808,6 +813,14 @@ FabricIndex CommandHandler::GetAccessingFabricIndex() const return mpResponder->GetAccessingFabricIndex(); } +void CommandHandler::Handle::Init(CommandHandler * handle) { + if (handle != nullptr) + { + handle->IncrementHoldOff(this); + mpHandler = handle; + } +} + CommandHandler * CommandHandler::Handle::Get() { // Not safe to work with CommandHandler in parallel with other Matter work. @@ -827,11 +840,7 @@ void CommandHandler::Handle::Release() CommandHandler::Handle::Handle(CommandHandler * handle) { - if (handle != nullptr) - { - handle->IncrementHoldOff(this); - mpHandler = handle; - } + Init(handle); } CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext() @@ -917,12 +926,6 @@ void CommandHandler::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "Command handler moving to [%10.10s]", GetStateStr()); } - -uint32_t CommandHandler::GetCommandHandlerGeneration() const { - VerifyOrReturnValue(mpCallback != nullptr, 0); - return mpCallback->GetCommandHandlerGeneration(); -} - #if CHIP_WITH_NLFAULTINJECTION namespace { diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 4fd064b01eb04b..450cff614f1112 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -85,12 +85,6 @@ class CommandHandler * fails to exist. */ virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; - - /* - * Get the generation number of the CommandHandler. A CommandHandler::Handle is valid iff - * its number is equals to the return of this call. - */ - virtual uint32_t GetCommandHandlerGeneration() const = 0; }; /** @@ -123,7 +117,10 @@ class CommandHandler Handle(const Handle & handle) = delete; Handle(Handle && handle) { - mpHandler = handle.mpHandler; + ChipLogDetail(DataManagement, "Handle(Handle && handle)"); + Init(handle.mpHandler); + + handle.Release(); handle.mpHandler = nullptr; } Handle(decltype(nullptr)) {} @@ -132,8 +129,11 @@ class CommandHandler Handle & operator=(Handle && handle) { + ChipLogDetail(DataManagement, "Handle & operator=(Handle && handle)"); Release(); - mpHandler = handle.mpHandler; + Init(handle.mpHandler); + + handle.Release(); handle.mpHandler = nullptr; return *this; } @@ -154,13 +154,15 @@ class CommandHandler private: friend CommandHandler; - /** - * Mechanism for keeping track of a chain of Handle. - */ + /** + * Mechanism for keeping track of a chain of Handle. + */ void SetNext(Handle * aNext) { mNext = aNext; } Handle* GetNext() const { return mNext; } void Invalidate() {mpHandler = nullptr;} + void Init(CommandHandler* handler); + CommandHandler * mpHandler = nullptr; Handle* mNext = nullptr; }; @@ -472,7 +474,7 @@ class CommandHandler * Tries to add response using lambda. Upon failure to add response, attempts * to rollback the InvokeResponseMessage to a known good state. If failure is due * to insufficient space in the current InvokeResponseMessage: - * - Finalizes the current InvokeResponsfeMessage. + * - Finalizes the current InvokeResponseMessage. * - Allocates a new InvokeResponseMessage. * - Reattempts to add the InvokeResponse to the new InvokeResponseMessage. * @@ -690,7 +692,11 @@ class CommandHandler size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; } - uint32_t GetCommandHandlerGeneration() const; + void AddToHandleList(Handle* handle); + + void RemoveFromHandleList(Handle* handle); + + void InvalidateHandles(); bool TestOnlyIsInIdleState() const { return mState == State::Idle; } @@ -698,7 +704,7 @@ class CommandHandler InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; - Handle* mpHandlerList = nullptr; + Handle* mpHandleList = nullptr; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index 8a0232cedbac92..d7f40caa476c8e 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -138,11 +138,6 @@ Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommand return mpCommandHandlerCallback->CommandExists(aCommandPath); } -uint32_t CommandResponseSender::GetCommandHandlerGeneration() const { - VerifyOrReturnValue(mpCommandHandlerCallback, 0); - return mpCommandHandlerCallback->GetCommandHandlerGeneration(); -} - CHIP_ERROR CommandResponseSender::SendCommandResponse() { VerifyOrReturnError(HasMoreToSend(), CHIP_ERROR_INCORRECT_STATE); diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index a890ae6c7cd403..a89970ddec6c92 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -66,8 +66,6 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; - uint32_t GetCommandHandlerGeneration() const override; - /** * Gets the inner exchange context object, without ownership. * diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 64c06f92354f48..97528768cb2561 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -85,7 +85,6 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this)); mReportingEngine.Init(); - mMagic++; StatusIB::RegisterErrorFormatter(); @@ -111,9 +110,6 @@ void InteractionModelEngine::Shutdown() mCommandHandlerList = nullptr; - // Increase magic number to invalidate all Handle-s. - mMagic++; - mCommandResponderObjs.ReleaseAll(); mTimedHandlers.ForEachActiveObject([this](TimedHandler * obj) -> Loop { diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 41eb56fb2b7944..87c497c9e9955d 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -192,12 +192,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex); - /** - * The generation number of this InteractionModelEngine, the number is updated during Init() and Shundown(). - * An CommandHandler::Handle is valid iff. its number equals to this one. - */ - uint32_t GetCommandHandlerGeneration() const override { return mMagic; } - reporting::Engine & GetReportingEngine() { return mReportingEngine; } reporting::ReportScheduler * GetReportScheduler() { return mReportScheduler; } @@ -707,10 +701,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, CASESessionManager * mpCASESessionMgr = nullptr; SubscriptionResumptionStorage * mpSubscriptionResumptionStorage = nullptr; - - // A magic number for tracking values between stack Shutdown()-s and Init()-s. - // An ObjectHandle is valid iff. its magic equals to this one. - uint32_t mMagic = 0; }; } // namespace app diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 7057dd2daf5144..87296580fa695f 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -316,8 +316,6 @@ class MockCommandHandlerCallback : public CommandHandler::Callback void ResetCounter() { onFinalCalledTimes = 0; } - uint32_t GetCommandHandlerGeneration() const { return chip::app::InteractionModelEngine::GetInstance()->GetCommandHandlerGeneration(); } - int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; From ab93bde60dcea0cdf736a9cb298151a1e35f3e8a Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Wed, 1 May 2024 14:48:37 -0400 Subject: [PATCH 10/16] self clean up --- examples/lighting-app/tizen/src/DBusInterface.cpp | 1 - src/app/CommandHandler.h | 2 -- 2 files changed, 3 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index fabdcda800e05f..e39ca5c1e1232c 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -26,7 +26,6 @@ #include #include #include -#include #include #include diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 450cff614f1112..d3a8efd0f86f4c 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -117,7 +117,6 @@ class CommandHandler Handle(const Handle & handle) = delete; Handle(Handle && handle) { - ChipLogDetail(DataManagement, "Handle(Handle && handle)"); Init(handle.mpHandler); handle.Release(); @@ -129,7 +128,6 @@ class CommandHandler Handle & operator=(Handle && handle) { - ChipLogDetail(DataManagement, "Handle & operator=(Handle && handle)"); Release(); Init(handle.mpHandler); From 172d79acc5a24e94f717d5ffa53e3b27b8c88937 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 1 May 2024 18:49:32 +0000 Subject: [PATCH 11/16] Restyled by clang-format --- src/app/CommandHandler.cpp | 30 ++++++++++++++++++------------ src/app/CommandHandler.h | 20 ++++++++++---------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index e4faf88dd2f1d2..373f9587072040 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -58,7 +58,8 @@ CommandHandler::CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apC } } -CommandHandler::~CommandHandler() { +CommandHandler::~CommandHandler() +{ InvalidateHandles(); } @@ -278,14 +279,16 @@ void CommandHandler::Close() } } -void CommandHandler::AddToHandleList(Handle* apHandle) { +void CommandHandler::AddToHandleList(Handle * apHandle) +{ apHandle->SetNext(mpHandleList); mpHandleList = apHandle; } -void CommandHandler::RemoveFromHandleList(Handle* apHandle) { - Handle* prev = nullptr; - for (Handle* cur = mpHandleList; cur; cur = cur->GetNext()) +void CommandHandler::RemoveFromHandleList(Handle * apHandle) +{ + Handle * prev = nullptr; + for (Handle * cur = mpHandleList; cur; cur = cur->GetNext()) { if (cur == apHandle) { @@ -302,21 +305,23 @@ void CommandHandler::RemoveFromHandleList(Handle* apHandle) { } prev = cur; } - } +} - void CommandHandler::InvalidateHandles() { - for (Handle* cur = mpHandleList; cur; cur = cur->GetNext()) { +void CommandHandler::InvalidateHandles() +{ + for (Handle * cur = mpHandleList; cur; cur = cur->GetNext()) + { cur->Invalidate(); } - } +} -void CommandHandler::IncrementHoldOff(Handle* apHandle) +void CommandHandler::IncrementHoldOff(Handle * apHandle) { mPendingWork++; AddToHandleList(apHandle); } -void CommandHandler::DecrementHoldOff(Handle* apHandle) +void CommandHandler::DecrementHoldOff(Handle * apHandle) { mPendingWork--; @@ -813,7 +818,8 @@ FabricIndex CommandHandler::GetAccessingFabricIndex() const return mpResponder->GetAccessingFabricIndex(); } -void CommandHandler::Handle::Init(CommandHandler * handle) { +void CommandHandler::Handle::Init(CommandHandler * handle) +{ if (handle != nullptr) { handle->IncrementHoldOff(this); diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index d3a8efd0f86f4c..cfaefceb3f33ef 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -156,13 +156,13 @@ class CommandHandler * Mechanism for keeping track of a chain of Handle. */ void SetNext(Handle * aNext) { mNext = aNext; } - Handle* GetNext() const { return mNext; } - void Invalidate() {mpHandler = nullptr;} + Handle * GetNext() const { return mNext; } + void Invalidate() { mpHandler = nullptr; } - void Init(CommandHandler* handler); + void Init(CommandHandler * handler); CommandHandler * mpHandler = nullptr; - Handle* mNext = nullptr; + Handle * mNext = nullptr; }; // Previously we kept adding arguments with default values individually as parameters. This is because there @@ -207,7 +207,7 @@ class CommandHandler * The call will also invalidate all Handles created for this CommandHandler. * */ - ~CommandHandler(); + ~CommandHandler(); /* * Constructor to override the number of supported paths per invoke and command responder. @@ -543,13 +543,13 @@ class CommandHandler * Users should use CommandHandler::Handle for management the lifespan of the CommandHandler. * DefRef should be released in reasonable time, and Close() should only be called when the refcount reached 0. */ - void IncrementHoldOff(Handle* apHandle); + void IncrementHoldOff(Handle * apHandle); /** * DecrementHoldOff is used by CommandHandler::Handle for decreasing the refcount of the CommandHandler. * When refcount reached 0, CommandHandler will send the response to the peer and shutdown. */ - void DecrementHoldOff(Handle* apHandle); + void DecrementHoldOff(Handle * apHandle); /* * Allocates a packet buffer used for encoding an invoke response payload. @@ -690,9 +690,9 @@ class CommandHandler size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; } - void AddToHandleList(Handle* handle); + void AddToHandleList(Handle * handle); - void RemoveFromHandleList(Handle* handle); + void RemoveFromHandleList(Handle * handle); void InvalidateHandles(); @@ -702,7 +702,7 @@ class CommandHandler InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; - Handle* mpHandleList = nullptr; + Handle * mpHandleList = nullptr; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; From 2bf9f610945e5e8f6ac8399b8f99a3e3b8f53ebd Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Thu, 2 May 2024 11:27:41 -0400 Subject: [PATCH 12/16] Use IntrusiveList for weak references of the Handles --- src/app/CommandHandler.cpp | 30 +++++++----------------------- src/app/CommandHandler.h | 18 +++++------------- 2 files changed, 12 insertions(+), 36 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 373f9587072040..f271511bd5d4e0 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -281,37 +282,20 @@ void CommandHandler::Close() void CommandHandler::AddToHandleList(Handle * apHandle) { - apHandle->SetNext(mpHandleList); - mpHandleList = apHandle; + mpHandleList.PushBack(apHandle); } void CommandHandler::RemoveFromHandleList(Handle * apHandle) { - Handle * prev = nullptr; - for (Handle * cur = mpHandleList; cur; cur = cur->GetNext()) - { - if (cur == apHandle) - { - if (prev == nullptr) - { - mpHandleList = cur->GetNext(); - } - else - { - prev->SetNext(cur->GetNext()); - } - - cur->SetNext(nullptr); - } - prev = cur; - } + VerifyOrDie(mpHandleList.Contains(apHandle)); + mpHandleList.Remove(apHandle); } void CommandHandler::InvalidateHandles() { - for (Handle * cur = mpHandleList; cur; cur = cur->GetNext()) + for (auto handle = mpHandleList.begin(); handle != mpHandleList.end(); ++handle) { - cur->Invalidate(); + handle->Invalidate(); } } @@ -840,7 +824,7 @@ void CommandHandler::Handle::Release() if (mpHandler != nullptr) { mpHandler->DecrementHoldOff(this); - mpHandler = nullptr; + Invalidate(); } } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index cfaefceb3f33ef..5458c14dd54831 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -110,7 +111,7 @@ class CommandHandler * The Invoke Response will not be sent until all outstanding Handles have * been destroyed or have had Release called. */ - class Handle + class Handle : public IntrusiveListNodeBase<> { public: Handle() {} @@ -118,9 +119,7 @@ class CommandHandler Handle(Handle && handle) { Init(handle.mpHandler); - handle.Release(); - handle.mpHandler = nullptr; } Handle(decltype(nullptr)) {} Handle(CommandHandler * handle); @@ -132,7 +131,6 @@ class CommandHandler Init(handle.mpHandler); handle.Release(); - handle.mpHandler = nullptr; return *this; } @@ -150,19 +148,12 @@ class CommandHandler void Release(); - private: - friend CommandHandler; - /** - * Mechanism for keeping track of a chain of Handle. - */ - void SetNext(Handle * aNext) { mNext = aNext; } - Handle * GetNext() const { return mNext; } void Invalidate() { mpHandler = nullptr; } + private: void Init(CommandHandler * handler); CommandHandler * mpHandler = nullptr; - Handle * mNext = nullptr; }; // Previously we kept adding arguments with default values individually as parameters. This is because there @@ -702,7 +693,8 @@ class CommandHandler InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; - Handle * mpHandleList = nullptr; + /* List to store the weak reference to all the Handle created with this Command Handler.*/ + IntrusiveList mpHandleList; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; From aadde4875d548de50f544a396c66a74fdc4beda5 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong <82843247+yyzhong-g@users.noreply.github.com> Date: Tue, 7 May 2024 12:52:35 -0400 Subject: [PATCH 13/16] Update src/app/CommandHandler.cpp Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index f271511bd5d4e0..10691aad55614e 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -802,7 +802,7 @@ FabricIndex CommandHandler::GetAccessingFabricIndex() const return mpResponder->GetAccessingFabricIndex(); } -void CommandHandler::Handle::Init(CommandHandler * handle) +void CommandHandler::Handle::Init(CommandHandler * handler) { if (handle != nullptr) { From 8813c004647cdbd71163696cc2b5bd9c63fcbe20 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong <82843247+yyzhong-g@users.noreply.github.com> Date: Tue, 7 May 2024 12:52:54 -0400 Subject: [PATCH 14/16] Update src/app/CommandHandler.cpp Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 10691aad55614e..701a6dade3c2f5 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -828,7 +828,7 @@ void CommandHandler::Handle::Release() } } -CommandHandler::Handle::Handle(CommandHandler * handle) +CommandHandler::Handle::Handle(CommandHandler * handler) { Init(handle); } From 20271bfaef65f1dba28faa693e33fbdc7a272089 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong <82843247+yyzhong-g@users.noreply.github.com> Date: Tue, 7 May 2024 12:53:06 -0400 Subject: [PATCH 15/16] Update src/app/CommandHandler.h Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 5458c14dd54831..01c2c37a227bf2 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -693,7 +693,7 @@ class CommandHandler InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; - /* List to store the weak reference to all the Handle created with this Command Handler.*/ + /* List to store all currently-outstanding Handles for this Command Handler.*/ IntrusiveList mpHandleList; chip::System::PacketBufferTLVWriter mCommandMessageWriter; From 75de07b3229c640c8cd2635e6a997f30304923b6 Mon Sep 17 00:00:00 2001 From: Yuanyao Zhong Date: Tue, 7 May 2024 14:21:19 -0400 Subject: [PATCH 16/16] compile fix after renaming --- src/app/CommandHandler.cpp | 8 ++++---- src/app/CommandHandler.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 701a6dade3c2f5..89ff52f9669d2e 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -804,10 +804,10 @@ FabricIndex CommandHandler::GetAccessingFabricIndex() const void CommandHandler::Handle::Init(CommandHandler * handler) { - if (handle != nullptr) + if (handler != nullptr) { - handle->IncrementHoldOff(this); - mpHandler = handle; + handler->IncrementHoldOff(this); + mpHandler = handler; } } @@ -830,7 +830,7 @@ void CommandHandler::Handle::Release() CommandHandler::Handle::Handle(CommandHandler * handler) { - Init(handle); + Init(handler); } CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext() diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 01c2c37a227bf2..cce19879a6c8a9 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -122,7 +122,7 @@ class CommandHandler handle.Release(); } Handle(decltype(nullptr)) {} - Handle(CommandHandler * handle); + Handle(CommandHandler * handler); ~Handle() { Release(); } Handle & operator=(Handle && handle)