From 8abf9667c3dbc782c9436b27b91e91ce38f2f15d Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 8 Jun 2026 19:34:19 -0700 Subject: [PATCH] [common] refactor `FrameBuilder` and `FrameData` to use templated API (#13216) This commit refactors the `FrameBuilder` and `FrameData` modules, replacing multiple distinct endian-specific methods with a unified, type-safe templated API. Key changes: - Introduced `enum Encoding` (`kBigEndian`, `kLittleEndian`) and a templated `HostSwap` helper in `encoding.hpp` to centralize byte-swapping logic. - Replaced `AppendBigEndianUint16()`, `AppendLittleEndianUint32()`, and other variations in `FrameBuilder` with a single templated method: `AppendUint()`. - Replaced `ReadBigEndianUint16()`, `ReadLittleEndianUint32()`, and other variations in `FrameData` with a single templated method: `ReadUint()`. - Updated all callers in `Lowpan` and `Mac` modules to use the new templated encoding APIs. - Updated `test_frame_builder.cpp` to validate the new syntax. --- src/core/common/encoding.hpp | 32 +++++++++++++++++++ src/core/common/frame_builder.cpp | 23 +++++++------- src/core/common/frame_builder.hpp | 42 ++++++------------------ src/core/common/frame_data.cpp | 46 +++++++-------------------- src/core/common/frame_data.hpp | 53 +++++++------------------------ src/core/mac/mac_frame.cpp | 10 +++--- src/core/thread/lowpan.cpp | 28 ++++++++-------- tests/unit/test_frame_builder.cpp | 14 +++++--- 8 files changed, 105 insertions(+), 143 deletions(-) diff --git a/src/core/common/encoding.hpp b/src/core/common/encoding.hpp index d0e6b3152..1b26b1234 100644 --- a/src/core/common/encoding.hpp +++ b/src/core/common/encoding.hpp @@ -427,6 +427,38 @@ template <> inline void Write(uint64_t aValue, uint8_t *aBuffer) { WriteUint64(a } // namespace LittleEndian +/** + * Represents the byte ordering (endianness) encoding. + */ +enum Encoding : uint8_t +{ + kBigEndian, ///< Big-endian. + kLittleEndian, ///< Little-endian. +}; + +/** + * Swaps the byte order of a given integer value from the host representation to the specified encoding, + * or vice-versa. + * + * @tparam kEncoding The target encoding (big or little endian). + * @tparam UintType The unsigned integer type. + * + * @param[in] aValue The value to swap. + * + * @returns The swapped value. + */ +template UintType HostSwap(UintType aValue); + +template <> inline uint8_t HostSwap(uint8_t aValue) { return aValue; } +template <> inline uint16_t HostSwap(uint16_t aValue) { return BigEndian::HostSwap16(aValue); } +template <> inline uint32_t HostSwap(uint32_t aValue) { return BigEndian::HostSwap32(aValue); } +template <> inline uint64_t HostSwap(uint64_t aValue) { return BigEndian::HostSwap64(aValue); } + +template <> inline uint8_t HostSwap(uint8_t aValue) { return aValue; } +template <> inline uint16_t HostSwap(uint16_t aValue) { return LittleEndian::HostSwap16(aValue); } +template <> inline uint32_t HostSwap(uint32_t aValue) { return LittleEndian::HostSwap32(aValue); } +template <> inline uint64_t HostSwap(uint64_t aValue) { return LittleEndian::HostSwap64(aValue); } + } // namespace ot #endif // OT_CORE_COMMON_ENCODING_HPP_ diff --git a/src/core/common/frame_builder.cpp b/src/core/common/frame_builder.cpp index 4eba998f6..563bc7ef6 100644 --- a/src/core/common/frame_builder.cpp +++ b/src/core/common/frame_builder.cpp @@ -38,6 +38,7 @@ #include "common/code_utils.hpp" #include "common/debug.hpp" #include "common/encoding.hpp" +#include "common/type_traits.hpp" #if OPENTHREAD_FTD || OPENTHREAD_MTD #include "common/message.hpp" @@ -54,19 +55,19 @@ void FrameBuilder::Init(void *aBuffer, uint16_t aMaxLength) Error FrameBuilder::AppendUint8(uint8_t aUint8) { return Append(aUint8); } -Error FrameBuilder::AppendBigEndianUint16(uint16_t aUint16) { return Append(BigEndian::HostSwap16(aUint16)); } - -Error FrameBuilder::AppendBigEndianUint32(uint32_t aUint32) { return Append(BigEndian::HostSwap32(aUint32)); } - -Error FrameBuilder::AppendLittleEndianUint16(uint16_t aUint16) +template Error FrameBuilder::AppendUint(UintType aUint) { - return Append(LittleEndian::HostSwap16(aUint16)); + static_assert(TypeTraits::IsUint::kValue, "UintType is not valid, it must be an unsigned int"); + + return Append(HostSwap(aUint)); } -Error FrameBuilder::AppendLittleEndianUint32(uint32_t aUint32) -{ - return Append(LittleEndian::HostSwap32(aUint32)); -} +template Error FrameBuilder::AppendUint(uint16_t aUint); +template Error FrameBuilder::AppendUint(uint32_t aUint); +template Error FrameBuilder::AppendUint(uint64_t aUint); +template Error FrameBuilder::AppendUint(uint16_t aUint); +template Error FrameBuilder::AppendUint(uint32_t aUint); +template Error FrameBuilder::AppendUint(uint64_t aUint); Error FrameBuilder::AppendBytes(const void *aBuffer, uint16_t aLength) { @@ -90,7 +91,7 @@ Error FrameBuilder::AppendMacAddress(const Mac::Address &aMacAddress) break; case Mac::Address::kTypeShort: - error = AppendLittleEndianUint16(aMacAddress.GetShort()); + error = AppendUint(aMacAddress.GetShort()); break; case Mac::Address::kTypeExtended: diff --git a/src/core/common/frame_builder.hpp b/src/core/common/frame_builder.hpp index 2fb733cd9..09cfbe9fe 100644 --- a/src/core/common/frame_builder.hpp +++ b/src/core/common/frame_builder.hpp @@ -36,6 +36,7 @@ #include "openthread-core-config.h" +#include "common/encoding.hpp" #include "common/error.hpp" #include "common/type_traits.hpp" #include "mac/mac_types.hpp" @@ -119,44 +120,19 @@ public: Error AppendUint8(uint8_t aUint8); /** - * Appends an `uint16_t` value assuming big endian encoding to the `FrameBuilder`. + * Appends an integer value with a specified encoding to the `FrameBuilder`. * - * @param[in] aUint16 The `uint16_t` value to append. + * The value is converted to the specified @p kEncoding byte order before being appended. + * + * @tparam kEncoding The encoding to use (big or little endian). + * @tparam UintType The unsigned integer type. + * + * @param[in] aUint The integer value to append. * * @retval kErrorNone Successfully appended the value. * @retval kErrorNoBufs Insufficient available buffers. */ - Error AppendBigEndianUint16(uint16_t aUint16); - - /** - * Appends an `uint32_t` value assuming big endian encoding to the `FrameBuilder`. - * - * @param[in] aUint32 The `uint32_t` value to append. - * - * @retval kErrorNone Successfully appended the value. - * @retval kErrorNoBufs Insufficient available buffers. - */ - Error AppendBigEndianUint32(uint32_t aUint32); - - /** - * Appends an `uint16_t` value assuming little endian encoding to the `FrameBuilder`. - * - * @param[in] aUint16 The `uint16_t` value to append. - * - * @retval kErrorNone Successfully appended the value. - * @retval kErrorNoBufs Insufficient available buffers. - */ - Error AppendLittleEndianUint16(uint16_t aUint16); - - /** - * Appends an `uint32_t` value assuming little endian encoding to the `FrameBuilder`. - * - * @param[in] aUint32 The `uint32_t` value to append. - * - * @retval kErrorNone Successfully appended the value. - * @retval kErrorNoBufs Insufficient available buffers. - */ - Error AppendLittleEndianUint32(uint32_t aUint32); + template Error AppendUint(UintType aUint); /** * Appends bytes from a given buffer to the `FrameBuilder`. diff --git a/src/core/common/frame_data.cpp b/src/core/common/frame_data.cpp index a9c390003..890d4784e 100644 --- a/src/core/common/frame_data.cpp +++ b/src/core/common/frame_data.cpp @@ -40,49 +40,25 @@ namespace ot { Error FrameData::ReadUint8(uint8_t &aUint8) { return ReadBytes(&aUint8, sizeof(uint8_t)); } -Error FrameData::ReadBigEndianUint16(uint16_t &aUint16) +template Error FrameData::ReadUint(UintType &aUint) { Error error; - SuccessOrExit(error = ReadBytes(&aUint16, sizeof(uint16_t))); - aUint16 = BigEndian::HostSwap16(aUint16); + static_assert(TypeTraits::IsUint::kValue, "UintType is not valid, it must be an unsigned int"); + + SuccessOrExit(error = ReadBytes(&aUint, sizeof(UintType))); + aUint = HostSwap(aUint); exit: return error; } -Error FrameData::ReadBigEndianUint32(uint32_t &aUint32) -{ - Error error; - - SuccessOrExit(error = ReadBytes(&aUint32, sizeof(uint32_t))); - aUint32 = BigEndian::HostSwap32(aUint32); - -exit: - return error; -} - -Error FrameData::ReadLittleEndianUint16(uint16_t &aUint16) -{ - Error error; - - SuccessOrExit(error = ReadBytes(&aUint16, sizeof(uint16_t))); - aUint16 = LittleEndian::HostSwap16(aUint16); - -exit: - return error; -} - -Error FrameData::ReadLittleEndianUint32(uint32_t &aUint32) -{ - Error error; - - SuccessOrExit(error = ReadBytes(&aUint32, sizeof(uint32_t))); - aUint32 = LittleEndian::HostSwap32(aUint32); - -exit: - return error; -} +template Error FrameData::ReadUint(uint16_t &aUint); +template Error FrameData::ReadUint(uint32_t &aUint); +template Error FrameData::ReadUint(uint64_t &aUint); +template Error FrameData::ReadUint(uint16_t &aUint); +template Error FrameData::ReadUint(uint32_t &aUint); +template Error FrameData::ReadUint(uint64_t &aUint); Error FrameData::ReadBytes(void *aBuffer, uint16_t aLength) { diff --git a/src/core/common/frame_data.hpp b/src/core/common/frame_data.hpp index 419ca80f4..bba80592e 100644 --- a/src/core/common/frame_data.hpp +++ b/src/core/common/frame_data.hpp @@ -37,6 +37,7 @@ #include "openthread-core-config.h" #include "common/data.hpp" +#include "common/encoding.hpp" #include "common/type_traits.hpp" namespace ot { @@ -74,52 +75,22 @@ public: Error ReadUint8(uint8_t &aUint8); /** - * Reads an `uint16_t` value assuming big endian encoding from the `FrameData`. + * Reads an integer value with a specified encoding from the `FrameData`. + * + * The value is read from the frame data, converted from the specified @p kEncoding byte order to + * host byte order, and returned in @p aUint. * * If read successfully, the `FrameData` is updated to skip over the read content. * - * @param[out] aUint16 A reference to an `uint16_t` to return the read value. + * @tparam kEncoding The encoding of the integer in the frame data (big or little endian). + * @tparam UintType The unsigned integer type. * - * @retval kErrorNone Successfully read `uint16_t` value and skipped over it. - * @retval kErrorParse Not enough bytes remaining to read. + * @param[out] aUint A reference to the integer to return the read value. + * + * @retval kErrorNone Successfully read the value and skipped over it. + * @retval kErrorParse Not enough bytes remaining to read. */ - Error ReadBigEndianUint16(uint16_t &aUint16); - - /** - * Reads an `uint32_t` value assuming big endian encoding from the `FrameData`. - * - * If read successfully, the `FrameData` is updated to skip over the read content. - * - * @param[out] aUint32 A reference to an `uint32_t` to return the read value. - * - * @retval kErrorNone Successfully read `uint32_t` value and skipped over it. - * @retval kErrorParse Not enough bytes remaining to read. - */ - Error ReadBigEndianUint32(uint32_t &aUint32); - - /** - * Reads an `uint16_t` value assuming little endian encoding from the `FrameData`. - * - * If read successfully, the `FrameData` is updated to skip over the read content. - * - * @param[out] aUint16 A reference to an `uint16_t` to return the read value. - * - * @retval kErrorNone Successfully read `uint16_t` value and skipped over it. - * @retval kErrorParse Not enough bytes remaining to read. - */ - Error ReadLittleEndianUint16(uint16_t &aUint16); - - /** - * Reads an `uint32_t` value assuming little endian encoding from the `FrameData`. - * - * If read successfully, the `FrameData` is updated to skip over the read content. - * - * @param[out] aUint32 A reference to an `uint32_t` to return the read value. - * - * @retval kErrorNone Successfully read `uint32_t` value and skipped over it. - * @retval kErrorParse Not enough bytes remaining to read. - */ - Error ReadLittleEndianUint32(uint32_t &aUint32); + template Error ReadUint(UintType &aUint); /** * Reads a given number of bytes from the `FrameData`. diff --git a/src/core/mac/mac_frame.cpp b/src/core/mac/mac_frame.cpp index ce00d30b7..36651bfdf 100644 --- a/src/core/mac/mac_frame.cpp +++ b/src/core/mac/mac_frame.cpp @@ -185,7 +185,7 @@ void TxFrame::Info::PrepareHeadersIn(TxFrame &aTxFrame) const #endif builder.Init(aTxFrame.mPsdu, aTxFrame.GetMtu()); - IgnoreError(builder.AppendLittleEndianUint16(fcf)); + IgnoreError(builder.AppendUint(fcf)); if (IsSequencePresent(fcf)) { @@ -194,14 +194,14 @@ void TxFrame::Info::PrepareHeadersIn(TxFrame &aTxFrame) const if (IsDstPanIdPresent(fcf)) { - IgnoreError(builder.AppendLittleEndianUint16(mPanIds.GetDestination())); + IgnoreError(builder.AppendUint(mPanIds.GetDestination())); } IgnoreError(builder.AppendMacAddress(mAddrs.mDestination)); if (IsSrcPanIdPresent(fcf)) { - IgnoreError(builder.AppendLittleEndianUint16(mPanIds.GetSource())); + IgnoreError(builder.AppendUint(mPanIds.GetSource())); } IgnoreError(builder.AppendMacAddress(mAddrs.mSource)); @@ -1583,8 +1583,8 @@ Error TxFrame::GenerateWakeupFrame(PanId aPanId, const WakeupRequest &aWakeupReq builder.Init(mPsdu, GetMtu()); - IgnoreError(builder.AppendLittleEndianUint16(fcf)); - IgnoreError(builder.AppendLittleEndianUint16(aPanId)); + IgnoreError(builder.AppendUint(fcf)); + IgnoreError(builder.AppendUint(aPanId)); IgnoreError(builder.AppendMacAddress(dest)); IgnoreError(builder.AppendMacAddress(aSource)); diff --git a/src/core/thread/lowpan.cpp b/src/core/thread/lowpan.cpp index 06f49cd91..390eeca02 100644 --- a/src/core/thread/lowpan.cpp +++ b/src/core/thread/lowpan.cpp @@ -122,7 +122,7 @@ Error Lowpan::CompressSourceIid(const Mac::Address &aMacAddr, else if (aIpAddr.GetIid().IsLocator()) { aHcCtl |= kHcSrcAddrMode2; - error = aFrameBuilder.AppendBigEndianUint16(aIpAddr.GetIid().GetLocator()); + error = aFrameBuilder.AppendUint(aIpAddr.GetIid().GetLocator()); } else { @@ -151,7 +151,7 @@ Error Lowpan::CompressDestinationIid(const Mac::Address &aMacAddr, else if (aIpAddr.GetIid().IsLocator()) { aHcCtl |= kHcDstAddrMode2; - error = aFrameBuilder.AppendBigEndianUint16(aIpAddr.GetIid().GetLocator()); + error = aFrameBuilder.AppendUint(aIpAddr.GetIid().GetLocator()); } else { @@ -274,7 +274,7 @@ Error Lowpan::Compress(Message &aMessage, // Lowpan HC Control Bits hcCtlOffset = aFrameBuilder.GetLength(); - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(hcCtl)); + SuccessOrExit(error = aFrameBuilder.AppendUint(hcCtl)); // Context Identifier if (srcContext.GetContextId() != 0 || dstContext.GetContextId() != 0) @@ -543,12 +543,12 @@ Error Lowpan::CompressUdp(Message &aMessage, FrameBuilder &aFrameBuilder) { SuccessOrExit(error = aFrameBuilder.AppendUint8(kUdpDispatch | 2)); SuccessOrExit(error = aFrameBuilder.AppendUint8(source & 0xff)); - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(destination)); + SuccessOrExit(error = aFrameBuilder.AppendUint(destination)); } else if ((destination & 0xff00) == 0xf000) { SuccessOrExit(error = aFrameBuilder.AppendUint8(kUdpDispatch | 1)); - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(source)); + SuccessOrExit(error = aFrameBuilder.AppendUint(source)); SuccessOrExit(error = aFrameBuilder.AppendUint8(destination & 0xff)); } else @@ -557,7 +557,7 @@ Error Lowpan::CompressUdp(Message &aMessage, FrameBuilder &aFrameBuilder) SuccessOrExit(error = aFrameBuilder.AppendBytes(&udpHeader, Ip6::UdpHeader::kLengthFieldOffset)); } - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(udpHeader.GetChecksum())); + SuccessOrExit(error = aFrameBuilder.AppendUint(udpHeader.GetChecksum())); exit: if (error != kErrorNone) @@ -623,7 +623,7 @@ Error Lowpan::DecompressBaseHeader(Ip6::Header &aIp6Header, Context dstContext; uint8_t nextHeader; - SuccessOrExit(aFrameData.ReadBigEndianUint16(hcCtl)); + SuccessOrExit(aFrameData.ReadUint(hcCtl)); // check Dispatch bits VerifyOrExit((hcCtl & kHcDispatchMask) == kHcDispatch); @@ -912,12 +912,12 @@ Error Lowpan::DecompressUdpHeader(Ip6::UdpHeader &aUdpHeader, FrameData &aFrameD switch (udpCtl & kUdpPortMask) { case 0: - SuccessOrExit(aFrameData.ReadBigEndianUint16(srcPort)); - SuccessOrExit(aFrameData.ReadBigEndianUint16(dstPort)); + SuccessOrExit(aFrameData.ReadUint(srcPort)); + SuccessOrExit(aFrameData.ReadUint(dstPort)); break; case 1: - SuccessOrExit(aFrameData.ReadBigEndianUint16(srcPort)); + SuccessOrExit(aFrameData.ReadUint(srcPort)); SuccessOrExit(aFrameData.ReadUint8(byte)); dstPort = (0xf000 | byte); break; @@ -925,7 +925,7 @@ Error Lowpan::DecompressUdpHeader(Ip6::UdpHeader &aUdpHeader, FrameData &aFrameD case 2: SuccessOrExit(aFrameData.ReadUint8(byte)); srcPort = (0xf000 | byte); - SuccessOrExit(aFrameData.ReadBigEndianUint16(dstPort)); + SuccessOrExit(aFrameData.ReadUint(dstPort)); break; case 3: @@ -946,7 +946,7 @@ Error Lowpan::DecompressUdpHeader(Ip6::UdpHeader &aUdpHeader, FrameData &aFrameD { uint16_t checksum; - SuccessOrExit(aFrameData.ReadBigEndianUint16(checksum)); + SuccessOrExit(aFrameData.ReadUint(checksum)); aUdpHeader.SetChecksum(checksum); } @@ -1195,8 +1195,8 @@ Error MeshHeader::AppendTo(FrameBuilder &aFrameBuilder) const SuccessOrExit(error = aFrameBuilder.AppendUint8(mHopsLeft)); } - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(mSource)); - SuccessOrExit(error = aFrameBuilder.AppendBigEndianUint16(mDestination)); + SuccessOrExit(error = aFrameBuilder.AppendUint(mSource)); + SuccessOrExit(error = aFrameBuilder.AppendUint(mDestination)); exit: return error; diff --git a/tests/unit/test_frame_builder.cpp b/tests/unit/test_frame_builder.cpp index 8c648e32e..74c30f417 100644 --- a/tests/unit/test_frame_builder.cpp +++ b/tests/unit/test_frame_builder.cpp @@ -49,6 +49,8 @@ void TestFrameBuilder(void) uint8_t buffer[kMaxBufferSize]; uint8_t zeroBuffer[kMaxBufferSize]; FrameBuilder frameBuilder; + uint16_t u16; + uint32_t u32; printf("TestFrameBuilder\n"); @@ -93,14 +95,17 @@ void TestFrameBuilder(void) VerifyOrQuit(!frameBuilder.CanAppend(sizeof(buffer) - sizeof(kData1) + 1)); SuccessOrQuit(frameBuilder.AppendUint8(0x01)); - SuccessOrQuit(frameBuilder.AppendBigEndianUint16(0x0203)); - SuccessOrQuit(frameBuilder.AppendLittleEndianUint16(0x0504)); + u16 = 0x203; + SuccessOrQuit(frameBuilder.AppendUint(u16)); + u16 = 0x0504; + SuccessOrQuit(frameBuilder.AppendUint(u16)); VerifyOrQuit(frameBuilder.GetLength() == sizeof(kData1) * 2); VerifyOrQuit(frameBuilder.GetBytes() == buffer); VerifyOrQuit(memcmp(buffer, kData1, sizeof(kData1)) == 0); VerifyOrQuit(memcmp(buffer + sizeof(kData1), kData1, sizeof(kData1)) == 0); - SuccessOrQuit(frameBuilder.AppendBigEndianUint32(0x01020304)); + u32 = 0x01020304; + SuccessOrQuit(frameBuilder.AppendUint(u32)); SuccessOrQuit(frameBuilder.AppendUint8(0x05)); VerifyOrQuit(frameBuilder.GetLength() == sizeof(kData1) * 3); VerifyOrQuit(frameBuilder.GetBytes() == buffer); @@ -124,7 +129,8 @@ void TestFrameBuilder(void) VerifyOrQuit(frameBuilder.GetLength() == 0); VerifyOrQuit(frameBuilder.GetMaxLength() == sizeof(buffer)); - SuccessOrQuit(frameBuilder.AppendLittleEndianUint32(0x04030201)); + u32 = 0x04030201; + SuccessOrQuit(frameBuilder.AppendUint(u32)); SuccessOrQuit(frameBuilder.AppendUint8(0x05)); VerifyOrQuit(frameBuilder.GetLength() == sizeof(kData1)); VerifyOrQuit(frameBuilder.GetBytes() == buffer);