Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subscription liveness timeout computation to include MRP backoff. #25593

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Fix subscription liveness timeout computation to include MRP backoff. #25593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/app/ReadClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <lib/core/TLVTypes.h>
#include <lib/support/FibonacciUtils.h>
#include <messaging/ReliableMessageMgr.h>
#include <messaging/ReliableMessageProtocolConfig.h>

namespace chip {
namespace app {
Expand Down Expand Up @@ -808,11 +809,17 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
// 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.
//
// Make sure to use the retransmission computation that includes backoff. For purposes of that computation, treat us as
// active now (since we are right now sending/receiving messages), and use the default "how long are we guaranteed to stay
// active" threshold for now.
//
// 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.
//
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout =
GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()).mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), Transport::kMinActiveTime);
timeout = System::Clock::Seconds16(mMaxInterval) + publisherTransmissionTimeout;
}

Expand Down
34 changes: 26 additions & 8 deletions src/controller/tests/data_model/TestRead.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,10 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback
// of reads in parallel and wait for them all to succeed.
static void SubscribeThenReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aSubscribeCount, size_t aReadCount);

// Compute the amount of time it would take a subscription with a given
// max-interval to time out.
static System::Clock::Timeout ComputeSubscriptionTimeout(System::Clock::Seconds16 aMaxInterval);

bool mEmitSubscriptionError = false;
int32_t mNumActiveSubscriptions = 0;
bool mAlterSubscriptionIntervals = false;
Expand Down Expand Up @@ -1684,7 +1688,9 @@ void TestReadInteraction::TestResubscribeAttributeTimeout(nlTestSuite * apSuite,
attributePathParams[0].mClusterId = app::Clusters::UnitTesting::Id;
attributePathParams[0].mAttributeId = app::Clusters::UnitTesting::Attributes::Boolean::Id;

readPrepareParams.mMaxIntervalCeilingSeconds = 1;
constexpr uint16_t maxIntervalCeilingSeconds = 1;

readPrepareParams.mMaxIntervalCeilingSeconds = maxIntervalCeilingSeconds;

auto err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -1701,10 +1707,9 @@ void TestReadInteraction::TestResubscribeAttributeTimeout(nlTestSuite * apSuite,
//
// Disable packet transmission, and drive IO till we have reported a re-subscription attempt.
//
// 1.5s should cover the liveness timeout in the client of 1s max interval + 50ms ACK timeout.
//
ctx.GetLoopback().mNumMessagesToDrop = chip::Test::LoopbackTransport::kUnlimitedMessageCount;
ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1500),
ctx.GetIOContext().DriveIOUntil(ComputeSubscriptionTimeout(System::Clock::Seconds16(maxIntervalCeilingSeconds)),
[&]() { return callback.mOnResubscriptionsAttempted > 0; });

NL_TEST_ASSERT(apSuite, callback.mOnResubscriptionsAttempted == 1);
Expand Down Expand Up @@ -1763,7 +1768,8 @@ void TestReadInteraction::TestSubscribeAttributeTimeout(nlTestSuite * apSuite, v
//
// Request a max interval that's very small to reduce time to discovering a liveness failure.
//
readPrepareParams.mMaxIntervalCeilingSeconds = 1;
constexpr uint16_t maxIntervalCeilingSeconds = 1;
readPrepareParams.mMaxIntervalCeilingSeconds = maxIntervalCeilingSeconds;

auto err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -1782,11 +1788,11 @@ void TestReadInteraction::TestSubscribeAttributeTimeout(nlTestSuite * apSuite, v

//
// Drive IO until we get an error on the subscription, which should be caused
// by the liveness timer firing within ~1s of the establishment of the subscription.
// by the liveness timer firing once we hit our max-interval plus
// retransmit timeouts.
//
// 1.5s should cover the liveness timeout in the client of 1s max interval + 50ms ACK timeout.
//
ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1500), [&]() { return callback.mOnError >= 1; });
ctx.GetIOContext().DriveIOUntil(ComputeSubscriptionTimeout(System::Clock::Seconds16(maxIntervalCeilingSeconds)),
[&]() { return callback.mOnError >= 1; });

NL_TEST_ASSERT(apSuite, callback.mOnError == 1);
NL_TEST_ASSERT(apSuite, callback.mLastError == CHIP_ERROR_TIMEOUT);
Expand Down Expand Up @@ -4617,6 +4623,18 @@ void TestReadInteraction::TestReadHandler_KeepSubscriptionTest(nlTestSuite * apS
ctx.DrainAndServiceIO();
}

System::Clock::Timeout TestReadInteraction::ComputeSubscriptionTimeout(System::Clock::Seconds16 aMaxInterval)
{
// Add 50ms of slack to our max interval to make sure we hit the
// subscription liveness timer.
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout =
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), Transport::kMinActiveTime);

return publisherTransmissionTimeout + aMaxInterval + System::Clock::Milliseconds32(50);
}

// clang-format off
const nlTest sTests[] =
{
Expand Down