From 6491072a77d15752b25223f489015ba41e63105e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 13 Apr 2026 16:54:43 -1000 Subject: [PATCH] [light] Alias clamp fields via anonymous-union float[8] to eliminate pointer UB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- esphome/components/light/light_call.cpp | 62 +++++-------------- esphome/components/light/light_call.h | 15 +---- esphome/components/light/light_color_values.h | 49 ++++++++------- 3 files changed, 44 insertions(+), 82 deletions(-) diff --git a/esphome/components/light/light_call.cpp b/esphome/components/light/light_call.cpp index ef344a9a74..7edcd9c82b 100644 --- a/esphome/components/light/light_call.cpp +++ b/esphome/components/light/light_call.cpp @@ -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 must be standard-layout"); static_assert(std::is_standard_layout_v, "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(); diff --git a/esphome/components/light/light_call.h b/esphome/components/light/light_call.h index 01591143d3..054f1b1571 100644 --- a/esphome/components/light/light_call.h +++ b/esphome/components/light/light_call.h @@ -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 diff --git a/esphome/components/light/light_color_values.h b/esphome/components/light/light_color_values.h index 7efea56dbb..43b5e325b8 100644 --- a/esphome/components/light/light_color_values.h +++ b/esphome/components/light/light_color_values.h @@ -10,20 +10,16 @@ namespace esphome::light { inline static uint8_t to_uint8_scale(float x) { return static_cast(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::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_; };