Skip to content

Commit

Permalink
Add MinValue/MaxValue helpers on our NumericAttributeTraits. (#35390)
Browse files Browse the repository at this point in the history 
* Add MinValue/MaxValue helpers on our NumericAttributeTraits.

Uses the new helpers in SceneHandlerImpl.

Fixes #35328

* Address review comments.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Sep 5, 2024
1 parent 9c40d08 commit 3621c43
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 75 deletions.
74 changes: 18 additions & 56 deletions src/app/clusters/scenes-server/SceneHandlerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 37 additions & 0 deletions src/app/util/attribute-storage-null-handling.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 signed 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 unsigned types.
return static_cast<WorkingType>(std::numeric_limits<WorkingType>::max() - 1);
}

return std::numeric_limits<WorkingType>::max();
}
};

template <typename T>
Expand Down Expand Up @@ -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;
};
Expand Down
55 changes: 36 additions & 19 deletions src/app/util/odd-sized-integers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
// 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<WorkingType>(1) << (8 * ByteSize - 1)) - 1;
}

static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; }
constexpr WorkingType unsignedMax = (static_cast<WorkingType>(1) << (8 * ByteSize)) - 1;
if (isNullable)
{
// Largest value is excluded for nullable unsigned types.
return unsignedMax - 1;
}

return unsignedMax;
}
};

} // namespace app
Expand Down

0 comments on commit 3621c43

Please sign in to comment.