diff --git a/scripts/tests/cirque_tests.sh b/scripts/tests/cirque_tests.sh index 74dab9748decfe..525e472300fb18 100755 --- a/scripts/tests/cirque_tests.sh +++ b/scripts/tests/cirque_tests.sh @@ -36,7 +36,7 @@ OT_SIMULATION_CACHE="$CIRQUE_CACHE_PATH/ot-simulation.tgz" CIRQUE_TESTS=( "EchoTest" "MobileDeviceTest" - "InteractionModel" + "InteractionModelTest" ) BOLD_GREEN_TEXT="\033[1;32m" diff --git a/src/app/Command.cpp b/src/app/Command.cpp index a026b4cd06b0cc..3259a36484bcd5 100644 --- a/src/app/Command.cpp +++ b/src/app/Command.cpp @@ -37,7 +37,6 @@ CHIP_ERROR Command::Init(Messaging::ExchangeManager * apExchangeMgr, Interaction // Error if already initialized. VerifyOrExit(apExchangeMgr != nullptr, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INCORRECT_STATE); - VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE); mpExchangeMgr = apExchangeMgr; mpDelegate = apDelegate; @@ -53,7 +52,7 @@ CHIP_ERROR Command::Reset() { CHIP_ERROR err = CHIP_NO_ERROR; CommandList::Builder commandListBuilder; - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); if (mCommandMessageBuf.IsNull()) { @@ -133,7 +132,7 @@ void Command::Shutdown() mCommandMessageWriter.Reset(); mCommandMessageBuf = nullptr; - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); mpExchangeMgr = nullptr; mpDelegate = nullptr; @@ -213,7 +212,7 @@ CHIP_ERROR Command::ConstructCommandPath(const CommandPathParams & aCommandPathP return commandPath.GetError(); } -CHIP_ERROR Command::ClearExistingExchangeContext() +CHIP_ERROR Command::AbortExistingExchangeContext() { // Discard any existing exchange context. Effectively we can only have one Echo exchange with // a single node at any one time. diff --git a/src/app/Command.h b/src/app/Command.h index 0758fe2f4c469c..255e0bb1012eb2 100644 --- a/src/app/Command.h +++ b/src/app/Command.h @@ -119,7 +119,7 @@ class Command virtual CHIP_ERROR ProcessCommandDataElement(CommandDataElement::Parser & aCommandElement) = 0; protected: - CHIP_ERROR ClearExistingExchangeContext(); + CHIP_ERROR AbortExistingExchangeContext(); void MoveToState(const CommandState aTargetState); CHIP_ERROR ProcessCommandMessage(System::PacketBufferHandle && payload, CommandRoleId aCommandRoleId); CHIP_ERROR ConstructCommandPath(const CommandPathParams & aCommandPathParams, CommandDataElement::Builder aCommandDataElement); diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 4a87e0fa7bc103..095d48a7d913d5 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -41,7 +41,9 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, Transport::AdminId err = FinalizeCommandsMessage(); SuccessOrExit(err); - ClearExistingExchangeContext(); + // Discard any existing exchange context. Effectively we can only have one exchange per CommandSender + // at any one time. + AbortExistingExchangeContext(); // Create a new exchange context. // TODO: temprary create a SecureSessionHandle from node id, will be fix in PR 3602 @@ -59,7 +61,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, Transport::AdminId exit: if (err != CHIP_NO_ERROR) { - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); } ChipLogFunctError(err); @@ -70,27 +72,23 @@ void CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExchangeCon const PayloadHeader & aPayloadHeader, System::PacketBufferHandle aPayload) { CHIP_ERROR err = CHIP_NO_ERROR; - // Assert that the exchange context matches the client's current context. - // This should never fail because even if SendCommandRequest is called - // back-to-back, the second call will call Close() on the first exchange, - // which clears the OnMessageReceived callback. - VerifyOrDie(apExchangeContext == mpExchangeCtx); - - // Verify that the message is an Invoke Command Response. - // If not, close the exchange and free the payload. - if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse)) - { - apExchangeContext->Close(); - mpExchangeCtx = nullptr; - goto exit; - } + VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE); + VerifyOrExit(aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse), + err = CHIP_ERROR_INVALID_MESSAGE_TYPE); err = ProcessCommandMessage(std::move(aPayload), CommandRoleId::SenderId); SuccessOrExit(err); exit: + ChipLogFunctError(err); + + // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received. + // This needs to be done before the Reset() call, because Reset() aborts mpExchangeCtx if its not null. + mpExchangeCtx->Close(); + mpExchangeCtx = nullptr; Reset(); + if (mpDelegate != nullptr) { if (err != CHIP_NO_ERROR) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 3ff2d1c01226c9..bb2ded857eeac6 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -34,13 +34,13 @@ CHIP_ERROR ReadClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interact // Error if already initialized. VerifyOrExit(apExchangeMgr != nullptr, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INCORRECT_STATE); - VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE); mpExchangeMgr = apExchangeMgr; - mpExchangeCtx = nullptr; mpDelegate = apDelegate; mState = ClientState::Initialized; + AbortExistingExchangeContext(); + exit: ChipLogFunctError(err); return err; @@ -48,7 +48,7 @@ CHIP_ERROR ReadClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interact void ReadClient::Shutdown() { - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); mpExchangeMgr = nullptr; mpDelegate = nullptr; MoveToState(ClientState::Uninitialized); @@ -88,7 +88,10 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin InteractionModelEngine::GetInstance()->GetReadClientArrayIndex(this), GetStateStr()); VerifyOrExit(ClientState::Initialized == mState, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(mpDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE); - VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE); + + // Discard any existing exchange context. Effectively we can only have one exchange per ReadClient + // at any one time. + AbortExistingExchangeContext(); { System::PacketBufferTLVWriter writer; @@ -175,6 +178,12 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin exit: ChipLogFunctError(err); + + if (err != CHIP_NO_ERROR) + { + AbortExistingExchangeContext(); + } + return err; } @@ -182,16 +191,20 @@ void ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContex const PayloadHeader & aPayloadHeader, System::PacketBufferHandle aPayload) { CHIP_ERROR err = CHIP_NO_ERROR; + + VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData), err = CHIP_ERROR_INVALID_MESSAGE_TYPE); - VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE); err = ProcessReportData(std::move(aPayload)); exit: ChipLogFunctError(err); - ClearExistingExchangeContext(); + // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received. + mpExchangeCtx->Close(); + mpExchangeCtx = nullptr; MoveToState(ClientState::Initialized); + if (mpDelegate != nullptr) { if (err != CHIP_NO_ERROR) @@ -207,7 +220,7 @@ void ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContex return; } -CHIP_ERROR ReadClient::ClearExistingExchangeContext() +CHIP_ERROR ReadClient::AbortExistingExchangeContext() { if (mpExchangeCtx != nullptr) { @@ -302,7 +315,7 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex { ChipLogProgress(DataManagement, "Time out! failed to receive report data from Exchange: %d", apExchangeContext->GetExchangeId()); - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); MoveToState(ClientState::Initialized); if (nullptr != mpDelegate) { diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 80234e328cd18c..650eac090b2b20 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -115,7 +115,7 @@ class ReadClient : public Messaging::ExchangeDelegate void MoveToState(const ClientState aTargetState); CHIP_ERROR ProcessReportData(System::PacketBufferHandle aPayload); - CHIP_ERROR ClearExistingExchangeContext(); + CHIP_ERROR AbortExistingExchangeContext(); const char * GetStateStr() const; Messaging::ExchangeManager * mpExchangeMgr = nullptr; diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 1250d701a884c3..3694a9e3647ff2 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -52,7 +52,7 @@ void ReadHandler::Shutdown() { InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpAttributeClusterInfoList); InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpEventClusterInfoList); - ClearExistingExchangeContext(); + AbortExistingExchangeContext(); MoveToState(HandlerState::Uninitialized); mpDelegate = nullptr; mpAttributeClusterInfoList = nullptr; @@ -60,7 +60,7 @@ void ReadHandler::Shutdown() mCurrentPriority = PriorityLevel::Invalid; } -CHIP_ERROR ReadHandler::ClearExistingExchangeContext() +CHIP_ERROR ReadHandler::AbortExistingExchangeContext() { if (mpExchangeCtx != nullptr) { diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index df8d8154e0c9a9..1833148c19fe12 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -128,7 +128,7 @@ class ReadHandler void MoveToState(const HandlerState aTargetState); const char * GetStateStr() const; - CHIP_ERROR ClearExistingExchangeContext(); + CHIP_ERROR AbortExistingExchangeContext(); Messaging::ExchangeContext * mpExchangeCtx = nullptr; InteractionModelDelegate * mpDelegate = nullptr;