Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
yunhanw-google committed Feb 24, 2025
1 parent bcd5183 commit 9a203b8
Show file tree
Hide file tree
Showing 23 changed files with 82 additions and 72 deletions.
1 change: 1 addition & 0 deletions src/app/CASEClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class DLL_EXPORT CASEClient
CHIP_ERROR EstablishSession(const CASEClientInitParams & params, const ScopedNodeId & peer,
const Transport::PeerAddress & peerAddress, const ReliableMessageProtocolConfig & remoteMRPConfig,
SessionEstablishmentDelegate * delegate);
bool IsPeerActive() { return mCASESession.IsPeerActive(); };

private:
CASESession mCASESession;
Expand Down
3 changes: 2 additions & 1 deletion src/app/CommandSender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ CHIP_ERROR CommandSender::SendCommandRequestInternal(const SessionHandle & sessi
mExchangeCtx.Grab(exchange);
VerifyOrReturnError(!mExchangeCtx->IsGroupExchangeContext(), CHIP_ERROR_INVALID_MESSAGE_TYPE);

mExchangeCtx->SetResponseTimeout(timeout.ValueOr(session->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
mExchangeCtx->SetResponseTimeout(timeout.ValueOr(
session->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime, mExchangeCtx.Get()->HasReceivedAtLeastOneMessage())));

if (mTimedInvokeTimeoutMs.HasValue())
{
Expand Down
7 changes: 5 additions & 2 deletions src/app/OperationalSessionSetup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,9 @@ CHIP_ERROR OperationalSessionSetup::ScheduleSessionSetupReattempt(System::Clock:
// but in practice if the other side is using its local config to
// compute Sigma2 response timeouts, then it's also returning useful
// values with BUSY, so we will wait long enough.
auto additionalTimeout = CASESession::ComputeSigma2ResponseTimeout(GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()));
bool isPeerActive = (mCASEClient != nullptr) && mCASEClient->IsPeerActive();
auto additionalTimeout =
CASESession::ComputeSigma2ResponseTimeout(GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()), isPeerActive);
actualTimerDelay += additionalTimeout;
}
timerDelay = std::chrono::duration_cast<System::Clock::Seconds16>(actualTimerDelay);
Expand Down Expand Up @@ -835,7 +837,8 @@ void OperationalSessionSetup::NotifyRetryHandlers(CHIP_ERROR error, const Reliab
{
// Compute the time we are likely to need to detect that the retry has
// failed.
System::Clock::Timeout messageTimeout = CASESession::ComputeSigma1ResponseTimeout(remoteMrpConfig);
bool isPeerActive = (mCASEClient != nullptr) && mCASEClient->IsPeerActive();
System::Clock::Timeout messageTimeout = CASESession::ComputeSigma1ResponseTimeout(remoteMrpConfig, isPeerActive);
auto timeoutSecs = std::chrono::duration_cast<System::Clock::Seconds16>(messageTimeout);
// Add 1 second in case we had fractional milliseconds in messageTimeout.
using namespace chip::System::Clock::Literals;
Expand Down
8 changes: 5 additions & 3 deletions src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -987,12 +987,14 @@ CHIP_ERROR ReadClient::ComputeLivenessCheckTimerTimeout(System::Clock::Timeout *
// So recompute the round-trip timeout directly. Assume MRP, since in practice that is likely what is happening.
auto & peerMRPConfig = mReadPrepareParams.mSessionHolder->GetRemoteMRPConfig();
// Peer will assume we are idle (hence we pass kZero to GetMessageReceiptTimeout()), but will assume we treat it as active
// for the response, so to match the retransmission timeout computation for the message back to the peeer, we should treat
// for the response, so to match the retransmission timeout computation for the message back to the peer, we should treat
// it as active.
auto roundTripTimeout = mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(System::Clock::kZero) +
auto roundTripTimeout = mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(
System::Clock::kZero, mExchange.Get()->HasReceivedAtLeastOneMessage()) +
kExpectedIMProcessingTime +
GetRetransmissionTimeout(peerMRPConfig.mActiveRetransTimeout, peerMRPConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime);
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime,
mExchange.Get()->HasReceivedAtLeastOneMessage());
*aTimeout = System::Clock::Seconds16(mMaxInterval) + roundTripTimeout;
return CHIP_NO_ERROR;
}
Expand Down
4 changes: 3 additions & 1 deletion src/app/TimedHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ CHIP_ERROR TimedHandler::HandleTimedRequestAction(Messaging::ExchangeContext * a
// it's missed its window).
auto delay = System::Clock::Milliseconds32(timeoutMs);
aExchangeContext->SetResponseTimeout(
std::max(delay, aExchangeContext->GetSessionHandle()->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
std::max(delay,
aExchangeContext->GetSessionHandle()->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime,
aExchangeContext->HasReceivedAtLeastOneMessage())));
ReturnErrorOnFailure(StatusResponse::Send(Status::Success, aExchangeContext, /* aExpectResponse = */ true));

// Now just wait for the client.
Expand Down
2 changes: 1 addition & 1 deletion src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(DeviceProxy
auto sessionHandle = device->GetSecureSession();
if (sessionHandle.HasValue())
{
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout);
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout, true /*isPeerActive*/);
}

// Enforce the spec minimal timeout. Maybe this enforcement should live in
Expand Down
5 changes: 3 additions & 2 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3735,8 +3735,9 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device,
// Try to make sure we have at least enough time for our expected
// commissioning bits plus the MRP retries for a Sigma1.
uint16_t failSafeTimeoutSecs = params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout);
auto sigma1Timeout = CASESession::ComputeSigma1ResponseTimeout(commissioneeDevice->GetPairing().GetRemoteMRPConfig());
uint16_t sigma1TimeoutSecs = std::chrono::duration_cast<System::Clock::Seconds16>(sigma1Timeout).count();
auto sigma1Timeout =
CASESession::ComputeSigma1ResponseTimeout(commissioneeDevice->GetPairing().GetRemoteMRPConfig(), true /*isPeerActive*/);
uint16_t sigma1TimeoutSecs = std::chrono::duration_cast<System::Clock::Seconds16>(sigma1Timeout).count();
if (UINT16_MAX - failSafeTimeoutSecs < sigma1TimeoutSecs)
{
failSafeTimeoutSecs = UINT16_MAX;
Expand Down
2 changes: 1 addition & 1 deletion src/controller/python/chip/utils/DeviceProxyUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ uint32_t pychip_DeviceProxy_ComputeRoundTripTimeout(DeviceProxy * device, uint32

return deviceProxy->GetSecureSession()
.Value()
->ComputeRoundTripTimeout(System::Clock::Milliseconds32(upperLayerProcessingTimeoutMs))
->ComputeRoundTripTimeout(System::Clock::Milliseconds32(upperLayerProcessingTimeoutMs), true /*isPeerActive*/)
.count();
}

Expand Down
2 changes: 1 addition & 1 deletion src/controller/tests/TestWriteChunking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ void TestWriteChunking::RunTest(Instructions instructions)
err = writeClient->SendWriteRequest(sessionHandle);
EXPECT_EQ(err, CHIP_NO_ERROR);

GetIOContext().DriveIOUntil(sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime) +
GetIOContext().DriveIOUntil(sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime, true /*isPeerActive*/) +
System::Clock::Seconds16(1),
[&]() { return GetExchangeManager().GetNumActiveExchanges() == 0; });

Expand Down
8 changes: 4 additions & 4 deletions src/controller/tests/data_model/TestRead.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4618,10 +4618,10 @@ System::Clock::Timeout TestRead::ComputeSubscriptionTimeout(System::Clock::Secon
// Add 1000ms of slack to our max interval to make sure we hit the
// subscription liveness timer. 100ms was tried in the past and is not
// sufficient: our process can easily lose the timeslice for 100ms.
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout =
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), ourMrpConfig.mActiveThresholdTime);
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout = GetRetransmissionTimeout(
ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout, System::SystemClock().GetMonotonicTimestamp(),
ourMrpConfig.mActiveThresholdTime, true /* isPeerActive */);

