From bdfd88441a3666ce6f89773bb9bbfbc384eee4af Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Nov 2025 19:31:23 -0600 Subject: [PATCH 1/6] [ci] Skip memory impact analysis when more than 40 components changed (#11741) --- script/determine-jobs.py | 12 +++++ tests/script/test_determine_jobs.py | 77 ++++++++++++++++++++++------- 2 files changed, 72 insertions(+), 17 deletions(-) diff --git a/script/determine-jobs.py b/script/determine-jobs.py index e9d17d8fe5..39a7571fbe 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -94,6 +94,7 @@ class Platform(StrEnum): # Memory impact analysis constants MEMORY_IMPACT_FALLBACK_COMPONENT = "api" # Representative component for core changes MEMORY_IMPACT_FALLBACK_PLATFORM = Platform.ESP32_IDF # Most representative platform +MEMORY_IMPACT_MAX_COMPONENTS = 40 # Max components before results become nonsensical # Platform-specific components that can only be built on their respective platforms # These components contain platform-specific code and cannot be cross-compiled @@ -555,6 +556,17 @@ def detect_memory_impact_config( if not components_with_tests: return {"should_run": "false"} + # Skip memory impact analysis if too many components changed + # Building 40+ components at once produces nonsensical memory impact results + # This typically happens with large refactorings or batch updates + if len(components_with_tests) > MEMORY_IMPACT_MAX_COMPONENTS: + print( + f"Memory impact: Skipping analysis for {len(components_with_tests)} components " + f"(limit is {MEMORY_IMPACT_MAX_COMPONENTS}, would give nonsensical results)", + file=sys.stderr, + ) + return {"should_run": "false"} + # Find common platforms supported by ALL components # This ensures we can build all components together in a merged config common_platforms = set(MEMORY_IMPACT_PLATFORM_PREFERENCE) diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 9f12d7ffcf..4894a5e28a 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -71,9 +71,10 @@ def mock_changed_files() -> Generator[Mock, None, None]: @pytest.fixture(autouse=True) -def clear_clang_tidy_cache() -> None: - """Clear the clang-tidy full scan cache before each test.""" +def clear_determine_jobs_caches() -> None: + """Clear all cached functions before each test.""" determine_jobs._is_clang_tidy_full_scan.cache_clear() + determine_jobs._component_has_tests.cache_clear() def test_main_all_tests_should_run( @@ -565,7 +566,6 @@ def test_main_filters_components_without_tests( patch.object(determine_jobs, "changed_files", return_value=[]), ): # Clear the cache since we're mocking root_path - determine_jobs._component_has_tests.cache_clear() determine_jobs.main() # Check output @@ -665,7 +665,6 @@ def test_main_detects_components_with_variant_tests( patch.object(determine_jobs, "changed_files", return_value=[]), ): # Clear the cache since we're mocking root_path - determine_jobs._component_has_tests.cache_clear() determine_jobs.main() # Check output @@ -714,7 +713,6 @@ def test_detect_memory_impact_config_with_common_platform(tmp_path: Path) -> Non "esphome/components/wifi/wifi.cpp", "esphome/components/api/api.cpp", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -744,7 +742,6 @@ def test_detect_memory_impact_config_core_only_changes(tmp_path: Path) -> None: "esphome/core/application.cpp", "esphome/core/component.h", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -775,7 +772,6 @@ def test_detect_memory_impact_config_core_python_only_changes(tmp_path: Path) -> "esphome/config.py", "esphome/core/config.py", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -808,7 +804,6 @@ def test_detect_memory_impact_config_no_common_platform(tmp_path: Path) -> None: "esphome/components/wifi/wifi.cpp", "esphome/components/logger/logger.cpp", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -830,7 +825,6 @@ def test_detect_memory_impact_config_no_changes(tmp_path: Path) -> None: patch.object(determine_jobs, "changed_files") as mock_changed_files, ): mock_changed_files.return_value = [] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -855,7 +849,6 @@ def test_detect_memory_impact_config_no_components_with_tests(tmp_path: Path) -> mock_changed_files.return_value = [ "esphome/components/my_custom_component/component.cpp", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -895,7 +888,6 @@ def test_detect_memory_impact_config_includes_base_bus_components( "esphome/components/uart/automation.h", # Header file with inline code "esphome/components/wifi/wifi.cpp", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -938,7 +930,6 @@ def test_detect_memory_impact_config_with_variant_tests(tmp_path: Path) -> None: "esphome/components/improv_serial/improv_serial.cpp", "esphome/components/ethernet/ethernet.cpp", ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -1168,7 +1159,6 @@ def test_detect_memory_impact_config_filters_incompatible_esp32_on_esp8266( "tests/components/esp8266/test.esp8266-ard.yaml", "esphome/core/helpers_esp8266.h", # ESP8266-specific file to hint platform ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -1222,7 +1212,6 @@ def test_detect_memory_impact_config_filters_incompatible_esp8266_on_esp32( "esphome/components/wifi/wifi_component_esp_idf.cpp", # ESP-IDF hint "esphome/components/ethernet/ethernet_esp32.cpp", # ESP32 hint ] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -1257,7 +1246,6 @@ def test_detect_memory_impact_config_skips_release_branch(tmp_path: Path) -> Non patch.object(determine_jobs, "get_target_branch", return_value="release"), ): mock_changed_files.return_value = ["esphome/components/wifi/wifi.cpp"] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -1280,7 +1268,6 @@ def test_detect_memory_impact_config_skips_beta_branch(tmp_path: Path) -> None: patch.object(determine_jobs, "get_target_branch", return_value="beta"), ): mock_changed_files.return_value = ["esphome/components/wifi/wifi.cpp"] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() @@ -1303,10 +1290,66 @@ def test_detect_memory_impact_config_runs_for_dev_branch(tmp_path: Path) -> None patch.object(determine_jobs, "get_target_branch", return_value="dev"), ): mock_changed_files.return_value = ["esphome/components/wifi/wifi.cpp"] - determine_jobs._component_has_tests.cache_clear() result = determine_jobs.detect_memory_impact_config() # Memory impact should run for dev branch assert result["should_run"] == "true" assert result["components"] == ["wifi"] + + +def test_detect_memory_impact_config_skips_too_many_components( + tmp_path: Path, +) -> None: + """Test that memory impact analysis is skipped when more than 40 components changed.""" + # Create test directory structure with 41 components + tests_dir = tmp_path / "tests" / "components" + component_names = [f"component_{i}" for i in range(41)] + + for component_name in component_names: + comp_dir = tests_dir / component_name + comp_dir.mkdir(parents=True) + (comp_dir / "test.esp32-idf.yaml").write_text(f"test: {component_name}") + + with ( + patch.object(determine_jobs, "root_path", str(tmp_path)), + patch.object(helpers, "root_path", str(tmp_path)), + patch.object(determine_jobs, "changed_files") as mock_changed_files, + patch.object(determine_jobs, "get_target_branch", return_value="dev"), + ): + mock_changed_files.return_value = [ + f"esphome/components/{name}/{name}.cpp" for name in component_names + ] + + result = determine_jobs.detect_memory_impact_config() + + # Memory impact should be skipped for too many components (41 > 40) + assert result["should_run"] == "false" + + +def test_detect_memory_impact_config_runs_at_component_limit(tmp_path: Path) -> None: + """Test that memory impact analysis runs with exactly 40 components (at limit).""" + # Create test directory structure with exactly 40 components + tests_dir = tmp_path / "tests" / "components" + component_names = [f"component_{i}" for i in range(40)] + + for component_name in component_names: + comp_dir = tests_dir / component_name + comp_dir.mkdir(parents=True) + (comp_dir / "test.esp32-idf.yaml").write_text(f"test: {component_name}") + + with ( + patch.object(determine_jobs, "root_path", str(tmp_path)), + patch.object(helpers, "root_path", str(tmp_path)), + patch.object(determine_jobs, "changed_files") as mock_changed_files, + patch.object(determine_jobs, "get_target_branch", return_value="dev"), + ): + mock_changed_files.return_value = [ + f"esphome/components/{name}/{name}.cpp" for name in component_names + ] + + result = determine_jobs.detect_memory_impact_config() + + # Memory impact should run at exactly 40 components (at limit but not over) + assert result["should_run"] == "true" + assert len(result["components"]) == 40 From 5eea7bdb44fcd58b99f6738264122e9528ebcbb0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Nov 2025 19:45:48 -0600 Subject: [PATCH 2/6] Update AI instructions with C++ style guidelines from developers docs (#11743) --- .ai/instructions.md | 74 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/.ai/instructions.md b/.ai/instructions.md index 5f314a0dc9..9309c67c65 100644 --- a/.ai/instructions.md +++ b/.ai/instructions.md @@ -51,7 +51,79 @@ This document provides essential context for AI models interacting with this pro * **Naming Conventions:** * **Python:** Follows PEP 8. Use clear, descriptive names following snake_case. - * **C++:** Follows the Google C++ Style Guide. + * **C++:** Follows the Google C++ Style Guide with these specifics (following clang-tidy conventions): + - Function, method, and variable names: `lower_snake_case` + - Class/struct/enum names: `UpperCamelCase` + - Top-level constants (global/namespace scope): `UPPER_SNAKE_CASE` + - Function-local constants: `lower_snake_case` + - Protected/private fields: `lower_snake_case_with_trailing_underscore_` + - Favor descriptive names over abbreviations + +* **C++ Field Visibility:** + * **Prefer `protected`:** Use `protected` for most class fields to enable extensibility and testing. Fields should be `lower_snake_case_with_trailing_underscore_`. + * **Use `private` for safety-critical cases:** Use `private` visibility when direct field access could introduce bugs or violate invariants: + 1. **Pointer lifetime issues:** When setters validate and store pointers from known lists to prevent dangling references. + ```cpp + // Helper to find matching string in vector and return its pointer + inline const char *vector_find(const std::vector &vec, const char *value) { + for (const char *item : vec) { + if (strcmp(item, value) == 0) + return item; + } + return nullptr; + } + + class ClimateDevice { + public: + void set_custom_fan_modes(std::initializer_list modes) { + this->custom_fan_modes_ = modes; + this->active_custom_fan_mode_ = nullptr; // Reset when modes change + } + bool set_custom_fan_mode(const char *mode) { + // Find mode in supported list and store that pointer (not the input pointer) + const char *validated_mode = vector_find(this->custom_fan_modes_, mode); + if (validated_mode != nullptr) { + this->active_custom_fan_mode_ = validated_mode; + return true; + } + return false; + } + private: + std::vector custom_fan_modes_; // Pointers to string literals in flash + const char *active_custom_fan_mode_{nullptr}; // Must point to entry in custom_fan_modes_ + }; + ``` + 2. **Invariant coupling:** When multiple fields must remain synchronized to prevent buffer overflows or data corruption. + ```cpp + class Buffer { + public: + void resize(size_t new_size) { + auto new_data = std::make_unique(new_size); + if (this->data_) { + std::memcpy(new_data.get(), this->data_.get(), std::min(this->size_, new_size)); + } + this->data_ = std::move(new_data); + this->size_ = new_size; // Must stay in sync with data_ + } + private: + std::unique_ptr data_; + size_t size_{0}; // Must match allocated size of data_ + }; + ``` + 3. **Resource management:** When setters perform cleanup or registration operations that derived classes might skip. + * **Provide `protected` accessor methods:** When derived classes need controlled access to `private` members. + +* **C++ Preprocessor Directives:** + * **Avoid `#define` for constants:** Using `#define` for constants is discouraged and should be replaced with `const` variables or enums. + * **Use `#define` only for:** + - Conditional compilation (`#ifdef`, `#ifndef`) + - Compile-time sizes calculated during Python code generation (e.g., configuring `std::array` or `StaticVector` dimensions via `cg.add_define()`) + +* **C++ Additional Conventions:** + * **Member access:** Prefix all class member access with `this->` (e.g., `this->value_` not `value_`) + * **Indentation:** Use spaces (two per indentation level), not tabs + * **Type aliases:** Prefer `using type_t = int;` over `typedef int type_t;` + * **Line length:** Wrap lines at no more than 120 characters * **Component Structure:** * **Standard Files:** From 83f30a64ed54f9285b0d68ba22b8654cdc75c5be Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Nov 2025 20:31:59 -0600 Subject: [PATCH 3/6] [api] Store YAML service names in flash instead of heap (#11744) --- esphome/components/api/custom_api_device.h | 4 +- esphome/components/api/user_services.h | 60 +++++++++++++++++-- .../fixtures/api_custom_services.yaml | 22 +++++++ tests/integration/test_api_custom_services.py | 60 ++++++++++++++++++- 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/esphome/components/api/custom_api_device.h b/esphome/components/api/custom_api_device.h index d34ccfa0ce..43ea644f0c 100644 --- a/esphome/components/api/custom_api_device.h +++ b/esphome/components/api/custom_api_device.h @@ -9,11 +9,11 @@ namespace esphome::api { #ifdef USE_API_SERVICES -template class CustomAPIDeviceService : public UserServiceBase { +template class CustomAPIDeviceService : public UserServiceDynamic { public: CustomAPIDeviceService(const std::string &name, const std::array &arg_names, T *obj, void (T::*callback)(Ts...)) - : UserServiceBase(name, arg_names), obj_(obj), callback_(callback) {} + : UserServiceDynamic(name, arg_names), obj_(obj), callback_(callback) {} protected: void execute(Ts... x) override { (this->obj_->*this->callback_)(x...); } // NOLINT diff --git a/esphome/components/api/user_services.h b/esphome/components/api/user_services.h index 9ca5e1093e..2a887fc52d 100644 --- a/esphome/components/api/user_services.h +++ b/esphome/components/api/user_services.h @@ -23,11 +23,13 @@ template T get_execute_arg_value(const ExecuteServiceArgument &arg); template enums::ServiceArgType to_service_arg_type(); +// Base class for YAML-defined services (most common case) +// Stores only pointers to string literals in flash - no heap allocation template class UserServiceBase : public UserServiceDescriptor { public: - UserServiceBase(std::string name, const std::array &arg_names) - : name_(std::move(name)), arg_names_(arg_names) { - this->key_ = fnv1_hash(this->name_); + UserServiceBase(const char *name, const std::array &arg_names) + : name_(name), arg_names_(arg_names) { + this->key_ = fnv1_hash(name); } ListEntitiesServicesResponse encode_list_service_response() override { @@ -47,7 +49,7 @@ template class UserServiceBase : public UserServiceDescriptor { bool execute_service(const ExecuteServiceRequest &req) override { if (req.key != this->key_) return false; - if (req.args.size() != this->arg_names_.size()) + if (req.args.size() != sizeof...(Ts)) return false; this->execute_(req.args, typename gens::type()); return true; @@ -59,14 +61,60 @@ template class UserServiceBase : public UserServiceDescriptor { this->execute((get_execute_arg_value(args[S]))...); } - std::string name_; + // Pointers to string literals in flash - no heap allocation + const char *name_; + std::array arg_names_; uint32_t key_{0}; +}; + +// Separate class for custom_api_device services (rare case) +// Stores copies of runtime-generated names +template class UserServiceDynamic : public UserServiceDescriptor { + public: + UserServiceDynamic(std::string name, const std::array &arg_names) + : name_(std::move(name)), arg_names_(arg_names) { + this->key_ = fnv1_hash(this->name_.c_str()); + } + + ListEntitiesServicesResponse encode_list_service_response() override { + ListEntitiesServicesResponse msg; + msg.set_name(StringRef(this->name_)); + msg.key = this->key_; + std::array arg_types = {to_service_arg_type()...}; + msg.args.init(sizeof...(Ts)); + for (size_t i = 0; i < sizeof...(Ts); i++) { + auto &arg = msg.args.emplace_back(); + arg.type = arg_types[i]; + arg.set_name(StringRef(this->arg_names_[i])); + } + return msg; + } + + bool execute_service(const ExecuteServiceRequest &req) override { + if (req.key != this->key_) + return false; + if (req.args.size() != sizeof...(Ts)) + return false; + this->execute_(req.args, typename gens::type()); + return true; + } + + protected: + virtual void execute(Ts... x) = 0; + template void execute_(const ArgsContainer &args, seq type) { + this->execute((get_execute_arg_value(args[S]))...); + } + + // Heap-allocated strings for runtime-generated names + std::string name_; std::array arg_names_; + uint32_t key_{0}; }; template class UserServiceTrigger : public UserServiceBase, public Trigger { public: - UserServiceTrigger(const std::string &name, const std::array &arg_names) + // Constructor for static names (YAML-defined services - used by code generator) + UserServiceTrigger(const char *name, const std::array &arg_names) : UserServiceBase(name, arg_names) {} protected: diff --git a/tests/integration/fixtures/api_custom_services.yaml b/tests/integration/fixtures/api_custom_services.yaml index 41efc95b85..a597c74126 100644 --- a/tests/integration/fixtures/api_custom_services.yaml +++ b/tests/integration/fixtures/api_custom_services.yaml @@ -11,6 +11,28 @@ api: then: - logger.log: "YAML service called" + # Test YAML service with arguments (tests UserServiceBase with const char* array) + - action: test_yaml_service_with_args + variables: + my_int: int + my_string: string + then: + - logger.log: + format: "YAML service with args: %d, %s" + args: [my_int, my_string.c_str()] + + # Test YAML service with multiple arguments + - action: test_yaml_service_many_args + variables: + arg1: int + arg2: float + arg3: bool + arg4: string + then: + - logger.log: + format: "YAML service many args: %d, %.2f, %d, %s" + args: [arg1, arg2, arg3, arg4.c_str()] + logger: level: DEBUG diff --git a/tests/integration/test_api_custom_services.py b/tests/integration/test_api_custom_services.py index 9ae4cdcb5d..967c504112 100644 --- a/tests/integration/test_api_custom_services.py +++ b/tests/integration/test_api_custom_services.py @@ -33,12 +33,16 @@ async def test_api_custom_services( # Track log messages yaml_service_future = loop.create_future() + yaml_args_future = loop.create_future() + yaml_many_args_future = loop.create_future() custom_service_future = loop.create_future() custom_args_future = loop.create_future() custom_arrays_future = loop.create_future() # Patterns to match in logs yaml_service_pattern = re.compile(r"YAML service called") + yaml_args_pattern = re.compile(r"YAML service with args: 123, test_value") + yaml_many_args_pattern = re.compile(r"YAML service many args: 42, 3\.14, 1, hello") custom_service_pattern = re.compile(r"Custom test service called!") custom_args_pattern = re.compile( r"Custom service called with: test_string, 456, 1, 78\.90" @@ -51,6 +55,10 @@ async def test_api_custom_services( """Check log output for expected messages.""" if not yaml_service_future.done() and yaml_service_pattern.search(line): yaml_service_future.set_result(True) + elif not yaml_args_future.done() and yaml_args_pattern.search(line): + yaml_args_future.set_result(True) + elif not yaml_many_args_future.done() and yaml_many_args_pattern.search(line): + yaml_many_args_future.set_result(True) elif not custom_service_future.done() and custom_service_pattern.search(line): custom_service_future.set_result(True) elif not custom_args_future.done() and custom_args_pattern.search(line): @@ -71,11 +79,13 @@ async def test_api_custom_services( # List services _, services = await client.list_entities_services() - # Should have 4 services: 1 YAML + 3 CustomAPIDevice - assert len(services) == 4, f"Expected 4 services, found {len(services)}" + # Should have 6 services: 3 YAML + 3 CustomAPIDevice + assert len(services) == 6, f"Expected 6 services, found {len(services)}" # Find our services yaml_service: UserService | None = None + yaml_args_service: UserService | None = None + yaml_many_args_service: UserService | None = None custom_service: UserService | None = None custom_args_service: UserService | None = None custom_arrays_service: UserService | None = None @@ -83,6 +93,10 @@ async def test_api_custom_services( for service in services: if service.name == "test_yaml_service": yaml_service = service + elif service.name == "test_yaml_service_with_args": + yaml_args_service = service + elif service.name == "test_yaml_service_many_args": + yaml_many_args_service = service elif service.name == "custom_test_service": custom_service = service elif service.name == "custom_service_with_args": @@ -91,6 +105,10 @@ async def test_api_custom_services( custom_arrays_service = service assert yaml_service is not None, "test_yaml_service not found" + assert yaml_args_service is not None, "test_yaml_service_with_args not found" + assert yaml_many_args_service is not None, ( + "test_yaml_service_many_args not found" + ) assert custom_service is not None, "custom_test_service not found" assert custom_args_service is not None, "custom_service_with_args not found" assert custom_arrays_service is not None, "custom_service_with_arrays not found" @@ -99,6 +117,44 @@ async def test_api_custom_services( client.execute_service(yaml_service, {}) await asyncio.wait_for(yaml_service_future, timeout=5.0) + # Verify YAML service with args arguments + assert len(yaml_args_service.args) == 2 + yaml_args_types = {arg.name: arg.type for arg in yaml_args_service.args} + assert yaml_args_types["my_int"] == UserServiceArgType.INT + assert yaml_args_types["my_string"] == UserServiceArgType.STRING + + # Test YAML service with arguments + client.execute_service( + yaml_args_service, + { + "my_int": 123, + "my_string": "test_value", + }, + ) + await asyncio.wait_for(yaml_args_future, timeout=5.0) + + # Verify YAML service with many args arguments + assert len(yaml_many_args_service.args) == 4 + yaml_many_args_types = { + arg.name: arg.type for arg in yaml_many_args_service.args + } + assert yaml_many_args_types["arg1"] == UserServiceArgType.INT + assert yaml_many_args_types["arg2"] == UserServiceArgType.FLOAT + assert yaml_many_args_types["arg3"] == UserServiceArgType.BOOL + assert yaml_many_args_types["arg4"] == UserServiceArgType.STRING + + # Test YAML service with many arguments + client.execute_service( + yaml_many_args_service, + { + "arg1": 42, + "arg2": 3.14, + "arg3": True, + "arg4": "hello", + }, + ) + await asyncio.wait_for(yaml_many_args_future, timeout=5.0) + # Test simple CustomAPIDevice service client.execute_service(custom_service, {}) await asyncio.wait_for(custom_service_future, timeout=5.0) From ab5d8f67aee0c631b3fdf7d6cae94aa71fb4b60c Mon Sep 17 00:00:00 2001 From: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com> Date: Thu, 6 Nov 2025 12:48:02 +1000 Subject: [PATCH 4/6] [core] Add helper functions for clamp_at_... (#10387) --- esphome/core/helpers.h | 16 +++++++++++++++- tests/components/esp32/common.yaml | 13 +++++++++++++ tests/components/esp8266/test.esp8266-ard.yaml | 13 +++++++++++++ tests/components/host/common.yaml | 7 +++++++ tests/components/libretiny/test.bk72xx-ard.yaml | 13 +++++++++++++ tests/components/nrf52/test.nrf52-adafruit.yaml | 10 ++++++++++ tests/components/rp2040/test.rp2040-ard.yaml | 13 +++++++++++++ 7 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 tests/components/esp32/common.yaml create mode 100644 tests/components/esp8266/test.esp8266-ard.yaml create mode 100644 tests/components/libretiny/test.bk72xx-ard.yaml create mode 100644 tests/components/rp2040/test.rp2040-ard.yaml diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 660874ed1a..48af7f674a 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -12,6 +12,7 @@ #include #include #include +#include #include "esphome/core/optional.h" @@ -1169,7 +1170,20 @@ template class RAMAllocator { template using ExternalRAMAllocator = RAMAllocator; -/// @} +/** + * Functions to constrain the range of arithmetic values. + */ + +template T clamp_at_least(T value, T min) { + if (value < min) + return min; + return value; +} +template T clamp_at_most(T value, T max) { + if (value > max) + return max; + return value; +} /// @name Internal functions ///@{ diff --git a/tests/components/esp32/common.yaml b/tests/components/esp32/common.yaml new file mode 100644 index 0000000000..039a261016 --- /dev/null +++ b/tests/components/esp32/common.yaml @@ -0,0 +1,13 @@ +logger: + level: VERBOSE + +esphome: + on_boot: + - lambda: |- + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); diff --git a/tests/components/esp8266/test.esp8266-ard.yaml b/tests/components/esp8266/test.esp8266-ard.yaml new file mode 100644 index 0000000000..039a261016 --- /dev/null +++ b/tests/components/esp8266/test.esp8266-ard.yaml @@ -0,0 +1,13 @@ +logger: + level: VERBOSE + +esphome: + on_boot: + - lambda: |- + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); diff --git a/tests/components/host/common.yaml b/tests/components/host/common.yaml index 5c329c8245..d5c8446ae8 100644 --- a/tests/components/host/common.yaml +++ b/tests/components/host/common.yaml @@ -15,3 +15,10 @@ esphome: static const uint8_t my_addr[6] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}; if (!mac_address_is_valid(my_addr)) ESP_LOGD("test", "Invalid mac address %X", my_addr[0]); // etc. + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); diff --git a/tests/components/libretiny/test.bk72xx-ard.yaml b/tests/components/libretiny/test.bk72xx-ard.yaml new file mode 100644 index 0000000000..039a261016 --- /dev/null +++ b/tests/components/libretiny/test.bk72xx-ard.yaml @@ -0,0 +1,13 @@ +logger: + level: VERBOSE + +esphome: + on_boot: + - lambda: |- + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); diff --git a/tests/components/nrf52/test.nrf52-adafruit.yaml b/tests/components/nrf52/test.nrf52-adafruit.yaml index 3fe80209b6..cf704ecceb 100644 --- a/tests/components/nrf52/test.nrf52-adafruit.yaml +++ b/tests/components/nrf52/test.nrf52-adafruit.yaml @@ -1,3 +1,13 @@ +esphome: + on_boot: + - lambda: |- + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); nrf52: dfu: reset_pin: diff --git a/tests/components/rp2040/test.rp2040-ard.yaml b/tests/components/rp2040/test.rp2040-ard.yaml new file mode 100644 index 0000000000..039a261016 --- /dev/null +++ b/tests/components/rp2040/test.rp2040-ard.yaml @@ -0,0 +1,13 @@ +logger: + level: VERBOSE + +esphome: + on_boot: + - lambda: |- + int x = 100; + x = clamp(x, 50, 90); + assert(x == 90); + x = clamp_at_least(x, 95); + assert(x == 95); + x = clamp_at_most(x, 40); + assert(x == 40); From 822eacfd77878067484601ec2fe58edde7fae9e4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Nov 2025 20:49:24 -0600 Subject: [PATCH 5/6] [core] Fix wait_until and for_condition timing regression in automation chains (#11716) --- esphome/core/base_automation.h | 82 +++++++------ .../fixtures/wait_until_mid_loop_timing.yaml | 109 +++++++++++++++++ .../test_wait_until_mid_loop_timing.py | 112 ++++++++++++++++++ 3 files changed, 269 insertions(+), 34 deletions(-) create mode 100644 tests/integration/fixtures/wait_until_mid_loop_timing.yaml create mode 100644 tests/integration/test_wait_until_mid_loop_timing.py diff --git a/esphome/core/base_automation.h b/esphome/core/base_automation.h index 128a2d4b08..6f392c8959 100644 --- a/esphome/core/base_automation.h +++ b/esphome/core/base_automation.h @@ -98,22 +98,28 @@ template class ForCondition : public Condition, public Co TEMPLATABLE_VALUE(uint32_t, time); - void loop() override { this->check_internal(); } - float get_setup_priority() const override { return setup_priority::DATA; } - bool check_internal() { - bool cond = this->condition_->check(); - if (!cond) - this->last_inactive_ = App.get_loop_component_start_time(); - return cond; + void loop() override { + // Safe to use cached time - only called from Application::loop() + this->check_internal_(App.get_loop_component_start_time()); } + float get_setup_priority() const override { return setup_priority::DATA; } + bool check(const Ts &...x) override { - if (!this->check_internal()) + auto now = millis(); + if (!this->check_internal_(now)) return false; - return millis() - this->last_inactive_ >= this->time_.value(x...); + return now - this->last_inactive_ >= this->time_.value(x...); } protected: + bool check_internal_(uint32_t now) { + bool cond = this->condition_->check(); + if (!cond) + this->last_inactive_ = now; + return cond; + } + Condition<> *condition_; uint32_t last_inactive_{0}; }; @@ -424,34 +430,17 @@ template class WaitUntilAction : public Action, public Co auto timeout = this->timeout_value_.optional_value(x...); this->var_queue_.emplace_front(now, timeout, std::make_tuple(x...)); - // Enable loop now that we have work to do - this->enable_loop(); - this->loop(); + // Do immediate check with fresh timestamp + if (this->process_queue_(now)) { + // Only enable loop if we still have pending items + this->enable_loop(); + } } void loop() override { - if (this->num_running_ == 0) - return; - - auto now = App.get_loop_component_start_time(); - - this->var_queue_.remove_if([&](auto &queued) { - auto start = std::get(queued); - auto timeout = std::get>(queued); - auto &var = std::get>(queued); - - auto expired = timeout && (now - start) >= *timeout; - - if (!expired && !this->condition_->check_tuple(var)) { - return false; - } - - this->play_next_tuple_(var); - return true; - }); - - // If queue is now empty, disable loop until next play_complex - if (this->var_queue_.empty()) { + // Safe to use cached time - only called from Application::loop() + if (this->num_running_ > 0 && !this->process_queue_(App.get_loop_component_start_time())) { + // If queue is now empty, disable loop until next play_complex this->disable_loop(); } } @@ -467,6 +456,31 @@ template class WaitUntilAction : public Action, public Co } protected: + // Helper: Process queue, triggering completed items and removing them + // Returns true if queue still has pending items + bool process_queue_(uint32_t now) { + // Process each queued wait_until and remove completed ones + this->var_queue_.remove_if([&](auto &queued) { + auto start = std::get(queued); + auto timeout = std::get>(queued); + auto &var = std::get>(queued); + + // Check if timeout has expired + auto expired = timeout && (now - start) >= *timeout; + + // Keep waiting if not expired and condition not met + if (!expired && !this->condition_->check_tuple(var)) { + return false; + } + + // Condition met or timed out - trigger next action + this->play_next_tuple_(var); + return true; + }); + + return !this->var_queue_.empty(); + } + Condition *condition_; std::forward_list, std::tuple>> var_queue_{}; }; diff --git a/tests/integration/fixtures/wait_until_mid_loop_timing.yaml b/tests/integration/fixtures/wait_until_mid_loop_timing.yaml new file mode 100644 index 0000000000..32f59e81a1 --- /dev/null +++ b/tests/integration/fixtures/wait_until_mid_loop_timing.yaml @@ -0,0 +1,109 @@ +# Test for PR #11676 bug: wait_until timeout when triggered mid-component-loop +# This demonstrates that App.get_loop_component_start_time() is stale when +# wait_until is triggered partway through a component's loop execution + +esphome: + name: wait-mid-loop + +host: + +api: + actions: + - action: test_mid_loop_timeout + then: + - logger.log: "=== Test: wait_until triggered mid-loop should timeout correctly ===" + + # Reset test state + - globals.set: + id: test_complete + value: 'false' + + # Trigger the slow script that will call wait_until mid-execution + - script.execute: slow_script + + # Wait for test to complete (should take ~300ms: 100ms delay + 200ms timeout) + - wait_until: + condition: + lambda: return id(test_complete); + timeout: 2s + + - if: + condition: + lambda: return id(test_complete); + then: + - logger.log: "✓ Test PASSED: wait_until timed out correctly" + else: + - logger.log: "✗ Test FAILED: wait_until did not complete properly" + +logger: + level: DEBUG + +globals: + - id: test_complete + type: bool + restore_value: false + initial_value: 'false' + + - id: test_condition + type: bool + restore_value: false + initial_value: 'false' + + - id: timeout_start_time + type: uint32_t + restore_value: false + initial_value: '0' + + - id: timeout_end_time + type: uint32_t + restore_value: false + initial_value: '0' + +script: + # This script simulates a component that takes time during its execution + # When wait_until is triggered mid-script, the loop_component_start_time + # will be stale (from when the script's component loop started) + - id: slow_script + then: + - logger.log: "Script: Starting, about to do some work..." + + # Simulate component doing work for 100ms + # This represents time spent in a component's loop() before triggering wait_until + - delay: 100ms + + - logger.log: "Script: 100ms elapsed, now starting wait_until with 200ms timeout" + - lambda: |- + // Record when timeout starts + id(timeout_start_time) = millis(); + id(test_condition) = false; + + # At this point: + # - Script component's loop started 100ms ago + # - App.loop_component_start_time_ = time from 100ms ago (stale!) + # - wait_until will capture millis() NOW (fresh) + # - BUG: loop() will use stale loop_component_start_time, causing immediate timeout + + - wait_until: + condition: + lambda: return id(test_condition); + timeout: 200ms + + - lambda: |- + // Record when timeout completes + id(timeout_end_time) = millis(); + uint32_t elapsed = id(timeout_end_time) - id(timeout_start_time); + + ESP_LOGD("TEST", "wait_until completed after %u ms (expected ~200ms)", elapsed); + + // Check if timeout took approximately correct time + // Should be ~200ms, not <50ms (immediate timeout) + if (elapsed >= 150 && elapsed <= 250) { + ESP_LOGD("TEST", "✓ Timeout duration correct: %u ms", elapsed); + id(test_complete) = true; + } else { + ESP_LOGE("TEST", "✗ Timeout duration WRONG: %u ms (expected 150-250ms)", elapsed); + if (elapsed < 50) { + ESP_LOGE("TEST", " → Likely BUG: Immediate timeout due to stale loop_component_start_time"); + } + id(test_complete) = false; + } diff --git a/tests/integration/test_wait_until_mid_loop_timing.py b/tests/integration/test_wait_until_mid_loop_timing.py new file mode 100644 index 0000000000..01cad747ae --- /dev/null +++ b/tests/integration/test_wait_until_mid_loop_timing.py @@ -0,0 +1,112 @@ +"""Integration test for PR #11676 mid-loop timing bug. + +This test validates that wait_until timeouts work correctly when triggered +mid-component-loop, where App.get_loop_component_start_time() is stale. + +The bug: When wait_until is triggered partway through a component's loop execution +(e.g., from a script or automation), the cached loop_component_start_time_ is stale +relative to when the action was actually triggered. This causes timeout calculations +to underflow and timeout immediately instead of waiting the specified duration. +""" + +from __future__ import annotations + +import asyncio +import re + +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_wait_until_mid_loop_timing( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that wait_until timeout works when triggered mid-component-loop. + + This test: + 1. Executes a script that delays 100ms (simulating component work) + 2. Then starts wait_until with 200ms timeout + 3. Verifies timeout takes ~200ms, not <50ms (immediate timeout bug) + """ + loop = asyncio.get_running_loop() + + # Track test results + test_results = { + "timeout_duration": None, + "passed": False, + "failed": False, + "bug_detected": False, + } + + # Patterns for log messages + timeout_duration = re.compile(r"wait_until completed after (\d+) ms") + test_pass = re.compile(r"✓ Timeout duration correct") + test_fail = re.compile(r"✗ Timeout duration WRONG") + bug_pattern = re.compile(r"Likely BUG: Immediate timeout") + test_passed = re.compile(r"✓ Test PASSED") + test_failed = re.compile(r"✗ Test FAILED") + + test_complete = loop.create_future() + + def check_output(line: str) -> None: + """Check log output for test results.""" + # Extract timeout duration + match = timeout_duration.search(line) + if match: + test_results["timeout_duration"] = int(match.group(1)) + + if test_pass.search(line): + test_results["passed"] = True + if test_fail.search(line): + test_results["failed"] = True + if bug_pattern.search(line): + test_results["bug_detected"] = True + + # Final test result + if ( + test_passed.search(line) + or test_failed.search(line) + and not test_complete.done() + ): + test_complete.set_result(True) + + async with ( + run_compiled(yaml_config, line_callback=check_output), + api_client_connected() as client, + ): + # Get the test service + _, services = await client.list_entities_services() + test_service = next( + (s for s in services if s.name == "test_mid_loop_timeout"), None + ) + assert test_service is not None, "test_mid_loop_timeout service not found" + + # Execute the test + client.execute_service(test_service, {}) + + # Wait for test to complete (100ms delay + 200ms timeout + margins = ~500ms) + await asyncio.wait_for(test_complete, timeout=5.0) + + # Verify results + assert test_results["timeout_duration"] is not None, ( + "Timeout duration not reported" + ) + assert test_results["passed"], ( + f"Test failed: wait_until took {test_results['timeout_duration']}ms, expected ~200ms. " + f"Bug detected: {test_results['bug_detected']}" + ) + assert not test_results["bug_detected"], ( + f"BUG DETECTED: wait_until timed out immediately ({test_results['timeout_duration']}ms) " + "instead of waiting 200ms. This indicates stale loop_component_start_time." + ) + + # Additional validation: timeout should be ~200ms (150-250ms range) + duration = test_results["timeout_duration"] + assert 150 <= duration <= 250, ( + f"Timeout duration {duration}ms outside expected range (150-250ms). " + f"This suggests timing regression from PR #11676." + ) From 70d947fab9cdb6a599d4b84844341a8be19a215e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Nov 2025 21:20:27 -0600 Subject: [PATCH 6/6] [esp32_ble] Store custom GAP device name in flash --- esphome/components/esp32_ble/ble.cpp | 4 ++-- esphome/components/esp32_ble/ble.h | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index 8bbb21e3ca..ce0aa8b2f5 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -257,8 +257,8 @@ bool ESP32BLE::ble_setup_() { #endif std::string name; - if (this->name_.has_value()) { - name = this->name_.value(); + if (this->name_ != nullptr) { + name = this->name_; if (App.is_name_add_mac_suffix_enabled()) { // MAC address suffix length (last 6 characters of 12-char MAC address string) constexpr size_t mac_address_suffix_len = 6; diff --git a/esphome/components/esp32_ble/ble.h b/esphome/components/esp32_ble/ble.h index 2fb60bb822..b6b6eb1fbc 100644 --- a/esphome/components/esp32_ble/ble.h +++ b/esphome/components/esp32_ble/ble.h @@ -112,7 +112,7 @@ class ESP32BLE : public Component { void loop() override; void dump_config() override; float get_setup_priority() const override; - void set_name(const std::string &name) { this->name_ = name; } + void set_name(const char *name) { this->name_ = name; } #ifdef USE_ESP32_BLE_ADVERTISING void advertising_start(); @@ -191,8 +191,8 @@ class ESP32BLE : public Component { esphome::LockFreeQueue ble_events_; esphome::EventPool ble_event_pool_; - // optional (typically 16+ bytes on 32-bit, aligned to 4 bytes) - optional name_; + // Pointer to compile-time string literal or nullptr (4 bytes on 32-bit) + const char *name_{nullptr}; // 4-byte aligned members #ifdef USE_ESP32_BLE_ADVERTISING