From 2f433c78bd1294748b5c3d0c57dbb10a27ed0ef2 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 18:56:36 -0400 Subject: [PATCH 01/16] [haier] Brace single-statement else-if in smartair2_climate (#16098) --- esphome/components/haier/smartair2_climate.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/esphome/components/haier/smartair2_climate.cpp b/esphome/components/haier/smartair2_climate.cpp index 2be5d13050..200cac2557 100644 --- a/esphome/components/haier/smartair2_climate.cpp +++ b/esphome/components/haier/smartair2_climate.cpp @@ -210,8 +210,9 @@ void Smartair2Climate::process_phase(std::chrono::steady_clock::time_point now) #ifdef USE_WIFI else if (this->send_wifi_signal_ && (std::chrono::duration_cast(now - this->last_signal_request_).count() > - SIGNAL_LEVEL_UPDATE_INTERVAL_MS)) + SIGNAL_LEVEL_UPDATE_INTERVAL_MS)) { this->set_phase(ProtocolPhases::SENDING_UPDATE_SIGNAL_REQUEST); + } #endif } break; default: From a241c9e622e623d5d0c20cfcc02ce44877de7428 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:02:39 -0400 Subject: [PATCH 02/16] [online_image][sim800l] Use std::string::starts_with for prefix checks (#16097) --- .../components/online_image/online_image.cpp | 2 +- esphome/components/sim800l/sim800l.cpp | 21 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/esphome/components/online_image/online_image.cpp b/esphome/components/online_image/online_image.cpp index 24926aa4dc..a5a3ea5104 100644 --- a/esphome/components/online_image/online_image.cpp +++ b/esphome/components/online_image/online_image.cpp @@ -28,7 +28,7 @@ bool OnlineImage::validate_url_(const std::string &url) { ESP_LOGE(TAG, "URL is too long"); return false; } - if (url.compare(0, 7, "http://") != 0 && url.compare(0, 8, "https://") != 0) { + if (!url.starts_with("http://") && !url.starts_with("https://")) { ESP_LOGE(TAG, "URL must start with http:// or https://"); return false; } diff --git a/esphome/components/sim800l/sim800l.cpp b/esphome/components/sim800l/sim800l.cpp index 913d920c94..001ec77454 100644 --- a/esphome/components/sim800l/sim800l.cpp +++ b/esphome/components/sim800l/sim800l.cpp @@ -110,7 +110,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { case STATE_INIT: { // While we were waiting for update to check for messages, this notifies a message // is available. - bool message_available = message.compare(0, 6, "+CMTI:") == 0; + bool message_available = message.starts_with("+CMTI:"); if (!message_available) { if (message == "RING") { // Incoming call... @@ -120,7 +120,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { this->call_state_ = 6; this->call_disconnected_callback_.call(); } - } else if (message.compare(0, 6, "+CUSD:") == 0) { + } else if (message.starts_with("+CUSD:")) { // Incoming USSD MESSAGE this->state_ = STATE_CHECK_USSD; } @@ -175,7 +175,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { break; case STATE_CHECK_USSD: ESP_LOGD(TAG, "Check ussd code: '%s'", message.c_str()); - if (message.compare(0, 6, "+CUSD:") == 0) { + if (message.starts_with("+CUSD:")) { this->state_ = STATE_RECEIVED_USSD; this->ussd_ = ""; size_t start = 10; @@ -196,8 +196,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { case STATE_CREG_WAIT: { // Response: "+CREG: 0,1" -- the one there means registered ok // "+CREG: -,-" means not registered ok - bool registered = - message.size() > 9 && message.compare(0, 6, "+CREG:") == 0 && (message[9] == '1' || message[9] == '5'); + bool registered = message.size() > 9 && message.starts_with("+CREG:") && (message[9] == '1' || message[9] == '5'); if (registered) { if (!this->registered_) { ESP_LOGD(TAG, "Registered OK"); @@ -223,7 +222,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { this->state_ = STATE_CSQ_RESPONSE; break; case STATE_CSQ_RESPONSE: - if (message.compare(0, 5, "+CSQ:") == 0) { + if (message.starts_with("+CSQ:")) { size_t comma = message.find(',', 6); if (comma != 6) { int rssi = parse_number(message.substr(6, comma - 6)).value_or(0); @@ -243,7 +242,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { this->state_ = STATE_CHECK_SMS; break; case STATE_PARSE_SMS_RESPONSE: - if (message.compare(0, 6, "+CMGL:") == 0 && this->parse_index_ == 0) { + if (message.starts_with("+CMGL:") && this->parse_index_ == 0) { size_t start = 7; size_t end = message.find(',', start); uint8_t item = 0; @@ -278,7 +277,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { } break; case STATE_CHECK_CALL: - if (message.compare(0, 6, "+CLCC:") == 0 && this->parse_index_ == 0) { + if (message.starts_with("+CLCC:") && this->parse_index_ == 0) { this->expect_ack_ = true; size_t start = 7; size_t end = message.find(',', start); @@ -324,7 +323,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { /* Our recipient is set and the message body is in message kick ESPHome callback now */ - if (ok || message.compare(0, 6, "+CMGL:") == 0) { + if (ok || message.starts_with("+CMGL:")) { ESP_LOGD(TAG, "Received SMS from: %s\n" " %s", @@ -360,7 +359,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { } break; case STATE_SENDING_SMS_3: - if (message.compare(0, 6, "+CMGS:") == 0) { + if (message.starts_with("+CMGS:")) { ESP_LOGD(TAG, "SMS Sent OK: %s", message.c_str()); this->send_pending_ = false; this->state_ = STATE_CHECK_SMS; @@ -383,7 +382,7 @@ void Sim800LComponent::parse_cmd_(std::string message) { this->state_ = STATE_INIT; break; case STATE_PARSE_CLIP: - if (message.compare(0, 6, "+CLIP:") == 0) { + if (message.starts_with("+CLIP:")) { std::string caller_id; size_t start = 7; size_t end = message.find(',', start); From be0ee738474d6535bcb48e7c5e5c274f07577ae2 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:22:42 -0400 Subject: [PATCH 03/16] [i2c] NOLINT readability-identifier-naming on Zephyr struct forward-decl (#16099) --- esphome/components/i2c/i2c_bus_zephyr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/components/i2c/i2c_bus_zephyr.h b/esphome/components/i2c/i2c_bus_zephyr.h index 49cac5b992..3c4aa9ed1d 100644 --- a/esphome/components/i2c/i2c_bus_zephyr.h +++ b/esphome/components/i2c/i2c_bus_zephyr.h @@ -5,7 +5,7 @@ #include "i2c_bus.h" #include "esphome/core/component.h" -struct device; +struct device; // NOLINT(readability-identifier-naming) - forward decl of Zephyr's device type namespace esphome::i2c { From 15df47747267cb6e287738a7acb12edd8ad04a5a Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:41:28 -0400 Subject: [PATCH 04/16] [core] Reduce copies in Callback/CallbackManager call paths (#16093) Co-authored-by: J. Nick Koston --- esphome/core/helpers.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 4a91c46074..b2b07c57a0 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -1666,7 +1666,7 @@ template struct Callback { void *ctx_{nullptr}; /// Invoke the callback. Only valid on Callbacks created via create(), never on default-constructed instances. - void call(Ts... args) const { this->fn_(this->ctx_, args...); } + void call(Ts... args) const { this->fn_(this->ctx_, std::forward(args)...); } /// Create from any callable. Small trivially-copyable callables (like [this] lambdas) /// are stored inline in the ctx pointer without heap allocation. @@ -1742,7 +1742,7 @@ template class CallbackManager { template void add(F &&callback) { this->add_(CbType::create(std::forward(callback))); } /// Call all callbacks in this manager. - inline void ESPHOME_ALWAYS_INLINE call(Ts... args) { + inline void ESPHOME_ALWAYS_INLINE call(const Ts &...args) { if (this->size_ != 0) { for (auto *it = this->data_, *end = it + this->size_; it != end; ++it) { it->call(args...); @@ -1752,7 +1752,7 @@ template class CallbackManager { uint16_t size() const { return this->size_; } /// Call all callbacks in this manager. - void operator()(Ts... args) { this->call(args...); } + void operator()(const Ts &...args) { this->call(args...); } protected: template friend class LazyCallbackManager; From 0b5835284aaab1c77810ec8c0dea4723f7858f48 Mon Sep 17 00:00:00 2001 From: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com> Date: Wed, 29 Apr 2026 10:35:24 +1000 Subject: [PATCH 05/16] [lvgl] Additional layout features (#16041) --- esphome/components/lvgl/layout.py | 119 +++++++-- tests/component_tests/lvgl/__init__.py | 0 .../component_tests/lvgl/test_grid_layout.py | 239 ++++++++++++++++++ tests/components/lvgl/lvgl-package.yaml | 83 ++++++ 4 files changed, 419 insertions(+), 22 deletions(-) create mode 100644 tests/component_tests/lvgl/__init__.py create mode 100644 tests/component_tests/lvgl/test_grid_layout.py diff --git a/esphome/components/lvgl/layout.py b/esphome/components/lvgl/layout.py index 46026852af..32304276d3 100644 --- a/esphome/components/lvgl/layout.py +++ b/esphome/components/lvgl/layout.py @@ -1,3 +1,4 @@ +import math import re import textwrap @@ -85,6 +86,22 @@ def grid_free_space(value): grid_spec = cv.Any(size, LvConstant("LV_GRID_", "CONTENT").one_of, grid_free_space) + +def grid_dimension(value): + """ + Validator for a grid `rows` or `columns` value. + Accepts either a positive integer (interpreted as that many cells of equal + `LV_GRID_FR(1)` size) or a non-empty list of grid specs. + """ + if isinstance(value, int): + value = cv.int_range(min=1)(value) + return ["LV_GRID_FR(1)"] * value + result = cv.Schema([grid_spec])(value) + if not result: + raise cv.Invalid("Grid dimension list must contain at least one entry") + return result + + GRID_CELL_SCHEMA = { cv.Optional(CONF_GRID_CELL_ROW_POS): cv.positive_int, cv.Optional(CONF_GRID_CELL_COLUMN_POS): cv.positive_int, @@ -184,7 +201,16 @@ class DirectionalLayout(FlexLayout): class GridLayout(Layout): - _GRID_LAYOUT_REGEX = re.compile(r"^\s*(\d+)\s*x\s*(\d+)\s*$") + # Match shorthand grid layout strings: "NxM", "Nx" or "xM". + # At least one of the two numbers must be present; this is enforced after matching. + _GRID_LAYOUT_REGEX = re.compile(r"^\s*(\d+)?\s*x\s*(\d+)?\s*$") + + @staticmethod + def _match_shorthand(layout): + match = GridLayout._GRID_LAYOUT_REGEX.match(layout) + if match is None or (match.group(1) is None and match.group(2) is None): + return None + return match def get_type(self): return TYPE_GRID @@ -192,7 +218,7 @@ class GridLayout(Layout): def get_layout_schemas(self, config: dict) -> tuple: layout = config.get(CONF_LAYOUT) if isinstance(layout, str): - if GridLayout._GRID_LAYOUT_REGEX.match(layout): + if GridLayout._match_shorthand(layout): return ( cv.string, { @@ -213,59 +239,107 @@ class GridLayout(Layout): if not isinstance(layout, dict) or layout.get(CONF_TYPE).lower() != TYPE_GRID: return None, {} + x_default = ( + "center" if isinstance(layout.get(CONF_GRID_ROWS), int) else cv.UNDEFINED + ) + y_default = ( + "center" if isinstance(layout.get(CONF_GRID_COLUMNS), int) else cv.UNDEFINED + ) + x_align = layout.get(CONF_GRID_CELL_X_ALIGN, x_default) + y_align = layout.get(CONF_GRID_CELL_Y_ALIGN, y_default) return ( { cv.Required(CONF_TYPE): cv.one_of(TYPE_GRID, lower=True), - cv.Required(CONF_GRID_ROWS): [grid_spec], - cv.Required(CONF_GRID_COLUMNS): [grid_spec], + cv.Optional(CONF_GRID_ROWS): grid_dimension, + cv.Optional(CONF_GRID_COLUMNS): grid_dimension, cv.Optional(CONF_GRID_COLUMN_ALIGN): grid_alignments, cv.Optional(CONF_GRID_ROW_ALIGN): grid_alignments, cv.Optional(CONF_PAD_ROW): padding, cv.Optional(CONF_PAD_COLUMN): padding, cv.Optional(CONF_MULTIPLE_WIDGETS_PER_CELL, default=False): cv.boolean, + cv.Optional(CONF_GRID_CELL_X_ALIGN): grid_alignments, + cv.Optional(CONF_GRID_CELL_Y_ALIGN): grid_alignments, }, { cv.Optional(CONF_GRID_CELL_ROW_POS): cv.positive_int, cv.Optional(CONF_GRID_CELL_COLUMN_POS): cv.positive_int, cv.Optional(CONF_GRID_CELL_ROW_SPAN): cv.int_range(min=1), cv.Optional(CONF_GRID_CELL_COLUMN_SPAN): cv.int_range(min=1), - cv.Optional(CONF_GRID_CELL_X_ALIGN): grid_alignments, - cv.Optional(CONF_GRID_CELL_Y_ALIGN): grid_alignments, + cv.Optional(CONF_GRID_CELL_X_ALIGN, default=x_align): grid_alignments, + cv.Optional(CONF_GRID_CELL_Y_ALIGN, default=y_align): grid_alignments, }, ) def validate(self, config: dict): """ Validate the grid layout. - The `layout:` key may be a dictionary with `rows` and `columns` keys, or a string in the format "rows x columns". + The `layout:` key may be a dictionary with `rows` and/or `columns` keys, or a + shorthand string in the format "x", "x" or "x". + Either dimension may be omitted, in which case it will be calculated from the + other dimension and the number of configured widgets. Either all cells must have a row and column, or none, in which case the grid layout is auto-generated. :param config: :return: The config updated with auto-generated values """ layout = config.get(CONF_LAYOUT) + widgets = config.get(CONF_WIDGETS, []) + num_widgets = len(widgets) if isinstance(layout, str): - # If the layout is a string, assume it is in the format "rows x columns", implying - # a grid layout with the specified number of rows and columns each with CONTENT sizing. + # Shorthand string: "x", "x" or "x". + # Each dimension defaults to LV_GRID_FR(1). A missing dimension is + # calculated from the other dimension and the number of widgets. layout = layout.strip() - match = GridLayout._GRID_LAYOUT_REGEX.match(layout) - if match: - rows = int(match.group(1)) - cols = int(match.group(2)) - layout = { - CONF_TYPE: TYPE_GRID, - CONF_GRID_ROWS: ["LV_GRID_FR(1)"] * rows, - CONF_GRID_COLUMNS: ["LV_GRID_FR(1)"] * cols, - } - config[CONF_LAYOUT] = layout - else: + match = GridLayout._match_shorthand(layout) + if not match: raise cv.Invalid( - f"Invalid grid layout format: {config}, expected 'rows x columns'", + f"Invalid grid layout format: {layout!r}, expected " + "'x', 'x' or 'x'", [CONF_LAYOUT], ) + rows_int = int(match.group(1)) if match.group(1) is not None else None + cols_int = int(match.group(2)) if match.group(2) is not None else None + for label, val in (("row", rows_int), ("column", cols_int)): + if val is not None and val < 1: + raise cv.Invalid( + f"Invalid grid layout {layout!r}: {label} count must be " + "at least 1", + [CONF_LAYOUT], + ) + if rows_int is not None and cols_int is not None: + rows = rows_int + cols = cols_int + elif rows_int is not None: + rows = rows_int + cols = max(1, math.ceil(num_widgets / rows)) if num_widgets else 1 + else: + cols = cols_int + rows = max(1, math.ceil(num_widgets / cols)) if num_widgets else 1 + layout = { + CONF_TYPE: TYPE_GRID, + CONF_GRID_ROWS: ["LV_GRID_FR(1)"] * rows, + CONF_GRID_COLUMNS: ["LV_GRID_FR(1)"] * cols, + } + config[CONF_LAYOUT] = layout # should be guaranteed to be a dict at this point assert isinstance(layout, dict) assert layout.get(CONF_TYPE).lower() == TYPE_GRID + rows_list = layout.get(CONF_GRID_ROWS) + cols_list = layout.get(CONF_GRID_COLUMNS) + if rows_list is None and cols_list is None: + raise cv.Invalid( + "Grid layout requires at least one of 'rows' or 'columns' to be " + "specified", + [CONF_LAYOUT], + ) + if rows_list is None: + cols = len(cols_list) + rows = max(1, math.ceil(num_widgets / cols)) if num_widgets else 1 + layout[CONF_GRID_ROWS] = ["LV_GRID_FR(1)"] * rows + elif cols_list is None: + rows = len(rows_list) + cols = max(1, math.ceil(num_widgets / rows)) if num_widgets else 1 + layout[CONF_GRID_COLUMNS] = ["LV_GRID_FR(1)"] * cols allow_multiple = layout.get(CONF_MULTIPLE_WIDGETS_PER_CELL, False) rows = len(layout[CONF_GRID_ROWS]) columns = len(layout[CONF_GRID_COLUMNS]) @@ -379,7 +453,8 @@ def append_layout_schema(schema, config: dict): textwrap.dedent( """ Invalid 'layout' value - layout choices are 'horizontal', 'vertical', 'x', + layout choices are 'horizontal', 'vertical', + 'x', 'x', 'x', or a dictionary with a 'type' key """ ), diff --git a/tests/component_tests/lvgl/__init__.py b/tests/component_tests/lvgl/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/component_tests/lvgl/test_grid_layout.py b/tests/component_tests/lvgl/test_grid_layout.py new file mode 100644 index 0000000000..dfd4b2460c --- /dev/null +++ b/tests/component_tests/lvgl/test_grid_layout.py @@ -0,0 +1,239 @@ +"""Unit tests for the LVGL grid layout shorthand and rows/columns auto-sizing.""" + +from __future__ import annotations + +import pytest +from voluptuous import Invalid + +from esphome.components.lvgl.defines import ( + CONF_GRID_COLUMNS, + CONF_GRID_ROWS, + CONF_LAYOUT, + CONF_WIDGETS, + TYPE_GRID, +) +from esphome.components.lvgl.layout import GridLayout, grid_dimension +from esphome.const import CONF_TYPE + +FR1 = "LV_GRID_FR(1)" + + +def _widgets(n: int) -> list[dict]: + """Build a list of `n` placeholder widgets for the validate() input.""" + return [{"label": {}} for _ in range(n)] + + +# --------------------------------------------------------------------------- +# grid_dimension validator +# --------------------------------------------------------------------------- + + +def test_grid_dimension_int_expands_to_fr1_list() -> None: + """A positive integer should expand to a list of LV_GRID_FR(1) entries.""" + assert grid_dimension(1) == [FR1] + assert grid_dimension(3) == [FR1, FR1, FR1] + + +def test_grid_dimension_zero_or_negative_rejected() -> None: + """Non-positive integers must be rejected.""" + with pytest.raises(Invalid): + grid_dimension(0) + with pytest.raises(Invalid): + grid_dimension(-2) + + +def test_grid_dimension_list_passes_through() -> None: + """A list should be validated through the existing grid_spec list schema.""" + result = grid_dimension(["100px", "content", "fr(2)"]) + # `grid_spec` normalises each entry: pixel sizes become ints, the + # CONTENT keyword is uppercased and prefixed, and FR(n) is normalised. + assert result == [100, "LV_GRID_CONTENT", "LV_GRID_FR(2)"] + + +def test_grid_dimension_invalid_string_rejected() -> None: + """A string is not a valid grid dimension and should be rejected.""" + with pytest.raises(Invalid): + grid_dimension("not a list") + + +def test_grid_dimension_empty_list_rejected() -> None: + """An empty list of grid specs must be rejected.""" + with pytest.raises(Invalid, match="at least one entry"): + grid_dimension([]) + + +# --------------------------------------------------------------------------- +# Shorthand string layouts +# --------------------------------------------------------------------------- + + +def test_shorthand_full_form_unchanged() -> None: + """`x` continues to work and yields the exact dimensions.""" + config = {CONF_LAYOUT: "2x3", CONF_WIDGETS: _widgets(0)} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + assert layout[CONF_TYPE] == TYPE_GRID + assert layout[CONF_GRID_ROWS] == [FR1, FR1] + assert layout[CONF_GRID_COLUMNS] == [FR1, FR1, FR1] + + +def test_shorthand_rows_only_calculates_columns_from_widgets() -> None: + """`x` derives the column count from the number of widgets.""" + config = {CONF_LAYOUT: "3x", CONF_WIDGETS: _widgets(7)} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + # 7 widgets / 3 rows -> ceil = 3 columns. + assert len(layout[CONF_GRID_ROWS]) == 3 + assert len(layout[CONF_GRID_COLUMNS]) == 3 + + +def test_shorthand_columns_only_calculates_rows_from_widgets() -> None: + """`x` derives the row count from the number of widgets.""" + config = {CONF_LAYOUT: "x4", CONF_WIDGETS: _widgets(5)} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + # 5 widgets / 4 cols -> ceil = 2 rows. + assert len(layout[CONF_GRID_ROWS]) == 2 + assert len(layout[CONF_GRID_COLUMNS]) == 4 + + +def test_shorthand_rows_only_no_widgets_defaults_columns_to_one() -> None: + """With no widgets and only rows specified, the column count defaults to 1.""" + config = {CONF_LAYOUT: "3x", CONF_WIDGETS: []} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + assert len(layout[CONF_GRID_ROWS]) == 3 + assert len(layout[CONF_GRID_COLUMNS]) == 1 + + +def test_shorthand_columns_only_no_widgets_defaults_rows_to_one() -> None: + """With no widgets and only columns specified, the row count defaults to 1.""" + config = {CONF_LAYOUT: "x4", CONF_WIDGETS: []} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + assert len(layout[CONF_GRID_ROWS]) == 1 + assert len(layout[CONF_GRID_COLUMNS]) == 4 + + +def test_shorthand_with_whitespace_accepted() -> None: + """The shorthand parser should tolerate whitespace around the components.""" + config = {CONF_LAYOUT: " 3 x ", CONF_WIDGETS: _widgets(6)} + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + # 6 widgets / 3 rows -> 2 columns. + assert len(layout[CONF_GRID_ROWS]) == 3 + assert len(layout[CONF_GRID_COLUMNS]) == 2 + + +def test_shorthand_bare_x_rejected() -> None: + """Pure `x` (no digits at all) is not a valid shorthand.""" + config = {CONF_LAYOUT: "x", CONF_WIDGETS: _widgets(2)} + with pytest.raises(Invalid): + GridLayout().validate(config) + + +@pytest.mark.parametrize( + "layout,bad_label", + [ + ("0x3", "row"), + ("3x0", "column"), + ("0x", "row"), + ("x0", "column"), + ("0x0", "row"), + ], +) +def test_shorthand_zero_dimension_rejected(layout: str, bad_label: str) -> None: + """Shorthand row/column counts must be >= 1.""" + config = {CONF_LAYOUT: layout, CONF_WIDGETS: _widgets(2)} + with pytest.raises(Invalid, match=f"{bad_label} count must be at least 1"): + GridLayout().validate(config) + + +def test_shorthand_get_layout_schemas_recognizes_partial_forms() -> None: + """`x` and `x` should be picked up by GridLayout.get_layout_schemas.""" + grid = GridLayout() + for layout in ("3x", "x4", "2x3"): + layout_schema, _ = grid.get_layout_schemas({CONF_LAYOUT: layout}) + assert layout_schema is not None, f"{layout!r} should be recognised" + # Pure `x` and unrelated strings should not be picked up as a grid layout. + for layout in ("x", "horizontal"): + layout_schema, _ = grid.get_layout_schemas({CONF_LAYOUT: layout}) + assert layout_schema is None, f"{layout!r} should not be recognised" + + +# --------------------------------------------------------------------------- +# Dict-form layouts with rows/columns auto-sizing +# --------------------------------------------------------------------------- + + +def test_dict_rows_only_calculates_columns_from_widgets() -> None: + """A dict layout with only rows fills in the column count from widget count.""" + config = { + CONF_LAYOUT: { + CONF_TYPE: TYPE_GRID, + CONF_GRID_ROWS: [FR1, FR1], + }, + CONF_WIDGETS: _widgets(5), + } + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + # 5 widgets / 2 rows -> ceil = 3 columns. + assert len(layout[CONF_GRID_ROWS]) == 2 + assert layout[CONF_GRID_COLUMNS] == [FR1, FR1, FR1] + + +def test_dict_columns_only_calculates_rows_from_widgets() -> None: + """A dict layout with only columns fills in the row count from widget count.""" + config = { + CONF_LAYOUT: { + CONF_TYPE: TYPE_GRID, + CONF_GRID_COLUMNS: [FR1, FR1, FR1], + }, + CONF_WIDGETS: _widgets(7), + } + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + # 7 widgets / 3 cols -> ceil = 3 rows. + assert layout[CONF_GRID_ROWS] == [FR1, FR1, FR1] + assert len(layout[CONF_GRID_COLUMNS]) == 3 + + +def test_dict_rows_only_no_widgets_defaults_columns_to_one() -> None: + """A dict layout with rows but no widgets defaults columns to 1.""" + config = { + CONF_LAYOUT: { + CONF_TYPE: TYPE_GRID, + CONF_GRID_ROWS: [FR1, FR1, FR1], + }, + CONF_WIDGETS: [], + } + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + assert len(layout[CONF_GRID_ROWS]) == 3 + assert layout[CONF_GRID_COLUMNS] == [FR1] + + +def test_dict_neither_rows_nor_columns_rejected() -> None: + """A grid layout dict without rows AND without columns must be rejected.""" + config = { + CONF_LAYOUT: {CONF_TYPE: TYPE_GRID}, + CONF_WIDGETS: _widgets(3), + } + with pytest.raises(Invalid): + GridLayout().validate(config) + + +def test_dict_both_rows_and_columns_unchanged() -> None: + """When both dimensions are present they are preserved as-is.""" + config = { + CONF_LAYOUT: { + CONF_TYPE: TYPE_GRID, + CONF_GRID_ROWS: [FR1, FR1], + CONF_GRID_COLUMNS: [FR1, FR1, FR1], + }, + CONF_WIDGETS: _widgets(0), + } + result = GridLayout().validate(config) + layout = result[CONF_LAYOUT] + assert layout[CONF_GRID_ROWS] == [FR1, FR1] + assert layout[CONF_GRID_COLUMNS] == [FR1, FR1, FR1] diff --git a/tests/components/lvgl/lvgl-package.yaml b/tests/components/lvgl/lvgl-package.yaml index d6e237199a..9c4ad4bbf8 100644 --- a/tests/components/lvgl/lvgl-package.yaml +++ b/tests/components/lvgl/lvgl-package.yaml @@ -1113,6 +1113,8 @@ lvgl: pad_row: 6px pad_column: 0 multiple_widgets_per_cell: true + grid_cell_x_align: center + grid_cell_y_align: center widgets: - image: grid_cell_row_pos: 0 @@ -1305,6 +1307,87 @@ lvgl: hidden: true mode: text_lower + # Grid shorthand "x": 3 rows specified, columns derived + # from widget count (4 widgets / 3 rows -> 2 columns) + - obj: + id: grid_rows_only_shorthand + layout: 3x + widgets: + - label: + text: "r1" + - label: + text: "r2" + - label: + text: "r3" + - label: + text: "r4" + + # Grid shorthand "x": 4 columns specified, rows derived + # from widget count (5 widgets / 4 cols -> 2 rows) + - obj: + id: grid_cols_only_shorthand + layout: x4 + widgets: + - label: + text: "a" + - label: + text: "b" + - label: + text: "c" + - label: + text: "d" + - label: + text: "e" + + # Grid dict form with grid_rows as a plain integer; columns derived + - obj: + id: grid_rows_int + layout: + type: grid + grid_rows: 2 + widgets: + - label: + text: "1" + - label: + text: "2" + - label: + text: "3" + + # Grid dict form with grid_columns as a plain integer; rows derived + - obj: + id: grid_cols_int + layout: + type: grid + grid_columns: 3 + widgets: + - label: + text: "x" + - label: + text: "y" + - label: + text: "z" + - label: + text: "w" + - label: + text: "v" + + # Grid dict form with both grid_rows and grid_columns as plain integers + - obj: + id: grid_both_int + layout: + type: grid + grid_rows: 2 + grid_columns: 2 + widgets: + - label: + text: "1,1" + - label: + text: "1,2" + - label: + text: "2,1" + - label: + text: "2,2" + font: - file: "gfonts://Roboto" id: space16 From 77b76ac48a41dcfda992199a0343ad06b8c5956a Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 20:56:03 -0400 Subject: [PATCH 06/16] [inkbird_ibsth1_mini][speaker][speaker_source] Fix performance-unnecessary-copy-initialization (#16101) --- .../components/inkbird_ibsth1_mini/inkbird_ibsth1_mini.cpp | 4 ++-- .../components/speaker/media_player/speaker_media_player.cpp | 2 +- .../components/speaker_source/speaker_source_media_player.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/esphome/components/inkbird_ibsth1_mini/inkbird_ibsth1_mini.cpp b/esphome/components/inkbird_ibsth1_mini/inkbird_ibsth1_mini.cpp index 94c22ae84d..c53d8e5029 100644 --- a/esphome/components/inkbird_ibsth1_mini/inkbird_ibsth1_mini.cpp +++ b/esphome/components/inkbird_ibsth1_mini/inkbird_ibsth1_mini.cpp @@ -41,12 +41,12 @@ bool InkbirdIbstH1Mini::parse_device(const esp32_ble_tracker::ESPBTDevice &devic ESP_LOGVV(TAG, "parse_device(): service_data is expected to be empty"); return false; } - auto mnf_datas = device.get_manufacturer_datas(); + const auto &mnf_datas = device.get_manufacturer_datas(); if (mnf_datas.size() != 1) { ESP_LOGVV(TAG, "parse_device(): manufacturer_datas is expected to have a single element"); return false; } - auto mnf_data = mnf_datas[0]; + const auto &mnf_data = mnf_datas[0]; if (mnf_data.uuid.get_uuid().len != ESP_UUID_LEN_16) { ESP_LOGVV(TAG, "parse_device(): manufacturer data element is expected to have uuid of length 16"); return false; diff --git a/esphome/components/speaker/media_player/speaker_media_player.cpp b/esphome/components/speaker/media_player/speaker_media_player.cpp index 930373c6fc..ab11a89c3f 100644 --- a/esphome/components/speaker/media_player/speaker_media_player.cpp +++ b/esphome/components/speaker/media_player/speaker_media_player.cpp @@ -502,7 +502,7 @@ void SpeakerMediaPlayer::control(const media_player::MediaPlayerCall &call) { media_command.announce = false; } - auto media_url = call.get_media_url(); + const auto &media_url = call.get_media_url(); if (media_url.has_value()) { media_command.url = new std::string(*media_url); // Must be manually deleted after receiving media_command from a queue diff --git a/esphome/components/speaker_source/speaker_source_media_player.cpp b/esphome/components/speaker_source/speaker_source_media_player.cpp index 2caab828fb..87fd4fe9ed 100644 --- a/esphome/components/speaker_source/speaker_source_media_player.cpp +++ b/esphome/components/speaker_source/speaker_source_media_player.cpp @@ -698,7 +698,7 @@ void SpeakerSourceMediaPlayer::control(const media_player::MediaPlayerCall &call } } - auto media_url = call.get_media_url(); + const auto &media_url = call.get_media_url(); if (media_url.has_value()) { auto command = call.get_command(); bool enqueue = command.has_value() && command.value() == media_player::MEDIA_PLAYER_COMMAND_ENQUEUE; From 29d3a3a4984b09d709e72b74252b1c2001ec68dc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 19:58:00 -0500 Subject: [PATCH 07/16] [esp8266] Replace millis() with fast accumulator, wrap Arduino callers (#15662) --- esphome/components/esp8266/__init__.py | 5 ++ esphome/components/esp8266/core.cpp | 80 +++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/esphome/components/esp8266/__init__.py b/esphome/components/esp8266/__init__.py index bef7e36470..34540bd48d 100644 --- a/esphome/components/esp8266/__init__.py +++ b/esphome/components/esp8266/__init__.py @@ -314,6 +314,11 @@ async def to_code(config): for symbol in ("vprintf", "printf", "fprintf"): cg.add_build_flag(f"-Wl,--wrap={symbol}") + # Wrap Arduino's millis() so all callers (including Arduino libraries and ISR + # handlers) use our fast accumulator instead of the expensive 4x 64-bit multiply + # implementation in the Arduino ESP8266 core. + cg.add_build_flag("-Wl,--wrap=millis") + cg.add_platformio_option("board_build.flash_mode", config[CONF_BOARD_FLASH_MODE]) ver: cv.Version = CORE.data[KEY_CORE][KEY_FRAMEWORK_VERSION] diff --git a/esphome/components/esp8266/core.cpp b/esphome/components/esp8266/core.cpp index 159ec20e77..c9bedb61be 100644 --- a/esphome/components/esp8266/core.cpp +++ b/esphome/components/esp8266/core.cpp @@ -16,9 +16,75 @@ extern "C" { namespace esphome { void HOT yield() { ::yield(); } -uint32_t IRAM_ATTR HOT millis() { return ::millis(); } -uint64_t millis_64() { return Millis64Impl::compute(::millis()); } -void HOT delay(uint32_t ms) { ::delay(ms); } +// Fast accumulator replacement for Arduino's millis() (~3.3 μs via 4× 64-bit +// multiplies on the LX106). Tracks a running ms counter from 32-bit +// system_get_time() deltas using pure 32-bit ops. Installed as __wrap_millis +// (via -Wl,--wrap=millis) so Arduino libs and IRAM_ATTR ISR handlers (e.g. +// Wiegand, ZyAura) also get the fast version. xt_rsil(15) guards the static +// state against ISR re-entry; the critical section is bounded (≤10 while-loop +// iterations, ~100 ns on the common path, or a constant-time /1000 ~2.5 μs on +// the rare path — well under WiFi's ~10 μs ISR latency budget). NMIs (level +// >15) are not masked, but the ESP8266 SDK's NMI handlers don't call millis(). +// +// system_get_time() wraps every ~71.6 min; unsigned (now_us - last_us) handles +// one wrap. The main loop calls millis() at 60+ Hz, so delta stays tiny — a +// >71 min block would trip the watchdog long before it could matter here. +static constexpr uint32_t MILLIS_RARE_PATH_THRESHOLD_US = 10000; +static constexpr uint32_t US_PER_MS = 1000; + +uint32_t IRAM_ATTR HOT millis() { + // Struct packs the three statics so the compiler loads one base address + // instead of three separate literal pool entries (saves ~8 bytes IRAM). + static struct { + uint32_t cache; + uint32_t remainder; + uint32_t last_us; + } state = {0, 0, 0}; + uint32_t ps = xt_rsil(15); + uint32_t now_us = system_get_time(); + uint32_t delta = now_us - state.last_us; + state.last_us = now_us; + state.remainder += delta; + if (state.remainder >= MILLIS_RARE_PATH_THRESHOLD_US) { + // Rare path: large gap (WiFi scan, boot, long block). Constant-time + // conversion keeps the critical section bounded. + uint32_t ms = state.remainder / US_PER_MS; + state.cache += ms; + // Reuse ms instead of `remainder %= US_PER_MS` — `%` would compile to a + // second __umodsi3 call on the LX106 (no hardware divide). + state.remainder -= ms * US_PER_MS; + } else { + // Common path: small gap. At most ~10 iterations since remainder was + // < threshold (10 ms) on entry and delta adds at most one more threshold + // before exiting this branch. + while (state.remainder >= US_PER_MS) { + state.cache++; + state.remainder -= US_PER_MS; + } + } + uint32_t result = state.cache; + xt_wsr_ps(ps); + return result; +} +uint64_t millis_64() { return Millis64Impl::compute(millis()); } +// Poll-based delay that avoids ::delay() — Arduino's __delay has an intra-object +// call to the original millis() that --wrap can't intercept, so calling ::delay() +// would keep the slow Arduino millis body alive in IRAM. optimistic_yield still +// enters esp_schedule()/esp_suspend_within_cont() via yield(), so SDK tasks and +// WiFi run correctly. Theoretically less power-efficient than Arduino's +// os_timer-based delay() for long waits, but nearly all ESPHome delays are short +// (sensor/I²C/SPI settling in the 1–100 ms range) where the difference is +// negligible. +void HOT delay(uint32_t ms) { + if (ms == 0) { + optimistic_yield(1000); + return; + } + uint32_t start = millis(); + while (millis() - start < ms) { + optimistic_yield(1000); + } +} uint32_t IRAM_ATTR HOT micros() { return ::micros(); } void IRAM_ATTR HOT delayMicroseconds(uint32_t us) { delay_microseconds_safe(us); } void arch_restart() { @@ -78,4 +144,12 @@ extern "C" void resetPins() { // NOLINT } // namespace esphome +// Linker wrap: redirect all ::millis() calls (Arduino libs, ISRs) to our accumulator. +// Requires -Wl,--wrap=millis in build flags (added by __init__.py). +// NOLINTNEXTLINE(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp,readability-identifier-naming) +extern "C" uint32_t IRAM_ATTR __wrap_millis() { return esphome::millis(); } +// Note: Arduino's init() registers a 60-second overflow timer for micros64(). +// We leave it running — wrapping init() as a no-op would break micros64()'s +// overflow tracking, and the timer's cost is negligible (~3 μs per 60 s). + #endif // USE_ESP8266 From 676f26919ec088cf0f9f6fafc498cc05d5d8cc26 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:02:21 -0500 Subject: [PATCH 08/16] [mdns] Drive MDNS.update() polling from IP state events on ESP8266/RP2040 (#15961) --- esphome/components/mdns/__init__.py | 39 +++++++++ esphome/components/mdns/mdns_component.h | 81 +++++++++++------ esphome/components/mdns/mdns_esp8266.cpp | 33 +++++-- esphome/components/mdns/mdns_rp2040.cpp | 86 ++++++++++++------- .../mdns/common-enabled-ethernet.yaml | 23 +++++ .../test-enabled-ethernet.rp2040-ard.yaml | 1 + 6 files changed, 202 insertions(+), 61 deletions(-) create mode 100644 tests/components/mdns/common-enabled-ethernet.yaml create mode 100644 tests/components/mdns/test-enabled-ethernet.rp2040-ard.yaml diff --git a/esphome/components/mdns/__init__.py b/esphome/components/mdns/__init__.py index 7c36295e8d..2b25cf243d 100644 --- a/esphome/components/mdns/__init__.py +++ b/esphome/components/mdns/__init__.py @@ -14,6 +14,7 @@ from esphome.const import ( from esphome.core import CORE, Lambda, coroutine_with_priority from esphome.coroutine import CoroPriority from esphome.cpp_generator import LambdaExpression +import esphome.final_validate as fv from esphome.types import ConfigType CODEOWNERS = ["@esphome/core"] @@ -61,6 +62,28 @@ def _consume_mdns_sockets(config: ConfigType) -> ConfigType: return config +def _require_network_interface(config: ConfigType) -> ConfigType: + """Require a network interface for mDNS on Arduino/LEAmDNS platforms. + + On ESP8266 and RP2040 the C++ implementation needs at least one IP state + listener (WiFi on ESP8266; WiFi or Ethernet on RP2040) to arm its polling + window. Reject at config time rather than silently producing a component + that never initializes. + """ + if config.get(CONF_DISABLED) or not (CORE.is_esp8266 or CORE.is_rp2040): + return config + full_config = fv.full_config.get() + has_wifi = "wifi" in full_config + has_ethernet = CORE.is_rp2040 and "ethernet" in full_config + if not (has_wifi or has_ethernet): + options = "'wifi'" if CORE.is_esp8266 else "'wifi' or 'ethernet'" + raise cv.Invalid( + "mdns on this platform requires a network interface — " + f"add a {options} component to your configuration." + ) + return config + + CONFIG_SCHEMA = cv.All( cv.Schema( { @@ -74,6 +97,9 @@ CONFIG_SCHEMA = cv.All( ) +FINAL_VALIDATE_SCHEMA = _require_network_interface + + def mdns_txt_record(key: str, value: str) -> cg.RawExpression: """Create a mDNS TXT record. @@ -169,6 +195,19 @@ async def to_code(config): elif CORE.is_rp2040: cg.add_library("LEAmDNS", None) + # Subscribe to the network IP state listener(s) so MDNS.update() is only + # scheduled during the probe+announce phase. Same on_ip_state() override + # serves both WiFi and Ethernet (signatures match). + if CORE.is_esp8266 or CORE.is_rp2040: + if "wifi" in CORE.config: + from esphome.components import wifi + + wifi.request_wifi_ip_state_listener() + if CORE.is_rp2040 and "ethernet" in CORE.config: + from esphome.components import ethernet + + ethernet.request_ethernet_ip_state_listener() + if CORE.is_esp32: add_idf_component(name="espressif/mdns", ref="1.11.0") diff --git a/esphome/components/mdns/mdns_component.h b/esphome/components/mdns/mdns_component.h index adf88a9cf1..798af0e0bf 100644 --- a/esphome/components/mdns/mdns_component.h +++ b/esphome/components/mdns/mdns_component.h @@ -5,6 +5,22 @@ #include "esphome/core/automation.h" #include "esphome/core/component.h" #include "esphome/core/helpers.h" +// On ESP8266 and RP2040 the scheduler-backed MDNS.update() polling window is armed by +// IP state listener events on whichever network interface is configured. +#if (defined(USE_ESP8266) || defined(USE_RP2040)) && \ + ((defined(USE_WIFI) && defined(USE_WIFI_IP_STATE_LISTENERS)) || \ + (defined(USE_ETHERNET) && defined(USE_ETHERNET_IP_STATE_LISTENERS))) +#include "esphome/components/network/ip_address.h" +#define USE_MDNS_EVENT_DRIVEN_POLLING +#if defined(USE_WIFI) && defined(USE_WIFI_IP_STATE_LISTENERS) +#include "esphome/components/wifi/wifi_component.h" +#define USE_MDNS_WIFI_LISTENER +#endif +#if defined(USE_ETHERNET) && defined(USE_ETHERNET_IP_STATE_LISTENERS) +#include "esphome/components/ethernet/ethernet_component.h" +#define USE_MDNS_ETHERNET_LISTENER +#endif +#endif namespace esphome::mdns { @@ -40,33 +56,40 @@ struct MDNSService { FixedVector txt_records; }; -class MDNSComponent final : public Component { +class MDNSComponent final : public Component +#ifdef USE_MDNS_WIFI_LISTENER + , + public wifi::WiFiIPStateListener +#endif +#ifdef USE_MDNS_ETHERNET_LISTENER + , + public ethernet::EthernetIPStateListener +#endif +{ public: void setup() override; void dump_config() override; - // Polling interval for MDNS.update() on platforms that require it (ESP8266, RP2040). - // - // On these platforms, MDNS.update() calls _process(true) which only manages timer-driven - // state machines (probe/announce timeouts and service query cache TTLs). Incoming mDNS - // packets are handled independently via the lwIP onRx UDP callback and are NOT affected - // by how often update() is called. - // - // The shortest internal timer is the 250ms probe interval (RFC 6762 Section 8.1). - // Announcement intervals are 1000ms and cache TTL checks are on the order of seconds - // to minutes. A 50ms polling interval provides sufficient resolution for all timers - // while completely removing mDNS from the per-iteration loop list. - // - // In steady state (after the ~8 second boot probe/announce phase completes), update() - // checks timers that are set to never expire, making every call pure overhead. - // - // Tasmota uses a 50ms main loop cycle with mDNS working correctly, confirming this - // interval is safe in production. - // - // By using set_interval() instead of overriding loop(), the component is excluded from - // the main loop list via has_overridden_loop(), eliminating all per-iteration overhead - // including virtual dispatch. +#ifdef USE_MDNS_EVENT_DRIVEN_POLLING + // LEAmDNS has meaningful work only during the probe+announce phase (3×250ms probes + + // 8×1000ms announces, ~9s). Afterwards every internal timer is resetToNeverExpires() + // and update() becomes pure overhead. We arm a bounded polling window from IP state + // listener events so update() runs only during that phase. static constexpr uint32_t MDNS_UPDATE_INTERVAL_MS = 50; + // Must exceed LEAmDNS's longest restart-to-announce-complete path: + // MDNS_PROBE_DELAY (250ms) × MDNS_PROBE_COUNT (3) = 750ms probing + // + MDNS_ANNOUNCE_DELAY (1000ms) × MDNS_ANNOUNCE_COUNT (8) = 8000ms announcing + // + rand() % MDNS_PROBE_DELAY jitter on first probe (0–250ms) + // + debounced schedule_function() hop when statusChangeCB fires on ESP8266 + // ≈ 9s nominal. 15s gives ~6s margin to absorb main-loop blocking (long + // component setup, WiFi scan, flash writes) that could stretch the deadlines + // between our polls. If LEAmDNS ever extends its phase (upstream library + // update) this constant needs to grow. Constants defined in LEAmDNS_Priv.h + // (ESP8266 core 3.1.2 / arduino-pico 5.5.1). + static constexpr uint32_t MDNS_POLL_WINDOW_MS = 15000; + static constexpr uint32_t MDNS_POLL_ID = 0; + static constexpr uint32_t MDNS_POLL_STOP_ID = 1; +#endif float get_setup_priority() const override { return setup_priority::AFTER_CONNECTION; } #ifdef USE_MDNS_EXTRA_SERVICES @@ -87,7 +110,17 @@ class MDNSComponent final : public Component { } #endif +#ifdef USE_MDNS_EVENT_DRIVEN_POLLING + void on_ip_state(const network::IPAddresses &ips, const network::IPAddress &dns1, + const network::IPAddress &dns2) override; +#endif + protected: +#ifdef USE_MDNS_EVENT_DRIVEN_POLLING + /// Arm a fresh MDNS_POLL_WINDOW_MS polling window. Idempotent — re-arming replaces + /// the previous window via the scheduler's atomic cancel-and-add on matching IDs. + void start_polling_window_(); +#endif /// Helper to set up services and MAC buffers, then call platform-specific registration using PlatformRegisterFn = void (*)(MDNSComponent *, StaticVector &); @@ -130,8 +163,8 @@ class MDNSComponent final : public Component { #ifdef USE_MDNS_STORE_SERVICES StaticVector services_{}; #endif -#ifdef USE_RP2040 - bool was_connected_{false}; +#if defined(USE_RP2040) && defined(USE_MDNS_EVENT_DRIVEN_POLLING) + // RP2040 defers MDNS.begin() until the first IP-up event; this tracks that. bool initialized_{false}; #endif void compile_records_(StaticVector &services, char *mac_address_buf); diff --git a/esphome/components/mdns/mdns_esp8266.cpp b/esphome/components/mdns/mdns_esp8266.cpp index 70c614f8d3..f6d5786675 100644 --- a/esphome/components/mdns/mdns_esp8266.cpp +++ b/esphome/components/mdns/mdns_esp8266.cpp @@ -8,6 +8,8 @@ #include "esphome/core/hal.h" #include "esphome/core/log.h" #include "mdns_component.h" +// wifi_component.h is pulled in transitively by mdns_component.h when +// USE_MDNS_WIFI_LISTENER is defined. namespace esphome::mdns { @@ -36,15 +38,36 @@ static void register_esp8266(MDNSComponent *, StaticVectorset_interval(MDNS_POLL_ID, MDNS_UPDATE_INTERVAL_MS, []() { MDNS.update(); }); + this->set_timeout(MDNS_POLL_STOP_ID, MDNS_POLL_WINDOW_MS, [this]() { this->cancel_interval(MDNS_POLL_ID); }); +} +#endif + void MDNSComponent::setup() { this->setup_buffers_and_register_(register_esp8266); - // Schedule MDNS.update() via set_interval() instead of overriding loop(). - // This removes the component from the per-iteration loop list entirely, - // eliminating virtual dispatch overhead on every main loop cycle. - // See MDNS_UPDATE_INTERVAL_MS comment in mdns_component.h for safety analysis. - this->set_interval(MDNS_UPDATE_INTERVAL_MS, []() { MDNS.update(); }); +#ifdef USE_MDNS_WIFI_LISTENER + // LEAmDNS's own LwipIntf::statusChangeCB drives _restart() on netif changes; we just + // arm the window around the initial probe/announce and each reconnect. Unconditional + // here is safe: setup_priority::AFTER_CONNECTION guarantees the network is up. + wifi::global_wifi_component->add_ip_state_listener(this); + this->start_polling_window_(); +#endif } +#ifdef USE_MDNS_WIFI_LISTENER +void MDNSComponent::on_ip_state(const network::IPAddresses &ips, const network::IPAddress &, + const network::IPAddress &) { + // IP listener only fires on acquisition (not loss), so any notification is a fresh + // IP worth re-arming for. start_polling_window_() is idempotent. + if (ips[0].is_set()) { + this->start_polling_window_(); + } +} +#endif + void MDNSComponent::on_shutdown() { MDNS.close(); delay(10); diff --git a/esphome/components/mdns/mdns_rp2040.cpp b/esphome/components/mdns/mdns_rp2040.cpp index 64b603030c..f5848893a3 100644 --- a/esphome/components/mdns/mdns_rp2040.cpp +++ b/esphome/components/mdns/mdns_rp2040.cpp @@ -6,9 +6,10 @@ #include "esphome/core/application.h" #include "esphome/core/log.h" #include "mdns_component.h" +// wifi_component.h / ethernet_component.h are pulled in transitively by +// mdns_component.h when their respective listener defines are active. // Arduino-Pico's PolledTimeout.h (pulled in by ESP8266mDNS.h) redefines IRAM_ATTR to empty. -// Save and restore our definition around the include to avoid a redefinition warning. #pragma push_macro("IRAM_ATTR") #undef IRAM_ATTR #include @@ -20,10 +21,7 @@ static void register_rp2040(MDNSComponent *, StaticVectorset_interval(MDNS_UPDATE_INTERVAL_MS, [this]() { - bool connected = network::is_connected(); - if (connected && !this->was_connected_) { - if (!this->initialized_) { - this->setup_buffers_and_register_(register_rp2040); - this->initialized_ = true; - } else { - MDNS.notifyAPChange(); - } - } - this->was_connected_ = connected; - if (this->initialized_) { - MDNS.update(); - } - }); +#ifdef USE_MDNS_EVENT_DRIVEN_POLLING +void MDNSComponent::start_polling_window_() { + // uint32_t-ID set_interval/set_timeout already does atomic cancel-and-add. + this->set_interval(MDNS_POLL_ID, MDNS_UPDATE_INTERVAL_MS, []() { MDNS.update(); }); + this->set_timeout(MDNS_POLL_STOP_ID, MDNS_POLL_WINDOW_MS, [this]() { this->cancel_interval(MDNS_POLL_ID); }); } +#endif + +void MDNSComponent::setup() { + // arduino-pico stubs out LwipIntf::stateUpCB (the netif status callback LEAmDNS uses + // on ESP8266 for auto-restart), so we must drive begin()/notifyAPChange() from our + // own IP state listener. Both WiFi and Ethernet have the same listener signature — + // one on_ip_state() override serves both. +#ifdef USE_MDNS_WIFI_LISTENER + wifi::global_wifi_component->add_ip_state_listener(this); + // AFTER_CONNECTION priority means the network may already be up; the listener only + // fires on subsequent changes, so seed the current state. + { + const auto ips = wifi::global_wifi_component->wifi_sta_ip_addresses(); + if (ips[0].is_set()) { + this->on_ip_state(ips, wifi::global_wifi_component->get_dns_address(0), + wifi::global_wifi_component->get_dns_address(1)); + } + } +#endif +#ifdef USE_MDNS_ETHERNET_LISTENER + ethernet::global_eth_component->add_ip_state_listener(this); + if (ethernet::global_eth_component->is_connected()) { + const auto ips = ethernet::global_eth_component->get_ip_addresses(); + if (ips[0].is_set()) { + this->on_ip_state(ips, network::IPAddress{}, network::IPAddress{}); + } + } +#endif +} + +#ifdef USE_MDNS_EVENT_DRIVEN_POLLING +void MDNSComponent::on_ip_state(const network::IPAddresses &ips, const network::IPAddress &, + const network::IPAddress &) { + // Listener only fires on IP acquisition (not loss); every event is a fresh IP. + if (!ips[0].is_set()) { + return; + } + if (!this->initialized_) { + this->setup_buffers_and_register_(register_rp2040); + this->initialized_ = true; + } else { + MDNS.notifyAPChange(); + } + this->start_polling_window_(); +} +#endif void MDNSComponent::on_shutdown() { MDNS.close(); diff --git a/tests/components/mdns/common-enabled-ethernet.yaml b/tests/components/mdns/common-enabled-ethernet.yaml new file mode 100644 index 0000000000..bfa9321d43 --- /dev/null +++ b/tests/components/mdns/common-enabled-ethernet.yaml @@ -0,0 +1,23 @@ +ethernet: + type: W5500 + clk_pin: 18 + mosi_pin: 19 + miso_pin: 16 + cs_pin: 17 + interrupt_pin: 21 + reset_pin: 20 + manual_ip: + static_ip: 192.168.178.56 + gateway: 192.168.178.1 + subnet: 255.255.255.0 + domain: .local + mac_address: "02:AA:BB:CC:DD:01" + +mdns: + disabled: false + services: + - service: _test_service + protocol: _tcp + port: 8888 + txt: + static_string: Anything diff --git a/tests/components/mdns/test-enabled-ethernet.rp2040-ard.yaml b/tests/components/mdns/test-enabled-ethernet.rp2040-ard.yaml new file mode 100644 index 0000000000..f84a0bc276 --- /dev/null +++ b/tests/components/mdns/test-enabled-ethernet.rp2040-ard.yaml @@ -0,0 +1 @@ +<<: !include common-enabled-ethernet.yaml From 9768380856ed1700155e44269d7394d9461c736c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:04:10 -0500 Subject: [PATCH 09/16] [api] Hoist memw out of socket ready check to once per main-loop iter (#15996) --- esphome/core/application.h | 13 +++++++++++++ esphome/core/lwip_fast_select.h | 30 ++++++++++++++---------------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/esphome/core/application.h b/esphome/core/application.h index 04e0f1138e..4a18714d0d 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -9,6 +9,10 @@ #include #include "esphome/core/component.h" #include "esphome/core/defines.h" + +#if defined(USE_LWIP_FAST_SELECT) && defined(ESPHOME_THREAD_MULTI_ATOMICS) +#include // for std::atomic_thread_fence in Application::loop() +#endif #include "esphome/core/hal.h" #include "esphome/core/helpers.h" #include "esphome/core/preferences.h" @@ -580,6 +584,15 @@ inline ESPHOME_ALWAYS_INLINE Application::ComponentPhaseGuard::ComponentPhaseGua } inline void ESPHOME_ALWAYS_INLINE Application::loop() { +#if defined(USE_LWIP_FAST_SELECT) && defined(ESPHOME_THREAD_MULTI_ATOMICS) + // Pairs with the TCP/IP thread's SYS_ARCH_UNPROTECT release on rcvevent so + // subsequent Socket::ready() checks in this iter observe the published state + // without a per-call memw. Wake is independent (xTaskNotifyGive/ + // ulTaskNotifyTake), so non-losing. Skipped on MULTI_NO_ATOMICS (e.g. + // BK72xx) — that path keeps `volatile` in esphome_lwip_socket_has_data() + // instead. + std::atomic_thread_fence(std::memory_order_acquire); +#endif #ifdef USE_RUNTIME_STATS // Capture the start of the active (non-sleeping) portion of this iteration. // Used to derive main-loop overhead = active time − Σ(component time) − diff --git a/esphome/core/lwip_fast_select.h b/esphome/core/lwip_fast_select.h index 3b5e449148..4ba2606d76 100644 --- a/esphome/core/lwip_fast_select.h +++ b/esphome/core/lwip_fast_select.h @@ -26,25 +26,23 @@ extern "C" { struct lwip_sock *esphome_lwip_get_sock(int fd); /// Check if a cached LwIP socket has data ready via unlocked hint read of rcvevent. -/// This avoids lwIP core lock contention between the main loop (CPU0) and -/// streaming/networking work (CPU1). Correctness is preserved because callers -/// already handle EWOULDBLOCK on nonblocking sockets — a stale hint simply causes -/// a harmless retry on the next loop iteration. In practice, stale reads have not -/// been observed across multi-day testing, but the design does not depend on that. -/// -/// The sock pointer must have been obtained from esphome_lwip_get_sock() and must -/// remain valid (caller owns socket lifetime — no concurrent close). -/// Hot path: inlined volatile 16-bit load — no function call overhead. -/// Uses offset-based access because lwip/priv/sockets_priv.h conflicts with C++. +/// On ESPHOME_THREAD_MULTI_ATOMICS builds, the caller must run on the main +/// loop task after Application::loop's per-iter std::atomic_thread_fence +/// (memory_order_acquire); that fence pairs with the TCP/IP thread's +/// SYS_ARCH_UNPROTECT release, so a plain load suffices and avoids the +/// per-call `memw` that volatile would emit on Xtensa under default +/// -mserialize-volatile. Without atomics (e.g. BK72xx), the fence is skipped +/// and the volatile load provides ordering on its own. +/// Stale reads are harmless either way: the hooked event_callback +/// xTaskNotifyGives on RCVPLUS, so the next iteration re-snapshots and +/// ulTaskNotifyTake never loses a wake. /// The offset and size are verified at compile time in lwip_fast_select.c. static inline bool esphome_lwip_socket_has_data(struct lwip_sock *sock) { - // Unlocked hint read — no lwIP core lock needed. - // volatile prevents the compiler from caching/reordering this cross-thread read. - // The write side (TCP/IP thread) commits via SYS_ARCH_UNPROTECT which releases a - // FreeRTOS mutex (ESP32) or resumes the scheduler (LibreTiny), ensuring the value - // is visible. Aligned 16-bit reads are single-instruction loads (L16SI/LH/LDRH) on - // Xtensa/RISC-V/ARM and cannot produce torn values. +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + return *(int16_t *) ((char *) sock + (int) ESPHOME_LWIP_SOCK_RCVEVENT_OFFSET) > 0; +#else return *(volatile int16_t *) ((char *) sock + (int) ESPHOME_LWIP_SOCK_RCVEVENT_OFFSET) > 0; +#endif } /// Hook a socket's netconn callback to notify the main loop task on receive events. From 1a57d9bc2fe16ad67342fefd5d7aa20e113858a0 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Tue, 28 Apr 2026 21:04:19 -0400 Subject: [PATCH 10/16] [sprinkler][pn532] Fix bugprone-unchecked-optional-access (#16102) --- esphome/components/pn532/pn532.cpp | 3 ++- esphome/components/sprinkler/sprinkler.cpp | 9 +++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/esphome/components/pn532/pn532.cpp b/esphome/components/pn532/pn532.cpp index 199a44dacc..3017b78414 100644 --- a/esphome/components/pn532/pn532.cpp +++ b/esphome/components/pn532/pn532.cpp @@ -317,6 +317,7 @@ enum PN532ReadReady PN532::read_ready_(bool block) { if (!this->rd_start_time_.has_value()) { this->rd_start_time_ = millis(); } + const uint32_t rd_start_time = *this->rd_start_time_; while (true) { if (this->is_read_ready()) { @@ -324,7 +325,7 @@ enum PN532ReadReady PN532::read_ready_(bool block) { break; } - if (millis() - *this->rd_start_time_ > 100) { + if (millis() - rd_start_time > 100) { ESP_LOGV(TAG, "Timed out waiting for readiness from PN532!"); this->rd_ready_ = TIMEOUT; break; diff --git a/esphome/components/sprinkler/sprinkler.cpp b/esphome/components/sprinkler/sprinkler.cpp index 0802cdec8e..e977c05c48 100644 --- a/esphome/components/sprinkler/sprinkler.cpp +++ b/esphome/components/sprinkler/sprinkler.cpp @@ -897,11 +897,12 @@ void Sprinkler::resume() { } if (this->paused_valve_.has_value() && (this->resume_duration_.has_value())) { + const size_t paused_valve = *this->paused_valve_; + const uint32_t resume_duration = *this->resume_duration_; // Resume only if valve has not been completed yet - if (!this->valve_cycle_complete_(this->paused_valve_.value())) { - ESP_LOGD(TAG, "Resuming valve %zu with %" PRIu32 " seconds remaining", this->paused_valve_.value_or(0), - this->resume_duration_.value_or(0)); - this->fsm_request_(this->paused_valve_.value(), this->resume_duration_.value()); + if (!this->valve_cycle_complete_(paused_valve)) { + ESP_LOGD(TAG, "Resuming valve %zu with %" PRIu32 " seconds remaining", paused_valve, resume_duration); + this->fsm_request_(paused_valve, resume_duration); } this->reset_resume(); } else { From 8af499b591fa3ed3a7693940ff6b59db0dbc3872 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:26:21 -0500 Subject: [PATCH 11/16] [api] Use custom deleter to fix incomplete-type error on macOS libc++ (#16050) --- esphome/components/api/api_server.cpp | 5 +++++ esphome/components/api/api_server.h | 10 ++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/esphome/components/api/api_server.cpp b/esphome/components/api/api_server.cpp index c30bd2e612..6c26c4e187 100644 --- a/esphome/components/api/api_server.cpp +++ b/esphome/components/api/api_server.cpp @@ -30,6 +30,11 @@ APIServer *global_api_server = nullptr; // NOLINT(cppcoreguidelines-avoid-non-c APIServer::APIServer() { global_api_server = this; } +// Custom deleter defined here so `delete` sees the complete APIConnection type. +// This prevents libc++ from emitting an "incomplete type" error when other +// translation units only have the forward declaration of APIConnection. +void APIServer::APIConnectionDeleter::operator()(APIConnection *p) const { delete p; } + void APIServer::socket_failed_(const LogString *msg) { ESP_LOGW(TAG, "Socket %s: errno %d", LOG_STR_ARG(msg), errno); this->destroy_socket_(); diff --git a/esphome/components/api/api_server.h b/esphome/components/api/api_server.h index e662d78eba..6b575e536d 100644 --- a/esphome/components/api/api_server.h +++ b/esphome/components/api/api_server.h @@ -193,7 +193,13 @@ class APIServer final : public Component, // Range-for view over the populated slice [0, api_connection_count_). Read-only with respect // to ownership — callers get `const unique_ptr&` so they can invoke non-const methods on the // APIConnection but cannot reset/move the slot and break the count invariant. - using APIConnectionPtr = std::unique_ptr; + // Custom deleter is defined out-of-line in api_server.cpp so libc++ does not + // eagerly instantiate `delete static_cast(p)` here, where + // only the forward declaration of APIConnection is visible (incomplete type). + struct APIConnectionDeleter { + void operator()(APIConnection *p) const; + }; + using APIConnectionPtr = std::unique_ptr; class ActiveClientsView { const APIConnectionPtr *begin_; const APIConnectionPtr *end_; @@ -292,7 +298,7 @@ class APIServer final : public Component, uint32_t last_connected_{0}; // Slots [0, api_connection_count_) are populated; trailing slots are always nullptr. - std::array, MAX_API_CONNECTIONS> clients_{}; + std::array clients_{}; // Vectors and strings (12 bytes each on 32-bit) // Shared proto write buffer for all connections. // Not pre-allocated: all send paths call prepare_first_message_buffer() which From 1363f661e6b3b1a8325e096b0e60172a863e104e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:26:25 -0500 Subject: [PATCH 12/16] [core] Inline ContinuationAction in If/While/RepeatAction (#16040) --- esphome/core/base_automation.h | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/esphome/core/base_automation.h b/esphome/core/base_automation.h index 17f937d10d..afd11c6867 100644 --- a/esphome/core/base_automation.h +++ b/esphome/core/base_automation.h @@ -273,18 +273,32 @@ template class WhileLoopContinuation : public Action { WhileAction *parent_; }; +// Wraps a ContinuationAction when Enabled, empty otherwise. +// Lets IfAction elide the else continuation when HasElse is false. +template struct OptionalContinuation { + ContinuationAction action; + explicit OptionalContinuation(Action *parent) : action(parent) {} +}; +template struct OptionalContinuation { + explicit OptionalContinuation(Action * /*parent*/) {} +}; + template class IfAction : public Action { public: explicit IfAction(Condition *condition) : condition_(condition) {} + // Precondition: add_then/add_else must be called at most once per instance. + // Codegen always batches the full action list into a single call. Calling + // twice would re-append the same inline continuation pointer and form a + // self-loop in the next_ chain. void add_then(const std::initializer_list *> &actions) { this->then_.add_actions(actions); - this->then_.add_action(new ContinuationAction(this)); + this->then_.add_action(&this->then_continuation_); } void add_else(const std::initializer_list *> &actions) requires(HasElse) { this->else_.add_actions(actions); - this->else_.add_action(new ContinuationAction(this)); + this->else_.add_action(&this->else_continuation_.action); } void play_complex(const Ts &...x) override { @@ -316,17 +330,20 @@ template class IfAction : public Action { protected: Condition *condition_; ActionList then_; + ContinuationAction then_continuation_{this}; struct NoElse {}; [[no_unique_address]] std::conditional_t, NoElse> else_; + [[no_unique_address]] OptionalContinuation else_continuation_{this}; }; template class WhileAction : public Action { public: WhileAction(Condition *condition) : condition_(condition) {} + // Precondition: must be called at most once per instance (see IfAction::add_then). void add_then(const std::initializer_list *> &actions) { this->then_.add_actions(actions); - this->then_.add_action(new WhileLoopContinuation(this)); + this->then_.add_action(&this->loop_continuation_); } friend class WhileLoopContinuation; @@ -354,6 +371,7 @@ template class WhileAction : public Action { protected: Condition *condition_; ActionList then_; + WhileLoopContinuation loop_continuation_{this}; }; // Implementation of WhileLoopContinuation::play @@ -386,9 +404,10 @@ template class RepeatAction : public Action { public: TEMPLATABLE_VALUE(uint32_t, count) + // Precondition: must be called at most once per instance (see IfAction::add_then). void add_then(const std::initializer_list *> &actions) { this->then_.add_actions(actions); - this->then_.add_action(new RepeatLoopContinuation(this)); + this->then_.add_action(&this->loop_continuation_); } friend class RepeatLoopContinuation; @@ -409,6 +428,7 @@ template class RepeatAction : public Action { protected: ActionList then_; + RepeatLoopContinuation loop_continuation_{this}; }; // Implementation of RepeatLoopContinuation::play From 35cb28edfe86a407c877ea133745862a1130062d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:27:22 -0500 Subject: [PATCH 13/16] [output] Gate FloatOutput power scaling fields behind USE_OUTPUT_FLOAT_POWER_SCALING (#15998) --- esphome/components/output/__init__.py | 10 ++++- esphome/components/output/automation.h | 3 ++ esphome/components/output/float_output.cpp | 8 +++- esphome/components/output/float_output.h | 51 ++++++++++++++++++++-- esphome/core/defines.h | 1 + 5 files changed, 67 insertions(+), 6 deletions(-) diff --git a/esphome/components/output/__init__.py b/esphome/components/output/__init__.py index 36798f2d7f..4f6c8943f5 100644 --- a/esphome/components/output/__init__.py +++ b/esphome/components/output/__init__.py @@ -54,10 +54,16 @@ async def setup_output_platform_(obj, config): power_supply_ = await cg.get_variable(config[CONF_POWER_SUPPLY]) cg.add(obj.set_power_supply(power_supply_)) if CONF_MAX_POWER in config: + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_max_power(config[CONF_MAX_POWER])) if CONF_MIN_POWER in config: + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_min_power(config[CONF_MIN_POWER])) - if CONF_ZERO_MEANS_ZERO in config: + # Only emit when zero_means_zero is actually enabled. The schema defaults to False + # so this key is always present; emitting unconditionally would force + # USE_OUTPUT_FLOAT_POWER_SCALING on for every output, defeating the gate. + if config.get(CONF_ZERO_MEANS_ZERO): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_zero_means_zero(config[CONF_ZERO_MEANS_ZERO])) @@ -121,6 +127,7 @@ async def output_set_level_to_code(config, action_id, template_arg, args): synchronous=True, ) async def output_set_min_power_to_code(config, action_id, template_arg, args): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") paren = await cg.get_variable(config[CONF_ID]) var = cg.new_Pvariable(action_id, template_arg, paren) template_ = await cg.templatable(config[CONF_MIN_POWER], args, cg.float_) @@ -140,6 +147,7 @@ async def output_set_min_power_to_code(config, action_id, template_arg, args): synchronous=True, ) async def output_set_max_power_to_code(config, action_id, template_arg, args): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") paren = await cg.get_variable(config[CONF_ID]) var = cg.new_Pvariable(action_id, template_arg, paren) template_ = await cg.templatable(config[CONF_MAX_POWER], args, cg.float_) diff --git a/esphome/components/output/automation.h b/esphome/components/output/automation.h index 3279378129..537226a143 100644 --- a/esphome/components/output/automation.h +++ b/esphome/components/output/automation.h @@ -2,6 +2,7 @@ #include "esphome/core/component.h" #include "esphome/core/automation.h" +#include "esphome/core/defines.h" #include "esphome/components/output/binary_output.h" #include "esphome/components/output/float_output.h" @@ -40,6 +41,7 @@ template class SetLevelAction : public Action { FloatOutput *output_; }; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING template class SetMinPowerAction : public Action { public: SetMinPowerAction(FloatOutput *output) : output_(output) {} @@ -63,6 +65,7 @@ template class SetMaxPowerAction : public Action { protected: FloatOutput *output_; }; +#endif // USE_OUTPUT_FLOAT_POWER_SCALING } // namespace output } // namespace esphome diff --git a/esphome/components/output/float_output.cpp b/esphome/components/output/float_output.cpp index 46014e0903..35629c828a 100644 --- a/esphome/components/output/float_output.cpp +++ b/esphome/components/output/float_output.cpp @@ -7,13 +7,15 @@ namespace output { static const char *const TAG = "output.float"; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING void FloatOutput::set_max_power(float max_power) { - this->max_power_ = clamp(max_power, this->min_power_, 1.0f); // Clamp to MIN>=MAX>=1.0 + this->max_power_ = clamp(max_power, this->min_power_, 1.0f); // Clamp to min_power <= max <= 1.0 } void FloatOutput::set_min_power(float min_power) { - this->min_power_ = clamp(min_power, 0.0f, this->max_power_); // Clamp to 0.0>=MIN>=MAX + this->min_power_ = clamp(min_power, 0.0f, this->max_power_); // Clamp to 0.0 <= min <= max_power } +#endif void FloatOutput::set_level(float state) { state = clamp(state, 0.0f, 1.0f); @@ -26,8 +28,10 @@ void FloatOutput::set_level(float state) { } #endif +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING if (state != 0.0f || !this->zero_means_zero_) // regardless of min_power_, 0.0 means off state = (state * (this->max_power_ - this->min_power_)) + this->min_power_; +#endif if (this->is_inverted()) state = 1.0f - state; diff --git a/esphome/components/output/float_output.h b/esphome/components/output/float_output.h index 5225f88c66..3e1bd83968 100644 --- a/esphome/components/output/float_output.h +++ b/esphome/components/output/float_output.h @@ -1,11 +1,13 @@ #pragma once #include "esphome/core/component.h" +#include "esphome/core/defines.h" #include "binary_output.h" namespace esphome { namespace output { +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING #define LOG_FLOAT_OUTPUT(this) \ LOG_BINARY_OUTPUT(this) \ if (this->max_power_ != 1.0f) { \ @@ -14,6 +16,9 @@ namespace output { if (this->min_power_ != 0.0f) { \ ESP_LOGCONFIG(TAG, " Min Power: %.1f%%", this->min_power_ * 100.0f); \ } +#else +#define LOG_FLOAT_OUTPUT(this) LOG_BINARY_OUTPUT(this) +#endif /** Base class for all output components that can output a variable level, like PWM. * @@ -22,14 +27,18 @@ namespace output { * makes using maths much easier and (in theory) supports all possible bit depths. * * If you want to create a FloatOutput yourself, you essentially just have to override write_state(float). - * That method will be called for you with inversion and max-min power and offset to min power already applied. + * That method will be called for you with inversion already applied. When USE_OUTPUT_FLOAT_POWER_SCALING is + * enabled (set automatically by Python codegen if any output uses min_power/max_power/zero_means_zero or the + * matching runtime actions), the value will additionally have max-min power scaling and offset to min_power + * applied; otherwise only inversion is applied. * * This interface is compatible with BinaryOutput (and will automatically convert the binary states to floating * point states for you). Additionally, this class provides a way for users to set a minimum and/or maximum power - * output + * output (gated on USE_OUTPUT_FLOAT_POWER_SCALING). */ class FloatOutput : public BinaryOutput { public: +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING /** Set the maximum power output of this component. * * All values are multiplied by max_power - min_power and offset to min_power to get the adjusted value. @@ -51,6 +60,32 @@ class FloatOutput : public BinaryOutput { * @param zero_means_zero True if a 0 state should mean 0 and not min_power. */ void set_zero_means_zero(bool zero_means_zero) { this->zero_means_zero_ = zero_means_zero; } +#else + // Compile-time guards for users calling these methods from lambdas (documented usage at + // https://esphome.io/components/output/#output-set_min_power_action). When power scaling + // is compiled out, these template stubs fail to compile with an actionable error pointing + // at the user's lambda. Templating on a default-false bool means static_assert only fires + // on instantiation (i.e. when the user actually calls the method), not on every parse. + template void set_max_power(float max_power) { + static_assert(_use_output_float_power_scaling, + "set_max_power() requires USE_OUTPUT_FLOAT_POWER_SCALING. " + "To enable it, add 'max_power: 100%' (or any value) to one output entry in your YAML — " + "the codegen will then keep the scaling fields. " + "See https://esphome.io/components/output/ for details."); + } + template void set_min_power(float min_power) { + static_assert(_use_output_float_power_scaling, + "set_min_power() requires USE_OUTPUT_FLOAT_POWER_SCALING. " + "To enable it, add 'min_power: 0%' (or any value) to one output entry in your YAML — " + "the codegen will then keep the scaling fields. " + "See https://esphome.io/components/output/ for details."); + } + template void set_zero_means_zero(bool zero_means_zero) { + static_assert(_use_output_float_power_scaling, + "set_zero_means_zero() requires USE_OUTPUT_FLOAT_POWER_SCALING. " + "To enable it, add 'zero_means_zero: true' to one output entry in your YAML."); + } +#endif /** Set the level of this float output, this is called from the front-end. * @@ -69,20 +104,30 @@ class FloatOutput : public BinaryOutput { // ========== INTERNAL METHODS ========== // (In most use cases you won't need these) +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING /// Get the maximum power output. float get_max_power() const { return this->max_power_; } /// Get the minimum power output. float get_min_power() const { return this->min_power_; } +#else + /// Get the maximum power output. + float get_max_power() const { return 1.0f; } + + /// Get the minimum power output. + float get_min_power() const { return 0.0f; } +#endif protected: /// Implement BinarySensor's write_enabled; this should never be called. void write_state(bool state) override; virtual void write_state(float state) = 0; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING float max_power_{1.0f}; float min_power_{0.0f}; - bool zero_means_zero_; + bool zero_means_zero_{false}; +#endif }; } // namespace output diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 592c8c46a2..99ec936c12 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -146,6 +146,7 @@ #define USE_NEXTION_WAVEFORM #define USE_NUMBER #define USE_OUTPUT +#define USE_OUTPUT_FLOAT_POWER_SCALING #define USE_POWER_SUPPLY #define USE_PREFERENCES_SYNC_EVERY_LOOP #define USE_QR_CODE From f05243bd9defaf5d1e665c8829af60f2512b4aa2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:48:35 -0500 Subject: [PATCH 14/16] [api] Add 48-bit MAC address varint fast path for BLE advertisements (#15988) --- esphome/components/api/api.proto | 2 +- esphome/components/api/api_options.proto | 6 + esphome/components/api/api_pb2.cpp | 4 +- esphome/components/api/api_pb2_service.cpp | 2 + esphome/components/api/proto.h | 34 +++++ script/api_protobuf/api_protobuf.py | 27 +++- script/build_helpers.py | 19 ++- .../components/api/test_proto_mac_varint.cpp | 123 ++++++++++++++++++ tests/components/json/__init__.py | 9 ++ 9 files changed, 220 insertions(+), 6 deletions(-) create mode 100644 tests/components/api/test_proto_mac_varint.cpp create mode 100644 tests/components/json/__init__.py diff --git a/esphome/components/api/api.proto b/esphome/components/api/api.proto index c0fd990eca..391efbd6eb 100644 --- a/esphome/components/api/api.proto +++ b/esphome/components/api/api.proto @@ -1639,7 +1639,7 @@ message BluetoothLEAdvertisementResponse { message BluetoothLERawAdvertisement { option (inline_encode) = true; - uint64 address = 1 [(force) = true]; + uint64 address = 1 [(force) = true, (mac_address) = true]; sint32 rssi = 2 [(force) = true]; uint32 address_type = 3 [(max_value) = 4]; diff --git a/esphome/components/api/api_options.proto b/esphome/components/api/api_options.proto index d5d0b37e8d..ac9c4e59cc 100644 --- a/esphome/components/api/api_options.proto +++ b/esphome/components/api/api_options.proto @@ -110,4 +110,10 @@ extend google.protobuf.FieldOptions { // length varint calculations and direct byte writes, since the length // varint is guaranteed to be 1 byte. optional uint32 max_data_length = 50018; + + // mac_address: Field is a 48-bit MAC address stored in a uint64. + // Emits encode_varint_raw_48bit which has a 7-byte fast path that avoids + // the per-byte loop when the upper bits are non-zero (the common case + // for real MAC addresses, since OUIs occupy the top 24 bits). + optional bool mac_address = 50019 [default=false]; } diff --git a/esphome/components/api/api_pb2.cpp b/esphome/components/api/api_pb2.cpp index f6ceee2296..eb25bf7461 100644 --- a/esphome/components/api/api_pb2.cpp +++ b/esphome/components/api/api_pb2.cpp @@ -2352,7 +2352,7 @@ BluetoothLERawAdvertisementsResponse::encode(ProtoWriteBuffer &buffer PROTO_ENCO uint8_t *len_pos = pos; ProtoEncode::reserve_byte(pos PROTO_ENCODE_DEBUG_ARG); ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 8); - ProtoEncode::encode_varint_raw_64(pos PROTO_ENCODE_DEBUG_ARG, sub_msg.address); + ProtoEncode::encode_varint_raw_48bit(pos PROTO_ENCODE_DEBUG_ARG, sub_msg.address); ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 16); ProtoEncode::encode_varint_raw_short(pos PROTO_ENCODE_DEBUG_ARG, encode_zigzag32(sub_msg.rssi)); if (sub_msg.address_type) { @@ -2373,7 +2373,7 @@ BluetoothLERawAdvertisementsResponse::calculate_size() const { for (uint16_t i = 0; i < this->advertisements_len; i++) { auto &sub_msg = this->advertisements[i]; size += 2; - size += ProtoSize::calc_uint64_force(1, sub_msg.address); + size += ProtoSize::calc_uint64_48bit_force(1, sub_msg.address); size += ProtoSize::calc_sint32_force(1, sub_msg.rssi); size += sub_msg.address_type ? 2 : 0; size += 2 + sub_msg.data_len; diff --git a/esphome/components/api/api_pb2_service.cpp b/esphome/components/api/api_pb2_service.cpp index 6ae2a3e369..0ba2961a13 100644 --- a/esphome/components/api/api_pb2_service.cpp +++ b/esphome/components/api/api_pb2_service.cpp @@ -21,6 +21,7 @@ void APIServerConnectionBase::log_receive_message_(const LogString *name) { } #endif +#ifdef USE_API void APIConnection::read_message_(uint32_t msg_size, uint32_t msg_type, const uint8_t *msg_data) { // Check authentication/connection requirements switch (msg_type) { @@ -706,5 +707,6 @@ void APIConnection::read_message_(uint32_t msg_size, uint32_t msg_type, const ui break; } } +#endif // USE_API } // namespace esphome::api diff --git a/esphome/components/api/proto.h b/esphome/components/api/proto.h index 8cac7fff3b..3ff65029e1 100644 --- a/esphome/components/api/proto.h +++ b/esphome/components/api/proto.h @@ -342,6 +342,32 @@ class ProtoEncode { } encode_varint_raw_loop(pos PROTO_ENCODE_DEBUG_ARG, value); } + /// Encode a 48-bit MAC address (stored in a uint64) as varint. + /// Real MAC addresses occupy the full 48 bits (OUI in upper 24), so the + /// fast path -- any non-zero bit in the top 6 of 48 -- emits exactly 7 bytes + /// with no per-byte branch. Falls back to the general loop otherwise. + /// Caller must guarantee value fits in 48 bits (checked in debug builds). + static inline void ESPHOME_ALWAYS_INLINE encode_varint_raw_48bit(uint8_t *__restrict__ &pos PROTO_ENCODE_DEBUG_PARAM, + uint64_t value) { +#ifdef ESPHOME_DEBUG_API + assert(value < (1ULL << (MAC_ADDRESS_SIZE * 8)) && "encode_varint_raw_48bit: value exceeds 48 bits"); +#endif + // 7-byte varint holds 49 bits (7 * 7), so a 48-bit value needs all 7 bytes + // whenever bit 42 or higher is set (i.e. value >= 1 << (48 - 6)). + if (value >= (1ULL << (MAC_ADDRESS_SIZE * 8 - 6))) [[likely]] { + PROTO_ENCODE_CHECK_BOUNDS(pos, 7); + pos[0] = static_cast(value | 0x80); + pos[1] = static_cast((value >> 7) | 0x80); + pos[2] = static_cast((value >> 14) | 0x80); + pos[3] = static_cast((value >> 21) | 0x80); + pos[4] = static_cast((value >> 28) | 0x80); + pos[5] = static_cast((value >> 35) | 0x80); + pos[6] = static_cast(value >> 42); + pos += 7; + return; + } + encode_varint_raw_64(pos PROTO_ENCODE_DEBUG_ARG, value); + } static inline void ESPHOME_ALWAYS_INLINE encode_field_raw(uint8_t *__restrict__ &pos PROTO_ENCODE_DEBUG_PARAM, uint32_t field_id, uint32_t type) { encode_varint_raw(pos PROTO_ENCODE_DEBUG_ARG, (field_id << 3) | type); @@ -817,6 +843,14 @@ class ProtoSize { static constexpr inline uint32_t ESPHOME_ALWAYS_INLINE calc_uint64_force(uint32_t field_id_size, uint64_t value) { return field_id_size + varint(value); } + /// 48-bit MAC address variant: matches encode_varint_raw_48bit's fast path. + /// When any of the top 6 of 48 bits is set the encoded varint is 7 bytes; + /// otherwise fall back to the general size calculation. + /// Caller must guarantee value fits in 48 bits (encoder asserts in debug). + static constexpr inline uint32_t ESPHOME_ALWAYS_INLINE calc_uint64_48bit_force(uint32_t field_id_size, + uint64_t value) { + return field_id_size + (value >= (1ULL << (MAC_ADDRESS_SIZE * 8 - 6)) ? 7 : varint(value)); + } static constexpr uint32_t calc_length(uint32_t field_id_size, size_t len) { return len ? field_id_size + varint(static_cast(len)) + static_cast(len) : 0; } diff --git a/script/api_protobuf/api_protobuf.py b/script/api_protobuf/api_protobuf.py index c10479a726..bf672d0567 100755 --- a/script/api_protobuf/api_protobuf.py +++ b/script/api_protobuf/api_protobuf.py @@ -184,6 +184,11 @@ class TypeInfo(ABC): """Check if this field should always be encoded (skip zero/empty check).""" return get_field_opt(self._field, pb.force, False) + @property + def mac_address(self) -> bool: + """Check if this uint64 field is a 48-bit MAC address (use 7-byte fast path).""" + return get_field_opt(self._field, pb.mac_address, False) + @property def max_value(self) -> int | None: """Get the max_value option for this field, or None if not set.""" @@ -665,8 +670,22 @@ class UInt64Type(VarintTypeMixin, TypeInfo): return o def get_size_calculation(self, name: str, force: bool = False) -> str: + if self.mac_address and force: + field_id_size = self.calculate_field_id_size() + return ( + f"size += ProtoSize::calc_uint64_48bit_force({field_id_size}, {name});" + ) return self._get_simple_size_calculation(name, force, "uint64") + @property + def RAW_ENCODE_MAP(self) -> dict[str, str]: # noqa: N802 + if self.mac_address: + return { + **TypeInfo.RAW_ENCODE_MAP, + "encode_uint64": "ProtoEncode::encode_varint_raw_48bit(pos, {value});", + } + return TypeInfo.RAW_ENCODE_MAP + def get_estimated_size(self) -> int: return self.calculate_field_id_size() + 3 # field ID + 3 bytes typical varint @@ -3558,8 +3577,13 @@ static const char *const TAG = "api.service"; # Generate read_message_ as APIConnection method (not base class) so the compiler # can devirtualize and inline the on_* handler calls within the same class. # APIConnection declares this method in api_connection.h. + # Guard with #ifdef USE_API since APIConnection itself is only defined when + # USE_API is set; without this, builds that compile this .cpp without + # USE_API (e.g. C++ unit tests for api dependencies) fail to find the + # class declaration. - out = "void APIConnection::read_message_(uint32_t msg_size, uint32_t msg_type, const uint8_t *msg_data) {\n" + out = "#ifdef USE_API\n" + out += "void APIConnection::read_message_(uint32_t msg_size, uint32_t msg_type, const uint8_t *msg_data) {\n" # Auth check block before dispatch switch out += " // Check authentication/connection requirements\n" @@ -3604,6 +3628,7 @@ static const char *const TAG = "api.service"; out += " break;\n" out += " }\n" out += "}\n" + out += "#endif // USE_API\n" cpp += out hpp += "};\n" diff --git a/script/build_helpers.py b/script/build_helpers.py index 1cfae51fca..4cf2f93fbb 100644 --- a/script/build_helpers.py +++ b/script/build_helpers.py @@ -324,8 +324,23 @@ def compile_and_get_binary( domain_list.append({CONF_PLATFORM: component}) # Skip "core" — it's a pseudo-component handled by the build # system, not a real loadable component (get_component returns None) - elif get_component(component_name) is not None: - config.setdefault(component_name, []) + elif (component := get_component(component_name)) is not None: + # MULTI_CONF components store their config as a list of dicts, + # everything else stores a single dict. Run the component's + # schema with {} so defaults get populated -- code paths like + # socket.FILTER_SOURCE_FILES expect a fully-populated mapping. + if component.multi_conf: + config.setdefault(component_name, []) + elif component_name not in config: + schema = component.config_schema + try: + config[component_name] = schema({}) if schema is not None else {} + except Exception: # noqa: BLE001 + # Schema requires explicit input we can't synthesize; fall + # back to an empty mapping so subscripting at least returns + # KeyError on missing keys rather than crashing on the + # wrong type. + config[component_name] = {} # Register platforms from the extra config (benchmark.yaml) so # USE_SENSOR, USE_LIGHT, etc. defines are emitted without needing diff --git a/tests/components/api/test_proto_mac_varint.cpp b/tests/components/api/test_proto_mac_varint.cpp new file mode 100644 index 0000000000..317a6fb9d6 --- /dev/null +++ b/tests/components/api/test_proto_mac_varint.cpp @@ -0,0 +1,123 @@ +#include + +#include +#include +#include + +#include "esphome/components/api/api_buffer.h" +#include "esphome/components/api/proto.h" + +namespace esphome::api::testing { + +// Generic varint decoder, used to verify the encoded bytes round-trip back to +// the original 48-bit MAC value, independent of the specialized encoder under +// test. +static uint64_t decode_varint(const uint8_t *buf, size_t len, size_t *consumed) { + uint64_t value = 0; + int shift = 0; + for (size_t i = 0; i < len; i++) { + value |= static_cast(buf[i] & 0x7F) << shift; + if ((buf[i] & 0x80) == 0) { + *consumed = i + 1; + return value; + } + shift += 7; + } + *consumed = 0; + return 0; +} + +// Reference encoder mirroring ProtoEncode::encode_varint_raw_64. +static size_t reference_encode(uint64_t value, uint8_t *out) { + uint8_t *p = out; + if (value < 128) { + *p++ = static_cast(value); + return p - out; + } + do { + *p++ = static_cast(value | 0x80); + value >>= 7; + } while (value > 0x7F); + *p++ = static_cast(value); + return p - out; +} + +// Encode `mac` via the 48-bit fast path and verify: +// - byte-identical output to the reference loop +// - encoded byte length matches `expected_bytes` +// - calc_uint64_48bit_force agrees on the size +// - the bytes round-trip through a generic varint decoder +static void verify_mac(uint64_t mac, size_t expected_bytes) { + ASSERT_LT(mac, 1ULL << 48) << "test fixture mac exceeds 48 bits"; + + uint8_t ref_buf[16] = {0}; + size_t ref_len = reference_encode(mac, ref_buf); + + APIBuffer api_buf; + api_buf.resize(16); + uint8_t *pos = api_buf.data(); +#ifdef ESPHOME_DEBUG_API + uint8_t *proto_debug_end_ = api_buf.data() + api_buf.size(); +#endif + ProtoEncode::encode_varint_raw_48bit(pos PROTO_ENCODE_DEBUG_ARG, mac); + size_t new_len = pos - api_buf.data(); + + EXPECT_EQ(new_len, expected_bytes) << "mac=0x" << std::hex << mac << std::dec; + EXPECT_EQ(ref_len, expected_bytes) << "reference disagrees on length for mac=0x" << std::hex << mac << std::dec; + + for (size_t i = 0; i < new_len; i++) { + EXPECT_EQ(api_buf.data()[i], ref_buf[i]) + << "byte " << i << " differs for mac=0x" << std::hex << mac << " (got 0x" << static_cast(api_buf.data()[i]) + << ", expected 0x" << static_cast(ref_buf[i]) << ")" << std::dec; + } + + size_t consumed = 0; + uint64_t decoded = decode_varint(api_buf.data(), new_len, &consumed); + EXPECT_EQ(consumed, new_len) << "decoder did not consume all bytes for mac=0x" << std::hex << mac << std::dec; + EXPECT_EQ(decoded, mac) << "round-trip mismatch for mac=0x" << std::hex << mac << std::dec; + + // Verify the size helper agrees. field_id_size = 1 (typical 1-byte tag). + uint32_t calc_size = ProtoSize::calc_uint64_48bit_force(1, mac); + EXPECT_EQ(calc_size, 1 + expected_bytes) + << "calc_uint64_48bit_force size mismatch for mac=0x" << std::hex << mac << std::dec; +} + +// Compute the canonical varint byte length for a value < 1<<48. +static size_t expected_varint_len(uint64_t v) { + if (v < (1ULL << 7)) + return 1; + if (v < (1ULL << 14)) + return 2; + if (v < (1ULL << 21)) + return 3; + if (v < (1ULL << 28)) + return 4; + if (v < (1ULL << 35)) + return 5; + if (v < (1ULL << 42)) + return 6; + return 7; +} + +// --- Specific MACs requested for verification --- + +TEST(ProtoMacVarint, AllZeros) { verify_mac(0x000000000000ULL, 1); } // 00:00:00:00:00:00 +TEST(ProtoMacVarint, FirstByteOnly) { verify_mac(0x110000000000ULL, 7); } // 11:00:00:00:00:00 +TEST(ProtoMacVarint, SecondByteOnly) { verify_mac(0x00AA00000000ULL, 6); } // 00:AA:00:00:00:00 +TEST(ProtoMacVarint, ThirdByteOnly) { verify_mac(0x0000BB000000ULL, 5); } // 00:00:BB:00:00:00 +TEST(ProtoMacVarint, FourthByteOnly) { verify_mac(0x000000CC0000ULL, 4); } // 00:00:00:CC:00:00 +TEST(ProtoMacVarint, FifthByteOnly) { verify_mac(0x00000000DD00ULL, 3); } // 00:00:00:00:DD:00 +TEST(ProtoMacVarint, SixthByteOnly) { verify_mac(0x0000000000EEULL, 2); } // 00:00:00:00:00:EE +TEST(ProtoMacVarint, AllOnes) { verify_mac(0xFFFFFFFFFFFFULL, 7); } // FF:FF:FF:FF:FF:FF + +// 100 deterministic-random 48-bit MACs to catch regressions across the space. +TEST(ProtoMacVarint, RandomSample) { + // NOLINTNEXTLINE(cert-msc32-c,cert-msc51-cpp) -- intentional fixed seed for reproducibility. + std::mt19937_64 rng(0xC0FFEE); + for (int i = 0; i < 100; i++) { + uint64_t mac = rng() & 0xFFFFFFFFFFFFULL; + verify_mac(mac, expected_varint_len(mac)); + } +} + +} // namespace esphome::api::testing diff --git a/tests/components/json/__init__.py b/tests/components/json/__init__.py new file mode 100644 index 0000000000..40ec1f996e --- /dev/null +++ b/tests/components/json/__init__.py @@ -0,0 +1,9 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # json's to_code calls cg.add_library("bblanchon/ArduinoJson", ...). C++ + # unit test builds that pull json in transitively (e.g. api) need that + # library registration to happen, otherwise json_util.cpp fails to find + # ArduinoJson.h. + manifest.enable_codegen() From d7b21a84a33a4cdae6db6f38dc4b0a06915c1373 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:49:51 -0500 Subject: [PATCH 15/16] [git] Make ref fetches and submodule updates shallow (#16014) --- esphome/git.py | 20 +++- tests/unit_tests/test_git.py | 190 +++++++++++++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 5 deletions(-) diff --git a/esphome/git.py b/esphome/git.py index 4d6e14001a..0106f24845 100644 --- a/esphome/git.py +++ b/esphome/git.py @@ -128,7 +128,10 @@ def clone_or_update( # We need to fetch the PR branch first, otherwise git will complain # about missing objects _LOGGER.info("Fetching %s", ref) - run_git_command(["git", "fetch", "--", "origin", ref], git_dir=repo_dir) + run_git_command( + ["git", "fetch", "--depth=1", "--", "origin", ref], + git_dir=repo_dir, + ) run_git_command( ["git", "reset", "--hard", "FETCH_HEAD"], git_dir=repo_dir ) @@ -138,7 +141,8 @@ def clone_or_update( "Initializing submodules (%s) for %s", ", ".join(submodules), key ) run_git_command( - ["git", "submodule", "update", "--init"] + submodules, + ["git", "submodule", "update", "--init", "--depth=1", "--"] + + submodules, git_dir=repo_dir, ) except GitException: @@ -179,8 +183,13 @@ def clone_or_update( git_dir=repo_dir, ) - # Fetch remote ref - cmd = ["git", "fetch", "--", "origin"] + # Fetch from the remote. --depth=1 keeps the clone shallow + # while still picking up new commits when the remote tip + # moves: a shallow fetch retrieves the current tip being + # fetched, whether that's an explicit ref or the remote's + # default branch, then reset --hard FETCH_HEAD updates the + # working tree to it. + cmd = ["git", "fetch", "--depth=1", "--", "origin"] if ref is not None: cmd.append(ref) run_git_command(cmd, git_dir=repo_dir) @@ -229,7 +238,8 @@ def clone_or_update( "Updating submodules (%s) for %s", ", ".join(submodules), key ) run_git_command( - ["git", "submodule", "update", "--init"] + submodules, + ["git", "submodule", "update", "--init", "--depth=1", "--"] + + submodules, git_dir=repo_dir, ) diff --git a/tests/unit_tests/test_git.py b/tests/unit_tests/test_git.py index dd7d26cb71..eab6bfc2cb 100644 --- a/tests/unit_tests/test_git.py +++ b/tests/unit_tests/test_git.py @@ -811,3 +811,193 @@ def test_clone_or_update_stale_clone_is_retried_after_cleanup( assert repo_dir.exists() assert call_count["clone"] == 2 assert call_count["fetch"] == 2 + + +def test_clone_with_ref_uses_shallow_fetch( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """Clone with a ref should use --depth=1 on both clone and fetch.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "pull/123/head" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "clone": + repo_dir.mkdir(parents=True, exist_ok=True) + (repo_dir / ".git").mkdir(exist_ok=True) + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + git.clone_or_update(url=url, ref=ref, refresh=None, domain=domain) + + call_list = mock_run_git_command.call_args_list + + clone_calls = [c for c in call_list if "clone" in c[0][0]] + assert len(clone_calls) == 1 + assert "--depth=1" in clone_calls[0][0][0] + + fetch_calls = [c for c in call_list if "fetch" in c[0][0]] + assert len(fetch_calls) == 1 + assert "--depth=1" in fetch_calls[0][0][0] + # Ref must still be passed so the requested commit/branch is fetched. + assert ref in fetch_calls[0][0][0] + + +def test_clone_with_submodules_uses_shallow_submodule_update( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """Submodule init on a fresh clone should use --depth=1.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "clone": + repo_dir.mkdir(parents=True, exist_ok=True) + (repo_dir / ".git").mkdir(exist_ok=True) + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + git.clone_or_update( + url=url, + ref=None, + refresh=None, + domain=domain, + submodules=["components/foo"], + ) + + submodule_calls = [ + c for c in mock_run_git_command.call_args_list if "submodule" in c[0][0] + ] + assert len(submodule_calls) == 1 + cmd = submodule_calls[0][0][0] + assert "--depth=1" in cmd + assert "components/foo" in cmd + # The `--` terminator must precede the submodule paths so a path + # beginning with `-` cannot be parsed as an option. + assert cmd.index("--") < cmd.index("components/foo") + + +def test_refresh_fetch_is_shallow(tmp_path: Path, mock_run_git_command: Mock) -> None: + """The refresh-path fetch should use --depth=1.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "main" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + _setup_old_repo(repo_dir) + mock_run_git_command.return_value = "abc123" + + git.clone_or_update( + url=url, ref=ref, refresh=TimePeriodSeconds(days=1), domain=domain + ) + + fetch_calls = [c for c in mock_run_git_command.call_args_list if "fetch" in c[0][0]] + assert len(fetch_calls) == 1 + cmd = fetch_calls[0][0][0] + assert "--depth=1" in cmd + # Ref must still be in the refresh fetch so the right tip is updated. + assert cmd[-1] == ref + + +def test_refresh_submodule_update_is_shallow( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """The refresh-path submodule update should use --depth=1.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + _setup_old_repo(repo_dir) + mock_run_git_command.return_value = "abc123" + + git.clone_or_update( + url=url, + ref=None, + refresh=TimePeriodSeconds(days=1), + domain=domain, + submodules=["components/foo"], + ) + + submodule_calls = [ + c for c in mock_run_git_command.call_args_list if "submodule" in c[0][0] + ] + assert len(submodule_calls) == 1 + cmd = submodule_calls[0][0][0] + assert "--depth=1" in cmd + assert "components/foo" in cmd + assert cmd.index("--") < cmd.index("components/foo") + + +def test_refresh_picks_up_new_remote_commits( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """Shallow fetch must still pull new commits when the remote tip moves. + + Simulates a stale local repo at SHA "old" while the remote has advanced + to SHA "new". The refresh path must run fetch (with --depth=1) followed + by reset --hard FETCH_HEAD so the working tree advances to the new tip. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "main" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + _setup_old_repo(repo_dir) + + # rev-parse is called once before fetch to record the pre-update SHA. + rev_parse_calls = {"count": 0} + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + cmd_type = _get_git_command_type(cmd) + if cmd_type == "rev-parse": + rev_parse_calls["count"] += 1 + return "old_sha" + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + _, revert = git.clone_or_update( + url=url, ref=ref, refresh=TimePeriodSeconds(days=1), domain=domain + ) + + # Verify the refresh sequence: rev-parse -> stash -> fetch (depth=1) -> reset + call_list = mock_run_git_command.call_args_list + cmd_sequence = [_get_git_command_type(c[0][0]) for c in call_list] + assert cmd_sequence == ["rev-parse", "stash", "fetch", "reset"] + + fetch_cmd = call_list[2][0][0] + assert "--depth=1" in fetch_cmd + assert fetch_cmd[-1] == ref + + reset_cmd = call_list[3][0][0] + assert reset_cmd[-1] == "FETCH_HEAD" + + # revert callback should reset back to the recorded pre-update SHA. + assert revert is not None + revert() + assert mock_run_git_command.call_args_list[-1][0][0] == [ + "git", + "reset", + "--hard", + "old_sha", + ] From eec770d622571ed9f625f3a0e6b0457be0542d55 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 28 Apr 2026 20:52:09 -0500 Subject: [PATCH 16/16] [core] Use ETag in external_files cache to fix re-downloads from raw.githubusercontent.com (#16020) --- esphome/external_files.py | 76 +++++- tests/unit_tests/test_external_files.py | 345 +++++++++++++++++++++--- 2 files changed, 379 insertions(+), 42 deletions(-) diff --git a/esphome/external_files.py b/esphome/external_files.py index b6f6149ebb..bd29dc93b1 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -1,14 +1,17 @@ from __future__ import annotations +import contextlib from datetime import UTC, datetime import logging +import os from pathlib import Path import requests import esphome.config_validation as cv from esphome.const import __version__ -from esphome.core import CORE, TimePeriodSeconds +from esphome.core import CORE, EsphomeError, TimePeriodSeconds +from esphome.helpers import write_file _LOGGER = logging.getLogger(__name__) CODEOWNERS = ["@landonr"] @@ -16,12 +19,72 @@ CODEOWNERS = ["@landonr"] NETWORK_TIMEOUT = 30 IF_MODIFIED_SINCE = "If-Modified-Since" +IF_NONE_MATCH = "If-None-Match" +ETAG = "ETag" CACHE_CONTROL = "Cache-Control" CACHE_CONTROL_MAX_AGE = "max-age=" CONTENT_DISPOSITION = "content-disposition" TEMP_DIR = "temp" +def _etag_sidecar_path(local_file_path: Path) -> Path: + return local_file_path.parent / f".{local_file_path.name}.etag" + + +def _mtime_seconds(path: Path) -> int: + """Return `path`'s mtime as integer seconds. + + Whole seconds is the common-denominator resolution across all + filesystems we run on (FAT/exFAT 2s, NTFS 100ns, APFS/ext4 ns), so + comparisons survive setting+reading round-trips that would lose + sub-second precision on lower-resolution filesystems. + """ + return int(path.stat().st_mtime) + + +def _read_etag(local_file_path: Path) -> str | None: + """Return the cached ETag if its sidecar's mtime still matches the cache + file's. A mismatch means the cache file was modified out-of-band, so the + ETag no longer describes its contents -- delete the stale sidecar and + return None. + """ + etag_path = _etag_sidecar_path(local_file_path) + try: + if _mtime_seconds(etag_path) != _mtime_seconds(local_file_path): + _LOGGER.debug( + "ETag sidecar mtime mismatch at %s; treating as stale", + local_file_path, + ) + etag_path.unlink() + return None + return etag_path.read_text().strip() or None + except OSError: + return None + + +def _write_etag(local_file_path: Path, etag: str | None) -> None: + etag_path = _etag_sidecar_path(local_file_path) + if not etag: + # ETag persistence is best-effort; matches `_read_etag`'s tolerance. + with contextlib.suppress(OSError): + etag_path.unlink() + return + try: + write_file(etag_path, etag) + except EsphomeError as e: + _LOGGER.debug("Could not save ETag for %s: %s", local_file_path, e) + return + # Pin the sidecar's mtime to the cache file's mtime. _read_etag relies on + # this match to detect out-of-band edits to the cache file. + try: + file_mtime = _mtime_seconds(local_file_path) + os.utime(etag_path, (file_mtime, file_mtime)) + except OSError as e: + _LOGGER.debug( + "Could not sync ETag sidecar mtime for %s: %s", local_file_path, e + ) + + def has_remote_file_changed(url: str, local_file_path: Path) -> bool: if local_file_path.exists(): _LOGGER.debug("has_remote_file_changed: File exists at %s", local_file_path) @@ -35,14 +98,17 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool: IF_MODIFIED_SINCE: local_modification_time_str, CACHE_CONTROL: CACHE_CONTROL_MAX_AGE + "3600", } + if etag := _read_etag(local_file_path): + headers[IF_NONE_MATCH] = etag response = requests.head( url, headers=headers, timeout=NETWORK_TIMEOUT, allow_redirects=True ) _LOGGER.debug( - "has_remote_file_changed: File %s, Local modified %s, response code %d", + "has_remote_file_changed: File %s, Local modified %s, ETag %s, response code %d", local_file_path, local_modification_time_str, + etag or "", response.status_code, ) @@ -51,6 +117,8 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool: "has_remote_file_changed: File not modified since %s", local_modification_time_str, ) + if (new_etag := response.headers.get(ETAG)) and new_etag != etag: + _write_etag(local_file_path, new_etag) return False _LOGGER.debug("has_remote_file_changed: File modified") return True @@ -112,7 +180,7 @@ def download_content(url: str, path: Path, timeout: int = NETWORK_TIMEOUT) -> by return path.read_bytes() raise cv.Invalid(f"Could not download from {url}: {e}") from e - path.parent.mkdir(parents=True, exist_ok=True) data = req.content - path.write_bytes(data) + write_file(path, data) + _write_etag(path, req.headers.get(ETAG)) return data diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index 4b0826db04..f4d268abe0 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -1,5 +1,6 @@ """Tests for external_files.py functions.""" +import os from pathlib import Path import time from unittest.mock import MagicMock, patch @@ -9,7 +10,54 @@ import requests from esphome import external_files from esphome.config_validation import Invalid -from esphome.core import CORE, TimePeriod +from esphome.core import CORE, EsphomeError, TimePeriod + + +def _seed_etag(cache_file: Path, etag: str) -> Path: + """Write an ETag sidecar with its mtime synced to the cache file's mtime, + matching the invariant that `_write_etag` enforces in production. + """ + sidecar = external_files._etag_sidecar_path(cache_file) + sidecar.write_text(etag) + file_mtime = int(cache_file.stat().st_mtime) + os.utime(sidecar, (file_mtime, file_mtime)) + return sidecar + + +@pytest.fixture +def mock_requests_head() -> MagicMock: + """Patch `external_files.requests.head` so the conditional HEAD-request + validator can be tested without doing real HTTP. + """ + with patch("esphome.external_files.requests.head") as m: + yield m + + +@pytest.fixture +def mock_requests_get() -> MagicMock: + """Patch `external_files.requests.get` so the download path can be + tested without doing real HTTP. + """ + with patch("esphome.external_files.requests.get") as m: + yield m + + +@pytest.fixture +def mock_has_remote_file_changed() -> MagicMock: + """Patch `external_files.has_remote_file_changed` so download tests can + control the conditional check independently from the GET path. + """ + with patch("esphome.external_files.has_remote_file_changed") as m: + yield m + + +@pytest.fixture +def mock_write_file() -> MagicMock: + """Patch `external_files.write_file` so atomic-write failures can be + injected without involving the real filesystem helper. + """ + with patch("esphome.external_files.write_file") as m: + yield m def test_compute_local_file_dir(setup_core: Path) -> None: @@ -88,9 +136,8 @@ def test_is_file_recent_with_zero_refresh(setup_core: Path) -> None: assert result is False -@patch("esphome.external_files.requests.head") def test_has_remote_file_changed_not_modified( - mock_head: MagicMock, setup_core: Path + mock_requests_head: MagicMock, setup_core: Path ) -> None: """Test has_remote_file_changed returns False when file not modified.""" test_file = setup_core / "cached.txt" @@ -98,23 +145,23 @@ def test_has_remote_file_changed_not_modified( mock_response = MagicMock() mock_response.status_code = 304 - mock_head.return_value = mock_response + mock_response.headers = {} + mock_requests_head.return_value = mock_response url = "https://example.com/file.txt" result = external_files.has_remote_file_changed(url, test_file) assert result is False - mock_head.assert_called_once() + mock_requests_head.assert_called_once() - call_args = mock_head.call_args + call_args = mock_requests_head.call_args headers = call_args[1]["headers"] assert external_files.IF_MODIFIED_SINCE in headers assert external_files.CACHE_CONTROL in headers -@patch("esphome.external_files.requests.head") def test_has_remote_file_changed_modified( - mock_head: MagicMock, setup_core: Path + mock_requests_head: MagicMock, setup_core: Path ) -> None: """Test has_remote_file_changed returns True when file modified.""" test_file = setup_core / "cached.txt" @@ -122,7 +169,8 @@ def test_has_remote_file_changed_modified( mock_response = MagicMock() mock_response.status_code = 200 - mock_head.return_value = mock_response + mock_response.headers = {} + mock_requests_head.return_value = mock_response url = "https://example.com/file.txt" result = external_files.has_remote_file_changed(url, test_file) @@ -140,15 +188,16 @@ def test_has_remote_file_changed_no_local_file(setup_core: Path) -> None: assert result is True -@patch("esphome.external_files.requests.head") def test_has_remote_file_changed_network_error( - mock_head: MagicMock, setup_core: Path + mock_requests_head: MagicMock, setup_core: Path ) -> None: """Test has_remote_file_changed returns False on network error when file is cached.""" test_file = setup_core / "cached.txt" test_file.write_text("cached content") - mock_head.side_effect = requests.exceptions.RequestException("Network error") + mock_requests_head.side_effect = requests.exceptions.RequestException( + "Network error" + ) url = "https://example.com/file.txt" result = external_files.has_remote_file_changed(url, test_file) @@ -156,9 +205,8 @@ def test_has_remote_file_changed_network_error( assert result is False -@patch("esphome.external_files.requests.head") def test_has_remote_file_changed_timeout( - mock_head: MagicMock, setup_core: Path + mock_requests_head: MagicMock, setup_core: Path ) -> None: """Test has_remote_file_changed respects timeout.""" test_file = setup_core / "cached.txt" @@ -166,15 +214,176 @@ def test_has_remote_file_changed_timeout( mock_response = MagicMock() mock_response.status_code = 304 - mock_head.return_value = mock_response + mock_response.headers = {} + mock_requests_head.return_value = mock_response url = "https://example.com/file.txt" external_files.has_remote_file_changed(url, test_file) - call_args = mock_head.call_args + call_args = mock_requests_head.call_args assert call_args[1]["timeout"] == external_files.NETWORK_TIMEOUT +def test_has_remote_file_changed_uses_etag( + mock_requests_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed sends If-None-Match when ETag is cached.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + _seed_etag(test_file, '"abc123"') + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {} + mock_requests_head.return_value = mock_response + + url = "https://example.com/file.txt" + result = external_files.has_remote_file_changed(url, test_file) + + assert result is False + headers = mock_requests_head.call_args[1]["headers"] + assert headers[external_files.IF_NONE_MATCH] == '"abc123"' + + +def test_has_remote_file_changed_no_etag_no_if_none_match( + mock_requests_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed omits If-None-Match when no ETag is cached.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {} + mock_requests_head.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.has_remote_file_changed(url, test_file) + + headers = mock_requests_head.call_args[1]["headers"] + assert external_files.IF_NONE_MATCH not in headers + + +def test_has_remote_file_changed_refreshes_etag_on_304( + mock_requests_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed updates the cached ETag when the 304 sends a new one.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + _seed_etag(test_file, '"old"') + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {external_files.ETAG: '"new"'} + mock_requests_head.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.has_remote_file_changed(url, test_file) + + assert external_files._etag_sidecar_path(test_file).read_text() == '"new"' + + +def test_has_remote_file_changed_ignores_etag_when_mtime_diverges( + mock_requests_head: MagicMock, setup_core: Path +) -> None: + """If the cache file was edited out-of-band (mtime no longer matches the + sidecar's), the cached ETag must not be used -- it no longer describes the + bytes on disk. + """ + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + sidecar = _seed_etag(test_file, '"abc123"') + + # Simulate an out-of-band edit to the cache file -- mtime advances by a + # full second (so it diverges at whole-second resolution) but the sidecar + # is left untouched, so the recorded ETag is now stale. + file_stat = test_file.stat() + os.utime(test_file, (file_stat.st_atime, file_stat.st_mtime + 1)) + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {} + mock_requests_head.return_value = mock_response + + external_files.has_remote_file_changed("https://example.com/file.txt", test_file) + + headers = mock_requests_head.call_args[1]["headers"] + assert external_files.IF_NONE_MATCH not in headers + # Stale sidecar should be removed so future calls don't keep paying the + # mtime-comparison cost on a known-bad sidecar. + assert not sidecar.exists() + + +def test_download_content_pins_etag_mtime_to_file_mtime( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + setup_core: Path, +) -> None: + """After a successful download, the sidecar's mtime must equal the cache + file's mtime so `_read_etag` accepts it on the next call. + """ + test_file = setup_core / "fresh.txt" + mock_has_remote_file_changed.return_value = True + mock_response = MagicMock() + mock_response.content = b"fresh content" + mock_response.headers = {external_files.ETAG: '"deadbeef"'} + mock_response.raise_for_status = MagicMock() + mock_requests_get.return_value = mock_response + + external_files.download_content("https://example.com/file.txt", test_file) + + sidecar = external_files._etag_sidecar_path(test_file) + assert int(sidecar.stat().st_mtime) == int(test_file.stat().st_mtime) + + +def test_write_etag_swallows_write_file_failure( + mock_write_file: MagicMock, setup_core: Path, caplog: pytest.LogCaptureFixture +) -> None: + """If `write_file` raises, _write_etag must not propagate -- ETag + persistence is best-effort and a failure here must not abort the + surrounding download. + """ + cache_file = setup_core / "cached.txt" + cache_file.write_text("cached content") + mock_write_file.side_effect = EsphomeError("disk full") + + with caplog.at_level("DEBUG", logger="esphome.external_files"): + external_files._write_etag(cache_file, '"abc123"') + + assert "Could not save ETag" in caplog.text + # Sidecar wasn't created, since write_file was mocked to fail before + # reaching the os.utime step. + assert not external_files._etag_sidecar_path(cache_file).exists() + + +def test_write_etag_swallows_utime_failure( + setup_core: Path, caplog: pytest.LogCaptureFixture +) -> None: + """If `os.utime` raises while pinning the sidecar's mtime, _write_etag + must not propagate. The sidecar is still written; if its mtime later + fails to match the cache file, `_read_etag` will discard it on next + read. + """ + cache_file = setup_core / "cached.txt" + cache_file.write_text("cached content") + + with ( + patch( + "esphome.external_files.os.utime", + side_effect=PermissionError("nope"), + ), + caplog.at_level("DEBUG", logger="esphome.external_files"), + ): + external_files._write_etag(cache_file, '"abc123"') + + assert "Could not sync ETag sidecar mtime" in caplog.text + # write_file succeeded, so the sidecar exists with the new value even + # though we couldn't pin its mtime. + sidecar = external_files._etag_sidecar_path(cache_file) + assert sidecar.exists() + assert sidecar.read_text() == '"abc123"' + + def test_compute_local_file_dir_creates_parent_dirs(setup_core: Path) -> None: """Test compute_local_file_dir creates parent directories.""" domain = "level1/level2/level3/level4" @@ -200,10 +409,10 @@ def test_is_file_recent_handles_float_seconds(setup_core: Path) -> None: assert result is True -@patch("esphome.external_files.requests.get") -@patch("esphome.external_files.has_remote_file_changed") def test_download_content_with_network_error_uses_cache( - mock_has_changed: MagicMock, mock_get: MagicMock, setup_core: Path + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + setup_core: Path, ) -> None: """Test download_content uses cached file when network fails.""" test_file = setup_core / "cached.txt" @@ -211,8 +420,10 @@ def test_download_content_with_network_error_uses_cache( test_file.write_bytes(cached_content) # Simulate file has changed, so it tries to download - mock_has_changed.return_value = True - mock_get.side_effect = requests.exceptions.RequestException("Network error") + mock_has_remote_file_changed.return_value = True + mock_requests_get.side_effect = requests.exceptions.RequestException( + "Network error" + ) url = "https://example.com/file.txt" result = external_files.download_content(url, test_file) @@ -220,17 +431,19 @@ def test_download_content_with_network_error_uses_cache( assert result == cached_content -@patch("esphome.external_files.requests.get") -@patch("esphome.external_files.has_remote_file_changed") def test_download_content_with_network_error_no_cache_fails( - mock_has_changed: MagicMock, mock_get: MagicMock, setup_core: Path + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + setup_core: Path, ) -> None: """Test download_content raises error when network fails and no cache exists.""" test_file = setup_core / "nonexistent.txt" # Simulate file has changed (doesn't exist), so it tries to download - mock_has_changed.return_value = True - mock_get.side_effect = requests.exceptions.RequestException("Network error") + mock_has_remote_file_changed.return_value = True + mock_requests_get.side_effect = requests.exceptions.RequestException( + "Network error" + ) url = "https://example.com/file.txt" @@ -238,11 +451,9 @@ def test_download_content_with_network_error_no_cache_fails( external_files.download_content(url, test_file) -@patch("esphome.external_files.requests.get") -@patch("esphome.external_files.has_remote_file_changed") def test_download_content_skip_external_update_uses_cache( - mock_has_changed: MagicMock, - mock_get: MagicMock, + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, setup_core: Path, ) -> None: """Test download_content skips network checks when CORE.skip_external_update is set.""" @@ -255,26 +466,25 @@ def test_download_content_skip_external_update_uses_cache( result = external_files.download_content(url, test_file) assert result == cached_content - mock_has_changed.assert_not_called() - mock_get.assert_not_called() + mock_has_remote_file_changed.assert_not_called() + mock_requests_get.assert_not_called() -@patch("esphome.external_files.requests.get") -@patch("esphome.external_files.has_remote_file_changed") def test_download_content_skip_external_update_downloads_when_missing( - mock_has_changed: MagicMock, - mock_get: MagicMock, + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, setup_core: Path, ) -> None: """Test download_content still downloads when file is missing, even with skip_external_update.""" test_file = setup_core / "missing.txt" new_content = b"fresh content" - mock_has_changed.return_value = True + mock_has_remote_file_changed.return_value = True mock_response = MagicMock() mock_response.content = new_content + mock_response.headers = {} mock_response.raise_for_status = MagicMock() - mock_get.return_value = mock_response + mock_requests_get.return_value = mock_response CORE.skip_external_update = True url = "https://example.com/file.txt" @@ -282,3 +492,62 @@ def test_download_content_skip_external_update_downloads_when_missing( assert result == new_content assert test_file.read_bytes() == new_content + + +def test_download_content_saves_etag( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + setup_core: Path, +) -> None: + """Test download_content writes the ETag sidecar after a successful download.""" + test_file = setup_core / "fresh.txt" + new_content = b"fresh content" + + mock_has_remote_file_changed.return_value = True + mock_response = MagicMock() + mock_response.content = new_content + mock_response.headers = {external_files.ETAG: '"deadbeef"'} + mock_response.raise_for_status = MagicMock() + mock_requests_get.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.download_content(url, test_file) + + assert external_files._etag_sidecar_path(test_file).read_text() == '"deadbeef"' + + +def test_download_content_atomic_write_no_partial_on_failure( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_write_file: MagicMock, + setup_core: Path, +) -> None: + """If `write_file` (the atomic-write helper) fails, the existing cache + file must remain untouched and no temp files may be left behind. Patching + `write_file` directly exercises the atomic-rename path -- a failure inside + `write_file` is the only reason the rename wouldn't have happened. + """ + from esphome.core import EsphomeError + + test_file = setup_core / "cached.txt" + original_content = b"original content" + test_file.write_bytes(original_content) + + mock_has_remote_file_changed.return_value = True + mock_response = MagicMock() + mock_response.content = b"new content" + mock_response.headers = {} + mock_response.raise_for_status = MagicMock() + mock_requests_get.return_value = mock_response + + mock_write_file.side_effect = EsphomeError("disk full") + + with pytest.raises(EsphomeError, match="disk full"): + external_files.download_content("https://example.com/file.txt", test_file) + + # Original file is untouched -- write_file aborted before its rename step. + assert test_file.read_bytes() == original_content + # write_file is responsible for cleaning its own temp files; nothing leaks + # into the cache directory either way. + leftover_tmps = list(setup_core.glob("tmp*")) + assert leftover_tmps == []