Removed un-necessary error, added checks on negative values in unit test, added signedness handling

lpbeliveau-silabs · lpbeliveau-silabs · commit c0e8ed8b00aa · 2024-04-17T15:20:52.000-04:00
diff --git a/docs/ERROR_CODES.md b/docs/ERROR_CODES.md
@@ -89,7 +89,6 @@ This file was **AUTOMATICALLY** generated by
 | 92        | 0x5C  | `CHIP_ERROR_WRONG_NODE_ID`                         |
 | 100       | 0x64  | `CHIP_ERROR_RETRANS_TABLE_FULL`                    |
 | 104       | 0x68  | `CHIP_ERROR_TRANSACTION_CANCELED`                  |
-| 106       | 0x6A  | `CHIP_ERROR_UNSUPPORTED_ATTRIBUTE`                 |
 | 107       | 0x6B  | `CHIP_ERROR_INVALID_SUBSCRIPTION`                  |
 | 108       | 0x6C  | `CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE`              |
 | 112       | 0x70  | `CHIP_ERROR_UNSOLICITED_MSG_NO_ORIGINATOR`         |
diff --git a/src/app/clusters/scenes-server/SceneHandlerImpl.cpp b/src/app/clusters/scenes-server/SceneHandlerImpl.cpp
@@ -26,47 +26,24 @@ namespace {
 using ConcreteAttributePath  = app::ConcreteAttributePath;
 using AttributeValuePairType = app::Clusters::ScenesManagement::Structs::AttributeValuePair::Type;
 
-/// IsAttributeTypeValid
-/// @brief Check if the attribute type is valid for a scene extension field value according to the spec.
-/// @param type Type of the attribute's metadata
-/// @return
-bool IsSceneValidAttributeType(EmberAfAttributeType type)
-{
-    return (type == ZCL_INT8U_ATTRIBUTE_TYPE || type == ZCL_INT16U_ATTRIBUTE_TYPE || type == ZCL_INT32U_ATTRIBUTE_TYPE ||
-            type == ZCL_INT64U_ATTRIBUTE_TYPE || type == ZCL_INT8S_ATTRIBUTE_TYPE || type == ZCL_INT16S_ATTRIBUTE_TYPE ||
-            type == ZCL_INT32S_ATTRIBUTE_TYPE || type == ZCL_INT64S_ATTRIBUTE_TYPE);
-}
-
-/// IsSignedAttribute
-/// @brief Compare the attribute type to the signed attribute types in ZCL and return true if it is signed.
-/// @param attributeType metadata's attribute type
-/// @return
-bool IsSignedAttribute(EmberAfAttributeType attributeType)
-{
-    return (attributeType >= ZCL_INT8S_ATTRIBUTE_TYPE && attributeType <= ZCL_INT64S_ATTRIBUTE_TYPE);
-}
-
 /// ConvertByteArrayToUInt64
 /// @brief Helper function to convert a byte array to a uint64_t value
 /// @param EmberAfDefaultAttributeValue & defaultValue
 /// @param len Length of the byte array
-/// @return uint64_t Value
+/// @return uint64_t or int64_t Value
 /// @note The attribute table supports 8 bytes only for unsigned integers, 4 bytes is the maximum for signed integers
-uint64_t ConvertByteArrayToUInt64(const EmberAfDefaultAttributeValue & defaultValue, uint16_t len)
+template <typename Type>
+Type ConvertByteArrayToType(const EmberAfDefaultAttributeValue & defaultValue, uint16_t len)
 {
     if (len <= 2)
     {
-        return static_cast<uint64_t>(defaultValue.defaultValue);
+        return static_cast<Type>(defaultValue.defaultValue);
     }
     else
     {
-        uint64_t result     = 0;
-        const uint8_t * val = defaultValue.ptrToDefaultValue;
-        for (size_t i = 0; i < len; i++)
-        {
-            result = (result << 8) | val[(CHIP_CONFIG_BIG_ENDIAN_TARGET ? i : (len - 1) - i)];
-        }
-        return result;
+        Type sValue = 0;
+        memcpy(&sValue, defaultValue.ptrToDefaultValue, len);
+        return app::NumericAttributeTraits<Type>::StorageToWorking(sValue);
     }
 }
 
@@ -76,32 +53,37 @@ uint64_t ConvertByteArrayToUInt64(const EmberAfDefaultAttributeValue & defaultVa
 /// @param[in] aVPair   AttributeValuePairType
 /// @param[in] metadata  EmberAfAttributeMetadata
 ///
+template <typename Type>
 void CapAttributeID(AttributeValuePairType & aVPair, const EmberAfAttributeMetadata * metadata)
 {
     // Calculate the maximum value that can be represented with the given number of bytes
-    uint64_t maxValue = 0;
+    Type maxValue = 0;
 
     // Check if the attribute type is signed
-    if (IsSignedAttribute(metadata->attributeType))
+    if (metadata->IsSignedIntegerAttribute())
     {
-        maxValue = (1ULL << ((emberAfAttributeSize(metadata) * 8) - 1)) - 1;
+        maxValue = static_cast<Type>((1ULL << (emberAfAttributeSize(metadata) * 8 - 1)) - 1);
     }
     else
     {
-        maxValue = (1ULL << (emberAfAttributeSize(metadata) * 8)) - 1;
+        maxValue = static_cast<Type>((1ULL << (emberAfAttributeSize(metadata) * 8)) - 1);
     }
 
     // Check metadata for min and max values
-    if ((metadata->mask & ATTRIBUTE_MASK_MIN_MAX) && metadata->defaultValue.ptrToMinMaxValue)
+    if (metadata->HasMinMax())
     {
         const EmberAfAttributeMinMaxValue * minMaxValue = metadata->defaultValue.ptrToMinMaxValue;
-        uint64_t minVal = ConvertByteArrayToUInt64(minMaxValue->minValue, emberAfAttributeSize(metadata));
-        uint64_t maxVal = ConvertByteArrayToUInt64(minMaxValue->maxValue, emberAfAttributeSize(metadata));
+        Type minVal = ConvertByteArrayToType<Type>(minMaxValue->minValue, emberAfAttributeSize(metadata));
+        Type maxVal = ConvertByteArrayToType<Type>(minMaxValue->maxValue, emberAfAttributeSize(metadata));
 
-        // Cap based on minValue
-        if (minVal > aVPair.attributeValue)
+        // Cap based on minimum value
+        if (minVal > static_cast<Type>(aVPair.attributeValue))
         {
-            aVPair.attributeValue = minVal;
+            uint64_t sValue = 0;
+            memcpy(&sValue, &minVal, sizeof(Type));
+            aVPair.attributeValue = app::NumericAttributeTraits<uint64_t>::StorageToWorking(sValue);
+            // We assume the max is >= min therefore we can return
+            return;
         }
 
         // Adjust maxValue if greater than the meta data's max value
@@ -112,17 +94,31 @@ void CapAttributeID(AttributeValuePairType & aVPair, const EmberAfAttributeMetad
     }
 
     // Cap based on maximum value
-    if (aVPair.attributeValue > maxValue)
+    if (metadata->IsSignedIntegerAttribute())
     {
-        aVPair.attributeValue = maxValue;
+        if (static_cast<int64_t>(aVPair.attributeValue) > static_cast<int64_t>(maxValue))
+        {
+            uint64_t sValue = 0;
+            memcpy(&sValue, &maxValue, sizeof(Type));
+            aVPair.attributeValue = app::NumericAttributeTraits<uint64_t>::StorageToWorking(sValue);
+        }
+    }
+    else
+    {
+        if (static_cast<uint64_t>(aVPair.attributeValue) > static_cast<uint64_t>(maxValue))
+        {
+            uint64_t sValue = 0;
+            memcpy(&sValue, &maxValue, sizeof(Type));
+            aVPair.attributeValue = app::NumericAttributeTraits<uint64_t>::StorageToWorking(sValue);
+        }
     }
 }
 
 /// @brief  Validate the attribute exists for a given cluster
 /// @param[in] endpoint   Endpoint ID
 /// @param[in] clusterID  Cluster ID
 /// @param[in] aVPair     AttributeValuePairType, will be mutated to cap the value if it is out of range
-/// @return CHIP_ERROR_UNSUPPORTED_ATTRIBUTE if the attribute does not exist for a given cluster or is not scenable
+/// @return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute) if the attribute does not exist for a given cluster or is not scenable
 /// @note This will allways fail for global list attributes. If we do want to make them scenable someday, we will need to
 ///       use a different validation method.
 // TODO: Add check for "S" quality to determine if the attribute is scenable once suported :
@@ -133,17 +129,39 @@ CHIP_ERROR ValidateAttributePath(EndpointId endpoint, ClusterId cluster, Attribu
 
     if (nullptr == metadata)
     {
-        return CHIP_ERROR_UNSUPPORTED_ATTRIBUTE;
+        return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
     }
 
-    if (!IsSceneValidAttributeType(metadata->attributeType))
+    switch (metadata->attributeType)
     {
-        return CHIP_ERROR_UNSUPPORTED_ATTRIBUTE;
+    case ZCL_INT8U_ATTRIBUTE_TYPE:
+        CapAttributeID<uint8_t>(aVPair, metadata);
+        break;
+    case ZCL_INT16U_ATTRIBUTE_TYPE:
+        CapAttributeID<uint16_t>(aVPair, metadata);
+        break;
+    case ZCL_INT32U_ATTRIBUTE_TYPE:
+        CapAttributeID<uint32_t>(aVPair, metadata);
+        break;
+    case ZCL_INT64U_ATTRIBUTE_TYPE:
+        CapAttributeID<uint64_t>(aVPair, metadata);
+        break;
+    case ZCL_INT8S_ATTRIBUTE_TYPE:
+        CapAttributeID<int8_t>(aVPair, metadata);
+        break;
+    case ZCL_INT16S_ATTRIBUTE_TYPE: // fallthrough
+        CapAttributeID<int16_t>(aVPair, metadata);
+        break;
+    case ZCL_INT32S_ATTRIBUTE_TYPE:
+        CapAttributeID<int32_t>(aVPair, metadata);
+        break;
+    case ZCL_INT64S_ATTRIBUTE_TYPE:
+        CapAttributeID<int64_t>(aVPair, metadata);
+        break;
+    default:
+        return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
     }
 
-    // Cap value based on the attribute type size
-    CapAttributeID(aVPair, metadata);
-
     return CHIP_NO_ERROR;
 }
 } // namespace
diff --git a/src/app/clusters/scenes-server/scenes-server.cpp b/src/app/clusters/scenes-server/scenes-server.cpp
@@ -71,10 +71,6 @@ CHIP_ERROR AddResponseOnError(CommandHandlerInterface::HandlerContext & ctx, Res
         {
             resp.status = to_underlying(Protocols::InteractionModel::Status::ResourceExhausted);
         }
-        else if (CHIP_ERROR_UNSUPPORTED_ATTRIBUTE == err)
-        {
-            resp.status = to_underlying(Protocols::InteractionModel::Status::InvalidCommand);
-        }
         else
         {
             resp.status = to_underlying(StatusIB(err).mStatus);
diff --git a/src/app/tests/TestSceneTable.cpp b/src/app/tests/TestSceneTable.cpp
@@ -723,10 +723,10 @@ void TestHandlerFunctions(nlTestSuite * aSuite, void * aContext)
     memset(failBuffer, 0, fail_list.size());
     memset(buffer, 0, buff_span.size());
 
-    // Test Serialize Add of an attribute value that is greater than the mock attribute size (size of uint32_t)
-    OOPairs[0].attributeValue = 0xFFFFFFFF'FFFFFFFF;
+    // Test Serialize Add of an attribute value that is greater than the mock attribute max (Max int32_t value)
+    OOPairs[0].attributeValue = 0x7FFFFFFF'FFFFFFFF;
 
-    // EFS to test caping of value once a variable above the mock attribute size is serialize
+    // EFS to test caping of value once a variable above the mock attribute size is serialized
     app::Clusters::ScenesManagement::Structs::ExtensionFieldSet::Type extensionFieldValueCapOut;
     app::Clusters::ScenesManagement::Structs::ExtensionFieldSet::DecodableType extensionFieldValueCapIn;
 
@@ -754,14 +754,44 @@ void TestHandlerFunctions(nlTestSuite * aSuite, void * aContext)
     NL_TEST_ASSERT(aSuite,
                    CHIP_NO_ERROR == sHandler.Deserialize(kTestEndpoint1, kOnOffClusterId, buff_span, extensionFieldValueCapOut));
 
-    printf("extensionFieldValueCapOut.attributeValueList[0].attributeValue = 0x%016" PRIX64 "\n",
-           extensionFieldValueCapOut.attributeValueList[0].attributeValue);
-
     // Verify that the output value is capped
     NL_TEST_ASSERT(aSuite, extensionFieldValueCapOut.attributeValueList[0].attributeValue == 0x00FFFFFF);
 
     // Clear buffer
-    memset(failBuffer, 0, fail_list.size());
+    memset(buffer, 0, buff_span.size());
+
+    // Test Serialize Add of an attribute value that is smaller than the mock attribute min (-2)
+    OOPairs[0].attributeValue = 0xFFFFFFFF'FFFFFFFE;
+
+    extensionFieldValueCapOut.clusterID          = kOnOffClusterId;
+    extensionFieldValueCapOut.attributeValueList = OOPairs;
+
+    /// Setup of input EFS (by temporary using the output one)
+    buff_span = MutableByteSpan(buffer);
+    writer.Init(buff_span);
+    NL_TEST_ASSERT(
+        aSuite, CHIP_NO_ERROR == app::DataModel::Encode(writer, TLV::AnonymousTag(), extensionFieldValueCapOut.attributeValueList));
+
+    reader.Init(buffer);
+    extensionFieldValueCapIn.clusterID = kOnOffClusterId;
+    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == reader.Next());
+    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == extensionFieldValueCapIn.attributeValueList.Decode(reader));
+
+    // Verify that the initial value is not capped
+    auto iterator = extensionFieldValueCapIn.attributeValueList.begin();
+    iterator.Next();
+    pair = iterator.GetValue();
+    NL_TEST_ASSERT(aSuite, pair.attributeValue == OOPairs[0].attributeValue);
+
+    // Verify that we cap the value to the mock attribute size when serializing
+    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == sHandler.SerializeAdd(kTestEndpoint1, extensionFieldValueCapIn, buff_span));
+    NL_TEST_ASSERT(aSuite,
+                   CHIP_NO_ERROR == sHandler.Deserialize(kTestEndpoint1, kOnOffClusterId, buff_span, extensionFieldValueCapOut));
+
+    // Verify that the output value is capped to -1 (0xFFFFFFFF)
+    NL_TEST_ASSERT(aSuite, extensionFieldValueCapOut.attributeValueList[0].attributeValue == 0xFFFFFFFF);
+
+    // Clear buffer
     memset(buffer, 0, buff_span.size());
 };
 
