From 5daca7c684adf5124e659b0171fb1b8679e08750 Mon Sep 17 00:00:00 2001 From: Kangping Date: Sat, 4 Jun 2022 01:59:16 +0800 Subject: [PATCH] [dataset] use full Timestamp TLV value in otOperationalDataset (#7739) The `otOperationalDataset#mActiveTimestamp` should represent the full Active Timestamp TLV value but not only the seconds. The same for `otOperationalDataset#mPendingTimestamp`. --- include/openthread/dataset.h | 15 ++++++-- include/openthread/instance.h | 2 +- src/cli/README_DATASET.md | 8 ++--- src/cli/cli_dataset.cpp | 24 ++++++++----- src/core/meshcop/dataset.cpp | 29 ++++++++-------- src/core/meshcop/dataset.hpp | 12 +++---- src/core/meshcop/dataset_updater.cpp | 13 +++++-- src/core/meshcop/timestamp.cpp | 52 +++++++++++++++++++++++----- src/core/meshcop/timestamp.hpp | 26 ++++++++++++++ src/lib/spinel/radio_spinel_impl.hpp | 2 +- src/ncp/ncp_base_mtd.cpp | 16 ++++++--- tests/unit/test_meshcop.cpp | 3 +- 12 files changed, 150 insertions(+), 52 deletions(-) diff --git a/include/openthread/dataset.h b/include/openthread/dataset.h index a92f3a78c..63e1e26ad 100644 --- a/include/openthread/dataset.h +++ b/include/openthread/dataset.h @@ -213,6 +213,17 @@ typedef struct otOperationalDatasetComponents bool mIsChannelMaskPresent : 1; ///< TRUE if Channel Mask is present, FALSE otherwise. } otOperationalDatasetComponents; +/** + * This structure represents a Thread Dataset timestamp component. + * + */ +typedef struct otTimestamp +{ + uint64_t mSeconds; + uint16_t mTicks; + bool mAuthoritative; +} otTimestamp; + /** * This structure represents an Active or Pending Operational Dataset. * @@ -221,8 +232,8 @@ typedef struct otOperationalDatasetComponents */ typedef struct otOperationalDataset { - uint64_t mActiveTimestamp; ///< Active Timestamp - uint64_t mPendingTimestamp; ///< Pending Timestamp + otTimestamp mActiveTimestamp; ///< Active Timestamp + otTimestamp mPendingTimestamp; ///< Pending Timestamp otNetworkKey mNetworkKey; ///< Network Key otNetworkName mNetworkName; ///< Network Name otExtendedPanId mExtendedPanId; ///< Extended PAN ID diff --git a/include/openthread/instance.h b/include/openthread/instance.h index e0ff78f8b..5f91d8165 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (218) +#define OPENTHREAD_API_VERSION (219) /** * @addtogroup api-instance diff --git a/src/cli/README_DATASET.md b/src/cli/README_DATASET.md index 1043bbaa0..87cd4d21c 100644 --- a/src/cli/README_DATASET.md +++ b/src/cli/README_DATASET.md @@ -196,7 +196,7 @@ Done Usage: `dataset activetimestamp [timestamp]` -Get active timestamp. +Get active timestamp seconds. ```bash > dataset activetimestamp @@ -204,7 +204,7 @@ Get active timestamp. Done ``` -Set active timestamp. +Set active timestamp seconds. ```bash > dataset activetimestamp 123456789 @@ -457,7 +457,7 @@ Done Usage: `dataset pendingtimestamp [timestamp]` -Get pending timestamp. +Get pending timestamp seconds. ```bash > dataset pendingtimestamp @@ -465,7 +465,7 @@ Get pending timestamp. Done ``` -Set pending timestamp. +Set pending timestamp seconds. ```bash > dataset pendingtimestamp 123456789 diff --git a/src/cli/cli_dataset.cpp b/src/cli/cli_dataset.cpp index efade0586..573436aea 100644 --- a/src/cli/cli_dataset.cpp +++ b/src/cli/cli_dataset.cpp @@ -51,12 +51,12 @@ otError Dataset::Print(otOperationalDataset &aDataset) { if (aDataset.mComponents.mIsPendingTimestampPresent) { - OutputLine("Pending Timestamp: %lu", aDataset.mPendingTimestamp); + OutputLine("Pending Timestamp: %lu", aDataset.mPendingTimestamp.mSeconds); } if (aDataset.mComponents.mIsActiveTimestampPresent) { - OutputLine("Active Timestamp: %lu", aDataset.mActiveTimestamp); + OutputLine("Active Timestamp: %lu", aDataset.mActiveTimestamp.mSeconds); } if (aDataset.mComponents.mIsChannelPresent) @@ -205,12 +205,14 @@ template <> otError Dataset::Process(Arg aArgs[]) { if (sDataset.mComponents.mIsActiveTimestampPresent) { - OutputLine("%lu", sDataset.mActiveTimestamp); + OutputLine("%lu", sDataset.mActiveTimestamp.mSeconds); } } else { - SuccessOrExit(error = aArgs[0].ParseAsUint64(sDataset.mActiveTimestamp)); + SuccessOrExit(error = aArgs[0].ParseAsUint64(sDataset.mActiveTimestamp.mSeconds)); + sDataset.mActiveTimestamp.mTicks = 0; + sDataset.mActiveTimestamp.mAuthoritative = false; sDataset.mComponents.mIsActiveTimestampPresent = true; } @@ -423,12 +425,14 @@ template <> otError Dataset::Process(Arg aArgs[]) { if (sDataset.mComponents.mIsPendingTimestampPresent) { - OutputLine("%lu", sDataset.mPendingTimestamp); + OutputLine("%lu", sDataset.mPendingTimestamp.mSeconds); } } else { - SuccessOrExit(error = aArgs[0].ParseAsUint64(sDataset.mPendingTimestamp)); + SuccessOrExit(error = aArgs[0].ParseAsUint64(sDataset.mPendingTimestamp.mSeconds)); + sDataset.mPendingTimestamp.mTicks = 0; + sDataset.mPendingTimestamp.mAuthoritative = false; sDataset.mComponents.mIsPendingTimestampPresent = true; } @@ -450,14 +454,18 @@ template <> otError Dataset::Process(Arg aArgs[]) if (*arg == "activetimestamp") { arg++; + SuccessOrExit(error = arg->ParseAsUint64(dataset.mActiveTimestamp.mSeconds)); + dataset.mActiveTimestamp.mTicks = 0; + dataset.mActiveTimestamp.mAuthoritative = false; dataset.mComponents.mIsActiveTimestampPresent = true; - SuccessOrExit(error = arg->ParseAsUint64(dataset.mActiveTimestamp)); } else if (*arg == "pendingtimestamp") { arg++; + SuccessOrExit(error = arg->ParseAsUint64(dataset.mPendingTimestamp.mSeconds)); + dataset.mPendingTimestamp.mTicks = 0; + dataset.mPendingTimestamp.mAuthoritative = false; dataset.mComponents.mIsPendingTimestampPresent = true; - SuccessOrExit(error = arg->ParseAsUint64(dataset.mPendingTimestamp)); } else if (*arg == "networkkey") { diff --git a/src/core/meshcop/dataset.cpp b/src/core/meshcop/dataset.cpp index 74d9a0919..047808594 100644 --- a/src/core/meshcop/dataset.cpp +++ b/src/core/meshcop/dataset.cpp @@ -43,6 +43,7 @@ #include "common/log.hpp" #include "mac/mac_types.hpp" #include "meshcop/meshcop_tlvs.hpp" +#include "meshcop/timestamp.hpp" #include "thread/mle_tlvs.hpp" namespace ot { @@ -69,10 +70,12 @@ Error Dataset::Info::GenerateRandom(Instance &aInstance) Clear(); - mActiveTimestamp = 1; - mChannel = preferredChannels.ChooseRandomChannel(); - mChannelMask = supportedChannels.GetMask(); - mPanId = Mac::GenerateRandomPanId(); + mActiveTimestamp.mSeconds = 1; + mActiveTimestamp.mTicks = 0; + mActiveTimestamp.mAuthoritative = false; + mChannel = preferredChannels.ChooseRandomChannel(); + mChannelMask = supportedChannels.GetMask(); + mPanId = Mac::GenerateRandomPanId(); AsCoreType(&mSecurityPolicy).SetToDefault(); SuccessOrExit(error = AsCoreType(&mNetworkKey).GenerateRandom()); @@ -193,7 +196,7 @@ void Dataset::ConvertTo(Info &aDatasetInfo) const switch (cur->GetType()) { case Tlv::kActiveTimestamp: - aDatasetInfo.SetActiveTimestamp(As(cur)->GetTimestamp().GetSeconds()); + aDatasetInfo.SetActiveTimestamp(As(cur)->GetTimestamp()); break; case Tlv::kChannel: @@ -237,7 +240,7 @@ void Dataset::ConvertTo(Info &aDatasetInfo) const break; case Tlv::kPendingTimestamp: - aDatasetInfo.SetPendingTimestamp(As(cur)->GetTimestamp().GetSeconds()); + aDatasetInfo.SetPendingTimestamp(As(cur)->GetTimestamp()); break; case Tlv::kPskc: @@ -286,20 +289,18 @@ Error Dataset::SetFrom(const Info &aDatasetInfo) if (aDatasetInfo.IsActiveTimestampPresent()) { - Timestamp timestamp; + Timestamp activeTimestamp; - timestamp.Clear(); - timestamp.SetSeconds(aDatasetInfo.GetActiveTimestamp()); - IgnoreError(SetTlv(Tlv::kActiveTimestamp, timestamp)); + aDatasetInfo.GetActiveTimestamp(activeTimestamp); + IgnoreError(SetTlv(Tlv::kActiveTimestamp, activeTimestamp)); } if (aDatasetInfo.IsPendingTimestampPresent()) { - Timestamp timestamp; + Timestamp pendingTimestamp; - timestamp.Clear(); - timestamp.SetSeconds(aDatasetInfo.GetPendingTimestamp()); - IgnoreError(SetTlv(Tlv::kPendingTimestamp, timestamp)); + aDatasetInfo.GetPendingTimestamp(pendingTimestamp); + IgnoreError(SetTlv(Tlv::kPendingTimestamp, pendingTimestamp)); } if (aDatasetInfo.IsDelayPresent()) diff --git a/src/core/meshcop/dataset.hpp b/src/core/meshcop/dataset.hpp index 4e200d6cf..380951373 100644 --- a/src/core/meshcop/dataset.hpp +++ b/src/core/meshcop/dataset.hpp @@ -203,7 +203,7 @@ public: * @returns The Active Timestamp in the Dataset. * */ - uint64_t GetActiveTimestamp(void) const { return mActiveTimestamp; } + void GetActiveTimestamp(Timestamp &aTimestamp) const { aTimestamp.SetFromTimestamp(mActiveTimestamp); } /** * This method sets the Active Timestamp in the Dataset. @@ -211,9 +211,9 @@ public: * @param[in] aTimestamp A Timestamp value. * */ - void SetActiveTimestamp(uint64_t aTimestamp) + void SetActiveTimestamp(const Timestamp &aTimestamp) { - mActiveTimestamp = aTimestamp; + aTimestamp.ConvertTo(mActiveTimestamp); mComponents.mIsActiveTimestampPresent = true; } @@ -234,7 +234,7 @@ public: * @returns The Pending Timestamp in the Dataset. * */ - uint64_t GetPendingTimestamp(void) const { return mPendingTimestamp; } + void GetPendingTimestamp(Timestamp &aTimestamp) const { aTimestamp.SetFromTimestamp(mPendingTimestamp); } /** * This method sets the Pending Timestamp in the Dataset. @@ -242,9 +242,9 @@ public: * @param[in] aTimestamp A Timestamp value. * */ - void SetPendingTimestamp(uint64_t aTimestamp) + void SetPendingTimestamp(const Timestamp &aTimestamp) { - mPendingTimestamp = aTimestamp; + aTimestamp.ConvertTo(mPendingTimestamp); mComponents.mIsPendingTimestampPresent = true; } diff --git a/src/core/meshcop/dataset_updater.cpp b/src/core/meshcop/dataset_updater.cpp index 00782b227..36d63d7f9 100644 --- a/src/core/meshcop/dataset_updater.cpp +++ b/src/core/meshcop/dataset_updater.cpp @@ -41,6 +41,7 @@ #include "common/locator_getters.hpp" #include "common/log.hpp" #include "common/random.hpp" +#include "meshcop/timestamp.hpp" namespace ot { namespace MeshCoP { @@ -193,9 +194,17 @@ void DatasetUpdater::HandleNotifierEvents(Events aEvents) { Finish(kErrorNone); } - else if (requestedDataset.GetActiveTimestamp() <= dataset.GetActiveTimestamp()) + else { - Finish(kErrorAlready); + Timestamp requestedDatasetTimestamp; + Timestamp activeDatasetTimestamp; + + requestedDataset.GetActiveTimestamp(requestedDatasetTimestamp); + dataset.GetActiveTimestamp(activeDatasetTimestamp); + if (Timestamp::Compare(requestedDatasetTimestamp, activeDatasetTimestamp) <= 0) + { + Finish(kErrorAlready); + } } } diff --git a/src/core/meshcop/timestamp.cpp b/src/core/meshcop/timestamp.cpp index 5d8f59bf6..08ed36b94 100644 --- a/src/core/meshcop/timestamp.cpp +++ b/src/core/meshcop/timestamp.cpp @@ -38,13 +38,23 @@ namespace ot { namespace MeshCoP { +void Timestamp::ConvertTo(otTimestamp &aTimestamp) const +{ + aTimestamp.mSeconds = GetSeconds(); + aTimestamp.mTicks = GetTicks(); + aTimestamp.mAuthoritative = GetAuthoritative(); +} + +void Timestamp::SetFromTimestamp(const otTimestamp &aTimestamp) +{ + SetSeconds(aTimestamp.mSeconds); + SetTicks(aTimestamp.mTicks); + SetAuthoritative(aTimestamp.mAuthoritative); +} + int Timestamp::Compare(const Timestamp *aFirst, const Timestamp *aSecond) { - int rval; - uint64_t firstSeconds; - uint64_t secondSeconds; - uint16_t firstTicks; - uint16_t secondTicks; + int rval; if (aFirst == nullptr) { @@ -62,22 +72,46 @@ int Timestamp::Compare(const Timestamp *aFirst, const Timestamp *aSecond) // Both are non-null. - firstSeconds = aFirst->GetSeconds(); - secondSeconds = aSecond->GetSeconds(); + rval = Compare(*aFirst, *aSecond); + +exit: + return rval; +} + +int Timestamp::Compare(const Timestamp &aFirst, const Timestamp &aSecond) +{ + int rval; + uint64_t firstSeconds; + uint64_t secondSeconds; + uint16_t firstTicks; + uint16_t secondTicks; + bool firstAuthoritative; + bool secondAuthoritative; + + firstSeconds = aFirst.GetSeconds(); + secondSeconds = aSecond.GetSeconds(); if (firstSeconds != secondSeconds) { ExitNow(rval = (firstSeconds > secondSeconds) ? 1 : -1); } - firstTicks = aFirst->GetTicks(); - secondTicks = aSecond->GetTicks(); + firstTicks = aFirst.GetTicks(); + secondTicks = aSecond.GetTicks(); if (firstTicks != secondTicks) { ExitNow(rval = (firstTicks > secondTicks) ? 1 : -1); } + firstAuthoritative = aFirst.GetAuthoritative(); + secondAuthoritative = aSecond.GetAuthoritative(); + + if (firstAuthoritative != secondAuthoritative) + { + ExitNow(rval = firstAuthoritative ? 1 : -1); + } + rval = 0; exit: diff --git a/src/core/meshcop/timestamp.hpp b/src/core/meshcop/timestamp.hpp index ea3b11583..b4a6bfd0a 100644 --- a/src/core/meshcop/timestamp.hpp +++ b/src/core/meshcop/timestamp.hpp @@ -39,6 +39,7 @@ #include +#include #include #include "common/clearable.hpp" @@ -59,6 +60,18 @@ OT_TOOL_PACKED_BEGIN class Timestamp : public Clearable { public: + /** + * This method converts the timestamp to `otTimestamp`. + * + */ + void ConvertTo(otTimestamp &aTimestamp) const; + + /** + * This method sets the timestamp from `otTimestamp`. + * + */ + void SetFromTimestamp(const otTimestamp &aTimestamp); + /** * This method returns the Seconds value. * @@ -143,6 +156,19 @@ public: */ static int Compare(const Timestamp *aFirst, const Timestamp *aSecond); + /** + * This static method compares two timestamps. + * + * @param[in] aFirst A reference to the first timestamp to compare. + * @param[in] aSecond A reference to the second timestamp to compare. + * + * @retval -1 if @p aFirst is less than @p aSecond (`aFirst < aSecond`). + * @retval 0 if @p aFirst is equal to @p aSecond (`aFirst == aSecond`). + * @retval 1 if @p aFirst is greater than @p aSecond (`aFirst > aSecond`). + * + */ + static int Compare(const Timestamp &aFirst, const Timestamp &aSecond); + private: static constexpr uint8_t kTicksOffset = 1; static constexpr uint16_t kTicksMask = 0x7fff << kTicksOffset; diff --git a/src/lib/spinel/radio_spinel_impl.hpp b/src/lib/spinel/radio_spinel_impl.hpp index b7650ddf7..0073817ae 100644 --- a/src/lib/spinel/radio_spinel_impl.hpp +++ b/src/lib/spinel/radio_spinel_impl.hpp @@ -752,7 +752,7 @@ otError RadioSpinel::ThreadDatasetHandler(con * Initially set Active Timestamp to 0. This is to allow the node to join the network * yet retrieve the full Active Dataset from a neighboring device if one exists. */ - opDataset.mActiveTimestamp = 0; + memset(&opDataset.mActiveTimestamp, 0, sizeof(opDataset.mActiveTimestamp)); opDataset.mComponents.mIsActiveTimestampPresent = true; SuccessOrExit(error = dataset.SetFrom(static_cast(opDataset))); diff --git a/src/ncp/ncp_base_mtd.cpp b/src/ncp/ncp_base_mtd.cpp index bd1f227cf..09fad54bd 100644 --- a/src/ncp/ncp_base_mtd.cpp +++ b/src/ncp/ncp_base_mtd.cpp @@ -1228,17 +1228,21 @@ otError NcpBase::EncodeOperationalDataset(const otOperationalDataset &aDataset) if (aDataset.mComponents.mIsActiveTimestampPresent) { + const otTimestamp &activeTimestamp = aDataset.mActiveTimestamp; + SuccessOrExit(error = mEncoder.OpenStruct()); SuccessOrExit(error = mEncoder.WriteUintPacked(SPINEL_PROP_DATASET_ACTIVE_TIMESTAMP)); - SuccessOrExit(error = mEncoder.WriteUint64(aDataset.mActiveTimestamp)); + SuccessOrExit(error = mEncoder.WriteUint64(activeTimestamp.mSeconds)); SuccessOrExit(error = mEncoder.CloseStruct()); } if (aDataset.mComponents.mIsPendingTimestampPresent) { + const otTimestamp &pendingTimestamp = aDataset.mPendingTimestamp; + SuccessOrExit(error = mEncoder.OpenStruct()); SuccessOrExit(error = mEncoder.WriteUintPacked(SPINEL_PROP_DATASET_PENDING_TIMESTAMP)); - SuccessOrExit(error = mEncoder.WriteUint64(aDataset.mPendingTimestamp)); + SuccessOrExit(error = mEncoder.WriteUint64(pendingTimestamp.mSeconds)); SuccessOrExit(error = mEncoder.CloseStruct()); } @@ -1400,7 +1404,9 @@ otError NcpBase::DecodeOperationalDataset(otOperationalDataset &aDataset, if (!aAllowEmptyValues || !mDecoder.IsAllReadInStruct()) { - SuccessOrExit(error = mDecoder.ReadUint64(aDataset.mActiveTimestamp)); + SuccessOrExit(error = mDecoder.ReadUint64(aDataset.mActiveTimestamp.mSeconds)); + aDataset.mActiveTimestamp.mTicks = 0; + aDataset.mActiveTimestamp.mAuthoritative = false; } aDataset.mComponents.mIsActiveTimestampPresent = true; @@ -1410,7 +1416,9 @@ otError NcpBase::DecodeOperationalDataset(otOperationalDataset &aDataset, if (!aAllowEmptyValues || !mDecoder.IsAllReadInStruct()) { - SuccessOrExit(error = mDecoder.ReadUint64(aDataset.mPendingTimestamp)); + SuccessOrExit(error = mDecoder.ReadUint64(aDataset.mPendingTimestamp.mSeconds)); + aDataset.mPendingTimestamp.mTicks = 0; + aDataset.mPendingTimestamp.mAuthoritative = false; } aDataset.mComponents.mIsPendingTimestampPresent = true; diff --git a/tests/unit/test_meshcop.cpp b/tests/unit/test_meshcop.cpp index bcf823ba4..6dae027de 100644 --- a/tests/unit/test_meshcop.cpp +++ b/tests/unit/test_meshcop.cpp @@ -139,7 +139,7 @@ void TestTimestamp(void) VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); t1.SetAuthoritative(true); - VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) > 0); t1.SetSeconds(1); VerifyOrQuit(t1.GetSeconds() == 1); @@ -147,6 +147,7 @@ void TestTimestamp(void) VerifyOrQuit(MeshCoP::Timestamp::Compare(&t2, &t1) < 0); t2.SetSeconds(1); + t2.SetAuthoritative(true); VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); printf("TestTimestamp() passed\n");