return publisherTransmissionTimeout + aMaxInterval + System::Clock::Milliseconds32(1000);
}
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1526,7 +1526,7 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
// Clamp to a number of seconds that will not overflow 32-bit
// int when converted to ms.
auto serverTimeoutInSeconds = System::Clock::Seconds16(serverSideProcessingTimeout.unsignedShortValue);
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds));
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds, true /*isPeerActive*/));
}
ReturnErrorOnFailure(commandSender->SendCommandRequest(session, invokeTimeout));

Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void ExchangeContext::SetResponseExpected(bool inResponseExpected)

void ExchangeContext::UseSuggestedResponseTimeout(Timeout applicationProcessingTimeout)
{
SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout));
SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout, HasReceivedAtLeastOneMessage()));
}

void ExchangeContext::SetResponseTimeout(Timeout timeout)
Expand Down
3 changes: 2 additions & 1 deletion src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
// 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))
// This function is an equivalent of SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout,
// HasReceivedAtLeastOneMessage()))
void UseSuggestedResponseTimeout(Timeout applicationProcessingTimeout);

// Set the response timeout for the exchange context, regardless of the underlying session type. Using
Expand Down
12 changes: 8 additions & 4 deletions src/messaging/ReliableMessageProtocolConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,20 +126,24 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
}

