From 15940442421615305d7a2a03badb3fc2a7ab7b67 Mon Sep 17 00:00:00 2001 From: Song GUO Date: Thu, 2 Jun 2022 15:34:41 +0800 Subject: [PATCH] [IM] Calculate interaction model timeout accoring to RMP timeouts (#18744) * [IM] Calculate interaction model timeout accoring to CRMP timeouts * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Add more generialized timeout calculation API * Address comments * Fix Co-authored-by: Boris Zbarsky --- .../main/include/CHIPProjectConfig.h | 2 +- src/app/CommandSender.cpp | 2 +- src/app/InteractionModelTimeout.h | 6 +++++- src/app/ReadClient.cpp | 19 +++++++++++++++++-- src/app/ReadHandler.cpp | 3 ++- src/app/ReadPrepareParams.h | 4 +++- src/app/StatusResponse.cpp | 2 +- src/app/TimedHandler.cpp | 3 ++- src/app/WriteClient.cpp | 10 +++++++++- src/app/WriteClient.h | 4 ++-- src/app/WriteHandler.cpp | 2 +- src/controller/tests/TestWriteChunking.cpp | 3 ++- src/messaging/ExchangeContext.cpp | 5 +++++ src/messaging/ExchangeContext.h | 9 +++++++++ src/transport/Session.cpp | 9 +++++++++ src/transport/Session.h | 6 ++++++ 16 files changed, 75 insertions(+), 14 deletions(-) diff --git a/examples/lock-app/nrfconnect/main/include/CHIPProjectConfig.h b/examples/lock-app/nrfconnect/main/include/CHIPProjectConfig.h index 3a6d7ab9f27f28..bfa54151bad46d 100644 --- a/examples/lock-app/nrfconnect/main/include/CHIPProjectConfig.h +++ b/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 diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 53cb5e8b1daab4..1e04fb58bc75db 100644 --- a/src/app/CommandSender.cpp +++ b/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()) { diff --git a/src/app/InteractionModelTimeout.h b/src/app/InteractionModelTimeout.h index d903c7ef37fa65..d2b1e2b567a554 100644 --- a/src/app/InteractionModelTimeout.h +++ b/src/app/InteractionModelTimeout.h @@ -18,11 +18,15 @@ #pragma once #include +#include +#include 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 diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index d58e41aca2f0e0..0d52d5649fbdcc 100644 --- a/src/app/ReadClient.cpp +++ b/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))); diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 8b722edbcfb83f..47877edff568c5 100644 --- a/src/app/ReadHandler.cpp +++ b/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 diff --git a/src/app/ReadPrepareParams.h b/src/app/ReadPrepareParams.h index 0633f870b0d4aa..6d95b8b0b48a17 100644 --- a/src/app/ReadPrepareParams.h +++ b/src/app/ReadPrepareParams.h @@ -49,7 +49,9 @@ struct ReadPrepareParams DataVersionFilter * mpDataVersionFilterList = nullptr; size_t mDataVersionFilterListSize = 0; Optional 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; diff --git a/src/app/StatusResponse.cpp b/src/app/StatusResponse.cpp index 19042ce06762c3..e9e8af70a5f656 100644 --- a/src/app/StatusResponse.cpp +++ b/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)); diff --git a/src/app/TimedHandler.cpp b/src/app/TimedHandler.cpp index 13ef75ca4eabaf..b62d835f2ace8e 100644 --- a/src/app/TimedHandler.cpp +++ b/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. diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index dfcd160bc45172..f5f84e847f824a 100644 --- a/src/app/WriteClient.cpp +++ b/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()) { diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index 7360d4566f23eb..5e8d4660a4558f 100644 --- a/src/app/WriteClient.h +++ b/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 diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index e00ea35fa81dcf..ba52461882abe9 100644 --- a/src/app/WriteHandler.cpp +++ b/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); diff --git a/src/controller/tests/TestWriteChunking.cpp b/src/controller/tests/TestWriteChunking.cpp index 6917aa0d17a38e..89b0c1379141d6 100644 --- a/src/controller/tests/TestWriteChunking.cpp +++ b/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()); diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 507f2711b768fd..ce43b77dc599b9 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/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; diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index bd45b65df86d3b..d1a8c943644561 100644 --- a/src/messaging/ExchangeContext.h +++ b/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: diff --git a/src/transport/Session.cpp b/src/transport/Session.cpp index 3e3434ea558d5b..50f39f85e594f3 100644 --- a/src/transport/Session.cpp +++ b/src/transport/Session.cpp @@ -46,5 +46,14 @@ OutgoingGroupSession * Session::AsOutgoingGroupSession() return static_cast(this); } +System::Clock::Timeout Session::ComputeRoundTripTimeout(System::Clock::Timeout upperlayerProcessingTimeout) +{ + if (IsGroupSession()) + { + return System::Clock::kZero; + } + return GetAckTimeout() + upperlayerProcessingTimeout; +} + } // namespace Transport } // namespace chip diff --git a/src/transport/Session.h b/src/transport/Session.h index d1eb81d03c6e7e..01eec0c8083676 100644 --- a/src/transport/Session.h +++ b/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();