From de44b8e859bf500721db4a4124785ac9a497bbc8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 13 Apr 2026 16:31:50 -1000 Subject: [PATCH] [light] Tighten comments in validate_ clamp loop and LightColorValues helpers --- esphome/components/light/light_call.cpp | 54 +++++-------------- esphome/components/light/light_call.h | 17 ++---- esphome/components/light/light_color_values.h | 37 ++++--------- 3 files changed, 27 insertions(+), 81 deletions(-) diff --git a/esphome/components/light/light_call.cpp b/esphome/components/light/light_call.cpp index bc0b12aa77..43fea14e4e 100644 --- a/esphome/components/light/light_call.cpp +++ b/esphome/components/light/light_call.cpp @@ -11,14 +11,9 @@ namespace esphome::light { static const char *const TAG = "light"; -// Cold-path logger: called only after the caller has determined `value` is -// out of range. Does not clamp — the caller handles that with the strategy -// appropriate to its range (bit-pattern clamp_unit_float for [0,1] on the -// hot path, std::clamp for arbitrary ranges like color_temperature). Keeping -// the range check at the caller avoids the call-site spill/reload and -// prologue when the value is in range. The `param_name_progmem` argument -// points into the FIELD_NAMES table in flash; `progmem_read_ptr` is a plain -// `*addr` inline on non-ESP8266 platforms. +// Cold-path logger. Caller handles the clamp so the in-range hot path avoids +// the call-site spill/reload. `param_name_progmem` is a pointer into FIELD_NAMES +// in flash; the `progmem_read_ptr` is a no-op on non-ESP8266. static void log_value_out_of_range_(const char *name, float value, const LogString *const *param_name_progmem, float min, float max) { const auto *param_name = @@ -285,34 +280,16 @@ LightColorValues LightCall::validate_() { v.set_state(this->state_); // Clamp the eight [0.0, 1.0] fields and copy them from `this` into `v`. - // - // LightCall and LightColorValues both declare the same eight float fields in - // the same order (brightness_, color_brightness_, red_, green_, blue_, - // white_, cold_white_, warm_white_), and their corresponding flag bits are - // also 0-7 in that order. Under that layout the LightCall offset for field i - // is `offsetof(LightCall, brightness_) + i * 4`, and the LightColorValues - // offset is exactly 12 bytes lower (enforced by the static_asserts below). - // Iterating via bit-position arithmetic lets us collapse eight inlined - // clamp/copy blocks into a single loop. - // offsetof is only well-defined on standard-layout types (C++17 relaxed it - // slightly, but GCC still warns on non-standard-layout). Verify here rather - // than relying on diagnostics: a future change that adds a virtual base, a - // non-public data member mixed with public ones, or a derived-class data - // member would break the layout contract below. - static_assert(std::is_standard_layout_v, "LightCall must be standard-layout for offsetof arithmetic"); - static_assert(std::is_standard_layout_v, - "LightColorValues must be standard-layout for offsetof arithmetic"); + // 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. + static_assert(std::is_standard_layout_v, "LightCall must be standard-layout"); + static_assert(std::is_standard_layout_v, "LightColorValues must be standard-layout"); constexpr size_t SRC_BASE = offsetof(LightCall, brightness_); constexpr size_t SRC_TO_DST_DELTA = SRC_BASE - offsetof(LightColorValues, brightness_); - // Per-field layout assertions: each clamp field must sit at its bit-indexed - // slot in both LightCall and LightColorValues, with the same byte-offset - // delta. A reorder of any single field (in either struct) trips the assert - // pointing at that field, so failures name the exact member at fault. - // The one case these cannot catch is a synchronized reorder in both structs - // plus FIELD_NAMES — that would compile silently, but requires deliberate - // three-place changes by the refactorer. #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), \ @@ -340,13 +317,8 @@ LightColorValues LightCall::validate_() { LOG_STR("Warm white"), // FLAG_HAS_WARM_WHITE (bit 7) }; - // The static_asserts above guarantee the eight clampable floats are laid - // out consecutively starting at brightness_ in both structs, so we can - // treat `&brightness_` as the base of an 8-element float array and index - // by bit position directly. Iterate only the set bits via __builtin_ctz + - // clear-lowest-bit: HA can drive high-frequency automations through - // perform(), so the hot path runs in O(popcount) instead of always - // scanning all eight slots. + // 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_; unsigned active = this->flags_ & CLAMP_FLAGS_MASK; @@ -361,9 +333,7 @@ LightColorValues LightCall::validate_() { dst_fields[bit] = value; } - // color_temperature uses a dynamic range from the light's traits and is - // handled separately. No bit-pattern shortcut here because the range is - // runtime-variable. + // color_temperature has a runtime range from traits — no bit-pattern shortcut. if (this->has_color_temperature()) { static const LogString *const CT_NAME PROGMEM = LOG_STR("Color temperature"); const float ct_min = traits.get_min_mireds(); diff --git a/esphome/components/light/light_call.h b/esphome/components/light/light_call.h index 9f34297dcd..01591143d3 100644 --- a/esphome/components/light/light_call.h +++ b/esphome/components/light/light_call.h @@ -195,12 +195,9 @@ class LightCall { /// Some color modes also can be set using non-native parameters, transform those calls. void transform_parameters_(const LightTraits &traits); - // Bitfield flags - each flag indicates whether a corresponding value has been set. - // - // Bits 0-7 are the eight float fields that share the [0.0, 1.0] clamp range, - // in member declaration order. The validate_() clamp loop relies on this - // layout to index into LightCall/LightColorValues via bit-position arithmetic - // without a per-field offset table. Do not reorder without updating the + // 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. enum FieldFlags : uint16_t { FLAG_HAS_BRIGHTNESS = 1 << 0, @@ -246,13 +243,7 @@ class LightCall { LightState *parent_; // Light state values - use flags_ to check if a value has been set. - // Group 4-byte aligned members first. - // - // The eight [0.0, 1.0]-clamped float fields (brightness_ ... warm_white_) - // are declared in the same order as their flag bits (0-7) and the matching - // fields in LightColorValues. validate_() exploits this to iterate them via - // bit-position arithmetic. color_temperature_ has a custom range and lives - // outside that block. + // brightness_..warm_white_ match FieldFlags bits 0-7 in order (see validate_). uint32_t transition_length_; uint32_t flash_length_; uint32_t effect_; diff --git a/esphome/components/light/light_color_values.h b/esphome/components/light/light_color_values.h index 74af29fe59..1c41d72716 100644 --- a/esphome/components/light/light_color_values.h +++ b/esphome/components/light/light_color_values.h @@ -10,24 +10,15 @@ namespace esphome::light { inline static uint8_t to_uint8_scale(float x) { return static_cast(roundf(x * 255.0f)); } -// IEEE 754 bit pattern of 1.0f. Floats in [0.0f, 1.0f] have unsigned bit -// pattern <= this value; negatives have the sign bit set (→ huge unsigned), -// values > 1.0f have a larger exponent, and NaN/Infinity also exceed this. -// Verify the platform actually provides IEEE 754 single-precision floats so -// the bit-pattern tricks below are well-defined. +// 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; -// sizeof check + is_iec559 together pin the format to IEEE 754 single-precision, -// which fixes the bit pattern of 1.0f as 0x3F800000. A direct bit-cast check -// would be cleaner but __builtin_bit_cast is not available on the older xtensa -// toolchain used for ESP8266. -static_assert(sizeof(float) == sizeof(uint32_t), "float must be 32-bit for bit-pattern range checks"); -static_assert(std::numeric_limits::is_iec559, "IEEE 754 single-precision float required"); +static_assert(sizeof(float) == sizeof(uint32_t), "float must be 32-bit"); +static_assert(std::numeric_limits::is_iec559, "IEEE 754 float required"); -// Returns true iff `x` is outside [0.0f, 1.0f] via a single unsigned compare on -// its IEEE 754 bit pattern. Uses a union type-pun (GCC/Clang extension) because -// memcpy/bit_cast don't optimize to a no-op on xtensa-gcc (same reasoning as -// api/proto.h's float_to_raw). Replaces two soft-float __ltsf2/__gtsf2 calls -// with one `bltu` on ESP8266 and is free on FPU targets. +// 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(). inline bool float_out_of_unit_range(float x) { union { float f; @@ -37,10 +28,8 @@ inline bool float_out_of_unit_range(float x) { return pun.u > ONE_F_BITS; } -// Clamps `x` to [0.0f, 1.0f] with no floating-point compares. In-range values -// return via a single branch; out-of-range pick 0.0f for negatives (sign bit -// set) and 1.0f otherwise (> 1.0f, NaN, Infinity). Cheaper than std::clamp on -// ESP8266, which expands to two soft-float calls per invocation. +// 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. inline float clamp_unit_float(float x) { union { float f; @@ -331,12 +320,8 @@ class LightColorValues { friend class LightCall; protected: - // The eight [0.0, 1.0]-clamped float fields are declared in the same order - // as their flag bits (0-7) in LightCall::FieldFlags and the matching fields - // in LightCall. LightCall::validate_() exploits this layout to iterate and - // copy them via bit-position arithmetic with a constant delta of 12 bytes - // between matching LightCall and LightColorValues members. color_temperature_ - // has a different range and is placed after the clamp block. + // 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_;