Skip to content

Commit

Permalink
Fix null handling for enum types. (#13091)
Browse files Browse the repository at this point in the history 
For enum types we ended up with an incorrect null representation via
NumericAttributeTraits, because while we called GetNullValue with the
template argument set to our underlying type the actual implementation
ignores the template argument: it's just there for the enable_if
tests.

As far as testing goes, the yaml tests being added would have caught
this if our attribute store interaction were actually using enums, but
that code switches to treating them as just bare ints very early,
before instantiating the NumericAttributeTraits template.  The unit
test does catch this problem, though.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Feb 8, 2022
1 parent 0cad142 commit 1498468
Show file tree
Hide file tree
Showing 5 changed files with 3,760 additions and 2,357 deletions.
154 changes: 154 additions & 0 deletions src/app/tests/TestNumericAttributeTraits.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,156 @@ using namespace chip::app;

namespace {

void Test_UINT8(nlTestSuite * apSuite, void * apContext)
{
// Unsigned 8-bit Integer : 1 byte, endianness does not matter.
using IntType = NumericAttributeTraits<uint8_t>;
using StorageType = typename IntType::StorageType;
using WorkingType = typename IntType::WorkingType;

StorageType sValue;
WorkingType wValue;
StorageType sNullValue;
WorkingType wNullValue;
const StorageType storageTestData = 17;
const WorkingType workingTestUnsignedNullValue = 0xFF;

// 1) Verify the size of the types
NL_TEST_ASSERT(apSuite, sizeof(sValue) == 1);
NL_TEST_ASSERT(apSuite, sizeof(wValue) >= 1);

// Initialize the Storage Value with the test-buffer
memcpy(&sValue, &storageTestData, sizeof(sValue));

// Convert the Storage Type to Working Type and
wValue = IntType::StorageToWorking(sValue);

// 2) Verify that the correct storage format has been used
NL_TEST_ASSERT(apSuite, wValue == 17);

StorageType sNewValue;

// Convert back to Storage Value
IntType::WorkingToStorage(wValue, sNewValue);

// 3) Verify that the bytes are located as intended
NL_TEST_ASSERT(apSuite, memcmp(&storageTestData, &sNewValue, sizeof(sNewValue)) == 0);

// Set Storage value to Null
IntType::SetNull(sNullValue);
wNullValue = IntType::StorageToWorking(sNullValue);
NL_TEST_ASSERT(apSuite, wNullValue == workingTestUnsignedNullValue);
NL_TEST_ASSERT(apSuite, (IntType::IsNullValue(sNullValue) == true));

// Verify that null values can fit into not nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(false, sNullValue) == true));

// Verify that null values can't fit into nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(true, sNullValue) == false));
}

void Test_SINT8(nlTestSuite * apSuite, void * apContext)
{
// Signed 8-bit Integer : 1 byte, endianness does not matter.
using IntType = NumericAttributeTraits<int8_t>;
using StorageType = typename IntType::StorageType;
using WorkingType = typename IntType::WorkingType;

StorageType sValue;
WorkingType wValue;
StorageType sNullValue;
WorkingType wNullValue;
const StorageType storageTestData = 17;
const WorkingType workingTestUnsignedNullValue = -128; // 0x80

// 1) Verify the size of the types
NL_TEST_ASSERT(apSuite, sizeof(sValue) == 1);
NL_TEST_ASSERT(apSuite, sizeof(wValue) >= 1);

// Initialize the Storage Value with the test-buffer
memcpy(&sValue, &storageTestData, sizeof(sValue));

// Convert the Storage Type to Working Type and
wValue = IntType::StorageToWorking(sValue);

// 2) Verify that the correct storage format has been used
NL_TEST_ASSERT(apSuite, wValue == 17);

StorageType sNewValue;

// Convert back to Storage Value
IntType::WorkingToStorage(wValue, sNewValue);

// 3) Verify that the bytes are located as intended
NL_TEST_ASSERT(apSuite, memcmp(&storageTestData, &sNewValue, sizeof(sNewValue)) == 0);

// Set Storage value to Null
IntType::SetNull(sNullValue);
wNullValue = IntType::StorageToWorking(sNullValue);
NL_TEST_ASSERT(apSuite, wNullValue == workingTestUnsignedNullValue);
NL_TEST_ASSERT(apSuite, (IntType::IsNullValue(sNullValue) == true));

// Verify that null values can fit into not nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(false, sNullValue) == true));

// Verify that null values can't fit into nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(true, sNullValue) == false));
}

enum class SimpleEnum : uint8_t
{
kZero = 0,
kOne = 1,
};

void Test_SimpleEnum(nlTestSuite * apSuite, void * apContext)
{
// Unsigned 8-bit Integer : 1 byte, endianness does not matter.
using IntType = NumericAttributeTraits<SimpleEnum>;
using StorageType = typename IntType::StorageType;
using WorkingType = typename IntType::WorkingType;

StorageType sValue;
WorkingType wValue;
StorageType sNullValue;
WorkingType wNullValue;
const StorageType storageTestData = SimpleEnum::kOne;
const WorkingType workingTestUnsignedNullValue = static_cast<SimpleEnum>(0xFF);

// 1) Verify the size of the types
NL_TEST_ASSERT(apSuite, sizeof(sValue) == 1);
NL_TEST_ASSERT(apSuite, sizeof(wValue) >= 1);

// Initialize the Storage Value with the test-buffer
memcpy(&sValue, &storageTestData, sizeof(sValue));

// Convert the Storage Type to Working Type and
wValue = IntType::StorageToWorking(sValue);

// 2) Verify that the correct storage format has been used
NL_TEST_ASSERT(apSuite, wValue == SimpleEnum::kOne);

StorageType sNewValue;

// Convert back to Storage Value
IntType::WorkingToStorage(wValue, sNewValue);

// 3) Verify that the bytes are located as intended
NL_TEST_ASSERT(apSuite, memcmp(&storageTestData, &sNewValue, sizeof(sNewValue)) == 0);

// Set Storage value to Null
IntType::SetNull(sNullValue);
wNullValue = IntType::StorageToWorking(sNullValue);
NL_TEST_ASSERT(apSuite, wNullValue == workingTestUnsignedNullValue);
NL_TEST_ASSERT(apSuite, (IntType::IsNullValue(sNullValue) == true));

// Verify that null values can fit into not nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(false, sNullValue) == true));

// Verify that null values can't fit into nullable
NL_TEST_ASSERT(apSuite, (IntType::CanRepresentValue(true, sNullValue) == false));
}

////////////////////////////////////////////////////////////
// ______ __ __ _______ ______ ________ //
// / \ / | / | / \ / |/ | //
Expand Down Expand Up @@ -1009,6 +1159,10 @@ static int TestTeardown(void * inContext)
// clang-format off
const nlTest sTests[] =
{
NL_TEST_DEF("Test_UINT8", Test_UINT8),
NL_TEST_DEF("Test_SINT8", Test_SINT8),
NL_TEST_DEF("Test_SimpleEnum", Test_SimpleEnum),

NL_TEST_DEF("Test_UINT24_LE",Test_UINT24_LE),
NL_TEST_DEF("Test_SINT24_LE",Test_SINT24_LE),
NL_TEST_DEF("Test_UINT24_BE",Test_UINT24_BE),
Expand Down
124 changes: 124 additions & 0 deletions src/app/tests/suites/TestCluster.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1745,6 +1745,18 @@ tests:

# Tests for nullable UInt8 attribute

- label: "Write attribute NULLABLE_INT8U Min Value"
command: "writeAttribute"
attribute: "nullable_int8u"
arguments:
value: 0

- label: "Read attribute NULLABLE_INT8U Min Value"
command: "readAttribute"
attribute: "nullable_int8u"
response:
value: 0

- label: "Write attribute NULLABLE_INT8U Max Value"
command: "writeAttribute"
attribute: "nullable_int8u"
Expand Down Expand Up @@ -1821,6 +1833,18 @@ tests:

# Tests for nullable UInt16 attribute

- label: "Write attribute NULLABLE_INT16U Min Value"
command: "writeAttribute"
attribute: "nullable_int16u"
arguments:
value: 0

- label: "Read attribute NULLABLE_INT16U Min Value"
command: "readAttribute"
attribute: "nullable_int16u"
response:
value: 0

- label: "Write attribute NULLABLE_INT16U Max Value"
command: "writeAttribute"
attribute: "nullable_int16u"
Expand Down Expand Up @@ -1897,6 +1921,18 @@ tests:

# Tests for nullable UInt32 attribute

- label: "Write attribute NULLABLE_INT32U Min Value"
command: "writeAttribute"
attribute: "nullable_int32u"
arguments:
value: 0

- label: "Read attribute NULLABLE_INT32U Min Value"
command: "readAttribute"
attribute: "nullable_int32u"
response:
value: 0

- label: "Write attribute NULLABLE_INT32U Max Value"
command: "writeAttribute"
attribute: "nullable_int32u"
Expand Down Expand Up @@ -1973,6 +2009,18 @@ tests:

# Tests for nullable UInt64 attribute

- label: "Write attribute NULLABLE_INT64U Min Value"
command: "writeAttribute"
attribute: "nullable_int64u"
arguments:
value: 0

- label: "Read attribute NULLABLE_INT64U Min Value"
command: "readAttribute"
attribute: "nullable_int64u"
response:
value: 0

- label: "Write attribute NULLABLE_INT64U Max Value"
command: "writeAttribute"
attribute: "nullable_int64u"
Expand Down Expand Up @@ -2480,6 +2528,18 @@ tests:

# Tests for Enum8 attribute

- label: "Write attribute NULLABLE_ENUM8 Min Value"
command: "writeAttribute"
attribute: "nullable_enum8"
arguments:
value: 0

- label: "Read attribute NULLABLE_ENUM8 Min Value"
command: "readAttribute"
attribute: "nullable_enum8"
response:
value: 0

- label: "Write attribute NULLABLE_ENUM8 Max Value"
command: "writeAttribute"
attribute: "nullable_enum8"
Expand Down Expand Up @@ -2520,6 +2580,18 @@ tests:

# Tests for Enum16 attribute

- label: "Write attribute NULLABLE_ENUM16 Min Value"
command: "writeAttribute"
attribute: "nullable_enum16"
arguments:
value: 0

- label: "Read attribute NULLABLE_ENUM16 Min Value"
command: "readAttribute"
attribute: "nullable_enum16"
response:
value: 0

- label: "Write attribute NULLABLE_ENUM16 Max Value"
command: "writeAttribute"
attribute: "nullable_enum16"
Expand Down Expand Up @@ -2558,6 +2630,58 @@ tests:
response:
value: null

# Tests for named enum attribute

- label: "Write attribute NULLABLE_SIMPLE_ENUM Min Value"
command: "writeAttribute"
attribute: "nullable_enum_attr"
arguments:
value: 0

- label: "Read attribute NULLABLE_SIMPLE_ENUM Min Value"
command: "readAttribute"
attribute: "nullable_enum_attr"
response:
value: 0

- label: "Write attribute NULLABLE_SIMPLE_ENUM Max Value"
command: "writeAttribute"
attribute: "nullable_enum_attr"
arguments:
value: 254

- label: "Read attribute NULLABLE_SIMPLE_ENUM Max Value"
command: "readAttribute"
attribute: "nullable_enum_attr"
response:
value: 254

- label: "Write attribute NULLABLE_SIMPLE_ENUM Invalid Value"
command: "writeAttribute"
attribute: "nullable_enum_attr"
arguments:
value: 255
response:
error: CONSTRAINT_ERROR

- label: "Read attribute NULLABLE_SIMPLE_ENUM unchanged Value"
command: "readAttribute"
attribute: "nullable_enum_attr"
response:
value: 254

- label: "Write attribute NULLABLE_SIMPLE_ENUM null Value"
command: "writeAttribute"
attribute: "nullable_enum_attr"
arguments:
value: null

- label: "Read attribute NULLABLE_SIMPLE_ENUM null Value"
command: "readAttribute"
attribute: "nullable_enum_attr"
response:
value: null

# Tests for Octet String attribute

- label: "Read attribute NULLABLE_OCTET_STRING Default Value"
Expand Down
3 changes: 2 additions & 1 deletion src/app/util/attribute-storage-null-handling.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ struct NumericAttributeTraits
template <typename U = T, typename std::enable_if_t<std::is_enum<U>::value, int> = 0>
static constexpr StorageType GetNullValue()
{
return GetNullValue<std::underlying_type_t<T>>();
static_assert(!std::is_signed<std::underlying_type_t<T>>::value, "Enums must be unsigned");
return static_cast<StorageType>(std::numeric_limits<std::underlying_type_t<T>>::max());
}

public:
Expand Down
Loading

0 comments on commit 1498468

Please sign in to comment.