From 2c32ac22212a8854d09672d4cb9606d71081832b Mon Sep 17 00:00:00 2001 From: Boris Krivonog Date: Sun, 23 Aug 2026 21:40:08 +0200 Subject: [PATCH 1/6] [mitsubishi_cn105] Mark configurable classes as final (#18272) --- .../components/mitsubishi_cn105/automation.h | 6 +- .../mitsubishi_cn105_climate.cpp | 83 ++------ .../mitsubishi_cn105_climate.h | 13 +- .../mitsubishi_cn105_component.h | 2 +- .../mitsubishi_cn105_swing_mode_manager.h | 86 +++++++++ .../mitsubishi_cn105_vane_select_vertical.h | 6 +- .../mitsubishi_cn105_climate_tests.cpp | 180 ++++-------------- ...subishi_cn105_swing_mode_manager_tests.cpp | 99 ++++++++++ tests/components/mitsubishi_cn105/common.h | 22 --- .../mitsubishi_cn105_component_tests.cpp | 21 +- ...bishi_cn105_vane_select_vertical_tests.cpp | 33 ++-- 11 files changed, 278 insertions(+), 273 deletions(-) create mode 100644 esphome/components/mitsubishi_cn105/mitsubishi_cn105_swing_mode_manager.h create mode 100644 tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_swing_mode_manager_tests.cpp diff --git a/esphome/components/mitsubishi_cn105/automation.h b/esphome/components/mitsubishi_cn105/automation.h index 2fc6ba3c32..f9ca3a47e6 100644 --- a/esphome/components/mitsubishi_cn105/automation.h +++ b/esphome/components/mitsubishi_cn105/automation.h @@ -9,7 +9,7 @@ namespace esphome::mitsubishi_cn105 { template -class SetRemoteTemperatureAction : public Action, public Parented { +class SetRemoteTemperatureAction final : public Action, public Parented { public: TEMPLATABLE_VALUE(float, temperature) @@ -17,12 +17,12 @@ class SetRemoteTemperatureAction : public Action, public Parented -class ClearRemoteTemperatureAction : public Action, public Parented { +class ClearRemoteTemperatureAction final : public Action, public Parented { public: void play(const Ts &...x) override { this->parent_->clear_remote_temperature(); } }; -template class VaneControlAction : public Action { +template class VaneControlAction final : public Action { public: using ApplyFn = void (*)(VaneCall &, const std::remove_cvref_t &...); diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.cpp b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.cpp index 17ff6d34ca..53b8c21de6 100644 --- a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.cpp +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.cpp @@ -74,7 +74,7 @@ climate::ClimateTraits MitsubishiCN105Climate::traits() { traits.add_supported_fan_mode(p.second); } - traits.set_supported_swing_modes(this->supported_swing_modes_); + traits.set_supported_swing_modes(this->swing_mode_manager_.supported_swing_modes()); const bool use_fahrenheit = this->parent_->get_temperature_mapping().get_use_fahrenheit(); traits.set_temperature_unit(use_fahrenheit ? TemperatureUnit::FAHRENHEIT : TemperatureUnit::CELSIUS); @@ -109,33 +109,11 @@ void MitsubishiCN105Climate::control(const climate::ClimateCall &call) { } if (const auto swing_mode = call.get_swing_mode()) { - auto vane = this->last_non_swing_vane_mode_; - auto wide = this->last_non_swing_wide_vane_mode_; - - switch (*swing_mode) { - case climate::CLIMATE_SWING_BOTH: - vane = MitsubishiCN105::VaneMode::SWING; - wide = MitsubishiCN105::WideVaneMode::SWING; - break; - - case climate::CLIMATE_SWING_VERTICAL: - vane = MitsubishiCN105::VaneMode::SWING; - break; - - case climate::CLIMATE_SWING_HORIZONTAL: - wide = MitsubishiCN105::WideVaneMode::SWING; - break; - - case climate::CLIMATE_SWING_OFF: - default: - break; + if (const auto vane = this->swing_mode_manager_.vane_from(*swing_mode)) { + this->parent_->set_vane_mode(*vane); } - - if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_VERTICAL)) { - this->parent_->set_vane_mode(vane); - } - if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_HORIZONTAL)) { - this->parent_->set_wide_vane_mode(wide); + if (const auto wide = this->swing_mode_manager_.wide_vane_from(*swing_mode)) { + this->parent_->set_wide_vane_mode(*wide); } } @@ -166,64 +144,39 @@ void MitsubishiCN105Climate::apply_values_() { ESP_LOGD(TAG, "Unable to map fan mode"); } - if (!this->supported_swing_modes_.empty()) { - bool vertical_swinging = false; - bool horizontal_swinging = false; - - if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_VERTICAL)) { - if (status.vane_mode == MitsubishiCN105::VaneMode::SWING) { - vertical_swinging = true; - } else if (status.vane_mode != MitsubishiCN105::VaneMode::UNKNOWN) { - this->last_non_swing_vane_mode_ = status.vane_mode; - } - } - - if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_HORIZONTAL)) { - if (status.wide_vane_mode == MitsubishiCN105::WideVaneMode::SWING) { - horizontal_swinging = true; - } else if (status.wide_vane_mode != MitsubishiCN105::WideVaneMode::UNKNOWN) { - this->last_non_swing_wide_vane_mode_ = status.wide_vane_mode; - } - } - - if (vertical_swinging && horizontal_swinging) { - this->swing_mode = climate::CLIMATE_SWING_BOTH; - } else if (vertical_swinging) { - this->swing_mode = climate::CLIMATE_SWING_VERTICAL; - } else if (horizontal_swinging) { - this->swing_mode = climate::CLIMATE_SWING_HORIZONTAL; - } else { - this->swing_mode = climate::CLIMATE_SWING_OFF; - } + if (const auto swing_mode = + this->swing_mode_manager_.update_and_get_swing_mode(status.vane_mode, status.wide_vane_mode)) { + this->swing_mode = *swing_mode; } this->publish_state(); } void MitsubishiCN105Climate::set_supported_swing_mode(climate::ClimateSwingMode mode) { - this->supported_swing_modes_.clear(); + climate::ClimateSwingModeMask supported_swing_modes; switch (mode) { case climate::CLIMATE_SWING_VERTICAL: - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_OFF); - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_VERTICAL); + supported_swing_modes.insert(climate::CLIMATE_SWING_OFF); + supported_swing_modes.insert(climate::CLIMATE_SWING_VERTICAL); break; case climate::CLIMATE_SWING_HORIZONTAL: - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_OFF); - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_HORIZONTAL); + supported_swing_modes.insert(climate::CLIMATE_SWING_OFF); + supported_swing_modes.insert(climate::CLIMATE_SWING_HORIZONTAL); break; case climate::CLIMATE_SWING_BOTH: - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_OFF); - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_VERTICAL); - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_HORIZONTAL); - this->supported_swing_modes_.insert(climate::CLIMATE_SWING_BOTH); + supported_swing_modes.insert(climate::CLIMATE_SWING_OFF); + supported_swing_modes.insert(climate::CLIMATE_SWING_VERTICAL); + supported_swing_modes.insert(climate::CLIMATE_SWING_HORIZONTAL); + supported_swing_modes.insert(climate::CLIMATE_SWING_BOTH); break; case climate::CLIMATE_SWING_OFF: default: break; } + this->swing_mode_manager_.set_supported_swing_modes(supported_swing_modes); } } // namespace esphome::mitsubishi_cn105 diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.h b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.h index 5341c2d2d9..cea76278ab 100644 --- a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.h +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.h @@ -6,10 +6,13 @@ #include "esphome/core/automation.h" #include "esphome/core/component.h" #include "esphome/components/climate/climate.h" +#include "mitsubishi_cn105_swing_mode_manager.h" namespace esphome::mitsubishi_cn105 { -class MitsubishiCN105Climate : public climate::Climate, public Component, public Parented { +class MitsubishiCN105Climate final : public climate::Climate, + public Component, + public Parented { public: void setup() override; void dump_config() override; @@ -25,14 +28,12 @@ class MitsubishiCN105Climate : public climate::Climate, public Component, public protected: void apply_values_(); - climate::ClimateSwingModeMask supported_swing_modes_{}; - MitsubishiCN105::VaneMode last_non_swing_vane_mode_{MitsubishiCN105::VaneMode::AUTO}; - MitsubishiCN105::WideVaneMode last_non_swing_wide_vane_mode_{MitsubishiCN105::WideVaneMode::CENTER}; + SwingModeManager swing_mode_manager_; }; // Legacy climate action compatibility. Remove in 2027.2.0. template -class LegacySetRemoteTemperatureAction : public Action, public Parented { +class LegacySetRemoteTemperatureAction final : public Action, public Parented { public: TEMPLATABLE_VALUE(float, temperature) @@ -41,7 +42,7 @@ class LegacySetRemoteTemperatureAction : public Action, public Parented -class LegacyClearRemoteTemperatureAction : public Action, public Parented { +class LegacyClearRemoteTemperatureAction final : public Action, public Parented { public: void play(const Ts &...x) override { this->parent_->clear_remote_temperature(); } }; diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_component.h b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_component.h index 508a15e6d5..aa9bfe0d8c 100644 --- a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_component.h +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_component.h @@ -80,7 +80,7 @@ struct VaneCall { MitsubishiCN105Component *parent_; }; -class MitsubishiCN105Component : public Component, public uart::UARTDevice { +class MitsubishiCN105Component final : public Component, public uart::UARTDevice { public: explicit MitsubishiCN105Component() : hp_(*this) {} diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105_swing_mode_manager.h b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_swing_mode_manager.h new file mode 100644 index 0000000000..20f54f0bbb --- /dev/null +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105_swing_mode_manager.h @@ -0,0 +1,86 @@ +#pragma once + +#include + +#include "esphome/components/climate/climate.h" +#include "mitsubishi_cn105.h" + +namespace esphome::mitsubishi_cn105 { + +class SwingModeManager final { + public: + const climate::ClimateSwingModeMask &supported_swing_modes() const { return this->supported_swing_modes_; } + void set_supported_swing_modes(const climate::ClimateSwingModeMask &supported_swing_modes) { + this->supported_swing_modes_ = supported_swing_modes; + } + + std::optional vane_from(climate::ClimateSwingMode swing_mode) const { + if (!this->supported_swing_modes_.count(climate::CLIMATE_SWING_VERTICAL)) { + return std::nullopt; + } + + switch (swing_mode) { + case climate::CLIMATE_SWING_BOTH: + case climate::CLIMATE_SWING_VERTICAL: + return MitsubishiCN105::VaneMode::SWING; + default: + return this->last_non_swing_vane_mode_; + } + } + + std::optional wide_vane_from(climate::ClimateSwingMode swing_mode) const { + if (!this->supported_swing_modes_.count(climate::CLIMATE_SWING_HORIZONTAL)) { + return std::nullopt; + } + + switch (swing_mode) { + case climate::CLIMATE_SWING_BOTH: + case climate::CLIMATE_SWING_HORIZONTAL: + return MitsubishiCN105::WideVaneMode::SWING; + default: + return this->last_non_swing_wide_vane_mode_; + } + } + + std::optional update_and_get_swing_mode(MitsubishiCN105::VaneMode vane_mode, + MitsubishiCN105::WideVaneMode wide_vane_mode) { + if (this->supported_swing_modes_.empty()) { + return std::nullopt; + } + + bool vertical_swinging = false; + bool horizontal_swinging = false; + if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_VERTICAL)) { + if (vane_mode == MitsubishiCN105::VaneMode::SWING) { + vertical_swinging = true; + } else if (vane_mode != MitsubishiCN105::VaneMode::UNKNOWN) { + this->last_non_swing_vane_mode_ = vane_mode; + } + } + if (this->supported_swing_modes_.count(climate::CLIMATE_SWING_HORIZONTAL)) { + if (wide_vane_mode == MitsubishiCN105::WideVaneMode::SWING) { + horizontal_swinging = true; + } else if (wide_vane_mode != MitsubishiCN105::WideVaneMode::UNKNOWN) { + this->last_non_swing_wide_vane_mode_ = wide_vane_mode; + } + } + + if (vertical_swinging && horizontal_swinging) { + return climate::CLIMATE_SWING_BOTH; + } + if (vertical_swinging) { + return climate::CLIMATE_SWING_VERTICAL; + } + if (horizontal_swinging) { + return climate::CLIMATE_SWING_HORIZONTAL; + } + return climate::CLIMATE_SWING_OFF; + } + + private: + climate::ClimateSwingModeMask supported_swing_modes_{}; + MitsubishiCN105::VaneMode last_non_swing_vane_mode_{MitsubishiCN105::VaneMode::AUTO}; + MitsubishiCN105::WideVaneMode last_non_swing_wide_vane_mode_{MitsubishiCN105::WideVaneMode::CENTER}; +}; + +} // namespace esphome::mitsubishi_cn105 diff --git a/esphome/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical.h b/esphome/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical.h index 76977d59d7..656b78b487 100644 --- a/esphome/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical.h +++ b/esphome/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical.h @@ -7,9 +7,9 @@ namespace esphome::mitsubishi_cn105 { -class MitsubishiCN105VerticalVaneDirectionSelect : public select::Select, - public Component, - public Parented { +class MitsubishiCN105VerticalVaneDirectionSelect final : public select::Select, + public Component, + public Parented { public: void setup() override; void publish_vane_state(MitsubishiCN105::VaneMode mode); diff --git a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp index b91252c9fa..669345f576 100644 --- a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp +++ b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp @@ -2,10 +2,19 @@ #include #include "../common.h" +#include "esphome/components/mitsubishi_cn105/mitsubishi_cn105_climate.h" + namespace esphome::mitsubishi_cn105::testing { +struct MitsubishiCN105ClimateTestContext { + MitsubishiCN105Component component; + MitsubishiCN105Climate sut; + + MitsubishiCN105ClimateTestContext() { this->sut.set_parent(&this->component); } +}; + TEST(MitsubishiCN105ClimateTests, CelsiusTemperatureMappingAndTraitsMatchExpectedValues) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; const auto mapping = TemperatureMapping(); for (int temperature = 16; temperature <= 31; ++temperature) { @@ -13,7 +22,7 @@ TEST(MitsubishiCN105ClimateTests, CelsiusTemperatureMappingAndTraitsMatchExpecte EXPECT_EQ(mapping.from_mitsubishi(temperature), temperature); } - const auto traits = sut.traits(); + const auto traits = context.sut.traits(); EXPECT_EQ(traits.get_temperature_unit(), TemperatureUnit::CELSIUS); EXPECT_FLOAT_EQ(traits.get_visual_min_temperature(), 16.0f); EXPECT_FLOAT_EQ(traits.get_visual_max_temperature(), 31.0f); @@ -22,10 +31,10 @@ TEST(MitsubishiCN105ClimateTests, CelsiusTemperatureMappingAndTraitsMatchExpecte } TEST(MitsubishiCN105ClimateTests, FahrenheitTemperatureMappingAndTraitsMatchExpectedValues) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; auto mapping = TemperatureMapping(); mapping.set_use_fahrenheit(true); - sut.set_use_fahrenheit(true); + context.component.set_use_fahrenheit(true); const std::array cases{ std::pair{61, 16.0f}, std::pair{62, 16.5f}, std::pair{63, 17.0f}, std::pair{64, 17.5f}, std::pair{65, 18.0f}, @@ -40,7 +49,7 @@ TEST(MitsubishiCN105ClimateTests, FahrenheitTemperatureMappingAndTraitsMatchExpe EXPECT_FLOAT_EQ(mapping.to_mitsubishi(fahrenheit), mitsubishi_celsius); EXPECT_FLOAT_EQ(mapping.from_mitsubishi(mitsubishi_celsius), fahrenheit); } - const auto traits = sut.traits(); + const auto traits = context.sut.traits(); EXPECT_EQ(traits.get_temperature_unit(), TemperatureUnit::FAHRENHEIT); EXPECT_FLOAT_EQ(traits.get_visual_min_temperature(), 61.0f); EXPECT_FLOAT_EQ(traits.get_visual_max_temperature(), 88.0f); @@ -63,163 +72,44 @@ TEST(MitsubishiCN105ClimateTests, FahrenheitTemperatureMappingUsesLinearConversi } TEST(MitsubishiCN105ClimateTests, SupportedSwingModeOffLeavesTraitsEmpty) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; - sut.set_supported_swing_mode(climate::CLIMATE_SWING_OFF); + context.sut.set_supported_swing_mode(climate::CLIMATE_SWING_OFF); - EXPECT_FALSE(sut.traits().get_supports_swing_modes()); + EXPECT_FALSE(context.sut.traits().get_supports_swing_modes()); } TEST(MitsubishiCN105ClimateTests, SupportedSwingModeVerticalExposesOffAndVertical) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; - sut.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); + context.sut.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); - EXPECT_FALSE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); - EXPECT_FALSE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); + EXPECT_FALSE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); + EXPECT_FALSE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); } TEST(MitsubishiCN105ClimateTests, SupportedSwingModeHorizontalExposesOffAndHorizontal) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; - sut.set_supported_swing_mode(climate::CLIMATE_SWING_HORIZONTAL); + context.sut.set_supported_swing_mode(climate::CLIMATE_SWING_HORIZONTAL); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); - EXPECT_FALSE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); - EXPECT_FALSE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); + EXPECT_FALSE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); + EXPECT_FALSE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); } TEST(MitsubishiCN105ClimateTests, SupportedSwingModeBothExposesAllExpectedModes) { - TestableMitsubishiCN105Climate sut; + MitsubishiCN105ClimateTestContext context; - sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); + context.sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); - EXPECT_TRUE(sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesMapsVerticalSwingWhenSupported) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::SWING; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::CENTER; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_VERTICAL); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesMapsHorizontalSwingWhenSupported) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_HORIZONTAL); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::AUTO; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::SWING; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_HORIZONTAL); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesMapsBothSwingWhenSupported) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::SWING; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::SWING; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_BOTH); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesMapsSwingOffWhenNoSwingActive) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::POSITION_3; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::CENTER; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_OFF); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesRemembersLastNonSwingPositions) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::POSITION_4; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::RIGHT; - - sut.apply_values_(); - - EXPECT_EQ(sut.last_non_swing_vane_mode_, MitsubishiCN105::VaneMode::POSITION_4); - EXPECT_EQ(sut.last_non_swing_wide_vane_mode_, MitsubishiCN105::WideVaneMode::RIGHT); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::SWING; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::SWING; - - sut.apply_values_(); - - EXPECT_EQ(sut.last_non_swing_vane_mode_, MitsubishiCN105::VaneMode::POSITION_4); - EXPECT_EQ(sut.last_non_swing_wide_vane_mode_, MitsubishiCN105::WideVaneMode::RIGHT); - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_BOTH); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesDoesNotOverwriteRememberedPositionWithUnknownValues) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_BOTH); - - sut.last_non_swing_vane_mode_ = MitsubishiCN105::VaneMode::POSITION_2; - sut.last_non_swing_wide_vane_mode_ = MitsubishiCN105::WideVaneMode::LEFT; - - sut.status().vane_mode = MitsubishiCN105::VaneMode::UNKNOWN; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::UNKNOWN; - - sut.apply_values_(); - - EXPECT_EQ(sut.last_non_swing_vane_mode_, MitsubishiCN105::VaneMode::POSITION_2); - EXPECT_EQ(sut.last_non_swing_wide_vane_mode_, MitsubishiCN105::WideVaneMode::LEFT); - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_OFF); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesIgnoresUnsupportedVerticalSwingState) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_HORIZONTAL); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::SWING; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::CENTER; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_OFF); -} - -TEST(MitsubishiCN105ClimateTests, ApplyValuesIgnoresUnsupportedHorizontalSwingState) { - TestableMitsubishiCN105Climate sut; - - sut.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); - - sut.status().vane_mode = MitsubishiCN105::VaneMode::AUTO; - sut.status().wide_vane_mode = MitsubishiCN105::WideVaneMode::SWING; - - sut.apply_values_(); - - EXPECT_EQ(sut.swing_mode, climate::CLIMATE_SWING_OFF); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_OFF)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_VERTICAL)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_HORIZONTAL)); + EXPECT_TRUE(context.sut.traits().supports_swing_mode(climate::CLIMATE_SWING_BOTH)); } } // namespace esphome::mitsubishi_cn105::testing diff --git a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_swing_mode_manager_tests.cpp b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_swing_mode_manager_tests.cpp new file mode 100644 index 0000000000..ab3048b874 --- /dev/null +++ b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_swing_mode_manager_tests.cpp @@ -0,0 +1,99 @@ +#include "../common.h" + +#include "esphome/components/mitsubishi_cn105/mitsubishi_cn105_swing_mode_manager.h" + +namespace esphome::mitsubishi_cn105::testing { + +static SwingModeManager make_swing_mode_manager(std::initializer_list supported_modes) { + SwingModeManager manager; + climate::ClimateSwingModeMask supported_swing_modes; + for (const auto mode : supported_modes) + supported_swing_modes.insert(mode); + manager.set_supported_swing_modes(supported_swing_modes); + return manager; +} + +TEST(SwingModeManagerTests, StatusMapsVerticalSwingWhenSupported) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL}); + EXPECT_EQ(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::SWING, MitsubishiCN105::WideVaneMode::CENTER), + std::optional{climate::CLIMATE_SWING_VERTICAL}); +} + +TEST(SwingModeManagerTests, StatusMapsHorizontalSwingWhenSupported) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_HORIZONTAL}); + EXPECT_EQ(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::AUTO, MitsubishiCN105::WideVaneMode::SWING), + std::optional{climate::CLIMATE_SWING_HORIZONTAL}); +} + +TEST(SwingModeManagerTests, StatusMapsBothSwingWhenSupported) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL, + climate::CLIMATE_SWING_HORIZONTAL, climate::CLIMATE_SWING_BOTH}); + EXPECT_EQ(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::SWING, MitsubishiCN105::WideVaneMode::SWING), + std::optional{climate::CLIMATE_SWING_BOTH}); +} + +TEST(SwingModeManagerTests, StatusMapsSwingOffWhenNoSwingActive) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL, + climate::CLIMATE_SWING_HORIZONTAL, climate::CLIMATE_SWING_BOTH}); + EXPECT_EQ( + manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::POSITION_3, MitsubishiCN105::WideVaneMode::CENTER), + std::optional{climate::CLIMATE_SWING_OFF}); +} + +TEST(SwingModeManagerTests, RemembersLastNonSwingPositions) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL, + climate::CLIMATE_SWING_HORIZONTAL, climate::CLIMATE_SWING_BOTH}); + manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::POSITION_4, MitsubishiCN105::WideVaneMode::RIGHT); + manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::SWING, MitsubishiCN105::WideVaneMode::SWING); + EXPECT_EQ(manager.vane_from(climate::CLIMATE_SWING_OFF), std::optional{MitsubishiCN105::VaneMode::POSITION_4}); + EXPECT_EQ(manager.wide_vane_from(climate::CLIMATE_SWING_OFF), std::optional{MitsubishiCN105::WideVaneMode::RIGHT}); +} + +TEST(SwingModeManagerTests, UnknownValuesDoNotOverwriteRememberedPositions) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL, + climate::CLIMATE_SWING_HORIZONTAL, climate::CLIMATE_SWING_BOTH}); + manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::POSITION_2, MitsubishiCN105::WideVaneMode::LEFT); + manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::UNKNOWN, MitsubishiCN105::WideVaneMode::UNKNOWN); + EXPECT_EQ(manager.vane_from(climate::CLIMATE_SWING_OFF), std::optional{MitsubishiCN105::VaneMode::POSITION_2}); + EXPECT_EQ(manager.wide_vane_from(climate::CLIMATE_SWING_OFF), std::optional{MitsubishiCN105::WideVaneMode::LEFT}); +} + +TEST(SwingModeManagerTests, UnsupportedVerticalSwingStateIsIgnored) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_HORIZONTAL}); + EXPECT_EQ(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::SWING, MitsubishiCN105::WideVaneMode::CENTER), + std::optional{climate::CLIMATE_SWING_OFF}); +} + +TEST(SwingModeManagerTests, UnsupportedHorizontalSwingStateIsIgnored) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL}); + EXPECT_EQ(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::AUTO, MitsubishiCN105::WideVaneMode::SWING), + std::optional{climate::CLIMATE_SWING_OFF}); +} + +TEST(SwingModeManagerTests, SwingModeFromReturnsNulloptWhenNoSwingModesSupported) { + auto manager = make_swing_mode_manager({}); + EXPECT_FALSE(manager.update_and_get_swing_mode(MitsubishiCN105::VaneMode::SWING, MitsubishiCN105::WideVaneMode::SWING) + .has_value()); +} + +TEST(SwingModeManagerTests, VaneFromSwingModeReturnsNulloptWhenVerticalUnsupported) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_HORIZONTAL}); + EXPECT_FALSE(manager.vane_from(climate::CLIMATE_SWING_VERTICAL).has_value()); +} + +TEST(SwingModeManagerTests, WideVaneFromSwingModeReturnsNulloptWhenHorizontalUnsupported) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL}); + EXPECT_FALSE(manager.wide_vane_from(climate::CLIMATE_SWING_HORIZONTAL).has_value()); +} + +TEST(SwingModeManagerTests, VaneAndWideVaneFromSwingModeMapSwingModes) { + auto manager = make_swing_mode_manager({climate::CLIMATE_SWING_OFF, climate::CLIMATE_SWING_VERTICAL, + climate::CLIMATE_SWING_HORIZONTAL, climate::CLIMATE_SWING_BOTH}); + EXPECT_EQ(manager.vane_from(climate::CLIMATE_SWING_VERTICAL), std::optional{MitsubishiCN105::VaneMode::SWING}); + EXPECT_EQ(manager.vane_from(climate::CLIMATE_SWING_BOTH), std::optional{MitsubishiCN105::VaneMode::SWING}); + EXPECT_EQ(manager.wide_vane_from(climate::CLIMATE_SWING_HORIZONTAL), + std::optional{MitsubishiCN105::WideVaneMode::SWING}); + EXPECT_EQ(manager.wide_vane_from(climate::CLIMATE_SWING_BOTH), std::optional{MitsubishiCN105::WideVaneMode::SWING}); +} + +} // namespace esphome::mitsubishi_cn105::testing diff --git a/tests/components/mitsubishi_cn105/common.h b/tests/components/mitsubishi_cn105/common.h index ee287d2548..6029327e9a 100644 --- a/tests/components/mitsubishi_cn105/common.h +++ b/tests/components/mitsubishi_cn105/common.h @@ -64,26 +64,4 @@ class TestableMitsubishiCN105 : public MitsubishiCN105 { void set_current_time(uint32_t ms) { test_loop_time_ms = ms; } }; -class TestableMitsubishiCN105Climate : public MitsubishiCN105Climate { - public: - TestableMitsubishiCN105Climate() { this->set_parent(&this->component_); } - - using MitsubishiCN105Climate::apply_values_; - using MitsubishiCN105Climate::last_non_swing_vane_mode_; - using MitsubishiCN105Climate::last_non_swing_wide_vane_mode_; - - MitsubishiCN105::Status &status() { return const_cast(this->component_.status()); } - void set_use_fahrenheit(bool value) { this->component_.set_use_fahrenheit(value); } - - protected: - MitsubishiCN105Component component_; -}; - -class TestableMitsubishiCN105Component : public MitsubishiCN105Component { - public: - MitsubishiCN105::Status &mutable_status() { return const_cast(this->status()); } - - void notify_status() { this->status_callback_.call(); } -}; - } // namespace esphome::mitsubishi_cn105::testing diff --git a/tests/components/mitsubishi_cn105/mitsubishi_cn105_component_tests.cpp b/tests/components/mitsubishi_cn105/mitsubishi_cn105_component_tests.cpp index c957759223..928d4123f6 100644 --- a/tests/components/mitsubishi_cn105/mitsubishi_cn105_component_tests.cpp +++ b/tests/components/mitsubishi_cn105/mitsubishi_cn105_component_tests.cpp @@ -3,7 +3,7 @@ namespace esphome::mitsubishi_cn105::testing { TEST(MitsubishiCN105ComponentTests, PublishesVaneStateForEveryValidSnapshot) { - TestableMitsubishiCN105Component hub; + MitsubishiCN105Component hub; size_t callback_count = 0; std::optional callback_direction; hub.add_on_vane_state_callback([&](const VaneState &state) { @@ -11,8 +11,9 @@ TEST(MitsubishiCN105ComponentTests, PublishesVaneStateForEveryValidSnapshot) { callback_direction = state.vertical.direction; }); - hub.mutable_status().room_temperature = 20.0f; - hub.mutable_status().vane_mode = MitsubishiCN105::VaneMode::POSITION_4; + hub.set_telemetry_request_min_interval(SCHEDULER_DONT_RUN); + hub.set_target_temperature(20.0f); + hub.set_vane_mode(MitsubishiCN105::VaneMode::POSITION_4); hub.publish_status(); EXPECT_EQ(callback_count, 1); @@ -25,7 +26,7 @@ TEST(MitsubishiCN105ComponentTests, PublishesVaneStateForEveryValidSnapshot) { } TEST(MitsubishiCN105ComponentTests, PublishesUnknownVaneState) { - TestableMitsubishiCN105Component hub; + MitsubishiCN105Component hub; size_t status_callback_count = 0; size_t vane_callback_count = 0; std::optional callback_direction; @@ -35,15 +36,16 @@ TEST(MitsubishiCN105ComponentTests, PublishesUnknownVaneState) { callback_direction = state.vertical.direction; }); - hub.mutable_status().room_temperature = 20.0f; - hub.mutable_status().vane_mode = MitsubishiCN105::VaneMode::UNKNOWN; + hub.set_telemetry_request_min_interval(SCHEDULER_DONT_RUN); + hub.set_target_temperature(20.0f); + ASSERT_EQ(hub.status().vane_mode, MitsubishiCN105::VaneMode::UNKNOWN); hub.publish_status(); EXPECT_EQ(status_callback_count, 1); EXPECT_EQ(vane_callback_count, 1); EXPECT_EQ(callback_direction, std::optional{VERTICAL_VANE_MODE_UNKNOWN}); - hub.mutable_status().vane_mode = MitsubishiCN105::VaneMode::POSITION_4; + hub.set_vane_mode(MitsubishiCN105::VaneMode::POSITION_4); hub.publish_status(); EXPECT_EQ(status_callback_count, 2); @@ -52,7 +54,7 @@ TEST(MitsubishiCN105ComponentTests, PublishesUnknownVaneState) { } TEST(MitsubishiCN105ComponentTests, VaneCallAppliesVerticalDirection) { - TestableMitsubishiCN105Component hub; + MitsubishiCN105Component hub; auto call = hub.make_vane_call(); call.vertical.set_direction(VERTICAL_VANE_MODE_POSITION_5); @@ -62,12 +64,11 @@ TEST(MitsubishiCN105ComponentTests, VaneCallAppliesVerticalDirection) { } TEST(MitsubishiCN105ComponentTests, VaneControlActionAppliesConfiguredFields) { - TestableMitsubishiCN105Component hub; + MitsubishiCN105Component hub; VaneControlAction<> action(&hub, [](VaneCall &call) { call.vertical.set_direction(VERTICAL_VANE_MODE_SWING); }); action.play(); EXPECT_EQ(hub.status().vane_mode, MitsubishiCN105::VaneMode::SWING); } - } // namespace esphome::mitsubishi_cn105::testing diff --git a/tests/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical_tests.cpp b/tests/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical_tests.cpp index 1f928e3bf4..55ae1d6c92 100644 --- a/tests/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical_tests.cpp +++ b/tests/components/mitsubishi_cn105/select/mitsubishi_cn105_vane_select_vertical_tests.cpp @@ -3,14 +3,9 @@ namespace esphome::mitsubishi_cn105::testing { -class TestableMitsubishiCN105VerticalVaneDirectionSelect : public MitsubishiCN105VerticalVaneDirectionSelect { - public: - using MitsubishiCN105VerticalVaneDirectionSelect::control; -}; - struct VerticalVaneDirectionSelectTestContext { - TestableMitsubishiCN105Component hub; - TestableMitsubishiCN105VerticalVaneDirectionSelect select; + MitsubishiCN105Component hub; + MitsubishiCN105VerticalVaneDirectionSelect select; VerticalVaneDirectionSelectTestContext() { this->select.traits.set_options({"Auto", "1", "2", "3", "4", "5", "Swing"}); @@ -31,13 +26,15 @@ TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, MapsIndexesToVaneModes) { for (size_t i = 0; i < expected_modes.size(); ++i) { SCOPED_TRACE(i); - ctx.select.control(i); + ctx.select.make_call().set_index(i).perform(); EXPECT_EQ(ctx.hub.status().vane_mode, expected_modes[i]); } } TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, PublishesIncomingVaneModes) { VerticalVaneDirectionSelectTestContext ctx; + ctx.hub.set_telemetry_request_min_interval(SCHEDULER_DONT_RUN); + ctx.hub.set_target_temperature(20.0f); constexpr std::array modes{ MitsubishiCN105::VaneMode::AUTO, MitsubishiCN105::VaneMode::POSITION_1, @@ -48,13 +45,12 @@ TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, PublishesIncomingVaneModes for (size_t i = 0; i < modes.size(); ++i) { SCOPED_TRACE(i); - ctx.hub.mutable_status().vane_mode = modes[i]; - ctx.hub.notify_status(); + ctx.hub.set_vane_mode(modes[i]); + ctx.hub.publish_status(); EXPECT_EQ(ctx.select.active_index(), std::optional{i}); } - ctx.hub.mutable_status().vane_mode = MitsubishiCN105::VaneMode::UNKNOWN; - ctx.hub.notify_status(); + ctx.select.publish_vane_state(MitsubishiCN105::VaneMode::UNKNOWN); EXPECT_EQ(ctx.select.active_index(), std::optional{modes.size() - 1}); } @@ -64,14 +60,15 @@ TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, ControlPublishesSelectAndC climate_entity.set_parent(&ctx.hub); climate_entity.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); - ctx.hub.mutable_status().room_temperature = 20.0f; + ctx.hub.set_telemetry_request_min_interval(SCHEDULER_DONT_RUN); + ctx.hub.set_target_temperature(20.0f); climate_entity.setup(); - ctx.select.control(6); + ctx.select.make_call().set_index(6).perform(); EXPECT_EQ(ctx.select.active_index(), std::optional{6}); EXPECT_EQ(climate_entity.swing_mode, climate::CLIMATE_SWING_VERTICAL); - ctx.select.control(3); + ctx.select.make_call().set_index(3).perform(); EXPECT_EQ(ctx.select.active_index(), std::optional{3}); EXPECT_EQ(climate_entity.swing_mode, climate::CLIMATE_SWING_OFF); } @@ -82,7 +79,8 @@ TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, ClimateControlPublishesSel climate_entity.set_parent(&ctx.hub); climate_entity.set_supported_swing_mode(climate::CLIMATE_SWING_VERTICAL); - ctx.hub.mutable_status().room_temperature = 20.0f; + ctx.hub.set_telemetry_request_min_interval(SCHEDULER_DONT_RUN); + ctx.hub.set_target_temperature(20.0f); climate_entity.setup(); climate_entity.make_call().set_swing_mode(climate::CLIMATE_SWING_VERTICAL).perform(); @@ -95,10 +93,9 @@ TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, ClimateControlPublishesSel TEST(MitsubishiCN105VerticalVaneDirectionSelectTests, BeforeInitializationDoesNotPublishSelectState) { VerticalVaneDirectionSelectTestContext ctx; - ctx.select.control(3); + ctx.select.make_call().set_index(3).perform(); EXPECT_EQ(ctx.hub.status().vane_mode, MitsubishiCN105::VaneMode::POSITION_3); EXPECT_FALSE(ctx.select.has_state()); } - } // namespace esphome::mitsubishi_cn105::testing From d84293931ba1bfe3583fd574d19a4bf3943aa373 Mon Sep 17 00:00:00 2001 From: guillempages Date: Sun, 23 Aug 2026 21:58:01 +0200 Subject: [PATCH 2/6] [online_image] Support image format auto-detection (#16337) Co-authored-by: J. Nick Koston Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Co-authored-by: J. Nick Koston --- .../http_request/http_request_idf.cpp | 3 + esphome/components/online_image/image.py | 13 +- .../components/online_image/online_image.cpp | 52 +++++- esphome/components/runtime_image/__init__.py | 27 ++- .../components/runtime_image/image_format.h | 3 +- .../runtime_image/runtime_image.cpp | 18 +- .../components/runtime_image/runtime_image.h | 4 +- tests/components/online_image/common.yaml | 5 + .../runtime_image/test_decoder_reuse.cpp | 68 +++++--- ...ine_image_auto_detects_image_bmp_mime.yaml | 28 ++++ ...uto_detects_redirected_image_bmp_mime.yaml | 28 ++++ .../fixtures/online_image_bmp.yaml | 5 +- tests/integration/online_image_utils.py | 158 ++++++++++++++++++ ...nline_image_auto_detects_image_bmp_mime.py | 71 ++++++++ ..._auto_detects_redirected_image_bmp_mime.py | 71 ++++++++ tests/integration/test_online_image_bmp.py | 63 +------ 16 files changed, 513 insertions(+), 104 deletions(-) create mode 100644 tests/integration/fixtures/online_image_auto_detects_image_bmp_mime.yaml create mode 100644 tests/integration/fixtures/online_image_auto_detects_redirected_image_bmp_mime.yaml create mode 100644 tests/integration/online_image_utils.py create mode 100644 tests/integration/test_online_image_auto_detects_image_bmp_mime.py create mode 100644 tests/integration/test_online_image_auto_detects_redirected_image_bmp_mime.py diff --git a/esphome/components/http_request/http_request_idf.cpp b/esphome/components/http_request/http_request_idf.cpp index ddff954950..470ed332f1 100644 --- a/esphome/components/http_request/http_request_idf.cpp +++ b/esphome/components/http_request/http_request_idf.cpp @@ -196,6 +196,9 @@ std::shared_ptr HttpRequestIDF::perform(const std::string &url, c } container->feed_wdt(); + // IDF is the only backend reusing the container across redirect hops; + // drop the previous hop's headers (Arduino/host collect only the final response) + container->response_headers_.clear(); container->content_length = esp_http_client_fetch_headers(client); container->set_chunked(esp_http_client_is_chunked_response(client)); container->feed_wdt(); diff --git a/esphome/components/online_image/image.py b/esphome/components/online_image/image.py index ae785d17f9..3e9517937e 100644 --- a/esphome/components/online_image/image.py +++ b/esphome/components/online_image/image.py @@ -4,8 +4,16 @@ from esphome.components import runtime_image from esphome.components.const import CONF_REQUEST_HEADERS from esphome.components.http_request import CONF_HTTP_REQUEST_ID, HttpRequestComponent from esphome.components.image import CONF_TRANSPARENCY, add_metadata +from esphome.components.runtime_image import IMAGE_FORMATS import esphome.config_validation as cv -from esphome.const import CONF_BUFFER_SIZE, CONF_ID, CONF_ON_ERROR, CONF_TYPE, CONF_URL +from esphome.const import ( + CONF_BUFFER_SIZE, + CONF_FORMAT, + CONF_ID, + CONF_ON_ERROR, + CONF_TYPE, + CONF_URL, +) from esphome.core import ID, Lambda from esphome.cpp_generator import MockObj, TemplateArgsType from esphome.types import ConfigType @@ -31,7 +39,6 @@ ReleaseImageAction = online_image_ns.class_( "OnlineImageReleaseAction", automation.Action, cg.Parented.template(OnlineImage) ) - ONLINE_IMAGE_SCHEMA = ( runtime_image.runtime_image_schema(OnlineImage) .extend( @@ -39,6 +46,8 @@ ONLINE_IMAGE_SCHEMA = ( # Online Image specific options cv.GenerateID(CONF_HTTP_REQUEST_ID): cv.use_id(HttpRequestComponent), cv.Required(CONF_URL): cv.url, + # AUTO (Content-Type detection) is online_image specific; not in the shared registry + cv.Required(CONF_FORMAT): cv.one_of(*IMAGE_FORMATS, "AUTO", upper=True), cv.Optional(CONF_BUFFER_SIZE, default=65536): cv.int_range(256, 65536), cv.Optional(CONF_REQUEST_HEADERS): cv.All( cv.Schema({cv.string: cv.templatable(cv.string)}) diff --git a/esphome/components/online_image/online_image.cpp b/esphome/components/online_image/online_image.cpp index fe4f727cd6..a2662ff0e3 100644 --- a/esphome/components/online_image/online_image.cpp +++ b/esphome/components/online_image/online_image.cpp @@ -1,9 +1,11 @@ #include "online_image.h" #include "esphome/components/runtime_image/image_decoder.h" +#include "esphome/core/helpers.h" #include "esphome/core/log.h" #include static const char *const TAG = "online_image"; +static const char *const CONTENT_TYPE_HEADER_NAME = "content-type"; static const char *const ETAG_HEADER_NAME = "etag"; static const char *const IF_NONE_MATCH_HEADER_NAME = "if-none-match"; static const char *const LAST_MODIFIED_HEADER_NAME = "last-modified"; @@ -62,7 +64,8 @@ void OnlineImage::update() { // Add Accept header based on image format const char *accept_mime_type; - switch (this->get_format()) { + runtime_image::ImageFormat format = this->get_format(); + switch (format) { #ifdef USE_RUNTIME_IMAGE_BMP case runtime_image::BMP: accept_mime_type = "image/bmp,*/*;q=0.8"; @@ -89,8 +92,8 @@ void OnlineImage::update() { headers.push_back(http_request::Header{header.first, header.second.value()}); } - this->downloader_ = this->parent_->get(this->url_, headers, {ETAG_HEADER_NAME, LAST_MODIFIED_HEADER_NAME}); - + this->downloader_ = + this->parent_->get(this->url_, headers, {ETAG_HEADER_NAME, LAST_MODIFIED_HEADER_NAME, CONTENT_TYPE_HEADER_NAME}); if (this->downloader_ == nullptr) { ESP_LOGE(TAG, "Download failed."); this->end_connection_(); @@ -115,17 +118,54 @@ void OnlineImage::update() { ESP_LOGD(TAG, "Starting download"); size_t total_size = this->downloader_->content_length; + ESP_LOGV(TAG, "Content-Length: %zu", total_size); + + if (format == runtime_image::AUTO) { + // Try to auto-detect format from Content-Type header + auto content_type_header = this->downloader_->get_response_header(CONTENT_TYPE_HEADER_NAME); + const char *content_type = content_type_header.c_str(); + ESP_LOGV(TAG, "Content-Type: %s", content_type); + // Includes aliases seen from real servers (older IIS, CDNs, S3) + if (str_contains_ignore_case(content_type, "image/bmp") || + str_contains_ignore_case(content_type, "image/x-ms-bmp") || + str_contains_ignore_case(content_type, "image/x-bmp")) { + format = runtime_image::BMP; + } else if (str_contains_ignore_case(content_type, "image/jpeg") || + str_contains_ignore_case(content_type, "image/jpg")) { + format = runtime_image::JPEG; + } else if (str_contains_ignore_case(content_type, "image/png") || + str_contains_ignore_case(content_type, "image/x-png")) { + format = runtime_image::PNG; + } else if (str_contains_ignore_case(content_type, "image/")) { + ESP_LOGW(TAG, "Unsupported image type: '%s'", content_type); + this->end_connection_(); + this->download_error_callback_.call(); + return; + } else { + // TODO: implement auto-detection in runtime_image by sniffing the first few bytes of the image data + if (content_type_header.empty()) { + ESP_LOGW(TAG, "Server sent no Content-Type header; cannot determine image format. Set `format:` explicitly"); + } else { + ESP_LOGE(TAG, "Could not determine image format from Content-Type: '%s'. Set `format:` explicitly", + content_type); + } + this->end_connection_(); + this->download_error_callback_.call(); + return; + } + } + ESP_LOGD(TAG, "Using image format: %d", format); // Initialize decoder with the known format - if (!this->begin_decode(total_size)) { - ESP_LOGE(TAG, "Failed to initialize decoder for format %d", this->get_format()); + if (!this->begin_decode(total_size, format)) { + ESP_LOGE(TAG, "Failed to initialize decoder for format %d", format); this->end_connection_(); this->download_error_callback_.call(); return; } // JPEG requires the complete image in the download buffer before decoding - if (this->get_format() == runtime_image::JPEG && total_size > this->download_buffer_.size()) { + if (format == runtime_image::JPEG && total_size > this->download_buffer_.size()) { this->download_buffer_.resize(total_size); } diff --git a/esphome/components/runtime_image/__init__.py b/esphome/components/runtime_image/__init__.py index 3c130a7d75..0d4345db5b 100644 --- a/esphome/components/runtime_image/__init__.py +++ b/esphome/components/runtime_image/__init__.py @@ -58,6 +58,18 @@ class Format: """Add defines and libraries needed for this format.""" +class AUTOFormat(Format): + """AUTO format - detect from MIME type.""" + + def __init__(self): + super().__init__("AUTO", None) + + def actions(self) -> None: + # dict.fromkeys dedupes the JPG/JPEG alias so each format runs once + for image_format in dict.fromkeys(IMAGE_FORMATS.values()): + image_format.actions() + + class BMPFormat(Format): """BMP format decoder configuration.""" @@ -102,18 +114,25 @@ class PNGFormat(Format): cg.add_library("pngle", "1.1.0") -# Registry of available formats +# Decodable formats only; platforms that support runtime detection accept +# "AUTO" in their own schema and get_format() resolves it +_JPEG_FORMAT = JPEGFormat() IMAGE_FORMATS = { "BMP": BMPFormat(), - "JPEG": JPEGFormat(), + "JPEG": _JPEG_FORMAT, + "JPG": _JPEG_FORMAT, # Alias for JPEG "PNG": PNGFormat(), - "JPG": JPEGFormat(), # Alias for JPEG } +AUTO_FORMAT = AUTOFormat() + def get_format(format_name: str) -> Format | None: """Get a format instance by name.""" - return IMAGE_FORMATS.get(format_name.upper()) + name = format_name.upper() + if name == "AUTO": + return AUTO_FORMAT + return IMAGE_FORMATS.get(name) def enable_format(format_name: str) -> Format | None: diff --git a/esphome/components/runtime_image/image_format.h b/esphome/components/runtime_image/image_format.h index 524e52d7bc..ca6e0782b9 100644 --- a/esphome/components/runtime_image/image_format.h +++ b/esphome/components/runtime_image/image_format.h @@ -6,7 +6,8 @@ namespace esphome::runtime_image { * @brief Image format types that can be decoded dynamically. */ enum ImageFormat { - /** Automatically detect from data. Not implemented yet. */ + /** Format is supplied per decode, e.g. detected from the Content-Type header + * by online_image; sniffing the image data is not implemented. */ AUTO, /** JPEG format. */ JPEG, diff --git a/esphome/components/runtime_image/runtime_image.cpp b/esphome/components/runtime_image/runtime_image.cpp index e269f7d8f3..254624caf4 100644 --- a/esphome/components/runtime_image/runtime_image.cpp +++ b/esphome/components/runtime_image/runtime_image.cpp @@ -171,22 +171,27 @@ void RuntimeImage::draw(int x, int y, display::Display *display, Color color_on, // If no image is loaded and no placeholder, nothing to draw } -bool RuntimeImage::begin_decode(size_t expected_size) { +bool RuntimeImage::begin_decode(size_t expected_size, ImageFormat format) { if (this->is_decoding()) { ESP_LOGW(TAG, "Decoding already in progress"); return false; } + if (format == AUTO && this->format_ != AUTO) { + // Fall back to the configured format before the reuse check below + format = this->format_; + } + // An idle decoder for a different format cannot be reused - if (this->decoder_ != nullptr && this->decoder_->get_format() != this->format_) { - ESP_LOGD(TAG, "Decoder format mismatch: current: %d, new: %d", this->decoder_->get_format(), this->format_); + if (this->decoder_ != nullptr && this->decoder_->get_format() != format) { + ESP_LOGD(TAG, "Decoder format mismatch: current: %d, new: %d", this->decoder_->get_format(), format); this->decoder_ = nullptr; } if (!this->decoder_) { - this->decoder_ = this->create_decoder_(this->format_); + this->decoder_ = this->create_decoder_(format); if (!this->decoder_) { - ESP_LOGE(TAG, "Failed to create decoder for format %d", this->format_); + ESP_LOGE(TAG, "Failed to create decoder for format %d", format); return false; } } @@ -364,6 +369,9 @@ std::unique_ptr RuntimeImage::create_decoder_(ImageFormat format) case PNG: return make_unique(this); #endif + case AUTO: + ESP_LOGE(TAG, "Image format could not be determined; set `format:` explicitly in the configuration"); + return nullptr; default: ESP_LOGE(TAG, "Unsupported image format: %d", format); return nullptr; diff --git a/esphome/components/runtime_image/runtime_image.h b/esphome/components/runtime_image/runtime_image.h index cfac253fdb..55d5c0ae86 100644 --- a/esphome/components/runtime_image/runtime_image.h +++ b/esphome/components/runtime_image/runtime_image.h @@ -62,9 +62,10 @@ class RuntimeImage : public image::Image { * @brief Begin decoding an image. * * @param expected_size Optional hint about the expected data size. + * @param format The image format to decode (defaults to AUTO, which uses the value set at construction). * @return true if decoder was successfully initialized. */ - bool begin_decode(size_t expected_size = 0); + bool begin_decode(size_t expected_size = 0, ImageFormat format = AUTO); /** * @brief Feed data to the decoder. @@ -103,6 +104,7 @@ class RuntimeImage : public image::Image { /** * @brief Get the image format. */ + /// Configured format; a format resolved per decode lives on the active decoder ImageFormat get_format() const { return this->format_; } /** diff --git a/tests/components/online_image/common.yaml b/tests/components/online_image/common.yaml index f71cf63de9..85901287c7 100644 --- a/tests/components/online_image/common.yaml +++ b/tests/components/online_image/common.yaml @@ -57,6 +57,11 @@ image: url: http://www.faqs.org/images/library.jpg format: JPG type: RGB565 + - platform: online_image + id: online_auto_image + url: http://www.faqs.org/images/library.jpg + format: AUTO + type: RGB565 # Check the set_url action esphome: diff --git a/tests/components/runtime_image/test_decoder_reuse.cpp b/tests/components/runtime_image/test_decoder_reuse.cpp index 87e77b00be..9c2d00b747 100644 --- a/tests/components/runtime_image/test_decoder_reuse.cpp +++ b/tests/components/runtime_image/test_decoder_reuse.cpp @@ -77,18 +77,12 @@ class TestableRuntimeImage : public RuntimeImage { : RuntimeImage(format, image::IMAGE_TYPE_RGB, image::TRANSPARENCY_OPAQUE, nullptr, false, 0, 0) {} ImageDecoder *decoder() { return this->decoder_.get(); } - - /// Simulates the state a dynamic-format producer (PR #16337) would leave behind: - /// a cached decoder whose format no longer matches the image's format. - /// TODO: once #16337 adds a public way to change the format, drive the mismatch - /// through it and delete this seam. - void plant_decoder(ImageFormat format) { this->decoder_ = this->create_decoder_(format); } }; /// Runs one full decode session. Returns true when every stage succeeded. -static bool decode_all(TestableRuntimeImage &img, const uint8_t *data, size_t len) { +static bool decode_all(TestableRuntimeImage &img, const uint8_t *data, size_t len, ImageFormat format = AUTO) { std::vector buffer(data, data + len); // feed_data needs mutable bytes - if (!img.begin_decode(len)) { + if (!img.begin_decode(len, format)) { return false; } size_t offset = 0; @@ -203,25 +197,51 @@ TEST(RuntimeImageDecoder, ChunkedFeedDecodesLikeDownloadLoop) { } TEST(RuntimeImageDecoder, FormatSwitchEvictsMismatchedDecoder) { - // PNG image holding a stale BMP decoder: begin_decode must evict and recreate. - TestableRuntimeImage png_img(PNG); - png_img.plant_decoder(BMP); - ASSERT_NE(png_img.decoder(), nullptr); - ASSERT_EQ(png_img.decoder()->get_format(), BMP); + // Drive the format switch through begin_decode()'s format parameter, the way + // a dynamic-format producer (online_image MIME detection) does. + TestableRuntimeImage img(AUTO); - ASSERT_TRUE(decode_all(png_img, PNG_RGB, sizeof(PNG_RGB))); - EXPECT_EQ(png_img.decoder()->get_format(), PNG); - expect_pixels(png_img, PNG_RGB_EXPECTED); + ASSERT_TRUE(decode_all(img, BMP_24BPP, sizeof(BMP_24BPP), BMP)); + ASSERT_NE(img.decoder(), nullptr); + ASSERT_EQ(img.decoder()->get_format(), BMP); + expect_pixels(img, BMP_24BPP_EXPECTED); - // And the other direction: BMP image holding a stale PNG decoder. - TestableRuntimeImage bmp_img(BMP); - bmp_img.plant_decoder(PNG); - ASSERT_NE(bmp_img.decoder(), nullptr); - ASSERT_EQ(bmp_img.decoder()->get_format(), PNG); + // Same explicit format again: the decoder must stay warm. + ImageDecoder *bmp_decoder = img.decoder(); + ASSERT_TRUE(decode_all(img, BMP_8BPP, sizeof(BMP_8BPP), BMP)); + expect_pixels(img, BMP_8BPP_EXPECTED); + EXPECT_EQ(img.decoder(), bmp_decoder); - ASSERT_TRUE(decode_all(bmp_img, BMP_24BPP, sizeof(BMP_24BPP))); - EXPECT_EQ(bmp_img.decoder()->get_format(), BMP); - expect_pixels(bmp_img, BMP_24BPP_EXPECTED); + // Different format: the stale decoder must be evicted and recreated. + ASSERT_TRUE(decode_all(img, PNG_RGB, sizeof(PNG_RGB), PNG)); + EXPECT_EQ(img.decoder()->get_format(), PNG); + expect_pixels(img, PNG_RGB_EXPECTED); + + // And back again. + ASSERT_TRUE(decode_all(img, BMP_24BPP, sizeof(BMP_24BPP), BMP)); + EXPECT_EQ(img.decoder()->get_format(), BMP); + expect_pixels(img, BMP_24BPP_EXPECTED); +} + +TEST(RuntimeImageDecoder, AutoFormatFallsBackToConfiguredAndKeepsDecoderWarm) { + // With a configured format, an AUTO begin_decode() must resolve to the + // configured format before the reuse check instead of evicting the decoder. + TestableRuntimeImage img(BMP); + + ASSERT_TRUE(decode_all(img, BMP_24BPP, sizeof(BMP_24BPP), AUTO)); + ImageDecoder *first = img.decoder(); + ASSERT_NE(first, nullptr); + EXPECT_EQ(first->get_format(), BMP); + + ASSERT_TRUE(decode_all(img, BMP_24BPP, sizeof(BMP_24BPP), AUTO)); + expect_pixels(img, BMP_24BPP_EXPECTED); + EXPECT_EQ(img.decoder(), first) << "AUTO must not evict the configured-format decoder"; +} + +TEST(RuntimeImageDecoder, AutoWithoutConfiguredFormatFails) { + // Neither a configured format nor an explicit one: there is nothing to decode with. + TestableRuntimeImage img(AUTO); + EXPECT_FALSE(img.begin_decode(64)); } TEST(RuntimeImageDecoder, ReleaseKeepsDecoderWarm) { diff --git a/tests/integration/fixtures/online_image_auto_detects_image_bmp_mime.yaml b/tests/integration/fixtures/online_image_auto_detects_image_bmp_mime.yaml new file mode 100644 index 0000000000..95e42b6755 --- /dev/null +++ b/tests/integration/fixtures/online_image_auto_detects_image_bmp_mime.yaml @@ -0,0 +1,28 @@ +esphome: + name: online-image-bmp + +host: + +http_request: + +display: + +image: + - platform: online_image + url: http://127.0.0.1:HTTP_PORT/foo.bmp + format: AUTO + id: myimg + type: RGB + on_download_finished: + logger.log: + format: "download finished. cache hit: %u" + args: [cached] + +api: + actions: + - action: fetch_image + then: + - component.update: myimg + +logger: + level: DEBUG diff --git a/tests/integration/fixtures/online_image_auto_detects_redirected_image_bmp_mime.yaml b/tests/integration/fixtures/online_image_auto_detects_redirected_image_bmp_mime.yaml new file mode 100644 index 0000000000..837be776f1 --- /dev/null +++ b/tests/integration/fixtures/online_image_auto_detects_redirected_image_bmp_mime.yaml @@ -0,0 +1,28 @@ +esphome: + name: online-image-bmp + +host: + +http_request: + +display: + +image: + - platform: online_image + url: http://127.0.0.1:HTTP_PORT/foo.bmp + id: myimg + format: AUTO + type: RGB + on_download_finished: + logger.log: + format: "download finished. cache hit: %u" + args: [cached] + +api: + actions: + - action: fetch_image + then: + - component.update: myimg + +logger: + level: DEBUG diff --git a/tests/integration/fixtures/online_image_bmp.yaml b/tests/integration/fixtures/online_image_bmp.yaml index e36514e9ae..2cd0658b56 100644 --- a/tests/integration/fixtures/online_image_bmp.yaml +++ b/tests/integration/fixtures/online_image_bmp.yaml @@ -7,8 +7,9 @@ http_request: display: -online_image: - - url: http://127.0.0.1:HTTP_PORT/foo.bmp +image: + - platform: online_image + url: http://127.0.0.1:HTTP_PORT/foo.bmp id: myimg format: BMP type: RGB diff --git a/tests/integration/online_image_utils.py b/tests/integration/online_image_utils.py new file mode 100644 index 0000000000..02f0023cb5 --- /dev/null +++ b/tests/integration/online_image_utils.py @@ -0,0 +1,158 @@ +"""Shared fixture server and log helpers for the online_image integration tests.""" + +from __future__ import annotations + +import asyncio +from collections.abc import Callable +import re + +# black 8x8 RGB BMP, generated with +# from PIL import Image +# from io import BytesIO +# b = BytesIO() +# img = Image.new("RGB", (8, 8)) +# img.save(b, format="BMP") +# b.getvalue() +BMP_IMAGE = b"BM\xf6\x00\x00\x00\x00\x00\x00\x006\x00\x00\x00(\x00\x00\x00\x08\x00\x00\x00\x08\x00\x00\x00\x01\x00\x18\x00\x00\x00\x00\x00\xc0\x00\x00\x00\xc4\x0e\x00\x00\xc4\x0e\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +LEN_BMP_IMAGE = len(BMP_IMAGE) + + +async def wait_for_download( + downloaded_bytes_future: asyncio.Future, + server_error_future: asyncio.Future, +) -> int: + """Await the downloaded byte count, raising a server handler error first.""" + await asyncio.wait( + {downloaded_bytes_future, server_error_future}, + return_when=asyncio.FIRST_COMPLETED, + ) + if server_error_future.done() and (exc := server_error_future.exception()): + raise exc + # Retrieve a late teardown error so asyncio does not log it at GC + server_error_future.add_done_callback(lambda f: f.exception()) + return downloaded_bytes_future.result() + + +def make_download_watcher( + downloaded_bytes_future: asyncio.Future, + download_finished_future: asyncio.Future, +) -> Callable[[str], None]: + """Build a line callback resolving the futures from the device log.""" + + def check_output(line: str) -> None: + if ( + match := re.search(r"Image fully downloaded, (\d+) bytes", line) + ) and not downloaded_bytes_future.done(): + downloaded_bytes_future.set_result(int(match.group(1))) + if "download finished" in line and not download_finished_future.done(): + download_finished_future.set_result(True) + + return check_output + + +def handle_http( + http_request_future, + content_type: str = "text/plain", + *, + request_path: str = "/foo.bmp", + request_line_consumed: bool = False, + server_error_future: asyncio.Future | None = None, +): + async def handler(reader, writer): + try: + # Only read the request line if it hasn't been consumed by a caller + if not request_line_consumed: + async with asyncio.timeout(1.0): + data = await reader.readuntil(b"\r\n") + + expected_request = f"GET {request_path} HTTP/1.1\r\n".encode() + assert data[: len(expected_request)] == expected_request + + async with asyncio.timeout(1.0): + await reader.readuntil(b"\r\n\r\n") + + if not http_request_future.done(): + http_request_future.set_result(True) + + http_response = [ + b"HTTP/1.1 200 OK", + b"Content-Length: %d" % LEN_BMP_IMAGE, + f"Content-Type: {content_type}".encode(), + b"Connection: close", + b"", + b"", + ] + writer.write(b"\r\n".join(http_response)) + await writer.drain() + + writer.write(BMP_IMAGE) + + await writer.drain() + except Exception as exc: + if server_error_future is not None and not server_error_future.done(): + server_error_future.set_exception(exc) + if not http_request_future.done(): + http_request_future.set_exception(exc) + raise + finally: + writer.close() + + return handler + + +def handle_http_redirect( + http_request_future, final_request_future, server_error_future, port_holder +): + async def handler(reader, writer): + try: + async with asyncio.timeout(1.0): + request = await reader.readuntil(b"\r\n") + + if ( + request[: len(b"GET /foo.bmp HTTP/1.1\r\n")] + == b"GET /foo.bmp HTTP/1.1\r\n" + ): + if not http_request_future.done(): + http_request_future.set_result(True) + async with asyncio.timeout(1.0): + await reader.readuntil(b"\r\n\r\n") + + http_response = [ + b"HTTP/1.1 302 Found", + f"Location: http://127.0.0.1:{port_holder['port']}/final.bmp".encode(), + b"Content-Type: text/html", + b"Content-Length: 0", + b"Connection: close", + b"", + b"", + ] + writer.write(b"\r\n".join(http_response)) + await writer.drain() + return + + assert ( + request[: len(b"GET /final.bmp HTTP/1.1\r\n")] + == b"GET /final.bmp HTTP/1.1\r\n" + ) + if not final_request_future.done(): + final_request_future.set_result(True) + await handle_http( + final_request_future, + "image/bmp", + request_path="/final.bmp", + request_line_consumed=True, + server_error_future=server_error_future, + )(reader, writer) + except Exception as exc: + # Route handler failures to the dedicated error future so they're not silently lost + if not server_error_future.done(): + server_error_future.set_exception(exc) + if not http_request_future.done(): + http_request_future.set_exception(exc) + if not final_request_future.done(): + final_request_future.set_exception(exc) + raise + finally: + writer.close() + + return handler diff --git a/tests/integration/test_online_image_auto_detects_image_bmp_mime.py b/tests/integration/test_online_image_auto_detects_image_bmp_mime.py new file mode 100644 index 0000000000..3d2f6afd95 --- /dev/null +++ b/tests/integration/test_online_image_auto_detects_image_bmp_mime.py @@ -0,0 +1,71 @@ +"""Test that online_image AUTO format detection reads the Content-Type header.""" + +from __future__ import annotations + +import asyncio + +import pytest + +from .online_image_utils import ( + LEN_BMP_IMAGE, + handle_http, + make_download_watcher, + wait_for_download, +) +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_online_image_auto_detects_image_bmp_mime( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """AUTO format detection should honor the final response MIME type without explicit format.""" + loop = asyncio.get_running_loop() + http_request_future = loop.create_future() + server_error_future = loop.create_future() + download_finished_future = loop.create_future() + downloaded_bytes_future = loop.create_future() + + check_output = make_download_watcher( + downloaded_bytes_future, download_finished_future + ) + + server = await asyncio.start_server( + handle_http( + http_request_future, + "image/bmp", + server_error_future=server_error_future, + ), + "127.0.0.1", + 0, + ) + http_server_port = server.sockets[0].getsockname()[1] + + config = yaml_config.replace("HTTP_PORT", str(http_server_port)) + + async with ( + server, + run_compiled(config, line_callback=check_output), + api_client_connected() as client, + ): + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "online-image-bmp" + + _, services = await client.list_entities_services() + request_service = next((s for s in services if s.name == "fetch_image"), None) + assert request_service is not None + + await client.execute_service(request_service, {}) + + async with asyncio.timeout(0.1): + await http_request_future + + async with asyncio.timeout(0.5): + numbytes = await wait_for_download( + downloaded_bytes_future, server_error_future + ) + assert numbytes == LEN_BMP_IMAGE + await download_finished_future diff --git a/tests/integration/test_online_image_auto_detects_redirected_image_bmp_mime.py b/tests/integration/test_online_image_auto_detects_redirected_image_bmp_mime.py new file mode 100644 index 0000000000..3fe28ac413 --- /dev/null +++ b/tests/integration/test_online_image_auto_detects_redirected_image_bmp_mime.py @@ -0,0 +1,71 @@ +"""Test that AUTO format detection uses the final Content-Type after redirects.""" + +from __future__ import annotations + +import asyncio + +import pytest + +from .online_image_utils import ( + LEN_BMP_IMAGE, + handle_http_redirect, + make_download_watcher, + wait_for_download, +) +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_online_image_auto_detects_redirected_image_bmp_mime( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Redirect hops should not leave the 302 HTML Content-Type in place for the final image.""" + loop = asyncio.get_running_loop() + http_request_future = loop.create_future() + final_request_future = loop.create_future() + server_error_future = loop.create_future() + download_finished_future = loop.create_future() + downloaded_bytes_future = loop.create_future() + + check_output = make_download_watcher( + downloaded_bytes_future, download_finished_future + ) + + port_holder = {} + server = await asyncio.start_server( + handle_http_redirect( + http_request_future, final_request_future, server_error_future, port_holder + ), + "127.0.0.1", + 0, + ) + port_holder["port"] = server.sockets[0].getsockname()[1] + + config = yaml_config.replace("HTTP_PORT", str(port_holder["port"])) + + async with ( + server, + run_compiled(config, line_callback=check_output), + api_client_connected() as client, + ): + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "online-image-bmp" + + _, services = await client.list_entities_services() + request_service = next((s for s in services if s.name == "fetch_image"), None) + assert request_service is not None + + await client.execute_service(request_service, {}) + + async with asyncio.timeout(0.1): + await http_request_future + async with asyncio.timeout(0.5): + await final_request_future + numbytes = await wait_for_download( + downloaded_bytes_future, server_error_future + ) + assert numbytes == LEN_BMP_IMAGE + await download_finished_future diff --git a/tests/integration/test_online_image_bmp.py b/tests/integration/test_online_image_bmp.py index 7c32154fdd..871a97b753 100644 --- a/tests/integration/test_online_image_bmp.py +++ b/tests/integration/test_online_image_bmp.py @@ -1,62 +1,12 @@ from __future__ import annotations import asyncio -import re import pytest +from .online_image_utils import LEN_BMP_IMAGE, handle_http, make_download_watcher from .types import APIClientConnectedFactory, RunCompiledFunction -# black 8x8 RGB BMP, generated with -# from PIL import Image -# from io import BytesIO -# b = BytesIO() -# img = Image.new("RGB", (8, 8)) -# img.save(b, format="BMP") -# b.getvalue() -BMP_IMAGE = b"BM\xf6\x00\x00\x00\x00\x00\x00\x006\x00\x00\x00(\x00\x00\x00\x08\x00\x00\x00\x08\x00\x00\x00\x01\x00\x18\x00\x00\x00\x00\x00\xc0\x00\x00\x00\xc4\x0e\x00\x00\xc4\x0e\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" -LEN_BMP_IMAGE = len(BMP_IMAGE) - - -def handle_http(http_request_future): - async def handler(reader, writer): - try: - async with asyncio.timeout(1.0): - data = await reader.readuntil(b"\r\n") - - # ensure our request matches the expectation - expected_request = b"GET /foo.bmp HTTP/1.1\r\n" - assert data[: len(expected_request)] == expected_request - - # consume rest of request - async with asyncio.timeout(1.0): - data = await reader.readuntil(b"\r\n\r\n") - - http_request_future.set_result(True) - - http_response = [ - b"HTTP/1.1 200 OK", - b"Content-Length: %d" % LEN_BMP_IMAGE, - b"Content-Type: text/plain", - b"Connection: close", - b"", - b"", - ] - writer.write(b"\r\n".join(http_response)) - await writer.drain() - - writer.write(BMP_IMAGE) - - await writer.drain() - except Exception as exc: - if not http_request_future.done(): - http_request_future.set_exception(exc) - raise - finally: - writer.close() - - return handler - @pytest.mark.asyncio async def test_online_image_bmp( @@ -72,14 +22,9 @@ async def test_online_image_bmp( download_finished_future = loop.create_future() downloaded_bytes_future = loop.create_future() - def check_output(line: str) -> None: - """Check log output for expected messages.""" - - if match := re.search(r"Image fully downloaded, (\d+) bytes", line): - downloaded_bytes_future.set_result(int(match.group(1))) - - if "download finished" in line: - download_finished_future.set_result(True) + check_output = make_download_watcher( + downloaded_bytes_future, download_finished_future + ) server = await asyncio.start_server( handle_http(http_request_future), "127.0.0.1", 0 From e6c49e2bd9f16a4cf32cd68958978bed48ff30fc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:25:39 -0500 Subject: [PATCH 3/6] [core] Retry transient network errors when downloading external files (#18538) --- esphome/external_files.py | 21 ++- esphome/framework_helpers.py | 32 +---- esphome/net_retry.py | 114 ++++++++++++++++ tests/unit_tests/test_external_files.py | 138 +++++++++++++++++++- tests/unit_tests/test_framework_helpers.py | 38 ------ tests/unit_tests/test_net_retry.py | 143 +++++++++++++++++++++ 6 files changed, 416 insertions(+), 70 deletions(-) create mode 100644 esphome/net_retry.py create mode 100644 tests/unit_tests/test_net_retry.py diff --git a/esphome/external_files.py b/esphome/external_files.py index f30d429425..58be4a7c26 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -16,6 +16,7 @@ from esphome.const import CONF_FILE, CONF_TYPE, CONF_URL, __version__ from esphome.core import CORE, EsphomeError, TimePeriodSeconds from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import write_file +from esphome.net_retry import fetch_with_retry from esphome.types import ConfigType _LOGGER = logging.getLogger(__name__) @@ -157,8 +158,17 @@ def has_remote_file_changed( } if etag := _read_etag(local_file_path): headers[IF_NONE_MATCH] = etag - response = requests.head( - url, headers=headers, timeout=timeout, allow_redirects=True + # Retried so allow_stale=False consumers don't hard-fail on a + # healed flake. Only connection-level failures retry: HEAD + # never raises on HTTP status (servers rejecting HEAD with + # 405/501 must fall through to the GET), so 5xx is handled by + # the GET's own retry. + response = fetch_with_retry( + url, + lambda: requests.head( + url, headers=headers, timeout=timeout, allow_redirects=True + ), + what="Revalidation", ) _LOGGER.debug( @@ -293,7 +303,7 @@ def download_content( _LOGGER.info("Downloading %s", url) _LOGGER.debug("Saving to %s", path) - try: + def _fetch() -> tuple[requests.Response, bytes]: req = requests.get( url, timeout=timeout, @@ -304,7 +314,10 @@ def download_content( # and mid-stream connection errors all surface here as # RequestException subclasses, so this needs the same fall-back # treatment as the request itself. - data = req.content + return req, req.content + + try: + req, data = fetch_with_retry(url, _fetch) except requests.exceptions.RequestException as e: if path.exists(): # Memoized so a flaky host warns once per run, not per consumer. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index b8a43220ff..105791c518 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -15,6 +15,7 @@ from typing import IO, TYPE_CHECKING from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import ProgressBar, rmtree +from esphome.net_retry import NETWORK_MAX_ATTEMPTS, is_transient_download_error if TYPE_CHECKING: import requests @@ -29,8 +30,9 @@ _LOGGER = logging.getLogger(__name__) _MIRROR_ATTEMPTS = 3 # Passes over the whole mirror list when a transient network error is in -# the mix; matches git.py's _NETWORK_MAX_ATTEMPTS (3 tries, 2s/4s backoff). -_MIRROR_SWEEP_ATTEMPTS = 3 +# the mix; shares net_retry's policy (3 tries, 2s/4s backoff), which in +# turn matches git.py's _NETWORK_MAX_ATTEMPTS. +_MIRROR_SWEEP_ATTEMPTS = NETWORK_MAX_ATTEMPTS def get_project_link_flags() -> list[str]: @@ -903,30 +905,6 @@ def _spent_attempts_error(e: Exception, attempts: int) -> Exception: return err -def _is_transient_download_error(e: Exception) -> bool: - """Return True when a download failure is worth retrying. - - Connection-level failures and HTTP 429/5xx are transient. Other HTTP - errors, local errors, and exhausted-attempts EsphomeError wrappers - (their per-mirror retries are already spent) are permanent. - """ - # Imported lazily: requests is a heavy import (~85ms) and is only - # needed when actually downloading, never during config validation. - import requests - - if isinstance(e, requests.exceptions.HTTPError): - resp = e.response - return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) - return isinstance( - e, - ( - requests.exceptions.ConnectionError, - requests.exceptions.Timeout, - requests.exceptions.ChunkedEncodingError, - ), - ) - - def _try_mirrors_once( urls: list[str], path_target: Path | None, @@ -1131,7 +1109,7 @@ def download_from_mirrors( # Permanent failures (404, verification mismatch) won't heal; # only retry when a transient error is in the mix (as git.py does). transient = next( - ((u, e) for u, e in sweep_failures if _is_transient_download_error(e)), + ((u, e) for u, e in sweep_failures if is_transient_download_error(e)), None, ) if transient is None: diff --git a/esphome/net_retry.py b/esphome/net_retry.py new file mode 100644 index 0000000000..f7e6e601ea --- /dev/null +++ b/esphome/net_retry.py @@ -0,0 +1,114 @@ +"""Retry policy for HTTP downloads. + +Kept import-light on purpose: this module is imported at config time, so it +must not pull in requests (a heavy import, ~85ms) at module scope. +""" + +from __future__ import annotations + +from collections.abc import Callable +import logging +import time + +_LOGGER = logging.getLogger(__name__) + +# 3 tries with 2s/4s backoff, matching git.py's _NETWORK_MAX_ATTEMPTS. +# Callers memoize failures so a flaky host pays this once per file per run. +NETWORK_MAX_ATTEMPTS = 3 + + +def _is_permanent_dns_failure(e: BaseException) -> bool: + """Whether a hard socket.gaierror hides in ``e``'s exception chain. + + EAI_AGAIN (flaky resolver) stays retryable; anything else is permanent + so offline builds fall back to their cache without sleeping first. + Narrower than git.py, which retries NXDOMAIN too. + + Walks ``__cause__``, ``args`` (requests wraps MaxRetryError without + ``from``) and MaxRetryError's ``reason``, but not implicit + ``__context__``: an unrelated earlier attempt's resolution failure + must not reclassify an error it did not cause. + """ + import socket + + seen: set[int] = set() + stack: list[BaseException] = [e] + while stack: + exc = stack.pop() + if id(exc) in seen: + continue + if ( + isinstance(exc, socket.gaierror) + and exc.errno is not None + and exc.errno != socket.EAI_AGAIN + ): + return True + seen.add(id(exc)) + stack.extend( + nxt + for nxt in ( + exc.__cause__, + getattr(exc, "reason", None), # urllib3 MaxRetryError + *exc.args, + ) + if isinstance(nxt, BaseException) + ) + return False + + +def is_transient_download_error(e: Exception) -> bool: + """Return True when a download failure is worth retrying. + + Connection-level failures and HTTP 429/5xx are transient; hard DNS + failures, other HTTP errors, and local errors are permanent. + """ + # Imported lazily: requests is a heavy import (~85ms) and is only + # needed when actually downloading, never during config validation. + import requests + + if isinstance(e, requests.exceptions.HTTPError): + resp = e.response + return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) + if isinstance(e, requests.exceptions.ConnectionError) and _is_permanent_dns_failure( + e + ): + return False + # SSLError (a ConnectionError subclass) stays transient on purpose: it + # also covers mid-handshake connection drops, not just bad certificates. + return isinstance( + e, + ( + requests.exceptions.ConnectionError, + requests.exceptions.Timeout, + requests.exceptions.ChunkedEncodingError, + requests.exceptions.ContentDecodingError, + ), + ) + + +def fetch_with_retry[T](url: str, fetch: Callable[[], T], what: str = "Download") -> T: + """Run ``fetch``, retrying transient failures with 2s/4s backoff. + + Permanent failures and the final attempt propagate to the caller; + ``what`` names the operation in the retry warning. + """ + import requests + + for attempt in range(1, NETWORK_MAX_ATTEMPTS): + try: + return fetch() + except requests.exceptions.RequestException as e: + if not is_transient_download_error(e): + raise + delay = 2**attempt + _LOGGER.warning( + "%s of %s failed: %s. Retrying in %d seconds... (attempt %d/%d)", + what, + url, + e, + delay, + attempt + 1, + NETWORK_MAX_ATTEMPTS, + ) + time.sleep(delay) + return fetch() diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index 4e993ff4f3..caefb7ed5c 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -4,7 +4,7 @@ import os from pathlib import Path import time from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest import requests @@ -81,6 +81,15 @@ def mock_download_content_many() -> MagicMock: yield m +@pytest.fixture +def mock_retry_sleep() -> MagicMock: + """Patch the retry backoff sleep (process-wide; net_retry.time is the + global module) so transient-error tests don't really wait 2s/4s. + """ + with patch("esphome.net_retry.time.sleep") as m: + yield m + + def test_compute_local_file_dir(setup_core: Path) -> None: """Test compute_local_file_dir creates and returns correct path.""" domain = "font" @@ -495,6 +504,7 @@ class _BodyReadErrorResponse: def test_download_content_with_body_read_error_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """Body-read errors (chunked-decode/gzip-decode/mid-stream connection @@ -519,6 +529,7 @@ def test_download_content_with_body_read_error_uses_cache( def test_download_content_with_body_read_error_no_cache_fails( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """A body-read failure with no cache available must surface as a @@ -535,6 +546,131 @@ def test_download_content_with_body_read_error_no_cache_fails( external_files.download_content("https://example.com/file.txt", test_file) +def test_download_content_retries_transient_error_then_succeeds( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Transient failures (connection reset, timeout) are retried with 2s/4s + backoff before giving up; a late success downloads normally.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + requests.exceptions.Timeout("timed out"), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert test_file.read_bytes() == b"downloaded" + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_transient_error_exhausts_attempts( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """A persistent transient failure gives up after three attempts and then + follows the normal no-cache error path.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + mock_requests_get.side_effect = requests.exceptions.ConnectionError("reset by peer") + + with pytest.raises(Invalid, match="Could not download from.*reset by peer"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_non_transient_error_not_retried( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Permanent failures like a 404 fail on the first attempt.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + + response = MagicMock() + response.status_code = 404 + mock_requests_get.side_effect = requests.exceptions.HTTPError( + "404 Client Error", response=response + ) + + with pytest.raises(Invalid, match="Could not download from.*404"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_requests_get.call_count == 1 + mock_retry_sleep.assert_not_called() + + +def test_download_content_retries_body_read_error( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Mid-stream failures surfacing from `.content` are retried too.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + _BodyReadErrorResponse( + requests.exceptions.ChunkedEncodingError("body truncated") + ), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert mock_requests_get.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + + +def test_has_remote_file_changed_retries_transient_error( + mock_requests_head: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A HEAD revalidation that fails transiently then returns 304 does not + mark the cached copy stale, and the retry warning names the operation.""" + test_file = setup_core / "cached.txt" + test_file.write_bytes(b"cached content") + + ok = MagicMock() + ok.status_code = 304 + ok.headers = {} + mock_requests_head.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + ok, + ] + + changed = external_files.has_remote_file_changed( + "https://example.com/file.txt", test_file + ) + + assert changed is False + assert test_file not in external_files._run_data().stale_paths + assert mock_requests_head.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + assert "Revalidation of" in caplog.text + + def test_download_content_skip_external_update_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 2022c15bfe..500705ef67 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -23,7 +23,6 @@ from esphome.core import EsphomeError from esphome.framework_helpers import ( _7z_extract_all, _detect_archive_root, - _is_transient_download_error, _rename_with_retry, _tar_extract_all, _zip_extract_all, @@ -1594,43 +1593,6 @@ class TestDownloadFromMirrors: mock_sleep.assert_not_called() -def _http_error(status: int) -> req.HTTPError: - """An HTTPError carrying a response with the given status, as raised by - ``raise_for_status`` on a real response.""" - resp = MagicMock() - resp.status_code = status - return req.HTTPError(str(status), response=resp) - - -class TestIsTransientDownloadError: - def test_connection_errors_are_transient(self) -> None: - assert _is_transient_download_error(req.ConnectionError("reset")) - assert _is_transient_download_error(req.Timeout("timed out")) - assert _is_transient_download_error( - req.exceptions.ChunkedEncodingError("dropped") - ) - - def test_http_statuses(self) -> None: - assert not _is_transient_download_error(_http_error(404)) - assert not _is_transient_download_error(_http_error(403)) - assert _is_transient_download_error(_http_error(429)) - assert _is_transient_download_error(_http_error(503)) - - def test_http_error_without_response_is_permanent(self) -> None: - assert not _is_transient_download_error(req.HTTPError("boom")) - - def test_exhausted_resume_attempts_are_permanent(self) -> None: - """download_with_resume already spent its own resume attempts; its - EsphomeError wrapper is not retried again at the sweep level.""" - wrapped = EsphomeError("Failed to download after 3 attempts") - wrapped.__cause__ = req.ConnectionError("down") - assert not _is_transient_download_error(wrapped) - - def test_unrelated_errors_are_permanent(self) -> None: - assert not _is_transient_download_error(OSError("disk full")) - assert not _is_transient_download_error(EsphomeError("size mismatch")) - - def test_importing_framework_helpers_does_not_import_requests() -> None: """Importing framework_helpers must not drag in requests. diff --git a/tests/unit_tests/test_net_retry.py b/tests/unit_tests/test_net_retry.py new file mode 100644 index 0000000000..c22bda5ee5 --- /dev/null +++ b/tests/unit_tests/test_net_retry.py @@ -0,0 +1,143 @@ +"""Tests for esphome.net_retry.""" + +import socket +from unittest.mock import MagicMock, call, patch + +import pytest +import requests as req + +from esphome.core import EsphomeError +from esphome.net_retry import fetch_with_retry, is_transient_download_error + + +def _http_error(status: int) -> req.HTTPError: + """An HTTPError carrying a response with the given status, as raised by + ``raise_for_status`` on a real response.""" + resp = MagicMock() + resp.status_code = status + return req.HTTPError(str(status), response=resp) + + +class TestIsTransientDownloadError: + def test_connection_errors_are_transient(self) -> None: + assert is_transient_download_error(req.ConnectionError("reset")) + assert is_transient_download_error(req.Timeout("timed out")) + assert is_transient_download_error( + req.exceptions.ChunkedEncodingError("dropped") + ) + assert is_transient_download_error( + req.exceptions.ContentDecodingError("gzip stream truncated") + ) + + def test_http_statuses(self) -> None: + assert not is_transient_download_error(_http_error(404)) + assert not is_transient_download_error(_http_error(403)) + assert is_transient_download_error(_http_error(429)) + assert is_transient_download_error(_http_error(503)) + + def test_http_error_without_response_is_permanent(self) -> None: + assert not is_transient_download_error(req.HTTPError("boom")) + + def test_hard_dns_failures_are_permanent(self) -> None: + """Hard resolution failures are permanent via both the cause chain + and MaxRetryError.reason.""" + from urllib3.exceptions import MaxRetryError, NameResolutionError + + gai = socket.gaierror(socket.EAI_NONAME, "nodename nor servname provided") + + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + assert not is_transient_download_error(chained) + + # The real urllib3 shape: gaierror on NameResolutionError.__cause__, + # carried by MaxRetryError.reason. + try: + raise NameResolutionError("example.invalid", None, gai) from gai + except NameResolutionError as nre: + wrapped = req.ConnectionError( + MaxRetryError(None, "http://example.invalid/", reason=nre) + ) + assert not is_transient_download_error(wrapped) + + # A garden-variety connection reset stays transient. + assert is_transient_download_error(req.ConnectionError("reset by peer")) + + def test_temporary_dns_failure_stays_transient(self) -> None: + """EAI_AGAIN (flaky resolver) stays retryable.""" + gai = socket.gaierror(socket.EAI_AGAIN, "temporary failure in name resolution") + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + + assert is_transient_download_error(chained) + + def test_implicit_context_does_not_reclassify(self) -> None: + """A gaierror riding along as implicit __context__ must not turn a + genuine connection reset permanent.""" + try: + try: + raise socket.gaierror(socket.EAI_NONAME, "first attempt") + except socket.gaierror: + raise req.ConnectionError("reset by peer") from None + except req.ConnectionError as reset: + assert reset.__context__ is not None + assert is_transient_download_error(reset) + + def test_gaierror_without_errno_stays_transient(self) -> None: + """A gaierror carrying no EAI code cannot prove a hard failure.""" + chained = req.ConnectionError("resolution failed") + chained.__cause__ = socket.gaierror("no errno") + + assert is_transient_download_error(chained) + + def test_mixed_chain_hard_failure_wins(self) -> None: + """EAI_AGAIN in the chain does not mask a hard failure elsewhere.""" + again = socket.gaierror(socket.EAI_AGAIN, "temporary failure") + hard = socket.gaierror(socket.EAI_NONAME, "unknown host") + + outer = req.ConnectionError(hard) + outer.__cause__ = again + assert not is_transient_download_error(outer) + + outer = req.ConnectionError(again) + outer.__cause__ = hard + assert not is_transient_download_error(outer) + + def test_dns_walk_survives_exception_cycles(self) -> None: + """A cyclic cause chain must terminate (and stay transient when no + resolution failure is present).""" + outer = req.ConnectionError("a") + inner = ValueError("b") + outer.__cause__ = inner + inner.__cause__ = outer + + assert is_transient_download_error(outer) + + def test_exhausted_resume_attempts_are_permanent(self) -> None: + """download_with_resume already spent its own resume attempts; its + EsphomeError wrapper is not retried again at the sweep level.""" + wrapped = EsphomeError("Failed to download after 3 attempts") + wrapped.__cause__ = req.ConnectionError("down") + assert not is_transient_download_error(wrapped) + + def test_unrelated_errors_are_permanent(self) -> None: + assert not is_transient_download_error(OSError("disk full")) + assert not is_transient_download_error(EsphomeError("size mismatch")) + + +class TestFetchWithRetry: + def test_logs_the_upcoming_attempt_number( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The warning names the attempt about to run, not the failed one.""" + with ( + patch("esphome.net_retry.time.sleep") as mock_sleep, + pytest.raises(req.ConnectionError), + ): + fetch_with_retry( + "https://example.com/f", + lambda: (_ for _ in ()).throw(req.ConnectionError("reset")), + ) + + assert mock_sleep.call_args_list == [call(2), call(4)] + assert "(attempt 2/3)" in caplog.text + assert "(attempt 3/3)" in caplog.text From 9179fe26ce7472687dedecb46139286d776eba87 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:38:19 -0500 Subject: [PATCH 4/6] [api] Fix loop stall that triggers the task watchdog when entity list sends block (#18577) --- esphome/components/api/api_connection.cpp | 16 +- esphome/components/api/api_connection.h | 8 +- esphome/components/api/api_frame_helper.h | 2 +- esphome/components/api/list_entities.cpp | 10 +- esphome/components/web_server/list_entities.h | 1 - esphome/components/web_server/web_server.cpp | 10 +- .../web_server_idf/web_server_idf.cpp | 4 +- esphome/core/component_iterator.cpp | 25 ++- esphome/core/component_iterator.h | 43 +++- tests/components/camera/__init__.py | 11 + .../camera/test_component_iterator_camera.cpp | 79 +++++++ tests/components/core/benchmark.yaml | 11 + .../core/test_component_iterator.cpp | 195 ++++++++++++++++++ tests/integration/README.md | 2 + .../api_list_entities_backpressure.yaml | 23 +++ .../sndbuf_pin_component/__init__.py | 20 ++ .../sndbuf_pin_component.cpp | 55 +++++ .../sndbuf_pin_component.h | 21 ++ tests/integration/raw_api_client.py | 148 +++++++++++++ .../test_api_list_entities_backpressure.py | 110 ++++++++++ 20 files changed, 750 insertions(+), 44 deletions(-) create mode 100644 tests/components/camera/__init__.py create mode 100644 tests/components/camera/test_component_iterator_camera.cpp create mode 100644 tests/components/core/benchmark.yaml create mode 100644 tests/components/core/test_component_iterator.cpp create mode 100644 tests/integration/fixtures/api_list_entities_backpressure.yaml create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h create mode 100644 tests/integration/raw_api_client.py create mode 100644 tests/integration/test_api_list_entities_backpressure.py diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 2eb8c21c73..c1dded1271 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -412,15 +412,15 @@ void APIConnection::finalize_iterator_sync_() { } void APIConnection::process_iterator_batch_(ComponentIterator &iterator) { - size_t initial_size = this->deferred_batch_.size(); - size_t max_batch = MAX_INITIAL_PER_BATCH; - while (!iterator.completed() && (this->deferred_batch_.size() - initial_size) < max_batch) { - iterator.advance(); - } + // Budget by remaining batch capacity so a pass cannot overfill the batch; + // stops early on a refused send and resumes next loop pass + size_t batch_size = this->deferred_batch_.size(); + if (batch_size < MAX_INITIAL_BATCH_SIZE) + iterator.try_advance(MAX_INITIAL_BATCH_SIZE - batch_size); - // If the batch is full, process it immediately - // Note: iterator.advance() already calls schedule_batch_() via schedule_message_() - if (this->deferred_batch_.size() >= max_batch) { + // Flush immediately once enough is queued (not guaranteed every pass); + // partial batches go out via the batch timer or finalize_iterator_sync_() + if (this->deferred_batch_.size() >= MAX_INITIAL_BATCH_SIZE) { this->process_batch_(); } } diff --git a/esphome/components/api/api_connection.h b/esphome/components/api/api_connection.h index bb51a13000..1b47c23cfe 100644 --- a/esphome/components/api/api_connection.h +++ b/esphome/components/api/api_connection.h @@ -53,11 +53,11 @@ void log_dropped_message(const char *tag, int line, const LogString *what); // Keepalive timeout in milliseconds static constexpr uint32_t KEEPALIVE_TIMEOUT_MS = 60000; -// Maximum number of entities to process in a single batch during initial state/info sending -static constexpr size_t MAX_INITIAL_PER_BATCH = 34; +// Deferred batch size cap during initial state/info sync +static constexpr size_t MAX_INITIAL_BATCH_SIZE = 34; // Verify MAX_MESSAGES_PER_BATCH (defined in api_frame_helper.h) can hold the initial batch -static_assert(MAX_MESSAGES_PER_BATCH >= MAX_INITIAL_PER_BATCH, - "MAX_MESSAGES_PER_BATCH must be >= MAX_INITIAL_PER_BATCH"); +static_assert(MAX_MESSAGES_PER_BATCH >= MAX_INITIAL_BATCH_SIZE, + "MAX_MESSAGES_PER_BATCH must be >= MAX_INITIAL_BATCH_SIZE"); #ifdef USE_BENCHMARK class APIConnection; diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index 9c49956bbd..1c60bb87a5 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -36,7 +36,7 @@ static constexpr uint16_t MAX_MESSAGE_SIZE = 32768; // 32 KiB for ESP32 and oth static constexpr uint16_t RX_BUF_NULL_TERMINATOR = 1; // Maximum number of messages to batch in a single write operation -// Must be >= MAX_INITIAL_PER_BATCH in api_connection.h (enforced by static_assert there) +// Must be >= MAX_INITIAL_BATCH_SIZE in api_connection.h (enforced by static_assert there) static constexpr size_t MAX_MESSAGES_PER_BATCH = 34; // Max client name length (e.g., "Home Assistant 2026.1.0.dev0" = 28 chars) diff --git a/esphome/components/api/list_entities.cpp b/esphome/components/api/list_entities.cpp index f9e645b506..57ff616ca7 100644 --- a/esphome/components/api/list_entities.cpp +++ b/esphome/components/api/list_entities.cpp @@ -95,9 +95,17 @@ bool ListEntitiesIterator::on_end() { return this->client_->send_list_info_done( ListEntitiesIterator::ListEntitiesIterator(APIConnection *client) : client_(client) {} #ifdef USE_API_USER_DEFINED_ACTIONS +// Yield after every Nth service; bounds direct (non-batched) writes per loop pass +static constexpr uint8_t SERVICE_YIELD_INTERVAL = 3; + bool ListEntitiesIterator::on_service(UserServiceDescriptor *service) { auto resp = service->encode_list_service_response(); - return this->client_->send_message(resp); + if (!this->client_->send_message(resp)) + return false; + // at_ is this service's index + if ((this->at_ + 1) % SERVICE_YIELD_INTERVAL == 0) + this->yield_after_step_(); + return true; } #endif diff --git a/esphome/components/web_server/list_entities.h b/esphome/components/web_server/list_entities.h index 3edb84f555..dc32cbd1ad 100644 --- a/esphome/components/web_server/list_entities.h +++ b/esphome/components/web_server/list_entities.h @@ -35,7 +35,6 @@ class ListEntitiesIterator final : public ComponentIterator { #undef ENTITY_TYPE_ #undef ENTITY_CONTROLLER_TYPE_ // NOLINTEND(bugprone-macro-parentheses) - bool completed() { return this->state_ == IteratorState::NONE; } protected: const WebServer *web_server_; diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index 9e50b7a394..ec536910e5 100644 --- a/esphome/components/web_server/web_server.cpp +++ b/esphome/components/web_server/web_server.cpp @@ -214,8 +214,8 @@ void DeferredUpdateEventSource::process_deferred_queue_() { void DeferredUpdateEventSource::loop() { process_deferred_queue_(); - if (!this->entities_iterator_.completed()) - this->entities_iterator_.advance(); + // One step per loop; refusals retry next pass + this->entities_iterator_.try_advance(1); } void DeferredUpdateEventSource::deferrable_send_state(void *source, const char *event_type, @@ -321,12 +321,6 @@ void DeferredUpdateEventSourceList::on_client_connect_(DeferredUpdateEventSource #endif source->entities_iterator_.begin(ws->include_internal_); - - // just dump them all up-front and take advantage of the deferred queue - // on second thought that takes too long, but leaving the commented code here for debug purposes - // while(!source->entities_iterator_.completed()) { - // source->entities_iterator_.advance(); - //} }); } diff --git a/esphome/components/web_server_idf/web_server_idf.cpp b/esphome/components/web_server_idf/web_server_idf.cpp index 993fb6c035..9550570cdc 100644 --- a/esphome/components/web_server_idf/web_server_idf.cpp +++ b/esphome/components/web_server_idf/web_server_idf.cpp @@ -935,8 +935,8 @@ void AsyncEventSourceResponse::process_buffer_() { void AsyncEventSourceResponse::loop() { process_buffer_(); process_deferred_queue_(); - if (!this->entities_iterator_.completed()) - this->entities_iterator_.advance(); + // One step per loop; refusals retry next pass + this->entities_iterator_.try_advance(1); } bool AsyncEventSourceResponse::try_send_nodefer(const char *message, size_t message_len, const char *event, uint32_t id, diff --git a/esphome/core/component_iterator.cpp b/esphome/core/component_iterator.cpp index f4d3c05e19..3a497db741 100644 --- a/esphome/core/component_iterator.cpp +++ b/esphome/core/component_iterator.cpp @@ -22,23 +22,23 @@ void ComponentIterator::advance_platform_() { this->at_ = 0; } -void ComponentIterator::advance() { +bool ComponentIterator::advance_step_() { switch (this->state_) { case IteratorState::NONE: // not started - return; + return false; case IteratorState::BEGIN: if (this->on_begin()) { advance_platform_(); + return true; } - break; + return false; // Entity iterator cases (generated from entity_types.h) // NOLINTBEGIN(bugprone-macro-parentheses) #define ENTITY_TYPE_(type, singular, plural, count, upper) \ case IteratorState::upper: \ - this->process_platform_item_(App.get_##plural(), &ComponentIterator::on_##singular); \ - break; + return this->process_platform_item_(App.get_##plural(), &ComponentIterator::on_##singular); #define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ ENTITY_TYPE_(type, singular, plural, count, upper) #include "esphome/core/entity_types.h" @@ -48,26 +48,29 @@ void ComponentIterator::advance() { #ifdef USE_API_USER_DEFINED_ACTIONS case IteratorState::SERVICE: - this->process_platform_item_(api::global_api_server->get_user_services(), &ComponentIterator::on_service); - break; + return this->process_platform_item_(api::global_api_server->get_user_services(), &ComponentIterator::on_service); #endif #ifdef USE_CAMERA case IteratorState::CAMERA: { camera::Camera *camera_instance = camera::Camera::instance(); - if (camera_instance != nullptr && (!camera_instance->is_internal() || this->include_internal_)) { - this->on_camera(camera_instance); + if (camera_instance != nullptr && (!camera_instance->is_internal() || this->include_internal_) && + !this->on_camera(camera_instance)) { + return false; } advance_platform_(); - } break; + return true; + } #endif case IteratorState::MAX: if (this->on_end()) { this->state_ = IteratorState::NONE; + return true; } - return; + return false; } + return false; } bool ComponentIterator::on_end() { return true; } diff --git a/esphome/core/component_iterator.h b/esphome/core/component_iterator.h index d271fcfed0..fac09e9e14 100644 --- a/esphome/core/component_iterator.h +++ b/esphome/core/component_iterator.h @@ -30,7 +30,23 @@ class RadioFrequency; class ComponentIterator { public: void begin(bool include_internal = false); - void advance(); + /// Run up to max_steps iteration steps; stops early when iteration + /// completes or a callback refuses (that step is retried on the next + /// call). Inline so an idle (completed) iterator costs one compare, no call. + ESPHOME_ALWAYS_INLINE void try_advance(size_t max_steps) { + size_t steps = 0; + while (steps < max_steps && !this->completed()) { + this->yield_requested_ = false; + if (!this->advance_step_()) + break; + steps++; + if (this->yield_requested_) + break; + } + } + // Remove before 2027.3.0 + ESPDEPRECATED("Use try_advance() instead. Removed in 2027.3.0", "2026.8.1") + void advance() { this->try_advance(1); } bool completed() const { return this->state_ == IteratorState::NONE; } virtual bool on_begin(); // Pure virtual entity callbacks (generated from entity_types.h) @@ -73,23 +89,34 @@ class ComponentIterator { #endif MAX, }; + /// End the current try_advance() pass after this step; lets callbacks + /// that write directly to the socket cap direct writes per pass. + void yield_after_step_() { this->yield_requested_ = true; } + uint16_t at_{0}; // Supports up to 65,535 entities per type IteratorState state_{IteratorState::NONE}; - bool include_internal_{false}; + bool yield_requested_ : 1 {false}; + bool include_internal_ : 1 {false}; template - void process_platform_item_(const Container &items, + bool process_platform_item_(const Container &items, bool (ComponentIterator::*on_item)(typename Container::value_type)) { if (this->at_ >= items.size()) { this->advance_platform_(); - } else { - typename Container::value_type item = items[this->at_]; - if ((item->is_internal() && !this->include_internal_) || (this->*on_item)(item)) { - this->at_++; - } + return true; } + typename Container::value_type item = items[this->at_]; + if ((item->is_internal() && !this->include_internal_) || (this->*on_item)(item)) { + this->at_++; + return true; + } + return false; } + /// One iteration step; false if no progress was made (callback refused + /// or iterator not running). + bool advance_step_(); + void advance_platform_(); }; diff --git a/tests/components/camera/__init__.py b/tests/components/camera/__init__.py new file mode 100644 index 0000000000..61104b0bb1 --- /dev/null +++ b/tests/components/camera/__init__.py @@ -0,0 +1,11 @@ +import esphome.codegen as cg +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # No host camera platform exists to emit USE_CAMERA; define it here so + # the iterator CAMERA state compiles into the test binary. + async def to_code_testing(config): + cg.add_define("USE_CAMERA") + + manifest.to_code = to_code_testing diff --git a/tests/components/camera/test_component_iterator_camera.cpp b/tests/components/camera/test_component_iterator_camera.cpp new file mode 100644 index 0000000000..efd8261554 --- /dev/null +++ b/tests/components/camera/test_component_iterator_camera.cpp @@ -0,0 +1,79 @@ +#include + +#include "esphome/core/component_iterator.h" + +#ifdef USE_CAMERA +#include "esphome/components/camera/camera.h" + +namespace esphome::testing { + +class StubCamera : public camera::Camera { + public: + void add_listener(camera::CameraListener *listener) override {} + camera::CameraImageReader *create_image_reader() override { return nullptr; } + void request_image(camera::CameraRequester requester) override {} + void start_stream(camera::CameraRequester requester) override {} + void stop_stream(camera::CameraRequester requester) override {} +}; + +// Iterator that accepts everything except the camera, which can refuse a +// configurable number of times. The CAMERA state is a singleton path +// distinct from process_platform_item_; this pins the same contract: +// a refused camera is re-offered, never skipped. +class CameraRefusingIterator : public ComponentIterator { + public: +// NOLINTBEGIN(bugprone-macro-parentheses) +#define ENTITY_TYPE_(type, singular, plural, count, upper) \ + bool on_##singular(type *obj) override { return true; } +#define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ + ENTITY_TYPE_(type, singular, plural, count, upper) +#include "esphome/core/entity_types.h" +#undef ENTITY_TYPE_ +#undef ENTITY_CONTROLLER_TYPE_ + // NOLINTEND(bugprone-macro-parentheses) + + bool on_camera(camera::Camera *obj) override { + this->camera_calls++; + if (this->camera_refusals > 0) { + this->camera_refusals--; + return false; + } + return true; + } + + int camera_calls{0}; + int camera_refusals{0}; +}; + +// Far above the fixed number of iterator states +static constexpr size_t BIG_BUDGET = 1000; + +class ComponentIteratorCameraTest : public ::testing::Test { + protected: + void SetUp() override { + // Constructing a Camera installs the process-wide singleton + static StubCamera stub_camera; + ASSERT_EQ(camera::Camera::instance(), &stub_camera); + } +}; + +TEST_F(ComponentIteratorCameraTest, RefusedCameraIsReofferedNotSkipped) { + CameraRefusingIterator it; + it.camera_refusals = 2; + it.begin(); + // Runs until the camera refuses, which stops the pass + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.camera_calls, 1); + EXPECT_FALSE(it.completed()); + // The camera is re-offered once per call, not skipped + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.camera_calls, 2); + EXPECT_FALSE(it.completed()); + // Once accepted, the iteration completes + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.camera_calls, 3); +} + +} // namespace esphome::testing +#endif // USE_CAMERA diff --git a/tests/components/core/benchmark.yaml b/tests/components/core/benchmark.yaml new file mode 100644 index 0000000000..063ac41eff --- /dev/null +++ b/tests/components/core/benchmark.yaml @@ -0,0 +1,11 @@ +# Pulls in sensor so entity iteration paths compile (USE_SENSOR); +# tests register their own instances. Plain yaml.safe_load, no ESPHome tags. +# An alphabetically-earlier component's sensor: block shadows this one in +# combined builds; the tests' sensor-count ASSERT catches a capacity drop. +sensor: + - platform: template + id: bench_sensor_a + name: "Bench A" + - platform: template + id: bench_sensor_b + name: "Bench B" diff --git a/tests/components/core/test_component_iterator.cpp b/tests/components/core/test_component_iterator.cpp new file mode 100644 index 0000000000..03d467c920 --- /dev/null +++ b/tests/components/core/test_component_iterator.cpp @@ -0,0 +1,195 @@ +#include + +#include "esphome/core/component_iterator.h" + +#ifdef USE_SENSOR +#include "esphome/components/sensor/sensor.h" +#include "esphome/core/application.h" +#endif + +namespace esphome::testing { + +// Iterator whose begin/end callbacks can refuse a configurable number of +// times; all entity callbacks accept (any registered entities are accepted). +class RefusingIterator : public ComponentIterator { + public: +// NOLINTBEGIN(bugprone-macro-parentheses) +#define ENTITY_TYPE_(type, singular, plural, count, upper) \ + bool on_##singular(type *obj) override { return true; } +#define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ + ENTITY_TYPE_(type, singular, plural, count, upper) +#include "esphome/core/entity_types.h" +#undef ENTITY_TYPE_ +#undef ENTITY_CONTROLLER_TYPE_ + // NOLINTEND(bugprone-macro-parentheses) + + bool on_begin() override { return step(this->begin_calls, this->begin_refusals); } + bool on_end() override { return step(this->end_calls, this->end_refusals); } + + int begin_calls{0}; + int end_calls{0}; + int begin_refusals{0}; + int end_refusals{0}; + + protected: + static bool step(int &calls, int &refusals) { + calls++; + if (refusals > 0) { + refusals--; + return false; + } + return true; + } +}; + +// Far above the fixed number of iterator states +static constexpr size_t BIG_BUDGET = 1000; + +TEST(ComponentIterator, NotRunningMakesNoProgress) { + RefusingIterator it; + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 0); + EXPECT_EQ(it.end_calls, 0); +} + +TEST(ComponentIterator, CompletesInOneCallWithoutRefusals) { + RefusingIterator it; + it.begin(); + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 1); + EXPECT_EQ(it.end_calls, 1); +} + +TEST(ComponentIterator, StepBudgetIsHonored) { + RefusingIterator it; + it.begin(); + it.try_advance(1); + EXPECT_EQ(it.begin_calls, 1); + EXPECT_EQ(it.end_calls, 0); + EXPECT_FALSE(it.completed()); +} + +TEST(ComponentIterator, RefusedStepStopsBatchAndRetriesSameStep) { + RefusingIterator it; + it.end_refusals = 3; + it.begin(); + // First call runs until the refused end step, which stops the pass + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.end_calls, 1); + EXPECT_FALSE(it.completed()); + // The refused step is retried once per call, not skipped + it.try_advance(BIG_BUDGET); + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.end_calls, 3); + EXPECT_FALSE(it.completed()); + // Once accepted, the iteration completes + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.end_calls, 4); +} + +TEST(ComponentIterator, RefusedBeginStopsBatchAndRetries) { + RefusingIterator it; + it.begin_refusals = 2; + it.begin(); + it.try_advance(BIG_BUDGET); + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.begin_calls, 2); + EXPECT_FALSE(it.completed()); + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 3); +} + +// The deprecated advance() wrapper must keep the legacy once-per-loop +// pattern working during the deprecation window. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +TEST(ComponentIterator, DeprecatedAdvanceKeepsLegacyPatternWorking) { + RefusingIterator it; + it.end_refusals = 2; + it.begin(); + size_t guard = 0; + while (!it.completed() && guard++ < BIG_BUDGET) { + it.advance(); + } + EXPECT_TRUE(it.completed()); + // Two refused end steps were retried, then accepted + EXPECT_EQ(it.end_calls, 3); +} +#pragma GCC diagnostic pop + +#ifdef USE_SENSOR +// Iterator whose sensor callback can refuse or yield; pins the per-item +// contract: a refused item is re-offered with at_ unchanged, never skipped. +class ItemRefusingIterator : public RefusingIterator { + public: + bool on_sensor(sensor::Sensor *obj) override { + this->last_sensor = obj; + if (!step(this->sensor_calls, this->sensor_refusals)) + return false; + if (this->yield_on_sensor) + this->yield_after_step_(); + return true; + } + sensor::Sensor *last_sensor{nullptr}; + int sensor_calls{0}; + int sensor_refusals{0}; + bool yield_on_sensor{false}; +}; + +class ComponentIteratorSensorTest : public ::testing::Test { + protected: + void SetUp() override { + static sensor::Sensor sensor_a; + static sensor::Sensor sensor_b; + static bool registered = false; + if (!registered) { + App.register_sensor(&sensor_a); + App.register_sensor(&sensor_b); + registered = true; + } + // StaticVector drops silently when full; fail the fixture, not the contract + ASSERT_EQ(App.get_sensors().size(), 2u) << "benchmark.yaml sensor count too small"; + } +}; + +TEST_F(ComponentIteratorSensorTest, RefusedItemIsReofferedNotSkipped) { + ItemRefusingIterator it; + it.sensor_refusals = 2; + it.begin(); + // Runs until the first sensor refuses + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 1); + EXPECT_FALSE(it.completed()); + // The refused item is re-offered, not skipped + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 2); + sensor::Sensor *refused = it.last_sensor; + // Once accepted, iteration continues through the second sensor to the end + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_NE(it.last_sensor, refused); + EXPECT_EQ(it.sensor_calls, 4); +} + +TEST_F(ComponentIteratorSensorTest, YieldAfterStepEndsPassAndResumes) { + ItemRefusingIterator it; + it.yield_on_sensor = true; + it.begin(); + // The pass ends right after the first sensor despite a big budget + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 1); + EXPECT_FALSE(it.completed()); + // The next pass ends after the second sensor + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 2); + // Remaining states then run to completion in one pass + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); +} +#endif // USE_SENSOR + +} // namespace esphome::testing diff --git a/tests/integration/README.md b/tests/integration/README.md index 44d9e0d644..790d9a3a11 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -7,6 +7,7 @@ This directory contains end-to-end integration tests for ESPHome, focusing on te - `conftest.py` - Common fixtures and utilities - `const.py` - Constants used throughout the integration tests - `types.py` - Type definitions for fixtures and functions +- `raw_api_client.py` - Minimal plaintext api client whose reads happen only on request (for backpressure tests) - `state_utils.py` - State handling utilities (e.g., `InitialStateHelper`, `find_entity`, `require_entity`) - `fixtures/` - YAML configuration files for tests - `test_*.py` - Individual test files @@ -347,6 +348,7 @@ Create C++ components in `fixtures/external_components/` for: - Custom entity behaviors - Scheduler testing - Memory management tests +- Deterministic network backpressure (`sndbuf_pin_component` pins socket send buffers; assert on its log line to prove the pin took effect) ##### Log Line Monitoring ```python diff --git a/tests/integration/fixtures/api_list_entities_backpressure.yaml b/tests/integration/fixtures/api_list_entities_backpressure.yaml new file mode 100644 index 0000000000..1e53af7b75 --- /dev/null +++ b/tests/integration/fixtures/api_list_entities_backpressure.yaml @@ -0,0 +1,23 @@ +esphome: + name: api-backpressure-test + +host: + +api: + # Smallest queue so a non-draining client blocks the send path quickly + max_send_queue: 1 + actions: +# GENERATED_ACTIONS + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + components: [sndbuf_pin_component] + +# Pins the device's socket send buffers for deterministic TCP backpressure +sndbuf_pin_component: + buffer_size: SERVER_SNDBUF + +logger: + level: DEBUG diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py b/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py new file mode 100644 index 0000000000..06dda5d1ac --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py @@ -0,0 +1,20 @@ +import esphome.codegen as cg +import esphome.config_validation as cv +from esphome.const import CONF_BUFFER_SIZE, CONF_ID + +DEPENDENCIES = ["api"] + +sndbuf_pin_ns = cg.esphome_ns.namespace("sndbuf_pin") +SndbufPinComponent = sndbuf_pin_ns.class_("SndbufPinComponent", cg.Component) + +CONFIG_SCHEMA = cv.Schema( + { + cv.GenerateID(): cv.declare_id(SndbufPinComponent), + cv.Required(CONF_BUFFER_SIZE): cv.int_range(min=1), + } +).extend(cv.COMPONENT_SCHEMA) + + +async def to_code(config): + var = cg.new_Pvariable(config[CONF_ID], config[CONF_BUFFER_SIZE]) + await cg.register_component(var, config) diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp new file mode 100644 index 0000000000..430c5075c7 --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp @@ -0,0 +1,55 @@ +#include "sndbuf_pin_component.h" + +#include +#include +#include + +#include "esphome/components/api/api_server.h" +#include "esphome/core/log.h" + +namespace esphome::sndbuf_pin { + +static const char *const TAG = "sndbuf_pin"; + +// Skip stdio; scan the low fd range where the listeners land +static constexpr int FIRST_USER_FD = 3; +static constexpr int MAX_FD_SCAN = 128; + +void SndbufPinComponent::setup() { + int pinned = 0; + for (int fd = FIRST_USER_FD; fd < MAX_FD_SCAN; fd++) { + int type = 0; + socklen_t len = sizeof(type); + if (::getsockopt(fd, SOL_SOCKET, SO_TYPE, &type, &len) != 0 || type != SOCK_STREAM) + continue; + struct sockaddr_in addr {}; + socklen_t addr_len = sizeof(addr); + if (::getsockname(fd, reinterpret_cast(&addr), &addr_len) != 0) { + ESP_LOGW(TAG, "fd %d: getsockname failed, errno %d", fd, errno); + continue; + } + if (ntohs(addr.sin_port) != api::global_api_server->get_port()) + continue; + if (::setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &this->buffer_size_, sizeof(this->buffer_size_)) != 0) { + ESP_LOGW(TAG, "fd %d: SO_SNDBUF pin failed, errno %d", fd, errno); + continue; + } + int applied = 0; + len = sizeof(applied); + if (::getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &applied, &len) != 0 || applied < this->buffer_size_) { + // Linux doubles the requested value; anything below it means clamped + ESP_LOGW(TAG, "fd %d: SO_SNDBUF readback %d below requested %d", fd, applied, this->buffer_size_); + continue; + } + // Tests assert on this line; accepted sockets inherit the pinned size + ESP_LOGD(TAG, "fd %d port %d: SO_SNDBUF pinned to %d (effective %d)", fd, ntohs(addr.sin_port), this->buffer_size_, + applied); + pinned++; + } + if (pinned == 0) { + ESP_LOGE(TAG, "api listener socket was not pinned"); + this->mark_failed(); + } +} + +} // namespace esphome::sndbuf_pin diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h new file mode 100644 index 0000000000..b0af226d14 --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h @@ -0,0 +1,21 @@ +#pragma once + +#include "esphome/core/component.h" + +namespace esphome::sndbuf_pin { + +// Test-only (host): pins SO_SNDBUF on every open TCP socket so integration +// tests get deterministic backpressure; an explicit SO_SNDBUF also disables +// kernel autotuning, and accepted sockets inherit it from the listener. +class SndbufPinComponent : public Component { + public: + explicit SndbufPinComponent(int buffer_size) : buffer_size_(buffer_size) {} + void setup() override; + // After the api server so its listening socket exists + float get_setup_priority() const override { return setup_priority::LATE; } + + protected: + int buffer_size_; +}; + +} // namespace esphome::sndbuf_pin diff --git a/tests/integration/raw_api_client.py b/tests/integration/raw_api_client.py new file mode 100644 index 0000000000..1dbe40933c --- /dev/null +++ b/tests/integration/raw_api_client.py @@ -0,0 +1,148 @@ +"""Minimal plaintext native-api client over a raw socket. + +Reads only when told to, so tests control when the TCP pipe backs up toward +the device; payloads are skipped and only message types are counted. +""" + +from __future__ import annotations + +import asyncio +from collections import Counter +import socket +from typing import Self + +from aioesphomeapi import api_pb2 +import aioesphomeapi.core as api_core +from google.protobuf import message + +from .const import LOCALHOST + +# Message type ids are protocol constants; derive them from aioesphomeapi so +# they cannot drift from the client library in use. +MESSAGE_TYPE_OF = {cls: num for num, cls in api_core.MESSAGE_TYPE_TO_PROTO.items()} + +_READ_CHUNK = 4096 + + +def encode_varint(value: int) -> bytes: + out = bytearray() + while True: + byte = value & 0x7F + value >>= 7 + if value: + out.append(byte | 0x80) + else: + out.append(byte) + return bytes(out) + + +def decode_varint(buf: bytearray, pos: int) -> tuple[int, int] | None: + """Decode one varint at pos; return (value, new_pos) or None if short.""" + value = shift = 0 + while pos < len(buf): + byte = buf[pos] + pos += 1 + value |= (byte & 0x7F) << shift + if not byte & 0x80: + return value, pos + shift += 7 + return None + + +def encode_frame(msg_type: int, payload: bytes) -> bytes: + """Encode one plaintext api frame: 0x00, payload length, message type.""" + return b"\x00" + encode_varint(len(payload)) + encode_varint(msg_type) + payload + + +class FrameParser: + """Incremental parser for the plaintext api frame stream.""" + + def __init__(self) -> None: + self._buf = bytearray() + + def feed(self, data: bytes) -> list[int]: + self._buf.extend(data) + types: list[int] = [] + while (msg_type := self._try_parse()) is not None: + types.append(msg_type) + return types + + def _try_parse(self) -> int | None: + buf = self._buf + if not buf: + return None + assert buf[0] == 0, f"expected plaintext frame, got indicator {buf[0]}" + if (size_decoded := decode_varint(buf, 1)) is None: + return None + size, pos = size_decoded + if (type_decoded := decode_varint(buf, pos)) is None: + return None + msg_type, pos = type_decoded + if len(buf) - pos < size: + return None + del buf[: pos + size] + return msg_type + + +class RawApiClient: + """Plaintext api client whose reads happen only on request.""" + + def __init__(self, port: int, recv_buffer_size: int | None = None) -> None: + self._port = port + self._parser = FrameParser() + self.bytes_received = 0 + self.frame_counts: Counter[int] = Counter() + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + if recv_buffer_size is not None: + sock.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, recv_buffer_size) + # Kernels may round up (Linux doubles) but must not clamp below + applied = sock.getsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF) + assert applied >= recv_buffer_size, ( + f"SO_RCVBUF clamped to {applied}, requested {recv_buffer_size}" + ) + sock.setblocking(False) + except Exception: + sock.close() + raise + self._sock = sock + + async def __aenter__(self) -> Self: + return self + + async def __aexit__(self, *exc_info: object) -> None: + self.close() + + async def connect(self, client_info: str = "raw-api-client") -> None: + """Connect and complete the Hello handshake (no auth step since 2026.1.0).""" + loop = asyncio.get_running_loop() + await loop.sock_connect(self._sock, (LOCALHOST, self._port)) + hello = api_pb2.HelloRequest() + hello.client_info = client_info + hello.api_version_major = 1 + hello.api_version_minor = 10 + await self.send_message(hello) + await self.read_until_frame(MESSAGE_TYPE_OF[api_pb2.HelloResponse]) + + async def send_message(self, msg: message.Message) -> None: + loop = asyncio.get_running_loop() + await loop.sock_sendall( + self._sock, + encode_frame(MESSAGE_TYPE_OF[type(msg)], msg.SerializeToString()), + ) + + async def read_until_frame(self, msg_type: int, timeout: float = 10.0) -> None: + """Read until at least one frame of msg_type has been received.""" + loop = asyncio.get_running_loop() + + async def _read_loop() -> None: + while not self.frame_counts[msg_type]: + data = await loop.sock_recv(self._sock, _READ_CHUNK) + assert data, "server closed the connection unexpectedly" + self.bytes_received += len(data) + self.frame_counts.update(self._parser.feed(data)) + + await asyncio.wait_for(_read_loop(), timeout) + + def close(self) -> None: + self._sock.close() diff --git a/tests/integration/test_api_list_entities_backpressure.py b/tests/integration/test_api_list_entities_backpressure.py new file mode 100644 index 0000000000..9df6c7a298 --- /dev/null +++ b/tests/integration/test_api_list_entities_backpressure.py @@ -0,0 +1,110 @@ +"""A client that stops reading the entity listing must not starve other clients. + +Service responses are sent directly (not via the deferred batch), so a full +TCP pipe makes the send path refuse; the drive loop now lives in +try_advance(), which stops on refusal instead of retrying forever. Not a +before/after regression test: pre-fix builds survive here because the +refusal path yields and pumps the socket each retry. + +The sndbuf_pin_component fixture pins the device's send buffers so the pipe +fills deterministically regardless of kernel autotuning; the test waits for +its log line before proceeding. +""" + +from __future__ import annotations + +import asyncio + +from aioesphomeapi import api_pb2 +import pytest + +from .raw_api_client import MESSAGE_TYPE_OF, RawApiClient +from .types import APIClientConnectedFactory, RunCompiledFunction + +SERVICES_RESPONSE = MESSAGE_TYPE_OF[api_pb2.ListEntitiesServicesResponse] +LIST_DONE_RESPONSE = MESSAGE_TYPE_OF[api_pb2.ListEntitiesDoneResponse] + +# Both ends of the pipe are pinned small; only tens of KB fit in the kernel +RECV_BUFFER_SIZE = 4096 +SERVER_SNDBUF = 8192 # substituted into the fixture yaml +# Logged by the sndbuf_pin_component fixture when it pins a socket +SNDBUF_PIN_LOG = "SO_SNDBUF pinned to" +# One response (~6.4 KB) must stay smaller than the pinned send buffer; an +# oversized message parks in the overflow buffer and reports as sent. +ARGS_PER_SERVICE = 8 +ARG_NAME_LEN = 800 +# ~160 KB listing versus a tens-of-KB pipe guarantees a mid-services block +NUM_SERVICES = 25 +assert ARGS_PER_SERVICE * ARG_NAME_LEN < SERVER_SNDBUF +# The pipe fills in well under a second +STALL_SECONDS = 0.5 +# Well above pipe capacity, well below the listing size +MIN_DRAINED_BYTES = 60_000 + + +def _generated_actions() -> str: + """Build the api actions block: services with long argument names.""" + lines: list[str] = [] + for i in range(NUM_SERVICES): + lines.append(f" - action: backpressure_service_{i:04d}") + lines.append(" variables:") + for j in range(ARGS_PER_SERVICE): + prefix = f"arg_{i:04d}_{j:02d}_" + lines.append( + f" {prefix}{'x' * (ARG_NAME_LEN - len(prefix))}: string" + ) + lines.append(" then:") + lines.append(" - logger.log: service called") + return "\n".join(lines) + + +@pytest.mark.asyncio +async def test_api_list_entities_backpressure( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, + unused_tcp_port: int, +) -> None: + """A stalled reader mid-services must not block other api clients.""" + assert "# GENERATED_ACTIONS" in yaml_config + config = yaml_config.replace("# GENERATED_ACTIONS", _generated_actions()) + config = config.replace("SERVER_SNDBUF", str(SERVER_SNDBUF)) + + pin_applied = asyncio.Event() + + def _on_log_line(line: str) -> None: + if SNDBUF_PIN_LOG in line: + pin_applied.set() + + async with run_compiled(config, line_callback=_on_log_line): + # Fails loudly if the pin never applied + await asyncio.wait_for(pin_applied.wait(), 10) + + async with RawApiClient( + unused_tcp_port, recv_buffer_size=RECV_BUFFER_SIZE + ) as stalled: + await stalled.connect(client_info="backpressure-stall-client") + await stalled.send_message(api_pb2.ListEntitiesRequest()) + # The client now stops reading entirely. + + # Let the server run against the full pipe + await asyncio.sleep(STALL_SECONDS) + + # Other clients must still be served while the first is blocked + async with api_client_connected(timeout=20) as client: + device_info = await asyncio.wait_for(client.device_info(), 20) + assert device_info.name == "api-backpressure-test" + _, services = await asyncio.wait_for( + client.list_entities_services(), 30 + ) + assert len(services) == NUM_SERVICES + + # Fixture-size guard: the listing must dwarf the pinned pipe + before = stalled.bytes_received + await stalled.read_until_frame(LIST_DONE_RESPONSE, timeout=60) + drained = stalled.bytes_received - before + assert drained > MIN_DRAINED_BYTES, ( + f"only {drained} bytes drained; the listing never backed up" + ) + assert stalled.frame_counts[SERVICES_RESPONSE] == NUM_SERVICES + assert stalled.frame_counts[LIST_DONE_RESPONSE] == 1 From 98f138f6b2b7b1cacd35e40e9a83f13cfdf6d739 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 16:16:01 -0500 Subject: [PATCH 5/6] [esp32_ble_tracker] Scan at the default window while a GATT connection is active (#18609) --- .../components/ble_device_base/__init__.py | 43 +++++--- .../components/esp32_ble_tracker/__init__.py | 47 +++++++- .../esp32_ble_tracker/esp32_ble_tracker.cpp | 73 ++++++++++--- .../esp32_ble_tracker/esp32_ble_tracker.h | 36 ++++-- .../config/scan_window_explicit.yaml | 19 ++++ .../config/scan_window_raised.yaml | 17 +++ .../config/scan_window_scan_only.yaml | 12 ++ .../scan_window_user_set_scan_only.yaml | 14 +++ .../test_scan_window_default.py | 103 +++++++++++++++++- 9 files changed, 320 insertions(+), 44 deletions(-) create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml diff --git a/esphome/components/ble_device_base/__init__.py b/esphome/components/ble_device_base/__init__.py index 15a8b08139..43ec736727 100644 --- a/esphome/components/ble_device_base/__init__.py +++ b/esphome/components/ble_device_base/__init__.py @@ -206,32 +206,36 @@ def validate_scan_parameters(config: ConfigType) -> ConfigType: interval = config[CONF_INTERVAL] window = config[CONF_WINDOW] - if window > interval: - raise cv.Invalid( - f"Scan window ({window}) needs to be smaller than scan interval ({interval})" - ) + # Labels are reused in every error below; the optional one names its key. + windows = [("Scan window", window)] + if (connection_window := config.get(CONF_CONNECTION_SCAN_WINDOW)) is not None: + windows.append((CONF_CONNECTION_SCAN_WINDOW, connection_window)) + + for name, value in windows: + if value > interval: + raise cv.Invalid( + f"{name} ({value}) needs to be smaller than scan interval ({interval})" + ) # BLE scan interval/window are programmed in 0.625 ms units as a 16-bit value; the # controller only accepts 2.5 ms .. 10240 ms (0x0004 .. 0x4000). Reject out-of-range # values here instead of letting the unit conversion silently overflow. - for name, value in (("interval", interval), ("window", window)): + for name, value in (("Scan interval", interval), *windows): if value.total_microseconds < 2500 or value.total_microseconds > 10_240_000: - raise cv.Invalid( - f"Scan {name} ({value}) must be between 2.5 ms and 10240 ms" - ) + raise cv.Invalid(f"{name} ({value}) must be between 2.5 ms and 10240 ms") # Validate what actually reaches the controller: both values are truncated to # whole 0.625 ms units, so a window/interval pair that differs by less than one # unit collapses to the same value — silently programming a 100 % duty cycle # (radio permanently on) from a config that asked for less. interval_units = to_ble_units(interval) - window_units = to_ble_units(window) - if window_units == interval_units and window < interval: - raise cv.Invalid( - f"Scan window ({window}) and interval ({interval}) both truncate to " - f"{interval_units} x 0.625 ms, which the controller scans at a 100 % duty " - f"cycle. Separate them by at least 0.625 ms." - ) + for name, value in windows: + if to_ble_units(value) == interval_units and value < interval: + raise cv.Invalid( + f"{name} ({value}) and interval ({interval}) both truncate to " + f"{interval_units} x 0.625 ms, which the controller scans at a 100 % duty " + f"cycle. Separate them by at least 0.625 ms." + ) if interval.total_microseconds * 3 > duration.total_microseconds: raise cv.Invalid( @@ -247,11 +251,14 @@ def validate_scan_parameters(config: ConfigType) -> ConfigType: # their own; also the fallback for esp32's conditional default. DEFAULT_SCAN_WINDOW = "30ms" +CONF_CONNECTION_SCAN_WINDOW = "connection_scan_window" + def scan_parameters_schema( interval_default: str, *, window_default: str | Callable[[], TimePeriod] = DEFAULT_SCAN_WINDOW, + connection_window: bool = False, ) -> cv.All: """Build the scan_parameters value schema shared by all BLE trackers. @@ -263,7 +270,9 @@ def scan_parameters_schema( can adjust it once sibling keys are resolved). The `active` option (default on) is unconditional: active scanning is part of the tracker contract — every current proxy client assumes it, so a passive-only - tracker must not share this schema. + tracker must not share this schema. connection_window opts in to the + `connection_scan_window` option for trackers that can fall back to a + smaller window while a GATT connection is active. """ schema = { cv.Optional(CONF_DURATION, default="5min"): cv.positive_time_period_seconds, @@ -272,6 +281,8 @@ def scan_parameters_schema( cv.Optional(CONF_CONTINUOUS, default=True): cv.boolean, cv.Optional(CONF_ACTIVE, default=True): cv.boolean, } + if connection_window: + schema[cv.Optional(CONF_CONNECTION_SCAN_WINDOW)] = cv.positive_time_period return cv.All(cv.Schema(schema), validate_scan_parameters) diff --git a/esphome/components/esp32_ble_tracker/__init__.py b/esphome/components/esp32_ble_tracker/__init__.py index 4f6355df70..c6e34f37ca 100644 --- a/esphome/components/esp32_ble_tracker/__init__.py +++ b/esphome/components/esp32_ble_tracker/__init__.py @@ -7,6 +7,7 @@ import logging from esphome import automation import esphome.codegen as cg from esphome.components import ble_device_base, esp32_ble, ota +from esphome.components.ble_device_base import CONF_CONNECTION_SCAN_WINDOW from esphome.components.const import CONF_ON_SCAN_END, CONF_SCAN_PARAMETERS, CONF_WINDOW from esphome.components.esp32 import ( add_idf_sdkconfig_option, @@ -73,8 +74,9 @@ def _get_required_features() -> set[BLEFeatures]: # Slot counters sizing the tracker's StaticVector storage; one request per # registered listener or client. +CLIENT_COUNT_DEFINE = "ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT" _request_listener_slot = cg.slot_counter("ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT") -_request_client_slot = cg.slot_counter("ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT") +_request_client_slot = cg.slot_counter(CLIENT_COUNT_DEFINE) def register_ble_features(features: set[BLEFeatures]) -> None: @@ -147,6 +149,7 @@ class TrackerData: """Per-run validation state, namespaced under DOMAIN in CORE.data.""" scan_window_defaulted: bool = False + connection_window_injected: bool = False def _get_data() -> TrackerData: @@ -175,17 +178,34 @@ def _raise_defaulted_scan_window(config: ConfigType) -> ConfigType: honors the window strictly (>= 5.5.5); without the arbiter a full-duty scan would starve wifi outright, and a user-set window is never touched. Raising to the interval cannot invalidate the already-validated - parameters, so no re-validation is needed. + parameters, so no re-validation is needed. The connection window is + checked against the window here, after the raise. """ + params = config[CONF_SCAN_PARAMETERS] if ( _get_data().scan_window_defaulted and config.get(CONF_SOFTWARE_COEXISTENCE) and idf_version() >= IDF_SCAN_WINDOW_FIX_VERSION ): - params = config[CONF_SCAN_PARAMETERS] # Copy so the config dump shows a plain value instead of a YAML # anchor/alias pair pointing at the interval. params[CONF_WINDOW] = copy.copy(params[CONF_INTERVAL]) + # Arm the connection-time fallback unless the user set one. Injected + # after validation; safe because it equals the validated window default. + if CONF_CONNECTION_SCAN_WINDOW not in params: + params[CONF_CONNECTION_SCAN_WINDOW] = cv.positive_time_period( + ble_device_base.DEFAULT_SCAN_WINDOW + ) + _get_data().connection_window_injected = True + if ( + connection_window := params.get(CONF_CONNECTION_SCAN_WINDOW) + ) is not None and connection_window > params[CONF_WINDOW]: + # A larger value would widen the scan during connections. + raise cv.Invalid( + f"{CONF_CONNECTION_SCAN_WINDOW} ({connection_window}) needs to be " + f"smaller than the scan window ({params[CONF_WINDOW]})", + path=[CONF_SCAN_PARAMETERS, CONF_CONNECTION_SCAN_WINDOW], + ) return config @@ -194,7 +214,7 @@ def _raise_defaulted_scan_window(config: ConfigType) -> ConfigType: # window/interval pairs that collapse to the same 0.625 ms unit count. # The window default is conditional (see _scan_window_default above). SCAN_PARAMETERS_SCHEMA = ble_device_base.scan_parameters_schema( - "320ms", window_default=_scan_window_default + "320ms", window_default=_scan_window_default, connection_window=True ) # Codegen helpers are owned by ble_device_base; kept under the historical names @@ -288,6 +308,25 @@ async def to_code(config: ConfigType) -> None: cg.add(var.set_scan_duration(params[CONF_DURATION])) cg.add(var.set_scan_interval(ble_device_base.to_ble_units(params[CONF_INTERVAL]))) cg.add(var.set_scan_window(ble_device_base.to_ble_units(params[CONF_WINDOW]))) + if (connection_window := params.get(CONF_CONNECTION_SCAN_WINDOW)) is not None: + # Emitted at FINAL so a scan-only build, where the guarded C++ path + # compiles out, skips the call entirely. + window_units = ble_device_base.to_ble_units(connection_window) + + @coroutine_with_priority(CoroPriority.FINAL) + async def _emit_connection_scan_window() -> None: + if cg.get_slot_count(CLIENT_COUNT_DEFINE): + cg.add(var.set_connection_scan_window(window_units)) + elif not _get_data().connection_window_injected: + # Warn only for a user-set value; the injected default drops silently. + _LOGGER.warning( + "'%s' has no effect because this build has no BLE client " + "components (for example bluetooth_proxy with active " + "connections, or ble_client)", + CONF_CONNECTION_SCAN_WINDOW, + ) + + CORE.add_job(_emit_connection_scan_window) cg.add(var.set_scan_active(params[CONF_ACTIVE])) cg.add(var.set_scan_continuous(params[CONF_CONTINUOUS])) diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp index 798fd6e0ca..5339565a32 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp @@ -122,6 +122,9 @@ void ESP32BLETracker::loop() { // - start_scan_(): scanner_state_ becomes IDLE via set_scanner_state_() in cleanup_scan_state_() // - try_promote_discovered_clients_(): client enters DISCOVERED via set_state(), or // connecting client finishes (state change), or scanner reaches RUNNING/IDLE + // - connection-window restart: scan_params_ is only written in start_scan_() + // (which changes scanner state via set_scanner_state_()), and + // counts.active/disconnecting only change on client state changes // // All conditions that affect the logic below are tied to state changes that increment // state_version_, so the fast path is safe. @@ -144,6 +147,19 @@ void ESP32BLETracker::loop() { (this->scan_set_param_failed_ && this->scanner_state_ == ScannerState::RUNNING)) { this->handle_scanner_failure_(); } + +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // The programmed window no longer matches the connection state (typically + // the last connection dropped): restart so the right window applies now + // instead of at the end of the scan period. Continuous only (a user-started + // scan would not restart); !disconnecting matches the restart gate below. + if (this->scanner_state_ == ScannerState::RUNNING && this->scan_continuous_ && !counts.disconnecting && + this->scan_params_.scan_window != this->desired_scan_window_(counts.active)) { + // Same logical scan period continues: no on_scan_end sweeps for this + // restart. Only armed when the stop was issued. + this->skip_next_scan_end_ = this->stop_scan_(); + } +#endif /* Avoid starting the scanner if: @@ -195,19 +211,23 @@ void ESP32BLETracker::stop_scan() { // reason at D themselves, and the user-facing stop action is deliberate. ESP_LOGV(TAG, "Stopping scan."); this->scan_continuous_ = false; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // The window-change restart is abandoned with continuous scanning. + this->skip_next_scan_end_ = false; +#endif this->stop_scan_(); } void ESP32BLETracker::ble_before_disabled_event_handler() { this->stop_scan_(); } -void ESP32BLETracker::stop_scan_() { +bool ESP32BLETracker::stop_scan_() { if (this->scanner_state_ != ScannerState::RUNNING && this->scanner_state_ != ScannerState::FAILED) { // IDLE means there is nothing to stop; STOPPING means a stop is already in // flight and will finish on its own. Neither is an error. if (this->scanner_state_ != ScannerState::IDLE && this->scanner_state_ != ScannerState::STOPPING) { ESP_LOGE(TAG, "Cannot stop scan: %s", this->scanner_state_to_string_(this->scanner_state_)); } - return; + return false; } // Reset timeout state machine when stopping scan this->scan_timeout_state_ = ScanTimeoutState::INACTIVE; @@ -215,8 +235,9 @@ void ESP32BLETracker::stop_scan_() { esp_err_t err = esp_ble_gap_stop_scanning(); if (err != ESP_OK) { ESP_LOGE(TAG, "esp_ble_gap_stop_scanning failed: %d", err); - return; + return false; } + return true; } void ESP32BLETracker::start_scan_(bool first) { @@ -230,16 +251,11 @@ void ESP32BLETracker::start_scan_(bool first) { } this->set_scanner_state_(ScannerState::STARTING); ESP_LOGV(TAG, "Starting scan, set scanner state to STARTING."); - if (!first) { -#ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT - for (auto *listener : this->listeners_) - listener->on_scan_end(); + if (!first) + this->notify_scan_end_(); +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + this->skip_next_scan_end_ = false; #endif -#ifdef ESPHOME_BLE_DEVICE_BASE_LISTENER_COUNT - for (auto *listener : this->neutral_listeners_) - listener->on_scan_end(); -#endif - } #ifdef USE_ESP32_BLE_DEVICE this->discovered_log_.clear(); #endif @@ -247,7 +263,17 @@ void ESP32BLETracker::start_scan_(bool first) { this->scan_params_.own_addr_type = BLE_ADDR_TYPE_PUBLIC; this->scan_params_.scan_filter_policy = BLE_SCAN_FILTER_ALLOW_ALL; this->scan_params_.scan_interval = this->scan_interval_; - this->scan_params_.scan_window = this->scan_window_; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // Count fresh: an automation can start a scan before loop() refreshes the counts. + const uint32_t window = this->desired_scan_window_(this->count_client_states_().active); + if (window != this->scan_window_) { + // Guarantee the connection airtime instead of scanning wall to wall. + ESP_LOGV(TAG, "Connection active, using %" PRIu32 " unit scan window", window); + } +#else + const uint32_t window = this->scan_window_; +#endif + this->scan_params_.scan_window = window; // Start timeout monitoring in loop() instead of using scheduler // This prevents false reboots when the loop is blocked @@ -408,6 +434,11 @@ void ESP32BLETracker::dump_config() { " Continuous Scanning: %s", this->scan_duration_, this->scan_interval_ * 0.625f, this->scan_window_ * 0.625f, this->scan_active_ ? "ACTIVE" : "PASSIVE", YESNO(this->scan_continuous_)); +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + if (this->connection_scan_window_ != 0) { + ESP_LOGCONFIG(TAG, " Connection Scan Window: %.1f ms", this->connection_scan_window_ * 0.625f); + } +#endif ESP_LOGCONFIG(TAG, " Scanner State: %s\n" " Connecting: %d, discovered: %d, disconnecting: %d, active: %d", @@ -487,6 +518,18 @@ void ESP32BLETracker::cleanup_scan_state_(bool is_stop_complete) { // Reset timeout state machine instead of cancelling scheduler timeout this->scan_timeout_state_ = ScanTimeoutState::INACTIVE; + this->notify_scan_end_(); + + this->set_scanner_state_(ScannerState::IDLE); +} + +void ESP32BLETracker::notify_scan_end_() { +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // Window-change restart continues the same scan period; the flag stays set + // across the stop and is cleared by the restart in start_scan_. + if (this->skip_next_scan_end_) + return; +#endif #ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT for (auto *listener : this->listeners_) listener->on_scan_end(); @@ -495,8 +538,6 @@ void ESP32BLETracker::cleanup_scan_state_(bool is_stop_complete) { for (auto *listener : this->neutral_listeners_) listener->on_scan_end(); #endif - - this->set_scanner_state_(ScannerState::IDLE); } void ESP32BLETracker::handle_scanner_failure_() { @@ -534,6 +575,8 @@ void ESP32BLETracker::try_promote_discovered_clients_() { } ESP_LOGD(TAG, "Promoting client to connect"); + // A connect ends the scan period a window-change restart was continuing. + this->skip_next_scan_end_ = false; #ifdef USE_ESP32_BLE_SOFTWARE_COEXISTENCE this->update_coex_preference_(true); #endif diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h index 7c3e5538fd..618444e626 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h @@ -169,6 +169,9 @@ class ESP32BLETracker final : public Component, void set_scan_duration(uint32_t scan_duration) { scan_duration_ = scan_duration; } void set_scan_interval(uint32_t scan_interval) { scan_interval_ = scan_interval; } void set_scan_window(uint32_t scan_window) { scan_window_ = scan_window; } +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + void set_connection_scan_window(uint32_t scan_window) { connection_scan_window_ = scan_window; } +#endif void set_scan_active(bool scan_active) { scan_active_ = scan_active; } bool get_scan_active() const { return scan_active_; } void set_scan_continuous(bool scan_continuous) { scan_continuous_ = scan_continuous; } @@ -226,7 +229,10 @@ class ESP32BLETracker final : public Component, ScannerState get_scanner_state() const { return this->scanner_state_; } protected: - void stop_scan_(); + /// Returns true when a stop was issued to the controller. + bool stop_scan_(); + /// Fire on_scan_end on every listener unless a window-change restart suppressed it. + void notify_scan_end_(); /// Start a single scan by setting up the parameters and doing some esp-idf calls. void start_scan_(bool first); /// Called when a `ESP_GAP_BLE_SCAN_RESULT_EVT` event is received. @@ -313,6 +319,15 @@ class ESP32BLETracker final : public Component, uint32_t scan_duration_; uint32_t scan_interval_; uint32_t scan_window_; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + /// Window used while a GATT connection is active; set by the user, or + /// defaulted when the window was raised to full duty (0 = no fallback). + uint32_t connection_scan_window_{0}; + /// The window to scan at for the given number of active GATT connections. + uint32_t desired_scan_window_(uint8_t active) const { + return (this->connection_scan_window_ != 0 && active > 0) ? this->connection_scan_window_ : this->scan_window_; + } +#endif esp_bt_status_t scan_start_failed_{ESP_BT_STATUS_SUCCESS}; esp_bt_status_t scan_set_param_failed_{ESP_BT_STATUS_SUCCESS}; @@ -330,15 +345,20 @@ class ESP32BLETracker final : public Component, /// state_version_ to detect if any state changed since last iteration. uint8_t last_processed_version_{0}; ScannerState scanner_state_{ScannerState::IDLE}; - bool scan_continuous_; - bool scan_active_; + // Packed 1-bit flags. + bool scan_continuous_ : 1; + bool scan_active_ : 1; #ifdef USE_OTA_STATE_LISTENER - bool scan_continuous_before_ota_{false}; + bool scan_continuous_before_ota_ : 1 {false}; +#endif + bool ble_was_disabled_ : 1 {true}; + bool parse_advertisements_ : 1 {false}; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + /// Suppress the window-change restart's on_scan_end sweeps (stop and start). + bool skip_next_scan_end_ : 1 {false}; #endif - bool ble_was_disabled_{true}; - bool parse_advertisements_{false}; #ifdef USE_ESP32_BLE_SOFTWARE_COEXISTENCE - bool coex_prefer_ble_{false}; + bool coex_prefer_ble_ : 1 {false}; #endif // Scan timeout state machine enum class ScanTimeoutState : uint8_t { @@ -346,10 +366,10 @@ class ESP32BLETracker final : public Component, MONITORING, // Actively monitoring for timeout EXCEEDED_WAIT, // Timeout exceeded, waiting one loop before reboot }; + ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; uint32_t scan_start_time_{0}; /// Precomputed timeout value: scan_duration_ * 2000 uint32_t scan_timeout_ms_{0}; - ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; }; // NOLINTNEXTLINE diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml new file mode 100644 index 0000000000..70760fdea6 --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml @@ -0,0 +1,19 @@ +esphome: + name: scan-window-explicit + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + scan_parameters: + window: 30ms + +bluetooth_proxy: + active: true + +api: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml new file mode 100644 index 0000000000..4febbfcf3b --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml @@ -0,0 +1,17 @@ +esphome: + name: scan-window-raised + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + +bluetooth_proxy: + active: true + +api: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml new file mode 100644 index 0000000000..5da5601388 --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml @@ -0,0 +1,12 @@ +esphome: + name: scan-window-scan-only + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml new file mode 100644 index 0000000000..2b9093164f --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml @@ -0,0 +1,14 @@ +esphome: + name: scan-window-user-scan-only + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + scan_parameters: + connection_scan_window: 20ms diff --git a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py index 8a25f488fa..8612ac6732 100644 --- a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py +++ b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py @@ -12,11 +12,12 @@ arbiter a full-duty scan would starve wifi, so the 30 ms default is kept. from __future__ import annotations from collections.abc import Callable +from pathlib import Path import pytest from esphome import config_validation as cv -from esphome.components.ble_device_base import to_ble_units +from esphome.components.ble_device_base import CONF_CONNECTION_SCAN_WINDOW, to_ble_units from esphome.components.const import CONF_SCAN_PARAMETERS, CONF_WINDOW from esphome.components.esp32 import KEY_IDF_VERSION from esphome.components.esp32_ble_tracker import ( @@ -120,3 +121,103 @@ def test_short_interval_without_window_still_rejected( stage_esp32("5.5.5", wifi=True) with pytest.raises(cv.Invalid, match="needs to be smaller than scan interval"): _scan_params({"scan_parameters": {"interval": "20ms"}}) + + +# The connection-time fallback window: while a GATT connection is active the +# scanner drops from a raised full-duty window back to this value so the +# connection gets guaranteed airtime. + + +def test_raise_arms_connection_scan_window_default( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + params = _scan_params({}) + assert params[CONF_WINDOW] == params[CONF_INTERVAL] + assert to_ble_units(params[CONF_CONNECTION_SCAN_WINDOW]) == 48 + + +def test_user_connection_scan_window_survives_raise( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + params = _scan_params({"scan_parameters": {"connection_scan_window": "60ms"}}) + assert params[CONF_WINDOW] == params[CONF_INTERVAL] + assert to_ble_units(params[CONF_CONNECTION_SCAN_WINDOW]) == 96 + + +def test_unraised_window_gets_no_connection_scan_window_default( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.4", wifi=True) + assert CONF_CONNECTION_SCAN_WINDOW not in _scan_params({}) + + +def test_connection_scan_window_above_interval_rejected( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + with pytest.raises( + cv.Invalid, match="connection_scan_window .* needs to be smaller" + ): + _scan_params({"scan_parameters": {"connection_scan_window": "400ms"}}) + + +def test_connection_scan_window_above_window_rejected( + stage_esp32: Callable[..., None], +) -> None: + """A connection window above the (post-raise) window would widen the scan + during connections; the reject runs after the raise so a fallback below a + raised window still validates (covered by the survives-raise test).""" + stage_esp32("5.5.5", wifi=True) + with pytest.raises( + cv.Invalid, match="connection_scan_window .* needs to be smaller" + ): + _scan_params( + {"scan_parameters": {"window": "30ms", "connection_scan_window": "300ms"}} + ) + + +def test_connection_scan_window_truncation_collapse_rejected( + stage_esp32: Callable[..., None], +) -> None: + """A connection window that truncates into the interval's 0.625 ms unit + would silently program a full-duty scan during connections.""" + stage_esp32("5.5.5", wifi=True) + with pytest.raises(cv.Invalid, match="connection_scan_window .* both truncate"): + _scan_params( + { + "scan_parameters": { + "interval": "320.5ms", + "connection_scan_window": "320.2ms", + } + } + ) + + +@pytest.mark.parametrize( + ("config_file", "window_call", "connection_call", "warns"), + [ + # Raised window with GATT clients: the injected fallback is emitted. + ("scan_window_raised.yaml", "set_scan_window(512)", True, False), + # Explicit window: nothing injected. + ("scan_window_explicit.yaml", "set_scan_window(48)", False, False), + # Scan-only build compiles the path out: the injected default is + # dropped silently, a user-set value warns. + ("scan_window_scan_only.yaml", "set_scan_window(512)", False, False), + ("scan_window_user_set_scan_only.yaml", "set_scan_window(512)", False, True), + ], +) +def test_connection_scan_window_codegen( + generate_main: Callable[[str | Path], str], + component_config_path: Callable[[str], Path], + caplog: pytest.LogCaptureFixture, + config_file: str, + window_call: str, + connection_call: bool, + warns: bool, +) -> None: + main_cpp = generate_main(component_config_path(config_file)) + assert window_call in main_cpp + assert ("set_connection_scan_window(48)" in main_cpp) == connection_call + assert ("'connection_scan_window' has no effect" in caplog.text) == warns From 6cfbe5b166268cd3289c372ceff4faf5430b0272 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 17:15:29 -0500 Subject: [PATCH 6/6] Normalize captured CPPDEFINES to a CppDefine named tuple at capture time --- esphome/platformio/extra_script.py | 67 ++++++++++++------- .../test_platformio_extra_script.py | 7 +- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index 8ffd6ee24b..e60a50d746 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -14,7 +14,7 @@ import logging import os from pathlib import Path import shlex -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, NamedTuple from esphome.core import EsphomeError from esphome.platformio.library import ESPHOME_DATA_KEY, ESPHOME_DATA_LINK_FLAGS_KEY @@ -99,20 +99,47 @@ class ExtraScriptResult: cpppath: list[str] = field(default_factory=list) libpath: list[str] = field(default_factory=list) libs: list[str] = field(default_factory=list) - cppdefines: list[str | tuple[str, str]] = field(default_factory=list) + cppdefines: list[CppDefine] = field(default_factory=list) linkflags: list[str] = field(default_factory=list) cppflags: list[str] = field(default_factory=list) -def _cppdefines_items(value: Any) -> list: - """Normalize SCons ``processDefines`` spellings: a bare 2-tuple is one - ``name=value`` pair, a dict maps names to values, a list is - element-wise.""" +class CppDefine(NamedTuple): + """One normalized CPPDEFINES entry; a ``value`` of None is a bare -DNAME.""" + + name: str + value: str | None = None + + +def _cppdefine(entry: Any) -> CppDefine | None: + """Normalize one CPPDEFINES element, or warn and drop an unsupported + shape; formatting those blind would hand the compiler garbage like + ``-D{'FOO': '1'}``.""" + if isinstance(entry, str): + return CppDefine(entry) + if ( + isinstance(entry, (tuple, list)) + and len(entry) == 2 + and isinstance(entry[0], (str, int)) + and isinstance(entry[1], (str, int, type(None))) + ): + value = entry[1] + return CppDefine(str(entry[0]), None if value is None else str(value)) + _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", entry) + return None + + +def _cppdefines_items(value: Any) -> list[CppDefine]: + """Normalize SCons ``processDefines`` spellings into ``CppDefine``s: a + bare 2-tuple is one ``name=value`` pair, a dict maps names to values, a + list is element-wise.""" if isinstance(value, tuple) and len(value) == 2: - return [value] - if isinstance(value, dict): - return list(value.items()) - return list(value) if isinstance(value, (list, tuple)) else [value] + elements: list[Any] = [value] + elif isinstance(value, dict): + elements = list(value.items()) + else: + elements = list(value) if isinstance(value, (list, tuple)) else [value] + return [d for e in elements if (d := _cppdefine(e)) is not None] class _FakeSConsEnv: @@ -316,23 +343,11 @@ def captured_as_build_flags( ) flags.extend(f"-l{shlex.quote(lib)}" for lib in _str_entries(result.libs, "LIBS")) for define in result.cppdefines: - # SCons also accepts nested containers; formatting those blind - # would hand the compiler garbage like -D{'FOO': '1'} - if ( - isinstance(define, (tuple, list)) - and len(define) == 2 - and isinstance(define[0], (str, int)) - and isinstance(define[1], (str, int, type(None))) - ): - if define[1] is None: - # {"FOO": None} / ("FOO", None) is a bare -DFOO in SCons - flags.append(shlex.quote(f"-D{define[0]}")) - else: - flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) - elif isinstance(define, str): - flags.append(shlex.quote(f"-D{define}")) + if define.value is None: + # {"FOO": None} / ("FOO", None) is a bare -DFOO in SCons + flags.append(shlex.quote(f"-D{define.name}")) else: - _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) + flags.append(shlex.quote(f"-D{define.name}={define.value}")) # Each captured entry is one argv token in SCons; quote so the # lex_build_flags round-trip cannot split a spaced value into two. # LINKFLAGS are deliberately absent: they travel via diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 90ae4aa43b..980ce29ccb 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -11,6 +11,7 @@ import pytest from esphome.core import EsphomeError from esphome.platformio.extra_script import ( + CppDefine, ExtraScriptResult, _FakeSConsEnv, apply_extra_script, @@ -51,8 +52,8 @@ def test_extra_script_captures_libpath_libs_and_defines(tmp_path): assert result.libpath == [str(Path("src") / "esp32")] assert result.libs == ["algobsec"] - assert ("BAR", "1") in result.cppdefines - assert "FOO" in result.cppdefines + assert CppDefine("BAR", "1") in result.cppdefines + assert CppDefine("FOO") in result.cppdefines assert result.linkflags == ["-Wl,--gc-sections"] # Lex like the consumer does: quoting makes raw strings platform-varying @@ -476,7 +477,7 @@ def test_spaced_cppflag_survives_relexing(tmp_path) -> None: """A captured argv token with a space stays one token after lexing.""" result = ExtraScriptResult( cppflags=["-include my hdr.h"], - cppdefines=[("MSG", '"hello world"'), "PLAIN"], + cppdefines=[CppDefine("MSG", '"hello world"'), CppDefine("PLAIN")], ) flags = captured_as_build_flags(result, library_dir=tmp_path) assert lex_build_flags(flags, "test") == [