Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@lpbeliveau-silabs @bzbarsky-apple
lpbeliveau-silabs and bzbarsky-apple committed Aug 29, 2024
1 parent 90daf92 commit bc694d8
Showing 1 changed file with 44 additions and 28 deletions.
72 changes: 44 additions & 28 deletions src/app/clusters/scenes-server/SceneHandlerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include <app/clusters/scenes-server/SceneHandlerImpl.h>
#include <app/util/ember-io-storage.h>
#include <app/util/endpoint-config-api.h>
#include <app/util/odd-sized-integers.h>

Expand All @@ -37,7 +38,7 @@ template <typename Type>
typename app::NumericAttributeTraits<Type>::WorkingType
ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)
{
if (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)
if constexpr (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)
{
return static_cast<typename app::NumericAttributeTraits<Type>::WorkingType>(defaultValue.defaultValue);
}
Expand All @@ -47,11 +48,11 @@ ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultVa
return app::NumericAttributeTraits<Type>::StorageToWorking(sValue);
}

/// IsOnlyOneValuePopulated
/// @brief Helper function to verify if only one value is populated in a given AttributeValuePairType
/// IsExactlyOneValuePopulated
/// @brief Helper function to verify that exactly one value is populated in a given AttributeValuePairType
/// @param AttributeValuePairType & type AttributeValuePairType to verify
/// @return bool true if only one value is populated, false otherwise
bool IsOnlyOneValuePopulated(const AttributeValuePairType & type)
bool IsExactlyOneValuePopulated(const AttributeValuePairType & type)
{
int count = 0;
if (type.valueUnsigned8.HasValue())
Expand All @@ -73,14 +74,14 @@ bool IsOnlyOneValuePopulated(const AttributeValuePairType & type)
return count == 1;
}

/// CapAttributeID
/// 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>
void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Value, const EmberAfAttributeMetadata * metadata)
void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType & value, const EmberAfAttributeMetadata * metadata)
{
using IntType = app::NumericAttributeTraits<Type>;
using WorkingType = typename IntType::WorkingType;
Expand All @@ -89,7 +90,7 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
WorkingType minValue;
uint16_t bitWidth = static_cast<uint16_t>(emberAfAttributeSize(metadata) * 8);

// Min/Max Value capps for the OddSize integers
// 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()
Expand Down Expand Up @@ -118,7 +119,7 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
if (metadata->IsBoolean())
{
// Caping the value to 1 in case values greater than 1 are set
Value = Value ? 1 : 0;
value = value ? 1 : 0;
return;
}

Expand All @@ -130,13 +131,20 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
maxValue = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);
}

if (minValue > Value)
if (metadata->IsNullable() && (minValue > value || maxValue < value))
{
Value = minValue;
// If the attribute is nullable, the value can be set to NULL
app::NumericAttributeTraits<WorkingType>::SetNull(value);
return;
}

if (minValue > value)
{
value = minValue;
}
else if (maxValue < Value)
else if (maxValue < value)
{
Value = maxValue;
value = maxValue;
}
}

Expand All @@ -159,72 +167,80 @@ CHIP_ERROR ValidateAttributePath(EndpointId endpoint, ClusterId cluster, Attribu
}

// There should never be more than one populated value in an ExtensionFieldSet
VerifyOrReturnError(IsOnlyOneValuePopulated(aVPair), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsExactlyOneValuePopulated(aVPair), CHIP_ERROR_INVALID_ARGUMENT);

switch (metadata->attributeType)
switch (app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
{
case ZCL_BOOLEAN_ATTRIBUTE_TYPE:
case ZCL_BITMAP8_ATTRIBUTE_TYPE:
case ZCL_ENUM8_ATTRIBUTE_TYPE:
case ZCL_INT8U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);
CapAttributeValue<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);
break;
case ZCL_BITMAP16_ATTRIBUTE_TYPE:
case ZCL_ENUM16_ATTRIBUTE_TYPE:
case ZCL_INT16U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned16.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<uint16_t>(aVPair.valueUnsigned16.Value(), metadata);
CapAttributeValue<uint16_t>(aVPair.valueUnsigned16.Value(), metadata);
break;
case ZCL_INT24U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<3, false>>(aVPair.valueUnsigned32.Value(), metadata);
CapAttributeValue<OddSizedInteger<3, false>>(aVPair.valueUnsigned32.Value(), metadata);
break;
case ZCL_BITMAP32_ATTRIBUTE_TYPE:
case ZCL_INT32U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<uint32_t>(aVPair.valueUnsigned32.Value(), metadata);
CapAttributeValue<uint32_t>(aVPair.valueUnsigned32.Value(), metadata);
break;
case ZCL_INT40U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeValue<OddSizedInteger<5, false>>(aVPair.valueUnsigned64.Value(), metadata);
break;
case ZCL_INT48U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<6, false>>(aVPair.valueUnsigned64.Value(), metadata);
CapAttributeValue<OddSizedInteger<6, false>>(aVPair.valueUnsigned64.Value(), metadata);
break;
case ZCL_INT56U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<7, false>>(aVPair.valueUnsigned64.Value(), metadata);
CapAttributeValue<OddSizedInteger<7, false>>(aVPair.valueUnsigned64.Value(), metadata);
break;
case ZCL_BITMAP64_ATTRIBUTE_TYPE:
case ZCL_INT64U_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<uint64_t>(aVPair.valueUnsigned64.Value(), metadata);
CapAttributeValue<uint64_t>(aVPair.valueUnsigned64.Value(), metadata);
break;
case ZCL_INT8S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<int8_t>(aVPair.valueSigned8.Value(), metadata);
CapAttributeValue<int8_t>(aVPair.valueSigned8.Value(), metadata);
break;
case ZCL_INT16S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned16.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<int16_t>(aVPair.valueSigned16.Value(), metadata);
CapAttributeValue<int16_t>(aVPair.valueSigned16.Value(), metadata);
break;
case ZCL_INT24S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<3, true>>(aVPair.valueSigned32.Value(), metadata);
CapAttributeValue<OddSizedInteger<3, true>>(aVPair.valueSigned32.Value(), metadata);
break;
case ZCL_INT32S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<int32_t>(aVPair.valueSigned32.Value(), metadata);
CapAttributeValue<int32_t>(aVPair.valueSigned32.Value(), metadata);
break;
case ZCL_INT40S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeValue<OddSizedInteger<5, true>>(aVPair.valueSigned64.Value(), metadata);
break;
case ZCL_INT48S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<6, true>>(aVPair.valueSigned64.Value(), metadata);
CapAttributeValue<OddSizedInteger<6, true>>(aVPair.valueSigned64.Value(), metadata);
break;
case ZCL_INT56S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<OddSizedInteger<7, true>>(aVPair.valueSigned64.Value(), metadata);
CapAttributeValue<OddSizedInteger<7, true>>(aVPair.valueSigned64.Value(), metadata);
break;
case ZCL_INT64S_ATTRIBUTE_TYPE:
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
CapAttributeID<int64_t>(aVPair.valueSigned64.Value(), metadata);
CapAttributeValue<int64_t>(aVPair.valueSigned64.Value(), metadata);
break;
default:
return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
Expand Down

0 comments on commit bc694d8

Please sign in to comment.