Implement server support for timed write. (#12441)
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index a3f2c9f..6ebd370 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -349,9 +349,10 @@
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 @@
{
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 @@
}
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::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 beeca95..c235c7d 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -209,9 +209,17 @@
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 @@
/**
* 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 c29ec04..ae71dcb 100644
--- a/src/app/TimedHandler.cpp
+++ b/src/app/TimedHandler.cpp
@@ -71,14 +71,20 @@
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 2cd1bcd..ff3ffcf 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 @@
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 @@
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 @@
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 @@
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 a60f20b..b6c8386 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -29,6 +29,7 @@
#include <messaging/ExchangeMgr.h>
#include <messaging/Flags.h>
#include <protocols/Protocols.h>
+#include <protocols/interaction_model/Constants.h>
#include <system/SystemPacketBuffer.h>
namespace chip {
@@ -60,11 +61,13 @@
*
* @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 @@
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 9855a03..cf6dfd9 100644
--- a/src/app/tests/TestTimedHandler.cpp
+++ b/src/app/tests/TestTimedHandler.cpp
@@ -42,11 +42,17 @@
{
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 @@
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<TestContext *>(aContext);
@@ -130,14 +136,24 @@
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<TestContext *>(aContext);
@@ -169,13 +185,23 @@
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<TestContext *>(aContext);
diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
index 24c6ee8..5ac7c28 100644
--- a/src/app/tests/TestWriteInteraction.cpp
+++ b/src/app/tests/TestWriteInteraction.cpp
@@ -52,7 +52,8 @@
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 @@
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 @@
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::TestWriteHandler(nlTestSuite * apSuite, void * apContext)
{
+ using namespace Protocols::InteractionModel;
+
TestContext & ctx = *static_cast<TestContext *>(apContext);
- CHIP_ERROR err = CHIP_NO_ERROR;
+ constexpr bool allBooleans[] = { true, false };
+ for (auto messageIsTimed : allBooleans)
+ {
+ for (auto transactionIsTimed : allBooleans)
+ {
+ CHIP_ERROR err = CHIP_NO_ERROR;
- app::WriteHandler writeHandler;
+ app::WriteHandler writeHandler;
- chip::app::InteractionModelDelegate IMdelegate;
- System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
- err = writeHandler.Init(&IMdelegate);
+ chip::app::InteractionModelDelegate IMdelegate;
+ System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
+ err = writeHandler.Init(&IMdelegate);
- GenerateWriteRequest(apSuite, apContext, buf);
+ GenerateWriteRequest(apSuite, apContext, messageIsTimed, buf);
- TestExchangeDelegate delegate;
- Messaging::ExchangeContext * exchange = ctx.NewExchangeToBob(&delegate);
- err = writeHandler.OnWriteRequest(exchange, std::move(buf));
- NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+ 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);
+ Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
+ NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);
+ }
+ }
}
CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * aWriteHandler)
