diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index a3f2c9f6a3f586..6ebd3707357494 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -349,9 +349,10 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte return CHIP_NO_ERROR; } -CHIP_ERROR InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, - const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload, - Protocols::InteractionModel::Status & aStatus) +Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, + const PayloadHeader & aPayloadHeader, + System::PacketBufferHandle && aPayload, + bool aIsTimedWrite) { ChipLogDetail(InteractionModel, "Received Write request"); @@ -359,15 +360,12 @@ CHIP_ERROR InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * a { if (writeHandler.IsFree()) { - ReturnErrorOnFailure(writeHandler.Init(mpDelegate)); - ReturnErrorOnFailure(writeHandler.OnWriteRequest(apExchangeContext, std::move(aPayload))); - aStatus = Protocols::InteractionModel::Status::Success; - return CHIP_NO_ERROR; + VerifyOrReturnError(writeHandler.Init(mpDelegate) == CHIP_NO_ERROR, Status::Busy); + return writeHandler.OnWriteRequest(apExchangeContext, std::move(aPayload), aIsTimedWrite); } } ChipLogProgress(InteractionModel, "no resource for write interaction"); - aStatus = Protocols::InteractionModel::Status::Busy; - return CHIP_NO_ERROR; + return Status::Busy; } CHIP_ERROR InteractionModelEngine::OnTimedRequest(Messaging::ExchangeContext * apExchangeContext, @@ -438,7 +436,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest)) { - SuccessOrExit(OnWriteRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), status)); + status = OnWriteRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedWrite = */ false); } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::SubscribeRequest)) { @@ -722,5 +720,25 @@ void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messag } } +void InteractionModelEngine::OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext, + const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) +{ + using namespace Protocols::InteractionModel; + + // Reset the ourselves as the exchange delegate for now, to match what we'd + // do with an initial unsolicited write. + apExchangeContext->SetDelegate(this); + mTimedHandlers.Deallocate(apTimedHandler); + + VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::WriteRequest)); + VerifyOrDie(!apExchangeContext->IsGroupExchangeContext()); + + Status status = OnWriteRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedWrite = */ true); + if (status != Status::Success) + { + StatusResponse::Send(status, apExchangeContext, /* aExpectResponse = */ false); + } +} + } // namespace app } // namespace chip diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index beeca955ab6485..c235c7d55c213d 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -209,9 +209,17 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman void OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload); + /** + * Called when a timed write is received. This function takes over all + * handling of the exchange, status reporting, and so forth. + */ + void OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext, + const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload); + private: friend class reporting::Engine; friend class TestCommandInteraction; + using Status = Protocols::InteractionModel::Status; void OnDone(CommandHandler & apCommandObj) override; @@ -239,11 +247,12 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman /** * Called when Interaction Model receives a Write Request message. Errors processing - * the Write Request are handled entirely within this function. The caller pre-sets status to failure and the callee is - * expected to set it to success if it does not want an automatic status response message to be sent. + * the Write Request are handled entirely within this function. If the + * status returned is not Status::Success, the caller will send a status + * response message with that status. */ - CHIP_ERROR OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, - System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus); + Status OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, + System::PacketBufferHandle && aPayload, bool aIsTimedWrite); /** * Called when Interaction Model receives a Timed Request message. Errors processing diff --git a/src/app/TimedHandler.cpp b/src/app/TimedHandler.cpp index c29ec0499bdaf6..ae71dcbc03d8ef 100644 --- a/src/app/TimedHandler.cpp +++ b/src/app/TimedHandler.cpp @@ -71,14 +71,20 @@ CHIP_ERROR TimedHandler::OnMessageReceived(Messaging::ExchangeContext * aExchang if (aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest)) { auto * imEngine = InteractionModelEngine::GetInstance(); - aExchangeContext->SetDelegate(imEngine); ChipLogDetail(DataManagement, "Handing timed invoke to IM engine: handler %p exchange " ChipLogFormatExchange, this, ChipLogValueExchange(aExchangeContext)); imEngine->OnTimedInvoke(this, aExchangeContext, aPayloadHeader, std::move(aPayload)); return CHIP_NO_ERROR; } - // TODO: Add handling of MsgType::WriteRequest here. + if (aPayloadHeader.HasMessageType(MsgType::WriteRequest)) + { + auto * imEngine = InteractionModelEngine::GetInstance(); + ChipLogDetail(DataManagement, "Handing timed write to IM engine: handler %p exchange " ChipLogFormatExchange, this, + ChipLogValueExchange(aExchangeContext)); + imEngine->OnTimedWrite(this, aExchangeContext, aPayloadHeader, std::move(aPayload)); + return CHIP_NO_ERROR; + } } // Not an expected message. Send an error response. The exchange will diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 2cd1bcd927deea..ff3ffcfcdb040d 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -26,6 +26,9 @@ namespace chip { namespace app { + +using namespace Protocols::InteractionModel; + CHIP_ERROR WriteHandler::Init(InteractionModelDelegate * apDelegate) { IgnoreUnusedVariable(apDelegate); @@ -53,23 +56,25 @@ void WriteHandler::Shutdown() ClearState(); } -CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload) +Status WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload, + bool aIsTimedWrite) { - CHIP_ERROR err = CHIP_NO_ERROR; - mpExchangeCtx = apExchangeContext; + mpExchangeCtx = apExchangeContext; - err = ProcessWriteRequest(std::move(aPayload)); - SuccessOrExit(err); + Status status = ProcessWriteRequest(std::move(aPayload), aIsTimedWrite); // Do not send response on Group Write - if (!apExchangeContext->IsGroupExchangeContext()) + if (status == Status::Success && !apExchangeContext->IsGroupExchangeContext()) { - err = SendWriteResponse(); + CHIP_ERROR err = SendWriteResponse(); + if (err != CHIP_NO_ERROR) + { + status = Status::Failure; + } } -exit: Shutdown(); - return err; + return status; } CHIP_ERROR WriteHandler::FinalizeMessage(System::PacketBufferHandle & packet) @@ -190,7 +195,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData return err; } -CHIP_ERROR WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload) +Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite) { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVReader reader; @@ -199,6 +204,14 @@ CHIP_ERROR WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayl AttributeDataIBs::Parser AttributeDataIBsParser; TLV::TLVReader AttributeDataIBsReader; bool needSuppressResponse = false; + // Default to InvalidAction for our status; that's what we want if any of + // the parsing of our overall structure or paths fails. Once we have a + // successfully parsed path, the only way we will get a failure return is if + // our path handling fails to AddStatus on us. + // + // TODO: That's not technically InvalidAction, and we should probably make + // our callees hand out Status as well. + Status status = Status::InvalidAction; reader.Init(std::move(aPayload)); @@ -228,11 +241,23 @@ CHIP_ERROR WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayl err = writeRequestParser.GetWriteRequests(&AttributeDataIBsParser); SuccessOrExit(err); + if (mIsTimedRequest != aIsTimedWrite) + { + // The message thinks it should be part of a timed interaction but it's + // not, or vice versa. Spec says to Respond with UNSUPPORTED_ACCESS. + status = Status::UnsupportedAccess; + goto exit; + } + AttributeDataIBsParser.GetReader(&AttributeDataIBsReader); err = ProcessAttributeDataIBs(AttributeDataIBsReader); + if (err == CHIP_NO_ERROR) + { + status = Status::Success; + } exit: - return err; + return status; } CHIP_ERROR WriteHandler::AddStatus(const AttributePathParams & aAttributePathParams, diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index a60f20b54340fd..b6c838641f8b87 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -29,6 +29,7 @@ #include #include #include +#include #include namespace chip { @@ -60,11 +61,13 @@ class WriteHandler * * @param[in] apExchangeContext A pointer to the ExchangeContext. * @param[in] aPayload A payload that has read request data + * @param[in] aIsTimedWrite Whether write is part of a timed interaction. * - * @retval #Others If fails to process read request - * @retval #CHIP_NO_ERROR On success. + * @retval Status. Callers are expected to send a status response if the + * return status is not Status::Success. */ - CHIP_ERROR OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload); + Protocols::InteractionModel::Status OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, + System::PacketBufferHandle && aPayload, bool aIsTimedWrite); bool IsFree() const { return mState == State::Uninitialized; } @@ -99,7 +102,7 @@ class WriteHandler AddStatus, // The handler has added status code Sending, // The handler has sent out the write response }; - CHIP_ERROR ProcessWriteRequest(System::PacketBufferHandle && aPayload); + Protocols::InteractionModel::Status ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite); CHIP_ERROR FinalizeMessage(System::PacketBufferHandle & packet); CHIP_ERROR SendWriteResponse(); diff --git a/src/app/tests/TestTimedHandler.cpp b/src/app/tests/TestTimedHandler.cpp index 9855a036991c60..cf6dfd9c775945 100644 --- a/src/app/tests/TestTimedHandler.cpp +++ b/src/app/tests/TestTimedHandler.cpp @@ -42,11 +42,17 @@ class TestTimedHandler { public: static void TestInvokeFastEnough(nlTestSuite * aSuite, void * aContext); + static void TestWriteFastEnough(nlTestSuite * aSuite, void * aContext); + static void TestInvokeTooSlow(nlTestSuite * aSuite, void * aContext); + static void TestWriteTooSlow(nlTestSuite * aSuite, void * aContext); static void TestInvokeNeverComes(nlTestSuite * aSuite, void * aContext); private: + static void TestFollowingMessageFastEnough(nlTestSuite * aSuite, void * aContext, MsgType aMsgType); + static void TestFollowingMessageTooSlow(nlTestSuite * aSuite, void * aContext, MsgType aMsgType); + static void GenerateTimedRequest(nlTestSuite * aSuite, uint16_t aTimeoutValue, System::PacketBufferHandle & aPayload); }; @@ -101,7 +107,7 @@ void TestTimedHandler::GenerateTimedRequest(nlTestSuite * aSuite, uint16_t aTime NL_TEST_ASSERT(aSuite, err == CHIP_NO_ERROR); } -void TestTimedHandler::TestInvokeFastEnough(nlTestSuite * aSuite, void * aContext) +void TestTimedHandler::TestFollowingMessageFastEnough(nlTestSuite * aSuite, void * aContext, MsgType aMsgType) { TestContext & ctx = *static_cast(aContext); @@ -130,14 +136,24 @@ void TestTimedHandler::TestInvokeFastEnough(nlTestSuite * aSuite, void * aContex delegate.mKeepExchangeOpen = false; delegate.mNewMessageReceived = false; - err = exchange->SendMessage(MsgType::InvokeCommandRequest, std::move(payload)); + err = exchange->SendMessage(aMsgType, std::move(payload)); NL_TEST_ASSERT(aSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(aSuite, delegate.mNewMessageReceived); NL_TEST_ASSERT(aSuite, delegate.mLastMessageWasStatus); NL_TEST_ASSERT(aSuite, delegate.mStatus.mStatus != Status::UnsupportedAccess); } -void TestTimedHandler::TestInvokeTooSlow(nlTestSuite * aSuite, void * aContext) +void TestTimedHandler::TestInvokeFastEnough(nlTestSuite * aSuite, void * aContext) +{ + TestFollowingMessageFastEnough(aSuite, aContext, MsgType::InvokeCommandRequest); +} + +void TestTimedHandler::TestWriteFastEnough(nlTestSuite * aSuite, void * aContext) +{ + TestFollowingMessageFastEnough(aSuite, aContext, MsgType::WriteRequest); +} + +void TestTimedHandler::TestFollowingMessageTooSlow(nlTestSuite * aSuite, void * aContext, MsgType aMsgType) { TestContext & ctx = *static_cast(aContext); @@ -169,13 +185,23 @@ void TestTimedHandler::TestInvokeTooSlow(nlTestSuite * aSuite, void * aContext) delegate.mKeepExchangeOpen = false; delegate.mNewMessageReceived = false; - err = exchange->SendMessage(MsgType::InvokeCommandRequest, std::move(payload)); + err = exchange->SendMessage(aMsgType, std::move(payload)); NL_TEST_ASSERT(aSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(aSuite, delegate.mNewMessageReceived); NL_TEST_ASSERT(aSuite, delegate.mLastMessageWasStatus); NL_TEST_ASSERT(aSuite, delegate.mStatus.mStatus == Status::UnsupportedAccess); } +void TestTimedHandler::TestInvokeTooSlow(nlTestSuite * aSuite, void * aContext) +{ + TestFollowingMessageTooSlow(aSuite, aContext, MsgType::InvokeCommandRequest); +} + +void TestTimedHandler::TestWriteTooSlow(nlTestSuite * aSuite, void * aContext) +{ + TestFollowingMessageTooSlow(aSuite, aContext, MsgType::WriteRequest); +} + void TestTimedHandler::TestInvokeNeverComes(nlTestSuite * aSuite, void * aContext) { TestContext & ctx = *static_cast(aContext); diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 24c6ee82efb557..5ac7c28b7ad9f8 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -52,7 +52,8 @@ class TestWriteInteraction private: static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClientHandle & aWriteClient); static void AddAttributeStatus(nlTestSuite * apSuite, void * apContext, WriteHandler & aWriteHandler); - static void GenerateWriteRequest(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload); + static void GenerateWriteRequest(nlTestSuite * apSuite, void * apContext, bool aIsTimedWrite, + System::PacketBufferHandle & aPayload); static void GenerateWriteResponse(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload); }; @@ -115,7 +116,8 @@ void TestWriteInteraction::AddAttributeStatus(nlTestSuite * apSuite, void * apCo NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } -void TestWriteInteraction::GenerateWriteRequest(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload) +void TestWriteInteraction::GenerateWriteRequest(nlTestSuite * apSuite, void * apContext, bool aIsTimedWrite, + System::PacketBufferHandle & aPayload) { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVWriter writer; @@ -155,7 +157,7 @@ void TestWriteInteraction::GenerateWriteRequest(nlTestSuite * apSuite, void * ap attributeDataIBsBuilder.EndOfAttributeDataIBs(); NL_TEST_ASSERT(apSuite, attributeDataIBsBuilder.GetError() == CHIP_NO_ERROR); - writeRequestBuilder.TimedRequest(false).IsFabricFiltered(false).EndOfWriteRequestMessage(); + writeRequestBuilder.TimedRequest(aIsTimedWrite).IsFabricFiltered(false).EndOfWriteRequestMessage(); NL_TEST_ASSERT(apSuite, writeRequestBuilder.GetError() == CHIP_NO_ERROR); err = writer.Finalize(&aPayload); @@ -263,25 +265,48 @@ void TestWriteInteraction::TestWriteClientGroup(nlTestSuite * apSuite, void * ap void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - - CHIP_ERROR err = CHIP_NO_ERROR; - - app::WriteHandler writeHandler; - - chip::app::InteractionModelDelegate IMdelegate; - System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - err = writeHandler.Init(&IMdelegate); - - GenerateWriteRequest(apSuite, apContext, buf); + using namespace Protocols::InteractionModel; - TestExchangeDelegate delegate; - Messaging::ExchangeContext * exchange = ctx.NewExchangeToBob(&delegate); - err = writeHandler.OnWriteRequest(exchange, std::move(buf)); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + TestContext & ctx = *static_cast(apContext); - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + constexpr bool allBooleans[] = { true, false }; + for (auto messageIsTimed : allBooleans) + { + for (auto transactionIsTimed : allBooleans) + { + CHIP_ERROR err = CHIP_NO_ERROR; + + app::WriteHandler writeHandler; + + chip::app::InteractionModelDelegate IMdelegate; + System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); + err = writeHandler.Init(&IMdelegate); + + GenerateWriteRequest(apSuite, apContext, messageIsTimed, buf); + + TestExchangeDelegate delegate; + Messaging::ExchangeContext * exchange = ctx.NewExchangeToBob(&delegate); + Status status = writeHandler.OnWriteRequest(exchange, std::move(buf), transactionIsTimed); + if (messageIsTimed == transactionIsTimed) + { + NL_TEST_ASSERT(apSuite, status == Status::Success); + } + else + { + NL_TEST_ASSERT(apSuite, status == Status::UnsupportedAccess); + // In the normal code flow, the exchange would now get closed + // when we send the error status on it (of if that fails when + // the stack unwinds). In the success case it's been closed + // already by the WriteHandler sending the response on it, but + // if we are in the error case we need to make sure it gets + // closed. + exchange->Close(); + } + + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + } + } } CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * aWriteHandler)