Skip to content

Commit

Permalink
Post merge cleanup 36229 ember buffer encode (project-chip#36266)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

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

Co-authored-by: Boris Zbarsky <[email protected]>

* 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 <[email protected]>
  • Loading branch information
@andy31415 @bzbarsky-apple @yyzhong-g
2 people authored and yyzhong-g committed Dec 11, 2024
1 parent d4ce1f4 commit de059af
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 40 deletions.
55 changes: 17 additions & 38 deletions src/app/codegen-data-model-provider/EmberAttributeDataBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/codegen-data-model-provider/EmberAttributeDataBuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
45 changes: 44 additions & 1 deletion src/app/codegen-data-model-provider/tests/TestEmberAttributeDataBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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());
Expand All @@ -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());
}
}

0 comments on commit de059af

Please sign in to comment.