Skip to content

Commit

Permalink
Fix ReadClient to use the right parameters when computing the liveness
Browse files Browse the repository at this point in the history
timeout

This fixes the logic in ReadClient to use the local IDLE interval when
computing the liveness timeout instead of the IDLE interval of our peer.

Testing:

Using the REPL and the liveness timer print, confirmed the fixes are
indeed applied.

Restyled by whitespace

Restyled by clang-format

Build fixes
  • Loading branch information
mrjerryjohns committed Sep 17, 2022
1 parent 989ad8e commit 54362b2
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 1 deletion.
15 changes: 14 additions & 1 deletion src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <assert.h>
#include <lib/core/CHIPTLVTypes.h>
#include <lib/support/FibonacciUtils.h>
#include <messaging/ReliableMessageMgr.h>

namespace chip {
namespace app {
Expand Down Expand Up @@ -798,7 +799,19 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
else
{
VerifyOrReturnError(mReadPrepareParams.mSessionHolder, CHIP_ERROR_INCORRECT_STATE);
timeout = System::Clock::Seconds16(mMaxInterval) + mReadPrepareParams.mSessionHolder->GetAckTimeout();

//
// To calculate the duration we're willing to wait for a report to come to us, we take into account the maximum interval of
// the subscription AND the time it takes for the report to make it to us in the worst case. This latter bit involves
// computing the Ack timeout from the publisher for the ReportData message being sent to us using our IDLE interval as the
// basis for that computation.
//
// TODO: We need to find a good home for this logic that will correctly compute this based on transport. For now, this will
// suffice since we don't use TCP as a transport currently and subscriptions over BLE aren't really a thing.
//
auto publisherTransmissionTimeout =
GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()).mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
timeout = System::Clock::Seconds16(mMaxInterval) + publisherTransmissionTimeout;
}

// EFR32/MBED/INFINION/K32W's chrono count return long unsinged, but other platform returns unsigned
Expand Down
27 changes: 27 additions & 0 deletions src/messaging/ReliableMessageProtocolConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@ namespace chip {

using namespace System::Clock::Literals;

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
static System::Clock::Milliseconds32 idleRetransTimeoutOverride = 0_ms32;
static System::Clock::Milliseconds32 activeRetransTimeoutOverride = 0_ms32;

void OverrideDefaultIdleMRPConfig(System::Clock::Milliseconds32 idleRetransTimeout)
{
idleRetransTimeoutOverride = idleRetransTimeout;
}

void OverrideDefaultActiveMRPConfig(System::Clock::Milliseconds32 activeRetransTimeout)
{
activeRetransTimeoutOverride = activeRetransTimeout;
}
#endif

ReliableMessageProtocolConfig GetDefaultMRPConfig()
{
// Default MRP intervals are defined in spec <2.11.3. Parameters and Constants>
Expand All @@ -55,6 +70,18 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
}
#endif

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
if (idleRetransTimeoutOverride != 0_ms32)
{
config.mIdleRetransTimeout = idleRetransTimeoutOverride;
}

if (activeRetransTimeoutOverride != 0_ms32)
{
config.mActiveRetransTimeout = activeRetransTimeoutOverride;
}
#endif

return (config == GetDefaultMRPConfig()) ? Optional<ReliableMessageProtocolConfig>::Missing()
: Optional<ReliableMessageProtocolConfig>::Value(config);
}
Expand Down
12 changes: 12 additions & 0 deletions src/messaging/ReliableMessageProtocolConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,18 @@ struct ReliableMessageProtocolConfig
}
};

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST

/**
* @brief
*
* Functions to override the default idle/active MRP intervals in test.
*
*/
void OverrideDefaultIdleMRPConfig(System::Clock::Milliseconds32 idleRetransTimeout);
void OverrideDefaultActiveMRPConfig(System::Clock::Milliseconds32 activeRetransTimeout);
#endif

/// @brief The default MRP config. The value is defined by spec, and shall be same for all implementations,
ReliableMessageProtocolConfig GetDefaultMRPConfig();

Expand Down
8 changes: 8 additions & 0 deletions src/messaging/tests/MessagingContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ void MessagingContext::ShutdownAndRestoreExisting(MessagingContext & existing)
existing.mTransport->SetSessionManager(&existing.GetSecureSessionManager());
}

using namespace System::Clock::Literals;

void MessagingContext::SetMRPMode(MRPMode mode)
{
if (mode == MRPMode::kDefault)
Expand All @@ -103,9 +105,15 @@ void MessagingContext::SetMRPMode(MRPMode mode)
mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());

OverrideDefaultIdleMRPConfig(0_ms32);
OverrideDefaultActiveMRPConfig(0_ms32);
}
else
{
OverrideDefaultIdleMRPConfig(10_ms32);
OverrideDefaultActiveMRPConfig(10_ms32);

mSessionBobToAlice->AsSecureSession()->SetRemoteMRPConfig(
ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(
Expand Down

0 comments on commit 54362b2

Please sign in to comment.