Fixed some of the attribute casting issues, need to rework the spec before completing this PR

lpbeliveau-silabs · lpbeliveau-silabs · commit a6232e656dd0 · 2024-04-30T19:20:59.000-04:00
diff --git a/src/app/clusters/scenes-server/SceneHandlerImpl.cpp b/src/app/clusters/scenes-server/SceneHandlerImpl.cpp
@@ -34,15 +34,16 @@ using AttributeValuePairType = app::Clusters::ScenesManagement::Structs::Attribu
 /// @param EmberAfDefaultAttributeValue & defaultValue
 /// @return Value converted to the given working type
 template <typename Type>
-Type ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)
+typename app::NumericAttributeTraits<Type>::WorkingType
+ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)
 {
-    if (sizeof(Type) <= 2)
+    if (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)
     {
-        return static_cast<Type>(defaultValue.defaultValue);
+        return static_cast<typename app::NumericAttributeTraits<Type>::WorkingType>(defaultValue.defaultValue);
     }
 
-    Type sValue = 0;
-    memcpy(&sValue, defaultValue.ptrToDefaultValue, sizeof(Type));
+    typename app::NumericAttributeTraits<Type>::StorageType sValue;
+    memcpy(&sValue, defaultValue.ptrToDefaultValue, sizeof(sValue));
     return app::NumericAttributeTraits<Type>::StorageToWorking(sValue);
 }
 
@@ -60,57 +61,67 @@ void CapAttributeID(AttributeValuePairType & aVPair, const EmberAfAttributeMetad
 
     WorkingType maxValue;
 
-    if (metadata->IsBoolean())
-    {
-        aVPair.attributeValue = aVPair.attributeValue ? 1 : 0;
-        return;
-    }
-
     // Check if the attribute type is signed
     if (metadata->IsSignedIntegerAttribute())
     {
+        // We use emberAfAttributeSize for cases like INT24S, INT40S, INT48S, INT56S where numeric_limits<WorkingType>::max()
+        // wouldn't work
         maxValue = static_cast<WorkingType>((1ULL << (emberAfAttributeSize(metadata) * 8 - 1)) - 1);
     }
     else
     {
+        // We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits<WorkingType>::max()
+        // wouldn't work
         maxValue = static_cast<WorkingType>((1ULL << (emberAfAttributeSize(metadata) * 8)) - 1);
     }
 
+    // Ensure that the metadata's signedness matches the working type's signedness
+    VerifyOrDie(metadata->IsSignedIntegerAttribute() == std::is_signed<WorkingType>::value);
+
+    if (metadata->IsBoolean())
+    {
+        // Caping the value to 1 in case values greater than 1 are set
+        aVPair.attributeValue = aVPair.attributeValue ? 1 : 0;
+        return;
+    }
+
     // Check metadata for min and max values
     if (metadata->HasMinMax())
     {
         const EmberAfAttributeMinMaxValue * minMaxValue = metadata->defaultValue.ptrToMinMaxValue;
-        WorkingType minVal                              = ConvertDefaultValueToWorkingValue<WorkingType>(minMaxValue->minValue);
-        WorkingType maxVal                              = ConvertDefaultValueToWorkingValue<WorkingType>(minMaxValue->maxValue);
+        WorkingType rangeMin                            = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->minValue);
+        WorkingType rangeMax                            = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);
 
         // Cap based on minimum value
-        if (minVal > static_cast<WorkingType>(aVPair.attributeValue))
+        if (rangeMin > static_cast<WorkingType>(aVPair.attributeValue))
         {
-            aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(minVal);
+            aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(rangeMin);
             // We assume the max is >= min therefore we can return
             return;
         }
 
         // Adjust maxValue if greater than the meta data's max value
-        if (maxVal < maxValue)
+        if (rangeMax < static_cast<WorkingType>(aVPair.attributeValue))
         {
-            maxValue = maxVal;
+            maxValue = rangeMax;
         }
     }
 