System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold)
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold,
bool isPeerActive)
{
auto timeSinceLastActivity = (System::SystemClock().GetMonotonicTimestamp() - lastActivityTime);

// Calculate the retransmission timeout and take into account that an active/idle state change can happen
// in the middle.
System::Clock::Timestamp timeout(0);
System::Clock::Milliseconds32 sitIcdSlowPollMaximum = System::Clock::Milliseconds32(15000);
for (uint8_t i = 0; i < CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1; i++)
{
auto baseInterval = activeInterval;
if (((timeSinceLastActivity + timeout) >= activityThreshold) && (idleInterval.count() <= sitIcdSlowPollMaximum.count()))
// Check if we have received at least one application-level message
// If we haven't received at least one message
// Choose active/idle timeout from PeerActiveMode of session per 4.11.2.1. Retransmissions.
// If we have received at least one message, assume peer is active and use ActiveRetransTimeout
if (!isPeerActive)
{
baseInterval = idleInterval;
baseInterval = ((timeSinceLastActivity + timeout) < activityThreshold) ? activeInterval : idleInterval;
}
timeout += Messaging::ReliableMessageMgr::GetBackoff(baseInterval, i, /* computeMaxPossible */ true);
}
Expand Down
5 changes: 3 additions & 2 deletions src/messaging/ReliableMessageProtocolConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,12 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
* @param[in] idleInterval The idle interval to use for the backoff calculation.
* @param[in] lastActivityTime The last time some activity has been recorded.
* @param[in] activityThreshold The activity threshold for a node to be considered active.
*
* @param[in] isPeerActive indicate whether exchange is active, which would determine base timeout
* @return The maximum transmission time
*/
System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold);
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold,
bool isPeerActive);

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST

