From ea05f9e595eb29d53d33ba48691c9daa63f1c76b Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Sun, 6 Sep 2026 15:01:57 -0700 Subject: [PATCH] [mac] move CSL period and timeout constants from radio (#13572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit moves `kMinCslPeriod` from `Radio` to `Mac` (`mac_types.hpp`) and `kMaxCslTimeout` to `Mle` (`mle.hpp`), grouping them logically within their respective layers. Key changes: - Adds `Time::MsecToUsec()` helper function. - Tightens constant types to native representations: `uint16_t` for `kMinCslPeriod` (units of 10 symbols) and `uint32_t` for `kMaxCslTimeout` (units of seconds), avoiding narrowing conversions across MAC and MLE call sites. - Bug fix in CSL IE validation: Previously, `Mac::ProcessCsl()` checked `csl->GetPeriod() >= kMinCslIePeriod`, where `kMinCslIePeriod` was defined directly as `OPENTHREAD_CONFIG_MAC_CSL_MIN_PERIOD` (in milliseconds, default 10). Because the CSL IE period field is in units of 10 symbols (160 µs), comparing directly against 10 meant it was mistakenly validating `>= 1.6 ms` instead of `>= 10 ms`. Replacing `kMinCslIePeriod` with `kMinCslPeriod` (which properly converts the configured millisecond period to 10-symbol units) fixes this unit mismatch bug. - Updates `Mle::SetCslTimeout()` to validate the timeout against `kMaxCslTimeout` and return `Error`, simplifying `otLinkSetCslTimeout()`. --- src/core/api/link_api.cpp | 10 ++-------- src/core/common/time.hpp | 15 ++++++++++++--- src/core/mac/mac.cpp | 2 +- src/core/mac/mac.hpp | 2 -- src/core/mac/mac_types.hpp | 13 +++++++++++++ src/core/radio/radio.hpp | 8 -------- src/core/thread/mle.cpp | 17 +++++++++++------ src/core/thread/mle.hpp | 8 +++++++- tests/nexus/test_1_2_LP_5_3_2.cpp | 6 +++--- tests/nexus/test_1_2_LP_5_3_3.cpp | 2 +- tests/nexus/test_1_2_LP_5_3_4.cpp | 2 +- tests/nexus/test_1_2_LP_5_3_5.cpp | 4 ++-- tests/nexus/test_1_2_LP_5_3_6.cpp | 2 +- tests/nexus/test_1_2_LP_5_3_7.cpp | 6 +++--- tests/nexus/test_1_2_LP_5_3_8.cpp | 2 +- tests/nexus/test_1_2_LP_7_2_2.cpp | 6 +++--- 16 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/core/api/link_api.cpp b/src/core/api/link_api.cpp index 7188d6d40..3b3af7326 100644 --- a/src/core/api/link_api.cpp +++ b/src/core/api/link_api.cpp @@ -455,7 +455,7 @@ otError otLinkSetCslPeriod(otInstance *aInstance, uint32_t aPeriod) { VerifyOrExit((aPeriod % Radio::kTenSymbolsDuration) == 0, error = kErrorInvalidArgs); periodInTenSymbolsUnit = ClampToUint16(aPeriod / Radio::kTenSymbolsDuration); - VerifyOrExit(periodInTenSymbolsUnit >= Radio::kMinCslPeriod, error = kErrorInvalidArgs); + VerifyOrExit(periodInTenSymbolsUnit >= Mac::kMinCslPeriod, error = kErrorInvalidArgs); } AsCoreType(aInstance).Get().SetCslPeriod(periodInTenSymbolsUnit); @@ -468,13 +468,7 @@ uint32_t otLinkGetCslTimeout(otInstance *aInstance) { return AsCoreType(aInstanc otError otLinkSetCslTimeout(otInstance *aInstance, uint32_t aTimeout) { - Error error = kErrorNone; - - VerifyOrExit(Radio::kMaxCslTimeout >= aTimeout, error = kErrorInvalidArgs); - AsCoreType(aInstance).Get().SetCslTimeout(aTimeout); - -exit: - return error; + return AsCoreType(aInstance).Get().SetCslTimeout(aTimeout); } #endif // OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE diff --git a/src/core/common/time.hpp b/src/core/common/time.hpp index 433d07105..0475bcc68 100644 --- a/src/core/common/time.hpp +++ b/src/core/common/time.hpp @@ -247,7 +247,7 @@ public: * * @returns The number of milliseconds. */ - static uint32_t constexpr SecToMsec(uint32_t aSeconds) { return aSeconds * 1000u; } + static uint32_t constexpr SecToMsec(uint32_t aSeconds) { return aSeconds * kOneSecondInMsec; } /** * Converts a given number of milliseconds to seconds. @@ -256,7 +256,16 @@ public: * * @returns The number of seconds. */ - static uint32_t constexpr MsecToSec(uint32_t aMilliseconds) { return aMilliseconds / 1000u; } + static uint32_t constexpr MsecToSec(uint32_t aMilliseconds) { return aMilliseconds / kOneSecondInMsec; } + + /** + * Converts a given number of milliseconds to microseconds. + * + * @param[in] aMilliseconds The milliseconds value to convert to microseconds. + * + * @returns The number of microseconds. + */ + static uint32_t constexpr MsecToUsec(uint32_t aMilliseconds) { return aMilliseconds * kOneMsecInUsec; } /** * Converts a given number of microseconds to milliseconds. @@ -265,7 +274,7 @@ public: * * @returns The number of milliseconds. */ - static uint32_t constexpr UsecToMsec(uint32_t aMicroseconds) { return aMicroseconds / 1000u; } + static uint32_t constexpr UsecToMsec(uint32_t aMicroseconds) { return aMicroseconds / kOneMsecInUsec; } private: static constexpr uint32_t kDistantInterval = (1UL << 31) - 1; diff --git a/src/core/mac/mac.cpp b/src/core/mac/mac.cpp index 2e9135f70..a75feaac1 100644 --- a/src/core/mac/mac.cpp +++ b/src/core/mac/mac.cpp @@ -2568,7 +2568,7 @@ void Mac::ProcessCsl(const RxFrame::ParseInfo &aFrameInfo, const Address &aSrcAd VerifyOrExit(neighbor != nullptr); - VerifyOrExit(csl->GetPeriod() >= kMinCslIePeriod); + VerifyOrExit(csl->GetPeriod() >= kMinCslPeriod); neighbor->SetCslPeriod(csl->GetPeriod()); neighbor->SetCslPhase(csl->GetPhase()); diff --git a/src/core/mac/mac.hpp b/src/core/mac/mac.hpp index d539badcf..48a1bcff1 100644 --- a/src/core/mac/mac.hpp +++ b/src/core/mac/mac.hpp @@ -699,8 +699,6 @@ private: static constexpr uint8_t kMaxFrameRetriesCsl = 0; static constexpr uint8_t kTxNumBcast = OPENTHREAD_CONFIG_MAC_TX_NUM_BCAST; - static constexpr uint16_t kMinCslIePeriod = OPENTHREAD_CONFIG_MAC_CSL_MIN_PERIOD; - static constexpr uint32_t kDefaultWedListenInterval = 1000000; static constexpr uint32_t kDefaultWedListenDuration = 8000; diff --git a/src/core/mac/mac_types.hpp b/src/core/mac/mac_types.hpp index 97bcfd6e3..821efdd25 100644 --- a/src/core/mac/mac_types.hpp +++ b/src/core/mac/mac_types.hpp @@ -48,7 +48,9 @@ #include "common/data.hpp" #include "common/equatable.hpp" #include "common/non_copyable.hpp" +#include "common/numeric_limits.hpp" #include "common/string.hpp" +#include "common/time.hpp" #include "crypto/storage.hpp" #include "radio/radio_types.hpp" @@ -868,6 +870,17 @@ private: #endif }; +/** + * Minimum CSL period supported in units of 10 symbols. + */ +constexpr uint16_t kMinCslPeriod = + static_cast(Time::MsecToUsec(OPENTHREAD_CONFIG_MAC_CSL_MIN_PERIOD) / Radio::kTenSymbolsDuration); + +static_assert(kMinCslPeriod > 0, "kMinCslPeriod must be greater than zero"); +static_assert((Time::MsecToUsec(OPENTHREAD_CONFIG_MAC_CSL_MIN_PERIOD) / Radio::kTenSymbolsDuration) <= + NumericLimits::kMax, + "kMinCslPeriod is too large to fit in uint16_t"); + /** * Converts a given CSL period in units of 10 symbols to microseconds. * diff --git a/src/core/radio/radio.hpp b/src/core/radio/radio.hpp index 75880c576..10b2f6779 100644 --- a/src/core/radio/radio.hpp +++ b/src/core/radio/radio.hpp @@ -71,14 +71,6 @@ constexpr uint32_t kHeaderShrDuration = 160; ///< Duration of SHR in us constexpr uint32_t kHeaderPhrDuration = 32; ///< Duration of PHR in us constexpr uint32_t kOctetDuration = 32; ///< Duration of one octet in us -#if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE -/** - * Minimum CSL period supported in units of 10 symbols. - */ -constexpr uint64_t kMinCslPeriod = OPENTHREAD_CONFIG_MAC_CSL_MIN_PERIOD * 1000 / kTenSymbolsDuration; -constexpr uint64_t kMaxCslTimeout = OPENTHREAD_CONFIG_MAC_CSL_MAX_TIMEOUT; -#endif - #if OPENTHREAD_CONFIG_TD_WAKE_LISTENER_ENABLE /** * Minimum wake-up listen duration supported in microseconds. diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index f52153add..7f05cd6c7 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -956,10 +956,14 @@ exit: } #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE -void Mle::SetCslTimeout(uint32_t aTimeout) -{ - VerifyOrExit(mCslTimeout != aTimeout); +Error Mle::SetCslTimeout(uint32_t aTimeout) +{ + Error error = kErrorNone; + + VerifyOrExit(aTimeout <= kMaxCslTimeout, error = kErrorInvalidArgs); + + VerifyOrExit(mCslTimeout != aTimeout); mCslTimeout = aTimeout; Get().RecalculatePollPeriod(); @@ -970,11 +974,12 @@ void Mle::SetCslTimeout(uint32_t aTimeout) } exit: - return; + return error; } bool Mle::IsCslSupported(void) const { return IsChild() && GetParent().IsThreadVersion1p2OrHigher(); } -#endif + +#endif // OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE void Mle::InitNeighbor(Neighbor &aNeighbor, const RxInfo &aRxInfo) { @@ -3047,7 +3052,7 @@ uint64_t Mle::CalcParentCslMetric(const Mac::CslAccuracy &aCslAccuracy) const static constexpr uint64_t usInSecond = 1000000; - uint64_t cslPeriodUs = Mac::CslPeriodToUsec(Radio::kMinCslPeriod); + uint64_t cslPeriodUs = Mac::CslPeriodToUsec(Mac::kMinCslPeriod); uint64_t cslTimeoutUs = GetCslTimeout() * usInSecond; uint64_t k = cslTimeoutUs / cslPeriodUs; diff --git a/src/core/thread/mle.hpp b/src/core/thread/mle.hpp index a5e6db040..bf1acce8b 100644 --- a/src/core/thread/mle.hpp +++ b/src/core/thread/mle.hpp @@ -742,8 +742,11 @@ public: * Sets the CSL timeout. * * @param[in] aTimeout The CSL timeout in seconds. + * + * @retval kErrorNone Successfully set the CSL timeout. + * @retval kErrorInvalidArgs @p aTimeout is larger than the maximum allowed CSL timeout. */ - void SetCslTimeout(uint32_t aTimeout); + Error SetCslTimeout(uint32_t aTimeout); /** * Calculates CSL metric of parent. @@ -1353,6 +1356,9 @@ private: static constexpr uint32_t kDefaultStoreFrameCounterAhead = OPENTHREAD_CONFIG_STORE_FRAME_COUNTER_AHEAD; static constexpr uint32_t kDefaultChildTimeout = OPENTHREAD_CONFIG_MLE_CHILD_TIMEOUT_DEFAULT; static constexpr uint32_t kDefaultCslTimeout = OPENTHREAD_CONFIG_CSL_TIMEOUT; + static constexpr uint32_t kMaxCslTimeout = OPENTHREAD_CONFIG_MAC_CSL_MAX_TIMEOUT; + + static_assert(kDefaultCslTimeout <= kMaxCslTimeout, "default CSL timeout exceeds max CSL timeout"); #if OPENTHREAD_FTD // Advertisement trickle timer constants - all times are in milliseconds. diff --git a/tests/nexus/test_1_2_LP_5_3_2.cpp b/tests/nexus/test_1_2_LP_5_3_2.cpp index fd10cccb4..c885d4a5d 100644 --- a/tests/nexus/test_1_2_LP_5_3_2.cpp +++ b/tests/nexus/test_1_2_LP_5_3_2.cpp @@ -197,7 +197,7 @@ void Test1_2_LP_5_3_2(void) // Pre-set CSL parameters on SSED_1 before it joins. ssed1.Get().SetCslPeriod(kCslPeriodInTenSymbols); - ssed1.Get().SetCslTimeout(kCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout)); /** * Step 1: All @@ -228,7 +228,7 @@ void Test1_2_LP_5_3_2(void) // Override again after Join just in case Join() resets them to defaults. ssed1.Get().SetCslPeriod(kCslPeriodInTenSymbols); - ssed1.Get().SetCslTimeout(kCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout)); nexus.AdvanceTime(kAttachAsSsedTime); VerifyOrQuit(ssed1.Get().IsAttached()); @@ -241,7 +241,7 @@ void Test1_2_LP_5_3_2(void) Log("Step 2a: SSED_1"); // Deactivate autosynchronization by setting a very long CSL timeout on SSED_1. - ssed1.Get().SetCslTimeout(kDeactivatedCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kDeactivatedCslTimeout)); /** * Step 3: Harness diff --git a/tests/nexus/test_1_2_LP_5_3_3.cpp b/tests/nexus/test_1_2_LP_5_3_3.cpp index 40b47b141..958d15415 100644 --- a/tests/nexus/test_1_2_LP_5_3_3.cpp +++ b/tests/nexus/test_1_2_LP_5_3_3.cpp @@ -170,7 +170,7 @@ void Test1_2_LP_5_3_3(void) VerifyOrQuit(ssed1.Get().IsAttached()); ssed1.Get().SetCslPeriod(kCslPeriodMs * 1000 / OT_US_PER_TEN_SYMBOLS); - ssed1.Get().SetCslTimeout(kCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout)); nexus.AdvanceTime(kCslSyncTime); VerifyOrQuit(ssed1.Get().GetCslPeriod() > 0); diff --git a/tests/nexus/test_1_2_LP_5_3_4.cpp b/tests/nexus/test_1_2_LP_5_3_4.cpp index 8b4d28814..b9e53a245 100644 --- a/tests/nexus/test_1_2_LP_5_3_4.cpp +++ b/tests/nexus/test_1_2_LP_5_3_4.cpp @@ -131,7 +131,7 @@ void Test1_2_LP_5_3_4(void) ssed1.Join(leader, Node::kAsSed); ssed1.Get().SetCslPeriod(kCslPeriod500ms); - ssed1.Get().SetCslTimeout(kCslTimeout20s); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout20s)); nexus.AdvanceTime(kStabilizationTime); VerifyOrQuit(ssed1.Get().IsAttached()); diff --git a/tests/nexus/test_1_2_LP_5_3_5.cpp b/tests/nexus/test_1_2_LP_5_3_5.cpp index 851cfd38b..7f485225f 100644 --- a/tests/nexus/test_1_2_LP_5_3_5.cpp +++ b/tests/nexus/test_1_2_LP_5_3_5.cpp @@ -196,7 +196,7 @@ void Test1_2_LP_5_3_5(void) { // For SSED_2-6, apply target parameters before joining so they are used in Child ID Request sseds[i]->Get().SetCslPeriod(kCslPeriod500ms); - sseds[i]->Get().SetCslTimeout(kCslTimeouts[i]); + SuccessOrQuit(sseds[i]->Get().SetCslTimeout(kCslTimeouts[i])); sseds[i]->Get().SetCslChannel(kCslChannels[i]); } @@ -229,7 +229,7 @@ void Test1_2_LP_5_3_5(void) // Now update SSED_1 to trigger MLE Child Update Request/Response (Criterion 2.2) sseds[0]->Get().SetCslPeriod(kCslPeriod500ms); - sseds[0]->Get().SetCslTimeout(kCslTimeouts[0]); + SuccessOrQuit(sseds[0]->Get().SetCslTimeout(kCslTimeouts[0])); sseds[0]->Get().SetCslChannel(kCslChannels[0]); nexus.AdvanceTime(kStabilizationTime); diff --git a/tests/nexus/test_1_2_LP_5_3_6.cpp b/tests/nexus/test_1_2_LP_5_3_6.cpp index e14007b20..f58680e28 100644 --- a/tests/nexus/test_1_2_LP_5_3_6.cpp +++ b/tests/nexus/test_1_2_LP_5_3_6.cpp @@ -162,7 +162,7 @@ void Test1_2_LP_5_3_6(void) VerifyOrQuit(ssed1.Get().GetParent().GetExtAddress() == router1.Get().GetExtAddress()); ssed1.Get().SetCslPeriod(kCslPeriodMs * 1000 / OT_US_PER_TEN_SYMBOLS); - ssed1.Get().SetCslTimeout(kCslTimeoutSec); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeoutSec)); nexus.AdvanceTime(kStabilizationTime); /** diff --git a/tests/nexus/test_1_2_LP_5_3_7.cpp b/tests/nexus/test_1_2_LP_5_3_7.cpp index 74bd88bdf..5020a508e 100644 --- a/tests/nexus/test_1_2_LP_5_3_7.cpp +++ b/tests/nexus/test_1_2_LP_5_3_7.cpp @@ -159,7 +159,7 @@ void Test1_2_LP_5_3_7(void) ssed1.Join(router1, Node::kAsSed); // Set CSL parameters again to override defaults set by Join() ssed1.Get().SetCslPeriod(kCslPeriod); - ssed1.Get().SetCslTimeout(kCslTimeout10s); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout10s)); ssed1.Get().StopPolling(); nexus.AdvanceTime(kAttachAsSsedTime); VerifyOrQuit(ssed1.Get().IsAttached()); @@ -196,7 +196,7 @@ void Test1_2_LP_5_3_7(void) * Child Update Request. * Pass Criteria: N/A. */ - ssed1.Get().SetCslTimeout(kCslTimeout20s); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout20s)); Log("---------------------------------------------------------------------------------------"); Log("Step 6: Router_1 (DUT): Automatically responds with MLE Child Update Response."); @@ -244,7 +244,7 @@ void Test1_2_LP_5_3_7(void) * Child Update Request. * Pass Criteria: N/A. */ - ssed1.Get().SetCslTimeout(kCslTimeout10s); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout10s)); Log("---------------------------------------------------------------------------------------"); Log("Step 10: Router_1 (DUT): Automatically responds with MLE Child Update Response."); diff --git a/tests/nexus/test_1_2_LP_5_3_8.cpp b/tests/nexus/test_1_2_LP_5_3_8.cpp index 6e5c3ac2e..6c3415a2e 100644 --- a/tests/nexus/test_1_2_LP_5_3_8.cpp +++ b/tests/nexus/test_1_2_LP_5_3_8.cpp @@ -162,7 +162,7 @@ void Test5_3_8(void) * - Pass Criteria: N/A. */ - ssed1.Get().SetCslTimeout(kCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout)); ssed1.Get().SetCslPeriod(kCslPeriod); Log("---------------------------------------------------------------------------------------"); diff --git a/tests/nexus/test_1_2_LP_7_2_2.cpp b/tests/nexus/test_1_2_LP_7_2_2.cpp index d0bfeae4a..16f0e4fac 100644 --- a/tests/nexus/test_1_2_LP_7_2_2.cpp +++ b/tests/nexus/test_1_2_LP_7_2_2.cpp @@ -190,9 +190,9 @@ void Test1_2_LP_7_2_2(void) SuccessOrQuit(ssed1.Get().SetExternalPollPeriod(kPollRate5s)); SuccessOrQuit(ssed2.Get().SetExternalPollPeriod(kPollRate5s)); SuccessOrQuit(ssed3.Get().SetExternalPollPeriod(kPollRate5s)); - ssed1.Get().SetCslTimeout(kCslTimeout); - ssed2.Get().SetCslTimeout(kCslTimeout); - ssed3.Get().SetCslTimeout(kCslTimeout); + SuccessOrQuit(ssed1.Get().SetCslTimeout(kCslTimeout)); + SuccessOrQuit(ssed2.Get().SetCslTimeout(kCslTimeout)); + SuccessOrQuit(ssed3.Get().SetCslTimeout(kCslTimeout)); /** * Step 1: All