Skip to content

Commit 64dbdd2

Browse files
lpbeliveau-silabsbzbarsky-apple
lpbeliveau-silabs
and
bzbarsky-apple
committed
Apply suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 90daf92 commit 64dbdd2

File tree

2 files changed

+136
-58
lines changed

2 files changed

+136
-58
lines changed

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

+48-35
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
*/
1717

1818
#include <app/clusters/scenes-server/SceneHandlerImpl.h>
19+
#include <app/util/ember-io-storage.h>
1920
#include <app/util/endpoint-config-api.h>
2021
#include <app/util/odd-sized-integers.h>
2122

@@ -37,7 +38,7 @@ template <typename Type>
3738
typename app::NumericAttributeTraits<Type>::WorkingType
3839
ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultValue)
3940
{
40-
if (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)
41+
if constexpr (sizeof(typename app::NumericAttributeTraits<Type>::WorkingType) <= 2)
4142
{
4243
return static_cast<typename app::NumericAttributeTraits<Type>::WorkingType>(defaultValue.defaultValue);
4344
}
@@ -47,11 +48,11 @@ ConvertDefaultValueToWorkingValue(const EmberAfDefaultAttributeValue & defaultVa
4748
return app::NumericAttributeTraits<Type>::StorageToWorking(sValue);
4849
}
4950

50-
/// IsOnlyOneValuePopulated
51-
/// @brief Helper function to verify if only one value is populated in a given AttributeValuePairType
51+
/// IsExactlyOneValuePopulated
52+
/// @brief Helper function to verify that exactly one value is populated in a given AttributeValuePairType
5253
/// @param AttributeValuePairType & type AttributeValuePairType to verify
5354
/// @return bool true if only one value is populated, false otherwise
54-
bool IsOnlyOneValuePopulated(const AttributeValuePairType & type)
55+
bool IsExactlyOneValuePopulated(const AttributeValuePairType & type)
5556
{
5657
int count = 0;
5758
if (type.valueUnsigned8.HasValue())
@@ -73,14 +74,14 @@ bool IsOnlyOneValuePopulated(const AttributeValuePairType & type)
7374
return count == 1;
7475
}
7576

76-
/// CapAttributeID
77+
/// CapAttributeValue
7778
/// Cap the attribute value based on the attribute's min and max if they are defined,
7879
/// or based on the attribute's size if they are not.
7980
/// @param[in] aVPair AttributeValuePairType
8081
/// @param[in] metadata EmberAfAttributeMetadata
8182
///
8283
template <typename Type>
83-
void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Value, const EmberAfAttributeMetadata * metadata)
84+
void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType & value, const EmberAfAttributeMetadata * metadata)
8485
{
8586
using IntType = app::NumericAttributeTraits<Type>;
8687
using WorkingType = typename IntType::WorkingType;
@@ -89,7 +90,10 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
8990
WorkingType minValue;
9091
uint16_t bitWidth = static_cast<uint16_t>(emberAfAttributeSize(metadata) * 8);
9192

92-
// Min/Max Value capps for the OddSize integers
93+
// TODO Use min/max values from Type to obtain min/max instead of relying on metadata. See:
94+
// https://github.com/project-chip/connectedhomeip/issues/35328
95+
96+
// Min/Max Value caps for the OddSize integers
9397
if (metadata->IsSignedIntegerAttribute())
9498
{
9599
// We use emberAfAttributeSize for cases like INT24S, INT40S, INT48S, INT56S where numeric_limits<WorkingType>::max()
@@ -101,7 +105,7 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
101105
{
102106
// We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits<WorkingType>::max()
103107
// wouldn't work
104-
if (ZCL_INT64U_ATTRIBUTE_TYPE == metadata->attributeType)
108+
if (ZCL_INT64U_ATTRIBUTE_TYPE == app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
105109
{
106110
maxValue = static_cast<WorkingType>(UINT64_MAX); // Bit shift of 64 is undefined so we use UINT64_MAX
107111
}
@@ -118,7 +122,7 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
118122
if (metadata->IsBoolean())
119123
{
120124
// Caping the value to 1 in case values greater than 1 are set
121-
Value = Value ? 1 : 0;
125+
value = value ? 1 : 0;
122126
return;
123127
}
124128

@@ -130,13 +134,20 @@ void CapAttributeID(typename app::NumericAttributeTraits<Type>::WorkingType & Va
130134
maxValue = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);
131135
}
132136

133-
if (minValue > Value)
137+
if (metadata->IsNullable() && (minValue > value || maxValue < value))
138+
{
139+
// If the attribute is nullable, the value can be set to NULL
140+
app::NumericAttributeTraits<WorkingType>::SetNull(value);
141+
return;
142+
}
143+
144+
if (minValue > value)
134145
{
135-
Value = minValue;
146+
value = minValue;
136147
}
137-
else if (maxValue < Value)
148+
else if (maxValue < value)
138149
{
139-
Value = maxValue;
150+
value = maxValue;
140151
}
141152
}
142153

@@ -159,72 +170,74 @@ CHIP_ERROR ValidateAttributePath(EndpointId endpoint, ClusterId cluster, Attribu
159170
}
160171

161172
// There should never be more than one populated value in an ExtensionFieldSet
162-
VerifyOrReturnError(IsOnlyOneValuePopulated(aVPair), CHIP_ERROR_INVALID_ARGUMENT);
173+
VerifyOrReturnError(IsExactlyOneValuePopulated(aVPair), CHIP_ERROR_INVALID_ARGUMENT);
163174

164-
switch (metadata->attributeType)
175+
switch (app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
165176
{
166177
case ZCL_BOOLEAN_ATTRIBUTE_TYPE:
167-
case ZCL_BITMAP8_ATTRIBUTE_TYPE:
168-
case ZCL_ENUM8_ATTRIBUTE_TYPE:
169178
case ZCL_INT8U_ATTRIBUTE_TYPE:
170179
VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
171-
CapAttributeID<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);
180+
CapAttributeValue<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);
172181
break;
173-
case ZCL_BITMAP16_ATTRIBUTE_TYPE:
174-
case ZCL_ENUM16_ATTRIBUTE_TYPE:
175182
case ZCL_INT16U_ATTRIBUTE_TYPE:
176183
VerifyOrReturnError(aVPair.valueUnsigned16.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
177-
CapAttributeID<uint16_t>(aVPair.valueUnsigned16.Value(), metadata);
184+
CapAttributeValue<uint16_t>(aVPair.valueUnsigned16.Value(), metadata);
178185
break;
179186
case ZCL_INT24U_ATTRIBUTE_TYPE:
180187
VerifyOrReturnError(aVPair.valueUnsigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
181-
CapAttributeID<OddSizedInteger<3, false>>(aVPair.valueUnsigned32.Value(), metadata);
188+
CapAttributeValue<OddSizedInteger<3, false>>(aVPair.valueUnsigned32.Value(), metadata);
182189
break;
183-
case ZCL_BITMAP32_ATTRIBUTE_TYPE:
184190
case ZCL_INT32U_ATTRIBUTE_TYPE:
185191
VerifyOrReturnError(aVPair.valueUnsigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
186-
CapAttributeID<uint32_t>(aVPair.valueUnsigned32.Value(), metadata);
192+
CapAttributeValue<uint32_t>(aVPair.valueUnsigned32.Value(), metadata);
193+
break;
194+
case ZCL_INT40U_ATTRIBUTE_TYPE:
195+
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
196+
CapAttributeValue<OddSizedInteger<5, false>>(aVPair.valueUnsigned64.Value(), metadata);
187197
break;
188198
case ZCL_INT48U_ATTRIBUTE_TYPE:
189199
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
190-
CapAttributeID<OddSizedInteger<6, false>>(aVPair.valueUnsigned64.Value(), metadata);
200+
CapAttributeValue<OddSizedInteger<6, false>>(aVPair.valueUnsigned64.Value(), metadata);
191201
break;
192202
case ZCL_INT56U_ATTRIBUTE_TYPE:
193203
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
194-
CapAttributeID<OddSizedInteger<7, false>>(aVPair.valueUnsigned64.Value(), metadata);
204+
CapAttributeValue<OddSizedInteger<7, false>>(aVPair.valueUnsigned64.Value(), metadata);
195205
break;
196-
case ZCL_BITMAP64_ATTRIBUTE_TYPE:
197206
case ZCL_INT64U_ATTRIBUTE_TYPE:
198207
VerifyOrReturnError(aVPair.valueUnsigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
199-
CapAttributeID<uint64_t>(aVPair.valueUnsigned64.Value(), metadata);
208+
CapAttributeValue<uint64_t>(aVPair.valueUnsigned64.Value(), metadata);
200209
break;
201210
case ZCL_INT8S_ATTRIBUTE_TYPE:
202211
VerifyOrReturnError(aVPair.valueSigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
203-
CapAttributeID<int8_t>(aVPair.valueSigned8.Value(), metadata);
212+
CapAttributeValue<int8_t>(aVPair.valueSigned8.Value(), metadata);
204213
break;
205214
case ZCL_INT16S_ATTRIBUTE_TYPE:
206215
VerifyOrReturnError(aVPair.valueSigned16.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
207-
CapAttributeID<int16_t>(aVPair.valueSigned16.Value(), metadata);
216+
CapAttributeValue<int16_t>(aVPair.valueSigned16.Value(), metadata);
208217
break;
209218
case ZCL_INT24S_ATTRIBUTE_TYPE:
210219
VerifyOrReturnError(aVPair.valueSigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
211-
CapAttributeID<OddSizedInteger<3, true>>(aVPair.valueSigned32.Value(), metadata);
220+
CapAttributeValue<OddSizedInteger<3, true>>(aVPair.valueSigned32.Value(), metadata);
212221
break;
213222
case ZCL_INT32S_ATTRIBUTE_TYPE:
214223
VerifyOrReturnError(aVPair.valueSigned32.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
215-
CapAttributeID<int32_t>(aVPair.valueSigned32.Value(), metadata);
224+
CapAttributeValue<int32_t>(aVPair.valueSigned32.Value(), metadata);
225+
break;
226+
case ZCL_INT40S_ATTRIBUTE_TYPE:
227+
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
228+
CapAttributeValue<OddSizedInteger<5, true>>(aVPair.valueSigned64.Value(), metadata);
216229
break;
217230
case ZCL_INT48S_ATTRIBUTE_TYPE:
218231
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
219-
CapAttributeID<OddSizedInteger<6, true>>(aVPair.valueSigned64.Value(), metadata);
232+
CapAttributeValue<OddSizedInteger<6, true>>(aVPair.valueSigned64.Value(), metadata);
220233
break;
221234
case ZCL_INT56S_ATTRIBUTE_TYPE:
222235
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
223-
CapAttributeID<OddSizedInteger<7, true>>(aVPair.valueSigned64.Value(), metadata);
236+
CapAttributeValue<OddSizedInteger<7, true>>(aVPair.valueSigned64.Value(), metadata);
224237
break;
225238
case ZCL_INT64S_ATTRIBUTE_TYPE:
226239
VerifyOrReturnError(aVPair.valueSigned64.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
227-
CapAttributeID<int64_t>(aVPair.valueSigned64.Value(), metadata);
240+
CapAttributeValue<int64_t>(aVPair.valueSigned64.Value(), metadata);
228241
break;
229242
default:
230243
return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);

0 commit comments

Comments
 (0)