Expand Down
3 changes: 2 additions & 1 deletion src/protocols/secure_channel/CASEServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const
{
// The delay should be however long we think it will take for
// that to time out.
auto sigma2Timeout = CASESession::ComputeSigma2ResponseTimeout(GetSession().GetRemoteMRPConfig());
auto sigma2Timeout = CASESession::ComputeSigma2ResponseTimeout(GetSession().GetRemoteMRPConfig(),
ec->HasReceivedAtLeastOneMessage());
if (sigma2Timeout < System::Clock::Milliseconds16::max())
{
delay = std::chrono::duration_cast<System::Clock::Milliseconds16>(sigma2Timeout);
Expand Down
16 changes: 9 additions & 7 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2611,7 +2611,7 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea

namespace {
System::Clock::Timeout ComputeRoundTripTimeout(ExchangeContext::Timeout serverProcessingTime,
const ReliableMessageProtocolConfig & remoteMrpConfig)
const ReliableMessageProtocolConfig & remoteMrpConfig, bool isPeerActive)
{
// TODO: This is duplicating logic from Session::ComputeRoundTripTimeout. Unfortunately, it's called by
// consumers who do not have a session.
Expand All @@ -2622,23 +2622,25 @@ System::Clock::Timeout ComputeRoundTripTimeout(ExchangeContext::Timeout serverPr
// Assume peer is idle, as a worst-case assumption (probably true for
// Sigma1, since that will be our initial message on the session, but less
// so for Sigma2).
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime) +
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime, isPeerActive) +
serverProcessingTime +
GetRetransmissionTimeout(localMRPConfig.mActiveRetransTimeout, localMRPConfig.mIdleRetransTimeout,
// Peer will assume we are active, since it's
// responding to our message.
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime);
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime, isPeerActive);
}
} // anonymous namespace

System::Clock::Timeout CASESession::ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
System::Clock::Timeout CASESession::ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig,
bool isPeerActive)
{
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig);
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig, isPeerActive);
}

System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig,
bool isPeerActive)
{
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig);
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig, isPeerActive);
}

bool CASESession::InvokeBackgroundWorkWatchdog()
Expand Down
7 changes: 5 additions & 2 deletions src/protocols/secure_channel/CASESession.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,16 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,

FabricIndex GetFabricIndex() const { return mFabricIndex; }

bool IsPeerActive() { return mExchangeCtxt.HasValue() && mExchangeCtxt.Value()->HasReceivedAtLeastOneMessage(); };
// Compute our Sigma1 response timeout. This can give consumers an idea of
// how long it will take to detect that our Sigma1 did not get through.
static System::Clock::Timeout ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig);
static System::Clock::Timeout ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig,
bool isPeerActive);

// Compute our Sigma2 response timeout. This can give consumers an idea of
// how long it will take to detect that our Sigma1 did not get through.
static System::Clock::Timeout ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig);
static System::Clock::Timeout ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig,
bool isPeerActive);

// TODO: remove Clear, we should create a new instance instead reset the old instance.
/** @brief This function zeroes out and resets the memory used by the object.
Expand Down
10 changes: 6 additions & 4 deletions src/transport/GroupSession.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,14 @@ class IncomingGroupSession : public Session, public ReferenceCounted<IncomingGro

System::Clock::Timestamp GetMRPBaseTimeout() const override { return System::Clock::kZero; }

System::Clock::Milliseconds32 GetAckTimeout() const override
System::Clock::Milliseconds32 GetAckTimeout(bool isPeerActive) const override
{
VerifyOrDie(false);
return System::Clock::Timeout();
}

System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
bool isPeerActive) const override
{
// There are no timeouts for group sessions.
VerifyOrDie(false);
Expand Down Expand Up @@ -128,13 +129,14 @@ class OutgoingGroupSession : public Session, public ReferenceCounted<OutgoingGro

System::Clock::Timestamp GetMRPBaseTimeout() const override { return System::Clock::kZero; }

System::Clock::Milliseconds32 GetAckTimeout() const override
System::Clock::Milliseconds32 GetAckTimeout(bool isPeerActive) const override
{
VerifyOrDie(false);
return System::Clock::Timeout();
}

System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
bool isPeerActive) const override
{
// There are no timeouts for group sessions.
VerifyOrDie(false);
Expand Down
Loading

0 comments on commit 9a203b8

Please sign in to comment.