mirror of
https://github.com/esphome/esphome.git
synced 2026-09-14 16:48:40 +00:00
[light] Alias clamp fields via anonymous-union float[8] to eliminate pointer UB
Replaces the previous `float *p = &this->brightness_; p[bit]` pattern (flagged by Copilot as pointer-into-scalar UB) with a shared anonymous union that exposes brightness_..warm_white_ as unit_fields_[8] in both LightCall and LightColorValues. validate_() now indexes the real array directly — defined behavior — while the named members stay accessible to getters/setters. Code generation is unchanged. The union is declared once via ESPHOME_LIGHT_UNIT_FIELDS_UNION() in light_color_values.h and expanded in both structs. The per-field offsetof static_asserts collapse to a single FieldFlags bit-layout assert since the union guarantees member ↔ array-index alignment. Caught by Copilot on PR review.
This commit is contained in:
@@ -11,8 +11,8 @@ namespace esphome::light {
|
||||
|
||||
static const char *const TAG = "light";
|
||||
|
||||
// Cold-path logger. Caller handles the clamp so the in-range hot path avoids
|
||||
// the call-site spill/reload around the out-of-line call.
|
||||
// Cold-path logger; caller handles the clamp so the in-range hot path avoids
|
||||
// the spill/reload around the call.
|
||||
static void log_value_out_of_range_(const char *name, float value, const LogString *param_name, float min, float max) {
|
||||
ESP_LOGW(TAG, "'%s': %s value %.2f is out of range [%.1f - %.1f]", name, LOG_STR_ARG(param_name), value, min, max);
|
||||
}
|
||||
@@ -55,20 +55,10 @@ static void log_invalid_parameter(const char *name, const LogString *message) {
|
||||
PROGMEM_STRING_TABLE(ColorModeHumanStrings, "Unknown", "On/Off", "Brightness", "White", "Color temperature",
|
||||
"Cold/warm white", "RGB", "RGBW", "RGB + color temperature", "RGB + cold/warm white");
|
||||
|
||||
// Field names for validate_(). PROGMEM_STRING_TABLE uses constexpr init so no
|
||||
// per-static guard variables in RAM (a plain LOG_STR array of static locals
|
||||
// would need them because LOG_STR is a statement-expression on ESP8266).
|
||||
// Indices 0-7 match FieldFlags bits 0-7; index 8 is color_temperature.
|
||||
PROGMEM_STRING_TABLE(ValidateFieldNames,
|
||||
"Brightness", // FLAG_HAS_BRIGHTNESS (bit 0)
|
||||
"Color brightness", // FLAG_HAS_COLOR_BRIGHTNESS (bit 1)
|
||||
"Red", // FLAG_HAS_RED (bit 2)
|
||||
"Green", // FLAG_HAS_GREEN (bit 3)
|
||||
"Blue", // FLAG_HAS_BLUE (bit 4)
|
||||
"White", // FLAG_HAS_WHITE (bit 5)
|
||||
"Cold white", // FLAG_HAS_COLD_WHITE (bit 6)
|
||||
"Warm white", // FLAG_HAS_WARM_WHITE (bit 7)
|
||||
"Color temperature");
|
||||
// PROGMEM_STRING_TABLE is constexpr-init (no RAM guard variable).
|
||||
PROGMEM_STRING_TABLE(ValidateFieldNames, "Brightness", "Color brightness", "Red", "Green", "Blue", "White",
|
||||
"Cold white", "Warm white", "Color temperature");
|
||||
static constexpr uint8_t VALIDATE_CT_INDEX = 8;
|
||||
|
||||
static const LogString *color_mode_to_human(ColorMode color_mode) {
|
||||
@@ -291,50 +281,30 @@ LightColorValues LightCall::validate_() {
|
||||
if (this->has_state())
|
||||
v.set_state(this->state_);
|
||||
|
||||
// Clamp the eight [0.0, 1.0] fields and copy them from `this` into `v`.
|
||||
// Both structs declare the same fields in the same order as FieldFlags bits
|
||||
// 0-7, with a constant byte-offset delta. The asserts below pin that layout
|
||||
// so the loop can index by bit position; any single-field reorder trips the
|
||||
// assert naming the field at fault.
|
||||
// FieldFlags bits 0-7 must match unit_fields_ array indices; the union in
|
||||
// both structs guarantees brightness_..warm_white_ alias unit_fields_[0..7].
|
||||
static_assert(std::is_standard_layout_v<LightCall>, "LightCall must be standard-layout");
|
||||
static_assert(std::is_standard_layout_v<LightColorValues>, "LightColorValues must be standard-layout");
|
||||
static_assert(FLAG_HAS_BRIGHTNESS == 1u << 0 && FLAG_HAS_COLOR_BRIGHTNESS == 1u << 1 && FLAG_HAS_RED == 1u << 2 &&
|
||||
FLAG_HAS_GREEN == 1u << 3 && FLAG_HAS_BLUE == 1u << 4 && FLAG_HAS_WHITE == 1u << 5 &&
|
||||
FLAG_HAS_COLD_WHITE == 1u << 6 && FLAG_HAS_WARM_WHITE == 1u << 7,
|
||||
"FieldFlags bits 0-7 must match unit_fields_ indices");
|
||||
|
||||
constexpr size_t SRC_BASE = offsetof(LightCall, brightness_);
|
||||
constexpr size_t SRC_TO_DST_DELTA = SRC_BASE - offsetof(LightColorValues, brightness_);
|
||||
|
||||
#define ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(bit, flag_suffix, member) \
|
||||
static_assert(FLAG_HAS_##flag_suffix == 1u << (bit), "FLAG_HAS_" #flag_suffix " bit position"); \
|
||||
static_assert(offsetof(LightCall, member) == SRC_BASE + (bit) * sizeof(float), \
|
||||
"LightCall::" #member " must be at bit-indexed slot"); \
|
||||
static_assert(offsetof(LightColorValues, member) == SRC_BASE + (bit) * sizeof(float) - SRC_TO_DST_DELTA, \
|
||||
"LightColorValues::" #member " must match LightCall delta")
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(0, BRIGHTNESS, brightness_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(1, COLOR_BRIGHTNESS, color_brightness_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(2, RED, red_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(3, GREEN, green_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(4, BLUE, blue_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(5, WHITE, white_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(6, COLD_WHITE, cold_white_);
|
||||
ESPHOME_LIGHT_ASSERT_CLAMP_FIELD(7, WARM_WHITE, warm_white_);
|
||||
#undef ESPHOME_LIGHT_ASSERT_CLAMP_FIELD
|
||||
|
||||
// Iterate only the set bits (ctz + clear-lowest) so the hot path is
|
||||
// O(popcount) — HA can drive perform() at high frequency.
|
||||
float *const src_fields = &this->brightness_;
|
||||
float *const dst_fields = &v.brightness_;
|
||||
// Iterate set bits only (ctz + clear-lowest) — HA can drive perform()
|
||||
// at high frequency so the hot path is O(popcount).
|
||||
unsigned active = this->flags_ & CLAMP_FLAGS_MASK;
|
||||
while (active != 0) {
|
||||
unsigned bit = __builtin_ctz(active);
|
||||
active &= active - 1; // clear lowest set bit
|
||||
float &value = src_fields[bit];
|
||||
float &value = this->unit_fields_[bit];
|
||||
if (float_out_of_unit_range(value)) {
|
||||
log_value_out_of_range_(name, value, ValidateFieldNames::get_log_str(bit, 0), 0.0f, 1.0f);
|
||||
value = clamp_unit_float(value);
|
||||
}
|
||||
dst_fields[bit] = value;
|
||||
v.unit_fields_[bit] = value;
|
||||
}
|
||||
|
||||
// color_temperature has a runtime range from traits — no bit-pattern shortcut.
|
||||
// color_temperature: runtime range from traits.
|
||||
if (this->has_color_temperature()) {
|
||||
const float ct_min = traits.get_min_mireds();
|
||||
const float ct_max = traits.get_max_mireds();
|
||||
|
||||
@@ -195,10 +195,7 @@ class LightCall {
|
||||
/// Some color modes also can be set using non-native parameters, transform those calls.
|
||||
void transform_parameters_(const LightTraits &traits);
|
||||
|
||||
// Each flag indicates whether the corresponding value has been set. Bits 0-7
|
||||
// are the [0.0, 1.0] clamp fields; validate_() iterates them via bit-position
|
||||
// arithmetic and asserts the layout — don't reorder without matching the
|
||||
// static_asserts in light_call.cpp.
|
||||
// Bits 0-7 index unit_fields_[] in validate_(); don't reorder (asserts in light_call.cpp).
|
||||
enum FieldFlags : uint16_t {
|
||||
FLAG_HAS_BRIGHTNESS = 1 << 0,
|
||||
FLAG_HAS_COLOR_BRIGHTNESS = 1 << 1,
|
||||
@@ -243,18 +240,10 @@ class LightCall {
|
||||
LightState *parent_;
|
||||
|
||||
// Light state values - use flags_ to check if a value has been set.
|
||||
// brightness_..warm_white_ match FieldFlags bits 0-7 in order (see validate_).
|
||||
uint32_t transition_length_;
|
||||
uint32_t flash_length_;
|
||||
uint32_t effect_;
|
||||
float brightness_;
|
||||
float color_brightness_;
|
||||
float red_;
|
||||
float green_;
|
||||
float blue_;
|
||||
float white_;
|
||||
float cold_white_;
|
||||
float warm_white_;
|
||||
ESPHOME_LIGHT_UNIT_FIELDS_UNION();
|
||||
float color_temperature_;
|
||||
|
||||
// Smaller members at the end for better packing
|
||||
|
||||
@@ -10,20 +10,16 @@ namespace esphome::light {
|
||||
|
||||
inline static uint8_t to_uint8_scale(float x) { return static_cast<uint8_t>(roundf(x * 255.0f)); }
|
||||
|
||||
// Bit pattern of 1.0f. Values in [0.0f, 1.0f] have bits <= this; out-of-range
|
||||
// values (including negatives, whose sign bit makes their uint32 huge) exceed
|
||||
// it. Lets a single unsigned compare replace two soft-float calls on ESP8266.
|
||||
static constexpr uint32_t ONE_F_BITS = 0x3F800000u;
|
||||
// Bit pattern of -0.0f: sign bit set, magnitude zero. Treated as in range.
|
||||
static constexpr uint32_t NEG_ZERO_F_BITS = 0x80000000u;
|
||||
// IEEE 754 bit patterns. Values in [0.0f, 1.0f] have bits <= ONE_F_BITS;
|
||||
// negatives have the sign bit set (→ huge unsigned). A single unsigned compare
|
||||
// replaces two soft-float __ltsf2/__gtsf2 calls on ESP8266.
|
||||
static constexpr uint32_t ONE_F_BITS = 0x3F800000u; // 1.0f
|
||||
static constexpr uint32_t NEG_ZERO_F_BITS = 0x80000000u; // -0.0f / sign-bit mask
|
||||
static_assert(sizeof(float) == sizeof(uint32_t), "float must be 32-bit");
|
||||
static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float required");
|
||||
|
||||
// Union type-pun (GCC/Clang extension): memcpy/bit_cast don't fold to a no-op
|
||||
// on xtensa-gcc. Same reasoning as api/proto.h's float_to_raw().
|
||||
// -0.0f (bit pattern 0x80000000) exceeds ONE_F_BITS as unsigned but is
|
||||
// numerically zero and clamps to 0.0f anyway — treat it as in range so we
|
||||
// don't log a spurious out-of-range warning.
|
||||
// Union pun — memcpy/bit_cast don't fold on xtensa-gcc (see api/proto.h).
|
||||
// -0.0f counts as in range (clamps to 0.0f anyway; don't log a false warning).
|
||||
inline bool float_out_of_unit_range(float x) {
|
||||
union {
|
||||
float f;
|
||||
@@ -33,8 +29,7 @@ inline bool float_out_of_unit_range(float x) {
|
||||
return pun.u > ONE_F_BITS && pun.u != NEG_ZERO_F_BITS;
|
||||
}
|
||||
|
||||
// Clamps to [0.0f, 1.0f] without float compares. Negatives (sign bit set)
|
||||
// fold to 0.0f; everything else out of range (>1, NaN, Inf) folds to 1.0f.
|
||||
// Clamps to [0.0f, 1.0f] without float compares. Negatives → 0; >1/NaN/Inf → 1.
|
||||
inline float clamp_unit_float(float x) {
|
||||
union {
|
||||
float f;
|
||||
@@ -46,6 +41,23 @@ inline float clamp_unit_float(float x) {
|
||||
return (pun.u & NEG_ZERO_F_BITS) ? 0.0f : 1.0f; // sign bit → negative → clamp to 0
|
||||
}
|
||||
|
||||
// Shared anonymous union: eight unit-range floats alias unit_fields_[8] so
|
||||
// LightCall::validate_() can iterate them as a real array. GCC/Clang ext.
|
||||
#define ESPHOME_LIGHT_UNIT_FIELDS_UNION() \
|
||||
union { \
|
||||
struct { \
|
||||
float brightness_; \
|
||||
float color_brightness_; \
|
||||
float red_; \
|
||||
float green_; \
|
||||
float blue_; \
|
||||
float white_; \
|
||||
float cold_white_; \
|
||||
float warm_white_; \
|
||||
}; \
|
||||
float unit_fields_[8]; \
|
||||
}
|
||||
|
||||
/** This class represents the color state for a light object.
|
||||
*
|
||||
* The representation of the color state is dependent on the active color mode. A color mode consists of multiple
|
||||
@@ -325,17 +337,8 @@ class LightColorValues {
|
||||
friend class LightCall;
|
||||
|
||||
protected:
|
||||
// brightness_..warm_white_ match LightCall::FieldFlags bits 0-7 in order.
|
||||
// LightCall::validate_() relies on this layout via static_asserts.
|
||||
float state_; ///< ON / OFF, float for transition
|
||||
float brightness_;
|
||||
float color_brightness_;
|
||||
float red_;
|
||||
float green_;
|
||||
float blue_;
|
||||
float white_;
|
||||
float cold_white_;
|
||||
float warm_white_;
|
||||
ESPHOME_LIGHT_UNIT_FIELDS_UNION();
|
||||
float color_temperature_; ///< Color Temperature in Mired
|
||||
ColorMode color_mode_;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user