Add MinValue/MaxValue helpers on our NumericAttributeTraits.

src/app/clusters/scenes-server/SceneHandlerImpl.cpp

@@ -77,62 +77,25 @@ bool IsExactlyOneValuePopulated(const AttributeValuePairType & type)
 /// CapAttributeValue
 /// Cap the attribute value based on the attribute's min and max if they are defined,
 /// or based on the attribute's size if they are not.
-/// @param[in] aVPair   AttributeValuePairType
-/// @param[in] metadata  EmberAfAttributeMetadata
 ///
-template <typename Type>
+/// The TypeForMinMax template parameter determines the type to use for the
+/// min/max computation.  The Type template parameter determines how the
+/// resulting value is actually represented, because for booleans we
+/// unfortunately end up using uint8, not an actual boolean.
+///
+/// @param[in] value   The value from the AttributeValuePairType that we were given.
+/// @param[in] metadata  The metadata for the attribute the value is for.
+///
+///
+///
+template <typename Type, typename TypeForMinMax = Type>
 void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType & value, const EmberAfAttributeMetadata * metadata)
 {
-    using IntType     = app::NumericAttributeTraits<Type>;
-    using WorkingType = typename IntType::WorkingType;
-
-    WorkingType maxValue;
-    WorkingType minValue;
-    uint16_t bitWidth = static_cast<uint16_t>(emberAfAttributeSize(metadata) * 8);
-
-    // TODO Use min/max values from Type to obtain min/max instead of relying on metadata. See:
-    // https://github.com/project-chip/connectedhomeip/issues/35328
-
-    // Min/Max Value caps for the OddSize integers
-    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 << (bitWidth - 1)) - 1);
-        minValue = static_cast<WorkingType>(-(1ULL << (bitWidth - 1)));
-    }
-    else
-    {
-        // We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits<WorkingType>::max()
-        // wouldn't work
-        if (ZCL_INT64U_ATTRIBUTE_TYPE == app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
-        {
-            maxValue = static_cast<WorkingType>(UINT64_MAX); // Bit shift of 64 is undefined so we use UINT64_MAX
-        }
-        else
-        {
-            maxValue = static_cast<WorkingType>((1ULL << bitWidth) - 1);
-        }
-        minValue = static_cast<WorkingType>(0);
-    }
+    using IntType     = app::NumericAttributeTraits<TypeForMinMax>;
+    using WorkingType = std::remove_reference_t<decltype(value)>;
 
-    // Ensure that the metadata's signedness matches the working type's signedness
-    VerifyOrDie(metadata->IsSignedIntegerAttribute() == std::is_signed<WorkingType>::value);
-
-    if (metadata->IsBoolean())
-    {
-        if (metadata->IsNullable() && (value != 1 && value != 0))
-        {
-            // If the attribute is nullable, the value can be set to NULL
-            app::NumericAttributeTraits<WorkingType>::SetNull(value);
-        }
-        else
-        {
-            // Caping the value to 1 in case values greater than 1 are set
-            value = value ? 1 : 0;
-        }
-        return;
-    }
+    WorkingType minValue = IntType::MinValue(metadata->IsNullable());
+    WorkingType maxValue = IntType::MaxValue(metadata->IsNullable());
 
     // Check metadata for min and max values
     if (metadata->HasMinMax())
@@ -142,10 +105,6 @@ void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType &
         maxValue                                        = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);
     }
 
