[IM] Calculate interaction model timeout accoring to RMP timeouts (#1…

…8744)

* [IM] Calculate interaction model timeout accoring to CRMP timeouts

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Add more generialized timeout calculation API

* Address comments

* Fix

Co-authored-by: Boris Zbarsky <[email protected]>

examples/lock-app/nrfconnect/main/include/CHIPProjectConfig.h

@@ -31,4 +31,4 @@
 #define CHIP_DEVICE_CONFIG_USE_TEST_SETUP_PIN_CODE 20202021
 #define CHIP_DEVICE_CONFIG_USE_TEST_SETUP_DISCRIMINATOR 0xF00
 
-#define CHIP_DEVICE_CONFIG_SED_IDLE_INTERVAL 2000_ms32
+#define CHIP_DEVICE_CONFIG_SED_IDLE_INTERVAL 20000_ms32

src/app/CommandSender.cpp

@@ -71,7 +71,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Opti
     VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY);
     VerifyOrReturnError(!mpExchangeCtx->IsGroupExchangeContext(), CHIP_ERROR_INVALID_MESSAGE_TYPE);
 
-    mpExchangeCtx->SetResponseTimeout(timeout.ValueOr(kImMessageTimeout));
+    mpExchangeCtx->SetResponseTimeout(timeout.ValueOr(session->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
 
     if (mTimedInvokeTimeoutMs.HasValue())
     {

src/app/InteractionModelTimeout.h

@@ -18,11 +18,15 @@
 #pragma once
 
 #include <system/SystemClock.h>
+#include <transport/Session.h>
+#include <transport/SessionHandle.h>
 
 namespace chip {
 namespace app {
 
-static constexpr System::Clock::Timeout kImMessageTimeout = System::Clock::Seconds16(12);
+// This is an expected upper bound of time-to-first-byte for IM transactions, the actual processing time should never exceed this
+// value unless specified elsewhere (e.g. async command handling for some clusters).
+static constexpr System::Clock::Timeout kExpectedIMProcessingTime = System::Clock::Seconds16(2);
 
 } // namespace app
 } // namespace chip

src/app/ReadClient.cpp

@@ -274,7 +274,14 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
     mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get().Value(), this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
-    mpExchangeCtx->SetResponseTimeout(aReadPrepareParams.mTimeout);
+    if (aReadPrepareParams.mTimeout == System::Clock::kZero)
+    {
+        mpExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
+    }
+    else
+    {
+        mpExchangeCtx->SetResponseTimeout(aReadPrepareParams.mTimeout);
+    }
 
     ReturnErrorOnFailure(mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReadRequest, std::move(msgBuf),
                                                     Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)));
@@ -921,7 +928,15 @@ CHIP_ERROR ReadClient::SendSubscribeRequestImpl(const ReadPrepareParams & aReadP
 
     mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get().Value(), this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY);
-    mpExchangeCtx->SetResponseTimeout(kImMessageTimeout);
+
+    if (aReadPrepareParams.mTimeout == System::Clock::kZero)
+    {
+        mpExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
+    }
+    else
+    {
+        mpExchangeCtx->SetResponseTimeout(aReadPrepareParams.mTimeout);
+    }
 
     ReturnErrorOnFailure(mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeRequest, std::move(msgBuf),
                                                     Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)));

src/app/ReadHandler.cpp

@@ -254,7 +254,8 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
     {
         MoveToState(HandlerState::AwaitingReportResponse);
     }
