diff --git a/src/app/clusters/scenes-server/SceneHandlerImpl.cpp b/src/app/clusters/scenes-server/SceneHandlerImpl.cpp index 4e01802f00f34a..fcaa3985734bb2 100644 --- a/src/app/clusters/scenes-server/SceneHandlerImpl.cpp +++ b/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 +/// 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 void CapAttributeValue(typename app::NumericAttributeTraits::WorkingType & value, const EmberAfAttributeMetadata * metadata) { - using IntType = app::NumericAttributeTraits; - using WorkingType = typename IntType::WorkingType; - - WorkingType maxValue; - WorkingType minValue; - uint16_t bitWidth = static_cast(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::max() - // wouldn't work - maxValue = static_cast((1ULL << (bitWidth - 1)) - 1); - minValue = static_cast(-(1ULL << (bitWidth - 1))); - } - else - { - // We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits::max() - // wouldn't work - if (ZCL_INT64U_ATTRIBUTE_TYPE == app::Compatibility::Internal::AttributeBaseType(metadata->attributeType)) - { - maxValue = static_cast(UINT64_MAX); // Bit shift of 64 is undefined so we use UINT64_MAX - } - else - { - maxValue = static_cast((1ULL << bitWidth) - 1); - } - minValue = static_cast(0); - } + using IntType = app::NumericAttributeTraits; + using WorkingType = std::remove_reference_t; - // Ensure that the metadata's signedness matches the working type's signedness - VerifyOrDie(metadata->IsSignedIntegerAttribute() == std::is_signed::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::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::WorkingType & maxValue = ConvertDefaultValueToWorkingValue(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(aVPair.valueUnsigned8.Value(), metadata); + break; case ZCL_INT8U_ATTRIBUTE_TYPE: VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); CapAttributeValue(aVPair.valueUnsigned8.Value(), metadata); diff --git a/src/app/util/attribute-storage-null-handling.h b/src/app/util/attribute-storage-null-handling.h index 8db72a2321bf7b..826e5aff531a98 100644 --- a/src/app/util/attribute-storage-null-handling.h +++ b/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(&value); } + + // Min and max values for the type. + static WorkingType MinValue(bool isNullable) + { + if constexpr (!std::is_signed_v) + { + return 0; + } + + if (isNullable) + { + // Smallest negative value is excluded for nullable signed types. + return static_cast(std::numeric_limits::min() + 1); + } + + return std::numeric_limits::min(); + } + + static WorkingType MaxValue(bool isNullable) + { + if constexpr (std::is_signed_v) + { + return std::numeric_limits::max(); + } + + if (isNullable) + { + // Largest value is excluded for nullable unsigned types. + return static_cast(std::numeric_limits::max() - 1); + } + + return std::numeric_limits::max(); + } }; template @@ -209,6 +242,10 @@ struct NumericAttributeTraits static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast(&value); } + static uint8_t MinValue(bool isNullable) { return 0; } + + static uint8_t MaxValue(bool isNullable) { return 1; } + private: static constexpr StorageType kNullValue = 0xFF; }; diff --git a/src/app/util/odd-sized-integers.h b/src/app/util/odd-sized-integers.h index f5f1bb50c6b4bf..32f2c8793b2e71 100644 --- a/src/app/util/odd-sized-integers.h +++ b/src/app/util/odd-sized-integers.h @@ -200,35 +200,52 @@ struct NumericAttributeTraits, 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(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(1) << (8 * ByteSize)) - 1; + // Since WorkingType has at least one extra byte, the bitshift cannot overflow. + constexpr WorkingType signedMin = -(static_cast(1) << (8 * ByteSize - 1)); if (isNullable) { - // we have one less value - max -= 1; + // Smallest negative value is excluded for nullable signed types. + 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(1) << (8 * ByteSize - 1)) - 1; + } - static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; } + constexpr WorkingType unsignedMax = (static_cast(1) << (8 * ByteSize)) - 1; + if (isNullable) + { + // Largest value is excluded for nullable unsigned types. + return unsignedMax - 1; + } + + return unsignedMax; + } }; } // namespace app