diff --git a/src/app/util/attribute-metadata.h b/src/app/util/attribute-metadata.h
@@ -17,8 +17,8 @@
 
 #pragma once
 
+#include <app-common/zap-generated/attribute-type.h>
 #include <app/util/basic-types.h>
-
 #include <cstdint>
 
 /**
@@ -158,6 +158,19 @@ struct EmberAfAttributeMetadata
      */
     EmberAfAttributeMask mask;
 
+    /**
+     * Check wether this attribute is signed based on its type according to the spec.
+     */
+    bool IsSignedIntegerAttribute() const
+    {
+        return (attributeType >= ZCL_INT8S_ATTRIBUTE_TYPE && attributeType <= ZCL_INT64S_ATTRIBUTE_TYPE);
+    }
+
+    /**
+     * Check whether this attribute has a define min and max.
+     */
+    bool HasMinMax() const { return mask & ATTRIBUTE_MASK_MIN_MAX; }
+
     /**
      * Check whether this attribute is nullable.
      */
diff --git a/src/app/util/attribute-table.cpp b/src/app/util/attribute-table.cpp
@@ -41,12 +41,6 @@ using namespace chip;
 using namespace chip::app;
 
 namespace {
-// Zigbee spec says types between signed 8 bit and signed 64 bit
-bool emberAfIsTypeSigned(EmberAfAttributeType dataType)
-{
-    return (dataType >= ZCL_INT8S_ATTRIBUTE_TYPE && dataType <= ZCL_INT64S_ATTRIBUTE_TYPE);
-}
-
 /**
  * @brief Simple integer comparison function.
  * Compares two values of a known length as integers.
@@ -385,7 +379,7 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
             maxBytes = maxv.ptrToDefaultValue;
         }
 
-        bool isAttributeSigned = emberAfIsTypeSigned(metadata->attributeType);
+        bool isAttributeSigned = metadata->IsSignedIntegerAttribute();
         bool isOutOfRange      = emberAfCompareValues(minBytes, data, dataLen, isAttributeSigned) == 1 ||
             emberAfCompareValues(maxBytes, data, dataLen, isAttributeSigned) == -1;
 
diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp
@@ -80,7 +80,7 @@ uint8_t mockAttribute4[256]  = {
 };
 
 const uint8_t defaultValueData[] = { 0x01, 0x02, 0x03, 0x04 };
-const uint8_t minValueData[]     = { 0x00, 0x00, 0x00, 0x00 };
+const uint8_t minValueData[]     = { 0xFF, 0xFF, 0xFF, 0xFF };
 #if CHIP_CONFIG_BIG_ENDIAN_TARGET
 const uint8_t maxValueData[] = { 0x00, 0xFF, 0xFF, 0xFF }; // Equivalent, in big-endian, to 0x00FFFFFF
 #else
@@ -91,8 +91,8 @@ EmberAfAttributeMinMaxValue minMaxValue = { defaultValueData, minValueData, maxV
 
 EmberAfAttributeMetadata mockmetadata = { .defaultValue  = EmberAfDefaultOrMinMaxAttributeValue(&minMaxValue),
                                           .attributeId   = 0,
-                                          .size          = sizeof(uint32_t),
-                                          .attributeType = ZCL_INT32U_ATTRIBUTE_TYPE,
+                                          .size          = sizeof(int32_t),
+                                          .attributeType = ZCL_INT32S_ATTRIBUTE_TYPE,
                                           .mask          = ATTRIBUTE_MASK_MIN_MAX }; // namespace
 
 } // namespace
diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h
@@ -1135,15 +1135,7 @@ using CHIP_ERROR = ::chip::ChipError;
 #define CHIP_ERROR_TRANSACTION_CANCELED                        CHIP_CORE_ERROR(0x68)
 
 // AVAILABLE: 0x69
-
-/**
- *  @def CHIP_ERROR_UNSUPPORTED_ATTRIBUTE
- *
- *  @brief
- *    The requested CHIP attribute is not supported.
- *
- */
-#define CHIP_ERROR_UNSUPPORTED_ATTRIBUTE                    CHIP_CORE_ERROR(0x6a)
+// AVAILABLE: 0x6a
 
 /**
  *  @def CHIP_ERROR_INVALID_SUBSCRIPTION

Original file line number	Diff line number	Diff line change
`@@ -71,10 +71,6 @@ CHIP_ERROR AddResponseOnError(CommandHandlerInterface::HandlerContext & ctx, Res`
`71`	`71`	`{`
`72`	`72`	`resp.status = to_underlying(Protocols::InteractionModel::Status::ResourceExhausted);`
`73`	`73`	`}`
`74`		`- else if (CHIP_ERROR_UNSUPPORTED_ATTRIBUTE == err)`
`75`		`- {`
`76`		`- resp.status = to_underlying(Protocols::InteractionModel::Status::InvalidCommand);`
`77`		`- }`
`78`	`74`	`else`
`79`	`75`	`{`
`80`	`76`	`resp.status = to_underlying(StatusIB(err).mStatus);`