From eeeae53f76b57c52af9ca442d6e9aba7ca8521db Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 11 Jan 2026 17:40:09 -1000 Subject: [PATCH] [fan] Return StringRef from get_preset_mode() for safety and modern API (#13092) --- esphome/components/api/api_connection.cpp | 2 +- esphome/components/copy/fan/copy_fan.cpp | 18 ++++--- esphome/components/copy/fan/copy_fan.h | 2 +- esphome/components/fan/automation.h | 7 ++- esphome/components/fan/fan.cpp | 45 ++++++++++++----- esphome/components/fan/fan.h | 14 ++++-- .../components/hbridge/fan/hbridge_fan.cpp | 2 +- esphome/components/speed/fan/speed_fan.cpp | 2 +- .../components/template/fan/template_fan.cpp | 2 +- tests/components/copy/common.yaml | 3 ++ tests/components/fan/common.yaml | 48 +++++++++++++++++++ 11 files changed, 113 insertions(+), 32 deletions(-) diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index fb3548d117..4bc19a8bad 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -443,7 +443,7 @@ uint16_t APIConnection::try_send_fan_state(EntityBase *entity, APIConnection *co if (traits.supports_direction()) msg.direction = static_cast(fan->direction); if (traits.supports_preset_modes() && fan->has_preset_mode()) - msg.preset_mode = StringRef(fan->get_preset_mode()); + msg.preset_mode = fan->get_preset_mode(); return fill_and_encode_entity_state(fan, msg, FanStateResponse::MESSAGE_TYPE, conn, remaining_size, is_single); } uint16_t APIConnection::try_send_fan_info(EntityBase *entity, APIConnection *conn, uint32_t remaining_size, diff --git a/esphome/components/copy/fan/copy_fan.cpp b/esphome/components/copy/fan/copy_fan.cpp index d35ece950b..b4a43cf2f1 100644 --- a/esphome/components/copy/fan/copy_fan.cpp +++ b/esphome/components/copy/fan/copy_fan.cpp @@ -8,20 +8,24 @@ static const char *const TAG = "copy.fan"; void CopyFan::setup() { source_->add_on_state_callback([this]() { - this->state = source_->state; - this->oscillating = source_->oscillating; - this->speed = source_->speed; - this->direction = source_->direction; - this->set_preset_mode_(source_->get_preset_mode()); + this->copy_state_from_source_(); this->publish_state(); }); + this->copy_state_from_source_(); + this->publish_state(); +} + +void CopyFan::copy_state_from_source_() { this->state = source_->state; this->oscillating = source_->oscillating; this->speed = source_->speed; this->direction = source_->direction; - this->set_preset_mode_(source_->get_preset_mode()); - this->publish_state(); + if (source_->has_preset_mode()) { + this->set_preset_mode_(source_->get_preset_mode()); + } else { + this->clear_preset_mode_(); + } } void CopyFan::dump_config() { LOG_FAN("", "Copy Fan", this); } diff --git a/esphome/components/copy/fan/copy_fan.h b/esphome/components/copy/fan/copy_fan.h index b474975bc4..988129f07b 100644 --- a/esphome/components/copy/fan/copy_fan.h +++ b/esphome/components/copy/fan/copy_fan.h @@ -16,7 +16,7 @@ class CopyFan : public fan::Fan, public Component { protected: void control(const fan::FanCall &call) override; - ; + void copy_state_from_source_(); fan::Fan *source_; }; diff --git a/esphome/components/fan/automation.h b/esphome/components/fan/automation.h index ce1db6fc64..77abc2f13f 100644 --- a/esphome/components/fan/automation.h +++ b/esphome/components/fan/automation.h @@ -212,19 +212,18 @@ class FanPresetSetTrigger : public Trigger { public: FanPresetSetTrigger(Fan *state) { state->add_on_state_callback([this, state]() { - const auto *preset_mode = state->get_preset_mode(); + auto preset_mode = state->get_preset_mode(); auto should_trigger = preset_mode != this->last_preset_mode_; this->last_preset_mode_ = preset_mode; if (should_trigger) { - // Trigger with empty string when nullptr to maintain backward compatibility - this->trigger(preset_mode != nullptr ? preset_mode : ""); + this->trigger(std::string(preset_mode)); } }); this->last_preset_mode_ = state->get_preset_mode(); } protected: - const char *last_preset_mode_{nullptr}; + StringRef last_preset_mode_{}; }; } // namespace fan diff --git a/esphome/components/fan/fan.cpp b/esphome/components/fan/fan.cpp index 0ffb60e50d..2e48d84eb9 100644 --- a/esphome/components/fan/fan.cpp +++ b/esphome/components/fan/fan.cpp @@ -61,7 +61,7 @@ void FanCall::perform() { if (this->direction_.has_value()) { ESP_LOGD(TAG, " Direction: %s", LOG_STR_ARG(fan_direction_to_string(*this->direction_))); } - if (this->has_preset_mode()) { + if (this->preset_mode_ != nullptr) { ESP_LOGD(TAG, " Preset Mode: %s", this->preset_mode_); } this->parent_.control(*this); @@ -83,7 +83,7 @@ void FanCall::validate_() { *this->binary_state_ // ..,and no preset mode will be active... && !this->has_preset_mode() && - this->parent_.get_preset_mode() == nullptr + !this->parent_.has_preset_mode() // ...and neither current nor new speed is available... && traits.supports_speed() && this->parent_.speed == 0 && !this->speed_.has_value()) { // ...set speed to 100% @@ -154,16 +154,16 @@ const char *Fan::find_preset_mode_(const char *preset_mode, size_t len) { return this->get_traits().find_preset_mode(preset_mode, len); } -bool Fan::set_preset_mode_(const char *preset_mode) { - if (preset_mode == nullptr) { - // Treat nullptr as clearing the preset mode +bool Fan::set_preset_mode_(const char *preset_mode, size_t len) { + if (preset_mode == nullptr || len == 0) { + // Treat nullptr or empty string as clearing the preset mode (no valid preset is "") if (this->preset_mode_ == nullptr) { return false; // No change } this->clear_preset_mode_(); return true; } - const char *validated = this->find_preset_mode_(preset_mode); + const char *validated = this->find_preset_mode_(preset_mode, len); if (validated == nullptr || this->preset_mode_ == validated) { return false; // Preset mode not supported or no change } @@ -171,10 +171,31 @@ bool Fan::set_preset_mode_(const char *preset_mode) { return true; } -bool Fan::set_preset_mode_(const std::string &preset_mode) { return this->set_preset_mode_(preset_mode.c_str()); } +bool Fan::set_preset_mode_(const char *preset_mode) { + return this->set_preset_mode_(preset_mode, preset_mode ? strlen(preset_mode) : 0); +} + +bool Fan::set_preset_mode_(const std::string &preset_mode) { + return this->set_preset_mode_(preset_mode.data(), preset_mode.size()); +} + +bool Fan::set_preset_mode_(StringRef preset_mode) { + // Safe: find_preset_mode_ only uses the input for comparison and returns + // a pointer from traits, so the input StringRef's lifetime doesn't matter. + return this->set_preset_mode_(preset_mode.c_str(), preset_mode.size()); +} void Fan::clear_preset_mode_() { this->preset_mode_ = nullptr; } +void Fan::apply_preset_mode_(const FanCall &call) { + if (call.has_preset_mode()) { + this->set_preset_mode_(call.get_preset_mode()); + } else if (call.get_speed().has_value()) { + // Manually setting speed clears preset (per Home Assistant convention) + this->clear_preset_mode_(); + } +} + void Fan::add_on_state_callback(std::function &&callback) { this->state_callback_.add(std::move(callback)); } void Fan::publish_state() { auto traits = this->get_traits(); @@ -192,9 +213,8 @@ void Fan::publish_state() { if (traits.supports_direction()) { ESP_LOGD(TAG, " Direction: %s", LOG_STR_ARG(fan_direction_to_string(this->direction))); } - const char *preset = this->get_preset_mode(); - if (preset != nullptr) { - ESP_LOGD(TAG, " Preset Mode: %s", preset); + if (this->preset_mode_ != nullptr) { + ESP_LOGD(TAG, " Preset Mode: %s", this->preset_mode_); } this->state_callback_.call(); #if defined(USE_FAN) && defined(USE_CONTROLLER_REGISTRY) @@ -249,12 +269,11 @@ void Fan::save_state_() { state.speed = this->speed; state.direction = this->direction; - const char *preset = this->get_preset_mode(); - if (preset != nullptr) { + if (this->has_preset_mode()) { const auto &preset_modes = traits.supported_preset_modes(); // Find index of current preset mode (pointer comparison is safe since preset is from traits) for (size_t i = 0; i < preset_modes.size(); i++) { - if (preset_modes[i] == preset) { + if (preset_modes[i] == this->preset_mode_) { state.preset_mode = i; break; } diff --git a/esphome/components/fan/fan.h b/esphome/components/fan/fan.h index 7c79fda83e..55d4ba8825 100644 --- a/esphome/components/fan/fan.h +++ b/esphome/components/fan/fan.h @@ -5,6 +5,7 @@ #include "esphome/core/log.h" #include "esphome/core/optional.h" #include "esphome/core/preferences.h" +#include "esphome/core/string_ref.h" #include "fan_traits.h" namespace esphome { @@ -128,8 +129,11 @@ class Fan : public EntityBase { /// Set the restore mode of this fan. void set_restore_mode(FanRestoreMode restore_mode) { this->restore_mode_ = restore_mode; } - /// Get the current preset mode (returns pointer to string stored in traits, or nullptr if not set) - const char *get_preset_mode() const { return this->preset_mode_; } + /// Get the current preset mode. + /// Returns a StringRef of the string stored in traits, or empty ref if not set. + /// The returned ref points to string literals from codegen (static storage). + /// Traits are set once at startup and valid for the lifetime of the program. + StringRef get_preset_mode() const { return StringRef::from_maybe_nullptr(this->preset_mode_); } /// Check if a preset mode is currently active bool has_preset_mode() const { return this->preset_mode_ != nullptr; } @@ -146,11 +150,15 @@ class Fan : public EntityBase { void dump_traits_(const char *tag, const char *prefix); /// Set the preset mode (finds and stores pointer from traits). Returns true if changed. + /// Passing nullptr or empty string clears the preset mode. + bool set_preset_mode_(const char *preset_mode, size_t len); bool set_preset_mode_(const char *preset_mode); - /// Set the preset mode (finds and stores pointer from traits). Returns true if changed. bool set_preset_mode_(const std::string &preset_mode); + bool set_preset_mode_(StringRef preset_mode); /// Clear the preset mode void clear_preset_mode_(); + /// Apply preset mode from a FanCall (handles speed-clears-preset convention) + void apply_preset_mode_(const FanCall &call); /// Find and return the matching preset mode pointer from traits, or nullptr if not found. const char *find_preset_mode_(const char *preset_mode); const char *find_preset_mode_(const char *preset_mode, size_t len); diff --git a/esphome/components/hbridge/fan/hbridge_fan.cpp b/esphome/components/hbridge/fan/hbridge_fan.cpp index 488208b725..9bf58f9d1e 100644 --- a/esphome/components/hbridge/fan/hbridge_fan.cpp +++ b/esphome/components/hbridge/fan/hbridge_fan.cpp @@ -57,7 +57,7 @@ void HBridgeFan::control(const fan::FanCall &call) { this->oscillating = *call.get_oscillating(); if (call.get_direction().has_value()) this->direction = *call.get_direction(); - this->set_preset_mode_(call.get_preset_mode()); + this->apply_preset_mode_(call); this->write_state_(); this->publish_state(); diff --git a/esphome/components/speed/fan/speed_fan.cpp b/esphome/components/speed/fan/speed_fan.cpp index 801593c2ac..af98e3a51f 100644 --- a/esphome/components/speed/fan/speed_fan.cpp +++ b/esphome/components/speed/fan/speed_fan.cpp @@ -29,7 +29,7 @@ void SpeedFan::control(const fan::FanCall &call) { this->oscillating = *call.get_oscillating(); if (call.get_direction().has_value()) this->direction = *call.get_direction(); - this->set_preset_mode_(call.get_preset_mode()); + this->apply_preset_mode_(call); this->write_state_(); this->publish_state(); diff --git a/esphome/components/template/fan/template_fan.cpp b/esphome/components/template/fan/template_fan.cpp index 384e6b0ca1..0e1920a984 100644 --- a/esphome/components/template/fan/template_fan.cpp +++ b/esphome/components/template/fan/template_fan.cpp @@ -28,7 +28,7 @@ void TemplateFan::control(const fan::FanCall &call) { this->oscillating = *call.get_oscillating(); if (call.get_direction().has_value() && this->has_direction_) this->direction = *call.get_direction(); - this->set_preset_mode_(call.get_preset_mode()); + this->apply_preset_mode_(call); this->publish_state(); } diff --git a/tests/components/copy/common.yaml b/tests/components/copy/common.yaml index a73b3467e6..a376004b2f 100644 --- a/tests/components/copy/common.yaml +++ b/tests/components/copy/common.yaml @@ -7,6 +7,9 @@ fan: - platform: speed id: fan_speed output: fan_output_1 + preset_modes: + - Eco + - Turbo - platform: copy source_id: fan_speed name: Fan Speed Copy diff --git a/tests/components/fan/common.yaml b/tests/components/fan/common.yaml index 55c2a656fd..099bbfef08 100644 --- a/tests/components/fan/common.yaml +++ b/tests/components/fan/common.yaml @@ -9,3 +9,51 @@ fan: has_oscillating: true has_direction: true speed_count: 3 + +# Test lambdas using get_preset_mode() which returns StringRef +# These examples match the migration guide in the PR description +binary_sensor: + - platform: template + id: fan_has_preset + name: "Fan Has Preset" + lambda: |- + // Migration guide: Checking if preset mode is set + // Use empty() or has_preset_mode() + if (!id(test_fan).get_preset_mode().empty()) { + // preset is set + } + if (id(test_fan).has_preset_mode()) { + // preset is set + } + + // Migration guide: Comparing preset mode + // Use == operator directly (safe, works even when empty) + if (id(test_fan).get_preset_mode() == "Eco") { + return true; + } + + // Migration guide: Checking for no preset + if (id(test_fan).get_preset_mode().empty()) { + // no preset + } + if (!id(test_fan).has_preset_mode()) { + // no preset + } + + // Migration guide: Getting as std::string + std::string preset = std::string(id(test_fan).get_preset_mode()); + + // Migration guide: Logging option 1 + // Use .c_str() - works because StringRef points to null-terminated string in traits + ESP_LOGD("test", "Preset: %s", id(test_fan).get_preset_mode().c_str()); + + // Migration guide: Logging option 2 + // Use %.*s format (safer, no null-termination assumption) + auto preset_ref = id(test_fan).get_preset_mode(); + ESP_LOGD("test", "Preset: %.*s", (int)preset_ref.size(), preset_ref.c_str()); + + // Test != comparison + if (id(test_fan).get_preset_mode() != "Sleep") { + return true; + } + return false;