-    // Cap based on maximum value
-    if (metadata->IsSignedIntegerAttribute())
+    // Cap based on type-enforced maximum value (and minnimum for signed types)
+    if constexpr (std::is_signed<WorkingType>::value)
     {
-        if (static_cast<int64_t>(aVPair.attributeValue) > static_cast<int64_t>(maxValue))
+        // Cap on max value, check for comparison to the WorkingType size and check for overflow
+        if (static_cast<WorkingType>(aVPair.attributeValue) > maxValue || aVPair.attributeValue & ~static_cast<uint64_t>(maxValue))
         {
+            // We treat overflows as > Max value since the wrong type is being used and we cannot determine the actual value
             aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(maxValue);
         }
     }
     else
     {
-        if (aVPair.attributeValue > static_cast<uint64_t>(maxValue))
+        if (aVPair.attributeValue > maxValue)
         {
-            aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(maxValue);
+            aVPair.attributeValue = maxValue;
         }
     }
 }
diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp
@@ -69,30 +69,29 @@ bool mockAttribute1          = true;
 int16_t mockAttribute2       = 42;
 uint64_t mockAttribute3      = 0xdeadbeef0000cafe;
 uint8_t mockAttribute4[256]  = {
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
-    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
+     0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
 };
 
 const uint8_t defaultValueData[] = { 0x01, 0x02, 0x03, 0x04 };
 const uint8_t minValueData[]     = { 0xFF, 0xFF, 0xFF, 0xFF };
 #if CHIP_CONFIG_BIG_ENDIAN_TARGET
 const uint8_t maxValueData[] = { 0x00, 0x7F, 0xFF, 0xFF };
-7
 #else
 const uint8_t maxValueData[] = { 0xFF, 0xFF, 0x7F, 0x00 }; // Equivalent, in little-endian, to 0x007FFFFF
 #endif
 
-    EmberAfAttributeMinMaxValue minMaxValue = { defaultValueData, minValueData, maxValueData };
+EmberAfAttributeMinMaxValue minMaxValue = { defaultValueData, minValueData, maxValueData };
 
 EmberAfAttributeMetadata mockmetadata = { .defaultValue  = EmberAfDefaultOrMinMaxAttributeValue(&minMaxValue),
                                           .attributeId   = 0,
-                                          .size          = sizeof(int32_t),
+                                          .size          = 3,
                                           .attributeType = ZCL_INT24S_ATTRIBUTE_TYPE,
                                           .mask          = ATTRIBUTE_MASK_MIN_MAX }; // namespace
 

Original file line number	Diff line number	Diff line change
`@@ -34,15 +34,16 @@ using AttributeValuePairType = app::Clusters::ScenesManagement::Structs::Attribu`
`34`	`34`	`/// @param EmberAfDefaultAttributeValue & defaultValue`
`35`	`35`	`/// @return Value converted to the given working type`
`36`	`36`	`template <typename Type>`
`37`		`-Type ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)`
	`37`	`+typename app::NumericAttributeTraits<Type>::WorkingType`
	`38`	`+ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)`
`38`	`39`	`{`
`39`		`- if (sizeof(Type) <= 2)`
	`40`	`+ if (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)`
`40`	`41`	`{`
`41`		`- return static_cast<Type>(defaultValue.defaultValue);`
	`42`	`+ return static_cast<typename app::NumericAttributeTraits<Type>::WorkingType>(defaultValue.defaultValue);`
`42`	`43`	`}`
`43`	`44`
`44`		`- Type sValue = 0;`
`45`		`- memcpy(&sValue, defaultValue.ptrToDefaultValue, sizeof(Type));`
	`45`	`+ typename app::NumericAttributeTraits<Type>::StorageType sValue;`
	`46`	`+ memcpy(&sValue, defaultValue.ptrToDefaultValue, sizeof(sValue));`
`46`	`47`	`return app::NumericAttributeTraits<Type>::StorageToWorking(sValue);`
`47`	`48`	`}`
`48`	`49`
`@@ -60,57 +61,67 @@ void CapAttributeID(AttributeValuePairType & aVPair, const EmberAfAttributeMetad`
`60`	`61`
`61`	`62`	`WorkingType maxValue;`
`62`	`63`
`63`		`- if (metadata->IsBoolean())`
`64`		`- {`
`65`		`- aVPair.attributeValue = aVPair.attributeValue ? 1 : 0;`
`66`		`- return;`
`67`		`- }`
`68`		`-`
`69`	`64`	`// Check if the attribute type is signed`
`70`	`65`	`if (metadata->IsSignedIntegerAttribute())`
`71`	`66`	`{`
	`67`	`+ // We use emberAfAttributeSize for cases like INT24S, INT40S, INT48S, INT56S where numeric_limits<WorkingType>::max()`
	`68`	`+ // wouldn't work`