-    mpExchangeCtx->SetResponseTimeout(kImMessageTimeout);
+
+    mpExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
     CHIP_ERROR err =
         mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload),
                                    Messaging::SendFlags(noResponseExpected ? Messaging::SendMessageFlags::kNone

src/app/ReadPrepareParams.h

@@ -49,7 +49,9 @@ struct ReadPrepareParams
     DataVersionFilter * mpDataVersionFilterList     = nullptr;
     size_t mDataVersionFilterListSize               = 0;
     Optional<EventNumber> mEventNumber;
-    System::Clock::Timeout mTimeout          = kImMessageTimeout;
+    // The timeout for waiting for the response or System::Clock::kZero to let the interaction model decide the timeout based on the
+    // MRP timeouts of the session.
+    System::Clock::Timeout mTimeout          = System::Clock::kZero;
     uint16_t mMinIntervalFloorSeconds        = 0;
     uint16_t mMaxIntervalCeilingSeconds      = 0;
     bool mKeepSubscriptions                  = false;

src/app/StatusResponse.cpp

@@ -37,7 +37,7 @@ CHIP_ERROR StatusResponse::Send(Protocols::InteractionModel::Status aStatus, Mes
     response.Status(aStatus);
     ReturnErrorOnFailure(response.GetError());
     ReturnErrorOnFailure(writer.Finalize(&msgBuf));
-    apExchangeContext->SetResponseTimeout(kImMessageTimeout);
+    apExchangeContext->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
     ReturnErrorOnFailure(apExchangeContext->SendMessage(Protocols::InteractionModel::MsgType::StatusResponse, std::move(msgBuf),
                                                         aExpectResponse ? Messaging::SendMessageFlags::kExpectResponse
                                                                         : Messaging::SendMessageFlags::kNone));

src/app/TimedHandler.cpp

@@ -130,7 +130,8 @@ CHIP_ERROR TimedHandler::HandleTimedRequestAction(Messaging::ExchangeContext * a
     // will send nothing and the other side will have to time out to realize
     // it's missed its window).
     auto delay = System::Clock::Milliseconds32(timeoutMs);
-    aExchangeContext->SetResponseTimeout(std::max(delay, kImMessageTimeout));
+    aExchangeContext->SetResponseTimeout(
+        std::max(delay, aExchangeContext->GetSessionHandle()->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
     ReturnErrorOnFailure(StatusResponse::Send(Status::Success, aExchangeContext, /* aExpectResponse = */ true));
 
     // Now just wait for the client.

src/app/WriteClient.cpp

@@ -369,7 +369,15 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System::
     mpExchangeCtx = mpExchangeMgr->NewContext(session, this);
     VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
     VerifyOrReturnError(!(mpExchangeCtx->IsGroupExchangeContext() && mHasDataVersion), CHIP_ERROR_INVALID_MESSAGE_TYPE);
-    mpExchangeCtx->SetResponseTimeout(timeout);
+
+    if (timeout == System::Clock::kZero)
+    {
+        mpExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
+    }
+    else
+    {
+        mpExchangeCtx->SetResponseTimeout(timeout);
+    }
 
     if (mTimedWriteTimeoutMs.HasValue())
     {

src/app/WriteClient.h

@@ -214,11 +214,11 @@ class WriteClient : public Messaging::ExchangeDelegate
      *  handle calling Shutdown on itself once it decides it's done with waiting
      *  for a response (i.e. times out or gets a response). Client can specify
      *  the maximum time to wait for response (in milliseconds) via timeout parameter.
-     *  Default timeout value will be used otherwise.
+     *  If the timeout is missing or is set to System::Clock::kZero, a value based on the MRP timeouts of the session will be used.
      *  If SendWriteRequest is never called, or the call fails, the API
      *  consumer is responsible for calling Shutdown on the WriteClient.
      */
-    CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = kImMessageTimeout);
+    CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = System::Clock::kZero);
 
     /**
      *  Shutdown the WriteClient. This terminates this instance

src/app/WriteHandler.cpp

@@ -192,7 +192,7 @@ CHIP_ERROR WriteHandler::SendWriteResponse(System::PacketBufferTLVWriter && aMes
     SuccessOrExit(err);
 
     VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
-    mpExchangeCtx->SetResponseTimeout(kImMessageTimeout);
+    mpExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
     err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::WriteResponse, std::move(packet),
                                      mHasMoreChunks ? Messaging::SendMessageFlags::kExpectResponse
                                                     : Messaging::SendMessageFlags::kNone);

src/controller/tests/TestWriteChunking.cpp

@@ -579,7 +579,8 @@ void RunTest(nlTestSuite * apSuite, TestContext & ctx, Instructions instructions
     err = writeClient->SendWriteRequest(sessionHandle);
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-    ctx.GetIOContext().DriveIOUntil(System::Clock::Seconds16(15),
+    ctx.GetIOContext().DriveIOUntil(sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime) +
+                                        System::Clock::Seconds16(1),
                                     [&]() { return ctx.GetExchangeManager().GetNumActiveExchanges() == 0; });
 
     NL_TEST_ASSERT(apSuite, onGoingPath == app::ConcreteAttributePath());

src/messaging/ExchangeContext.cpp

@@ -79,6 +79,11 @@ void ExchangeContext::SetResponseExpected(bool inResponseExpected)
     mFlags.Set(Flags::kFlagResponseExpected, inResponseExpected);
 }
 
+void ExchangeContext::UseSuggestedResponseTimeout(Timeout applicationProcessingTimeout)
+{
+    SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout));
+}
+
 void ExchangeContext::SetResponseTimeout(Timeout timeout)
 {
     mResponseTimeout = timeout;

src/messaging/ExchangeContext.h

@@ -175,6 +175,15 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
     void Close();
     void Abort();
 
+    // Applies a suggested response timeout value based on the session type and the given upper layer processing time for
+    // the next message to the exchange. The exchange context must have a valid session when calling this function.
+    //
+    // This function is an equivalent of SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout))
+    void UseSuggestedResponseTimeout(Timeout applicationProcessingTimeout);
+
+    // Set the response timeout for the exchange context, regardless of the underlying session type. Using
+    // UseSuggestedResponseTimeout to set a timeout based on the type of the session and the application processing time instead of
+    // using this function is recommended.
     void SetResponseTimeout(Timeout timeout);
 
 private:

src/transport/Session.cpp

@@ -46,5 +46,14 @@ OutgoingGroupSession * Session::AsOutgoingGroupSession()
     return static_cast<OutgoingGroupSession *>(this);
 }
 
+System::Clock::Timeout Session::ComputeRoundTripTimeout(System::Clock::Timeout upperlayerProcessingTimeout)
+{
+    if (IsGroupSession())
+    {
+        return System::Clock::kZero;
+    }
+    return GetAckTimeout() + upperlayerProcessingTimeout;
+}
+
 } // namespace Transport
 } // namespace chip

src/transport/Session.h

@@ -78,6 +78,12 @@ class Session
     virtual System::Clock::Timestamp GetMRPBaseTimeout()               = 0;
     virtual System::Clock::Milliseconds32 GetAckTimeout() const        = 0;
 
+    // Returns a suggested timeout value based on the round-trip time it takes for the peer at the other end of the session to
+    // receive a message, process it and send it back. This is computed based on the session type, the type of transport, sleepy
+    // characteristics of the target and a caller-provided value for the time it takes to process a message at the upper layer on
+    // the target For group sessions, this function will always return 0.
+    System::Clock::Timeout ComputeRoundTripTimeout(System::Clock::Timeout upperlayerProcessingTimeout);
+
     FabricIndex GetFabricIndex() const { return mFabricIndex; }
 
     SecureSession * AsSecureSession();