From 500337bc63d8cc9e8603442b26c1c7b134356776 Mon Sep 17 00:00:00 2001 From: Li Cao Date: Thu, 17 Sep 2020 13:15:37 +0800 Subject: [PATCH] [low-power] fix csl channel (#5501) If CSL Channel is never specified, the actual CSL channel should always be the phy channel that the device is using. If CSL Channel hasn't been specified and radio channel changes, the actual CSL channel should also change. In current implementation, CSL Channel is by default a valid value (not specified by upper layer) and wouldn't change when radio channel changes even if it's never specified. This commit fixes this issue. On CSL receiver, a bit is extracted from mCslChannel to be used as a flag mIsCslChannelSpecified. If it is false, mCslChannel would keep synchronized to the value of mRadioChannel in Mac and Csl Channel TLV wouldn't be appended in Child Update Request to its parent. On CSL transmitter, CSL channel info in child is allowed to be 0. If it's 0, the channel info in frame would be set using Mac::GetPanChannel. --- include/openthread/link.h | 3 +- src/core/api/link_api.cpp | 2 +- src/core/mac/mac.cpp | 3 +- src/core/mac/mac.hpp | 8 +++++ src/core/mac/sub_mac.cpp | 10 ++++-- src/core/mac/sub_mac.hpp | 32 +++++++++++++++---- src/core/radio/radio.hpp | 12 +++++++ src/core/thread/csl_tx_scheduler.cpp | 3 +- src/core/thread/mle.cpp | 6 +++- src/core/thread/mle_router.cpp | 5 +-- tests/scripts/thread-cert/config.py | 1 + tests/scripts/thread-cert/mle.py | 15 +++++++++ .../thread-cert/v1_2_test_csl_transmission.py | 23 ++++++++++++- 13 files changed, 107 insertions(+), 16 deletions(-) diff --git a/include/openthread/link.h b/include/openthread/link.h index e42b0b9f7..8b4f565af 100644 --- a/include/openthread/link.h +++ b/include/openthread/link.h @@ -1019,7 +1019,8 @@ uint8_t otLinkCslGetChannel(otInstance *aInstance); * This function sets the CSL channel. * * @param[in] aInstance A pointer to an OpenThread instance. - * @param[in] aChannel The CSL sample channel. + * @param[in] aChannel The CSL sample channel. Channel value should be `0` (Set CSL Channel unspecified) or + * within the range [1, 10] (if 915-MHz supported) and [11, 26] (if 2.4 GHz supported). * * @retval OT_ERROR_NONE Successfully set the CSL parameters. * @retval OT_ERROR_INVALID_ARGS Invalid @p aChannel. diff --git a/src/core/api/link_api.cpp b/src/core/api/link_api.cpp index c3910c5dc..fb0ebecb6 100644 --- a/src/core/api/link_api.cpp +++ b/src/core/api/link_api.cpp @@ -493,7 +493,7 @@ otError otLinkCslSetChannel(otInstance *aInstance, uint8_t aChannel) otError error = OT_ERROR_NONE; Instance &instance = *static_cast(aInstance); - VerifyOrExit((Radio::kChannelMin <= aChannel) && (aChannel <= Radio::kChannelMax), error = OT_ERROR_INVALID_ARGS); + VerifyOrExit(Radio::IsCslChannelValid(aChannel), error = OT_ERROR_INVALID_ARGS); instance.Get().SetCslChannel(aChannel); diff --git a/src/core/mac/mac.cpp b/src/core/mac/mac.cpp index 6b5ea92fb..4d1cfa761 100644 --- a/src/core/mac/mac.cpp +++ b/src/core/mac/mac.cpp @@ -658,7 +658,7 @@ void Mac::UpdateIdleMode(void) #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE if (IsCslEnabled()) { - IgnoreError(mSubMac.CslSample()); + IgnoreError(mSubMac.CslSample(mRadioChannel)); ExitNow(); } #endif @@ -2269,6 +2269,7 @@ void Mac::SetCslChannel(uint8_t aChannel) VerifyOrExit(GetCslChannel() != aChannel, OT_NOOP); mSubMac.SetCslChannel(aChannel); + mSubMac.SetCslChannelSpecified(aChannel != 0 ? true : false); if (IsCslEnabled()) { diff --git a/src/core/mac/mac.hpp b/src/core/mac/mac.hpp index 984d5ce4d..0483cc2d5 100644 --- a/src/core/mac/mac.hpp +++ b/src/core/mac/mac.hpp @@ -693,6 +693,14 @@ public: */ void SetCslChannel(uint8_t aChannel); + /** + * This method indicates if CSL channel has been explicitly specified by the upper layer. + * + * @returns If CSL channel has been specified. + * + */ + bool IsCslChannelSpecified(void) const { return mSubMac.IsCslChannelSpecified(); } + /** * This method gets the CSL period. * diff --git a/src/core/mac/sub_mac.cpp b/src/core/mac/sub_mac.cpp index 790426cc8..793792f92 100644 --- a/src/core/mac/sub_mac.cpp +++ b/src/core/mac/sub_mac.cpp @@ -68,7 +68,8 @@ SubMac::SubMac(Instance &aInstance) #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE , mCslTimeout(OPENTHREAD_CONFIG_CSL_TIMEOUT) , mCslPeriod(0) - , mCslChannel(OPENTHREAD_CONFIG_DEFAULT_CHANNEL) + , mCslChannel(0) + , mIsCslChannelSpecified(false) , mCslState(kCslIdle) , mCslTimer(aInstance, SubMac::HandleCslTimer, this) #endif @@ -211,10 +212,15 @@ exit: } #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE -otError SubMac::CslSample(void) +otError SubMac::CslSample(uint8_t aPanChannel) { otError error = OT_ERROR_NONE; + if (!IsCslChannelSpecified()) + { + mCslChannel = aPanChannel; + } + switch (mCslState) { case kCslSample: diff --git a/src/core/mac/sub_mac.hpp b/src/core/mac/sub_mac.hpp index 3ec42a9dd..390b5fcd4 100644 --- a/src/core/mac/sub_mac.hpp +++ b/src/core/mac/sub_mac.hpp @@ -305,12 +305,15 @@ public: * started, `mState` will become `kStateCslSample`. But it could be doing `Sleep` or `Receive` at this moment * (depending on `mCslState`). * + * @param[in] aPanChannel The current phy channel used by the device. This param will only take effect when CSL + * channel hasn't been explicitly specified. + * * @retval OT_ERROR_NONE Successfully entered CSL operation (sleep or receive according to CSL timer). * @retval OT_ERROR_BUSY The radio was transmitting. * @retval OT_ERROR_INVALID_STATE The radio was disabled. * */ - otError CslSample(void); + otError CslSample(uint8_t aPanChannel); #endif /** @@ -384,11 +387,25 @@ public: /** * This method sets the CSL channel. * - * @param[in] aChannel The CSL channel. + * @param[in] aChannel The CSL channel. `0` to set CSL Channel unspecified. * */ void SetCslChannel(uint8_t aChannel); + /** + * This method indicates if CSL channel has been explicitly specified by the upper layer. + * + * @returns If CSL channel has been specified. + * + */ + bool IsCslChannelSpecified(void) const { return mIsCslChannelSpecified; } + + /** + * This method sets the flag representing if CSL channel has been specified. + * + */ + void SetCslChannelSpecified(bool aIsSpecified) { mIsCslChannelSpecified = aIsSpecified; } + /** * This method gets the CSL period. * @@ -604,10 +621,13 @@ private: #endif #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE - uint32_t mCslTimeout; ///< The CSL synchronized timeout in seconds. - TimeMicro mCslSampleTime; ///< The CSL sample time of the current period. - uint16_t mCslPeriod; ///< The CSL sample period, in units of 10 symbols (160 microseconds). - uint8_t mCslChannel; ///< The CSL sample channel. + uint32_t mCslTimeout; ///< The CSL synchronized timeout in seconds. + TimeMicro mCslSampleTime; ///< The CSL sample time of the current period. + uint16_t mCslPeriod; ///< The CSL sample period, in units of 10 symbols (160 microseconds). + uint8_t mCslChannel : 7; ///< The actually CSL sample channel. If `mIsCslChannelSpecified` is 0, this should be + ///< equal to the Pan channel of `Mac`. + uint8_t mIsCslChannelSpecified : 1; ///< Indicates whether or not the CSL channel was explicitly specified by + ///< the user. CslState mCslState; diff --git a/src/core/radio/radio.hpp b/src/core/radio/radio.hpp index 727e76b51..74eb68be9 100644 --- a/src/core/radio/radio.hpp +++ b/src/core/radio/radio.hpp @@ -594,6 +594,18 @@ public: */ uint32_t GetPreferredChannelMask(void) { return otPlatRadioGetPreferredChannelMask(GetInstance()); } + /** + * This method checks if a given channel is valid as a CSL channel. + * + * @retval true The channel is valid. + * @retval false The channel is invalid. + * + */ + static bool IsCslChannelValid(uint8_t aCslChannel) + { + return (aCslChannel == 0) || ((kChannelMin <= aCslChannel) && (aCslChannel <= kChannelMax)); + } + private: otInstance *GetInstance(void) { return reinterpret_cast(&InstanceLocator::GetInstance()); } diff --git a/src/core/thread/csl_tx_scheduler.cpp b/src/core/thread/csl_tx_scheduler.cpp index 61cc73f6d..94d23e923 100644 --- a/src/core/thread/csl_tx_scheduler.cpp +++ b/src/core/thread/csl_tx_scheduler.cpp @@ -200,7 +200,8 @@ otError CslTxScheduler::HandleFrameRequest(Mac::TxFrame &aFrame) aFrame.SetIsARetransmission(false); } - aFrame.SetChannel(mCslTxChild->GetCslChannel()); + aFrame.SetChannel(mCslTxChild->GetCslChannel() == 0 ? Get().GetPanChannel() + : mCslTxChild->GetCslChannel()); aFrame.SetTxPhase(mCslTxChild->GetCslPhase()); aFrame.SetTxPeriod(mCslTxChild->GetCslPeriod()); diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index d9b05ad4f..83d5b4705 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -1396,7 +1396,11 @@ otError Mle::AppendCslChannel(Message &aMessage) otError error = OT_ERROR_NONE; CslChannelTlv cslChannel; - VerifyOrExit(Get().GetPanChannel() != Get().GetCslChannel(), OT_NOOP); + // In current implementation, it's allowed to set CSL Channel unspecified. As `0` is not valid for Channel value + // in CSL Channel TLV, if CSL channel is not specified, we don't append CSL Channel TLV. + // And on transmitter side, it would also set CSL Channel for the child to `0` if it doesn't find a CSL Channel + // TLV. + VerifyOrExit(Get().IsCslChannelSpecified(), OT_NOOP); cslChannel.Init(); cslChannel.SetChannelPage(0); diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index a084d9a78..b37af183f 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -2509,9 +2509,10 @@ void MleRouter::HandleChildUpdateRequest(const Message & aMessage, { child->SetCslChannel(static_cast(cslChannel.GetChannel())); } - else if (child->GetCslChannel() == 0) + else { - child->SetCslChannel(Get().GetPanChannel()); + // Set CSL Channel unspecified. + child->SetCslChannel(0); } } #endif // OPENTHREAD_CONFIG_MAC_CSL_TRANSMITTER_ENABLE diff --git a/tests/scripts/thread-cert/config.py b/tests/scripts/thread-cert/config.py index c90bd6ffe..7e12da0fc 100644 --- a/tests/scripts/thread-cert/config.py +++ b/tests/scripts/thread-cert/config.py @@ -229,6 +229,7 @@ def create_default_mle_tlvs_factories(): mle.TlvType.PANID: mle.PanIdFactory(), mle.TlvType.ACTIVE_TIMESTAMP: mle.ActiveTimestampFactory(), mle.TlvType.PENDING_TIMESTAMP: mle.PendingTimestampFactory(), + mle.TlvType.CSL_CHANNEL: mle.CslChannelFactory(), mle.TlvType.CSL_SYNCHRONIZED_TIMEOUT: mle.CslSynchronizedTimeoutFactory(), mle.TlvType.ACTIVE_OPERATIONAL_DATASET: mle.ActiveOperationalDatasetFactory(), mle.TlvType.PENDING_OPERATIONAL_DATASET: mle.PendingOperationalDatasetFactory(), diff --git a/tests/scripts/thread-cert/mle.py b/tests/scripts/thread-cert/mle.py index 768cc23b6..df6aaf101 100644 --- a/tests/scripts/thread-cert/mle.py +++ b/tests/scripts/thread-cert/mle.py @@ -87,6 +87,7 @@ class TlvType(IntEnum): ACTIVE_OPERATIONAL_DATASET = 24 PENDING_OPERATIONAL_DATASET = 25 THREAD_DISCOVERY = 26 + CSL_CHANNEL = 80 CSL_SYNCHRONIZED_TIMEOUT = 85 TIME_REQUEST = 252 TIME_PARAMETER = 253 @@ -1048,6 +1049,20 @@ class ThreadDiscoveryFactory: return ThreadDiscovery(tlvs) +class CslChannel: + # TODO: Not implemented yet + + def __init__(self): + print("CslChannel is not implemented yet.") + + +class CslChannelFactory: + # TODO: Not implemented yet + + def parse(self, data, message_info): + return CslChannel() + + class CslSynchronizedTimeout: # TODO: Not implemented yet diff --git a/tests/scripts/thread-cert/v1_2_test_csl_transmission.py b/tests/scripts/thread-cert/v1_2_test_csl_transmission.py index fd84a69d6..e79a05261 100755 --- a/tests/scripts/thread-cert/v1_2_test_csl_transmission.py +++ b/tests/scripts/thread-cert/v1_2_test_csl_transmission.py @@ -29,6 +29,7 @@ import unittest +import mle import thread_cert LEADER = 1 @@ -55,7 +56,6 @@ class SSED_CslTransmission(thread_cert.TestCase): self.nodes[SSED_1].set_csl_period(CSL_PERIOD) self.nodes[SSED_1].set_csl_timeout(CSL_TIMEOUT) - self.nodes[SSED_1].set_csl_channel(CSL_CHANNEL) self.nodes[SSED_1].get_csl_info() @@ -71,6 +71,27 @@ class SSED_CslTransmission(thread_cert.TestCase): self.assertTrue(self.nodes[LEADER].ping(self.nodes[SSED_1].get_rloc())) self.simulator.go(5) + ssed_messages = self.simulator.get_messages_sent_by(SSED_1) + msg = ssed_messages.next_mle_message(mle.CommandType.CHILD_UPDATE_REQUEST) + msg.assertMleMessageDoesNotContainTlv(mle.CslChannel) + + self.nodes[SSED_1].set_csl_channel(CSL_CHANNEL) + self.simulator.go(1) + ssed_messages = self.simulator.get_messages_sent_by(SSED_1) + msg = ssed_messages.next_mle_message(mle.CommandType.CHILD_UPDATE_REQUEST) + msg.assertMleMessageContainsTlv(mle.CslChannel) + self.assertTrue(self.nodes[LEADER].ping(self.nodes[SSED_1].get_rloc())) + self.simulator.go(5) + + self.nodes[SSED_1].set_csl_channel(0) + self.simulator.go(1) + self.assertTrue(self.nodes[LEADER].ping(self.nodes[SSED_1].get_rloc())) + self.simulator.go(5) + + ssed_messages = self.simulator.get_messages_sent_by(SSED_1) + msg = ssed_messages.next_mle_message(mle.CommandType.CHILD_UPDATE_REQUEST) + msg.assertMleMessageDoesNotContainTlv(mle.CslChannel) + self.nodes[SSED_1].set_csl_period(0) self.assertFalse(self.nodes[LEADER].ping(self.nodes[SSED_1].get_rloc())) self.simulator.go(2)