Implement server support for timed write. (#12441)

project-chip · Dec 10, 2021 · 1051092 · 1051092
1 parent 1c3fab0
commit 1051092
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 55 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -349,25 +349,23 @@ 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");
 
     for (auto & writeHandler : mWriteHandlers)
     {
         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
@@ -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
@@ -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
@@ -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
@@ -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 @@ 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
@@ -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<TestContext *>(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<TestContext *>(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<TestContext *>(aContext);