From 5f06679d78316582e09b8d0f6f56ef0dd8816b1a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:16:44 -1000 Subject: [PATCH 01/17] [espnow] Fix EventPool/LockFreeQueue sizing off-by-one (#14893) --- esphome/components/espnow/espnow_component.cpp | 6 ++++-- esphome/components/espnow/espnow_component.h | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/esphome/components/espnow/espnow_component.cpp b/esphome/components/espnow/espnow_component.cpp index 991803d870..78916891f4 100644 --- a/esphome/components/espnow/espnow_component.cpp +++ b/esphome/components/espnow/espnow_component.cpp @@ -87,7 +87,8 @@ void on_send_report(const uint8_t *mac_addr, esp_now_send_status_t status) // Push the packet to the queue global_esp_now->receive_packet_queue_.push(packet); - // Push always because we're the only producer and the pool ensures we never exceed queue size + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. // Wake main loop immediately to process ESP-NOW send event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) @@ -109,7 +110,8 @@ void on_data_received(const esp_now_recv_info_t *info, const uint8_t *data, int // Push the packet to the queue global_esp_now->receive_packet_queue_.push(packet); - // Push always because we're the only producer and the pool ensures we never exceed queue size + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. // Wake main loop immediately to process ESP-NOW receive event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) diff --git a/esphome/components/espnow/espnow_component.h b/esphome/components/espnow/espnow_component.h index 9941e97227..ee4adc1b4d 100644 --- a/esphome/components/espnow/espnow_component.h +++ b/esphome/components/espnow/espnow_component.h @@ -163,10 +163,14 @@ class ESPNowComponent : public Component { uint8_t own_address_[ESP_NOW_ETH_ALEN]{0}; LockFreeQueue receive_packet_queue_{}; - EventPool receive_packet_pool_{}; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements. This guarantees allocate() returns nullptr + // before push() can fail, preventing a pool slot leak. + EventPool receive_packet_pool_{}; LockFreeQueue send_packet_queue_{}; - EventPool send_packet_pool_{}; + // Pool sized to queue capacity (SIZE-1) — see receive_packet_pool_ comment. + EventPool send_packet_pool_{}; ESPNowSendPacket *current_send_packet_{nullptr}; // Currently sending packet, nullptr if none uint8_t wifi_channel_{0}; From 97382ed8148fbe42c18bfd1ed7c10cd76b3b922e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:17:43 -1000 Subject: [PATCH 02/17] [usb_cdc_acm] Fix EventPool/LockFreeQueue sizing off-by-one (#14894) --- .../components/usb_cdc_acm/usb_cdc_acm.cpp | 26 +++++++------------ esphome/components/usb_cdc_acm/usb_cdc_acm.h | 6 ++++- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/esphome/components/usb_cdc_acm/usb_cdc_acm.cpp b/esphome/components/usb_cdc_acm/usb_cdc_acm.cpp index a4c2e6c4a4..253626f0a3 100644 --- a/esphome/components/usb_cdc_acm/usb_cdc_acm.cpp +++ b/esphome/components/usb_cdc_acm/usb_cdc_acm.cpp @@ -26,16 +26,13 @@ void USBCDCACMInstance::queue_line_state_event(bool dtr, bool rts) { event->data.line_state.dtr = dtr; event->data.line_state.rts = rts; - if (!this->event_queue_.push(event)) { - ESP_LOGW(TAG, "Event queue full, line state event dropped (itf=%d)", this->itf_); - // Return event to pool since we couldn't queue it - this->event_pool_.release(event); - } else { - // Wake main loop immediately to process event + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. + this->event_queue_.push(event); + #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) - App.wake_loop_threadsafe(); + App.wake_loop_threadsafe(); #endif - } } void USBCDCACMInstance::queue_line_coding_event(uint32_t bit_rate, uint8_t stop_bits, uint8_t parity, @@ -53,16 +50,13 @@ void USBCDCACMInstance::queue_line_coding_event(uint32_t bit_rate, uint8_t stop_ event->data.line_coding.parity = parity; event->data.line_coding.data_bits = data_bits; - if (!this->event_queue_.push(event)) { - ESP_LOGW(TAG, "Event queue full, line coding event dropped (itf=%d)", this->itf_); - // Return event to pool since we couldn't queue it - this->event_pool_.release(event); - } else { - // Wake main loop immediately to process event + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. + this->event_queue_.push(event); + #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) - App.wake_loop_threadsafe(); + App.wake_loop_threadsafe(); #endif - } } void USBCDCACMInstance::process_events_() { diff --git a/esphome/components/usb_cdc_acm/usb_cdc_acm.h b/esphome/components/usb_cdc_acm/usb_cdc_acm.h index 020542e749..a56abc9ee8 100644 --- a/esphome/components/usb_cdc_acm/usb_cdc_acm.h +++ b/esphome/components/usb_cdc_acm/usb_cdc_acm.h @@ -102,7 +102,11 @@ class USBCDCACMInstance : public uart::UARTComponent, public Parented event_pool_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements. This guarantees allocate() returns nullptr + // before push() can fail, preventing both a pool slot leak and an SPSC + // violation on the pool's internal free list. + EventPool event_pool_; LockFreeQueue event_queue_; }; From c19c75220bbf2f20e37dec81ac18dc0735e5e058 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:17:59 -1000 Subject: [PATCH 03/17] [usb_host] Fix EventPool/LockFreeQueue sizing off-by-one (#14896) --- esphome/components/usb_host/usb_host.h | 5 ++++- esphome/components/usb_host/usb_host_client.cpp | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/esphome/components/usb_host/usb_host.h b/esphome/components/usb_host/usb_host.h index 2eec0c9699..dcb76a3a3b 100644 --- a/esphome/components/usb_host/usb_host.h +++ b/esphome/components/usb_host/usb_host.h @@ -144,7 +144,10 @@ class USBClient : public Component { // Lock-free event queue and pool for USB task to main loop communication // Must be public for access from static callbacks LockFreeQueue event_queue; - EventPool event_pool; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements. This guarantees allocate() returns nullptr + // before push() can fail, preventing a pool slot leak. + EventPool event_pool; protected: // Process USB events from the queue. Returns true if any work was done. diff --git a/esphome/components/usb_host/usb_host_client.cpp b/esphome/components/usb_host/usb_host_client.cpp index 2a460d1a07..18d938344c 100644 --- a/esphome/components/usb_host/usb_host_client.cpp +++ b/esphome/components/usb_host/usb_host_client.cpp @@ -193,7 +193,8 @@ static void client_event_cb(const usb_host_client_event_msg_t *event_msg, void * return; } - // Push to lock-free queue (always succeeds since pool size == queue size) + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. client->event_queue.push(event); // Re-enable component loop to process the queued event From a94bb74d043da0b1128c7a899a1765a3e13f6af0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:18:31 -1000 Subject: [PATCH 04/17] [usb_uart] Fix EventPool/LockFreeQueue sizing off-by-one (#14895) --- esphome/components/usb_uart/usb_uart.cpp | 11 +++++------ esphome/components/usb_uart/usb_uart.h | 8 ++++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/esphome/components/usb_uart/usb_uart.cpp b/esphome/components/usb_uart/usb_uart.cpp index 997f836146..a5d312f191 100644 --- a/esphome/components/usb_uart/usb_uart.cpp +++ b/esphome/components/usb_uart/usb_uart.cpp @@ -160,11 +160,9 @@ void USBUartChannel::write_array(const uint8_t *data, size_t len) { size_t chunk_len = std::min(len, UsbOutputChunk::MAX_CHUNK_SIZE); memcpy(chunk->data, data, chunk_len); chunk->length = static_cast(chunk_len); - if (!this->output_queue_.push(chunk)) { - this->output_pool_.release(chunk); - ESP_LOGE(TAG, "Output queue full - lost %zu bytes", len); - break; - } + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. + this->output_queue_.push(chunk); data += chunk_len; len -= chunk_len; } @@ -320,7 +318,8 @@ void USBUartComponent::start_input(USBUartChannel *channel) { chunk->channel = channel; // Push to lock-free queue for main loop processing - // Push always succeeds because pool size == queue size + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. this->usb_data_queue_.push(chunk); // Re-enable component loop to process the queued data diff --git a/esphome/components/usb_uart/usb_uart.h b/esphome/components/usb_uart/usb_uart.h index 16469df7f6..7a06b04f11 100644 --- a/esphome/components/usb_uart/usb_uart.h +++ b/esphome/components/usb_uart/usb_uart.h @@ -158,7 +158,10 @@ class USBUartChannel : public uart::UARTComponent, public Parented output_queue_; - EventPool output_pool_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements. This guarantees allocate() returns nullptr + // before push() can fail, preventing a pool slot leak. + EventPool output_pool_; std::function rx_callback_{}; CdcEps cdc_dev_{}; StringRef debug_prefix_{}; @@ -190,7 +193,8 @@ class USBUartComponent : public usb_host::USBClient { // Lock-free data transfer from USB task to main loop static constexpr int USB_DATA_QUEUE_SIZE = 32; LockFreeQueue usb_data_queue_; - EventPool chunk_pool_; + // Pool sized to queue capacity (SIZE-1) — see USBUartChannel::output_pool_ comment. + EventPool chunk_pool_; protected: std::vector channels_{}; From 1adf05e2d59b60e7b22d0a0efedd5f9181f1fa12 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:24:02 -1000 Subject: [PATCH 05/17] [esp32_ble] Fix EventPool/LockFreeQueue sizing off-by-one (#14892) --- esphome/components/esp32_ble/ble.cpp | 3 ++- esphome/components/esp32_ble/ble.h | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index ff9d9bb15a..fee1c546be 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -575,8 +575,9 @@ template void enqueue_ble_event(Args... args) { load_ble_event(event, args...); // Push the event to the queue + // Push always succeeds: pool is sized to queue capacity (N-1), so if + // allocate() returned non-null, the queue is guaranteed to have room. global_ble->ble_events_.push(event); - // Push always succeeds because we're the only producer and the pool ensures we never exceed queue size } // Explicit template instantiations for the friend function diff --git a/esphome/components/esp32_ble/ble.h b/esphome/components/esp32_ble/ble.h index 04bec3f785..752ddc9d1f 100644 --- a/esphome/components/esp32_ble/ble.h +++ b/esphome/components/esp32_ble/ble.h @@ -221,7 +221,13 @@ class ESP32BLE : public Component { // Large objects (size depends on template parameters, but typically aligned to 4 bytes) esphome::LockFreeQueue ble_events_; - esphome::EventPool ble_event_pool_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements (one slot distinguishes full from empty). + // This guarantees allocate() returns nullptr before push() can fail, which: + // 1. Prevents leaking a pool slot (the Nth allocate succeeds but push fails) + // 2. Avoids needing release() on the producer path after a failed push(), + // preserving the SPSC contract on the pool's internal free list + esphome::EventPool ble_event_pool_; // 4-byte aligned members #ifdef USE_ESP32_BLE_ADVERTISING From 1670f04a8715380be9452e37d0b0c133c092c5f8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 12:29:38 -1000 Subject: [PATCH 06/17] [core] Add CodSpeed C++ benchmarks for protobuf, main loop, and helpers (#14878) --- .github/workflows/ci.yml | 34 ++ esphome/core/component.h | 2 + script/clang-tidy | 3 + script/cpp_benchmark.py | 130 ++++++++ script/determine-jobs.py | 61 ++++ script/setup_codspeed_lib.py | 231 ++++++++++++++ tests/benchmarks/components/.gitignore | 2 + .../components/api/bench_proto_decode.cpp | 93 ++++++ .../components/api/bench_proto_encode.cpp | 298 ++++++++++++++++++ .../components/api/bench_proto_varint.cpp | 133 ++++++++ .../benchmarks/components/api/benchmark.yaml | 114 +++++++ tests/benchmarks/components/main.cpp | 42 +++ .../core/bench_application_loop.cpp | 22 ++ tests/benchmarks/core/bench_helpers.cpp | 41 +++ tests/benchmarks/core/bench_logger.cpp | 54 ++++ tests/benchmarks/core/bench_scheduler.cpp | 133 ++++++++ tests/script/test_determine_jobs.py | 148 +++++++++ 17 files changed, 1541 insertions(+) create mode 100755 script/cpp_benchmark.py create mode 100755 script/setup_codspeed_lib.py create mode 100644 tests/benchmarks/components/.gitignore create mode 100644 tests/benchmarks/components/api/bench_proto_decode.cpp create mode 100644 tests/benchmarks/components/api/bench_proto_encode.cpp create mode 100644 tests/benchmarks/components/api/bench_proto_varint.cpp create mode 100644 tests/benchmarks/components/api/benchmark.yaml create mode 100644 tests/benchmarks/components/main.cpp create mode 100644 tests/benchmarks/core/bench_application_loop.cpp create mode 100644 tests/benchmarks/core/bench_helpers.cpp create mode 100644 tests/benchmarks/core/bench_logger.cpp create mode 100644 tests/benchmarks/core/bench_scheduler.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f7710589c5..1926ad5bf4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -185,6 +185,7 @@ jobs: cpp-unit-tests-run-all: ${{ steps.determine.outputs.cpp-unit-tests-run-all }} cpp-unit-tests-components: ${{ steps.determine.outputs.cpp-unit-tests-components }} component-test-batches: ${{ steps.determine.outputs.component-test-batches }} + benchmarks: ${{ steps.determine.outputs.benchmarks }} steps: - name: Check out code from GitHub uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -227,6 +228,7 @@ jobs: echo "cpp-unit-tests-run-all=$(echo "$output" | jq -r '.cpp_unit_tests_run_all')" >> $GITHUB_OUTPUT echo "cpp-unit-tests-components=$(echo "$output" | jq -c '.cpp_unit_tests_components')" >> $GITHUB_OUTPUT echo "component-test-batches=$(echo "$output" | jq -c '.component_test_batches')" >> $GITHUB_OUTPUT + echo "benchmarks=$(echo "$output" | jq -r '.benchmarks')" >> $GITHUB_OUTPUT - name: Save components graph cache if: github.ref == 'refs/heads/dev' uses: actions/cache/save@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 @@ -308,6 +310,38 @@ jobs: script/cpp_unit_test.py $ARGS fi + benchmarks: + name: Run CodSpeed benchmarks + runs-on: ubuntu-24.04 + needs: + - common + - determine-jobs + if: github.event_name == 'pull_request' && needs.determine-jobs.outputs.benchmarks == 'true' + steps: + - name: Check out code from GitHub + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Restore Python + uses: ./.github/actions/restore-python + with: + python-version: ${{ env.DEFAULT_PYTHON }} + cache-key: ${{ needs.common.outputs.cache-key }} + + - name: Build benchmarks + id: build + run: | + . venv/bin/activate + export BENCHMARK_LIB_CONFIG=$(python script/setup_codspeed_lib.py) + # --build-only prints BUILD_BINARY= to stdout + BINARY=$(script/cpp_benchmark.py --all --build-only | grep '^BUILD_BINARY=' | tail -1 | cut -d= -f2-) + echo "binary=$BINARY" >> $GITHUB_OUTPUT + + - name: Run CodSpeed benchmarks + uses: CodSpeedHQ/action@281164b0f014a4e7badd2c02cecad9b595b70537 # v4 + with: + run: ${{ steps.build.outputs.binary }} + mode: simulation + clang-tidy-single: name: ${{ matrix.name }} runs-on: ubuntu-24.04 diff --git a/esphome/core/component.h b/esphome/core/component.h index 5fdf23e128..557ba09bbc 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -598,9 +598,11 @@ class WarnIfComponentBlockingGuard { #ifdef USE_RUNTIME_STATS this->record_runtime_stats_(); #endif +#ifndef USE_BENCHMARK if (blocking_time > WARN_IF_BLOCKING_OVER_MS) [[unlikely]] { warn_blocking(this->component_, blocking_time); } +#endif return curr_time; } diff --git a/script/clang-tidy b/script/clang-tidy index 9c2899026d..f2834b44ac 100755 --- a/script/clang-tidy +++ b/script/clang-tidy @@ -231,6 +231,9 @@ def main(): cwd = os.getcwd() files = [os.path.relpath(path, cwd) for path in git_ls_files(["*.cpp"])] + # Exclude benchmark files — they require google benchmark headers not + # available in the ESP32 toolchain and use different naming conventions. + files = [f for f in files if not f.startswith("tests/benchmarks/")] # Print initial file count if it's large if len(files) > 50: diff --git a/script/cpp_benchmark.py b/script/cpp_benchmark.py new file mode 100755 index 0000000000..bd92266ea6 --- /dev/null +++ b/script/cpp_benchmark.py @@ -0,0 +1,130 @@ +#!/usr/bin/env python3 +"""Build and run C++ benchmarks for ESPHome components using Google Benchmark.""" + +import argparse +import json +import os +from pathlib import Path +import sys + +from helpers import root_path +from test_helpers import ( + BASE_CODEGEN_COMPONENTS, + PLATFORMIO_GOOGLE_BENCHMARK_LIB, + USE_TIME_TIMEZONE_FLAG, + build_and_run, +) + +# Path to /tests/benchmarks/components +BENCHMARKS_DIR: Path = Path(root_path) / "tests" / "benchmarks" / "components" + +# Path to /tests/benchmarks/core (always included, not a component) +CORE_BENCHMARKS_DIR: Path = Path(root_path) / "tests" / "benchmarks" / "core" + +# Additional codegen components beyond the base set. +# json is needed because its to_code adds the ArduinoJson library +# (auto-loaded by api, but cpp_testing suppresses to_code unless listed). +BENCHMARK_CODEGEN_COMPONENTS = BASE_CODEGEN_COMPONENTS | {"json"} + +PLATFORMIO_OPTIONS = { + "build_unflags": [ + "-Os", # remove default size-opt + ], + "build_flags": [ + "-O2", # optimize for speed (CodSpeed recommends RelWithDebInfo) + "-g", # debug symbols for profiling + USE_TIME_TIMEZONE_FLAG, + "-DUSE_BENCHMARK", # disable WarnIfComponentBlockingGuard in finish() + ], + # Use deep+ LDF mode to ensure PlatformIO detects the benchmark + # library dependency from nested includes. + "lib_ldf_mode": "deep+", +} + + +def run_benchmarks(selected_components: list[str], build_only: bool = False) -> int: + # Allow CI to override the benchmark library (e.g. with CodSpeed's fork). + # BENCHMARK_LIB_CONFIG is a JSON string from setup_codspeed_lib.py + # containing {"lib_path": "/path/to/google_benchmark"}. + lib_config_json = os.environ.get("BENCHMARK_LIB_CONFIG") + + pio_options = PLATFORMIO_OPTIONS + if lib_config_json: + lib_config = json.loads(lib_config_json) + benchmark_lib = f"benchmark=symlink://{lib_config['lib_path']}" + # These defines must be global (not just in library.json) because + # benchmark.h uses #ifdef CODSPEED_ENABLED to switch benchmark + # registration to CodSpeed-instrumented variants, and + # CODSPEED_ROOT_DIR is used to display relative file paths in reports. + project_root = Path(__file__).resolve().parent.parent + codspeed_flags = [ + "-DNDEBUG", + "-DCODSPEED_ENABLED", + "-DCODSPEED_ANALYSIS", + f'-DCODSPEED_ROOT_DIR=\\"{project_root}\\"', + ] + pio_options = { + **PLATFORMIO_OPTIONS, + "build_flags": PLATFORMIO_OPTIONS["build_flags"] + codspeed_flags, + } + else: + benchmark_lib = PLATFORMIO_GOOGLE_BENCHMARK_LIB + + return build_and_run( + selected_components=selected_components, + tests_dir=BENCHMARKS_DIR, + codegen_components=BENCHMARK_CODEGEN_COMPONENTS, + config_prefix="cppbench", + friendly_name="CPP Benchmarks", + libraries=benchmark_lib, + platformio_options=pio_options, + main_entry="main.cpp", + label="benchmarks", + build_only=build_only, + extra_include_dirs=[CORE_BENCHMARKS_DIR], + ) + + +def main() -> None: + parser = argparse.ArgumentParser( + description="Build and run C++ benchmarks for ESPHome components." + ) + parser.add_argument( + "components", + nargs="*", + help="List of components to benchmark (must have files in tests/benchmarks/components/).", + ) + parser.add_argument( + "--all", + action="store_true", + help="Benchmark all components with benchmark files.", + ) + parser.add_argument( + "--build-only", + action="store_true", + help="Only build, print binary path without running.", + ) + + args = parser.parse_args() + + if args.all: + # Find all component directories that have .cpp files + components: list[str] = ( + sorted( + d.name + for d in BENCHMARKS_DIR.iterdir() + if d.is_dir() + and d.name != "__pycache__" + and (any(d.glob("*.cpp")) or any(d.glob("*.h"))) + ) + if BENCHMARKS_DIR.is_dir() + else [] + ) + else: + components: list[str] = args.components + + sys.exit(run_benchmarks(components, build_only=args.build_only)) + + +if __name__ == "__main__": + main() diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 6808a3cf6c..ad08f8dce5 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -381,6 +381,63 @@ def determine_cpp_unit_tests( return (False, get_cpp_changed_components(cpp_files)) +# Paths within tests/benchmarks/ that contain component benchmark files +BENCHMARKS_COMPONENTS_PATH = "tests/benchmarks/components" + +# Files that, when changed, should trigger benchmark runs +BENCHMARK_INFRASTRUCTURE_FILES = frozenset( + { + "script/cpp_benchmark.py", + "script/test_helpers.py", + "script/setup_codspeed_lib.py", + } +) + + +def should_run_benchmarks(branch: str | None = None) -> bool: + """Determine if C++ benchmarks should run based on changed files. + + Benchmarks run when any of the following conditions are met: + + 1. Core C++ files changed (esphome/core/*) + 2. A directly changed component has benchmark files (no dependency expansion) + 3. Benchmark infrastructure changed (tests/benchmarks/*, script/cpp_benchmark.py, + script/test_helpers.py, script/setup_codspeed_lib.py) + + Unlike unit tests, benchmarks do NOT expand to dependent components. + Changing ``sensor`` does not trigger ``api`` benchmarks just because + api depends on sensor. + + Args: + branch: Branch to compare against. If None, uses default. + + Returns: + True if benchmarks should run, False otherwise. + """ + files = changed_files(branch) + if core_changed(files): + return True + + # Check if benchmark infrastructure changed + if any( + f.startswith("tests/benchmarks/") or f in BENCHMARK_INFRASTRUCTURE_FILES + for f in files + ): + return True + + # Check if any directly changed component has benchmarks + benchmarks_dir = Path(root_path) / BENCHMARKS_COMPONENTS_PATH + if not benchmarks_dir.is_dir(): + return False + benchmarked_components = { + d.name + for d in benchmarks_dir.iterdir() + if d.is_dir() and (any(d.glob("*.cpp")) or any(d.glob("*.h"))) + } + # Only direct changes — no dependency expansion + return any(get_component_from_path(f) in benchmarked_components for f in files) + + def _any_changed_file_endswith(branch: str | None, extensions: tuple[str, ...]) -> bool: """Check if a changed file ends with any of the specified extensions.""" return any(file.endswith(extensions) for file in changed_files(branch)) @@ -804,6 +861,9 @@ def main() -> None: # Determine which C++ unit tests to run cpp_run_all, cpp_components = determine_cpp_unit_tests(args.branch) + # Determine if benchmarks should run + run_benchmarks = should_run_benchmarks(args.branch) + # Split components into batches for CI testing # This intelligently groups components with similar bus configurations component_test_batches: list[str] @@ -856,6 +916,7 @@ def main() -> None: "cpp_unit_tests_run_all": cpp_run_all, "cpp_unit_tests_components": cpp_components, "component_test_batches": component_test_batches, + "benchmarks": run_benchmarks, } # Output as JSON diff --git a/script/setup_codspeed_lib.py b/script/setup_codspeed_lib.py new file mode 100755 index 0000000000..959c89d05b --- /dev/null +++ b/script/setup_codspeed_lib.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +"""Set up CodSpeed's google_benchmark fork as a PlatformIO library. + +CodSpeed requires their codspeed-cpp fork for CPU simulation instrumentation. +This script clones the repo and assembles a flat PlatformIO-compatible library +by combining google_benchmark sources, codspeed core, and instrument-hooks. + +PlatformIO quirks addressed: + - .cc files renamed to .cpp (PlatformIO ignores .cc) + - All sources merged into one src/ dir (PlatformIO can't compile from + multiple source directories in a single library) + - library.json created with required CodSpeed preprocessor defines + +Usage: + python script/setup_codspeed_lib.py [--output-dir DIR] + +Prints JSON to stdout with lib_path for cpp_benchmark.py. +Git output goes to stderr. + +See https://codspeed.io/docs/benchmarks/cpp#custom-build-systems +""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path +import shutil +import subprocess +import sys + +# Pin to a specific release for reproducibility +CODSPEED_CPP_REPO = "https://github.com/CodSpeedHQ/codspeed-cpp.git" +CODSPEED_CPP_SHA = "e633aca00da3d0ad14e7bf424d9cb47165a29028" # v2.1.0 + +DEFAULT_OUTPUT_DIR = "/tmp/codspeed-cpp" + +# Well-known paths within the codspeed-cpp repository +GOOGLE_BENCHMARK_SUBDIR = "google_benchmark" +CORE_SUBDIR = "core" +INSTRUMENT_HOOKS_SUBDIR = Path(CORE_SUBDIR) / "instrument-hooks" +INSTRUMENT_HOOKS_INCLUDES = INSTRUMENT_HOOKS_SUBDIR / "includes" +INSTRUMENT_HOOKS_DIST = INSTRUMENT_HOOKS_SUBDIR / "dist" / "core.c" +CORE_CMAKE = Path(CORE_SUBDIR) / "CMakeLists.txt" + + +def _git(args: list[str], **kwargs: object) -> None: + """Run a git command, sending output to stderr.""" + subprocess.run( + ["git", *args], + check=True, + stdout=kwargs.pop("stdout", sys.stderr), + stderr=kwargs.pop("stderr", sys.stderr), + **kwargs, + ) + + +def _clone_repo(output_dir: Path) -> None: + """Shallow-clone codspeed-cpp at the pinned SHA with submodules.""" + output_dir.mkdir(parents=True, exist_ok=True) + _git(["init", str(output_dir)]) + _git(["-C", str(output_dir), "remote", "add", "origin", CODSPEED_CPP_REPO]) + _git(["-C", str(output_dir), "fetch", "--depth", "1", "origin", CODSPEED_CPP_SHA]) + _git( + ["-C", str(output_dir), "checkout", "FETCH_HEAD"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + _git( + [ + "-C", + str(output_dir), + "submodule", + "update", + "--init", + "--recursive", + "--depth", + "1", + ] + ) + + +def _read_codspeed_version(cmake_path: Path) -> str: + """Extract CODSPEED_VERSION from core/CMakeLists.txt.""" + if not cmake_path.exists(): + return "0.0.0" + for line in cmake_path.read_text().splitlines(): + if line.startswith("set(CODSPEED_VERSION"): + return line.split()[1].rstrip(")") + return "0.0.0" + + +def _rename_cc_to_cpp(src_dir: Path) -> None: + """Rename .cc files to .cpp so PlatformIO compiles them.""" + for cc_file in src_dir.glob("*.cc"): + cpp_file = cc_file.with_suffix(".cpp") + if not cpp_file.exists(): + cc_file.rename(cpp_file) + + +def _copy_if_missing(src: Path, dest: Path) -> None: + """Copy a file only if the destination doesn't already exist.""" + if not dest.exists(): + shutil.copy2(src, dest) + + +def _merge_codspeed_core_into_lib(core_src: Path, lib_src: Path) -> None: + """Copy codspeed core sources into the benchmark library src/. + + .cpp files get a ``codspeed_`` prefix to avoid name collisions with + google_benchmark's own sources. .h files keep their original names + since they're referenced by ``#include "walltime.h"`` etc. + """ + for src_file in core_src.iterdir(): + if src_file.suffix == ".cpp": + _copy_if_missing(src_file, lib_src / f"codspeed_{src_file.name}") + elif src_file.suffix == ".h": + _copy_if_missing(src_file, lib_src / src_file.name) + + +def _write_library_json( + benchmark_dir: Path, + core_include: Path, + hooks_include: Path, + version: str, + project_root: Path, +) -> None: + """Write a PlatformIO library.json with CodSpeed build flags.""" + library_json = { + "name": "benchmark", + "version": "0.0.0", + "build": { + "flags": [ + f"-I{core_include}", + f"-I{hooks_include}", + # google benchmark build flags + # -O2 is critical: without it, instrument_hooks_start_benchmark_inline + # doesn't get inlined and shows up as overhead in profiles + "-O2", + "-DNDEBUG", + "-DHAVE_STD_REGEX", + "-DHAVE_STEADY_CLOCK", + "-DBENCHMARK_STATIC_DEFINE", + # CodSpeed instrumentation flags + # https://codspeed.io/docs/benchmarks/cpp#custom-build-systems + "-DCODSPEED_ENABLED", + "-DCODSPEED_ANALYSIS", + f'-DCODSPEED_VERSION=\\"{version}\\"', + f'-DCODSPEED_ROOT_DIR=\\"{project_root}\\"', + '-DCODSPEED_MODE_DISPLAY=\\"simulation\\"', + ], + "includeDir": "include", + }, + } + (benchmark_dir / "library.json").write_text( + json.dumps(library_json, indent=2) + "\n" + ) + + +def setup_codspeed_lib(output_dir: Path) -> None: + """Clone codspeed-cpp and assemble a flat PlatformIO library. + + The resulting library at ``output_dir/google_benchmark/`` contains: + - google_benchmark sources (.cc renamed to .cpp) + - codspeed core sources (prefixed ``codspeed_``) + - instrument-hooks C source (as ``instrument_hooks.c``) + - library.json with all required CodSpeed defines + + Args: + output_dir: Directory to clone the repository into + """ + if not (output_dir / ".git").exists(): + _clone_repo(output_dir) + else: + # Verify the existing checkout matches the pinned SHA + result = subprocess.run( + ["git", "-C", str(output_dir), "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0 or result.stdout.strip() != CODSPEED_CPP_SHA: + print( + f"Stale codspeed-cpp checkout, re-cloning at {CODSPEED_CPP_SHA}", + file=sys.stderr, + ) + shutil.rmtree(output_dir) + _clone_repo(output_dir) + + benchmark_dir = output_dir / GOOGLE_BENCHMARK_SUBDIR + lib_src = benchmark_dir / "src" + core_dir = output_dir / CORE_SUBDIR + core_include = core_dir / "include" + hooks_include = output_dir / INSTRUMENT_HOOKS_INCLUDES + hooks_dist_c = output_dir / INSTRUMENT_HOOKS_DIST + project_root = Path(__file__).resolve().parent.parent + + # 1. Rename .cc → .cpp (PlatformIO doesn't compile .cc) + _rename_cc_to_cpp(lib_src) + + # 2. Merge codspeed core sources into the library + _merge_codspeed_core_into_lib(core_dir / "src", lib_src) + + # 3. Copy instrument-hooks C source (provides instrument_hooks_* symbols) + if hooks_dist_c.exists(): + _copy_if_missing(hooks_dist_c, lib_src / "instrument_hooks.c") + + # 4. Write library.json + version = _read_codspeed_version(output_dir / CORE_CMAKE) + _write_library_json( + benchmark_dir, core_include, hooks_include, version, project_root + ) + + # Output JSON config for cpp_benchmark.py + print(json.dumps({"lib_path": str(benchmark_dir)})) + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--output-dir", + type=Path, + default=Path(DEFAULT_OUTPUT_DIR), + help=f"Directory to clone codspeed-cpp into (default: {DEFAULT_OUTPUT_DIR})", + ) + args = parser.parse_args() + setup_codspeed_lib(args.output_dir) + + +if __name__ == "__main__": + main() diff --git a/tests/benchmarks/components/.gitignore b/tests/benchmarks/components/.gitignore new file mode 100644 index 0000000000..163bec7b80 --- /dev/null +++ b/tests/benchmarks/components/.gitignore @@ -0,0 +1,2 @@ +/.esphome/ +/secrets.yaml diff --git a/tests/benchmarks/components/api/bench_proto_decode.cpp b/tests/benchmarks/components/api/bench_proto_decode.cpp new file mode 100644 index 0000000000..113201dd8a --- /dev/null +++ b/tests/benchmarks/components/api/bench_proto_decode.cpp @@ -0,0 +1,93 @@ +#include + +#include "esphome/components/api/api_pb2.h" +#include "esphome/components/api/api_buffer.h" + +namespace esphome::api::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +// Helper: encode a message into a buffer and return it. +// Benchmarks encode once in setup, then decode the resulting bytes in a loop. +// This keeps decode benchmarks in sync with the actual protobuf schema — +// hand-encoded byte arrays would silently break when fields change. +template static APIBuffer encode_message(const T &msg) { + APIBuffer buffer; + uint32_t size = msg.calculate_size(); + buffer.resize(size); + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + return buffer; +} + +// --- HelloRequest decode (string + varint fields) --- + +static void Decode_HelloRequest(benchmark::State &state) { + HelloRequest source; + source.client_info = StringRef::from_lit("aioesphomeapi"); + source.api_version_major = 1; + source.api_version_minor = 10; + auto encoded = encode_message(source); + + for (auto _ : state) { + HelloRequest msg; + for (int i = 0; i < kInnerIterations; i++) { + msg.decode(encoded.data(), encoded.size()); + } + benchmark::DoNotOptimize(msg.api_version_major); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Decode_HelloRequest); + +// --- SwitchCommandRequest decode (simple command) --- + +static void Decode_SwitchCommandRequest(benchmark::State &state) { + SwitchCommandRequest source; + source.key = 0x12345678; + source.state = true; + auto encoded = encode_message(source); + + for (auto _ : state) { + SwitchCommandRequest msg; + for (int i = 0; i < kInnerIterations; i++) { + msg.decode(encoded.data(), encoded.size()); + } + benchmark::DoNotOptimize(msg.state); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Decode_SwitchCommandRequest); + +// --- LightCommandRequest decode (complex command with many fields) --- + +static void Decode_LightCommandRequest(benchmark::State &state) { + LightCommandRequest source; + source.key = 0x11223344; + source.has_state = true; + source.state = true; + source.has_brightness = true; + source.brightness = 0.8f; + source.has_rgb = true; + source.red = 1.0f; + source.green = 0.5f; + source.blue = 0.2f; + source.has_effect = true; + source.effect = StringRef::from_lit("rainbow"); + auto encoded = encode_message(source); + + for (auto _ : state) { + LightCommandRequest msg; + for (int i = 0; i < kInnerIterations; i++) { + msg.decode(encoded.data(), encoded.size()); + } + benchmark::DoNotOptimize(msg.brightness); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Decode_LightCommandRequest); + +} // namespace esphome::api::benchmarks diff --git a/tests/benchmarks/components/api/bench_proto_encode.cpp b/tests/benchmarks/components/api/bench_proto_encode.cpp new file mode 100644 index 0000000000..656c1e17db --- /dev/null +++ b/tests/benchmarks/components/api/bench_proto_encode.cpp @@ -0,0 +1,298 @@ +#include + +#include "esphome/components/api/api_pb2.h" +#include "esphome/components/api/api_buffer.h" + +namespace esphome::api::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +// --- SensorStateResponse (highest frequency message) --- + +static void Encode_SensorStateResponse(benchmark::State &state) { + APIBuffer buffer; + SensorStateResponse msg; + msg.key = 0x12345678; + msg.state = 23.5f; + msg.missing_state = false; + uint32_t size = msg.calculate_size(); + buffer.resize(size); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_SensorStateResponse); + +static void CalculateSize_SensorStateResponse(benchmark::State &state) { + SensorStateResponse msg; + msg.key = 0x12345678; + msg.state = 23.5f; + msg.missing_state = false; + + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += msg.calculate_size(); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalculateSize_SensorStateResponse); + +// Steady state: buffer already allocated from previous iteration +static void CalcAndEncode_SensorStateResponse(benchmark::State &state) { + APIBuffer buffer; + SensorStateResponse msg; + msg.key = 0x12345678; + msg.state = 23.5f; + msg.missing_state = false; + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + uint32_t size = msg.calculate_size(); + buffer.resize(size); + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalcAndEncode_SensorStateResponse); + +// Cold path: fresh buffer each iteration (measures heap allocation cost). +// Inner loop still needed to amortize CodSpeed instrumentation overhead. +// Each inner iteration creates a fresh buffer, so this measures +// alloc+calc+encode per item. +static void CalcAndEncode_SensorStateResponse_Fresh(benchmark::State &state) { + SensorStateResponse msg; + msg.key = 0x12345678; + msg.state = 23.5f; + msg.missing_state = false; + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + APIBuffer buffer; + uint32_t size = msg.calculate_size(); + buffer.resize(size); + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + benchmark::DoNotOptimize(buffer.data()); + } + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalcAndEncode_SensorStateResponse_Fresh); + +// --- BinarySensorStateResponse --- + +static void Encode_BinarySensorStateResponse(benchmark::State &state) { + APIBuffer buffer; + BinarySensorStateResponse msg; + msg.key = 0xAABBCCDD; + msg.state = true; + msg.missing_state = false; + uint32_t size = msg.calculate_size(); + buffer.resize(size); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_BinarySensorStateResponse); + +// --- HelloResponse (string fields) --- + +static void Encode_HelloResponse(benchmark::State &state) { + APIBuffer buffer; + HelloResponse msg; + msg.api_version_major = 1; + msg.api_version_minor = 10; + msg.server_info = StringRef::from_lit("esphome v2026.3.0"); + msg.name = StringRef::from_lit("living-room-sensor"); + uint32_t size = msg.calculate_size(); + buffer.resize(size); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_HelloResponse); + +// --- LightStateResponse (complex multi-field message) --- + +static void Encode_LightStateResponse(benchmark::State &state) { + APIBuffer buffer; + LightStateResponse msg; + msg.key = 0x11223344; + msg.state = true; + msg.brightness = 0.8f; + msg.color_mode = enums::COLOR_MODE_RGB_WHITE; + msg.color_brightness = 1.0f; + msg.red = 1.0f; + msg.green = 0.5f; + msg.blue = 0.2f; + msg.white = 0.0f; + msg.color_temperature = 4000.0f; + msg.cold_white = 0.0f; + msg.warm_white = 0.0f; + msg.effect = StringRef::from_lit("rainbow"); + uint32_t size = msg.calculate_size(); + buffer.resize(size); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_LightStateResponse); + +static void CalculateSize_LightStateResponse(benchmark::State &state) { + LightStateResponse msg; + msg.key = 0x11223344; + msg.state = true; + msg.brightness = 0.8f; + msg.color_mode = enums::COLOR_MODE_RGB_WHITE; + msg.color_brightness = 1.0f; + msg.red = 1.0f; + msg.green = 0.5f; + msg.blue = 0.2f; + msg.white = 0.0f; + msg.color_temperature = 4000.0f; + msg.cold_white = 0.0f; + msg.warm_white = 0.0f; + msg.effect = StringRef::from_lit("rainbow"); + + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += msg.calculate_size(); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalculateSize_LightStateResponse); + +// --- DeviceInfoResponse (nested submessages: 20 devices + 20 areas) --- + +static DeviceInfoResponse make_device_info_response() { + DeviceInfoResponse msg; + msg.name = StringRef::from_lit("living-room-sensor"); + msg.mac_address = StringRef::from_lit("AA:BB:CC:DD:EE:FF"); + msg.esphome_version = StringRef::from_lit("2026.3.0"); + msg.compilation_time = StringRef::from_lit("Mar 16 2026, 12:00:00"); + msg.model = StringRef::from_lit("esp32-poe-iso"); + msg.manufacturer = StringRef::from_lit("Olimex"); + msg.friendly_name = StringRef::from_lit("Living Room Sensor"); +#ifdef USE_DEVICES + for (uint32_t i = 0; i < ESPHOME_DEVICE_COUNT && i < 20; i++) { + msg.devices[i].device_id = i + 1; + msg.devices[i].name = StringRef::from_lit("device"); + msg.devices[i].area_id = (i % 20) + 1; + } +#endif +#ifdef USE_AREAS + for (uint32_t i = 0; i < ESPHOME_AREA_COUNT && i < 20; i++) { + msg.areas[i].area_id = i + 1; + msg.areas[i].name = StringRef::from_lit("area"); + } +#endif + return msg; +} + +static void CalculateSize_DeviceInfoResponse(benchmark::State &state) { + auto msg = make_device_info_response(); + + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += msg.calculate_size(); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalculateSize_DeviceInfoResponse); + +static void Encode_DeviceInfoResponse(benchmark::State &state) { + auto msg = make_device_info_response(); + APIBuffer buffer; + uint32_t total_size = msg.calculate_size(); + buffer.resize(total_size); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_DeviceInfoResponse); + +// Steady state: buffer already allocated from previous iteration +static void CalcAndEncode_DeviceInfoResponse(benchmark::State &state) { + auto msg = make_device_info_response(); + APIBuffer buffer; + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + uint32_t size = msg.calculate_size(); + buffer.resize(size); + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalcAndEncode_DeviceInfoResponse); + +// Cold path: fresh buffer each iteration (measures heap allocation cost). +// Inner loop still needed to amortize CodSpeed instrumentation overhead. +// Each inner iteration creates a fresh buffer, so this measures +// alloc+calc+encode per item. +static void CalcAndEncode_DeviceInfoResponse_Fresh(benchmark::State &state) { + auto msg = make_device_info_response(); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + APIBuffer buffer; + uint32_t size = msg.calculate_size(); + buffer.resize(size); + ProtoWriteBuffer writer(&buffer, 0); + msg.encode(writer); + benchmark::DoNotOptimize(buffer.data()); + } + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(CalcAndEncode_DeviceInfoResponse_Fresh); + +} // namespace esphome::api::benchmarks diff --git a/tests/benchmarks/components/api/bench_proto_varint.cpp b/tests/benchmarks/components/api/bench_proto_varint.cpp new file mode 100644 index 0000000000..0b5ccc2b7d --- /dev/null +++ b/tests/benchmarks/components/api/bench_proto_varint.cpp @@ -0,0 +1,133 @@ +#include + +#include "esphome/components/api/proto.h" +#include "esphome/components/api/api_buffer.h" + +namespace esphome::api::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +// --- ProtoVarInt::parse() benchmarks --- + +static void ProtoVarInt_Parse_SingleByte(benchmark::State &state) { + uint8_t buf[] = {0x42}; // value = 66 + + for (auto _ : state) { + ProtoVarIntResult result{}; + for (int i = 0; i < kInnerIterations; i++) { + result = ProtoVarInt::parse(buf, sizeof(buf)); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(ProtoVarInt_Parse_SingleByte); + +static void ProtoVarInt_Parse_TwoByte(benchmark::State &state) { + uint8_t buf[] = {0x80, 0x01}; // value = 128 + + for (auto _ : state) { + ProtoVarIntResult result{}; + for (int i = 0; i < kInnerIterations; i++) { + result = ProtoVarInt::parse(buf, sizeof(buf)); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(ProtoVarInt_Parse_TwoByte); + +static void ProtoVarInt_Parse_FiveByte(benchmark::State &state) { + uint8_t buf[] = {0xFF, 0xFF, 0xFF, 0xFF, 0x0F}; + + for (auto _ : state) { + ProtoVarIntResult result{}; + for (int i = 0; i < kInnerIterations; i++) { + result = ProtoVarInt::parse(buf, sizeof(buf)); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(ProtoVarInt_Parse_FiveByte); + +// --- Varint encoding benchmarks --- + +static void Encode_Varint_Small(benchmark::State &state) { + APIBuffer buffer; + buffer.resize(16); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + writer.encode_varint_raw(42); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_Varint_Small); + +static void Encode_Varint_Large(benchmark::State &state) { + APIBuffer buffer; + buffer.resize(16); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + writer.encode_varint_raw(300); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_Varint_Large); + +static void Encode_Varint_MaxUint32(benchmark::State &state) { + APIBuffer buffer; + buffer.resize(16); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ProtoWriteBuffer writer(&buffer, 0); + writer.encode_varint_raw(0xFFFFFFFF); + } + benchmark::DoNotOptimize(buffer.data()); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Encode_Varint_MaxUint32); + +// --- ProtoSize::varint() benchmarks --- + +static void ProtoSize_Varint_Small(benchmark::State &state) { + // Use varying input to prevent constant folding. + // Values 0-127 all take 1 byte but the compiler can't prove that. + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += ProtoSize::varint(static_cast(i) & 0x7F); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(ProtoSize_Varint_Small); + +static void ProtoSize_Varint_Large(benchmark::State &state) { + // Use varying input to prevent constant folding. + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += ProtoSize::varint(0xFFFF0000 | static_cast(i)); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(ProtoSize_Varint_Large); + +} // namespace esphome::api::benchmarks diff --git a/tests/benchmarks/components/api/benchmark.yaml b/tests/benchmarks/components/api/benchmark.yaml new file mode 100644 index 0000000000..bfc24d7440 --- /dev/null +++ b/tests/benchmarks/components/api/benchmark.yaml @@ -0,0 +1,114 @@ +# Components needed for API protobuf benchmarks. +# Merged into the base config before validation so all +# dependencies get proper defaults. +# +# esphome: sub-keys are merged into the base config. +esphome: + areas: + - id: area_1 + name: "Area 1" + - id: area_2 + name: "Area 2" + - id: area_3 + name: "Area 3" + - id: area_4 + name: "Area 4" + - id: area_5 + name: "Area 5" + - id: area_6 + name: "Area 6" + - id: area_7 + name: "Area 7" + - id: area_8 + name: "Area 8" + - id: area_9 + name: "Area 9" + - id: area_10 + name: "Area 10" + - id: area_11 + name: "Area 11" + - id: area_12 + name: "Area 12" + - id: area_13 + name: "Area 13" + - id: area_14 + name: "Area 14" + - id: area_15 + name: "Area 15" + - id: area_16 + name: "Area 16" + - id: area_17 + name: "Area 17" + - id: area_18 + name: "Area 18" + - id: area_19 + name: "Area 19" + - id: area_20 + name: "Area 20" + devices: + - id: device_1 + name: "Device 1" + area_id: area_1 + - id: device_2 + name: "Device 2" + area_id: area_2 + - id: device_3 + name: "Device 3" + area_id: area_3 + - id: device_4 + name: "Device 4" + area_id: area_4 + - id: device_5 + name: "Device 5" + area_id: area_5 + - id: device_6 + name: "Device 6" + area_id: area_6 + - id: device_7 + name: "Device 7" + area_id: area_7 + - id: device_8 + name: "Device 8" + area_id: area_8 + - id: device_9 + name: "Device 9" + area_id: area_9 + - id: device_10 + name: "Device 10" + area_id: area_10 + - id: device_11 + name: "Device 11" + area_id: area_11 + - id: device_12 + name: "Device 12" + area_id: area_12 + - id: device_13 + name: "Device 13" + area_id: area_13 + - id: device_14 + name: "Device 14" + area_id: area_14 + - id: device_15 + name: "Device 15" + area_id: area_15 + - id: device_16 + name: "Device 16" + area_id: area_16 + - id: device_17 + name: "Device 17" + area_id: area_17 + - id: device_18 + name: "Device 18" + area_id: area_18 + - id: device_19 + name: "Device 19" + area_id: area_19 + - id: device_20 + name: "Device 20" + area_id: area_20 + +api: +sensor: +binary_sensor: +light: +switch: diff --git a/tests/benchmarks/components/main.cpp b/tests/benchmarks/components/main.cpp new file mode 100644 index 0000000000..9bc0c31a15 --- /dev/null +++ b/tests/benchmarks/components/main.cpp @@ -0,0 +1,42 @@ +#include + +#include "esphome/components/logger/logger.h" + +/* +This special main.cpp provides the entry point for Google Benchmark. +It replaces the default ESPHome main with a benchmark runner. + +*/ + +// Auto generated code by esphome +// ========== AUTO GENERATED INCLUDE BLOCK BEGIN =========== +// ========== AUTO GENERATED INCLUDE BLOCK END =========== + +void original_setup() { + // Code-generated App initialization (pre_setup, area/device registration, etc.) + + // ========== AUTO GENERATED CODE BEGIN =========== + // =========== AUTO GENERATED CODE END ============ +} + +void setup() { + // Run auto-generated initialization (App.pre_setup, area/device registration, + // looping_components_.init, etc.) so benchmarks that use App work correctly. + original_setup(); + + // Log functions call global_logger->log_vprintf_() without a null check, + // so we must set up a Logger before any test that triggers logging. + static esphome::logger::Logger test_logger(0); + test_logger.set_log_level(ESPHOME_LOG_LEVEL); + test_logger.pre_setup(); + + int argc = 1; + char arg0[] = "benchmark"; + char *argv[] = {arg0, nullptr}; + ::benchmark::Initialize(&argc, argv); + ::benchmark::RunSpecifiedBenchmarks(); + ::benchmark::Shutdown(); + exit(0); +} + +void loop() {} diff --git a/tests/benchmarks/core/bench_application_loop.cpp b/tests/benchmarks/core/bench_application_loop.cpp new file mode 100644 index 0000000000..dde78ae739 --- /dev/null +++ b/tests/benchmarks/core/bench_application_loop.cpp @@ -0,0 +1,22 @@ +#include + +#include "esphome/core/application.h" + +namespace esphome::benchmarks { + +// Benchmark Application::loop() with no registered components. +// App is initialized by original_setup() in main.cpp (code-generated +// pre_setup, area/device registration, looping_components_.init). +// This measures the baseline overhead of the main loop: scheduler, +// timing, before/after loop tasks, and yield_with_select_. +static void ApplicationLoop_Empty(benchmark::State &state) { + // Set loop interval to 0 so yield_with_select_ returns immediately + // instead of sleeping. This benchmarks the loop overhead, not the sleep. + App.set_loop_interval(0); + for (auto _ : state) { + App.loop(); + } +} +BENCHMARK(ApplicationLoop_Empty); + +} // namespace esphome::benchmarks diff --git a/tests/benchmarks/core/bench_helpers.cpp b/tests/benchmarks/core/bench_helpers.cpp new file mode 100644 index 0000000000..c6e1e6930e --- /dev/null +++ b/tests/benchmarks/core/bench_helpers.cpp @@ -0,0 +1,41 @@ +#include + +#include "esphome/core/helpers.h" + +namespace esphome::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +// --- random_float() --- +// Ported from ol.yaml:148 "Random Float Benchmark" + +static void RandomFloat(benchmark::State &state) { + for (auto _ : state) { + float result = 0.0f; + for (int i = 0; i < kInnerIterations; i++) { + result += random_float(); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(RandomFloat); + +// --- random_uint32() --- + +static void RandomUint32(benchmark::State &state) { + for (auto _ : state) { + uint32_t result = 0; + for (int i = 0; i < kInnerIterations; i++) { + result += random_uint32(); + } + benchmark::DoNotOptimize(result); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(RandomUint32); + +} // namespace esphome::benchmarks diff --git a/tests/benchmarks/core/bench_logger.cpp b/tests/benchmarks/core/bench_logger.cpp new file mode 100644 index 0000000000..b7e9a1c4ea --- /dev/null +++ b/tests/benchmarks/core/bench_logger.cpp @@ -0,0 +1,54 @@ +#include + +#include "esphome/core/log.h" + +namespace esphome::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +static const char *const TAG = "bench"; + +// --- Log a message with no format specifiers (fastest path) --- + +static void Logger_NoFormat(benchmark::State &state) { + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ESP_LOGW(TAG, "Something happened"); + } + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Logger_NoFormat); + +// --- Log a message with 3 uint32_t format specifiers --- + +static void Logger_3Uint32(benchmark::State &state) { + uint32_t a = 12345, b = 67890, c = 99999; + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ESP_LOGW(TAG, "Values: %" PRIu32 " %" PRIu32 " %" PRIu32, a, b, c); + } + benchmark::DoNotOptimize(a); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Logger_3Uint32); + +// --- Log a message with 3 floats (common for sensor values) --- + +static void Logger_3Float(benchmark::State &state) { + float temp = 23.456f, humidity = 67.89f, pressure = 1013.25f; + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + ESP_LOGW(TAG, "Sensor: %.2f %.1f %.2f", temp, humidity, pressure); + } + benchmark::DoNotOptimize(temp); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Logger_3Float); + +} // namespace esphome::benchmarks diff --git a/tests/benchmarks/core/bench_scheduler.cpp b/tests/benchmarks/core/bench_scheduler.cpp new file mode 100644 index 0000000000..764f17ed73 --- /dev/null +++ b/tests/benchmarks/core/bench_scheduler.cpp @@ -0,0 +1,133 @@ +#include + +#include "esphome/core/scheduler.h" +#include "esphome/core/hal.h" + +namespace esphome::benchmarks { + +// Inner iteration count to amortize CodSpeed instrumentation overhead. +// Without this, the ~60ns per-iteration valgrind start/stop cost dominates +// sub-microsecond benchmarks. +static constexpr int kInnerIterations = 2000; + +// --- Scheduler fast path: no work to do --- + +static void Scheduler_Call_NoWork(benchmark::State &state) { + Scheduler scheduler; + uint32_t now = millis(); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + scheduler.call(now); + } + benchmark::DoNotOptimize(now); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Scheduler_Call_NoWork); + +// --- Scheduler with timers: call() when timers exist but aren't due --- + +static void Scheduler_Call_TimersNotDue(benchmark::State &state) { + Scheduler scheduler; + Component dummy_component; + + // Add some timeouts far in the future + for (int i = 0; i < 10; i++) { + scheduler.set_timeout(&dummy_component, static_cast(i), 1000000, []() {}); + } + scheduler.process_to_add(); + + uint32_t now = millis(); + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + scheduler.call(now); + } + benchmark::DoNotOptimize(now); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Scheduler_Call_TimersNotDue); + +// --- Scheduler with 5 intervals firing every call --- + +static void Scheduler_Call_5IntervalsFiring(benchmark::State &state) { + Scheduler scheduler; + Component dummy_component; + int fire_count = 0; + + // Benchmarks the heap-based scheduler dispatch with 5 callbacks firing. + // Uses monotonically increasing fake time so intervals reliably fire every call. + // USE_BENCHMARK ifdef in component.h disables WarnIfComponentBlockingGuard + // (fake now > real millis() would cause underflow in finish()). + // interval=0 would cause an infinite loop (reschedules at same now). + for (int i = 0; i < 5; i++) { + scheduler.set_interval(&dummy_component, static_cast(i), 1, [&fire_count]() { fire_count++; }); + } + scheduler.process_to_add(); + + uint32_t now = millis() + 100; + + for (auto _ : state) { + scheduler.call(now); + now++; + benchmark::DoNotOptimize(fire_count); + } +} +BENCHMARK(Scheduler_Call_5IntervalsFiring); + +// --- Scheduler: set_timeout registration --- + +static void Scheduler_SetTimeout(benchmark::State &state) { + Scheduler scheduler; + Component dummy_component; + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + scheduler.set_timeout(&dummy_component, static_cast(i % 5), 1000, []() {}); + } + scheduler.process_to_add(); + benchmark::DoNotOptimize(scheduler); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Scheduler_SetTimeout); + +// --- Scheduler: set_interval registration --- + +static void Scheduler_SetInterval(benchmark::State &state) { + Scheduler scheduler; + Component dummy_component; + + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + scheduler.set_interval(&dummy_component, static_cast(i % 5), 1000, []() {}); + } + scheduler.process_to_add(); + benchmark::DoNotOptimize(scheduler); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Scheduler_SetInterval); + +// --- Scheduler: defer registration (set_timeout with delay=0) --- + +static void Scheduler_Defer(benchmark::State &state) { + Scheduler scheduler; + Component dummy_component; + + // defer() is Component::defer which calls set_timeout(delay=0). + // Call set_timeout directly since defer() is protected. + for (auto _ : state) { + for (int i = 0; i < kInnerIterations; i++) { + scheduler.set_timeout(&dummy_component, static_cast(i % 5), 0, []() {}); + } + scheduler.process_to_add(); + benchmark::DoNotOptimize(scheduler); + } + state.SetItemsProcessed(state.iterations() * kInnerIterations); +} +BENCHMARK(Scheduler_Defer); + +} // namespace esphome::benchmarks diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 5c81ad374b..29535d1fd3 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1821,3 +1821,151 @@ def test_component_batching_beta_branch_40_per_batch( all_components.extend(batch_str.split()) assert len(all_components) == 120 assert set(all_components) == set(component_names) + + +# --- should_run_benchmarks tests --- + + +def test_should_run_benchmarks_core_change() -> None: + """Test benchmarks trigger on core C++ file changes.""" + with patch.object( + determine_jobs, "changed_files", return_value=["esphome/core/scheduler.cpp"] + ): + assert determine_jobs.should_run_benchmarks() is True + + +def test_should_run_benchmarks_core_header_change() -> None: + """Test benchmarks trigger on core header changes.""" + with patch.object( + determine_jobs, "changed_files", return_value=["esphome/core/helpers.h"] + ): + assert determine_jobs.should_run_benchmarks() is True + + +def test_should_run_benchmarks_benchmark_infra_change() -> None: + """Test benchmarks trigger on benchmark infrastructure changes.""" + for infra_file in [ + "script/cpp_benchmark.py", + "script/test_helpers.py", + "script/setup_codspeed_lib.py", + ]: + with patch.object(determine_jobs, "changed_files", return_value=[infra_file]): + assert determine_jobs.should_run_benchmarks() is True, ( + f"Expected benchmarks to run for {infra_file}" + ) + + +def test_should_run_benchmarks_benchmark_file_change() -> None: + """Test benchmarks trigger on benchmark file changes.""" + with patch.object( + determine_jobs, + "changed_files", + return_value=["tests/benchmarks/components/api/bench_proto_encode.cpp"], + ): + assert determine_jobs.should_run_benchmarks() is True + + +def test_should_run_benchmarks_core_benchmark_file_change() -> None: + """Test benchmarks trigger on core benchmark file changes.""" + with patch.object( + determine_jobs, + "changed_files", + return_value=["tests/benchmarks/core/bench_scheduler.cpp"], + ): + assert determine_jobs.should_run_benchmarks() is True + + +def test_should_run_benchmarks_benchmarked_component_change(tmp_path: Path) -> None: + """Test benchmarks trigger when a benchmarked component changes.""" + # Create a fake benchmarks directory with an 'api' component + benchmarks_dir = tmp_path / "tests" / "benchmarks" / "components" / "api" + benchmarks_dir.mkdir(parents=True) + (benchmarks_dir / "bench_proto_encode.cpp").write_text("// benchmark") + + with ( + patch.object( + determine_jobs, + "changed_files", + return_value=["esphome/components/api/proto.h"], + ), + patch.object(determine_jobs, "root_path", str(tmp_path)), + patch.object( + determine_jobs, + "BENCHMARKS_COMPONENTS_PATH", + "tests/benchmarks/components", + ), + ): + assert determine_jobs.should_run_benchmarks() is True + + +def test_should_run_benchmarks_non_benchmarked_component_change( + tmp_path: Path, +) -> None: + """Test benchmarks do NOT trigger for non-benchmarked component changes.""" + # Create a fake benchmarks directory with only 'api' + benchmarks_dir = tmp_path / "tests" / "benchmarks" / "components" / "api" + benchmarks_dir.mkdir(parents=True) + (benchmarks_dir / "bench_proto_encode.cpp").write_text("// benchmark") + + with ( + patch.object( + determine_jobs, + "changed_files", + return_value=["esphome/components/sensor/__init__.py"], + ), + patch.object(determine_jobs, "root_path", str(tmp_path)), + patch.object( + determine_jobs, + "BENCHMARKS_COMPONENTS_PATH", + "tests/benchmarks/components", + ), + ): + assert determine_jobs.should_run_benchmarks() is False + + +def test_should_run_benchmarks_no_dependency_expansion(tmp_path: Path) -> None: + """Test benchmarks do NOT expand to dependent components. + + Changing 'sensor' should not trigger 'api' benchmarks even if api + depends on sensor. This is intentional — benchmark runs should be + targeted to directly changed components only. + """ + benchmarks_dir = tmp_path / "tests" / "benchmarks" / "components" / "api" + benchmarks_dir.mkdir(parents=True) + (benchmarks_dir / "bench_proto_encode.cpp").write_text("// benchmark") + + with ( + patch.object( + determine_jobs, + "changed_files", + # sensor is a dependency of api, but benchmarks don't expand + return_value=["esphome/components/sensor/sensor.cpp"], + ), + patch.object(determine_jobs, "root_path", str(tmp_path)), + patch.object( + determine_jobs, + "BENCHMARKS_COMPONENTS_PATH", + "tests/benchmarks/components", + ), + ): + assert determine_jobs.should_run_benchmarks() is False + + +def test_should_run_benchmarks_unrelated_change() -> None: + """Test benchmarks do NOT trigger for unrelated changes.""" + with patch.object(determine_jobs, "changed_files", return_value=["README.md"]): + assert determine_jobs.should_run_benchmarks() is False + + +def test_should_run_benchmarks_no_changes() -> None: + """Test benchmarks do NOT trigger with no changes.""" + with patch.object(determine_jobs, "changed_files", return_value=[]): + assert determine_jobs.should_run_benchmarks() is False + + +def test_should_run_benchmarks_with_branch() -> None: + """Test should_run_benchmarks passes branch to changed_files.""" + with patch.object(determine_jobs, "changed_files") as mock_changed: + mock_changed.return_value = [] + determine_jobs.should_run_benchmarks("release") + mock_changed.assert_called_with("release") From 6b91df8d756686cbcd9e575733402be78f02d929 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:05:16 -1000 Subject: [PATCH 07/17] [esp32_ble][esp32_ble_server] Inline is_active/is_running and remove STL bloat (#14875) --- esphome/components/esp32_ble/ble.cpp | 2 -- esphome/components/esp32_ble/ble.h | 2 +- .../components/esp32_ble_server/ble_server.cpp | 16 +++++++--------- esphome/components/esp32_ble_server/ble_server.h | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index fee1c546be..317f8fd11b 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -81,8 +81,6 @@ void ESP32BLE::disable() { this->state_ = BLE_COMPONENT_STATE_DISABLE; } -bool ESP32BLE::is_active() { return this->state_ == BLE_COMPONENT_STATE_ACTIVE; } - #ifdef USE_ESP32_BLE_ADVERTISING void ESP32BLE::advertising_start() { this->advertising_init_(); diff --git a/esphome/components/esp32_ble/ble.h b/esphome/components/esp32_ble/ble.h index 752ddc9d1f..82b2789461 100644 --- a/esphome/components/esp32_ble/ble.h +++ b/esphome/components/esp32_ble/ble.h @@ -135,7 +135,7 @@ class ESP32BLE : public Component { void enable(); void disable(); - bool is_active(); + ESPHOME_ALWAYS_INLINE bool is_active() { return this->state_ == BLE_COMPONENT_STATE_ACTIVE; } void setup() override; void loop() override; void dump_config() override; diff --git a/esphome/components/esp32_ble_server/ble_server.cpp b/esphome/components/esp32_ble_server/ble_server.cpp index ecc53e197f..be0691dc06 100644 --- a/esphome/components/esp32_ble_server/ble_server.cpp +++ b/esphome/components/esp32_ble_server/ble_server.cpp @@ -7,7 +7,6 @@ #ifdef USE_ESP32 -#include #include #include #include @@ -39,16 +38,17 @@ void BLEServer::loop() { case RUNNING: { // Start all services that are pending to start if (!this->services_to_start_.empty()) { - for (auto &service : this->services_to_start_) { + size_t write_idx = 0; + for (auto *service : this->services_to_start_) { if (service->is_created()) { service->start(); // Needs to be called once per characteristic in the service } + // Remove services that have started or are starting + if (!service->is_starting() && !service->is_running()) { + this->services_to_start_[write_idx++] = service; + } } - // Remove services that have been started - this->services_to_start_.erase( - std::remove_if(this->services_to_start_.begin(), this->services_to_start_.end(), - [](BLEService *service) { return service->is_starting() || service->is_running(); }), - this->services_to_start_.end()); + this->services_to_start_.erase(this->services_to_start_.begin() + write_idx, this->services_to_start_.end()); } break; } @@ -91,8 +91,6 @@ void BLEServer::loop() { } } -bool BLEServer::is_running() { return this->parent_->is_active() && this->state_ == RUNNING; } - bool BLEServer::can_proceed() { return this->is_running() || !this->parent_->is_active(); } void BLEServer::restart_advertising_() { diff --git a/esphome/components/esp32_ble_server/ble_server.h b/esphome/components/esp32_ble_server/ble_server.h index ff7e0044e4..1b419d2ee4 100644 --- a/esphome/components/esp32_ble_server/ble_server.h +++ b/esphome/components/esp32_ble_server/ble_server.h @@ -32,7 +32,7 @@ class BLEServer : public Component, public GATTsEventHandler, public BLEStatusEv float get_setup_priority() const override; bool can_proceed() override; - bool is_running(); + ESPHOME_ALWAYS_INLINE bool is_running() { return this->parent_->is_active() && this->state_ == RUNNING; } void set_manufacturer_data(const std::vector &data) { this->manufacturer_data_ = data; From 77b7201eb88434531cba130c2772c9d40623b833 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:08:45 -1000 Subject: [PATCH 08/17] [ci] Run CodSpeed benchmarks on push to dev for baseline (#14899) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1926ad5bf4..0a03579abc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -316,7 +316,9 @@ jobs: needs: - common - determine-jobs - if: github.event_name == 'pull_request' && needs.determine-jobs.outputs.benchmarks == 'true' + if: >- + (github.event_name == 'push' && github.ref_name == 'dev') || + (github.event_name == 'pull_request' && needs.determine-jobs.outputs.benchmarks == 'true') steps: - name: Check out code from GitHub uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 From f3409acfa85a8bd7f7cda4d1a76a6deb07f4a0c0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:08:58 -1000 Subject: [PATCH 09/17] [core] Document EventPool sizing requirement with LockFreeQueue (#14897) --- esphome/core/event_pool.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/esphome/core/event_pool.h b/esphome/core/event_pool.h index 99541d4a17..ee8e81225a 100644 --- a/esphome/core/event_pool.h +++ b/esphome/core/event_pool.h @@ -13,6 +13,14 @@ namespace esphome { // Events are allocated on first use and reused thereafter, growing to peak usage // @tparam T The type of objects managed by the pool (must have a release() method) // @tparam SIZE The maximum number of objects in the pool (1-255, limited by uint8_t) +// +// SIZING: When paired with a LockFreeQueue, the pool SIZE should be +// Q_SIZE - 1 (the queue's actual capacity, since the ring buffer reserves one slot). +// This ensures allocate() returns nullptr before push() can fail, which: +// - Prevents the allocate-succeeds-but-push-fails mismatch that permanently +// leaks a pool slot (the element is never returned to the pool) +// - Avoids needing release() on the producer path after a failed push(), +// preserving the SPSC contract on the internal free list template class EventPool { public: EventPool() : total_created_(0) {} From ece235218fe77108170d640b7320337d13c2260a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:27:46 -1000 Subject: [PATCH 10/17] [debug][bme680_bsec] Use fnv1_hash_extend to avoid temporary string allocations (#14876) --- esphome/components/bme680_bsec/bme680_bsec.cpp | 2 +- esphome/components/debug/debug_esp32.cpp | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/esphome/components/bme680_bsec/bme680_bsec.cpp b/esphome/components/bme680_bsec/bme680_bsec.cpp index 75efb6835a..bb0417b823 100644 --- a/esphome/components/bme680_bsec/bme680_bsec.cpp +++ b/esphome/components/bme680_bsec/bme680_bsec.cpp @@ -521,7 +521,7 @@ int BME680BSECComponent::reinit_bsec_lib_() { } void BME680BSECComponent::load_state_() { - uint32_t hash = fnv1_hash("bme680_bsec_state_" + this->device_id_); + uint32_t hash = fnv1_hash_extend(fnv1_hash("bme680_bsec_state_"), this->device_id_); this->bsec_state_ = global_preferences->make_preference(hash, true); if (!this->bsec_state_.load(&this->bsec_state_data_)) { diff --git a/esphome/components/debug/debug_esp32.cpp b/esphome/components/debug/debug_esp32.cpp index aa379599c6..2e04090749 100644 --- a/esphome/components/debug/debug_esp32.cpp +++ b/esphome/components/debug/debug_esp32.cpp @@ -49,7 +49,8 @@ static const size_t REBOOT_MAX_LEN = 24; void DebugComponent::on_shutdown() { auto *component = App.get_current_component(); char buffer[REBOOT_MAX_LEN]{}; - auto pref = global_preferences->make_preference(REBOOT_MAX_LEN, fnv1_hash(REBOOT_KEY + App.get_name())); + auto pref = global_preferences->make_preference(REBOOT_MAX_LEN, + fnv1_hash_extend(fnv1_hash(REBOOT_KEY), App.get_name().c_str())); if (component != nullptr) { strncpy(buffer, LOG_STR_ARG(component->get_component_log_str()), REBOOT_MAX_LEN - 1); buffer[REBOOT_MAX_LEN - 1] = '\0'; @@ -66,7 +67,8 @@ const char *DebugComponent::get_reset_reason_(std::spanmake_preference(REBOOT_MAX_LEN, fnv1_hash(REBOOT_KEY + App.get_name())); + auto pref = global_preferences->make_preference(REBOOT_MAX_LEN, + fnv1_hash_extend(fnv1_hash(REBOOT_KEY), App.get_name().c_str())); char reboot_source[REBOOT_MAX_LEN]{}; if (pref.load(&reboot_source)) { reboot_source[REBOOT_MAX_LEN - 1] = '\0'; From 83484a8828f2bb4936e41d069700c3b12cf2fa49 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:38:41 -1000 Subject: [PATCH 11/17] [esp32_ble_server] Remove vestigial semaphore from BLECharacteristic (#14900) --- .../components/esp32_ble_server/ble_characteristic.cpp | 10 +--------- .../components/esp32_ble_server/ble_characteristic.h | 4 ---- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/esphome/components/esp32_ble_server/ble_characteristic.cpp b/esphome/components/esp32_ble_server/ble_characteristic.cpp index 1806354712..aa82b773ba 100644 --- a/esphome/components/esp32_ble_server/ble_characteristic.cpp +++ b/esphome/components/esp32_ble_server/ble_characteristic.cpp @@ -16,13 +16,9 @@ BLECharacteristic::~BLECharacteristic() { for (auto *descriptor : this->descriptors_) { delete descriptor; // NOLINT(cppcoreguidelines-owning-memory) } - vSemaphoreDelete(this->set_value_lock_); } BLECharacteristic::BLECharacteristic(const ESPBTUUID uuid, uint32_t properties) : uuid_(uuid) { - this->set_value_lock_ = xSemaphoreCreateBinary(); - xSemaphoreGive(this->set_value_lock_); - this->properties_ = (esp_gatt_char_prop_t) 0; this->set_broadcast_property((properties & PROPERTY_BROADCAST) != 0); @@ -35,11 +31,7 @@ BLECharacteristic::BLECharacteristic(const ESPBTUUID uuid, uint32_t properties) void BLECharacteristic::set_value(ByteBuffer buffer) { this->set_value(buffer.get_data()); } -void BLECharacteristic::set_value(std::vector &&buffer) { - xSemaphoreTake(this->set_value_lock_, 0L); - this->value_ = std::move(buffer); - xSemaphoreGive(this->set_value_lock_); -} +void BLECharacteristic::set_value(std::vector &&buffer) { this->value_ = std::move(buffer); } void BLECharacteristic::set_value(std::initializer_list data) { this->set_value(std::vector(data)); // Delegate to move overload diff --git a/esphome/components/esp32_ble_server/ble_characteristic.h b/esphome/components/esp32_ble_server/ble_characteristic.h index 72897d1dfb..062052cdf8 100644 --- a/esphome/components/esp32_ble_server/ble_characteristic.h +++ b/esphome/components/esp32_ble_server/ble_characteristic.h @@ -16,8 +16,6 @@ #include #include #include -#include -#include namespace esphome { namespace esp32_ble_server { @@ -84,8 +82,6 @@ class BLECharacteristic { uint16_t value_read_offset_{0}; std::vector value_; - SemaphoreHandle_t set_value_lock_; - std::vector descriptors_; struct ClientNotificationEntry { From 53bfb02a21487d70b4070a75a2709fcad2e3ff58 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 17 Mar 2026 19:46:26 -0400 Subject: [PATCH 12/17] [sensor][ee895][hdc2010] Fix misc bugs found during component scan (#14890) --- esphome/components/ee895/ee895.cpp | 15 ++--- esphome/components/hdc2010/hdc2010.cpp | 76 +++++++++++--------------- esphome/components/sensor/__init__.py | 4 +- 3 files changed, 40 insertions(+), 55 deletions(-) diff --git a/esphome/components/ee895/ee895.cpp b/esphome/components/ee895/ee895.cpp index 22f28be9bc..93e5d4203b 100644 --- a/esphome/components/ee895/ee895.cpp +++ b/esphome/components/ee895/ee895.cpp @@ -24,7 +24,7 @@ void EE895Component::setup() { this->read(serial_number, 20); crc16_check = (serial_number[19] << 8) + serial_number[18]; - if (crc16_check != calc_crc16_(serial_number, 19)) { + if (crc16_check != calc_crc16_(serial_number, 18)) { this->error_code_ = CRC_CHECK_FAILED; this->mark_failed(); return; @@ -84,7 +84,7 @@ void EE895Component::write_command_(uint16_t addr, uint16_t reg_cnt) { address[2] = addr & 0xFF; address[3] = (reg_cnt >> 8) & 0xFF; address[4] = reg_cnt & 0xFF; - crc16 = calc_crc16_(address, 6); + crc16 = calc_crc16_(address, 5); address[5] = crc16 & 0xFF; address[6] = (crc16 >> 8) & 0xFF; this->write(address, 7); @@ -95,7 +95,7 @@ float EE895Component::read_float_() { uint8_t i2c_response[8]; this->read(i2c_response, 8); crc16_check = (i2c_response[7] << 8) + i2c_response[6]; - if (crc16_check != calc_crc16_(i2c_response, 7)) { + if (crc16_check != calc_crc16_(i2c_response, 6)) { this->error_code_ = CRC_CHECK_FAILED; this->status_set_warning(); return 0; @@ -107,12 +107,9 @@ float EE895Component::read_float_() { } uint16_t EE895Component::calc_crc16_(const uint8_t buf[], uint8_t len) { - uint8_t crc_check_buf[22]; - for (int i = 0; i < len; i++) { - crc_check_buf[i + 1] = buf[i]; - } - crc_check_buf[0] = this->address_; - return crc16(crc_check_buf, len); + uint8_t addr = this->address_; + uint16_t crc = crc16(&addr, 1); + return crc16(buf, len, crc); } } // namespace ee895 } // namespace esphome diff --git a/esphome/components/hdc2010/hdc2010.cpp b/esphome/components/hdc2010/hdc2010.cpp index c53fdb3f5b..0334b30eec 100644 --- a/esphome/components/hdc2010/hdc2010.cpp +++ b/esphome/components/hdc2010/hdc2010.cpp @@ -7,50 +7,36 @@ namespace hdc2010 { static const char *const TAG = "hdc2010"; -static const uint8_t HDC2010_ADDRESS = 0x40; // 0b1000000 or 0b1000001 from datasheet -static const uint8_t HDC2010_CMD_CONFIGURATION_MEASUREMENT = 0x8F; -static const uint8_t HDC2010_CMD_START_MEASUREMENT = 0xF9; -static const uint8_t HDC2010_CMD_TEMPERATURE_LOW = 0x00; -static const uint8_t HDC2010_CMD_TEMPERATURE_HIGH = 0x01; -static const uint8_t HDC2010_CMD_HUMIDITY_LOW = 0x02; -static const uint8_t HDC2010_CMD_HUMIDITY_HIGH = 0x03; -static const uint8_t CONFIG = 0x0E; -static const uint8_t MEASUREMENT_CONFIG = 0x0F; +// Register addresses +static constexpr uint8_t REG_TEMPERATURE_LOW = 0x00; +static constexpr uint8_t REG_TEMPERATURE_HIGH = 0x01; +static constexpr uint8_t REG_HUMIDITY_LOW = 0x02; +static constexpr uint8_t REG_HUMIDITY_HIGH = 0x03; +static constexpr uint8_t REG_RESET_DRDY_INT_CONF = 0x0E; +static constexpr uint8_t REG_MEASUREMENT_CONF = 0x0F; + +// REG_MEASUREMENT_CONF (0x0F) bit masks +static constexpr uint8_t MEAS_TRIG = 0x01; // Bit 0: measurement trigger +static constexpr uint8_t MEAS_CONF_MASK = 0x06; // Bits 2:1: measurement mode +static constexpr uint8_t HRES_MASK = 0x30; // Bits 5:4: humidity resolution +static constexpr uint8_t TRES_MASK = 0xC0; // Bits 7:6: temperature resolution + +// REG_RESET_DRDY_INT_CONF (0x0E) bit masks +static constexpr uint8_t AMM_MASK = 0x70; // Bits 6:4: auto measurement mode void HDC2010Component::setup() { ESP_LOGCONFIG(TAG, "Running setup"); - const uint8_t data[2] = { - 0b00000000, // resolution 14bit for both humidity and temperature - 0b00000000 // reserved - }; - - if (!this->write_bytes(HDC2010_CMD_CONFIGURATION_MEASUREMENT, data, 2)) { - ESP_LOGW(TAG, "Initial config instruction error"); - this->status_set_warning(); - return; - } - - // Set measurement mode to temperature and humidity + // Set 14-bit resolution for both sensors and measurement mode to temp + humidity uint8_t config_contents; - this->read_register(MEASUREMENT_CONFIG, &config_contents, 1); - config_contents = (config_contents & 0xF9); // Always set to TEMP_AND_HUMID mode - this->write_bytes(MEASUREMENT_CONFIG, &config_contents, 1); + this->read_register(REG_MEASUREMENT_CONF, &config_contents, 1); + config_contents &= ~(TRES_MASK | HRES_MASK | MEAS_CONF_MASK); // 14-bit temp, 14-bit humidity, temp+humidity mode + this->write_bytes(REG_MEASUREMENT_CONF, &config_contents, 1); - // Set rate to manual - this->read_register(CONFIG, &config_contents, 1); - config_contents &= 0x8F; - this->write_bytes(CONFIG, &config_contents, 1); - - // Set temperature resolution to 14bit - this->read_register(CONFIG, &config_contents, 1); - config_contents &= 0x3F; - this->write_bytes(CONFIG, &config_contents, 1); - - // Set humidity resolution to 14bit - this->read_register(CONFIG, &config_contents, 1); - config_contents &= 0xCF; - this->write_bytes(CONFIG, &config_contents, 1); + // Set auto measurement rate to manual (on-demand via MEAS_TRIG) + this->read_register(REG_RESET_DRDY_INT_CONF, &config_contents, 1); + config_contents &= ~AMM_MASK; + this->write_bytes(REG_RESET_DRDY_INT_CONF, &config_contents, 1); } void HDC2010Component::dump_config() { @@ -67,9 +53,9 @@ void HDC2010Component::dump_config() { void HDC2010Component::update() { // Trigger measurement uint8_t config_contents; - this->read_register(CONFIG, &config_contents, 1); - config_contents |= 0x01; - this->write_bytes(MEASUREMENT_CONFIG, &config_contents, 1); + this->read_register(REG_MEASUREMENT_CONF, &config_contents, 1); + config_contents |= MEAS_TRIG; + this->write_bytes(REG_MEASUREMENT_CONF, &config_contents, 1); // 1ms delay after triggering the sample set_timeout(1, [this]() { @@ -90,8 +76,8 @@ void HDC2010Component::update() { float HDC2010Component::read_temp() { uint8_t byte[2]; - this->read_register(HDC2010_CMD_TEMPERATURE_LOW, &byte[0], 1); - this->read_register(HDC2010_CMD_TEMPERATURE_HIGH, &byte[1], 1); + this->read_register(REG_TEMPERATURE_LOW, &byte[0], 1); + this->read_register(REG_TEMPERATURE_HIGH, &byte[1], 1); uint16_t temp = encode_uint16(byte[1], byte[0]); return (float) temp * 0.0025177f - 40.0f; @@ -100,8 +86,8 @@ float HDC2010Component::read_temp() { float HDC2010Component::read_humidity() { uint8_t byte[2]; - this->read_register(HDC2010_CMD_HUMIDITY_LOW, &byte[0], 1); - this->read_register(HDC2010_CMD_HUMIDITY_HIGH, &byte[1], 1); + this->read_register(REG_HUMIDITY_LOW, &byte[0], 1); + this->read_register(REG_HUMIDITY_HIGH, &byte[1], 1); uint16_t humidity = encode_uint16(byte[1], byte[0]); return (float) humidity * 0.001525879f; diff --git a/esphome/components/sensor/__init__.py b/esphome/components/sensor/__init__.py index 64d4dc4177..9f3c1484b0 100644 --- a/esphome/components/sensor/__init__.py +++ b/esphome/components/sensor/__init__.py @@ -406,7 +406,9 @@ QUANTILE_SCHEMA = cv.All( cv.Optional(CONF_WINDOW_SIZE, default=5): cv.int_range(min=1, max=65535), cv.Optional(CONF_SEND_EVERY, default=5): cv.int_range(min=1, max=65535), cv.Optional(CONF_SEND_FIRST_AT, default=1): cv.int_range(min=1, max=65535), - cv.Optional(CONF_QUANTILE, default=0.9): cv.zero_to_one_float, + cv.Optional(CONF_QUANTILE, default=0.9): cv.float_range( + min=0, min_included=False, max=1 + ), } ), validate_send_first_at, From 62f9bc79c4e4ee929e158cab64087c7bcd289072 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:48:21 -1000 Subject: [PATCH 13/17] [ci] Add CodSpeed badge to README (#14901) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b8ce8d091d..16497ee0be 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# ESPHome [![Discord Chat](https://img.shields.io/discord/429907082951524364.svg)](https://discord.gg/KhAMKrd) [![GitHub release](https://img.shields.io/github/release/esphome/esphome.svg)](https://GitHub.com/esphome/esphome/releases/) +# ESPHome [![Discord Chat](https://img.shields.io/discord/429907082951524364.svg)](https://discord.gg/KhAMKrd) [![GitHub release](https://img.shields.io/github/release/esphome/esphome.svg)](https://GitHub.com/esphome/esphome/releases/) [![CodSpeed](https://img.shields.io/endpoint?url=https://codspeed.io/badge.json)](https://codspeed.io/esphome/esphome) From 342020e1d3c53378febedc38819674b10049ea82 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 13:49:24 -1000 Subject: [PATCH 14/17] [mqtt] Fix data race on inbound event queue (#14891) Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com> --- .../components/mqtt/mqtt_backend_esp32.cpp | 34 ++++++--- esphome/components/mqtt/mqtt_backend_esp32.h | 69 +++++++++++-------- 2 files changed, 66 insertions(+), 37 deletions(-) diff --git a/esphome/components/mqtt/mqtt_backend_esp32.cpp b/esphome/components/mqtt/mqtt_backend_esp32.cpp index 5642fd5f7b..ab067c4418 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.cpp +++ b/esphome/components/mqtt/mqtt_backend_esp32.cpp @@ -82,10 +82,16 @@ bool MQTTBackendESP32::initialize_() { void MQTTBackendESP32::loop() { // process new events // handle only 1 message per loop iteration - if (!mqtt_events_.empty()) { - auto &event = mqtt_events_.front(); - mqtt_event_handler_(event); - mqtt_events_.pop(); + Event *event = this->mqtt_event_queue_.pop(); + if (event != nullptr) { + this->mqtt_event_handler_(*event); + this->mqtt_event_pool_.release(event); + } + + // Log dropped inbound events (check is cheap - single atomic load in common case) + uint16_t inbound_dropped = this->mqtt_event_queue_.get_and_reset_dropped_count(); + if (inbound_dropped > 0) { + ESP_LOGW(TAG, "Dropped %u inbound MQTT events", inbound_dropped); } #if defined(USE_MQTT_IDF_ENQUEUE) @@ -183,10 +189,18 @@ void MQTTBackendESP32::mqtt_event_handler_(const Event &event) { void MQTTBackendESP32::mqtt_event_handler(void *handler_args, esp_event_base_t base, int32_t event_id, void *event_data) { MQTTBackendESP32 *instance = static_cast(handler_args); - // queue event to decouple processing + // queue event to decouple processing from ESP-IDF MQTT task to main loop if (instance) { - auto event = *static_cast(event_data); - instance->mqtt_events_.emplace(event); + auto *event = instance->mqtt_event_pool_.allocate(); + if (event == nullptr) { + // Pool exhausted, drop event (counted via queue's dropped counter) + instance->mqtt_event_queue_.increment_dropped_count(); + return; + } + event->populate(*static_cast(event_data)); + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. + instance->mqtt_event_queue_.push(event); // Wake main loop immediately to process MQTT event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) @@ -226,14 +240,14 @@ void MQTTBackendESP32::esphome_mqtt_task(void *params) { break; } } - this_mqtt->mqtt_event_pool_.release(elem); + this_mqtt->mqtt_outbound_pool_.release(elem); } } } bool MQTTBackendESP32::enqueue_(MqttQueueTypeT type, const char *topic, int qos, bool retain, const char *payload, size_t len) { - auto *elem = this->mqtt_event_pool_.allocate(); + auto *elem = this->mqtt_outbound_pool_.allocate(); if (!elem) { // Queue is full - increment counter but don't log immediately. @@ -253,7 +267,7 @@ bool MQTTBackendESP32::enqueue_(MqttQueueTypeT type, const char *topic, int qos, // Use the helper to allocate and copy data if (!elem->set_data(topic, payload, len)) { // Allocation failed, return elem to pool - this->mqtt_event_pool_.release(elem); + this->mqtt_outbound_pool_.release(elem); // Increment counter without logging to avoid cascade effect during memory pressure this->mqtt_queue_.increment_dropped_count(); return false; diff --git a/esphome/components/mqtt/mqtt_backend_esp32.h b/esphome/components/mqtt/mqtt_backend_esp32.h index 5c4dc413bd..58d1b29b32 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.h +++ b/esphome/components/mqtt/mqtt_backend_esp32.h @@ -5,7 +5,6 @@ #ifdef USE_ESP32 #include -#include #include #include #include @@ -18,32 +17,39 @@ namespace esphome::mqtt { struct Event { - esp_mqtt_event_id_t event_id; + esp_mqtt_event_id_t event_id{}; std::vector data; - int total_data_len; - int current_data_offset; + int total_data_len{0}; + int current_data_offset{0}; std::string topic; - int msg_id; - bool retain; - int qos; - bool dup; - bool session_present; - esp_mqtt_error_codes_t error_handle; + int msg_id{0}; + bool retain{false}; + int qos{0}; + bool dup{false}; + bool session_present{false}; + esp_mqtt_error_codes_t error_handle{}; - // Construct from esp_mqtt_event_t - // Any pointer values that are unsafe to keep are converted to safe copies - Event(const esp_mqtt_event_t &event) - : event_id(event.event_id), - data(event.data, event.data + event.data_len), - total_data_len(event.total_data_len), - current_data_offset(event.current_data_offset), - topic(event.topic, event.topic_len), - msg_id(event.msg_id), - retain(event.retain), - qos(event.qos), - dup(event.dup), - session_present(event.session_present), - error_handle(*event.error_handle) {} + // Populate from esp_mqtt_event_t + // Copies pointer-based data to owned storage for safe cross-thread transfer + void populate(const esp_mqtt_event_t &event) { + this->event_id = event.event_id; + this->data.assign(event.data, event.data + event.data_len); + this->total_data_len = event.total_data_len; + this->current_data_offset = event.current_data_offset; + this->topic.assign(event.topic, event.topic_len); + this->msg_id = event.msg_id; + this->retain = event.retain; + this->qos = event.qos; + this->dup = event.dup; + this->session_present = event.session_present; + this->error_handle = *event.error_handle; + } + + // Release owned resources for pool reuse (keeps allocated capacity for efficiency) + void release() { + this->data.clear(); + this->topic.clear(); + } }; enum MqttQueueTypeT : uint8_t { @@ -118,7 +124,8 @@ class MQTTBackendESP32 final : public MQTTBackend { static constexpr size_t TASK_STACK_SIZE = 3072; static constexpr size_t TASK_STACK_SIZE_TLS = 4096; // Larger stack for TLS operations static constexpr ssize_t TASK_PRIORITY = 5; - static constexpr uint8_t MQTT_QUEUE_LENGTH = 30; // 30*12 bytes = 360 + static constexpr uint8_t MQTT_QUEUE_LENGTH = 30; // 30*12 bytes = 360 + static constexpr uint8_t MQTT_EVENT_QUEUE_LENGTH = 32; // Inbound events from broker void set_keep_alive(uint16_t keep_alive) final { this->keep_alive_ = keep_alive; } void set_client_id(const char *client_id) final { this->client_id_ = client_id; } @@ -251,7 +258,8 @@ class MQTTBackendESP32 final : public MQTTBackend { bool skip_cert_cn_check_{false}; #if defined(USE_MQTT_IDF_ENQUEUE) static void esphome_mqtt_task(void *params); - EventPool mqtt_event_pool_; + // Pool sized to queue capacity (SIZE-1) — see mqtt_event_pool_ comment. + EventPool mqtt_outbound_pool_; NotifyingLockFreeQueue mqtt_queue_; TaskHandle_t task_handle_{nullptr}; bool enqueue_(MqttQueueTypeT type, const char *topic, int qos = 0, bool retain = false, const char *payload = NULL, @@ -266,7 +274,14 @@ class MQTTBackendESP32 final : public MQTTBackend { CallbackManager on_message_; CallbackManager on_publish_; std::string cached_topic_; - std::queue mqtt_events_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements (one slot distinguishes full from empty). + // This guarantees allocate() returns nullptr before push() can fail, which: + // 1. Prevents leaking a pool slot (the Nth allocate succeeds but push fails) + // 2. Avoids needing release() on the producer path after a failed push(), + // preserving the SPSC contract on the pool's internal free list + EventPool mqtt_event_pool_; + LockFreeQueue mqtt_event_queue_; #if defined(USE_MQTT_IDF_ENQUEUE) uint32_t last_dropped_log_time_{0}; From 0c5f055d45a333733460d5fe10877cefeb1a71ed Mon Sep 17 00:00:00 2001 From: Javier Peletier Date: Wed, 18 Mar 2026 01:16:01 +0100 Subject: [PATCH 15/17] [core] cpp tests: Allow customizing code generation during tests (#14681) Co-authored-by: J. Nick Koston Co-authored-by: J. Nick Koston --- esphome/core/__init__.py | 6 - esphome/loader.py | 15 +- script/{test_helpers.py => build_helpers.py} | 128 ++++++--- script/cpp_benchmark.py | 18 +- script/cpp_unit_test.py | 13 +- script/determine-jobs.py | 4 +- script/helpers.py | 5 +- tests/benchmarks/components/core/__init__.py | 7 + tests/benchmarks/components/host/__init__.py | 7 + tests/benchmarks/components/json/__init__.py | 7 + .../benchmarks/components/logger/__init__.py | 7 + tests/benchmarks/components/time/__init__.py | 9 + tests/components/README.md | 60 +++- tests/components/core/__init__.py | 8 + tests/components/host/__init__.py | 7 + tests/components/logger/__init__.py | 7 + tests/components/time/__init__.py | 9 + tests/script/test_determine_jobs.py | 2 +- tests/script/test_helpers.py | 22 -- tests/script/test_test_helpers.py | 260 ++++++++++++++++++ tests/testing_helpers.py | 63 +++++ tests/unit_tests/test_loader.py | 99 ++++++- 22 files changed, 667 insertions(+), 96 deletions(-) rename script/{test_helpers.py => build_helpers.py} (78%) create mode 100644 tests/benchmarks/components/core/__init__.py create mode 100644 tests/benchmarks/components/host/__init__.py create mode 100644 tests/benchmarks/components/json/__init__.py create mode 100644 tests/benchmarks/components/logger/__init__.py create mode 100644 tests/benchmarks/components/time/__init__.py create mode 100644 tests/components/core/__init__.py create mode 100644 tests/components/host/__init__.py create mode 100644 tests/components/logger/__init__.py create mode 100644 tests/components/time/__init__.py create mode 100644 tests/script/test_test_helpers.py create mode 100644 tests/testing_helpers.py diff --git a/esphome/core/__init__.py b/esphome/core/__init__.py index a86478aca1..009fef2f86 100644 --- a/esphome/core/__init__.py +++ b/esphome/core/__init__.py @@ -615,10 +615,6 @@ class EsphomeCore: self.address_cache: AddressCache | None = None # Cached config hash (computed lazily) self._config_hash: int | None = None - # True if compiling for C++ unit tests - self.cpp_testing = False - # Allowlist of components whose to_code should run during C++ testing - self.cpp_testing_codegen: set[str] = set() def reset(self): from esphome.pins import PIN_SCHEMA_REGISTRY @@ -648,8 +644,6 @@ class EsphomeCore: self.current_component = None self.address_cache = None self._config_hash = None - self.cpp_testing = False - self.cpp_testing_codegen = set() PIN_SCHEMA_REGISTRY.reset() @contextmanager diff --git a/esphome/loader.py b/esphome/loader.py index 5771e07473..68664aaa26 100644 --- a/esphome/loader.py +++ b/esphome/loader.py @@ -71,11 +71,6 @@ class ComponentManifest: @property def to_code(self) -> Callable[[Any], None] | None: - if CORE.cpp_testing: - # During C++ testing, only run to_code for allowlisted components - name = self.module.__package__.rsplit(".", 1)[-1] - if name not in CORE.cpp_testing_codegen: - return None return getattr(self.module, "to_code", None) @property @@ -243,3 +238,13 @@ def get_platform(domain: str, platform: str) -> ComponentManifest | None: _COMPONENT_CACHE: dict[str, ComponentManifest] = {} CORE_COMPONENTS_PATH = (Path(__file__).parent / "components").resolve() _COMPONENT_CACHE["esphome"] = ComponentManifest(esphome.core.config) + + +def _replace_component_manifest(domain: str, manifest: ComponentManifest) -> None: + """Replace the cached manifest for a component. + + This is an intentionally-supported hook for the C++ test infrastructure + to install ``ComponentManifestOverride`` wrappers. Normal application + code should never call this. + """ + _COMPONENT_CACHE[domain] = manifest diff --git a/script/test_helpers.py b/script/build_helpers.py similarity index 78% rename from script/test_helpers.py rename to script/build_helpers.py index e872bbc516..1cfae51fca 100644 --- a/script/test_helpers.py +++ b/script/build_helpers.py @@ -2,21 +2,29 @@ from __future__ import annotations +from collections.abc import Callable import hashlib +import importlib.util import os from pathlib import Path import subprocess import sys -from helpers import get_all_dependencies +from helpers import get_all_dependencies, root_path as _root_path import yaml +# Ensure the repo root is on sys.path so that ``tests.testing_helpers`` and +# override ``__init__.py`` modules can ``from tests.testing_helpers import ...``. +if _root_path not in sys.path: + sys.path.insert(0, _root_path) + from esphome.__main__ import command_compile, parse_args from esphome.config import validate_config from esphome.const import CONF_PLATFORM from esphome.core import CORE -from esphome.loader import get_component +from esphome.loader import get_component, get_platform from esphome.platformio_api import get_idedata +from tests.testing_helpers import ComponentManifestOverride, set_testing_manifest # This must coincide with the version in /platformio.ini PLATFORMIO_GOOGLE_TEST_LIB = "google/googletest@^1.15.2" @@ -32,19 +40,6 @@ ESPHOME_KEY = "esphome" HOST_KEY = "host" LOGGER_KEY = "logger" -# Base config keys that are always present and must not be fully overridden -# by component benchmark.yaml files. esphome: allows sub-key merging. -BASE_CONFIG_KEYS = frozenset({ESPHOME_KEY, HOST_KEY, LOGGER_KEY}) - -# Shared build flag — enables timezone code paths for testing/benchmarking. -USE_TIME_TIMEZONE_FLAG = "-DUSE_TIME_TIMEZONE" - -# Components whose to_code should always run during C++ test/benchmark builds. -# These are the minimal infrastructure components needed for host compilation. -# Note: "core" is the esphome core config module (esphome/core/config.py), -# which registers under package name "core" not "esphome". -BASE_CODEGEN_COMPONENTS = {"core", "host", "logger"} - # Exit codes EXIT_OK = 0 EXIT_SKIPPED = 1 @@ -110,6 +105,8 @@ def get_platform_components(components: list[str], tests_dir: Path) -> list[str] if not domain_dir.is_dir(): continue domain = domain_dir.name + if domain.startswith("__"): + continue domain_module = get_component(domain) if domain_module is None or not domain_module.is_platform_component: raise ValueError( @@ -130,7 +127,8 @@ def load_component_yaml_configs(components: list[str], tests_dir: Path) -> dict: The ``esphome:`` key is special: its sub-keys are merged into the existing esphome config (e.g. to add ``areas:`` or ``devices:``). - Other base config keys (``host:``, ``logger:``) are not overridable. + Keys already present in the base config (e.g. ``host:``, ``logger:``) + are protected by ``setdefault`` in the caller. Args: components: List of component directory names @@ -139,11 +137,6 @@ def load_component_yaml_configs(components: list[str], tests_dir: Path) -> dict: Returns: Merged dict of component configs to add to the base config """ - # Note: components are processed in sorted order. For conflicting keys - # (e.g. two benchmark.yaml files both declaring sensor:), the first - # component alphabetically wins via setdefault(). This is fine for now - # with a single benchmark component (api) but would need a real merge - # strategy if multiple components declare overlapping configs. merged: dict = {} for component in components: yaml_path = tests_dir / component / BENCHMARK_YAML_FILENAME @@ -153,9 +146,6 @@ def load_component_yaml_configs(components: list[str], tests_dir: Path) -> dict: component_config = yaml.safe_load(f) if component_config and isinstance(component_config, dict): for key, value in component_config.items(): - if key in BASE_CONFIG_KEYS - {ESPHOME_KEY}: - # host: and logger: are not overridable - continue if key == ESPHOME_KEY and isinstance(value, dict): # Merge esphome sub-keys rather than replacing esphome_extra = merged.setdefault(ESPHOME_KEY, {}) @@ -198,11 +188,78 @@ def create_host_config( } +def _wrap_manifest( + comp_name: str, +) -> ComponentManifestOverride | None: + """Wrap a component manifest in a ComponentManifestOverride with to_code suppressed. + + If the manifest is already wrapped or not found, returns None. + Otherwise returns the newly created override after installing it. + """ + if "." in comp_name: + domain, component = comp_name.split(".", maxsplit=1) + manifest = get_platform(domain, component) + cache_key = f"{component}.{domain}" + else: + manifest = get_component(comp_name) + cache_key = comp_name + + if manifest is None or isinstance(manifest, ComponentManifestOverride): + return None + + override = ComponentManifestOverride(manifest) + override.to_code = None # suppress by default + set_testing_manifest(cache_key, override) + return override + + +def load_test_manifest_overrides( + components: list[str], + tests_dir: Path, +) -> None: + """Apply per-component manifest overrides from test ``__init__.py`` files. + + For every component, wraps its manifest and suppresses ``to_code``. + If the component's test directory contains an ``__init__.py`` that + defines ``override_manifest(manifest)``, it is called to customise + the override (e.g. ``manifest.enable_codegen()``). + """ + for comp_name in components: + override = _wrap_manifest(comp_name) + if override is None: + continue + + if "." in comp_name: + domain, component = comp_name.split(".", maxsplit=1) + cache_key = f"{component}.{domain}" + test_init = tests_dir / component / domain / "__init__.py" + else: + cache_key = comp_name + test_init = tests_dir / comp_name / "__init__.py" + + if not test_init.is_file(): + continue + spec = importlib.util.spec_from_file_location( + f"_test_manifest_override.{cache_key}", test_init + ) + if spec is None or spec.loader is None: + continue + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + override_fn = getattr(mod, "override_manifest", None) + if override_fn is not None: + override_fn(override) + + +# Type alias for manifest override loaders +ManifestOverrideLoader = Callable[[list[str]], None] + + def compile_and_get_binary( config: dict, components: list[str], - codegen_components: set[str], tests_dir: Path, + manifest_override_loader: ManifestOverrideLoader, label: str = "build", ) -> tuple[int, str | None]: """Compile an ESPHome configuration and return the binary path. @@ -210,8 +267,8 @@ def compile_and_get_binary( Args: config: ESPHome configuration dict (already created via create_host_config) components: List of components to include in the build - codegen_components: Set of component names whose to_code should run tests_dir: Base directory for test files (used as config_path base) + manifest_override_loader: Callback to apply manifest overrides for components label: Label for log messages (e.g. "unit tests", "benchmarks") Returns: @@ -238,18 +295,21 @@ def compile_and_get_binary( else: config.setdefault(key, value) + # Apply manifest overrides before dependency resolution so that any + # dependency additions made by override_manifest() are picked up. + manifest_override_loader(components) + # Obtain possible dependencies BEFORE validate_config, because # get_all_dependencies calls CORE.reset() which clears build_path. - # Always include 'time' because USE_TIME_TIMEZONE is defined as a build flag, - # which causes core/time.h to include components/time/posix_tz.h. components_with_dependencies: list[str] = sorted( - get_all_dependencies(set(components) | {"time"}, cpp_testing=True) + get_all_dependencies(set(components)) ) + # Apply overrides for any transitively discovered dependencies. + manifest_override_loader(components_with_dependencies) + CORE.config_path = tests_dir / "dummy.yaml" CORE.dashboard = None - CORE.cpp_testing = True - CORE.cpp_testing_codegen = codegen_components # Validate config will expand the above with defaults: config = validate_config(config, {}) @@ -305,7 +365,7 @@ def compile_and_get_binary( def build_and_run( selected_components: list[str], tests_dir: Path, - codegen_components: set[str], + manifest_override_loader: ManifestOverrideLoader, config_prefix: str, friendly_name: str, libraries: str | list[str], @@ -324,7 +384,7 @@ def build_and_run( Args: selected_components: Components to include (directory names in tests_dir) tests_dir: Directory containing test/benchmark files - codegen_components: Components whose to_code should run + manifest_override_loader: Callback to apply manifest overrides for components config_prefix: Prefix for the config name (e.g. "cpptests", "cppbench") friendly_name: Human-readable name for the config libraries: PlatformIO library specification(s) @@ -382,7 +442,7 @@ def build_and_run( ) exit_code, program_path = compile_and_get_binary( - config, components, codegen_components, tests_dir, label + config, components, tests_dir, manifest_override_loader, label ) if exit_code != EXIT_OK or program_path is None: diff --git a/script/cpp_benchmark.py b/script/cpp_benchmark.py index bd92266ea6..a54d3752df 100755 --- a/script/cpp_benchmark.py +++ b/script/cpp_benchmark.py @@ -2,18 +2,18 @@ """Build and run C++ benchmarks for ESPHome components using Google Benchmark.""" import argparse +from functools import partial import json import os from pathlib import Path import sys -from helpers import root_path -from test_helpers import ( - BASE_CODEGEN_COMPONENTS, +from build_helpers import ( PLATFORMIO_GOOGLE_BENCHMARK_LIB, - USE_TIME_TIMEZONE_FLAG, build_and_run, + load_test_manifest_overrides, ) +from helpers import root_path # Path to /tests/benchmarks/components BENCHMARKS_DIR: Path = Path(root_path) / "tests" / "benchmarks" / "components" @@ -21,11 +21,6 @@ BENCHMARKS_DIR: Path = Path(root_path) / "tests" / "benchmarks" / "components" # Path to /tests/benchmarks/core (always included, not a component) CORE_BENCHMARKS_DIR: Path = Path(root_path) / "tests" / "benchmarks" / "core" -# Additional codegen components beyond the base set. -# json is needed because its to_code adds the ArduinoJson library -# (auto-loaded by api, but cpp_testing suppresses to_code unless listed). -BENCHMARK_CODEGEN_COMPONENTS = BASE_CODEGEN_COMPONENTS | {"json"} - PLATFORMIO_OPTIONS = { "build_unflags": [ "-Os", # remove default size-opt @@ -33,7 +28,6 @@ PLATFORMIO_OPTIONS = { "build_flags": [ "-O2", # optimize for speed (CodSpeed recommends RelWithDebInfo) "-g", # debug symbols for profiling - USE_TIME_TIMEZONE_FLAG, "-DUSE_BENCHMARK", # disable WarnIfComponentBlockingGuard in finish() ], # Use deep+ LDF mode to ensure PlatformIO detects the benchmark @@ -73,7 +67,9 @@ def run_benchmarks(selected_components: list[str], build_only: bool = False) -> return build_and_run( selected_components=selected_components, tests_dir=BENCHMARKS_DIR, - codegen_components=BENCHMARK_CODEGEN_COMPONENTS, + manifest_override_loader=partial( + load_test_manifest_overrides, tests_dir=BENCHMARKS_DIR + ), config_prefix="cppbench", friendly_name="CPP Benchmarks", libraries=benchmark_lib, diff --git a/script/cpp_unit_test.py b/script/cpp_unit_test.py index 81c56b82da..5594d64240 100755 --- a/script/cpp_unit_test.py +++ b/script/cpp_unit_test.py @@ -1,15 +1,15 @@ #!/usr/bin/env python3 import argparse +from functools import partial from pathlib import Path import sys -from helpers import get_all_components, root_path -from test_helpers import ( - BASE_CODEGEN_COMPONENTS, +from build_helpers import ( PLATFORMIO_GOOGLE_TEST_LIB, - USE_TIME_TIMEZONE_FLAG, build_and_run, + load_test_manifest_overrides, ) +from helpers import get_all_components, root_path # Path to /tests/components COMPONENTS_TESTS_DIR: Path = Path(root_path) / "tests" / "components" @@ -21,7 +21,6 @@ PLATFORMIO_OPTIONS = { ], "build_flags": [ "-Og", # optimize for debug - USE_TIME_TIMEZONE_FLAG, "-DESPHOME_DEBUG", # enable debug assertions # Enable the address and undefined behavior sanitizers "-fsanitize=address", @@ -39,7 +38,9 @@ def run_tests(selected_components: list[str]) -> int: return build_and_run( selected_components=selected_components, tests_dir=COMPONENTS_TESTS_DIR, - codegen_components=BASE_CODEGEN_COMPONENTS, + manifest_override_loader=partial( + load_test_manifest_overrides, tests_dir=COMPONENTS_TESTS_DIR + ), config_prefix="cpptests", friendly_name="CPP Unit Tests", libraries=PLATFORMIO_GOOGLE_TEST_LIB, diff --git a/script/determine-jobs.py b/script/determine-jobs.py index ad08f8dce5..9f32238780 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -388,7 +388,7 @@ BENCHMARKS_COMPONENTS_PATH = "tests/benchmarks/components" BENCHMARK_INFRASTRUCTURE_FILES = frozenset( { "script/cpp_benchmark.py", - "script/test_helpers.py", + "script/build_helpers.py", "script/setup_codspeed_lib.py", } ) @@ -402,7 +402,7 @@ def should_run_benchmarks(branch: str | None = None) -> bool: 1. Core C++ files changed (esphome/core/*) 2. A directly changed component has benchmark files (no dependency expansion) 3. Benchmark infrastructure changed (tests/benchmarks/*, script/cpp_benchmark.py, - script/test_helpers.py, script/setup_codspeed_lib.py) + script/build_helpers.py, script/setup_codspeed_lib.py) Unlike unit tests, benchmarks do NOT expand to dependent components. Changing ``sensor`` does not trigger ``api`` benchmarks just because diff --git a/script/helpers.py b/script/helpers.py index 9665af70ec..290dcadf0b 100644 --- a/script/helpers.py +++ b/script/helpers.py @@ -627,14 +627,12 @@ def get_usable_cpu_count() -> int: def get_all_dependencies( - component_names: set[str], cpp_testing: bool = False + component_names: set[str], ) -> set[str]: """Get all dependencies for a set of components. Args: component_names: Set of component names to get dependencies for - cpp_testing: If True, set CORE.cpp_testing so AUTO_LOAD callables that - conditionally include testing-only dependencies work correctly Returns: Set of all components including dependencies and auto-loaded components @@ -652,7 +650,6 @@ def get_all_dependencies( # Reset CORE to ensure clean state CORE.reset() - CORE.cpp_testing = cpp_testing # Set up fake config path for component loading root = Path(__file__).parent.parent diff --git a/tests/benchmarks/components/core/__init__.py b/tests/benchmarks/components/core/__init__.py new file mode 100644 index 0000000000..d676ab669b --- /dev/null +++ b/tests/benchmarks/components/core/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # core (esphome/core/config.py) must run its to_code during builds + # because it bootstraps the fundamental application infrastructure. + manifest.enable_codegen() diff --git a/tests/benchmarks/components/host/__init__.py b/tests/benchmarks/components/host/__init__.py new file mode 100644 index 0000000000..f418c25f88 --- /dev/null +++ b/tests/benchmarks/components/host/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # host must run its to_code during builds because it sets up + # the host platform target execution environment. + manifest.enable_codegen() diff --git a/tests/benchmarks/components/json/__init__.py b/tests/benchmarks/components/json/__init__.py new file mode 100644 index 0000000000..56826b64bd --- /dev/null +++ b/tests/benchmarks/components/json/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # json must run its to_code during benchmark builds because it + # adds the ArduinoJson library dependency needed by the API component. + manifest.enable_codegen() diff --git a/tests/benchmarks/components/logger/__init__.py b/tests/benchmarks/components/logger/__init__.py new file mode 100644 index 0000000000..cfab73e1e5 --- /dev/null +++ b/tests/benchmarks/components/logger/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # logger must run its to_code during builds because it configures + # the logging subsystem used by ESP_LOG* macros. + manifest.enable_codegen() diff --git a/tests/benchmarks/components/time/__init__.py b/tests/benchmarks/components/time/__init__.py new file mode 100644 index 0000000000..7f68003e29 --- /dev/null +++ b/tests/benchmarks/components/time/__init__.py @@ -0,0 +1,9 @@ +import esphome.codegen as cg +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + async def to_code(config): + cg.add_build_flag("-DUSE_TIME_TIMEZONE") + + manifest.to_code = to_code diff --git a/tests/components/README.md b/tests/components/README.md index 6da0dadd25..145a3440d2 100644 --- a/tests/components/README.md +++ b/tests/components/README.md @@ -14,11 +14,63 @@ include the relevant `.cpp` and `.h` test files there. ### Override component code generation for testing -When generating code for testing, ESPHome won't invoke the component's `to_code` function, since most components do not -need to generate configuration code for testing. +During C++ test builds, `to_code` is suppressed for every component by default — most components do not +need to generate configuration code for a unit test binary. -If you do need to generate code to for example configure compilation flags or add libraries, -add the component name to the `CPP_TESTING_CODEGEN_COMPONENTS` allowlist in `script/cpp_unit_test.py`. +#### Manifest overrides + +If your component needs to customise code generation behavior for testing — for example to re-enable +`to_code`, supply a lightweight stub, add a test-only dependency, or change any other manifest attribute — +create an `__init__.py` in your component's test directory and define `override_manifest`: + +**Top-level component** (`tests/components//__init__.py`): + +```python +from tests.testing_helpers import ComponentManifestOverride + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # Re-enable the component's own to_code (needed when the component must + # emit C++ setup code that the test binary depends on at link time). + manifest.enable_codegen() +``` + +Or supply a lightweight stub instead of the real `to_code`: + +```python +from tests.testing_helpers import ComponentManifestOverride + +def override_manifest(manifest: ComponentManifestOverride) -> None: + async def to_code_testing(config): + # Only emit what the C++ tests actually need + pass + + manifest.to_code = to_code_testing + manifest.dependencies = manifest.dependencies + ["some_test_only_dep"] +``` + +**Platform component** (`tests/components///__init__.py`, +e.g. `tests/components/my_sensor/sensor/__init__.py`): + +```python +from tests.testing_helpers import ComponentManifestOverride + +def override_manifest(manifest: ComponentManifestOverride) -> None: + manifest.enable_codegen() +``` + +`override_manifest` receives a `ComponentManifestOverride` that wraps the real manifest. +Attribute assignments store an override; reads fall back to the real manifest when no +override is present. + +Key methods: + +| Method | Effect | +|---|---| +| `manifest.enable_codegen()` | Remove the `to_code` suppression, re-enabling code generation | +| `manifest.restore()` | Clear **all** overrides, reverting every attribute to the original | + +The function is called after `to_code` has already been set to `None`, so calling +`enable_codegen()` is a deliberate opt-in. ## Running component unit tests diff --git a/tests/components/core/__init__.py b/tests/components/core/__init__.py new file mode 100644 index 0000000000..34ca4fbe4f --- /dev/null +++ b/tests/components/core/__init__.py @@ -0,0 +1,8 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # core (esphome/core/config.py) must run its to_code during C++ test builds + # because it bootstraps the fundamental application infrastructure that all + # components depend on (component registration, event loop, etc.). + manifest.enable_codegen() diff --git a/tests/components/host/__init__.py b/tests/components/host/__init__.py new file mode 100644 index 0000000000..bcb363cdc3 --- /dev/null +++ b/tests/components/host/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # host must run its to_code during C++ test builds because it sets up the + # host platform target, which is the execution environment for all unit tests. + manifest.enable_codegen() diff --git a/tests/components/logger/__init__.py b/tests/components/logger/__init__.py new file mode 100644 index 0000000000..3acfe02748 --- /dev/null +++ b/tests/components/logger/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # logger must run its to_code during C++ test builds because it configures + # the logging subsystem used by ESP_LOG* macros throughout component code. + manifest.enable_codegen() diff --git a/tests/components/time/__init__.py b/tests/components/time/__init__.py new file mode 100644 index 0000000000..7f68003e29 --- /dev/null +++ b/tests/components/time/__init__.py @@ -0,0 +1,9 @@ +import esphome.codegen as cg +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + async def to_code(config): + cg.add_build_flag("-DUSE_TIME_TIMEZONE") + + manifest.to_code = to_code diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 29535d1fd3..de239ee0b5 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1846,7 +1846,7 @@ def test_should_run_benchmarks_benchmark_infra_change() -> None: """Test benchmarks trigger on benchmark infrastructure changes.""" for infra_file in [ "script/cpp_benchmark.py", - "script/test_helpers.py", + "script/build_helpers.py", "script/setup_codspeed_lib.py", ]: with patch.object(determine_jobs, "changed_files", return_value=[infra_file]): diff --git a/tests/script/test_helpers.py b/tests/script/test_helpers.py index e3802d2d51..28f111d758 100644 --- a/tests/script/test_helpers.py +++ b/tests/script/test_helpers.py @@ -1073,28 +1073,6 @@ def test_get_all_dependencies_platform_component_with_dependencies() -> None: assert result == {"sensor.bthome", "sensor"} -def test_get_all_dependencies_cpp_testing_flag() -> None: - """cpp_testing=True propagates to CORE.cpp_testing during resolution.""" - from esphome.core import CORE - - with ( - patch("esphome.loader.get_component") as mock_get_component, - patch("esphome.loader.get_platform"), - ): - observed: list[bool] = [] - - def capturing_get_component(name: str): - observed.append(CORE.cpp_testing) - - mock_get_component.side_effect = capturing_get_component - - helpers.get_all_dependencies({"some_comp"}, cpp_testing=True) - - assert observed and all(observed), ( - "CORE.cpp_testing should be True during resolution" - ) - - def test_get_components_from_integration_fixtures() -> None: """Test extraction of components from fixture YAML files.""" yaml_content = { diff --git a/tests/script/test_test_helpers.py b/tests/script/test_test_helpers.py new file mode 100644 index 0000000000..467940fc33 --- /dev/null +++ b/tests/script/test_test_helpers.py @@ -0,0 +1,260 @@ +"""Unit tests for script/build_helpers.py manifest override and build helpers.""" + +import os +from pathlib import Path +import sys +import textwrap +from unittest.mock import MagicMock, patch + +import pytest + +# Add the script directory to Python path so we can import build_helpers +sys.path.insert( + 0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", "script")) +) + +import build_helpers # noqa: E402 + +from esphome.loader import ComponentManifest # noqa: E402 +from tests.testing_helpers import ComponentManifestOverride # noqa: E402 + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_component_manifest(*, to_code=None, dependencies=None) -> ComponentManifest: + mod = MagicMock() + mod.to_code = to_code + mod.DEPENDENCIES = dependencies or [] + return ComponentManifest(mod) + + +# --------------------------------------------------------------------------- +# filter_components_with_files +# --------------------------------------------------------------------------- + + +def test_filter_keeps_components_with_cpp_files(tmp_path: Path) -> None: + comp_dir = tmp_path / "mycomp" + comp_dir.mkdir() + (comp_dir / "mycomp_test.cpp").write_text("") + + result = build_helpers.filter_components_with_files(["mycomp"], tmp_path) + + assert result == ["mycomp"] + + +def test_filter_keeps_components_with_h_files(tmp_path: Path) -> None: + comp_dir = tmp_path / "mycomp" + comp_dir.mkdir() + (comp_dir / "helpers.h").write_text("") + + result = build_helpers.filter_components_with_files(["mycomp"], tmp_path) + + assert result == ["mycomp"] + + +def test_filter_drops_components_without_test_dir(tmp_path: Path) -> None: + result = build_helpers.filter_components_with_files(["nodir"], tmp_path) + + assert result == [] + + +def test_filter_drops_components_with_no_cpp_or_h(tmp_path: Path) -> None: + comp_dir = tmp_path / "mycomp" + comp_dir.mkdir() + (comp_dir / "README.md").write_text("") + + result = build_helpers.filter_components_with_files(["mycomp"], tmp_path) + + assert result == [] + + +# --------------------------------------------------------------------------- +# get_platform_components +# --------------------------------------------------------------------------- + + +def test_get_platform_components_discovers_subdirectory(tmp_path: Path) -> None: + (tmp_path / "bthome" / "sensor").mkdir(parents=True) + + sensor_mod = MagicMock() + sensor_mod.IS_PLATFORM_COMPONENT = True + + with patch( + "build_helpers.get_component", return_value=ComponentManifest(sensor_mod) + ): + result = build_helpers.get_platform_components(["bthome"], tmp_path) + + assert result == ["sensor.bthome"] + + +def test_get_platform_components_skips_pycache(tmp_path: Path) -> None: + (tmp_path / "bthome" / "__pycache__").mkdir(parents=True) + + result = build_helpers.get_platform_components(["bthome"], tmp_path) + + assert result == [] + + +def test_get_platform_components_raises_for_invalid_domain(tmp_path: Path) -> None: + (tmp_path / "bthome" / "notadomain").mkdir(parents=True) + + with ( + patch("build_helpers.get_component", return_value=None), + pytest.raises(ValueError, match="notadomain"), + ): + build_helpers.get_platform_components(["bthome"], tmp_path) + + +# --------------------------------------------------------------------------- +# load_test_manifest_overrides +# --------------------------------------------------------------------------- + + +def test_load_suppresses_to_code(tmp_path: Path) -> None: + """to_code is always set to None before the override is called.""" + + async def real_to_code(config): + pass + + inner = _make_component_manifest(to_code=real_to_code) + + with ( + patch("build_helpers.get_component", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + installed: ComponentManifestOverride = mock_set.call_args[0][1] + + assert installed.to_code is None + + +def test_load_calls_override_fn(tmp_path: Path) -> None: + """override_manifest() in test_init is called with the ComponentManifestOverride.""" + comp_dir = tmp_path / "mycomp" + comp_dir.mkdir() + init_py = comp_dir / "__init__.py" + init_py.write_text( + textwrap.dedent("""\ + def override_manifest(manifest): + manifest.dependencies = ["injected"] + """) + ) + + inner = _make_component_manifest() + override = ComponentManifestOverride(inner) + override.to_code = None + + with ( + patch("build_helpers.get_component", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + installed: ComponentManifestOverride = mock_set.call_args[0][1] + + assert installed.dependencies == ["injected"] + + +def test_load_enable_codegen_in_override(tmp_path: Path) -> None: + """An override_manifest that calls enable_codegen() restores to_code.""" + + async def real_to_code(config): + pass + + comp_dir = tmp_path / "mycomp" + comp_dir.mkdir() + init_py = comp_dir / "__init__.py" + init_py.write_text( + textwrap.dedent("""\ + def override_manifest(manifest): + manifest.enable_codegen() + """) + ) + + inner = _make_component_manifest(to_code=real_to_code) + + with ( + patch("build_helpers.get_component", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + installed: ComponentManifestOverride = mock_set.call_args[0][1] + + assert installed.to_code is real_to_code + + +def test_load_no_override_file(tmp_path: Path) -> None: + """No override file: manifest is wrapped and to_code suppressed, nothing else.""" + inner = _make_component_manifest() + + with ( + patch("build_helpers.get_component", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + + mock_set.assert_called_once() + key, installed = mock_set.call_args[0] + assert key == "mycomp" + assert isinstance(installed, ComponentManifestOverride) + + +def test_load_skips_already_wrapped(tmp_path: Path) -> None: + """Components already wrapped as ComponentManifestOverride are not double-wrapped.""" + inner = _make_component_manifest() + already_wrapped = ComponentManifestOverride(inner) + + with ( + patch("build_helpers.get_component", return_value=already_wrapped), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + + mock_set.assert_not_called() + + +def test_load_skips_platform_component_already_wrapped(tmp_path: Path) -> None: + inner = _make_component_manifest() + already_wrapped = ComponentManifestOverride(inner) + + with ( + patch("build_helpers.get_platform", return_value=already_wrapped), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["sensor.bthome"], tmp_path) + + mock_set.assert_not_called() + + +def test_load_wraps_top_level_component(tmp_path: Path) -> None: + inner = _make_component_manifest() + + with ( + patch("build_helpers.get_component", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["mycomp"], tmp_path) + + mock_set.assert_called_once() + key, installed = mock_set.call_args[0] + assert key == "mycomp" + assert isinstance(installed, ComponentManifestOverride) + assert installed.to_code is None + + +def test_load_wraps_platform_component(tmp_path: Path) -> None: + inner = _make_component_manifest() + + with ( + patch("build_helpers.get_platform", return_value=inner), + patch("build_helpers.set_testing_manifest") as mock_set, + ): + build_helpers.load_test_manifest_overrides(["sensor.bthome"], tmp_path) + + mock_set.assert_called_once() + key, installed = mock_set.call_args[0] + assert key == "bthome.sensor" + assert isinstance(installed, ComponentManifestOverride) + assert installed.to_code is None diff --git a/tests/testing_helpers.py b/tests/testing_helpers.py new file mode 100644 index 0000000000..20b76697a1 --- /dev/null +++ b/tests/testing_helpers.py @@ -0,0 +1,63 @@ +from typing import Any + +from esphome.loader import ComponentManifest, _replace_component_manifest + + +class ComponentManifestOverride: + """Mutable wrapper around ComponentManifest for test-specific attribute overrides. + + When ``tests/components//__init__.py`` defines:: + + def override_manifest(manifest: ComponentManifestOverride) -> None: + ... + + the function receives an instance of this class wrapping the real component + manifest. Any attribute assignment stores an override; reads fall back to + the underlying ``ComponentManifest`` when no override has been set. + + Example:: + + def override_manifest(manifest: ComponentManifestOverride) -> None: + async def to_code_testing(config): + pass # lightweight no-op stub for C++ unit tests + + manifest.to_code = to_code_testing + manifest.dependencies = manifest.dependencies + ["extra_dep_for_tests"] + """ + + def __init__(self, wrapped: "ComponentManifest") -> None: + object.__setattr__(self, "_wrapped", wrapped) + object.__setattr__(self, "_overrides", {}) + + def __getattr__(self, name: str) -> Any: + overrides: dict[str, Any] = object.__getattribute__(self, "_overrides") + if name in overrides: + return overrides[name] + wrapped: ComponentManifest = object.__getattribute__(self, "_wrapped") + return getattr(wrapped, name) + + def __setattr__(self, name: str, value: Any) -> None: + overrides: dict[str, Any] = object.__getattribute__(self, "_overrides") + overrides[name] = value + + def enable_codegen(self) -> None: + """Remove the to_code suppression, re-enabling code generation for this component. + + Call this from ``override_manifest`` when the component needs its real (or a + custom stub) ``to_code`` to run during C++ unit test builds. + """ + overrides: dict[str, Any] = object.__getattribute__(self, "_overrides") + overrides.pop("to_code", None) + + def restore(self) -> None: + """Clear all overrides, reverting to the wrapped manifest's values.""" + object.__getattribute__(self, "_overrides").clear() + + +def set_testing_manifest(domain: str, manifest: ComponentManifestOverride) -> None: + """Install a testing manifest override into the component cache. + + Called from the C++ unit test infrastructure when a component's test + directory provides an ``override_manifest`` function. + """ + _replace_component_manifest(domain, manifest) diff --git a/tests/unit_tests/test_loader.py b/tests/unit_tests/test_loader.py index c6d4c4aef0..a42cc5cca7 100644 --- a/tests/unit_tests/test_loader.py +++ b/tests/unit_tests/test_loader.py @@ -2,7 +2,104 @@ from unittest.mock import MagicMock, patch -from esphome.loader import ComponentManifest +from esphome.loader import ComponentManifest, _replace_component_manifest, get_component +from tests.testing_helpers import ComponentManifestOverride + +# --------------------------------------------------------------------------- +# ComponentManifestOverride +# --------------------------------------------------------------------------- + + +def _make_manifest(*, to_code=None, dependencies=None) -> ComponentManifest: + """Return a ComponentManifest backed by a minimal mock module.""" + mod = MagicMock() + mod.to_code = to_code + mod.DEPENDENCIES = dependencies or [] + return ComponentManifest(mod) + + +def test_testing_manifest_delegates_to_wrapped() -> None: + """Unoverridden attributes fall through to the wrapped manifest.""" + inner = _make_manifest(dependencies=["wifi"]) + tm = ComponentManifestOverride(inner) + assert tm.dependencies == ["wifi"] + + +def test_testing_manifest_override_shadows_wrapped() -> None: + """An assigned attribute shadows the wrapped value.""" + inner = _make_manifest(dependencies=["wifi"]) + tm = ComponentManifestOverride(inner) + tm.dependencies = ["ble"] + assert tm.dependencies == ["ble"] + # Wrapped value unchanged + assert inner.dependencies == ["wifi"] + + +def test_testing_manifest_to_code_suppression() -> None: + """Setting to_code=None suppresses code generation.""" + + async def real_to_code(config): + pass + + inner = _make_manifest(to_code=real_to_code) + tm = ComponentManifestOverride(inner) + tm.to_code = None + assert tm.to_code is None + + +def test_testing_manifest_enable_codegen_removes_suppression() -> None: + """enable_codegen() removes the to_code override, restoring the original.""" + + async def real_to_code(config): + pass + + inner = _make_manifest(to_code=real_to_code) + tm = ComponentManifestOverride(inner) + tm.to_code = None + assert tm.to_code is None + + tm.enable_codegen() + assert tm.to_code is real_to_code + + +def test_testing_manifest_enable_codegen_preserves_other_overrides() -> None: + """enable_codegen() only removes to_code; other overrides survive.""" + inner = _make_manifest(dependencies=["wifi"]) + tm = ComponentManifestOverride(inner) + tm.to_code = None + tm.dependencies = ["ble"] + + tm.enable_codegen() + + assert tm.to_code is inner.to_code + assert tm.dependencies == ["ble"] + + +def test_testing_manifest_restore_clears_all_overrides() -> None: + """restore() removes every override, reverting all attributes to wrapped values.""" + + async def real_to_code(config): + pass + + inner = _make_manifest(to_code=real_to_code, dependencies=["wifi"]) + tm = ComponentManifestOverride(inner) + tm.to_code = None + tm.dependencies = ["ble"] + + tm.restore() + + assert tm.to_code is real_to_code + assert tm.dependencies == ["wifi"] + + +def test_replace_component_manifest_installs_override() -> None: + """_replace_component_manifest replaces the cached manifest for a domain.""" + inner = _make_manifest() + override = ComponentManifestOverride(inner) + + _replace_component_manifest("_test_dummy_domain", override) + + assert get_component("_test_dummy_domain") is override def test_component_manifest_resources_with_filter_source_files() -> None: From b9e8da92c734a4b761b9835b6e584d87c1d47eaa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 14:19:31 -1000 Subject: [PATCH 16/17] [scheduler] Fix UB in cross-thread counter/vector reads, add atomic fast-path (#14880) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> --- esphome/core/scheduler.cpp | 48 ++++++----- esphome/core/scheduler.h | 159 ++++++++++++++++++++++++++++++++++--- 2 files changed, 177 insertions(+), 30 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 72b183384e..db40ede78c 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -212,6 +212,14 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type this->cancel_item_locked_(component, name_type, static_name, hash_or_id, type); } target->push_back(item); + if (target == &this->to_add_) { + this->to_add_count_increment_(); + } +#ifndef ESPHOME_THREAD_SINGLE + else { + this->defer_count_increment_(); + } +#endif } void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, @@ -388,7 +396,7 @@ optional HOT Scheduler::next_schedule_in(uint32_t now) { // safe when called from the main thread. Other threads must not call this method. // If no items, return empty optional - if (this->cleanup_() == 0) + if (!this->cleanup_()) return {}; SchedulerItem *item = this->items_[0]; @@ -422,7 +430,7 @@ void Scheduler::full_cleanup_removed_items_() { this->items_.erase(this->items_.begin() + write, this->items_.end()); // Rebuild the heap structure since items are no longer in heap order std::make_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); - this->to_remove_ = 0; + this->to_remove_clear_(); } #ifndef ESPHOME_THREAD_SINGLE @@ -503,7 +511,7 @@ void HOT Scheduler::call(uint32_t now) { // If we still have too many cancelled items, do a full cleanup // This only happens if cancelled items are stuck in the middle/bottom of the heap - if (this->to_remove_ >= MAX_LOGICALLY_DELETED_ITEMS) { + if (this->to_remove_count_() >= MAX_LOGICALLY_DELETED_ITEMS) { this->full_cleanup_removed_items_(); } while (!this->items_.empty()) { @@ -530,7 +538,7 @@ void HOT Scheduler::call(uint32_t now) { LockGuard guard{this->lock_}; if (is_item_removed_locked_(item)) { this->recycle_item_main_loop_(this->pop_raw_locked_()); - this->to_remove_--; + this->to_remove_decrement_(); continue; } } @@ -539,7 +547,7 @@ void HOT Scheduler::call(uint32_t now) { if (is_item_removed_(item)) { LockGuard guard{this->lock_}; this->recycle_item_main_loop_(this->pop_raw_locked_()); - this->to_remove_--; + this->to_remove_decrement_(); continue; } #endif @@ -567,7 +575,7 @@ void HOT Scheduler::call(uint32_t now) { if (this->is_item_removed_locked_(executed_item)) { // We were removed/cancelled in the function call, recycle and continue - this->to_remove_--; + this->to_remove_decrement_(); this->recycle_item_main_loop_(executed_item); continue; } @@ -577,6 +585,7 @@ void HOT Scheduler::call(uint32_t now) { // Add new item directly to to_add_ // since we have the lock held this->to_add_.push_back(executed_item); + this->to_add_count_increment_(); } else { // Timeout completed - recycle it this->recycle_item_main_loop_(executed_item); @@ -605,6 +614,10 @@ void HOT Scheduler::call(uint32_t now) { #endif } void HOT Scheduler::process_to_add() { + // Fast path: skip lock acquisition when nothing to add. + // Worst case is a one-loop-iteration delay before newly added items are processed. + if (this->to_add_empty_()) + return; LockGuard guard{this->lock_}; for (auto *&it : this->to_add_) { if (is_item_removed_locked_(it)) { @@ -618,17 +631,14 @@ void HOT Scheduler::process_to_add() { std::push_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); } this->to_add_.clear(); + this->to_add_count_clear_(); } -size_t HOT Scheduler::cleanup_() { - // Fast path: if nothing to remove, just return the current size - // Reading to_remove_ without lock is safe because: - // 1. We only call this from the main thread during call() - // 2. If it's 0, there's definitely nothing to cleanup - // 3. If it becomes non-zero after we check, cleanup will happen on the next loop iteration - // 4. Not all platforms support atomics, so we accept this race in favor of performance - // 5. The worst case is a one-loop-iteration delay in cleanup, which is harmless - if (this->to_remove_ == 0) - return this->items_.size(); +bool HOT Scheduler::cleanup_() { + // Fast path: if nothing to remove, just check if items exist. + // Uses atomic load on platforms with atomics, falls back to always taking the lock otherwise. + // Worst case is a one-loop-iteration delay in cleanup. + if (this->to_remove_empty_()) + return !this->items_.empty(); // We must hold the lock for the entire cleanup operation because: // 1. We're modifying items_ (via pop_raw_locked_) which requires exclusive access @@ -643,10 +653,10 @@ size_t HOT Scheduler::cleanup_() { SchedulerItem *item = this->items_[0]; if (!this->is_item_removed_locked_(item)) break; - this->to_remove_--; + this->to_remove_decrement_(); this->recycle_item_main_loop_(this->pop_raw_locked_()); } - return this->items_.size(); + return !this->items_.empty(); } Scheduler::SchedulerItem *HOT Scheduler::pop_raw_locked_() { std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); @@ -699,7 +709,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, NameType name_type size_t heap_cancelled = this->mark_matching_items_removed_locked_(this->items_, component, name_type, static_name, hash_or_id, type, match_retry); total_cancelled += heap_cancelled; - this->to_remove_ += heap_cancelled; + this->to_remove_add_(heap_cancelled); } // Cancel items in to_add_ diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 0476513bb9..e545055fca 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -284,9 +284,9 @@ class Scheduler { #endif } // Cleanup logically deleted items from the scheduler - // Returns the number of items remaining after cleanup + // Returns true if items remain after cleanup // IMPORTANT: This method should only be called from the main thread (loop task). - size_t cleanup_(); + bool cleanup_(); // Remove and return the front item from the heap as a raw pointer. // Caller takes ownership and must either recycle or delete the item. // IMPORTANT: Caller must hold the scheduler lock before calling this function. @@ -395,15 +395,9 @@ class Scheduler { // erase() on every pop, which would be O(n). The queue is processed once per loop - // any items added during processing are left for the next loop iteration. - // Snapshot the queue end point - only process items that existed at loop start - // Items added during processing (by callbacks or other threads) run next loop - // No lock needed: single consumer (main loop), stale read just means we process less this iteration - size_t defer_queue_end = this->defer_queue_.size(); - // Fast path: nothing to process, avoid lock entirely. - // Safe without lock: single consumer (main loop) reads front_, and a stale size() read - // from a concurrent push can only make us see fewer items — they'll be processed next loop. - if (this->defer_queue_front_ >= defer_queue_end) + // Worst case is a one-loop-iteration delay before newly deferred items are processed. + if (this->defer_empty_()) return; // Merge lock acquisitions: instead of separate locks for move-out and recycle (2N+1 total), @@ -412,6 +406,13 @@ class Scheduler { SchedulerItem *item; this->lock_.lock(); + // Reset counter and snapshot queue end under lock + this->defer_count_clear_(); + size_t defer_queue_end = this->defer_queue_.size(); + if (this->defer_queue_front_ >= defer_queue_end) { + this->lock_.unlock(); + return; + } while (this->defer_queue_front_ < defer_queue_end) { // Take ownership of the item, leaving nullptr in the vector slot. // This is safe because: @@ -527,14 +528,150 @@ class Scheduler { Mutex lock_; std::vector items_; std::vector to_add_; + +#ifndef ESPHOME_THREAD_SINGLE + // Fast-path counter for process_to_add() to skip taking the lock when there is + // nothing to add. Uses std::atomic on platforms that support it, plain uint32_t + // otherwise. On non-atomic platforms, callers must hold the scheduler lock when + // mutating this counter. Not needed on single-threaded platforms where we can + // check to_add_.empty() directly. +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + std::atomic to_add_count_{0}; +#else + uint32_t to_add_count_{0}; +#endif +#endif /* ESPHOME_THREAD_SINGLE */ + + // Fast-path helper for process_to_add() to decide if it can try the lock-free path. + // - On ESPHOME_THREAD_SINGLE: direct container check is safe (no concurrent writers). + // - On ESPHOME_THREAD_MULTI_ATOMICS: performs a lock-free check via to_add_count_. + // - On ESPHOME_THREAD_MULTI_NO_ATOMICS: always returns false to force the caller + // down the locked path; this is NOT a lock-free emptiness check on that platform. + bool to_add_empty_() const { +#ifdef ESPHOME_THREAD_SINGLE + return this->to_add_.empty(); +#elif defined(ESPHOME_THREAD_MULTI_ATOMICS) + return this->to_add_count_.load(std::memory_order_relaxed) == 0; +#else + return false; +#endif + } + + // Increment to_add_count_ (no-op on single-threaded platforms) + void to_add_count_increment_() { +#ifdef ESPHOME_THREAD_SINGLE + // No counter needed — to_add_empty_() checks the vector directly +#elif defined(ESPHOME_THREAD_MULTI_ATOMICS) + this->to_add_count_.fetch_add(1, std::memory_order_relaxed); +#else + this->to_add_count_++; +#endif + } + + // Reset to_add_count_ (no-op on single-threaded platforms) + void to_add_count_clear_() { +#ifdef ESPHOME_THREAD_SINGLE + // No counter needed — to_add_empty_() checks the vector directly +#elif defined(ESPHOME_THREAD_MULTI_ATOMICS) + this->to_add_count_.store(0, std::memory_order_relaxed); +#else + this->to_add_count_ = 0; +#endif + } + #ifndef ESPHOME_THREAD_SINGLE // Single-core platforms don't need the defer queue and save ~32 bytes of RAM // Using std::vector instead of std::deque avoids 512-byte chunked allocations // Index tracking avoids O(n) erase() calls when draining the queue each loop std::vector defer_queue_; // FIFO queue for defer() calls size_t defer_queue_front_{0}; // Index of first valid item in defer_queue_ (tracks consumed items) -#endif /* ESPHOME_THREAD_SINGLE */ + + // Fast-path counter for process_defer_queue_() to skip lock when nothing to process. +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + std::atomic defer_count_{0}; +#else + uint32_t defer_count_{0}; +#endif + + bool defer_empty_() const { + // defer_queue_ only exists on multi-threaded platforms, so no ESPHOME_THREAD_SINGLE path + // ESPHOME_THREAD_MULTI_NO_ATOMICS: always take the lock +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + return this->defer_count_.load(std::memory_order_relaxed) == 0; +#else + return false; +#endif + } + + void defer_count_increment_() { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + this->defer_count_.fetch_add(1, std::memory_order_relaxed); +#else + this->defer_count_++; +#endif + } + + void defer_count_clear_() { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + this->defer_count_.store(0, std::memory_order_relaxed); +#else + this->defer_count_ = 0; +#endif + } + +#endif /* ESPHOME_THREAD_SINGLE */ + + // Counter for items marked for removal. Incremented cross-thread in cancel_item_locked_(). + // On ESPHOME_THREAD_MULTI_ATOMICS this is read without a lock in the cleanup_() fast path; + // on ESPHOME_THREAD_MULTI_NO_ATOMICS the fast path is disabled so cleanup_() always takes the lock. +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + std::atomic to_remove_{0}; +#else uint32_t to_remove_{0}; +#endif + + // Lock-free check if there are items to remove (for fast-path in cleanup_) + bool to_remove_empty_() const { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + return this->to_remove_.load(std::memory_order_relaxed) == 0; +#elif defined(ESPHOME_THREAD_SINGLE) + return this->to_remove_ == 0; +#else + return false; // Always take the lock path +#endif + } + + void to_remove_add_(uint32_t count) { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + this->to_remove_.fetch_add(count, std::memory_order_relaxed); +#else + this->to_remove_ += count; +#endif + } + + void to_remove_decrement_() { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + this->to_remove_.fetch_sub(1, std::memory_order_relaxed); +#else + this->to_remove_--; +#endif + } + + void to_remove_clear_() { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + this->to_remove_.store(0, std::memory_order_relaxed); +#else + this->to_remove_ = 0; +#endif + } + + uint32_t to_remove_count_() const { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + return this->to_remove_.load(std::memory_order_relaxed); +#else + return this->to_remove_; +#endif + } // Memory pool for recycling SchedulerItem objects to reduce heap churn. // Design decisions: From cdf05263df84e6967210f99aeece5e097d51cfed Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 14:26:24 -1000 Subject: [PATCH 17/17] [tests] Fix test_show_logs_serial taking 30s due to unmocked serial port wait The test was calling show_logs() which invokes _wait_for_serial_port("/dev/ttyUSB0"). Since that path doesn't exist in CI/dev environments, it spun in a 30-second polling loop before returning. Add mock_wait_for_serial_port fixture to skip the wait. --- tests/unit_tests/test_main.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index b853461151..5e36c06bb3 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -167,6 +167,13 @@ def mock_run_miniterm() -> Generator[Mock]: yield mock +@pytest.fixture +def mock_wait_for_serial_port() -> Generator[Mock]: + """Mock _wait_for_serial_port for testing.""" + with patch("esphome.__main__._wait_for_serial_port") as mock: + yield mock + + @pytest.fixture def mock_upload_using_esptool() -> Generator[Mock]: """Mock upload_using_esptool for testing.""" @@ -1706,6 +1713,7 @@ def test_show_logs_serial( mock_get_port_type: Mock, mock_check_permissions: Mock, mock_run_miniterm: Mock, + mock_wait_for_serial_port: Mock, ) -> None: """Test show_logs with serial port.""" setup_core(config={"logger": {}}, platform=PLATFORM_ESP32)