-    // If the attribute is nullable, the min and max values calculated for types will not be valid, however this does not
-    // change the behavior here as the value will already be NULL if it is out of range. E.g. a nullable INT8U has a minValue of
-    // -127. The code above determin minValue = -128, so an input value of -128 would not enter the condition block below, but would
-    // be considered NULL nonetheless.
     if (metadata->IsNullable() && (minValue > value || maxValue < value))
     {
         // If the attribute is nullable, the value can be set to NULL
@@ -187,6 +146,9 @@ CHIP_ERROR ValidateAttributePath(EndpointId endpoint, ClusterId cluster, Attribu
     switch (app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
     {
     case ZCL_BOOLEAN_ATTRIBUTE_TYPE:
+        VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
+        CapAttributeValue<uint8_t, bool>(aVPair.valueUnsigned8.Value(), metadata);
+        break;
     case ZCL_INT8U_ATTRIBUTE_TYPE:
         VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
         CapAttributeValue<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);

src/app/util/attribute-storage-null-handling.h

@@ -127,6 +127,39 @@ struct NumericAttributeTraits
     // Utility that lets consumers treat a StorageType instance as a uint8_t*
     // for writing to the attribute store.
     static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
+
+    // Min and max values for the type.
+    static WorkingType MinValue(bool isNullable)
+    {
+        if constexpr (!std::is_signed_v<WorkingType>)
+        {
+            return 0;
+        }
+
+        if (isNullable)
+        {
+            // Smallest negative value is excluded for nullable types.
+            return static_cast<WorkingType>(std::numeric_limits<WorkingType>::min() + 1);
+        }
+
+        return std::numeric_limits<WorkingType>::min();
+    }
+
+    static WorkingType MaxValue(bool isNullable)
+    {
+        if constexpr (std::is_signed_v<WorkingType>)
+        {
+            return std::numeric_limits<WorkingType>::max();
+        }
+
+        if (isNullable)
+        {
+            // Largest value is excluded for nullable types.
+            return static_cast<WorkingType>(std::numeric_limits<WorkingType>::max() - 1);
+        }
+
+        return std::numeric_limits<WorkingType>::max();
+    }
 };
 
 template <typename T>
@@ -209,6 +242,10 @@ struct NumericAttributeTraits<bool>
 
     static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
 
+    static uint8_t MinValue(bool isNullable) { return 0; }
+
+    static uint8_t MaxValue(bool isNullable) { return 1; }
+
 private:
     static constexpr StorageType kNullValue = 0xFF;
 };

src/app/util/odd-sized-integers.h

@@ -200,35 +200,52 @@ struct NumericAttributeTraits<OddSizedInteger<ByteSize, IsSigned>, IsBigEndian>
 
     static constexpr bool CanRepresentValue(bool isNullable, WorkingType value)
     {
-        // Since WorkingType has at least one extra byte, none of our bitshifts
-        // overflow.
-        if (IsSigned)
+        return MinValue(isNullable) <= value && value <= MaxValue(isNullable);
+    }
+
+    static CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, StorageType value)
+    {
+        return writer.Put(tag, StorageToWorking(value));
+    }
+
+    static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; }
+
+    static WorkingType MinValue(bool isNullable)
+    {
+        if constexpr (!IsSigned)
         {
-            WorkingType max = (static_cast<WorkingType>(1) << (8 * ByteSize - 1)) - 1;
-            WorkingType min = -max;
-            if (!isNullable)
-            {
-                // We have one more value.
-                min -= 1;
-            }
-            return value >= min && value <= max;
+            return 0;
         }
 
-        WorkingType max = (static_cast<WorkingType>(1) << (8 * ByteSize)) - 1;
+        // Since WorkingType has at least one extra byte, the bitshift cannot overflow.
+        constexpr WorkingType signedMin = -(static_cast<WorkingType>(1) << (8 * ByteSize - 1));
         if (isNullable)
         {
-            // we have one less value
-            max -= 1;
+            // We have one fewer value.
+            return signedMin + 1;
         }
-        return value <= max;
+
+        return signedMin;
     }
 
-    static CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, StorageType value)
+    static WorkingType MaxValue(bool isNullable)
     {
-        return writer.Put(tag, StorageToWorking(value));
-    }
+        // Since WorkingType has at least one extra byte, none of our bitshifts
+        // overflow.
+        if constexpr (IsSigned)
+        {
+            return (static_cast<WorkingType>(1) << (8 * ByteSize - 1)) - 1;
+        }
 
-    static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; }
+        constexpr WorkingType unsignedMax = (static_cast<WorkingType>(1) << (8 * ByteSize));
+        if (isNullable)
+        {
+            // Largest value is excluded for nullable types.
+            return unsignedMax - 1;
+        }
+
+        return unsignedMax;
+    }
 };
 
 } // namespace app