`72`	`69`	`maxValue = static_cast<WorkingType>((1ULL << (emberAfAttributeSize(metadata) * 8 - 1)) - 1);`
`73`	`70`	`}`
`74`	`71`	`else`
`75`	`72`	`{`
	`73`	`+ // We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits<WorkingType>::max()`
	`74`	`+ // wouldn't work`
`76`	`75`	`maxValue = static_cast<WorkingType>((1ULL << (emberAfAttributeSize(metadata) * 8)) - 1);`
`77`	`76`	`}`
`78`	`77`
	`78`	`+ // Ensure that the metadata's signedness matches the working type's signedness`
	`79`	`+ VerifyOrDie(metadata->IsSignedIntegerAttribute() == std::is_signed<WorkingType>::value);`
	`80`	`+`
	`81`	`+ if (metadata->IsBoolean())`
	`82`	`+ {`
	`83`	`+ // Caping the value to 1 in case values greater than 1 are set`
	`84`	`+ aVPair.attributeValue = aVPair.attributeValue ? 1 : 0;`
	`85`	`+ return;`
	`86`	`+ }`
	`87`	`+`
`79`	`88`	`// Check metadata for min and max values`
`80`	`89`	`if (metadata->HasMinMax())`
`81`	`90`	`{`
`82`	`91`	`const EmberAfAttributeMinMaxValue * minMaxValue = metadata->defaultValue.ptrToMinMaxValue;`
`83`		`- WorkingType minVal = ConvertDefaultValueToWorkingValue<WorkingType>(minMaxValue->minValue);`
`84`		`- WorkingType maxVal = ConvertDefaultValueToWorkingValue<WorkingType>(minMaxValue->maxValue);`
	`92`	`+ WorkingType rangeMin = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->minValue);`
	`93`	`+ WorkingType rangeMax = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);`
`85`	`94`
`86`	`95`	`// Cap based on minimum value`
`87`		`- if (minVal > static_cast<WorkingType>(aVPair.attributeValue))`
	`96`	`+ if (rangeMin > static_cast<WorkingType>(aVPair.attributeValue))`
`88`	`97`	`{`
`89`		`- aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(minVal);`
	`98`	`+ aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(rangeMin);`
`90`	`99`	`// We assume the max is >= min therefore we can return`
`91`	`100`	`return;`
`92`	`101`	`}`
`93`	`102`
`94`	`103`	`// Adjust maxValue if greater than the meta data's max value`
`95`		`- if (maxVal < maxValue)`
	`104`	`+ if (rangeMax < static_cast<WorkingType>(aVPair.attributeValue))`
`96`	`105`	`{`
`97`		`- maxValue = maxVal;`
	`106`	`+ maxValue = rangeMax;`
`98`	`107`	`}`
`99`	`108`	`}`
`100`	`109`
`101`		`- // Cap based on maximum value`
`102`		`- if (metadata->IsSignedIntegerAttribute())`
	`110`	`+ // Cap based on type-enforced maximum value (and minnimum for signed types)`
	`111`	`+ if constexpr (std::is_signed<WorkingType>::value)`
`103`	`112`	`{`
`104`		`- if (static_cast<int64_t>(aVPair.attributeValue) > static_cast<int64_t>(maxValue))`
	`113`	`+ // Cap on max value, check for comparison to the WorkingType size and check for overflow`
	`114`	`+ if (static_cast<WorkingType>(aVPair.attributeValue) > maxValue \|\| aVPair.attributeValue & ~static_cast<uint64_t>(maxValue))`
`105`	`115`	`{`
	`116`	`+ // We treat overflows as > Max value since the wrong type is being used and we cannot determine the actual value`
`106`	`117`	`aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(maxValue);`
`107`	`118`	`}`
`108`	`119`	`}`
`109`	`120`	`else`
`110`	`121`	`{`
`111`		`- if (aVPair.attributeValue > static_cast<uint64_t>(maxValue))`
	`122`	`+ if (aVPair.attributeValue > maxValue)`
`112`	`123`	`{`
`113`		`- aVPair.attributeValue = static_cast<std::make_unsigned_t<WorkingType>>(maxValue);`
	`124`	`+ aVPair.attributeValue = maxValue;`
`114`	`125`	`}`
`115`	`126`	`}`
`116`	`127`	`}`