From 0703a1639c3d63c80ece42fda65e24a4e3f932f4 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Fri, 23 Dec 2022 16:55:46 +0100 Subject: [PATCH 1/4] [tlv] Encapsulate Tag encoding Make the internal representation of Tag private so that it is easier and safer to change it. Signed-off-by: Damian Krolik --- src/lib/core/CHIPTLVReader.cpp | 6 +- src/lib/core/CHIPTLVTags.h | 125 +++++++++++++++++++-------------- src/lib/core/CHIPTLVWriter.cpp | 27 ++++--- 3 files changed, 86 insertions(+), 72 deletions(-) diff --git a/src/lib/core/CHIPTLVReader.cpp b/src/lib/core/CHIPTLVReader.cpp index 9a95a42779e9dc..5d175eb35b7108 100644 --- a/src/lib/core/CHIPTLVReader.cpp +++ b/src/lib/core/CHIPTLVReader.cpp @@ -749,7 +749,7 @@ CHIP_ERROR TLVReader::VerifyElement() } else { - if (mElemTag == UnknownImplicitTag) + if (mElemTag == UnknownImplicitTag()) return CHIP_ERROR_UNKNOWN_IMPLICIT_TLV_TAG; switch (mContainerType) { @@ -806,11 +806,11 @@ Tag TLVReader::ReadTag(TLVTagControl tagControl, const uint8_t *& p) const return CommonTag(LittleEndian::Read32(p)); case TLVTagControl::ImplicitProfile_2Bytes: if (ImplicitProfileId == kProfileIdNotSpecified) - return UnknownImplicitTag; + return UnknownImplicitTag(); return ProfileTag(ImplicitProfileId, LittleEndian::Read16(p)); case TLVTagControl::ImplicitProfile_4Bytes: if (ImplicitProfileId == kProfileIdNotSpecified) - return UnknownImplicitTag; + return UnknownImplicitTag(); return ProfileTag(ImplicitProfileId, LittleEndian::Read32(p)); case TLVTagControl::FullyQualified_6Bytes: vendorId = LittleEndian::Read16(p); diff --git a/src/lib/core/CHIPTLVTags.h b/src/lib/core/CHIPTLVTags.h index 12de1d9e3ef2e1..cc7298909d7070 100644 --- a/src/lib/core/CHIPTLVTags.h +++ b/src/lib/core/CHIPTLVTags.h @@ -29,12 +29,40 @@ namespace chip { namespace TLV { -struct Tag +class Tag { - explicit constexpr Tag(uint64_t val) : mVal(val) {} - Tag() {} +public: + Tag() = default; + constexpr bool operator==(const Tag & other) const { return mVal == other.mVal; } constexpr bool operator!=(const Tag & other) const { return mVal != other.mVal; } + +private: + explicit constexpr Tag(uint64_t val) : mVal(val) {} + + friend constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum); + friend constexpr Tag ProfileTag(uint16_t vendorId, uint16_t profileNum, uint32_t tagNum); + friend constexpr Tag ContextTag(uint8_t tagNum); + friend constexpr Tag CommonTag(uint32_t tagNum); + friend constexpr Tag AnonymousTag(); + friend constexpr Tag UnknownImplicitTag(); + + friend constexpr uint32_t ProfileIdFromTag(Tag tag); + friend constexpr uint16_t VendorIdFromTag(Tag tag); + friend constexpr uint16_t ProfileNumFromTag(Tag tag); + friend constexpr uint32_t TagNumFromTag(Tag tag); + + friend constexpr bool IsProfileTag(Tag tag); + friend constexpr bool IsContextTag(Tag tag); + friend constexpr bool IsSpecialTag(Tag tag); + + static constexpr uint32_t kProfileIdShift = 32; + static constexpr uint32_t kVendorIdShift = 48; + static constexpr uint32_t kProfileNumShift = 32; + static constexpr uint32_t kSpecialTagProfileId = 0xFFFFFFFF; + static constexpr uint32_t kAnonymousTagNum = 0xFFFFFFFF; + static constexpr uint32_t kContextTagMaxNum = UINT8_MAX; + uint64_t mVal; }; @@ -50,20 +78,6 @@ enum TLVCommonProfiles kCommonProfileId = 0 }; -// TODO: Move to private namespace -enum TLVTagFields -{ - kProfileIdMask = 0xFFFFFFFF00000000ULL, - kProfileNumMask = 0x0000FFFF00000000ULL, - kVendorIdMask = 0xFFFF000000000000ULL, - kProfileIdShift = 32, - kVendorIdShift = 48, - kProfileNumShift = 32, - kTagNumMask = 0x00000000FFFFFFFFULL, - kSpecialTagMarker = 0xFFFFFFFF00000000ULL, - kContextTagMaxNum = UINT8_MAX -}; - // TODO: Move to private namespace enum class TLVTagControl : uint8_t { @@ -99,9 +113,9 @@ enum * @param[in] tagNum The profile-specific tag number assigned to the tag. * @return A 64-bit integer representing the tag. */ -inline constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum) +constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum) { - return Tag(((static_cast(profileId)) << kProfileIdShift) | tagNum); + return Tag((static_cast(profileId) << Tag::kProfileIdShift) | tagNum); } /** @@ -112,10 +126,10 @@ inline constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum) * @param[in] tagNum The profile-specific tag number assigned to the tag. * @return A 64-bit integer representing the tag. */ -inline constexpr Tag ProfileTag(uint16_t vendorId, uint16_t profileNum, uint32_t tagNum) +constexpr Tag ProfileTag(uint16_t vendorId, uint16_t profileNum, uint32_t tagNum) { - return Tag(((static_cast(vendorId)) << kVendorIdShift) | ((static_cast(profileNum)) << kProfileNumShift) | - tagNum); + return Tag((static_cast(vendorId) << Tag::kVendorIdShift) | + (static_cast(profileNum) << Tag::kProfileNumShift) | tagNum); } /** @@ -124,9 +138,9 @@ inline constexpr Tag ProfileTag(uint16_t vendorId, uint16_t profileNum, uint32_t * @param[in] tagNum The context-specific tag number assigned to the tag. * @return A 64-bit integer representing the tag. */ -inline constexpr Tag ContextTag(uint8_t tagNum) +constexpr Tag ContextTag(uint8_t tagNum) { - return Tag(kSpecialTagMarker | tagNum); + return ProfileTag(Tag::kSpecialTagProfileId, tagNum); } /** @@ -135,7 +149,7 @@ inline constexpr Tag ContextTag(uint8_t tagNum) * @param[in] tagNum The common profile tag number assigned to the tag. * @return A 64-bit integer representing the tag. */ -inline constexpr Tag CommonTag(uint32_t tagNum) +constexpr Tag CommonTag(uint32_t tagNum) { return ProfileTag(kCommonProfileId, tagNum); } @@ -143,12 +157,15 @@ inline constexpr Tag CommonTag(uint32_t tagNum) /** * A value signifying a TLV element that has no tag (i.e. an anonymous element). */ -inline constexpr Tag AnonymousTag() +constexpr Tag AnonymousTag() { - return Tag(kSpecialTagMarker | 0x00000000FFFFFFFFULL); + return ProfileTag(Tag::kSpecialTagProfileId, Tag::kAnonymousTagNum); +} + +constexpr Tag UnknownImplicitTag() +{ + return ProfileTag(Tag::kSpecialTagProfileId, 0xFFFFFFFE); } -// TODO: Move to private namespace -static constexpr Tag UnknownImplicitTag(kSpecialTagMarker | 0x00000000FFFFFFFEULL); /** * Returns the profile id from a TLV tag @@ -158,9 +175,22 @@ static constexpr Tag UnknownImplicitTag(kSpecialTagMarker | 0x00000000FFFFFFFEUL * @param[in] tag The API representation of a profile-specific TLV tag. * @return The profile id. */ -inline constexpr uint32_t ProfileIdFromTag(Tag tag) +constexpr uint32_t ProfileIdFromTag(Tag tag) { - return static_cast((tag.mVal & kProfileIdMask) >> kProfileIdShift); + return static_cast(tag.mVal >> Tag::kProfileIdShift); +} + +/** + * Returns the vendor id from a TLV tag + * + * @note The behavior of this function is undefined if the supplied tag is not a profile-specific tag. + * + * @param[in] tag The API representation of a profile-specific TLV tag. + * @return The associated vendor id. + */ +constexpr uint16_t VendorIdFromTag(Tag tag) +{ + return static_cast(tag.mVal >> Tag::kVendorIdShift); } /** @@ -171,9 +201,9 @@ inline constexpr uint32_t ProfileIdFromTag(Tag tag) * @param[in] tag The API representation of a profile-specific TLV tag. * @return The associated profile number. */ -inline constexpr uint16_t ProfileNumFromTag(Tag tag) +constexpr uint16_t ProfileNumFromTag(Tag tag) { - return static_cast((tag.mVal & kProfileNumMask) >> kProfileNumShift); + return static_cast(tag.mVal >> Tag::kProfileNumShift); } /** @@ -187,44 +217,31 @@ inline constexpr uint16_t ProfileNumFromTag(Tag tag) * @param[in] tag The API representation of a profile-specific or context-specific TLV tag. * @return The associated tag number. */ -inline constexpr uint32_t TagNumFromTag(Tag tag) -{ - return static_cast(tag.mVal & kTagNumMask); -} - -/** - * Returns the vendor id from a TLV tag - * - * @note The behavior of this function is undefined if the supplied tag is not a profile-specific tag. - * - * @param[in] tag The API representation of a profile-specific TLV tag. - * @return The associated vendor id. - */ -inline constexpr uint16_t VendorIdFromTag(Tag tag) +constexpr uint32_t TagNumFromTag(Tag tag) { - return static_cast((tag.mVal & kVendorIdMask) >> kVendorIdShift); + return static_cast(tag.mVal); } /** * Returns true of the supplied tag is a profile-specific tag. */ -inline constexpr bool IsProfileTag(Tag tag) +constexpr bool IsProfileTag(Tag tag) { - return (tag.mVal & kProfileIdMask) != kSpecialTagMarker; + return ProfileIdFromTag(tag) != Tag::kSpecialTagProfileId; } /** * Returns true if the supplied tag is a context-specific tag. */ -inline constexpr bool IsContextTag(Tag tag) +constexpr bool IsContextTag(Tag tag) { - return (tag.mVal & kProfileIdMask) == kSpecialTagMarker && TagNumFromTag(tag) <= kContextTagMaxNum; + return ProfileIdFromTag(tag) == Tag::kSpecialTagProfileId && TagNumFromTag(tag) <= Tag::kContextTagMaxNum; } // TODO: move to private namespace -inline constexpr bool IsSpecialTag(Tag tag) +constexpr bool IsSpecialTag(Tag tag) { - return (tag.mVal & kProfileIdMask) == kSpecialTagMarker; + return ProfileIdFromTag(tag) == Tag::kSpecialTagProfileId; } } // namespace TLV diff --git a/src/lib/core/CHIPTLVWriter.cpp b/src/lib/core/CHIPTLVWriter.cpp index 3f9743e23464f4..688f269086a824 100644 --- a/src/lib/core/CHIPTLVWriter.cpp +++ b/src/lib/core/CHIPTLVWriter.cpp @@ -629,24 +629,21 @@ CHIP_ERROR TLVWriter::WriteElementHead(TLVElementType elemType, Tag tag, uint64_ else p = stagingBuf; - if (IsSpecialTag(tag)) + if (IsContextTag(tag)) { - if (tagNum <= kContextTagMaxNum) - { - if (mContainerType != kTLVType_Structure && mContainerType != kTLVType_List) - return CHIP_ERROR_INVALID_TLV_TAG; + if (mContainerType != kTLVType_Structure && mContainerType != kTLVType_List) + return CHIP_ERROR_INVALID_TLV_TAG; - Write8(p, TLVTagControl::ContextSpecific | elemType); - Write8(p, static_cast(tagNum)); - } - else - { - if (elemType != TLVElementType::EndOfContainer && mContainerType != kTLVType_NotSpecified && - mContainerType != kTLVType_Array && mContainerType != kTLVType_List) - return CHIP_ERROR_INVALID_TLV_TAG; + Write8(p, TLVTagControl::ContextSpecific | elemType); + Write8(p, static_cast(tagNum)); + } + else if (IsSpecialTag(tag)) + { + if (elemType != TLVElementType::EndOfContainer && mContainerType != kTLVType_NotSpecified && + mContainerType != kTLVType_Array && mContainerType != kTLVType_List) + return CHIP_ERROR_INVALID_TLV_TAG; - Write8(p, TLVTagControl::Anonymous | elemType); - } + Write8(p, TLVTagControl::Anonymous | elemType); } else { From d2cd763ed6657c6d14f60410c3dd0796939a7cb7 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Fri, 23 Dec 2022 17:07:22 +0100 Subject: [PATCH 2/4] [tlv] Optimize code size of processing context tags TLV tag is represented as uint64_t that encodes the following components: - 16-bit vendor ID - 16-bit profile number - 32-bit tag number Context tags, which account for vast majority of tag usage in the SDK, are encoded as having both vendor ID and profile number equal to 0xFFFF. Anonymous tags are encoded in the same way, but using 0xFFFFFFFF tag number. This is correct because vendor IDs higher than 0xFFF0 shall not be assigned to real manufacturers, but constructing 0xFF... constants in hundreds of places adds non-negligible overhead to the flash usage. Encode profile ID in the negated form internally to optimize the code size when using special tags. --- src/lib/core/CHIPTLVTags.h | 45 +++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/src/lib/core/CHIPTLVTags.h b/src/lib/core/CHIPTLVTags.h index cc7298909d7070..4fef642b8c126b 100644 --- a/src/lib/core/CHIPTLVTags.h +++ b/src/lib/core/CHIPTLVTags.h @@ -47,6 +47,11 @@ class Tag friend constexpr Tag AnonymousTag(); friend constexpr Tag UnknownImplicitTag(); + // The following friend functions could be Tag class methods, but it turns out in some cases + // they may not be inlined and then passing the tag by argument/value results in smaller code + // than passing it by 'this' pointer. This can be worked around by applying 'always_inline' + // function attribute, but friend functions are likely a more portable solution. + friend constexpr uint32_t ProfileIdFromTag(Tag tag); friend constexpr uint16_t VendorIdFromTag(Tag tag); friend constexpr uint16_t ProfileNumFromTag(Tag tag); @@ -60,9 +65,23 @@ class Tag static constexpr uint32_t kVendorIdShift = 48; static constexpr uint32_t kProfileNumShift = 32; static constexpr uint32_t kSpecialTagProfileId = 0xFFFFFFFF; - static constexpr uint32_t kAnonymousTagNum = 0xFFFFFFFF; - static constexpr uint32_t kContextTagMaxNum = UINT8_MAX; + enum SpecialTagNumber : uint32_t + { + kContextTagMaxNum = UINT8_MAX, + kAnonymousTagNum, + kUnknownImplicitTagNum + }; + + // The API representation of the tag uses the following encoding: + // + // 63 47 31 + // +-----------------------+-----------------------+----------------------------------------------+ + // | Vendor id (negated) | Profile num (negated) | Tag number | + // +-----------------------+-----------------------+----------------------------------------------+ + // + // Vendor id and profile number are negated in order to optimize the code size when using + // context tags, the most commonly used tags in the SDK. uint64_t mVal; }; @@ -115,7 +134,7 @@ enum */ constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum) { - return Tag((static_cast(profileId) << Tag::kProfileIdShift) | tagNum); + return Tag((static_cast(~profileId) << Tag::kProfileIdShift) | tagNum); } /** @@ -128,12 +147,13 @@ constexpr Tag ProfileTag(uint32_t profileId, uint32_t tagNum) */ constexpr Tag ProfileTag(uint16_t vendorId, uint16_t profileNum, uint32_t tagNum) { - return Tag((static_cast(vendorId) << Tag::kVendorIdShift) | - (static_cast(profileNum) << Tag::kProfileNumShift) | tagNum); + constexpr uint32_t kVendorIdShift = Tag::kVendorIdShift - Tag::kProfileIdShift; + + return ProfileTag((static_cast(vendorId) << kVendorIdShift) | profileNum, tagNum); } /** - * Generates the API representation for of context-specific TLV tag + * Generates the API representation of a context-specific TLV tag * * @param[in] tagNum The context-specific tag number assigned to the tag. * @return A 64-bit integer representing the tag. @@ -162,9 +182,12 @@ constexpr Tag AnonymousTag() return ProfileTag(Tag::kSpecialTagProfileId, Tag::kAnonymousTagNum); } +/** + * An invalid tag that represents a TLV element decoding error due to unknown implicit profile id. + */ constexpr Tag UnknownImplicitTag() { - return ProfileTag(Tag::kSpecialTagProfileId, 0xFFFFFFFE); + return ProfileTag(Tag::kSpecialTagProfileId, Tag::kUnknownImplicitTagNum); } /** @@ -177,7 +200,7 @@ constexpr Tag UnknownImplicitTag() */ constexpr uint32_t ProfileIdFromTag(Tag tag) { - return static_cast(tag.mVal >> Tag::kProfileIdShift); + return ~static_cast(tag.mVal >> Tag::kProfileIdShift); } /** @@ -190,7 +213,9 @@ constexpr uint32_t ProfileIdFromTag(Tag tag) */ constexpr uint16_t VendorIdFromTag(Tag tag) { - return static_cast(tag.mVal >> Tag::kVendorIdShift); + constexpr uint32_t kVendorIdShift = Tag::kVendorIdShift - Tag::kProfileIdShift; + + return static_cast(ProfileIdFromTag(tag) >> kVendorIdShift); } /** @@ -203,7 +228,7 @@ constexpr uint16_t VendorIdFromTag(Tag tag) */ constexpr uint16_t ProfileNumFromTag(Tag tag) { - return static_cast(tag.mVal >> Tag::kProfileNumShift); + return static_cast(ProfileIdFromTag(tag)); } /** From 507b4943701cd05276220dc5fcfc626077e67cc7 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Mon, 2 Jan 2023 08:45:53 +0100 Subject: [PATCH 3/4] Fix build --- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 7725c9ab6e1f1b..e8211845e91cae 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1437,7 +1437,7 @@ + (id)CHIPEncodeAndDecodeNSObject:(id)object uint8_t buffer[1024]; writer.Init(buffer, sizeof(buffer)); - CHIP_ERROR error = originalData.Encode(writer, chip::TLV::Tag(1)); + CHIP_ERROR error = originalData.Encode(writer, chip::TLV::CommonTag(1)); if (error != CHIP_NO_ERROR) { MTR_LOG_ERROR("Error: Data encoding failed: %s", error.AsString()); return nil; @@ -1456,8 +1456,9 @@ + (id)CHIPEncodeAndDecodeNSObject:(id)object return nil; } __auto_type tag = reader.GetTag(); - if (tag != chip::TLV::Tag(1)) { - MTR_LOG_ERROR("Error: TLV reader did not read the tag correctly: %llu", tag.mVal); + if (tag != chip::TLV::CommonTag(1)) { + MTR_LOG_ERROR("Error: TLV reader did not read the tag correctly: %x.%u", chip::TLV::ProfileIdFromTag(tag), + chip::TLV::TagNumFromTag(tag)); return nil; } MTRDataValueDictionaryDecodableType decodedData; From 72e9d235d8eae40375e0ec707755c47ad4dd0eb7 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Wed, 4 Jan 2023 08:39:53 +0100 Subject: [PATCH 4/4] Code review --- src/lib/core/CHIPTLVTags.h | 28 ++++++++++++++-------------- src/lib/core/CHIPTLVWriter.cpp | 27 +++++++++++++++------------ 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/src/lib/core/CHIPTLVTags.h b/src/lib/core/CHIPTLVTags.h index 4fef642b8c126b..da8f91006fae48 100644 --- a/src/lib/core/CHIPTLVTags.h +++ b/src/lib/core/CHIPTLVTags.h @@ -32,6 +32,13 @@ namespace TLV { class Tag { public: + enum SpecialTagNumber : uint32_t + { + kContextTagMaxNum = UINT8_MAX, + kAnonymousTagNum, + kUnknownImplicitTagNum + }; + Tag() = default; constexpr bool operator==(const Tag & other) const { return mVal == other.mVal; } @@ -66,22 +73,15 @@ class Tag static constexpr uint32_t kProfileNumShift = 32; static constexpr uint32_t kSpecialTagProfileId = 0xFFFFFFFF; - enum SpecialTagNumber : uint32_t - { - kContextTagMaxNum = UINT8_MAX, - kAnonymousTagNum, - kUnknownImplicitTagNum - }; - - // The API representation of the tag uses the following encoding: + // The storage of the tag value uses the following encoding: // - // 63 47 31 - // +-----------------------+-----------------------+----------------------------------------------+ - // | Vendor id (negated) | Profile num (negated) | Tag number | - // +-----------------------+-----------------------+----------------------------------------------+ + // 63 47 31 + // +-------------------------------+-------------------------------+----------------------------------------------+ + // | Vendor id (bitwise-negated) | Profile num (bitwise-negated) | Tag number | + // +-------------------------------+-------------------------------+----------------------------------------------+ // - // Vendor id and profile number are negated in order to optimize the code size when using - // context tags, the most commonly used tags in the SDK. + // Vendor id and profile number are bitwise-negated in order to optimize the code size when + // using context tags, the most commonly used tags in the SDK. uint64_t mVal; }; diff --git a/src/lib/core/CHIPTLVWriter.cpp b/src/lib/core/CHIPTLVWriter.cpp index 688f269086a824..8a1eb7021edb66 100644 --- a/src/lib/core/CHIPTLVWriter.cpp +++ b/src/lib/core/CHIPTLVWriter.cpp @@ -629,21 +629,24 @@ CHIP_ERROR TLVWriter::WriteElementHead(TLVElementType elemType, Tag tag, uint64_ else p = stagingBuf; - if (IsContextTag(tag)) + if (IsSpecialTag(tag)) { - if (mContainerType != kTLVType_Structure && mContainerType != kTLVType_List) - return CHIP_ERROR_INVALID_TLV_TAG; + if (tagNum <= Tag::kContextTagMaxNum) + { + if (mContainerType != kTLVType_Structure && mContainerType != kTLVType_List) + return CHIP_ERROR_INVALID_TLV_TAG; - Write8(p, TLVTagControl::ContextSpecific | elemType); - Write8(p, static_cast(tagNum)); - } - else if (IsSpecialTag(tag)) - { - if (elemType != TLVElementType::EndOfContainer && mContainerType != kTLVType_NotSpecified && - mContainerType != kTLVType_Array && mContainerType != kTLVType_List) - return CHIP_ERROR_INVALID_TLV_TAG; + Write8(p, TLVTagControl::ContextSpecific | elemType); + Write8(p, static_cast(tagNum)); + } + else + { + if (elemType != TLVElementType::EndOfContainer && mContainerType != kTLVType_NotSpecified && + mContainerType != kTLVType_Array && mContainerType != kTLVType_List) + return CHIP_ERROR_INVALID_TLV_TAG; - Write8(p, TLVTagControl::Anonymous | elemType); + Write8(p, TLVTagControl::Anonymous | elemType); + } } else {