From de059af0e8448e05961cdaacba7a884d7f7f095e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 28 Oct 2024 14:30:11 -0400 Subject: [PATCH] Post merge cleanup 36229 ember buffer encode (#36266) * Use chip::app::IsSignedAttributeType * Fix up put as well as naming for null value and comment * Fix up nullable tests * Test that you cannot decode a null value for non-nullable double and single * Allow NAN for non-nullable floating points * Add test case for non nullable bool * Restyle * Add a header for efr32 * Update src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp Co-authored-by: Boris Zbarsky * Update src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp Co-authored-by: Boris Zbarsky * Remove extra comment * Replace switch with if * Comment fix * Another try to make efr32 build of tests happy * Move includes around, to try to work around issues within efr32 compiles... * more updates, this time local efr32 compiles --------- Co-authored-by: Boris Zbarsky --- .../EmberAttributeDataBuffer.cpp | 55 ++++++------------- .../EmberAttributeDataBuffer.h | 2 +- .../tests/TestEmberAttributeDataBuffer.cpp | 45 ++++++++++++++- 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp b/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp index 2c764241ebf3d8..7eb2e9d2b1d2ea 100644 --- a/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp +++ b/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp @@ -384,29 +384,22 @@ CHIP_ERROR EmberAttributeDataBuffer::EncodeInteger(chip::TLV::TLVWriter & writer uint8_t raw_bytes[8]; - bool isSigned = (mAttributeType == ZCL_INT8S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT16S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT24S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT32S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT40S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT48S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT56S_ATTRIBUTE_TYPE) // - || (mAttributeType == ZCL_INT64S_ATTRIBUTE_TYPE); + const bool isSigned = IsSignedAttributeType(mAttributeType); unsigned byteCount; - uint64_t nullValue; + uint64_t nullValueAsU64; if (isSigned) { const SignedDecodeInfo info = GetSignedDecodeInfo(mAttributeType); byteCount = info.byteCount; - nullValue = static_cast(info.minValue); // just a bit cast for easy compare + nullValueAsU64 = static_cast(info.minValue); } else { const UnsignedDecodeInfo info = GetUnsignedDecodeInfo(mAttributeType); byteCount = info.byteCount; - nullValue = info.maxValue; + nullValueAsU64 = info.maxValue; } VerifyOrDie(sizeof(raw_bytes) >= byteCount); @@ -445,36 +438,21 @@ CHIP_ERROR EmberAttributeDataBuffer::EncodeInteger(chip::TLV::TLVWriter & writer value.uint_value = (value.uint_value & ~0xFFULL) | raw_bytes[i]; } - if (mIsNullable && (value.uint_value == nullValue)) + // We place the null value as either int_value or uint_value into a union that is + // bit-formatted as both int64 and uint64. When we define the nullValue, + // it is bitcast into u64 hence this comparison. This is ugly, however this + // code prioritizes code size over readability here. + if (mIsNullable && (value.uint_value == nullValueAsU64)) { - // MaxValue is used for NULL setting return writer.PutNull(tag); } - switch (mAttributeType) + if (isSigned) { - case ZCL_INT8U_ATTRIBUTE_TYPE: // Unsigned 8-bit integer - return writer.Put(tag, static_cast(value.uint_value)); - case ZCL_INT16U_ATTRIBUTE_TYPE: // Unsigned 16-bit integer - return writer.Put(tag, static_cast(value.uint_value)); - case ZCL_INT24U_ATTRIBUTE_TYPE: // Unsigned 24-bit integer - case ZCL_INT32U_ATTRIBUTE_TYPE: // Unsigned 32-bit integer - return writer.Put(tag, static_cast(value.uint_value)); - case ZCL_INT40U_ATTRIBUTE_TYPE: // Unsigned 40-bit integer - case ZCL_INT48U_ATTRIBUTE_TYPE: // Unsigned 48-bit integer - case ZCL_INT56U_ATTRIBUTE_TYPE: // Signed 56-bit integer - case ZCL_INT64U_ATTRIBUTE_TYPE: // Signed 64-bit integer - return writer.Put(tag, static_cast(value.uint_value)); - case ZCL_INT8S_ATTRIBUTE_TYPE: // Signed 8-bit integer - return writer.Put(tag, static_cast(value.int_value)); - case ZCL_INT16S_ATTRIBUTE_TYPE: // Signed 16-bit integer - return writer.Put(tag, static_cast(value.int_value)); - case ZCL_INT24S_ATTRIBUTE_TYPE: // Signed 24-bit integer - case ZCL_INT32S_ATTRIBUTE_TYPE: // Signed 32-bit integer - return writer.Put(tag, static_cast(value.int_value)); - default: - return writer.Put(tag, static_cast(value.int_value)); + return writer.Put(tag, value.int_value); } + + return writer.Put(tag, value.uint_value); } CHIP_ERROR EmberAttributeDataBuffer::Encode(chip::TLV::TLVWriter & writer, TLV::Tag tag) const @@ -497,10 +475,11 @@ CHIP_ERROR EmberAttributeDataBuffer::Encode(chip::TLV::TLVWriter & writer, TLV:: case 1: return writer.PutBoolean(tag, value != 0); case 0xFF: + VerifyOrReturnError(mIsNullable, CHIP_ERROR_INVALID_ARGUMENT); return writer.PutNull(tag); default: // Unknown types - return CHIP_ERROR_INCORRECT_STATE; + return CHIP_ERROR_INVALID_ARGUMENT; } } case ZCL_INT8U_ATTRIBUTE_TYPE: // Unsigned 8-bit integer @@ -531,7 +510,7 @@ CHIP_ERROR EmberAttributeDataBuffer::Encode(chip::TLV::TLVWriter & writer, TLV:: { return endianReader.StatusCode(); } - if (NumericAttributeTraits::IsNullValue(value.value)) + if (mIsNullable && NumericAttributeTraits::IsNullValue(value.value)) { return writer.PutNull(tag); } @@ -548,7 +527,7 @@ CHIP_ERROR EmberAttributeDataBuffer::Encode(chip::TLV::TLVWriter & writer, TLV:: { return endianReader.StatusCode(); } - if (NumericAttributeTraits::IsNullValue(value.value)) + if (mIsNullable && NumericAttributeTraits::IsNullValue(value.value)) { return writer.PutNull(tag); } diff --git a/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.h b/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.h index c3d7acfcafb72b..f4a2de268591d4 100644 --- a/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.h +++ b/src/app/codegen-data-model-provider/EmberAttributeDataBuffer.h @@ -85,7 +85,7 @@ class EmberAttributeDataBuffer /// Takes into account internal mIsNullable. CHIP_ERROR DecodeSignedInteger(chip::TLV::TLVReader & reader, EndianWriter & writer); - /// Encodes the UNSIGNED integer into `writer`. + /// Encodes the given integer into `writer`. /// Takes into account internal mIsNullable. CHIP_ERROR EncodeInteger(chip::TLV::TLVWriter & writer, TLV::Tag tag, EndianReader & reader) const; diff --git a/src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp b/src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp index dc83773454d1ff..04e8b94dcce809 100644 --- a/src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp +++ b/src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp @@ -16,6 +16,8 @@ */ #include +#include + #include #include @@ -107,6 +109,28 @@ bool IsEqual(const T & a, const T & b) return a == b; } +template <> +bool IsEqual(const float & a, const float & b) +{ + if (std::isnan(a) && std::isnan(b)) + { + return true; + } + + return a == b; +} + +template <> +bool IsEqual(const double & a, const double & b) +{ + if (std::isnan(a) && std::isnan(b)) + { + return true; + } + + return a == b; +} + template <> bool IsEqual(const ByteSpan & a, const ByteSpan & b) { @@ -756,7 +780,7 @@ TEST(TestEmberAttributeBuffer, TestDecodeFailures) { // Bad boolean data EncodeTester tester(CreateFakeMeta(ZCL_BOOLEAN_ATTRIBUTE_TYPE, false /* nullable */)); - EXPECT_EQ(tester.TryDecode(true, { 123 }), CHIP_ERROR_INCORRECT_STATE); + EXPECT_EQ(tester.TryDecode(true, { 123 }), CHIP_ERROR_INVALID_ARGUMENT); } } @@ -1097,6 +1121,13 @@ TEST(TestEmberAttributeBuffer, TestDecodeBool) EXPECT_TRUE(tester.TryDecode>(false, { 0 }).IsSuccess()); EXPECT_TRUE(tester.TryDecode>(DataModel::NullNullable, { 0xFF }).IsSuccess()); } + + { + // Boolean that is NOT nullable + EncodeTester tester(CreateFakeMeta(ZCL_BOOLEAN_ATTRIBUTE_TYPE, false /* nullable */)); + EXPECT_EQ(tester.TryDecode>(DataModel::NullNullable, { 0xFF }), CHIP_ERROR_INVALID_ARGUMENT); + EXPECT_EQ(tester.TryDecode(true, { 0xFF }), CHIP_ERROR_INVALID_ARGUMENT); + } } TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint) @@ -1120,6 +1151,12 @@ TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint) EXPECT_TRUE(tester.TryDecode>(DataModel::NullNullable, { 0, 0, 0xC0, 0x7F }).IsSuccess()); } + { + EncodeTester tester(CreateFakeMeta(ZCL_SINGLE_ATTRIBUTE_TYPE, false /* nullable */)); + // non-nullable float + EXPECT_TRUE(tester.TryDecode(std::nanf("0"), { 0, 0, 0xC0, 0x7F }).IsSuccess()); + } + { EncodeTester tester(CreateFakeMeta(ZCL_DOUBLE_ATTRIBUTE_TYPE, false /* nullable */)); EXPECT_TRUE(tester.TryDecode(123.55, { 0x33, 0x33, 0x33, 0x33, 0x33, 0xE3, 0x5E, 0x40 }).IsSuccess()); @@ -1133,4 +1170,10 @@ TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint) EXPECT_TRUE( tester.TryDecode>(DataModel::NullNullable, { 0, 0, 0, 0, 0, 0, 0xF8, 0x7F }).IsSuccess()); } + + { + EncodeTester tester(CreateFakeMeta(ZCL_DOUBLE_ATTRIBUTE_TYPE, false /* nullable */)); + // non-nullable double + EXPECT_TRUE(tester.TryDecode(std::nan("0"), { 0, 0, 0, 0, 0, 0, 0xF8, 0x7F }).IsSuccess()); + } }