Skip to content

Commit 39d2244

Browse files
bzbarsky-applebzbarsky-apple
committed
Add MinValue/MaxValue helpers on our NumericAttributeTraits.
Uses the new helpers in SceneHandlerImpl. Fixes project-chip#35328
1 parent a6f083e commit 39d2244

File tree

3 files changed

+91
-75
lines changed

3 files changed

+91
-75
lines changed

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

+18-56
Original file line numberDiff line numberDiff line change
@@ -77,62 +77,25 @@ bool IsExactlyOneValuePopulated(const AttributeValuePairType & type)
7777
/// CapAttributeValue
7878
/// Cap the attribute value based on the attribute's min and max if they are defined,
7979
/// or based on the attribute's size if they are not.
80-
/// @param[in] aVPair AttributeValuePairType
81-
/// @param[in] metadata EmberAfAttributeMetadata
8280
///
83-
template <typename Type>
81+
/// The TypeForMinMax template parameter determines the type to use for the
82+
/// min/max computation. The Type template parameter determines how the
83+
/// resulting value is actually represented, because for booleans we
84+
/// unfortunately end up using uint8, not an actual boolean.
85+
///
86+
/// @param[in] value The value from the AttributeValuePairType that we were given.
87+
/// @param[in] metadata The metadata for the attribute the value is for.
88+
///
89+
///
90+
///
91+
template <typename Type, typename TypeForMinMax = Type>
8492
void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType & value, const EmberAfAttributeMetadata * metadata)
8593
{
86-
using IntType = app::NumericAttributeTraits<Type>;
87-
using WorkingType = typename IntType::WorkingType;
88-
89-
WorkingType maxValue;
90-
WorkingType minValue;
91-
uint16_t bitWidth = static_cast<uint16_t>(emberAfAttributeSize(metadata) * 8);
92-
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
97-
if (metadata->IsSignedIntegerAttribute())
98-
{
99-
// We use emberAfAttributeSize for cases like INT24S, INT40S, INT48S, INT56S where numeric_limits<WorkingType>::max()
100-
// wouldn't work
101-
maxValue = static_cast<WorkingType>((1ULL << (bitWidth - 1)) - 1);
102-
minValue = static_cast<WorkingType>(-(1ULL << (bitWidth - 1)));
103-
}
104-
else
105-
{
106-
// We use emberAfAttributeSize for cases like INT24U, INT40U, INT48U, INT56U where numeric_limits<WorkingType>::max()
107-
// wouldn't work
108-
if (ZCL_INT64U_ATTRIBUTE_TYPE == app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
109-
{
110-
maxValue = static_cast<WorkingType>(UINT64_MAX); // Bit shift of 64 is undefined so we use UINT64_MAX
111-
}
112-
else
113-
{
114-
maxValue = static_cast<WorkingType>((1ULL << bitWidth) - 1);
115-
}
116-
minValue = static_cast<WorkingType>(0);
117-
}
94+
using IntType = app::NumericAttributeTraits<TypeForMinMax>;
95+
using WorkingType = std::remove_reference_t<decltype(value)>;
11896

119-
// Ensure that the metadata's signedness matches the working type's signedness
120-
VerifyOrDie(metadata->IsSignedIntegerAttribute() == std::is_signed<WorkingType>::value);
121-
122-
if (metadata->IsBoolean())
123-
{
124-
if (metadata->IsNullable() && (value != 1 && value != 0))
125-
{
126-
// If the attribute is nullable, the value can be set to NULL
127-
app::NumericAttributeTraits<WorkingType>::SetNull(value);
128-
}
129-
else
130-
{
131-
// Caping the value to 1 in case values greater than 1 are set
132-
value = value ? 1 : 0;
133-
}
134-
return;
135-
}
97+
WorkingType minValue = IntType::MinValue(metadata->IsNullable());
98+
WorkingType maxValue = IntType::MaxValue(metadata->IsNullable());
13699

137100
// Check metadata for min and max values
138101
if (metadata->HasMinMax())
@@ -142,10 +105,6 @@ void CapAttributeValue(typename app::NumericAttributeTraits<Type>::WorkingType &
142105
maxValue = ConvertDefaultValueToWorkingValue<Type>(minMaxValue->maxValue);
143106
}
144107

145-
// If the attribute is nullable, the min and max values calculated for types will not be valid, however this does not
146-
// 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
147-
// -127. The code above determin minValue = -128, so an input value of -128 would not enter the condition block below, but would
148-
// be considered NULL nonetheless.
149108
if (metadata->IsNullable() && (minValue > value || maxValue < value))
150109
{
151110
// If the attribute is nullable, the value can be set to NULL
@@ -187,6 +146,9 @@ CHIP_ERROR ValidateAttributePath(EndpointId endpoint, ClusterId cluster, Attribu
187146
switch (app::Compatibility::Internal::AttributeBaseType(metadata->attributeType))
188147
{
189148
case ZCL_BOOLEAN_ATTRIBUTE_TYPE:
149+
VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
150+
CapAttributeValue<uint8_t, bool>(aVPair.valueUnsigned8.Value(), metadata);
151+
break;
190152
case ZCL_INT8U_ATTRIBUTE_TYPE:
191153
VerifyOrReturnError(aVPair.valueUnsigned8.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
192154
CapAttributeValue<uint8_t>(aVPair.valueUnsigned8.Value(), metadata);

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

+37
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,39 @@ struct NumericAttributeTraits
127127
// Utility that lets consumers treat a StorageType instance as a uint8_t*
128128
// for writing to the attribute store.
129129
static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
130+
131+
// Min and max values for the type.
132+
static WorkingType MinValue(bool isNullable)
133+
{
134+
if constexpr (!std::is_signed_v<WorkingType>)
135+
{
136+
return 0;
137+
}
138+
139+
if (isNullable)
140+
{
141+
// Smallest negative value is excluded for nullable types.
142+
return static_cast<WorkingType>(std::numeric_limits<WorkingType>::min() + 1);
143+
}
144+
145+
return std::numeric_limits<WorkingType>::min();
146+
}
147+
148+
static WorkingType MaxValue(bool isNullable)
149+
{
150+
if constexpr (std::is_signed_v<WorkingType>)
151+
{
152+
return std::numeric_limits<WorkingType>::max();
153+
}
154+
155+
if (isNullable)
156+
{
157+
// Largest value is excluded for nullable types.
158+
return static_cast<WorkingType>(std::numeric_limits<WorkingType>::max() - 1);
159+
}
160+
161+
return std::numeric_limits<WorkingType>::max();
162+
}
130163
};
131164

132165
template <typename T>
@@ -209,6 +242,10 @@ struct NumericAttributeTraits<bool>
209242

210243
static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
211244

245+
static uint8_t MinValue(bool isNullable) { return 0; }
246+
247+
static uint8_t MaxValue(bool isNullable) { return 1; }
248+
212249
private:
213250
static constexpr StorageType kNullValue = 0xFF;
214251
};

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

+36-19
Original file line numberDiff line numberDiff line change
@@ -200,35 +200,52 @@ struct NumericAttributeTraits<OddSizedInteger<ByteSize, IsSigned>, IsBigEndian>
200200

201201
static constexpr bool CanRepresentValue(bool isNullable, WorkingType value)
202202
{
203-
// Since WorkingType has at least one extra byte, none of our bitshifts
204-
// overflow.
205-
if (IsSigned)
203+
return MinValue(isNullable) <= value && value <= MaxValue(isNullable);
204+
}
205+
206+
static CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, StorageType value)
207+
{
208+
return writer.Put(tag, StorageToWorking(value));
209+
}
210+
211+
static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; }
212+
213+
static WorkingType MinValue(bool isNullable)
214+
{
215+
if constexpr (!IsSigned)
206216
{
207-
WorkingType max = (static_cast<WorkingType>(1) << (8 * ByteSize - 1)) - 1;
208-
WorkingType min = -max;
209-
if (!isNullable)
210-
{
211-
// We have one more value.
212-
min -= 1;
213-
}
214-
return value >= min && value <= max;
217+
return 0;
215218
}
216219

217-
WorkingType max = (static_cast<WorkingType>(1) << (8 * ByteSize)) - 1;
220+
// Since WorkingType has at least one extra byte, the bitshift cannot overflow.
221+
constexpr WorkingType signedMin = -(static_cast<WorkingType>(1) << (8 * ByteSize - 1));
218222
if (isNullable)
219223
{
220-
// we have one less value
221-
max -= 1;
224+
// We have one fewer value.
225+
return signedMin + 1;
222226
}
223-
return value <= max;
227+
228+
return signedMin;
224229
}
225230

226-
static CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, StorageType value)
231+
static WorkingType MaxValue(bool isNullable)
227232
{
228-
return writer.Put(tag, StorageToWorking(value));
229-
}
233+
// Since WorkingType has at least one extra byte, none of our bitshifts
234+
// overflow.
235+
if constexpr (IsSigned)
236+
{
237+
return (static_cast<WorkingType>(1) << (8 * ByteSize - 1)) - 1;
238+
}
230239

231-
static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return value; }
240+
constexpr WorkingType unsignedMax = (static_cast<WorkingType>(1) << (8 * ByteSize));
241+
if (isNullable)
242+
{
243+
// Largest value is excluded for nullable types.
244+
return unsignedMax - 1;
245+
}
246+
247+
return unsignedMax;
248+
}
232249
};
233250

234251
} // namespace app

0 commit comments

Comments
 (0)