Post merge cleanup 36229 ember buffer encode (project-chip#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 <bzbarsky@apple.com>

* Update src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* 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 <bzbarsky@apple.com>

Author: andy31415

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<uint64_t>(info.minValue); // just a bit cast for easy compare
+        nullValueAsU64              = static_cast<uint64_t>(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<uint8_t>(value.uint_value));
-    case ZCL_INT16U_ATTRIBUTE_TYPE: // Unsigned 16-bit integer
-        return writer.Put(tag, static_cast<uint16_t>(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<uint32_t>(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<uint64_t>(value.uint_value));
-    case ZCL_INT8S_ATTRIBUTE_TYPE: // Signed 8-bit integer
-        return writer.Put(tag, static_cast<int8_t>(value.int_value));
-    case ZCL_INT16S_ATTRIBUTE_TYPE: // Signed 16-bit integer
-        return writer.Put(tag, static_cast<int16_t>(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<int32_t>(value.int_value));
-    default:
-        return writer.Put(tag, static_cast<int64_t>(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<float>::IsNullValue(value.value))
+        if (mIsNullable && NumericAttributeTraits<float>::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<double>::IsNullValue(value.value))
+        if (mIsNullable && NumericAttributeTraits<double>::IsNullValue(value.value))
         {
             return writer.PutNull(tag);
         }

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;
 

src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp

@@ -16,6 +16,8 @@
  */
 #include <pw_unit_test/framework.h>
 
+#include <cmath>
+
 #include <app/codegen-data-model-provider/EmberAttributeDataBuffer.h>
 
 #include <app-common/zap-generated/attribute-type.h>
@@ -107,6 +109,28 @@ bool IsEqual(const T & a, const T & b)
     return a == b;
 }
 
+template <>
+bool IsEqual<float>(const float & a, const float & b)
+{
+    if (std::isnan(a) && std::isnan(b))
+    {
+        return true;
+    }
+
+    return a == b;
+}
+
+template <>
+bool IsEqual<double>(const double & a, const double & b)
+{
+    if (std::isnan(a) && std::isnan(b))
+    {
+        return true;
+    }
+
+    return a == b;
+}
+
 template <>
 bool IsEqual<ByteSpan>(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<bool>(true, { 123 }), CHIP_ERROR_INCORRECT_STATE);
+        EXPECT_EQ(tester.TryDecode<bool>(true, { 123 }), CHIP_ERROR_INVALID_ARGUMENT);
     }
 }
 
@@ -1097,6 +1121,13 @@ TEST(TestEmberAttributeBuffer, TestDecodeBool)
         EXPECT_TRUE(tester.TryDecode<DataModel::Nullable<bool>>(false, { 0 }).IsSuccess());
         EXPECT_TRUE(tester.TryDecode<DataModel::Nullable<bool>>(DataModel::NullNullable, { 0xFF }).IsSuccess());
     }
+
+    {
+        // Boolean that is NOT nullable
+        EncodeTester tester(CreateFakeMeta(ZCL_BOOLEAN_ATTRIBUTE_TYPE, false /* nullable */));
+        EXPECT_EQ(tester.TryDecode<DataModel::Nullable<bool>>(DataModel::NullNullable, { 0xFF }), CHIP_ERROR_INVALID_ARGUMENT);
+        EXPECT_EQ(tester.TryDecode<bool>(true, { 0xFF }), CHIP_ERROR_INVALID_ARGUMENT);
+    }
 }
 
 TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint)
@@ -1120,6 +1151,12 @@ TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint)
         EXPECT_TRUE(tester.TryDecode<DataModel::Nullable<float>>(DataModel::NullNullable, { 0, 0, 0xC0, 0x7F }).IsSuccess());
     }
 
+    {
+        EncodeTester tester(CreateFakeMeta(ZCL_SINGLE_ATTRIBUTE_TYPE, false /* nullable */));
+        // non-nullable float
+        EXPECT_TRUE(tester.TryDecode<float>(std::nanf("0"), { 0, 0, 0xC0, 0x7F }).IsSuccess());
+    }
+
     {
         EncodeTester tester(CreateFakeMeta(ZCL_DOUBLE_ATTRIBUTE_TYPE, false /* nullable */));
         EXPECT_TRUE(tester.TryDecode<double>(123.55, { 0x33, 0x33, 0x33, 0x33, 0x33, 0xE3, 0x5E, 0x40 }).IsSuccess());
@@ -1133,4 +1170,10 @@ TEST(TestEmberAttributeBuffer, TestDecodeFloatingPoint)
         EXPECT_TRUE(
             tester.TryDecode<DataModel::Nullable<double>>(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<double>(std::nan("0"), { 0, 0, 0, 0, 0, 0, 0xF8, 0x7F }).IsSuccess());
+    }
 }