From 964fc1ef3fa780f5f7830df25f57a9b441e81469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20H=C3=A4ll?= Date: Thu, 20 Aug 2026 06:31:27 +0200 Subject: [PATCH 01/21] [wifi] Take the lwIP core lock around sntp_servermode_dhcp() (#18511) --- esphome/components/wifi/wifi_component_esp_idf.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/esphome/components/wifi/wifi_component_esp_idf.cpp b/esphome/components/wifi/wifi_component_esp_idf.cpp index 245390b097..24cb060edb 100644 --- a/esphome/components/wifi/wifi_component_esp_idf.cpp +++ b/esphome/components/wifi/wifi_component_esp_idf.cpp @@ -580,7 +580,14 @@ bool WiFiComponent::wifi_sta_ip_config_(const optional &manual_ip) { // lwIP starts the SNTP client if it gets an SNTP server from DHCP. We don't need the time, and more importantly, // the built-in SNTP client has a memory leak in certain situations. Disable this feature. // https://github.com/esphome/issues/issues/2299 - sntp_servermode_dhcp(false); + { +#if SNTP_GET_SERVERS_FROM_DHCP || SNTP_GET_SERVERS_FROM_DHCPV6 + // sntp_servermode_dhcp() is an empty macro unless lwIP is built with + // DHCP-supplied NTP servers, so only that build needs the core lock. + LwIPLock lock; +#endif + sntp_servermode_dhcp(false); + } // No manual IP is set; use DHCP client if (dhcp_status != ESP_NETIF_DHCP_STARTED) { From c90a5f4cffcdc5f1c6af137ab23802163470919c Mon Sep 17 00:00:00 2001 From: "esphome[bot]" <115708604+esphome[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 00:00:44 -0500 Subject: [PATCH 02/21] Bump bundled esphome-device-builder to 1.12.1 (#18541) --- docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 18f705b501..55aa0ac982 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -22,7 +22,7 @@ RUN \ -r /requirements.txt # Install the ESPHome Device Builder dashboard. -RUN uv pip install --no-cache-dir esphome-device-builder==1.12.0 +RUN uv pip install --no-cache-dir esphome-device-builder==1.12.1 RUN \ platformio settings set enable_telemetry No \ From 4c856949c283f0de0acbb0145e282c8e3df723f6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 06:59:57 -0500 Subject: [PATCH 03/21] [nrf52] Rebuild the Python env when its interpreter symlink dangles (#18540) --- esphome/components/nrf52/framework.py | 26 ++++--- tests/unit_tests/test_nrf52_framework.py | 90 +++++++++++++++++++++++- 2 files changed, 107 insertions(+), 9 deletions(-) diff --git a/esphome/components/nrf52/framework.py b/esphome/components/nrf52/framework.py index 6b32fe1fea..d487820440 100644 --- a/esphome/components/nrf52/framework.py +++ b/esphome/components/nrf52/framework.py @@ -62,6 +62,22 @@ def get_sdk_nrf_tools_path() -> Path: return path.resolve() +def _needs_venv_rebuild( + env_python_path: Path, sentinel: Path, requirements_hash: str +) -> bool: + """True when a penv must be (re)built. + + Rebuild when the interpreter is not a regular file, which covers a + dangling symlink (a cached venv outliving a host interpreter upgrade) + and a corrupt restore, or when the sentinel is missing or stale. + """ + return ( + not env_python_path.is_file() + or not sentinel.exists() + or sentinel.read_text(encoding="utf-8") != requirements_hash + ) + + def _get_python_env_path(version: str) -> Path: return get_sdk_nrf_tools_path() / "penvs" / version @@ -198,10 +214,7 @@ def setup_platformio_python_env() -> None: + "\n".join(_PLATFORMIO_PENV_REQUIREMENTS).encode() + f"python{sys.version_info.major}.{sys.version_info.minor}".encode() ).hexdigest() - if ( - not sentinel.exists() - or sentinel.read_text(encoding="utf-8") != requirements_hash - ): + if _needs_venv_rebuild(env_python_path, sentinel, requirements_hash): rmdir(penv_path, msg="Clean up PlatformIO toolchain Python environment") create_venv(penv_path, msg="PlatformIO toolchain") @@ -250,10 +263,7 @@ def check_and_install() -> None: env_python_path = get_python_env_executable_path(python_env_path, "python") sentinel = python_env_path / ".ready" requirements_hash = hashlib.sha256(_REQUIREMENTS.read_bytes()).hexdigest() - install_venv = ( - not sentinel.exists() - or sentinel.read_text(encoding="utf-8") != requirements_hash - ) + install_venv = _needs_venv_rebuild(env_python_path, sentinel, requirements_hash) if install_venv: rmdir(python_env_path, msg=f"Clean up {version} Python environment") diff --git a/tests/unit_tests/test_nrf52_framework.py b/tests/unit_tests/test_nrf52_framework.py index 0a6bddc280..c2ee0c2a75 100644 --- a/tests/unit_tests/test_nrf52_framework.py +++ b/tests/unit_tests/test_nrf52_framework.py @@ -16,6 +16,7 @@ from esphome.components.nrf52.framework import ( _get_penv_site_packages, _get_platformio_penv_path, _get_toolchain_platform_info, + _needs_venv_rebuild, check_and_install, get_build_env, get_sdk_nrf_tools_path, @@ -123,10 +124,19 @@ def mock_nrf52_ops(): # --------------------------------------------------------------------------- +def _touch_penv_python(penv: Path) -> None: + """Create the interpreter file so the rebuild gate sees a live venv.""" + python = get_python_env_executable_path(penv, "python") + python.parent.mkdir(parents=True, exist_ok=True) + python.touch() + + def _mark_venv_ready(python_env: Path) -> None: - """Write the venv sentinel with the current requirements hash.""" + """Write the venv sentinel with the current requirements hash and a + present interpreter so the rebuild gate passes.""" requirements_hash = hashlib.sha256(_REQUIREMENTS.read_bytes()).hexdigest() (python_env / ".ready").write_text(requirements_hash, encoding="utf-8") + _touch_penv_python(python_env) class TestCheckAndInstall: @@ -148,6 +158,23 @@ class TestCheckAndInstall: mock_nrf52_ops.download_from_mirrors.assert_not_called() mock_nrf52_ops.archive_extract_all.assert_not_called() + def test_missing_interpreter_rebuilds_venv( + self, + nrf52_dirs: SimpleNamespace, + mock_nrf52_ops: SimpleNamespace, + ) -> None: + """A valid sentinel must not mask a missing interpreter (a cached venv + restored after a host interpreter upgrade).""" + requirements_hash = hashlib.sha256(_REQUIREMENTS.read_bytes()).hexdigest() + (nrf52_dirs.python_env / ".ready").write_text( + requirements_hash, encoding="utf-8" + ) + # no interpreter on disk + + check_and_install() + + mock_nrf52_ops.create_venv.assert_called_once() + def test_fresh_install_runs_all_steps( self, nrf52_dirs: SimpleNamespace, @@ -348,6 +375,7 @@ class TestSetupPlatformioPythonEnv: (platformio_penv_dir / ".ready").write_text( _platformio_requirements_hash(), encoding="utf-8" ) + _touch_penv_python(platformio_penv_dir) with patch.dict(os.environ): setup_platformio_python_env() @@ -392,6 +420,22 @@ class TestSetupPlatformioPythonEnv: assert not (platformio_penv_dir / ".ready").exists() + def test_missing_interpreter_reinstalls( + self, + platformio_penv_dir: Path, + mock_nrf52_ops: SimpleNamespace, + ) -> None: + """A valid sentinel must not mask a missing interpreter.""" + (platformio_penv_dir / ".ready").write_text( + _platformio_requirements_hash(), encoding="utf-8" + ) + # no interpreter on disk + + with patch.dict(os.environ): + setup_platformio_python_env() + + mock_nrf52_ops.create_venv.assert_called_once() + def test_repeated_calls_do_not_duplicate_env_entries( self, platformio_penv_dir: Path, @@ -401,6 +445,7 @@ class TestSetupPlatformioPythonEnv: (platformio_penv_dir / ".ready").write_text( _platformio_requirements_hash(), encoding="utf-8" ) + _touch_penv_python(platformio_penv_dir) site_packages = str(_get_penv_site_packages(platformio_penv_dir)) bin_dir = str( get_python_env_executable_path(platformio_penv_dir, "python").parent @@ -422,6 +467,7 @@ class TestSetupPlatformioPythonEnv: (platformio_penv_dir / ".ready").write_text( _platformio_requirements_hash(), encoding="utf-8" ) + _touch_penv_python(platformio_penv_dir) site_packages = str(_get_penv_site_packages(platformio_penv_dir)) with patch.dict(os.environ, {"PYTHONPATH": "/existing/path"}): @@ -531,3 +577,45 @@ def testget_tools_path_default_is_global_cache( Path(platformdirs.user_cache_dir("esphome", appauthor=False)) / "sdk-nrf" ).resolve() assert get_sdk_nrf_tools_path() == expected + + +def test_needs_venv_rebuild_gates(tmp_path: Path) -> None: + """The shared penv gate rebuilds on any missing or stale piece.""" + penv = tmp_path / "penv" + penv.mkdir() + python = penv / "python" + sentinel = penv / ".ready" + good_hash = "abc123" + + # Nothing in place yet + assert _needs_venv_rebuild(python, sentinel, good_hash) + + python.write_text("") + # Interpreter present but no sentinel + assert _needs_venv_rebuild(python, sentinel, good_hash) + + sentinel.write_text(good_hash, encoding="utf-8") + # Everything in place + assert not _needs_venv_rebuild(python, sentinel, good_hash) + + # Stale requirements hash + assert _needs_venv_rebuild(python, sentinel, "otherhash") + + +@pytest.mark.skipif( + sys.platform == "win32", reason="symlink creation needs privileges on Windows" +) +def test_needs_venv_rebuild_on_dangling_interpreter_symlink(tmp_path: Path) -> None: + """A cached venv restored after a host interpreter upgrade has a + bin/python symlink whose target is gone; the valid sentinel must not + mask it.""" + penv = tmp_path / "penv" + penv.mkdir() + python = penv / "python" + sentinel = penv / ".ready" + sentinel.write_text("abc123", encoding="utf-8") + python.symlink_to(tmp_path / "hostedtoolcache" / "3.12.14" / "python3") + assert python.is_symlink() + assert not python.exists() + + assert _needs_venv_rebuild(python, sentinel, "abc123") From f509516d6063ce1fc8549bd221c3223f3e668fba Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 11:32:15 -0500 Subject: [PATCH 04/21] [core] Keep templated !include filenames as strings so Windows path normalization cannot corrupt them (#18549) --- esphome/components/substitutions/__init__.py | 5 +- esphome/yaml_util.py | 28 ++++++---- tests/unit_tests/test_bundle.py | 56 +++++++++++++++++++- tests/unit_tests/test_substitutions.py | 19 +++++++ tests/unit_tests/test_yaml_util.py | 27 +++++++++- 5 files changed, 120 insertions(+), 15 deletions(-) diff --git a/esphome/components/substitutions/__init__.py b/esphome/components/substitutions/__init__.py index b4fcf36c9e..5ef7a699eb 100644 --- a/esphome/components/substitutions/__init__.py +++ b/esphome/components/substitutions/__init__.py @@ -363,13 +363,12 @@ def resolve_include( an explicit non-goal here. """ original = include.file - original_str = str(original) filename = str( _expand_substitutions( - original_str, path + ["file"], context_vars, strict_undefined, errors + original, path + ["file"], context_vars, strict_undefined, errors ) ) - substituted = filename != original_str + substituted = filename != original if substituted: include = include.with_file(filename) try: diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index d3c6caf60b..c280e550c9 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -231,18 +231,22 @@ class IncludeFile: def __init__( self, parent_file: Path, - file: Path | str, + file: str, vars: dict[str, Any] | None, yaml_loader: Callable[[Path], Any], ) -> None: self.parent_file = parent_file - self.file = Path(file) + # The raw include text may be a substitution/Jinja expression, so it + # must never round-trip through Path(): on Windows, WindowsPath str() + # rewrites "/" to "\", which Jinja then decodes as escapes like + # "\b" -> backspace (issue #18545). + self.file = file self.vars = vars self.yaml_loader = yaml_loader self._content: Any = _UNSET def __repr__(self) -> str: - return f"IncludeFile({self.file.as_posix()})" + return f"IncludeFile({self.file})" def load(self) -> Any: """Load and cache the included file content. @@ -258,15 +262,15 @@ class IncludeFile: raise Invalid( f"Cannot load include with unresolved substitutions: {self.file}" ) - self._content = self.yaml_loader(Path(self.parent_file.parent / self.file)) + self._content = self.yaml_loader(self.parent_file.parent / self.file) self._content = add_context(self._content, self.vars) return self._content def has_unresolved_expressions(self) -> bool: """Check if the filename contains substitution variables or Jinja expressions.""" - return has_substitution_or_expression(str(self.file)) + return has_substitution_or_expression(self.file) - def with_file(self, file: Path | str) -> IncludeFile: + def with_file(self, file: str) -> IncludeFile: """Clone this include with *file* as the filename.""" return IncludeFile(self.parent_file, file, self.vars, self.yaml_loader) @@ -313,7 +317,7 @@ def _candidate_include_paths(include: IncludeFile) -> list[Path]: parent_dir = include.parent_file.parent parent_resolved = include.parent_file.resolve() candidates: list[Path] = [] - for pattern in include_candidate_patterns(str(include.file)): + for pattern in include_candidate_patterns(include.file): if "*" in pattern: matches = sorted(_glob_include_candidates(parent_dir, pattern)) else: @@ -362,7 +366,7 @@ def _load_include_candidates( continue expanded_paths.add(candidate) try: - loaded = include.with_file(candidate).load() + loaded = include.with_file(candidate.as_posix()).load() except (EsphomeError, Invalid) as err: # Unlike an unresolved pattern (expected during the discovery # re-parse), a matched on-disk candidate that fails to load is a @@ -794,6 +798,10 @@ class ESPHomeLoaderMixin: file = fields.get("file") if file is None: raise yaml.MarkedYAMLError("Must include 'file'", node.start_mark) + if not isinstance(file, str): + raise yaml.MarkedYAMLError( + "Include 'file' must be a string", node.start_mark + ) vars = fields.get(CONF_VARS) return file, vars @@ -1333,11 +1341,11 @@ class ESPHomeDumper(yaml.SafeDumper): def represent_include_file(self, value): if value.vars: - mapping = {"file": value.file.as_posix(), "vars": value.vars} + mapping = {"file": value.file, "vars": value.vars} return self.represent_mapping( tag="!include", mapping=mapping, flow_style=False ) - return self.represent_scalar(tag="!include", value=value.file.as_posix()) + return self.represent_scalar(tag="!include", value=value.file) def represent_id(self, value): if is_secret(value.id): diff --git a/tests/unit_tests/test_bundle.py b/tests/unit_tests/test_bundle.py index 29e917fe44..1abc7a3ab8 100644 --- a/tests/unit_tests/test_bundle.py +++ b/tests/unit_tests/test_bundle.py @@ -29,8 +29,9 @@ from esphome.bundle import ( read_bundle_manifest, remap_bundle_path, ) +from esphome.components.substitutions import do_substitution_pass from esphome.core import CORE, EsphomeError -from esphome.yaml_util import force_load_include_files +from esphome.yaml_util import force_load_include_files, load_yaml # --------------------------------------------------------------------------- # Helpers @@ -1277,6 +1278,59 @@ def test_discover_files_bundles_all_include_candidates(tmp_path: Path) -> None: assert "includes/empty.yaml" in paths +@pytest.mark.parametrize("enable_proxy", [True, False]) +def test_bundle_roundtrip_templated_include_with_path_separator( + tmp_path: Path, enable_proxy: bool +) -> None: + r"""The issue-18545 flow: a Jinja !include whose branches contain "/" still + resolves after the bundle is extracted on the build server. + + Windows is the leg that regresses: the raw expression text must survive + verbatim, or its separators get rewritten to "\" and Jinja decodes + sequences like "\b" as string escapes. + """ + config_dir = _setup_config_dir( + tmp_path, + files={ + "includes/boards/board.yaml": ( + "packages:\n" + ' - !include ${ "bluetooth/bluetooth_proxy_single_core.yaml"' + ' if enable_bluetooth_proxy else "../empty.yaml" }\n' + ), + "includes/boards/bluetooth/bluetooth_proxy_single_core.yaml": ( + "bluetooth_proxy:\n active: true\n" + ), + "includes/empty.yaml": "{}\n", + }, + ) + (config_dir / "test.yaml").write_text( + "substitutions:\n" + f" enable_bluetooth_proxy: {str(enable_proxy).lower()}\n" + "esphome:\n name: test\n" + "packages:\n - !include includes/boards/board.yaml\n" + ) + + result = ConfigBundleCreator({}).create_bundle() + bundle_path = tmp_path / "device.esphomebundle.tar.gz" + bundle_path.write_bytes(result.data) + + # Both conditional branches must ship in the bundle. + paths = [f.path for f in result.files] + assert "includes/boards/bluetooth/bluetooth_proxy_single_core.yaml" in paths + assert "includes/empty.yaml" in paths + + # Extract to a fresh directory and resolve the config from there, as a + # remote build server would. + extracted_config = extract_bundle(bundle_path, tmp_path / "remote") + config = do_substitution_pass(load_yaml(extracted_config)) + + board_pkg = config["packages"][0]["packages"][0] + if enable_proxy: + assert board_pkg == {"bluetooth_proxy": {"active": True}} + else: + assert board_pkg == {} + + def test_discover_files_candidate_outside_config_dir_skipped( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: diff --git a/tests/unit_tests/test_substitutions.py b/tests/unit_tests/test_substitutions.py index f4063237b1..73c6e496a9 100644 --- a/tests/unit_tests/test_substitutions.py +++ b/tests/unit_tests/test_substitutions.py @@ -744,6 +744,25 @@ def test_include_filename_substitution_undefined_var(tmp_path: Path) -> None: substitutions.do_substitution_pass(config) +def test_include_filename_jinja_expression_with_path_separator( + tmp_path: Path, +) -> None: + """A jinja !include whose string literals contain "/" resolves correctly (issue #18545).""" + main_file = tmp_path / "main.yaml" + main_file.write_text( + "substitutions:\n" + " enable_bluetooth_proxy: true\n" + "result: !include " + '${ "bluetooth/proxy.yaml" if enable_bluetooth_proxy else "../empty.yaml" }\n' + ) + (tmp_path / "bluetooth").mkdir() + (tmp_path / "bluetooth" / "proxy.yaml").write_text("value: 42\n") + + config = yaml_util.load_yaml(main_file) + config = substitutions.do_substitution_pass(config) + assert config["result"] == {"value": 42} + + def test_raise_first_undefined_logs_extras_at_debug( caplog: pytest.LogCaptureFixture, ) -> None: diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index e0a81652e3..3bdbd04396 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -701,6 +701,31 @@ def test_include_file_has_unresolved_expressions( assert include.has_unresolved_expressions() == expected +def test_mapping_include_non_string_file_rejected(tmp_path: Path) -> None: + """The mapping !include form rejects a non-string 'file' with a clear error.""" + entry = tmp_path / "entry.yaml" + entry.write_text("wifi: !include\n file: [not, a, string]\n") + with pytest.raises(EsphomeError, match="Include 'file' must be a string"): + yaml_util.load_yaml(entry) + + +def test_include_file_templated_filename_stays_raw_string(tmp_path: Path) -> None: + """A templated filename keeps its verbatim text (issue #18545).""" + parent = tmp_path / "main.yaml" + expr = '${ "bluetooth/proxy.yaml" if enable_bluetooth_proxy else "../empty.yaml" }' + include = yaml_util.IncludeFile(parent, expr, None, lambda _: {}) + assert include.file == expr + assert include.has_unresolved_expressions() + assert repr(include) == f"IncludeFile({expr})" + + +def test_represent_include_file_templated() -> None: + """Dumping a templated IncludeFile emits the raw expression unchanged.""" + expr = '${ "a/b.yaml" if flag else "../c.yaml" }' + include = yaml_util.IncludeFile(Path("/fake/main.yaml"), expr, None, lambda _: {}) + assert yaml_util.dump({"key": include}) == f"key: !include '{expr}'\n" + + def test_include_in_list_context() -> None: """!include of a file returning a list is handled correctly, including when that list itself contains a nested IncludeFile.""" @@ -1051,7 +1076,7 @@ class _StubInclude: ) -> None: # Default parent lives in a nonexistent directory so unresolved # stubs never glob real files during candidate expansion. - self.file = Path(file) + self.file = file self.parent_file = parent_file or Path("/nonexistent/parent.yaml") self._unresolved = unresolved self._load_result = load_result if load_result is not None else {} From bdb203d74200ab0d23db746f8dcf8359af8822c6 Mon Sep 17 00:00:00 2001 From: "esphome[bot]" <115708604+esphome[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:59:30 -0500 Subject: [PATCH 05/21] Bump bundled esphome-device-builder to 1.12.2 (#18573) --- docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 55aa0ac982..2bbe5331e5 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -22,7 +22,7 @@ RUN \ -r /requirements.txt # Install the ESPHome Device Builder dashboard. -RUN uv pip install --no-cache-dir esphome-device-builder==1.12.1 +RUN uv pip install --no-cache-dir esphome-device-builder==1.12.2 RUN \ platformio settings set enable_telemetry No \ From c6d329db643dfd47de00cabb640fee02536e8f1b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 18:06:23 -0500 Subject: [PATCH 06/21] [espnow] Fix dump_config crash when enable_on_boot is false (#18572) --- esphome/components/espnow/espnow_component.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/esphome/components/espnow/espnow_component.cpp b/esphome/components/espnow/espnow_component.cpp index df9a1b8668..ecf0f79e4a 100644 --- a/esphome/components/espnow/espnow_component.cpp +++ b/esphome/components/espnow/espnow_component.cpp @@ -129,14 +129,17 @@ void on_data_received(const esp_now_recv_info_t *info, const uint8_t *data, int ESPNowComponent::ESPNowComponent() { global_esp_now = this; } void ESPNowComponent::dump_config() { - uint32_t version = 0; - esp_now_get_version(&version); - ESP_LOGCONFIG(TAG, "espnow:"); - if (this->is_disabled()) { - ESP_LOGCONFIG(TAG, " Disabled"); + // Only report driver details once enabled; with enable_on_boot: false the + // Wi-Fi driver is not initialized yet and esp_now_get_version() would crash, + // and after a failed enable_() the values would be meaningless. + if (this->state_ != ESPNOW_STATE_ENABLED) { + // OFF here means enable_() failed; the core logs the FAILED marker separately + ESP_LOGCONFIG(TAG, " %s", this->is_disabled() ? LOG_STR_LITERAL("Disabled") : LOG_STR_LITERAL("Not enabled")); return; } + uint32_t version = 0; + esp_now_get_version(&version); char own_addr_buf[MAC_ADDRESS_PRETTY_BUFFER_SIZE]; format_mac_addr_upper(this->own_address_, own_addr_buf); ESP_LOGCONFIG(TAG, From 5a3d7e3292b396c7c833394398e8da0b11153a79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Metrich?= <45318189+FredM67@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:57:58 +0200 Subject: [PATCH 07/21] [emontx] Fix sensor state_class defaults not being applied correctly (#17610) Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 --- esphome/components/emontx/sensor/__init__.py | 63 ++++++----- tests/component_tests/emontx/__init__.py | 0 .../emontx/test_sensor_defaults.py | 100 ++++++++++++++++++ tests/components/emontx/test.esp32-idf.yaml | 3 +- tests/components/emontx/test.esp8266-ard.yaml | 3 +- tests/components/emontx/test.rp2040-ard.yaml | 3 +- .../components/emontx/validate.esp32-idf.yaml | 73 +++++++++++++ 7 files changed, 213 insertions(+), 32 deletions(-) create mode 100644 tests/component_tests/emontx/__init__.py create mode 100644 tests/component_tests/emontx/test_sensor_defaults.py create mode 100644 tests/components/emontx/validate.esp32-idf.yaml diff --git a/esphome/components/emontx/sensor/__init__.py b/esphome/components/emontx/sensor/__init__.py index 83a972c5e0..967bc4e699 100644 --- a/esphome/components/emontx/sensor/__init__.py +++ b/esphome/components/emontx/sensor/__init__.py @@ -68,6 +68,7 @@ PATTERN_CONFIGS = { "PULSE": { CONF_UNIT_OF_MEASUREMENT: UNIT_PULSES, CONF_DEVICE_CLASS: DEVICE_CLASS_ENERGY, + CONF_STATE_CLASS: STATE_CLASS_TOTAL_INCREASING, CONF_ACCURACY_DECIMALS: 0, }, "PF": { @@ -78,12 +79,13 @@ PATTERN_CONFIGS = { }, } -# Create a base schema that's flexible for any tag -BASE_SCHEMA = sensor.sensor_schema( - EmonTxSensor, - state_class=STATE_CLASS_MEASUREMENT, - accuracy_decimals=0, -).extend( +# BASE_SCHEMA intentionally omits state_class and accuracy_decimals defaults. +# Passing them to sensor_schema() would register them via cv.Optional(key, default=...), +# making them always present in the validated config dict and preventing +# apply_tag_defaults from overriding them with the correct per-prefix values. +# They are injected by apply_tag_defaults below, after running through +# sensor.validate_state_class() so the value is code-generation-ready. +BASE_SCHEMA = sensor.sensor_schema(EmonTxSensor).extend( { cv.GenerateID(CONF_EMONTX_ID): cv.use_id(EmonTx), cv.Required(CONF_TAG_NAME): cv.string, @@ -91,34 +93,43 @@ BASE_SCHEMA = sensor.sensor_schema( ) +def _apply_defaults(config: ConfigType, defaults: dict) -> None: + """Inject defaults into config, skipping keys already set by the user. + state_class values are run through validate_state_class so they are + code-generation-ready, matching what sensor_schema() would normally do.""" + for key, value in defaults.items(): + if key not in config: + if key == CONF_STATE_CLASS: + value = sensor.validate_state_class(value) + config[key] = value + + def apply_tag_defaults(config: ConfigType) -> ConfigType: """Apply defaults based on tag prefix if applicable, but don't restrict any tags.""" tag = config[CONF_TAG_NAME] - # Skip if tag is too short - if len(tag) < 2: - return config + if len(tag) >= 2: + tag_upper = tag.upper() - # Check if this tag starts with a known prefix - tag_upper = tag.upper() + for pattern, pattern_config in PATTERN_CONFIGS.items(): + if tag_upper.startswith(pattern): + _apply_defaults(config, pattern_config) + return config - for pattern, pattern_config in PATTERN_CONFIGS.items(): - if tag_upper.startswith(pattern): - # Apply pattern defaults if not overridden by user - for key, value in pattern_config.items(): - if key not in config: - config[key] = value + # Only apply defaults for known prefixes with numeric indices (e.g. E1, V2, T3) + prefix = tag_upper[0] + if prefix in SENSOR_CONFIGS and tag[1:].isdigit(): + _apply_defaults(config, SENSOR_CONFIGS[prefix]) return config - # Only apply defaults for known prefixes with numeric indices - prefix = tag_upper[0] - if prefix in SENSOR_CONFIGS and len(tag) > 1 and tag[1:].isdigit(): - # Apply defaults for known tag types, but only if not overridden by user - defaults = SENSOR_CONFIGS[prefix] - for key, value in defaults.items(): - if key not in config: - config[key] = value - + # Fall back to generic defaults for tags with no known prefix + _apply_defaults( + config, + { + CONF_STATE_CLASS: STATE_CLASS_MEASUREMENT, + CONF_ACCURACY_DECIMALS: 0, + }, + ) return config diff --git a/tests/component_tests/emontx/__init__.py b/tests/component_tests/emontx/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/component_tests/emontx/test_sensor_defaults.py b/tests/component_tests/emontx/test_sensor_defaults.py new file mode 100644 index 0000000000..00d24d282e --- /dev/null +++ b/tests/component_tests/emontx/test_sensor_defaults.py @@ -0,0 +1,100 @@ +"""Tests for emontx sensor tag defaults.""" + +import pytest + +from esphome.components import sensor +from esphome.components.emontx.sensor import CONFIG_SCHEMA, apply_tag_defaults +from esphome.const import ( + CONF_ACCURACY_DECIMALS, + CONF_STATE_CLASS, + STATE_CLASS_MEASUREMENT, + STATE_CLASS_TOTAL_INCREASING, +) + + +def _resolve_via_config_schema(tag: str) -> dict: + """Run a minimal config through the real CONFIG_SCHEMA pipeline, the + same path a user's YAML goes through.""" + return CONFIG_SCHEMA( + {"tag_name": tag, "emontx_id": "my_emontx", "name": f"{tag} sensor"} + ) + + +def test_config_schema_applies_tag_default_state_class(): + """If sensor_schema(state_class=...) is reintroduced, the schema-level + default wins over apply_tag_defaults' per-prefix value, and E1 would + resolve to measurement instead of total_increasing. Driving the real + CONFIG_SCHEMA (not just apply_tag_defaults) catches that, since + sensor_schema() runs before apply_tag_defaults in the cv.All() chain. + """ + result = _resolve_via_config_schema("E1") + assert result[CONF_STATE_CLASS] == sensor.validate_state_class( + STATE_CLASS_TOTAL_INCREASING + ) + + +def test_config_schema_applies_tag_default_accuracy_decimals(): + """Same root cause as the state_class regression: reintroducing + sensor_schema(accuracy_decimals=...) would make V1 resolve to the + schema-level default instead of the prefix-specific value of 2. + """ + result = _resolve_via_config_schema("V1") + assert result[CONF_ACCURACY_DECIMALS] == 2 + + +def _make_config(tag: str) -> dict: + """Minimal config dict with only tag_name set — no overrides.""" + return {"tag_name": tag} + + +@pytest.mark.parametrize( + ("tag", "expected_state_class", "expected_decimals"), + [ + # Known numeric-index prefixes + ("E1", STATE_CLASS_TOTAL_INCREASING, 0), + ("E12", STATE_CLASS_TOTAL_INCREASING, 0), + ("P1", STATE_CLASS_MEASUREMENT, 0), + ("V1", STATE_CLASS_MEASUREMENT, 2), + ("I1", STATE_CLASS_MEASUREMENT, 2), + ("T1", STATE_CLASS_MEASUREMENT, 2), + # Known patterns + ("PULSE1", STATE_CLASS_TOTAL_INCREASING, 0), + ("PULSE12", STATE_CLASS_TOTAL_INCREASING, 0), + ("PF1", STATE_CLASS_MEASUREMENT, 2), + # Unknown / free-form tags fall back to generic defaults + ("CUSTOM1", STATE_CLASS_MEASUREMENT, 0), + ("X", STATE_CLASS_MEASUREMENT, 0), + ], +) +def test_apply_tag_defaults(tag, expected_state_class, expected_decimals): + """apply_tag_defaults must inject the correct state_class and accuracy_decimals + for each tag type when no user overrides are present.""" + config = _make_config(tag) + result = apply_tag_defaults(config) + + assert result[CONF_STATE_CLASS] == sensor.validate_state_class(expected_state_class) + assert result[CONF_ACCURACY_DECIMALS] == expected_decimals + + +@pytest.mark.parametrize( + ("tag", "user_state_class", "user_decimals"), + [ + # User overrides must not be clobbered by defaults + ("E1", STATE_CLASS_MEASUREMENT, 3), + ("PULSE1", STATE_CLASS_MEASUREMENT, 1), + ("V1", STATE_CLASS_TOTAL_INCREASING, 0), + ("CUSTOM1", STATE_CLASS_TOTAL_INCREASING, 4), + ], +) +def test_apply_tag_defaults_respects_user_overrides( + tag, user_state_class, user_decimals +): + """apply_tag_defaults must not overwrite values already set by the user.""" + config = _make_config(tag) + config[CONF_STATE_CLASS] = sensor.validate_state_class(user_state_class) + config[CONF_ACCURACY_DECIMALS] = user_decimals + + result = apply_tag_defaults(config) + + assert result[CONF_STATE_CLASS] == sensor.validate_state_class(user_state_class) + assert result[CONF_ACCURACY_DECIMALS] == user_decimals diff --git a/tests/components/emontx/test.esp32-idf.yaml b/tests/components/emontx/test.esp32-idf.yaml index a0784fcd53..e56b1bda5d 100644 --- a/tests/components/emontx/test.esp32-idf.yaml +++ b/tests/components/emontx/test.esp32-idf.yaml @@ -1,4 +1,3 @@ packages: uart_115200: !include ../../test_build_components/common/uart_115200/esp32-idf.yaml - -<<: !include common.yaml + emontx: !include common.yaml diff --git a/tests/components/emontx/test.esp8266-ard.yaml b/tests/components/emontx/test.esp8266-ard.yaml index 80a2cb2fc0..9ec9377437 100644 --- a/tests/components/emontx/test.esp8266-ard.yaml +++ b/tests/components/emontx/test.esp8266-ard.yaml @@ -1,4 +1,3 @@ packages: uart_115200: !include ../../test_build_components/common/uart_115200/esp8266-ard.yaml - -<<: !include common.yaml + emontx: !include common.yaml diff --git a/tests/components/emontx/test.rp2040-ard.yaml b/tests/components/emontx/test.rp2040-ard.yaml index 410c579d4b..6f4952d8e5 100644 --- a/tests/components/emontx/test.rp2040-ard.yaml +++ b/tests/components/emontx/test.rp2040-ard.yaml @@ -1,4 +1,3 @@ packages: uart_115200: !include ../../test_build_components/common/uart_115200/rp2040-ard.yaml - -<<: !include common.yaml + emontx: !include common.yaml diff --git a/tests/components/emontx/validate.esp32-idf.yaml b/tests/components/emontx/validate.esp32-idf.yaml new file mode 100644 index 0000000000..7caee78a07 --- /dev/null +++ b/tests/components/emontx/validate.esp32-idf.yaml @@ -0,0 +1,73 @@ +packages: + uart_115200: !include ../../test_build_components/common/uart_115200/esp32-idf.yaml + emontx: !include common.yaml + +# Validate that each sensor type gets the correct default state_class, +# unit_of_measurement, device_class, and accuracy_decimals when NO overrides +# are provided. The values are intentionally omitted so apply_tag_defaults is +# exercised, not the user-override path. + +sensor: + # Energy sensor (E prefix): expects state_class=total_increasing, unit=Wh, + # device_class=energy, accuracy_decimals=0 + - platform: emontx + tag_name: E1 + name: Energy 1 + emontx_id: test_emontx + + # Power sensor (P prefix): expects state_class=measurement, unit=W, + # device_class=power, accuracy_decimals=0 + - platform: emontx + tag_name: P1 + name: Power 1 + emontx_id: test_emontx + + # Voltage sensor (V prefix): expects state_class=measurement, unit=V, + # device_class=voltage, accuracy_decimals=2 + - platform: emontx + tag_name: V1 + name: Voltage 1 + emontx_id: test_emontx + + # Current sensor (I prefix): expects state_class=measurement, unit=A, + # device_class=current, accuracy_decimals=2 + - platform: emontx + tag_name: I1 + name: Current 1 + emontx_id: test_emontx + + # Temperature sensor (T prefix): expects state_class=measurement, unit=°C, + # device_class=temperature, accuracy_decimals=2 + - platform: emontx + tag_name: T1 + name: Temperature 1 + emontx_id: test_emontx + + # Pulse sensor (PULSE pattern): expects state_class=total_increasing, + # unit=pulses, device_class=energy, accuracy_decimals=0 + - platform: emontx + tag_name: PULSE1 + name: Pulse 1 + emontx_id: test_emontx + + # Power factor sensor (PF pattern): expects state_class=measurement, + # device_class=power_factor, accuracy_decimals=2 + - platform: emontx + tag_name: PF1 + name: Power Factor 1 + emontx_id: test_emontx + + # Unknown tag: no prefix match, falls back to state_class=measurement, + # accuracy_decimals=0 + - platform: emontx + tag_name: CUSTOM1 + name: Custom sensor + emontx_id: test_emontx + + # User override: verify that explicit values are respected and not clobbered + - platform: emontx + tag_name: E2 + name: Energy 2 (user override) + emontx_id: test_emontx + state_class: measurement + accuracy_decimals: 3 From f248a85b518b9f112e5e536a8a20ebadd384e0bc Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:53:44 -0400 Subject: [PATCH 08/21] [esp32_hosted] Fire on_update_available trigger when update is detected (#18591) --- .../esp32_hosted/update/esp32_hosted_update.cpp | 9 +++++++++ .../esp32_hosted/test-embedded.esp32-p4-idf.yaml | 3 +++ .../components/esp32_hosted/test-http.esp32-p4-idf.yaml | 3 +++ 3 files changed, 15 insertions(+) diff --git a/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp b/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp index 351b0869b0..4eb5d1745b 100644 --- a/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp +++ b/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp @@ -135,6 +135,10 @@ void Esp32HostedUpdate::setup() { // Publish state this->status_clear_error(); this->publish_state(); + // Defer so the automation runs on the main loop after setup, not during App.setup() + if (this->state_ == update::UPDATE_STATE_AVAILABLE && this->update_available_trigger_) { + this->defer([this]() { this->update_available_trigger_->trigger(this->update_info_); }); + } #else // HTTP mode: check every 10s until network is ready (max 6 attempts) // Only if update interval is > 1 minute to avoid redundant checks @@ -185,6 +189,8 @@ void Esp32HostedUpdate::check() { return; } + const bool was_available = this->state_ == update::UPDATE_STATE_AVAILABLE; + // Compare versions if (this->update_info_.latest_version.empty() || this->update_info_.latest_version == this->update_info_.current_version) { @@ -197,6 +203,9 @@ void Esp32HostedUpdate::check() { this->update_info_.progress = 0.0f; this->status_clear_error(); this->publish_state(); + if (this->state_ == update::UPDATE_STATE_AVAILABLE && !was_available && this->update_available_trigger_) { + this->update_available_trigger_->trigger(this->update_info_); + } #endif } diff --git a/tests/components/esp32_hosted/test-embedded.esp32-p4-idf.yaml b/tests/components/esp32_hosted/test-embedded.esp32-p4-idf.yaml index 9640032b34..5cf33179ba 100644 --- a/tests/components/esp32_hosted/test-embedded.esp32-p4-idf.yaml +++ b/tests/components/esp32_hosted/test-embedded.esp32-p4-idf.yaml @@ -6,3 +6,6 @@ update: type: embedded path: $component_dir/test_firmware.bin sha256: de2f256064a0af797747c2b97505dc0b9f3df0de4f489eac731c23ae9ca9cc31 + on_update_available: + then: + - logger.log: "Coprocessor update available" diff --git a/tests/components/esp32_hosted/test-http.esp32-p4-idf.yaml b/tests/components/esp32_hosted/test-http.esp32-p4-idf.yaml index 17cde0f35d..88b620cfe8 100644 --- a/tests/components/esp32_hosted/test-http.esp32-p4-idf.yaml +++ b/tests/components/esp32_hosted/test-http.esp32-p4-idf.yaml @@ -8,3 +8,6 @@ update: type: http source: https://esphome.github.io/esp-hosted-firmware/manifest/esp32c6.json update_interval: 6h + on_update_available: + then: + - logger.log: "Coprocessor update available" From 603c3539a36cf513e602103f20932d7e31d5d23b Mon Sep 17 00:00:00 2001 From: "esphome[bot]" <115708604+esphome[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:05:00 -0500 Subject: [PATCH 09/21] Bump bundled esphome-device-builder to 1.12.3 (#18601) --- docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 2bbe5331e5..4cde6505b3 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -22,7 +22,7 @@ RUN \ -r /requirements.txt # Install the ESPHome Device Builder dashboard. -RUN uv pip install --no-cache-dir esphome-device-builder==1.12.2 +RUN uv pip install --no-cache-dir esphome-device-builder==1.12.3 RUN \ platformio settings set enable_telemetry No \ From 6b6d27f905ad9c813b806e2fde8f63173876177d Mon Sep 17 00:00:00 2001 From: "esphome[bot]" <115708604+esphome[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:42:53 +0000 Subject: [PATCH 10/21] Bump bundled esphome-device-builder to 1.12.4 (#18651) --- docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 4cde6505b3..9f27d51059 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -22,7 +22,7 @@ RUN \ -r /requirements.txt # Install the ESPHome Device Builder dashboard. -RUN uv pip install --no-cache-dir esphome-device-builder==1.12.3 +RUN uv pip install --no-cache-dir esphome-device-builder==1.12.4 RUN \ platformio settings set enable_telemetry No \ From b28efcd545290dca53166f416b77117771fb69dc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 18:59:24 -0500 Subject: [PATCH 11/21] [bk72xx_ble] Block BK7238 until the LibreTiny bonding partition fix lands (#18649) --- esphome/components/bk72xx_ble/__init__.py | 30 +++++++++---------- .../bk72xx_ble/config/test_bk7238.yaml | 7 +++++ .../bk72xx_ble/test_family_gate.py | 1 + 3 files changed, 23 insertions(+), 15 deletions(-) create mode 100644 tests/component_tests/bk72xx_ble/config/test_bk7238.yaml diff --git a/esphome/components/bk72xx_ble/__init__.py b/esphome/components/bk72xx_ble/__init__.py index b58464a1f6..fc89a7bfc4 100644 --- a/esphome/components/bk72xx_ble/__init__.py +++ b/esphome/components/bk72xx_ble/__init__.py @@ -4,9 +4,12 @@ The platform analog of esp32_ble / rp2040_ble: owns the Beken BDK BLE stack bring-up and the controller BLE address. Consumers (bk72xx_ble_tracker) build on this component and contain no SDK calls of their own. -Supported SoCs (BLE 5.x): BK7231N/BK7236 (BLE 5.1), BK7238/BK7252N/BK7253 -(BLE 5.2), and any future BLE-5.x SoC. Known non-5.x families are rejected in -to_code; unknown families are capability-checked at compile time via +Supported SoCs (BLE 5.x): BK7231N/BK7236 (BLE 5.1), BK7252N/BK7253 (BLE 5.2), +and any future BLE-5.x SoC. BK7238 (BLE 5.2) is blocked for now: with BLE +compiled in, the Beken SDK erases the bootloader flash sector at boot because +LibreTiny's partition table has no BLE bonding entry (esphome#18646, +libretiny-eu/libretiny#408). Known non-5.x families and BK7238 are rejected in +to_code. Unknown families are capability-checked at compile time via `__has_include("app_ble.h")`, a header only on the BLE 5.x include path (ble_api.h ships for every SoC, so it cannot be the probe). A non-5.x build fails with a clear #error. @@ -65,6 +68,14 @@ def _unsupported_family_message(family: str) -> str | None: ) if family == FAMILY_BK7231Q: return "bk72xx_ble does not support BK7231Q: this SoC has no BLE" + if family == FAMILY_BK7238: + return ( + "bk72xx_ble is disabled on BK7238: with BLE compiled in, the Beken SDK " + "erases the bootloader flash sector at boot and the device can no longer " + "start (see https://github.com/esphome/esphome/issues/18646); support " + "returns once the LibreTiny partition table fix " + "(libretiny-eu/libretiny#408) is released" + ) return None @@ -114,18 +125,7 @@ async def to_code(config: ConfigType) -> None: # BK7231N, but NOT on BK7238 (its BLE stack has no such symbol; the address is # derived from the WiFi MAC instead — the BDK's own fallback). Tell the C++ # which path is available so it doesn't reference a missing symbol. - family = libretiny.get_libretiny_family() - if family == FAMILY_BK7231N: + if libretiny.get_libretiny_family() == FAMILY_BK7231N: cg.add_define("BK72XX_BLE_HAS_COMMON_BDADDR") - elif family == FAMILY_BK7238: - # ESPHome's LibreTiny disables BLE on BK7238 because the SDK can hang at - # WiFi STA startup when BLE init runs. This component re-enables BLE, so - # warn loudly: BK7238 is accepted but not hardware-verified and may be - # WiFi-unstable with BLE on. - _LOGGER.warning( - "bk72xx_ble on BK7238: enabling BLE is known to risk a WiFi STA startup " - "hang on this family and is not yet hardware-verified. Expect possible " - "instability." - ) cg.add_define("USE_BK72XX_BLE") diff --git a/tests/component_tests/bk72xx_ble/config/test_bk7238.yaml b/tests/component_tests/bk72xx_ble/config/test_bk7238.yaml new file mode 100644 index 0000000000..0880cf69f5 --- /dev/null +++ b/tests/component_tests/bk72xx_ble/config/test_bk7238.yaml @@ -0,0 +1,7 @@ +esphome: + name: bk-family-gate-7238 + +bk72xx: + board: generic-bk7238 + +bk72xx_ble: diff --git a/tests/component_tests/bk72xx_ble/test_family_gate.py b/tests/component_tests/bk72xx_ble/test_family_gate.py index da67749bb3..86f3ef0039 100644 --- a/tests/component_tests/bk72xx_ble/test_family_gate.py +++ b/tests/component_tests/bk72xx_ble/test_family_gate.py @@ -16,6 +16,7 @@ from esphome.core import EsphomeError ("test_bk7231t.yaml", "BK7231T.*BLE 4.2"), ("test_bk7252.yaml", "BK7251.*BLE 4.2"), ("test_bk7231q.yaml", "BK7231Q.*no BLE"), + ("test_bk7238.yaml", "BK7238.*bootloader"), ], ) def test_unsupported_family_rejected( From d770004e0eec62e1e9610c6dc8408562d2d309f9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 20:57:58 -0500 Subject: [PATCH 12/21] [esp32_ble] Log connection parameter update results (#18607) --- esphome/components/esp32_ble/ble.cpp | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index e2d79173ff..6e6fb0e30d 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -643,8 +643,28 @@ void ESP32BLE::gap_event_handler(esp_gap_ble_cb_event_t event, esp_ble_gap_cb_pa App.wake_loop_threadsafe(); return; + // Log the result of connection parameter updates: a peer can reject or + // never answer an update, and without this the link silently stays on the + // old parameters (visible only as unexplained supervision timeouts). + case ESP_GAP_BLE_UPDATE_CONN_PARAMS_EVT: { + if (param->update_conn_params.status != ESP_BT_STATUS_SUCCESS) { + char mac_s[MAC_ADDRESS_PRETTY_BUFFER_SIZE]; + format_mac_addr_upper(param->update_conn_params.bda, mac_s); + ESP_LOGW(TAG, "[%s] Conn param update failed, status=%d", mac_s, param->update_conn_params.status); + } +#if ESPHOME_LOG_LEVEL >= ESPHOME_LOG_LEVEL_VERBOSE + else { + char mac_s[MAC_ADDRESS_PRETTY_BUFFER_SIZE]; + format_mac_addr_upper(param->update_conn_params.bda, mac_s); + ESP_LOGV(TAG, "[%s] Conn params updated: interval=%u (x1.25ms) latency=%u timeout=%u (x10ms)", mac_s, + param->update_conn_params.conn_int, param->update_conn_params.latency, + param->update_conn_params.timeout); + } +#endif + return; + } + // Ignore these GAP events as they are not relevant for our use case - case ESP_GAP_BLE_UPDATE_CONN_PARAMS_EVT: case ESP_GAP_BLE_SET_PKT_LENGTH_COMPLETE_EVT: case ESP_GAP_BLE_PHY_UPDATE_COMPLETE_EVT: // BLE 5.0 PHY update complete case ESP_GAP_BLE_CHANNEL_SELECT_ALGORITHM_EVT: // BLE 5.0 channel selection algorithm From ca3f31643fcc9b8341a8fb6b0295fb7125b48654 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 20:58:13 -0500 Subject: [PATCH 13/21] [esp8266] Don't report stale crash state after hardware WDT resets (#18597) --- esphome/components/esp8266/crash_handler.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/esphome/components/esp8266/crash_handler.cpp b/esphome/components/esp8266/crash_handler.cpp index 91b0cf9082..dc79043f21 100644 --- a/esphome/components/esp8266/crash_handler.cpp +++ b/esphome/components/esp8266/crash_handler.cpp @@ -118,8 +118,6 @@ static const LogString *get_exception_cause(uint32_t cause) { } static const LogString *get_reset_reason(uint32_t reason) { - if (reason == REASON_WDT_RST) - return LOG_STR("Hardware WDT"); if (reason == REASON_EXCEPTION_RST) return LOG_STR("Exception"); if (reason == REASON_SOFT_WDT_RST) @@ -162,13 +160,20 @@ void crash_handler_log() { if (!is_crash_reason(resetInfo.reason)) return; + ESP_LOGE(TAG, "*** CRASH DETECTED ON PREVIOUS BOOT ***"); + if (resetInfo.reason == REASON_WDT_RST) { + // A hardware WDT reset happens entirely in hardware: the postmortem hook + // never runs, so rst_info epc1/exccause and the RTC backtrace are + // leftovers from an earlier crash. Don't misattribute them (#18596). + ESP_LOGE(TAG, " Reason: Hardware WDT (no crash state is recorded for hardware WDT resets)"); + return; + } + // Read and filter backtrace from RTC into stack-local buffer (no persistent RAM cost). // Both resetInfo and RTC data survive until the next reset, so this can be // called multiple times (logger init + API subscribe) with the same result. uint32_t backtrace[MAX_BACKTRACE]; uint8_t bt_count = read_rtc_backtrace(backtrace, MAX_BACKTRACE); - - ESP_LOGE(TAG, "*** CRASH DETECTED ON PREVIOUS BOOT ***"); // GCC's ROM divide routine triggers IllegalInstruction (exccause=0) at specific // ROM addresses instead of IntegerDivideByZero (exccause=6). Patch to match // the Arduino core's postmortem handler behavior. From 4c62420f1bce9b52bb8650a96fce663329863cde Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 20:58:28 -0500 Subject: [PATCH 14/21] [core] Dump the main.cpp config comment with sorted keys (#18653) --- esphome/__main__.py | 6 ++++-- tests/unit_tests/test_main.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index 1262a4525e..695581a435 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -762,9 +762,11 @@ def _wrap_to_code(name, comp, yaml_util): async def wrapped(conf): cg.add(cg.LineComment(f"{name}:")) if comp.config_schema is not None: - conf_str = yaml_util.dump(conf) + # sort_keys: voluptuous fills defaults in set order, so an + # unsorted dump would churn main.cpp and relink every run + conf_str = yaml_util.dump(conf, sort_keys=True) conf_str = conf_str.replace("//", "") - # remove tailing \ to avoid multi-line comment warning + # remove trailing \ to avoid multi-line comment warning conf_str = conf_str.replace("\\\n", "\n") cg.add(cg.LineComment(indent(conf_str))) await coro(conf) diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index a40341e194..1cb710ca58 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -11,6 +11,7 @@ from pathlib import Path import re import sys import time +from types import SimpleNamespace from typing import Any, Self from unittest.mock import AsyncMock, MagicMock, Mock, patch @@ -18,7 +19,7 @@ import pytest from pytest import CaptureFixture from zeroconf import ServiceStateChange -from esphome import __main__ as main +from esphome import __main__ as main, yaml_util from esphome.__main__ import ( Purpose, _get_configured_xtal_freq, @@ -29,6 +30,7 @@ from esphome.__main__ import ( _unresolved_default_error, _validate_bootloader_binary, _validate_partition_table_binary, + _wrap_to_code, check_permissions, choose_upload_log_host, command_analyze_memory, @@ -116,6 +118,7 @@ from esphome.espota2 import ( OTA_TYPE_UPDATE_PARTITION_TABLE, ) from esphome.platformio import toolchain +from esphome.types import ConfigType from esphome.util import BootselResult, FlashImage from esphome.zeroconf import _await_discovery, discover_mdns_devices @@ -7130,3 +7133,28 @@ def test_warn_source_tree_mismatch_falls_back_when_stat_fails( # Same tree, so the path comparison still finds them equal and stays silent assert not caplog.text + + +@pytest.mark.asyncio +async def test_wrap_to_code_comment_is_insertion_order_independent() -> None: + """The config comment dumps with sorted keys: voluptuous fills schema + defaults in set-iteration order, so an unsorted dump would churn + main.cpp and relink the firmware on every run.""" + comments: list[str] = [] + + async def to_code(conf: ConfigType) -> None: + """Accept any config; only the wrapper's comment output matters.""" + + comp = SimpleNamespace(to_code=to_code, config_schema=object()) + wrapped = _wrap_to_code("demo", comp, yaml_util) + with patch("esphome.codegen.add", side_effect=lambda st: comments.append(str(st))): + # Nested on purpose: the real churn lives in nested action configs, + # so sorting must apply at every mapping level + await wrapped({"beta": 1, "alpha": {"z": 1, "a": 2}}) + first = "\n".join(comments) + comments.clear() + await wrapped({"alpha": {"a": 2, "z": 1}, "beta": 1}) + second = "\n".join(comments) + assert first == second + assert second.index("alpha") < second.index("beta") + assert second.index("a: 2") < second.index("z: 1") From fb1332792262c7a15a48b1eb9028889f75f7cbda Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 22:00:17 -0500 Subject: [PATCH 15/21] [esp32] Report abort and task watchdog panics correctly in crash handler (#18575) --- esphome/components/esp32/crash_handler.cpp | 61 +++++++++++++++++++--- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/esphome/components/esp32/crash_handler.cpp b/esphome/components/esp32/crash_handler.cpp index b61dad7386..6f65243aaa 100644 --- a/esphome/components/esp32/crash_handler.cpp +++ b/esphome/components/esp32/crash_handler.cpp @@ -124,6 +124,15 @@ static uint8_t IRAM_ATTR capture_riscv_backtrace(RvExcFrame *frame, uint32_t *ou // Version is uint32_t because it would be padded to 4 bytes anyway before the next // uint32_t field, so we use the full width rather than wasting 3 bytes of padding. static constexpr uint32_t CRASH_DATA_VERSION = 4; +#if CONFIG_IDF_TARGET_ARCH_XTENSA +// EXCCAUSE is a 6-bit register; larger recorded values mean the frame's +// cause/vaddr slots were never written (not a real exception frame). +static constexpr uint32_t XTENSA_EXCCAUSE_COUNT = XCHAL_EXCCAUSE_NUM; +#elif CONFIG_IDF_TARGET_ARCH_RISCV +// Synchronous mcause exception codes are small and have no interrupt bit; +// anything else in a non-pseudo record is a stale slot. +static constexpr uint32_t RISCV_EXCEPTION_CAUSE_COUNT = 32; +#endif struct RawCrashData { uint32_t version; uint32_t magic; @@ -198,10 +207,28 @@ void crash_handler_clear() { s_raw_crash_data.magic = 0; } +// Whether the cause slot was written by a real exception frame. +static bool cause_slot_was_written() { +#if CONFIG_IDF_TARGET_ARCH_XTENSA + return s_raw_crash_data.cause < XTENSA_EXCCAUSE_COUNT; +#else + return s_raw_crash_data.cause < RISCV_EXCEPTION_CAUSE_COUNT; +#endif +} + // Look up the exception cause as a human-readable string. // Tables mirror ESP-IDF's panic_arch_fill_info() which uses local static arrays // not exposed via any public API. static const char *get_exception_reason() { + uint8_t exception = s_raw_crash_data.exception; + if (exception == PANIC_EXCEPTION_ABORT || exception == PANIC_EXCEPTION_TWDT) { + // Abort-class panics carry no cause register + return nullptr; + } + if (!cause_slot_was_written()) { + // Garbage from old-build or corrupt records; report just the type + return nullptr; + } #if CONFIG_IDF_TARGET_ARCH_XTENSA if (s_raw_crash_data.pseudo_excause) { // SoC-level panic: watchdog, cache error, etc. @@ -354,10 +381,11 @@ static const char *const FAULT_ADDR_REG = "MTVAL"; static const char *const FAULT_ADDR_REG_LOWER = "mtval"; #endif -// Whether the fault address is meaningful — real CPU faults only, not -// aborts/watchdogs or SoC-level pseudo exceptions. +// Whether the fault address is meaningful: real CPU faults with a validly +// written frame only. static bool has_fault_addr() { - return s_raw_crash_data.exception == PANIC_EXCEPTION_FAULT && !s_raw_crash_data.pseudo_excause; + return s_raw_crash_data.exception == PANIC_EXCEPTION_FAULT && !s_raw_crash_data.pseudo_excause && + cause_slot_was_written(); } // The record was captured by a different firmware build (it survives soft @@ -458,6 +486,10 @@ void crash_handler_log() { // into NOINIT memory before the normal panic handler runs. // extern "C" { +// Set by IDF's task watchdog (task_wdt.c, no header) before it simulates an +// abort; weak so builds without the task watchdog still link. +extern bool g_twdt_isr __attribute__((weak)); + // NOLINTBEGIN(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp,readability-identifier-naming) // Names are mandated by the --wrap linker mechanism extern void __real_esp_panic_handler(panic_info_t *info); @@ -470,6 +502,14 @@ void IRAM_ATTR __wrap_esp_panic_handler(panic_info_t *info) { s_raw_crash_data.exception = (uint8_t) info->exception; s_raw_crash_data.pseudo_excause = info->pseudo_excause ? 1 : 0; s_raw_crash_data.crashed_core = (uint8_t) info->core; + if (g_panic_abort) { + // IDF reclassifies to ABORT only inside esp_panic_handler(), after this + // wrapper captured info->exception; correct it here. TWDT is our own + // distinction (IDF never assigns PANIC_EXCEPTION_TWDT). The abort text is + // not stored; the symbolized backtrace already identifies the site. + bool is_twdt = &g_twdt_isr != nullptr && g_twdt_isr; + s_raw_crash_data.exception = (uint8_t) (is_twdt ? PANIC_EXCEPTION_TWDT : PANIC_EXCEPTION_ABORT); + } // Zero unconditionally so a null frame doesn't leave stale .noinit data from a previous boot s_raw_crash_data.cause = 0; s_raw_crash_data.fault_addr = 0; @@ -487,8 +527,12 @@ void IRAM_ATTR __wrap_esp_panic_handler(panic_info_t *info) { // Xtensa: walk the backtrace using the public API if (info->frame != nullptr) { auto *xt_frame = (XtExcFrame *) info->frame; - s_raw_crash_data.cause = xt_frame->exccause; - s_raw_crash_data.fault_addr = xt_frame->excvaddr; + if (!g_panic_abort) { + // Abort-class frames carry no useful cause/vaddr: TWDT task snapshots + // never wrote them and abort() traps describe only the synthetic trap. + s_raw_crash_data.cause = xt_frame->exccause; + s_raw_crash_data.fault_addr = xt_frame->excvaddr; + } s_raw_crash_data.backtrace_count = walk_xtensa_backtrace(xt_frame, s_raw_crash_data.backtrace, MAX_BACKTRACE); } @@ -510,8 +554,11 @@ void IRAM_ATTR __wrap_esp_panic_handler(panic_info_t *info) { // RISC-V: capture MEPC + RA, then scan stack for code addresses if (info->frame != nullptr) { auto *rv_frame = (RvExcFrame *) info->frame; - s_raw_crash_data.cause = rv_frame->mcause; - s_raw_crash_data.fault_addr = rv_frame->mtval; + if (!g_panic_abort) { + // See the Xtensa branch: abort-class frames carry no valid cause/vaddr. + s_raw_crash_data.cause = rv_frame->mcause; + s_raw_crash_data.fault_addr = rv_frame->mtval; + } s_raw_crash_data.backtrace_count = capture_riscv_backtrace(rv_frame, s_raw_crash_data.backtrace, MAX_BACKTRACE, &s_raw_crash_data.reg_frame_count); } From 4ce4768ebd2af64ddba7741b187e51d26eaf1476 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:25:39 -0500 Subject: [PATCH 16/21] [core] Retry transient network errors when downloading external files (#18538) --- esphome/external_files.py | 21 ++- esphome/framework_helpers.py | 32 +---- esphome/net_retry.py | 114 ++++++++++++++++ tests/unit_tests/test_external_files.py | 138 +++++++++++++++++++- tests/unit_tests/test_framework_helpers.py | 38 ------ tests/unit_tests/test_net_retry.py | 143 +++++++++++++++++++++ 6 files changed, 416 insertions(+), 70 deletions(-) create mode 100644 esphome/net_retry.py create mode 100644 tests/unit_tests/test_net_retry.py diff --git a/esphome/external_files.py b/esphome/external_files.py index f30d429425..58be4a7c26 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -16,6 +16,7 @@ from esphome.const import CONF_FILE, CONF_TYPE, CONF_URL, __version__ from esphome.core import CORE, EsphomeError, TimePeriodSeconds from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import write_file +from esphome.net_retry import fetch_with_retry from esphome.types import ConfigType _LOGGER = logging.getLogger(__name__) @@ -157,8 +158,17 @@ def has_remote_file_changed( } if etag := _read_etag(local_file_path): headers[IF_NONE_MATCH] = etag - response = requests.head( - url, headers=headers, timeout=timeout, allow_redirects=True + # Retried so allow_stale=False consumers don't hard-fail on a + # healed flake. Only connection-level failures retry: HEAD + # never raises on HTTP status (servers rejecting HEAD with + # 405/501 must fall through to the GET), so 5xx is handled by + # the GET's own retry. + response = fetch_with_retry( + url, + lambda: requests.head( + url, headers=headers, timeout=timeout, allow_redirects=True + ), + what="Revalidation", ) _LOGGER.debug( @@ -293,7 +303,7 @@ def download_content( _LOGGER.info("Downloading %s", url) _LOGGER.debug("Saving to %s", path) - try: + def _fetch() -> tuple[requests.Response, bytes]: req = requests.get( url, timeout=timeout, @@ -304,7 +314,10 @@ def download_content( # and mid-stream connection errors all surface here as # RequestException subclasses, so this needs the same fall-back # treatment as the request itself. - data = req.content + return req, req.content + + try: + req, data = fetch_with_retry(url, _fetch) except requests.exceptions.RequestException as e: if path.exists(): # Memoized so a flaky host warns once per run, not per consumer. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index b8a43220ff..105791c518 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -15,6 +15,7 @@ from typing import IO, TYPE_CHECKING from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import ProgressBar, rmtree +from esphome.net_retry import NETWORK_MAX_ATTEMPTS, is_transient_download_error if TYPE_CHECKING: import requests @@ -29,8 +30,9 @@ _LOGGER = logging.getLogger(__name__) _MIRROR_ATTEMPTS = 3 # Passes over the whole mirror list when a transient network error is in -# the mix; matches git.py's _NETWORK_MAX_ATTEMPTS (3 tries, 2s/4s backoff). -_MIRROR_SWEEP_ATTEMPTS = 3 +# the mix; shares net_retry's policy (3 tries, 2s/4s backoff), which in +# turn matches git.py's _NETWORK_MAX_ATTEMPTS. +_MIRROR_SWEEP_ATTEMPTS = NETWORK_MAX_ATTEMPTS def get_project_link_flags() -> list[str]: @@ -903,30 +905,6 @@ def _spent_attempts_error(e: Exception, attempts: int) -> Exception: return err -def _is_transient_download_error(e: Exception) -> bool: - """Return True when a download failure is worth retrying. - - Connection-level failures and HTTP 429/5xx are transient. Other HTTP - errors, local errors, and exhausted-attempts EsphomeError wrappers - (their per-mirror retries are already spent) are permanent. - """ - # Imported lazily: requests is a heavy import (~85ms) and is only - # needed when actually downloading, never during config validation. - import requests - - if isinstance(e, requests.exceptions.HTTPError): - resp = e.response - return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) - return isinstance( - e, - ( - requests.exceptions.ConnectionError, - requests.exceptions.Timeout, - requests.exceptions.ChunkedEncodingError, - ), - ) - - def _try_mirrors_once( urls: list[str], path_target: Path | None, @@ -1131,7 +1109,7 @@ def download_from_mirrors( # Permanent failures (404, verification mismatch) won't heal; # only retry when a transient error is in the mix (as git.py does). transient = next( - ((u, e) for u, e in sweep_failures if _is_transient_download_error(e)), + ((u, e) for u, e in sweep_failures if is_transient_download_error(e)), None, ) if transient is None: diff --git a/esphome/net_retry.py b/esphome/net_retry.py new file mode 100644 index 0000000000..f7e6e601ea --- /dev/null +++ b/esphome/net_retry.py @@ -0,0 +1,114 @@ +"""Retry policy for HTTP downloads. + +Kept import-light on purpose: this module is imported at config time, so it +must not pull in requests (a heavy import, ~85ms) at module scope. +""" + +from __future__ import annotations + +from collections.abc import Callable +import logging +import time + +_LOGGER = logging.getLogger(__name__) + +# 3 tries with 2s/4s backoff, matching git.py's _NETWORK_MAX_ATTEMPTS. +# Callers memoize failures so a flaky host pays this once per file per run. +NETWORK_MAX_ATTEMPTS = 3 + + +def _is_permanent_dns_failure(e: BaseException) -> bool: + """Whether a hard socket.gaierror hides in ``e``'s exception chain. + + EAI_AGAIN (flaky resolver) stays retryable; anything else is permanent + so offline builds fall back to their cache without sleeping first. + Narrower than git.py, which retries NXDOMAIN too. + + Walks ``__cause__``, ``args`` (requests wraps MaxRetryError without + ``from``) and MaxRetryError's ``reason``, but not implicit + ``__context__``: an unrelated earlier attempt's resolution failure + must not reclassify an error it did not cause. + """ + import socket + + seen: set[int] = set() + stack: list[BaseException] = [e] + while stack: + exc = stack.pop() + if id(exc) in seen: + continue + if ( + isinstance(exc, socket.gaierror) + and exc.errno is not None + and exc.errno != socket.EAI_AGAIN + ): + return True + seen.add(id(exc)) + stack.extend( + nxt + for nxt in ( + exc.__cause__, + getattr(exc, "reason", None), # urllib3 MaxRetryError + *exc.args, + ) + if isinstance(nxt, BaseException) + ) + return False + + +def is_transient_download_error(e: Exception) -> bool: + """Return True when a download failure is worth retrying. + + Connection-level failures and HTTP 429/5xx are transient; hard DNS + failures, other HTTP errors, and local errors are permanent. + """ + # Imported lazily: requests is a heavy import (~85ms) and is only + # needed when actually downloading, never during config validation. + import requests + + if isinstance(e, requests.exceptions.HTTPError): + resp = e.response + return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) + if isinstance(e, requests.exceptions.ConnectionError) and _is_permanent_dns_failure( + e + ): + return False + # SSLError (a ConnectionError subclass) stays transient on purpose: it + # also covers mid-handshake connection drops, not just bad certificates. + return isinstance( + e, + ( + requests.exceptions.ConnectionError, + requests.exceptions.Timeout, + requests.exceptions.ChunkedEncodingError, + requests.exceptions.ContentDecodingError, + ), + ) + + +def fetch_with_retry[T](url: str, fetch: Callable[[], T], what: str = "Download") -> T: + """Run ``fetch``, retrying transient failures with 2s/4s backoff. + + Permanent failures and the final attempt propagate to the caller; + ``what`` names the operation in the retry warning. + """ + import requests + + for attempt in range(1, NETWORK_MAX_ATTEMPTS): + try: + return fetch() + except requests.exceptions.RequestException as e: + if not is_transient_download_error(e): + raise + delay = 2**attempt + _LOGGER.warning( + "%s of %s failed: %s. Retrying in %d seconds... (attempt %d/%d)", + what, + url, + e, + delay, + attempt + 1, + NETWORK_MAX_ATTEMPTS, + ) + time.sleep(delay) + return fetch() diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index 4e993ff4f3..caefb7ed5c 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -4,7 +4,7 @@ import os from pathlib import Path import time from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest import requests @@ -81,6 +81,15 @@ def mock_download_content_many() -> MagicMock: yield m +@pytest.fixture +def mock_retry_sleep() -> MagicMock: + """Patch the retry backoff sleep (process-wide; net_retry.time is the + global module) so transient-error tests don't really wait 2s/4s. + """ + with patch("esphome.net_retry.time.sleep") as m: + yield m + + def test_compute_local_file_dir(setup_core: Path) -> None: """Test compute_local_file_dir creates and returns correct path.""" domain = "font" @@ -495,6 +504,7 @@ class _BodyReadErrorResponse: def test_download_content_with_body_read_error_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """Body-read errors (chunked-decode/gzip-decode/mid-stream connection @@ -519,6 +529,7 @@ def test_download_content_with_body_read_error_uses_cache( def test_download_content_with_body_read_error_no_cache_fails( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """A body-read failure with no cache available must surface as a @@ -535,6 +546,131 @@ def test_download_content_with_body_read_error_no_cache_fails( external_files.download_content("https://example.com/file.txt", test_file) +def test_download_content_retries_transient_error_then_succeeds( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Transient failures (connection reset, timeout) are retried with 2s/4s + backoff before giving up; a late success downloads normally.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + requests.exceptions.Timeout("timed out"), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert test_file.read_bytes() == b"downloaded" + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_transient_error_exhausts_attempts( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """A persistent transient failure gives up after three attempts and then + follows the normal no-cache error path.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + mock_requests_get.side_effect = requests.exceptions.ConnectionError("reset by peer") + + with pytest.raises(Invalid, match="Could not download from.*reset by peer"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_non_transient_error_not_retried( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Permanent failures like a 404 fail on the first attempt.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + + response = MagicMock() + response.status_code = 404 + mock_requests_get.side_effect = requests.exceptions.HTTPError( + "404 Client Error", response=response + ) + + with pytest.raises(Invalid, match="Could not download from.*404"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_requests_get.call_count == 1 + mock_retry_sleep.assert_not_called() + + +def test_download_content_retries_body_read_error( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Mid-stream failures surfacing from `.content` are retried too.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + _BodyReadErrorResponse( + requests.exceptions.ChunkedEncodingError("body truncated") + ), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert mock_requests_get.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + + +def test_has_remote_file_changed_retries_transient_error( + mock_requests_head: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A HEAD revalidation that fails transiently then returns 304 does not + mark the cached copy stale, and the retry warning names the operation.""" + test_file = setup_core / "cached.txt" + test_file.write_bytes(b"cached content") + + ok = MagicMock() + ok.status_code = 304 + ok.headers = {} + mock_requests_head.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + ok, + ] + + changed = external_files.has_remote_file_changed( + "https://example.com/file.txt", test_file + ) + + assert changed is False + assert test_file not in external_files._run_data().stale_paths + assert mock_requests_head.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + assert "Revalidation of" in caplog.text + + def test_download_content_skip_external_update_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 2022c15bfe..500705ef67 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -23,7 +23,6 @@ from esphome.core import EsphomeError from esphome.framework_helpers import ( _7z_extract_all, _detect_archive_root, - _is_transient_download_error, _rename_with_retry, _tar_extract_all, _zip_extract_all, @@ -1594,43 +1593,6 @@ class TestDownloadFromMirrors: mock_sleep.assert_not_called() -def _http_error(status: int) -> req.HTTPError: - """An HTTPError carrying a response with the given status, as raised by - ``raise_for_status`` on a real response.""" - resp = MagicMock() - resp.status_code = status - return req.HTTPError(str(status), response=resp) - - -class TestIsTransientDownloadError: - def test_connection_errors_are_transient(self) -> None: - assert _is_transient_download_error(req.ConnectionError("reset")) - assert _is_transient_download_error(req.Timeout("timed out")) - assert _is_transient_download_error( - req.exceptions.ChunkedEncodingError("dropped") - ) - - def test_http_statuses(self) -> None: - assert not _is_transient_download_error(_http_error(404)) - assert not _is_transient_download_error(_http_error(403)) - assert _is_transient_download_error(_http_error(429)) - assert _is_transient_download_error(_http_error(503)) - - def test_http_error_without_response_is_permanent(self) -> None: - assert not _is_transient_download_error(req.HTTPError("boom")) - - def test_exhausted_resume_attempts_are_permanent(self) -> None: - """download_with_resume already spent its own resume attempts; its - EsphomeError wrapper is not retried again at the sweep level.""" - wrapped = EsphomeError("Failed to download after 3 attempts") - wrapped.__cause__ = req.ConnectionError("down") - assert not _is_transient_download_error(wrapped) - - def test_unrelated_errors_are_permanent(self) -> None: - assert not _is_transient_download_error(OSError("disk full")) - assert not _is_transient_download_error(EsphomeError("size mismatch")) - - def test_importing_framework_helpers_does_not_import_requests() -> None: """Importing framework_helpers must not drag in requests. diff --git a/tests/unit_tests/test_net_retry.py b/tests/unit_tests/test_net_retry.py new file mode 100644 index 0000000000..c22bda5ee5 --- /dev/null +++ b/tests/unit_tests/test_net_retry.py @@ -0,0 +1,143 @@ +"""Tests for esphome.net_retry.""" + +import socket +from unittest.mock import MagicMock, call, patch + +import pytest +import requests as req + +from esphome.core import EsphomeError +from esphome.net_retry import fetch_with_retry, is_transient_download_error + + +def _http_error(status: int) -> req.HTTPError: + """An HTTPError carrying a response with the given status, as raised by + ``raise_for_status`` on a real response.""" + resp = MagicMock() + resp.status_code = status + return req.HTTPError(str(status), response=resp) + + +class TestIsTransientDownloadError: + def test_connection_errors_are_transient(self) -> None: + assert is_transient_download_error(req.ConnectionError("reset")) + assert is_transient_download_error(req.Timeout("timed out")) + assert is_transient_download_error( + req.exceptions.ChunkedEncodingError("dropped") + ) + assert is_transient_download_error( + req.exceptions.ContentDecodingError("gzip stream truncated") + ) + + def test_http_statuses(self) -> None: + assert not is_transient_download_error(_http_error(404)) + assert not is_transient_download_error(_http_error(403)) + assert is_transient_download_error(_http_error(429)) + assert is_transient_download_error(_http_error(503)) + + def test_http_error_without_response_is_permanent(self) -> None: + assert not is_transient_download_error(req.HTTPError("boom")) + + def test_hard_dns_failures_are_permanent(self) -> None: + """Hard resolution failures are permanent via both the cause chain + and MaxRetryError.reason.""" + from urllib3.exceptions import MaxRetryError, NameResolutionError + + gai = socket.gaierror(socket.EAI_NONAME, "nodename nor servname provided") + + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + assert not is_transient_download_error(chained) + + # The real urllib3 shape: gaierror on NameResolutionError.__cause__, + # carried by MaxRetryError.reason. + try: + raise NameResolutionError("example.invalid", None, gai) from gai + except NameResolutionError as nre: + wrapped = req.ConnectionError( + MaxRetryError(None, "http://example.invalid/", reason=nre) + ) + assert not is_transient_download_error(wrapped) + + # A garden-variety connection reset stays transient. + assert is_transient_download_error(req.ConnectionError("reset by peer")) + + def test_temporary_dns_failure_stays_transient(self) -> None: + """EAI_AGAIN (flaky resolver) stays retryable.""" + gai = socket.gaierror(socket.EAI_AGAIN, "temporary failure in name resolution") + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + + assert is_transient_download_error(chained) + + def test_implicit_context_does_not_reclassify(self) -> None: + """A gaierror riding along as implicit __context__ must not turn a + genuine connection reset permanent.""" + try: + try: + raise socket.gaierror(socket.EAI_NONAME, "first attempt") + except socket.gaierror: + raise req.ConnectionError("reset by peer") from None + except req.ConnectionError as reset: + assert reset.__context__ is not None + assert is_transient_download_error(reset) + + def test_gaierror_without_errno_stays_transient(self) -> None: + """A gaierror carrying no EAI code cannot prove a hard failure.""" + chained = req.ConnectionError("resolution failed") + chained.__cause__ = socket.gaierror("no errno") + + assert is_transient_download_error(chained) + + def test_mixed_chain_hard_failure_wins(self) -> None: + """EAI_AGAIN in the chain does not mask a hard failure elsewhere.""" + again = socket.gaierror(socket.EAI_AGAIN, "temporary failure") + hard = socket.gaierror(socket.EAI_NONAME, "unknown host") + + outer = req.ConnectionError(hard) + outer.__cause__ = again + assert not is_transient_download_error(outer) + + outer = req.ConnectionError(again) + outer.__cause__ = hard + assert not is_transient_download_error(outer) + + def test_dns_walk_survives_exception_cycles(self) -> None: + """A cyclic cause chain must terminate (and stay transient when no + resolution failure is present).""" + outer = req.ConnectionError("a") + inner = ValueError("b") + outer.__cause__ = inner + inner.__cause__ = outer + + assert is_transient_download_error(outer) + + def test_exhausted_resume_attempts_are_permanent(self) -> None: + """download_with_resume already spent its own resume attempts; its + EsphomeError wrapper is not retried again at the sweep level.""" + wrapped = EsphomeError("Failed to download after 3 attempts") + wrapped.__cause__ = req.ConnectionError("down") + assert not is_transient_download_error(wrapped) + + def test_unrelated_errors_are_permanent(self) -> None: + assert not is_transient_download_error(OSError("disk full")) + assert not is_transient_download_error(EsphomeError("size mismatch")) + + +class TestFetchWithRetry: + def test_logs_the_upcoming_attempt_number( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The warning names the attempt about to run, not the failed one.""" + with ( + patch("esphome.net_retry.time.sleep") as mock_sleep, + pytest.raises(req.ConnectionError), + ): + fetch_with_retry( + "https://example.com/f", + lambda: (_ for _ in ()).throw(req.ConnectionError("reset")), + ) + + assert mock_sleep.call_args_list == [call(2), call(4)] + assert "(attempt 2/3)" in caplog.text + assert "(attempt 3/3)" in caplog.text From 0d71ab8efbfcf4f7cf34d9267fab048c2692737c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:38:19 -0500 Subject: [PATCH 17/21] [api] Fix loop stall that triggers the task watchdog when entity list sends block (#18577) --- esphome/components/api/api_connection.cpp | 16 +- esphome/components/api/api_connection.h | 8 +- esphome/components/api/api_frame_helper.h | 2 +- esphome/components/api/list_entities.cpp | 10 +- esphome/components/web_server/list_entities.h | 1 - esphome/components/web_server/web_server.cpp | 10 +- .../web_server_idf/web_server_idf.cpp | 4 +- esphome/core/component_iterator.cpp | 25 ++- esphome/core/component_iterator.h | 43 +++- tests/components/camera/__init__.py | 11 + .../camera/test_component_iterator_camera.cpp | 79 +++++++ tests/components/core/benchmark.yaml | 11 + .../core/test_component_iterator.cpp | 195 ++++++++++++++++++ tests/integration/README.md | 2 + .../api_list_entities_backpressure.yaml | 23 +++ .../sndbuf_pin_component/__init__.py | 20 ++ .../sndbuf_pin_component.cpp | 55 +++++ .../sndbuf_pin_component.h | 21 ++ tests/integration/raw_api_client.py | 148 +++++++++++++ .../test_api_list_entities_backpressure.py | 110 ++++++++++ 20 files changed, 750 insertions(+), 44 deletions(-) create mode 100644 tests/components/camera/__init__.py create mode 100644 tests/components/camera/test_component_iterator_camera.cpp create mode 100644 tests/components/core/benchmark.yaml create mode 100644 tests/components/core/test_component_iterator.cpp create mode 100644 tests/integration/fixtures/api_list_entities_backpressure.yaml create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp create mode 100644 tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h create mode 100644 tests/integration/raw_api_client.py create mode 100644 tests/integration/test_api_list_entities_backpressure.py diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 73b4f3e5bd..6607e211da 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -417,15 +417,15 @@ void APIConnection::finalize_iterator_sync_() { } void APIConnection::process_iterator_batch_(ComponentIterator &iterator) { - size_t initial_size = this->deferred_batch_.size(); - size_t max_batch = MAX_INITIAL_PER_BATCH; - while (!iterator.completed() && (this->deferred_batch_.size() - initial_size) < max_batch) { - iterator.advance(); - } + // Budget by remaining batch capacity so a pass cannot overfill the batch; + // stops early on a refused send and resumes next loop pass + size_t batch_size = this->deferred_batch_.size(); + if (batch_size < MAX_INITIAL_BATCH_SIZE) + iterator.try_advance(MAX_INITIAL_BATCH_SIZE - batch_size); - // If the batch is full, process it immediately - // Note: iterator.advance() already calls schedule_batch_() via schedule_message_() - if (this->deferred_batch_.size() >= max_batch) { + // Flush immediately once enough is queued (not guaranteed every pass); + // partial batches go out via the batch timer or finalize_iterator_sync_() + if (this->deferred_batch_.size() >= MAX_INITIAL_BATCH_SIZE) { this->process_batch_(); } } diff --git a/esphome/components/api/api_connection.h b/esphome/components/api/api_connection.h index bb51a13000..1b47c23cfe 100644 --- a/esphome/components/api/api_connection.h +++ b/esphome/components/api/api_connection.h @@ -53,11 +53,11 @@ void log_dropped_message(const char *tag, int line, const LogString *what); // Keepalive timeout in milliseconds static constexpr uint32_t KEEPALIVE_TIMEOUT_MS = 60000; -// Maximum number of entities to process in a single batch during initial state/info sending -static constexpr size_t MAX_INITIAL_PER_BATCH = 34; +// Deferred batch size cap during initial state/info sync +static constexpr size_t MAX_INITIAL_BATCH_SIZE = 34; // Verify MAX_MESSAGES_PER_BATCH (defined in api_frame_helper.h) can hold the initial batch -static_assert(MAX_MESSAGES_PER_BATCH >= MAX_INITIAL_PER_BATCH, - "MAX_MESSAGES_PER_BATCH must be >= MAX_INITIAL_PER_BATCH"); +static_assert(MAX_MESSAGES_PER_BATCH >= MAX_INITIAL_BATCH_SIZE, + "MAX_MESSAGES_PER_BATCH must be >= MAX_INITIAL_BATCH_SIZE"); #ifdef USE_BENCHMARK class APIConnection; diff --git a/esphome/components/api/api_frame_helper.h b/esphome/components/api/api_frame_helper.h index 9c49956bbd..1c60bb87a5 100644 --- a/esphome/components/api/api_frame_helper.h +++ b/esphome/components/api/api_frame_helper.h @@ -36,7 +36,7 @@ static constexpr uint16_t MAX_MESSAGE_SIZE = 32768; // 32 KiB for ESP32 and oth static constexpr uint16_t RX_BUF_NULL_TERMINATOR = 1; // Maximum number of messages to batch in a single write operation -// Must be >= MAX_INITIAL_PER_BATCH in api_connection.h (enforced by static_assert there) +// Must be >= MAX_INITIAL_BATCH_SIZE in api_connection.h (enforced by static_assert there) static constexpr size_t MAX_MESSAGES_PER_BATCH = 34; // Max client name length (e.g., "Home Assistant 2026.1.0.dev0" = 28 chars) diff --git a/esphome/components/api/list_entities.cpp b/esphome/components/api/list_entities.cpp index f9e645b506..57ff616ca7 100644 --- a/esphome/components/api/list_entities.cpp +++ b/esphome/components/api/list_entities.cpp @@ -95,9 +95,17 @@ bool ListEntitiesIterator::on_end() { return this->client_->send_list_info_done( ListEntitiesIterator::ListEntitiesIterator(APIConnection *client) : client_(client) {} #ifdef USE_API_USER_DEFINED_ACTIONS +// Yield after every Nth service; bounds direct (non-batched) writes per loop pass +static constexpr uint8_t SERVICE_YIELD_INTERVAL = 3; + bool ListEntitiesIterator::on_service(UserServiceDescriptor *service) { auto resp = service->encode_list_service_response(); - return this->client_->send_message(resp); + if (!this->client_->send_message(resp)) + return false; + // at_ is this service's index + if ((this->at_ + 1) % SERVICE_YIELD_INTERVAL == 0) + this->yield_after_step_(); + return true; } #endif diff --git a/esphome/components/web_server/list_entities.h b/esphome/components/web_server/list_entities.h index 3edb84f555..dc32cbd1ad 100644 --- a/esphome/components/web_server/list_entities.h +++ b/esphome/components/web_server/list_entities.h @@ -35,7 +35,6 @@ class ListEntitiesIterator final : public ComponentIterator { #undef ENTITY_TYPE_ #undef ENTITY_CONTROLLER_TYPE_ // NOLINTEND(bugprone-macro-parentheses) - bool completed() { return this->state_ == IteratorState::NONE; } protected: const WebServer *web_server_; diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index 9e50b7a394..ec536910e5 100644 --- a/esphome/components/web_server/web_server.cpp +++ b/esphome/components/web_server/web_server.cpp @@ -214,8 +214,8 @@ void DeferredUpdateEventSource::process_deferred_queue_() { void DeferredUpdateEventSource::loop() { process_deferred_queue_(); - if (!this->entities_iterator_.completed()) - this->entities_iterator_.advance(); + // One step per loop; refusals retry next pass + this->entities_iterator_.try_advance(1); } void DeferredUpdateEventSource::deferrable_send_state(void *source, const char *event_type, @@ -321,12 +321,6 @@ void DeferredUpdateEventSourceList::on_client_connect_(DeferredUpdateEventSource #endif source->entities_iterator_.begin(ws->include_internal_); - - // just dump them all up-front and take advantage of the deferred queue - // on second thought that takes too long, but leaving the commented code here for debug purposes - // while(!source->entities_iterator_.completed()) { - // source->entities_iterator_.advance(); - //} }); } diff --git a/esphome/components/web_server_idf/web_server_idf.cpp b/esphome/components/web_server_idf/web_server_idf.cpp index 993fb6c035..9550570cdc 100644 --- a/esphome/components/web_server_idf/web_server_idf.cpp +++ b/esphome/components/web_server_idf/web_server_idf.cpp @@ -935,8 +935,8 @@ void AsyncEventSourceResponse::process_buffer_() { void AsyncEventSourceResponse::loop() { process_buffer_(); process_deferred_queue_(); - if (!this->entities_iterator_.completed()) - this->entities_iterator_.advance(); + // One step per loop; refusals retry next pass + this->entities_iterator_.try_advance(1); } bool AsyncEventSourceResponse::try_send_nodefer(const char *message, size_t message_len, const char *event, uint32_t id, diff --git a/esphome/core/component_iterator.cpp b/esphome/core/component_iterator.cpp index f4d3c05e19..3a497db741 100644 --- a/esphome/core/component_iterator.cpp +++ b/esphome/core/component_iterator.cpp @@ -22,23 +22,23 @@ void ComponentIterator::advance_platform_() { this->at_ = 0; } -void ComponentIterator::advance() { +bool ComponentIterator::advance_step_() { switch (this->state_) { case IteratorState::NONE: // not started - return; + return false; case IteratorState::BEGIN: if (this->on_begin()) { advance_platform_(); + return true; } - break; + return false; // Entity iterator cases (generated from entity_types.h) // NOLINTBEGIN(bugprone-macro-parentheses) #define ENTITY_TYPE_(type, singular, plural, count, upper) \ case IteratorState::upper: \ - this->process_platform_item_(App.get_##plural(), &ComponentIterator::on_##singular); \ - break; + return this->process_platform_item_(App.get_##plural(), &ComponentIterator::on_##singular); #define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ ENTITY_TYPE_(type, singular, plural, count, upper) #include "esphome/core/entity_types.h" @@ -48,26 +48,29 @@ void ComponentIterator::advance() { #ifdef USE_API_USER_DEFINED_ACTIONS case IteratorState::SERVICE: - this->process_platform_item_(api::global_api_server->get_user_services(), &ComponentIterator::on_service); - break; + return this->process_platform_item_(api::global_api_server->get_user_services(), &ComponentIterator::on_service); #endif #ifdef USE_CAMERA case IteratorState::CAMERA: { camera::Camera *camera_instance = camera::Camera::instance(); - if (camera_instance != nullptr && (!camera_instance->is_internal() || this->include_internal_)) { - this->on_camera(camera_instance); + if (camera_instance != nullptr && (!camera_instance->is_internal() || this->include_internal_) && + !this->on_camera(camera_instance)) { + return false; } advance_platform_(); - } break; + return true; + } #endif case IteratorState::MAX: if (this->on_end()) { this->state_ = IteratorState::NONE; + return true; } - return; + return false; } + return false; } bool ComponentIterator::on_end() { return true; } diff --git a/esphome/core/component_iterator.h b/esphome/core/component_iterator.h index d271fcfed0..fac09e9e14 100644 --- a/esphome/core/component_iterator.h +++ b/esphome/core/component_iterator.h @@ -30,7 +30,23 @@ class RadioFrequency; class ComponentIterator { public: void begin(bool include_internal = false); - void advance(); + /// Run up to max_steps iteration steps; stops early when iteration + /// completes or a callback refuses (that step is retried on the next + /// call). Inline so an idle (completed) iterator costs one compare, no call. + ESPHOME_ALWAYS_INLINE void try_advance(size_t max_steps) { + size_t steps = 0; + while (steps < max_steps && !this->completed()) { + this->yield_requested_ = false; + if (!this->advance_step_()) + break; + steps++; + if (this->yield_requested_) + break; + } + } + // Remove before 2027.3.0 + ESPDEPRECATED("Use try_advance() instead. Removed in 2027.3.0", "2026.8.1") + void advance() { this->try_advance(1); } bool completed() const { return this->state_ == IteratorState::NONE; } virtual bool on_begin(); // Pure virtual entity callbacks (generated from entity_types.h) @@ -73,23 +89,34 @@ class ComponentIterator { #endif MAX, }; + /// End the current try_advance() pass after this step; lets callbacks + /// that write directly to the socket cap direct writes per pass. + void yield_after_step_() { this->yield_requested_ = true; } + uint16_t at_{0}; // Supports up to 65,535 entities per type IteratorState state_{IteratorState::NONE}; - bool include_internal_{false}; + bool yield_requested_ : 1 {false}; + bool include_internal_ : 1 {false}; template - void process_platform_item_(const Container &items, + bool process_platform_item_(const Container &items, bool (ComponentIterator::*on_item)(typename Container::value_type)) { if (this->at_ >= items.size()) { this->advance_platform_(); - } else { - typename Container::value_type item = items[this->at_]; - if ((item->is_internal() && !this->include_internal_) || (this->*on_item)(item)) { - this->at_++; - } + return true; } + typename Container::value_type item = items[this->at_]; + if ((item->is_internal() && !this->include_internal_) || (this->*on_item)(item)) { + this->at_++; + return true; + } + return false; } + /// One iteration step; false if no progress was made (callback refused + /// or iterator not running). + bool advance_step_(); + void advance_platform_(); }; diff --git a/tests/components/camera/__init__.py b/tests/components/camera/__init__.py new file mode 100644 index 0000000000..61104b0bb1 --- /dev/null +++ b/tests/components/camera/__init__.py @@ -0,0 +1,11 @@ +import esphome.codegen as cg +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # No host camera platform exists to emit USE_CAMERA; define it here so + # the iterator CAMERA state compiles into the test binary. + async def to_code_testing(config): + cg.add_define("USE_CAMERA") + + manifest.to_code = to_code_testing diff --git a/tests/components/camera/test_component_iterator_camera.cpp b/tests/components/camera/test_component_iterator_camera.cpp new file mode 100644 index 0000000000..efd8261554 --- /dev/null +++ b/tests/components/camera/test_component_iterator_camera.cpp @@ -0,0 +1,79 @@ +#include + +#include "esphome/core/component_iterator.h" + +#ifdef USE_CAMERA +#include "esphome/components/camera/camera.h" + +namespace esphome::testing { + +class StubCamera : public camera::Camera { + public: + void add_listener(camera::CameraListener *listener) override {} + camera::CameraImageReader *create_image_reader() override { return nullptr; } + void request_image(camera::CameraRequester requester) override {} + void start_stream(camera::CameraRequester requester) override {} + void stop_stream(camera::CameraRequester requester) override {} +}; + +// Iterator that accepts everything except the camera, which can refuse a +// configurable number of times. The CAMERA state is a singleton path +// distinct from process_platform_item_; this pins the same contract: +// a refused camera is re-offered, never skipped. +class CameraRefusingIterator : public ComponentIterator { + public: +// NOLINTBEGIN(bugprone-macro-parentheses) +#define ENTITY_TYPE_(type, singular, plural, count, upper) \ + bool on_##singular(type *obj) override { return true; } +#define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ + ENTITY_TYPE_(type, singular, plural, count, upper) +#include "esphome/core/entity_types.h" +#undef ENTITY_TYPE_ +#undef ENTITY_CONTROLLER_TYPE_ + // NOLINTEND(bugprone-macro-parentheses) + + bool on_camera(camera::Camera *obj) override { + this->camera_calls++; + if (this->camera_refusals > 0) { + this->camera_refusals--; + return false; + } + return true; + } + + int camera_calls{0}; + int camera_refusals{0}; +}; + +// Far above the fixed number of iterator states +static constexpr size_t BIG_BUDGET = 1000; + +class ComponentIteratorCameraTest : public ::testing::Test { + protected: + void SetUp() override { + // Constructing a Camera installs the process-wide singleton + static StubCamera stub_camera; + ASSERT_EQ(camera::Camera::instance(), &stub_camera); + } +}; + +TEST_F(ComponentIteratorCameraTest, RefusedCameraIsReofferedNotSkipped) { + CameraRefusingIterator it; + it.camera_refusals = 2; + it.begin(); + // Runs until the camera refuses, which stops the pass + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.camera_calls, 1); + EXPECT_FALSE(it.completed()); + // The camera is re-offered once per call, not skipped + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.camera_calls, 2); + EXPECT_FALSE(it.completed()); + // Once accepted, the iteration completes + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.camera_calls, 3); +} + +} // namespace esphome::testing +#endif // USE_CAMERA diff --git a/tests/components/core/benchmark.yaml b/tests/components/core/benchmark.yaml new file mode 100644 index 0000000000..063ac41eff --- /dev/null +++ b/tests/components/core/benchmark.yaml @@ -0,0 +1,11 @@ +# Pulls in sensor so entity iteration paths compile (USE_SENSOR); +# tests register their own instances. Plain yaml.safe_load, no ESPHome tags. +# An alphabetically-earlier component's sensor: block shadows this one in +# combined builds; the tests' sensor-count ASSERT catches a capacity drop. +sensor: + - platform: template + id: bench_sensor_a + name: "Bench A" + - platform: template + id: bench_sensor_b + name: "Bench B" diff --git a/tests/components/core/test_component_iterator.cpp b/tests/components/core/test_component_iterator.cpp new file mode 100644 index 0000000000..03d467c920 --- /dev/null +++ b/tests/components/core/test_component_iterator.cpp @@ -0,0 +1,195 @@ +#include + +#include "esphome/core/component_iterator.h" + +#ifdef USE_SENSOR +#include "esphome/components/sensor/sensor.h" +#include "esphome/core/application.h" +#endif + +namespace esphome::testing { + +// Iterator whose begin/end callbacks can refuse a configurable number of +// times; all entity callbacks accept (any registered entities are accepted). +class RefusingIterator : public ComponentIterator { + public: +// NOLINTBEGIN(bugprone-macro-parentheses) +#define ENTITY_TYPE_(type, singular, plural, count, upper) \ + bool on_##singular(type *obj) override { return true; } +#define ENTITY_CONTROLLER_TYPE_(type, singular, plural, count, upper, callback) \ + ENTITY_TYPE_(type, singular, plural, count, upper) +#include "esphome/core/entity_types.h" +#undef ENTITY_TYPE_ +#undef ENTITY_CONTROLLER_TYPE_ + // NOLINTEND(bugprone-macro-parentheses) + + bool on_begin() override { return step(this->begin_calls, this->begin_refusals); } + bool on_end() override { return step(this->end_calls, this->end_refusals); } + + int begin_calls{0}; + int end_calls{0}; + int begin_refusals{0}; + int end_refusals{0}; + + protected: + static bool step(int &calls, int &refusals) { + calls++; + if (refusals > 0) { + refusals--; + return false; + } + return true; + } +}; + +// Far above the fixed number of iterator states +static constexpr size_t BIG_BUDGET = 1000; + +TEST(ComponentIterator, NotRunningMakesNoProgress) { + RefusingIterator it; + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 0); + EXPECT_EQ(it.end_calls, 0); +} + +TEST(ComponentIterator, CompletesInOneCallWithoutRefusals) { + RefusingIterator it; + it.begin(); + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 1); + EXPECT_EQ(it.end_calls, 1); +} + +TEST(ComponentIterator, StepBudgetIsHonored) { + RefusingIterator it; + it.begin(); + it.try_advance(1); + EXPECT_EQ(it.begin_calls, 1); + EXPECT_EQ(it.end_calls, 0); + EXPECT_FALSE(it.completed()); +} + +TEST(ComponentIterator, RefusedStepStopsBatchAndRetriesSameStep) { + RefusingIterator it; + it.end_refusals = 3; + it.begin(); + // First call runs until the refused end step, which stops the pass + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.end_calls, 1); + EXPECT_FALSE(it.completed()); + // The refused step is retried once per call, not skipped + it.try_advance(BIG_BUDGET); + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.end_calls, 3); + EXPECT_FALSE(it.completed()); + // Once accepted, the iteration completes + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.end_calls, 4); +} + +TEST(ComponentIterator, RefusedBeginStopsBatchAndRetries) { + RefusingIterator it; + it.begin_refusals = 2; + it.begin(); + it.try_advance(BIG_BUDGET); + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.begin_calls, 2); + EXPECT_FALSE(it.completed()); + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_EQ(it.begin_calls, 3); +} + +// The deprecated advance() wrapper must keep the legacy once-per-loop +// pattern working during the deprecation window. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +TEST(ComponentIterator, DeprecatedAdvanceKeepsLegacyPatternWorking) { + RefusingIterator it; + it.end_refusals = 2; + it.begin(); + size_t guard = 0; + while (!it.completed() && guard++ < BIG_BUDGET) { + it.advance(); + } + EXPECT_TRUE(it.completed()); + // Two refused end steps were retried, then accepted + EXPECT_EQ(it.end_calls, 3); +} +#pragma GCC diagnostic pop + +#ifdef USE_SENSOR +// Iterator whose sensor callback can refuse or yield; pins the per-item +// contract: a refused item is re-offered with at_ unchanged, never skipped. +class ItemRefusingIterator : public RefusingIterator { + public: + bool on_sensor(sensor::Sensor *obj) override { + this->last_sensor = obj; + if (!step(this->sensor_calls, this->sensor_refusals)) + return false; + if (this->yield_on_sensor) + this->yield_after_step_(); + return true; + } + sensor::Sensor *last_sensor{nullptr}; + int sensor_calls{0}; + int sensor_refusals{0}; + bool yield_on_sensor{false}; +}; + +class ComponentIteratorSensorTest : public ::testing::Test { + protected: + void SetUp() override { + static sensor::Sensor sensor_a; + static sensor::Sensor sensor_b; + static bool registered = false; + if (!registered) { + App.register_sensor(&sensor_a); + App.register_sensor(&sensor_b); + registered = true; + } + // StaticVector drops silently when full; fail the fixture, not the contract + ASSERT_EQ(App.get_sensors().size(), 2u) << "benchmark.yaml sensor count too small"; + } +}; + +TEST_F(ComponentIteratorSensorTest, RefusedItemIsReofferedNotSkipped) { + ItemRefusingIterator it; + it.sensor_refusals = 2; + it.begin(); + // Runs until the first sensor refuses + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 1); + EXPECT_FALSE(it.completed()); + // The refused item is re-offered, not skipped + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 2); + sensor::Sensor *refused = it.last_sensor; + // Once accepted, iteration continues through the second sensor to the end + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); + EXPECT_NE(it.last_sensor, refused); + EXPECT_EQ(it.sensor_calls, 4); +} + +TEST_F(ComponentIteratorSensorTest, YieldAfterStepEndsPassAndResumes) { + ItemRefusingIterator it; + it.yield_on_sensor = true; + it.begin(); + // The pass ends right after the first sensor despite a big budget + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 1); + EXPECT_FALSE(it.completed()); + // The next pass ends after the second sensor + it.try_advance(BIG_BUDGET); + EXPECT_EQ(it.sensor_calls, 2); + // Remaining states then run to completion in one pass + it.try_advance(BIG_BUDGET); + EXPECT_TRUE(it.completed()); +} +#endif // USE_SENSOR + +} // namespace esphome::testing diff --git a/tests/integration/README.md b/tests/integration/README.md index 44d9e0d644..790d9a3a11 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -7,6 +7,7 @@ This directory contains end-to-end integration tests for ESPHome, focusing on te - `conftest.py` - Common fixtures and utilities - `const.py` - Constants used throughout the integration tests - `types.py` - Type definitions for fixtures and functions +- `raw_api_client.py` - Minimal plaintext api client whose reads happen only on request (for backpressure tests) - `state_utils.py` - State handling utilities (e.g., `InitialStateHelper`, `find_entity`, `require_entity`) - `fixtures/` - YAML configuration files for tests - `test_*.py` - Individual test files @@ -347,6 +348,7 @@ Create C++ components in `fixtures/external_components/` for: - Custom entity behaviors - Scheduler testing - Memory management tests +- Deterministic network backpressure (`sndbuf_pin_component` pins socket send buffers; assert on its log line to prove the pin took effect) ##### Log Line Monitoring ```python diff --git a/tests/integration/fixtures/api_list_entities_backpressure.yaml b/tests/integration/fixtures/api_list_entities_backpressure.yaml new file mode 100644 index 0000000000..1e53af7b75 --- /dev/null +++ b/tests/integration/fixtures/api_list_entities_backpressure.yaml @@ -0,0 +1,23 @@ +esphome: + name: api-backpressure-test + +host: + +api: + # Smallest queue so a non-draining client blocks the send path quickly + max_send_queue: 1 + actions: +# GENERATED_ACTIONS + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + components: [sndbuf_pin_component] + +# Pins the device's socket send buffers for deterministic TCP backpressure +sndbuf_pin_component: + buffer_size: SERVER_SNDBUF + +logger: + level: DEBUG diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py b/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py new file mode 100644 index 0000000000..06dda5d1ac --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/__init__.py @@ -0,0 +1,20 @@ +import esphome.codegen as cg +import esphome.config_validation as cv +from esphome.const import CONF_BUFFER_SIZE, CONF_ID + +DEPENDENCIES = ["api"] + +sndbuf_pin_ns = cg.esphome_ns.namespace("sndbuf_pin") +SndbufPinComponent = sndbuf_pin_ns.class_("SndbufPinComponent", cg.Component) + +CONFIG_SCHEMA = cv.Schema( + { + cv.GenerateID(): cv.declare_id(SndbufPinComponent), + cv.Required(CONF_BUFFER_SIZE): cv.int_range(min=1), + } +).extend(cv.COMPONENT_SCHEMA) + + +async def to_code(config): + var = cg.new_Pvariable(config[CONF_ID], config[CONF_BUFFER_SIZE]) + await cg.register_component(var, config) diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp new file mode 100644 index 0000000000..430c5075c7 --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.cpp @@ -0,0 +1,55 @@ +#include "sndbuf_pin_component.h" + +#include +#include +#include + +#include "esphome/components/api/api_server.h" +#include "esphome/core/log.h" + +namespace esphome::sndbuf_pin { + +static const char *const TAG = "sndbuf_pin"; + +// Skip stdio; scan the low fd range where the listeners land +static constexpr int FIRST_USER_FD = 3; +static constexpr int MAX_FD_SCAN = 128; + +void SndbufPinComponent::setup() { + int pinned = 0; + for (int fd = FIRST_USER_FD; fd < MAX_FD_SCAN; fd++) { + int type = 0; + socklen_t len = sizeof(type); + if (::getsockopt(fd, SOL_SOCKET, SO_TYPE, &type, &len) != 0 || type != SOCK_STREAM) + continue; + struct sockaddr_in addr {}; + socklen_t addr_len = sizeof(addr); + if (::getsockname(fd, reinterpret_cast(&addr), &addr_len) != 0) { + ESP_LOGW(TAG, "fd %d: getsockname failed, errno %d", fd, errno); + continue; + } + if (ntohs(addr.sin_port) != api::global_api_server->get_port()) + continue; + if (::setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &this->buffer_size_, sizeof(this->buffer_size_)) != 0) { + ESP_LOGW(TAG, "fd %d: SO_SNDBUF pin failed, errno %d", fd, errno); + continue; + } + int applied = 0; + len = sizeof(applied); + if (::getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &applied, &len) != 0 || applied < this->buffer_size_) { + // Linux doubles the requested value; anything below it means clamped + ESP_LOGW(TAG, "fd %d: SO_SNDBUF readback %d below requested %d", fd, applied, this->buffer_size_); + continue; + } + // Tests assert on this line; accepted sockets inherit the pinned size + ESP_LOGD(TAG, "fd %d port %d: SO_SNDBUF pinned to %d (effective %d)", fd, ntohs(addr.sin_port), this->buffer_size_, + applied); + pinned++; + } + if (pinned == 0) { + ESP_LOGE(TAG, "api listener socket was not pinned"); + this->mark_failed(); + } +} + +} // namespace esphome::sndbuf_pin diff --git a/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h new file mode 100644 index 0000000000..b0af226d14 --- /dev/null +++ b/tests/integration/fixtures/external_components/sndbuf_pin_component/sndbuf_pin_component.h @@ -0,0 +1,21 @@ +#pragma once + +#include "esphome/core/component.h" + +namespace esphome::sndbuf_pin { + +// Test-only (host): pins SO_SNDBUF on every open TCP socket so integration +// tests get deterministic backpressure; an explicit SO_SNDBUF also disables +// kernel autotuning, and accepted sockets inherit it from the listener. +class SndbufPinComponent : public Component { + public: + explicit SndbufPinComponent(int buffer_size) : buffer_size_(buffer_size) {} + void setup() override; + // After the api server so its listening socket exists + float get_setup_priority() const override { return setup_priority::LATE; } + + protected: + int buffer_size_; +}; + +} // namespace esphome::sndbuf_pin diff --git a/tests/integration/raw_api_client.py b/tests/integration/raw_api_client.py new file mode 100644 index 0000000000..1dbe40933c --- /dev/null +++ b/tests/integration/raw_api_client.py @@ -0,0 +1,148 @@ +"""Minimal plaintext native-api client over a raw socket. + +Reads only when told to, so tests control when the TCP pipe backs up toward +the device; payloads are skipped and only message types are counted. +""" + +from __future__ import annotations + +import asyncio +from collections import Counter +import socket +from typing import Self + +from aioesphomeapi import api_pb2 +import aioesphomeapi.core as api_core +from google.protobuf import message + +from .const import LOCALHOST + +# Message type ids are protocol constants; derive them from aioesphomeapi so +# they cannot drift from the client library in use. +MESSAGE_TYPE_OF = {cls: num for num, cls in api_core.MESSAGE_TYPE_TO_PROTO.items()} + +_READ_CHUNK = 4096 + + +def encode_varint(value: int) -> bytes: + out = bytearray() + while True: + byte = value & 0x7F + value >>= 7 + if value: + out.append(byte | 0x80) + else: + out.append(byte) + return bytes(out) + + +def decode_varint(buf: bytearray, pos: int) -> tuple[int, int] | None: + """Decode one varint at pos; return (value, new_pos) or None if short.""" + value = shift = 0 + while pos < len(buf): + byte = buf[pos] + pos += 1 + value |= (byte & 0x7F) << shift + if not byte & 0x80: + return value, pos + shift += 7 + return None + + +def encode_frame(msg_type: int, payload: bytes) -> bytes: + """Encode one plaintext api frame: 0x00, payload length, message type.""" + return b"\x00" + encode_varint(len(payload)) + encode_varint(msg_type) + payload + + +class FrameParser: + """Incremental parser for the plaintext api frame stream.""" + + def __init__(self) -> None: + self._buf = bytearray() + + def feed(self, data: bytes) -> list[int]: + self._buf.extend(data) + types: list[int] = [] + while (msg_type := self._try_parse()) is not None: + types.append(msg_type) + return types + + def _try_parse(self) -> int | None: + buf = self._buf + if not buf: + return None + assert buf[0] == 0, f"expected plaintext frame, got indicator {buf[0]}" + if (size_decoded := decode_varint(buf, 1)) is None: + return None + size, pos = size_decoded + if (type_decoded := decode_varint(buf, pos)) is None: + return None + msg_type, pos = type_decoded + if len(buf) - pos < size: + return None + del buf[: pos + size] + return msg_type + + +class RawApiClient: + """Plaintext api client whose reads happen only on request.""" + + def __init__(self, port: int, recv_buffer_size: int | None = None) -> None: + self._port = port + self._parser = FrameParser() + self.bytes_received = 0 + self.frame_counts: Counter[int] = Counter() + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + if recv_buffer_size is not None: + sock.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, recv_buffer_size) + # Kernels may round up (Linux doubles) but must not clamp below + applied = sock.getsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF) + assert applied >= recv_buffer_size, ( + f"SO_RCVBUF clamped to {applied}, requested {recv_buffer_size}" + ) + sock.setblocking(False) + except Exception: + sock.close() + raise + self._sock = sock + + async def __aenter__(self) -> Self: + return self + + async def __aexit__(self, *exc_info: object) -> None: + self.close() + + async def connect(self, client_info: str = "raw-api-client") -> None: + """Connect and complete the Hello handshake (no auth step since 2026.1.0).""" + loop = asyncio.get_running_loop() + await loop.sock_connect(self._sock, (LOCALHOST, self._port)) + hello = api_pb2.HelloRequest() + hello.client_info = client_info + hello.api_version_major = 1 + hello.api_version_minor = 10 + await self.send_message(hello) + await self.read_until_frame(MESSAGE_TYPE_OF[api_pb2.HelloResponse]) + + async def send_message(self, msg: message.Message) -> None: + loop = asyncio.get_running_loop() + await loop.sock_sendall( + self._sock, + encode_frame(MESSAGE_TYPE_OF[type(msg)], msg.SerializeToString()), + ) + + async def read_until_frame(self, msg_type: int, timeout: float = 10.0) -> None: + """Read until at least one frame of msg_type has been received.""" + loop = asyncio.get_running_loop() + + async def _read_loop() -> None: + while not self.frame_counts[msg_type]: + data = await loop.sock_recv(self._sock, _READ_CHUNK) + assert data, "server closed the connection unexpectedly" + self.bytes_received += len(data) + self.frame_counts.update(self._parser.feed(data)) + + await asyncio.wait_for(_read_loop(), timeout) + + def close(self) -> None: + self._sock.close() diff --git a/tests/integration/test_api_list_entities_backpressure.py b/tests/integration/test_api_list_entities_backpressure.py new file mode 100644 index 0000000000..9df6c7a298 --- /dev/null +++ b/tests/integration/test_api_list_entities_backpressure.py @@ -0,0 +1,110 @@ +"""A client that stops reading the entity listing must not starve other clients. + +Service responses are sent directly (not via the deferred batch), so a full +TCP pipe makes the send path refuse; the drive loop now lives in +try_advance(), which stops on refusal instead of retrying forever. Not a +before/after regression test: pre-fix builds survive here because the +refusal path yields and pumps the socket each retry. + +The sndbuf_pin_component fixture pins the device's send buffers so the pipe +fills deterministically regardless of kernel autotuning; the test waits for +its log line before proceeding. +""" + +from __future__ import annotations + +import asyncio + +from aioesphomeapi import api_pb2 +import pytest + +from .raw_api_client import MESSAGE_TYPE_OF, RawApiClient +from .types import APIClientConnectedFactory, RunCompiledFunction + +SERVICES_RESPONSE = MESSAGE_TYPE_OF[api_pb2.ListEntitiesServicesResponse] +LIST_DONE_RESPONSE = MESSAGE_TYPE_OF[api_pb2.ListEntitiesDoneResponse] + +# Both ends of the pipe are pinned small; only tens of KB fit in the kernel +RECV_BUFFER_SIZE = 4096 +SERVER_SNDBUF = 8192 # substituted into the fixture yaml +# Logged by the sndbuf_pin_component fixture when it pins a socket +SNDBUF_PIN_LOG = "SO_SNDBUF pinned to" +# One response (~6.4 KB) must stay smaller than the pinned send buffer; an +# oversized message parks in the overflow buffer and reports as sent. +ARGS_PER_SERVICE = 8 +ARG_NAME_LEN = 800 +# ~160 KB listing versus a tens-of-KB pipe guarantees a mid-services block +NUM_SERVICES = 25 +assert ARGS_PER_SERVICE * ARG_NAME_LEN < SERVER_SNDBUF +# The pipe fills in well under a second +STALL_SECONDS = 0.5 +# Well above pipe capacity, well below the listing size +MIN_DRAINED_BYTES = 60_000 + + +def _generated_actions() -> str: + """Build the api actions block: services with long argument names.""" + lines: list[str] = [] + for i in range(NUM_SERVICES): + lines.append(f" - action: backpressure_service_{i:04d}") + lines.append(" variables:") + for j in range(ARGS_PER_SERVICE): + prefix = f"arg_{i:04d}_{j:02d}_" + lines.append( + f" {prefix}{'x' * (ARG_NAME_LEN - len(prefix))}: string" + ) + lines.append(" then:") + lines.append(" - logger.log: service called") + return "\n".join(lines) + + +@pytest.mark.asyncio +async def test_api_list_entities_backpressure( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, + unused_tcp_port: int, +) -> None: + """A stalled reader mid-services must not block other api clients.""" + assert "# GENERATED_ACTIONS" in yaml_config + config = yaml_config.replace("# GENERATED_ACTIONS", _generated_actions()) + config = config.replace("SERVER_SNDBUF", str(SERVER_SNDBUF)) + + pin_applied = asyncio.Event() + + def _on_log_line(line: str) -> None: + if SNDBUF_PIN_LOG in line: + pin_applied.set() + + async with run_compiled(config, line_callback=_on_log_line): + # Fails loudly if the pin never applied + await asyncio.wait_for(pin_applied.wait(), 10) + + async with RawApiClient( + unused_tcp_port, recv_buffer_size=RECV_BUFFER_SIZE + ) as stalled: + await stalled.connect(client_info="backpressure-stall-client") + await stalled.send_message(api_pb2.ListEntitiesRequest()) + # The client now stops reading entirely. + + # Let the server run against the full pipe + await asyncio.sleep(STALL_SECONDS) + + # Other clients must still be served while the first is blocked + async with api_client_connected(timeout=20) as client: + device_info = await asyncio.wait_for(client.device_info(), 20) + assert device_info.name == "api-backpressure-test" + _, services = await asyncio.wait_for( + client.list_entities_services(), 30 + ) + assert len(services) == NUM_SERVICES + + # Fixture-size guard: the listing must dwarf the pinned pipe + before = stalled.bytes_received + await stalled.read_until_frame(LIST_DONE_RESPONSE, timeout=60) + drained = stalled.bytes_received - before + assert drained > MIN_DRAINED_BYTES, ( + f"only {drained} bytes drained; the listing never backed up" + ) + assert stalled.frame_counts[SERVICES_RESPONSE] == NUM_SERVICES + assert stalled.frame_counts[LIST_DONE_RESPONSE] == 1 From 662bf7d7f05d7bbd585b9258af034f179c8a60ae Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 16:16:01 -0500 Subject: [PATCH 18/21] [esp32_ble_tracker] Scan at the default window while a GATT connection is active (#18609) --- .../components/ble_device_base/__init__.py | 43 +++++--- .../components/esp32_ble_tracker/__init__.py | 47 +++++++- .../esp32_ble_tracker/esp32_ble_tracker.cpp | 73 ++++++++++--- .../esp32_ble_tracker/esp32_ble_tracker.h | 36 ++++-- .../config/scan_window_explicit.yaml | 19 ++++ .../config/scan_window_raised.yaml | 17 +++ .../config/scan_window_scan_only.yaml | 12 ++ .../scan_window_user_set_scan_only.yaml | 14 +++ .../test_scan_window_default.py | 103 +++++++++++++++++- 9 files changed, 320 insertions(+), 44 deletions(-) create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml create mode 100644 tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml diff --git a/esphome/components/ble_device_base/__init__.py b/esphome/components/ble_device_base/__init__.py index 15a8b08139..43ec736727 100644 --- a/esphome/components/ble_device_base/__init__.py +++ b/esphome/components/ble_device_base/__init__.py @@ -206,32 +206,36 @@ def validate_scan_parameters(config: ConfigType) -> ConfigType: interval = config[CONF_INTERVAL] window = config[CONF_WINDOW] - if window > interval: - raise cv.Invalid( - f"Scan window ({window}) needs to be smaller than scan interval ({interval})" - ) + # Labels are reused in every error below; the optional one names its key. + windows = [("Scan window", window)] + if (connection_window := config.get(CONF_CONNECTION_SCAN_WINDOW)) is not None: + windows.append((CONF_CONNECTION_SCAN_WINDOW, connection_window)) + + for name, value in windows: + if value > interval: + raise cv.Invalid( + f"{name} ({value}) needs to be smaller than scan interval ({interval})" + ) # BLE scan interval/window are programmed in 0.625 ms units as a 16-bit value; the # controller only accepts 2.5 ms .. 10240 ms (0x0004 .. 0x4000). Reject out-of-range # values here instead of letting the unit conversion silently overflow. - for name, value in (("interval", interval), ("window", window)): + for name, value in (("Scan interval", interval), *windows): if value.total_microseconds < 2500 or value.total_microseconds > 10_240_000: - raise cv.Invalid( - f"Scan {name} ({value}) must be between 2.5 ms and 10240 ms" - ) + raise cv.Invalid(f"{name} ({value}) must be between 2.5 ms and 10240 ms") # Validate what actually reaches the controller: both values are truncated to # whole 0.625 ms units, so a window/interval pair that differs by less than one # unit collapses to the same value — silently programming a 100 % duty cycle # (radio permanently on) from a config that asked for less. interval_units = to_ble_units(interval) - window_units = to_ble_units(window) - if window_units == interval_units and window < interval: - raise cv.Invalid( - f"Scan window ({window}) and interval ({interval}) both truncate to " - f"{interval_units} x 0.625 ms, which the controller scans at a 100 % duty " - f"cycle. Separate them by at least 0.625 ms." - ) + for name, value in windows: + if to_ble_units(value) == interval_units and value < interval: + raise cv.Invalid( + f"{name} ({value}) and interval ({interval}) both truncate to " + f"{interval_units} x 0.625 ms, which the controller scans at a 100 % duty " + f"cycle. Separate them by at least 0.625 ms." + ) if interval.total_microseconds * 3 > duration.total_microseconds: raise cv.Invalid( @@ -247,11 +251,14 @@ def validate_scan_parameters(config: ConfigType) -> ConfigType: # their own; also the fallback for esp32's conditional default. DEFAULT_SCAN_WINDOW = "30ms" +CONF_CONNECTION_SCAN_WINDOW = "connection_scan_window" + def scan_parameters_schema( interval_default: str, *, window_default: str | Callable[[], TimePeriod] = DEFAULT_SCAN_WINDOW, + connection_window: bool = False, ) -> cv.All: """Build the scan_parameters value schema shared by all BLE trackers. @@ -263,7 +270,9 @@ def scan_parameters_schema( can adjust it once sibling keys are resolved). The `active` option (default on) is unconditional: active scanning is part of the tracker contract — every current proxy client assumes it, so a passive-only - tracker must not share this schema. + tracker must not share this schema. connection_window opts in to the + `connection_scan_window` option for trackers that can fall back to a + smaller window while a GATT connection is active. """ schema = { cv.Optional(CONF_DURATION, default="5min"): cv.positive_time_period_seconds, @@ -272,6 +281,8 @@ def scan_parameters_schema( cv.Optional(CONF_CONTINUOUS, default=True): cv.boolean, cv.Optional(CONF_ACTIVE, default=True): cv.boolean, } + if connection_window: + schema[cv.Optional(CONF_CONNECTION_SCAN_WINDOW)] = cv.positive_time_period return cv.All(cv.Schema(schema), validate_scan_parameters) diff --git a/esphome/components/esp32_ble_tracker/__init__.py b/esphome/components/esp32_ble_tracker/__init__.py index 28c8c7fcf1..55aa67c96d 100644 --- a/esphome/components/esp32_ble_tracker/__init__.py +++ b/esphome/components/esp32_ble_tracker/__init__.py @@ -7,6 +7,7 @@ import logging from esphome import automation import esphome.codegen as cg from esphome.components import ble_device_base, esp32_ble, ota +from esphome.components.ble_device_base import CONF_CONNECTION_SCAN_WINDOW from esphome.components.const import CONF_ON_SCAN_END, CONF_SCAN_PARAMETERS, CONF_WINDOW from esphome.components.esp32 import ( add_idf_sdkconfig_option, @@ -72,8 +73,9 @@ def _get_required_features() -> set[BLEFeatures]: # Slot counters sizing the tracker's StaticVector storage; one request per # registered listener or client. +CLIENT_COUNT_DEFINE = "ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT" _request_listener_slot = cg.slot_counter("ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT") -_request_client_slot = cg.slot_counter("ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT") +_request_client_slot = cg.slot_counter(CLIENT_COUNT_DEFINE) def register_ble_features(features: set[BLEFeatures]) -> None: @@ -146,6 +148,7 @@ class TrackerData: """Per-run validation state, namespaced under DOMAIN in CORE.data.""" scan_window_defaulted: bool = False + connection_window_injected: bool = False def _get_data() -> TrackerData: @@ -174,17 +177,34 @@ def _raise_defaulted_scan_window(config: ConfigType) -> ConfigType: honors the window strictly (>= 5.5.5); without the arbiter a full-duty scan would starve wifi outright, and a user-set window is never touched. Raising to the interval cannot invalidate the already-validated - parameters, so no re-validation is needed. + parameters, so no re-validation is needed. The connection window is + checked against the window here, after the raise. """ + params = config[CONF_SCAN_PARAMETERS] if ( _get_data().scan_window_defaulted and config.get(CONF_SOFTWARE_COEXISTENCE) and idf_version() >= IDF_SCAN_WINDOW_FIX_VERSION ): - params = config[CONF_SCAN_PARAMETERS] # Copy so the config dump shows a plain value instead of a YAML # anchor/alias pair pointing at the interval. params[CONF_WINDOW] = copy.copy(params[CONF_INTERVAL]) + # Arm the connection-time fallback unless the user set one. Injected + # after validation; safe because it equals the validated window default. + if CONF_CONNECTION_SCAN_WINDOW not in params: + params[CONF_CONNECTION_SCAN_WINDOW] = cv.positive_time_period( + ble_device_base.DEFAULT_SCAN_WINDOW + ) + _get_data().connection_window_injected = True + if ( + connection_window := params.get(CONF_CONNECTION_SCAN_WINDOW) + ) is not None and connection_window > params[CONF_WINDOW]: + # A larger value would widen the scan during connections. + raise cv.Invalid( + f"{CONF_CONNECTION_SCAN_WINDOW} ({connection_window}) needs to be " + f"smaller than the scan window ({params[CONF_WINDOW]})", + path=[CONF_SCAN_PARAMETERS, CONF_CONNECTION_SCAN_WINDOW], + ) return config @@ -193,7 +213,7 @@ def _raise_defaulted_scan_window(config: ConfigType) -> ConfigType: # window/interval pairs that collapse to the same 0.625 ms unit count. # The window default is conditional (see _scan_window_default above). SCAN_PARAMETERS_SCHEMA = ble_device_base.scan_parameters_schema( - "320ms", window_default=_scan_window_default + "320ms", window_default=_scan_window_default, connection_window=True ) # Codegen helpers are owned by ble_device_base; kept under the historical names @@ -287,6 +307,25 @@ async def to_code(config): cg.add(var.set_scan_duration(params[CONF_DURATION])) cg.add(var.set_scan_interval(ble_device_base.to_ble_units(params[CONF_INTERVAL]))) cg.add(var.set_scan_window(ble_device_base.to_ble_units(params[CONF_WINDOW]))) + if (connection_window := params.get(CONF_CONNECTION_SCAN_WINDOW)) is not None: + # Emitted at FINAL so a scan-only build, where the guarded C++ path + # compiles out, skips the call entirely. + window_units = ble_device_base.to_ble_units(connection_window) + + @coroutine_with_priority(CoroPriority.FINAL) + async def _emit_connection_scan_window() -> None: + if cg.get_slot_count(CLIENT_COUNT_DEFINE): + cg.add(var.set_connection_scan_window(window_units)) + elif not _get_data().connection_window_injected: + # Warn only for a user-set value; the injected default drops silently. + _LOGGER.warning( + "'%s' has no effect because this build has no BLE client " + "components (for example bluetooth_proxy with active " + "connections, or ble_client)", + CONF_CONNECTION_SCAN_WINDOW, + ) + + CORE.add_job(_emit_connection_scan_window) cg.add(var.set_scan_active(params[CONF_ACTIVE])) cg.add(var.set_scan_continuous(params[CONF_CONTINUOUS])) diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp index 798fd6e0ca..5339565a32 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp @@ -122,6 +122,9 @@ void ESP32BLETracker::loop() { // - start_scan_(): scanner_state_ becomes IDLE via set_scanner_state_() in cleanup_scan_state_() // - try_promote_discovered_clients_(): client enters DISCOVERED via set_state(), or // connecting client finishes (state change), or scanner reaches RUNNING/IDLE + // - connection-window restart: scan_params_ is only written in start_scan_() + // (which changes scanner state via set_scanner_state_()), and + // counts.active/disconnecting only change on client state changes // // All conditions that affect the logic below are tied to state changes that increment // state_version_, so the fast path is safe. @@ -144,6 +147,19 @@ void ESP32BLETracker::loop() { (this->scan_set_param_failed_ && this->scanner_state_ == ScannerState::RUNNING)) { this->handle_scanner_failure_(); } + +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // The programmed window no longer matches the connection state (typically + // the last connection dropped): restart so the right window applies now + // instead of at the end of the scan period. Continuous only (a user-started + // scan would not restart); !disconnecting matches the restart gate below. + if (this->scanner_state_ == ScannerState::RUNNING && this->scan_continuous_ && !counts.disconnecting && + this->scan_params_.scan_window != this->desired_scan_window_(counts.active)) { + // Same logical scan period continues: no on_scan_end sweeps for this + // restart. Only armed when the stop was issued. + this->skip_next_scan_end_ = this->stop_scan_(); + } +#endif /* Avoid starting the scanner if: @@ -195,19 +211,23 @@ void ESP32BLETracker::stop_scan() { // reason at D themselves, and the user-facing stop action is deliberate. ESP_LOGV(TAG, "Stopping scan."); this->scan_continuous_ = false; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // The window-change restart is abandoned with continuous scanning. + this->skip_next_scan_end_ = false; +#endif this->stop_scan_(); } void ESP32BLETracker::ble_before_disabled_event_handler() { this->stop_scan_(); } -void ESP32BLETracker::stop_scan_() { +bool ESP32BLETracker::stop_scan_() { if (this->scanner_state_ != ScannerState::RUNNING && this->scanner_state_ != ScannerState::FAILED) { // IDLE means there is nothing to stop; STOPPING means a stop is already in // flight and will finish on its own. Neither is an error. if (this->scanner_state_ != ScannerState::IDLE && this->scanner_state_ != ScannerState::STOPPING) { ESP_LOGE(TAG, "Cannot stop scan: %s", this->scanner_state_to_string_(this->scanner_state_)); } - return; + return false; } // Reset timeout state machine when stopping scan this->scan_timeout_state_ = ScanTimeoutState::INACTIVE; @@ -215,8 +235,9 @@ void ESP32BLETracker::stop_scan_() { esp_err_t err = esp_ble_gap_stop_scanning(); if (err != ESP_OK) { ESP_LOGE(TAG, "esp_ble_gap_stop_scanning failed: %d", err); - return; + return false; } + return true; } void ESP32BLETracker::start_scan_(bool first) { @@ -230,16 +251,11 @@ void ESP32BLETracker::start_scan_(bool first) { } this->set_scanner_state_(ScannerState::STARTING); ESP_LOGV(TAG, "Starting scan, set scanner state to STARTING."); - if (!first) { -#ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT - for (auto *listener : this->listeners_) - listener->on_scan_end(); + if (!first) + this->notify_scan_end_(); +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + this->skip_next_scan_end_ = false; #endif -#ifdef ESPHOME_BLE_DEVICE_BASE_LISTENER_COUNT - for (auto *listener : this->neutral_listeners_) - listener->on_scan_end(); -#endif - } #ifdef USE_ESP32_BLE_DEVICE this->discovered_log_.clear(); #endif @@ -247,7 +263,17 @@ void ESP32BLETracker::start_scan_(bool first) { this->scan_params_.own_addr_type = BLE_ADDR_TYPE_PUBLIC; this->scan_params_.scan_filter_policy = BLE_SCAN_FILTER_ALLOW_ALL; this->scan_params_.scan_interval = this->scan_interval_; - this->scan_params_.scan_window = this->scan_window_; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // Count fresh: an automation can start a scan before loop() refreshes the counts. + const uint32_t window = this->desired_scan_window_(this->count_client_states_().active); + if (window != this->scan_window_) { + // Guarantee the connection airtime instead of scanning wall to wall. + ESP_LOGV(TAG, "Connection active, using %" PRIu32 " unit scan window", window); + } +#else + const uint32_t window = this->scan_window_; +#endif + this->scan_params_.scan_window = window; // Start timeout monitoring in loop() instead of using scheduler // This prevents false reboots when the loop is blocked @@ -408,6 +434,11 @@ void ESP32BLETracker::dump_config() { " Continuous Scanning: %s", this->scan_duration_, this->scan_interval_ * 0.625f, this->scan_window_ * 0.625f, this->scan_active_ ? "ACTIVE" : "PASSIVE", YESNO(this->scan_continuous_)); +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + if (this->connection_scan_window_ != 0) { + ESP_LOGCONFIG(TAG, " Connection Scan Window: %.1f ms", this->connection_scan_window_ * 0.625f); + } +#endif ESP_LOGCONFIG(TAG, " Scanner State: %s\n" " Connecting: %d, discovered: %d, disconnecting: %d, active: %d", @@ -487,6 +518,18 @@ void ESP32BLETracker::cleanup_scan_state_(bool is_stop_complete) { // Reset timeout state machine instead of cancelling scheduler timeout this->scan_timeout_state_ = ScanTimeoutState::INACTIVE; + this->notify_scan_end_(); + + this->set_scanner_state_(ScannerState::IDLE); +} + +void ESP32BLETracker::notify_scan_end_() { +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // Window-change restart continues the same scan period; the flag stays set + // across the stop and is cleared by the restart in start_scan_. + if (this->skip_next_scan_end_) + return; +#endif #ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT for (auto *listener : this->listeners_) listener->on_scan_end(); @@ -495,8 +538,6 @@ void ESP32BLETracker::cleanup_scan_state_(bool is_stop_complete) { for (auto *listener : this->neutral_listeners_) listener->on_scan_end(); #endif - - this->set_scanner_state_(ScannerState::IDLE); } void ESP32BLETracker::handle_scanner_failure_() { @@ -534,6 +575,8 @@ void ESP32BLETracker::try_promote_discovered_clients_() { } ESP_LOGD(TAG, "Promoting client to connect"); + // A connect ends the scan period a window-change restart was continuing. + this->skip_next_scan_end_ = false; #ifdef USE_ESP32_BLE_SOFTWARE_COEXISTENCE this->update_coex_preference_(true); #endif diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h index 7c3e5538fd..618444e626 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h @@ -169,6 +169,9 @@ class ESP32BLETracker final : public Component, void set_scan_duration(uint32_t scan_duration) { scan_duration_ = scan_duration; } void set_scan_interval(uint32_t scan_interval) { scan_interval_ = scan_interval; } void set_scan_window(uint32_t scan_window) { scan_window_ = scan_window; } +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + void set_connection_scan_window(uint32_t scan_window) { connection_scan_window_ = scan_window; } +#endif void set_scan_active(bool scan_active) { scan_active_ = scan_active; } bool get_scan_active() const { return scan_active_; } void set_scan_continuous(bool scan_continuous) { scan_continuous_ = scan_continuous; } @@ -226,7 +229,10 @@ class ESP32BLETracker final : public Component, ScannerState get_scanner_state() const { return this->scanner_state_; } protected: - void stop_scan_(); + /// Returns true when a stop was issued to the controller. + bool stop_scan_(); + /// Fire on_scan_end on every listener unless a window-change restart suppressed it. + void notify_scan_end_(); /// Start a single scan by setting up the parameters and doing some esp-idf calls. void start_scan_(bool first); /// Called when a `ESP_GAP_BLE_SCAN_RESULT_EVT` event is received. @@ -313,6 +319,15 @@ class ESP32BLETracker final : public Component, uint32_t scan_duration_; uint32_t scan_interval_; uint32_t scan_window_; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + /// Window used while a GATT connection is active; set by the user, or + /// defaulted when the window was raised to full duty (0 = no fallback). + uint32_t connection_scan_window_{0}; + /// The window to scan at for the given number of active GATT connections. + uint32_t desired_scan_window_(uint8_t active) const { + return (this->connection_scan_window_ != 0 && active > 0) ? this->connection_scan_window_ : this->scan_window_; + } +#endif esp_bt_status_t scan_start_failed_{ESP_BT_STATUS_SUCCESS}; esp_bt_status_t scan_set_param_failed_{ESP_BT_STATUS_SUCCESS}; @@ -330,15 +345,20 @@ class ESP32BLETracker final : public Component, /// state_version_ to detect if any state changed since last iteration. uint8_t last_processed_version_{0}; ScannerState scanner_state_{ScannerState::IDLE}; - bool scan_continuous_; - bool scan_active_; + // Packed 1-bit flags. + bool scan_continuous_ : 1; + bool scan_active_ : 1; #ifdef USE_OTA_STATE_LISTENER - bool scan_continuous_before_ota_{false}; + bool scan_continuous_before_ota_ : 1 {false}; +#endif + bool ble_was_disabled_ : 1 {true}; + bool parse_advertisements_ : 1 {false}; +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + /// Suppress the window-change restart's on_scan_end sweeps (stop and start). + bool skip_next_scan_end_ : 1 {false}; #endif - bool ble_was_disabled_{true}; - bool parse_advertisements_{false}; #ifdef USE_ESP32_BLE_SOFTWARE_COEXISTENCE - bool coex_prefer_ble_{false}; + bool coex_prefer_ble_ : 1 {false}; #endif // Scan timeout state machine enum class ScanTimeoutState : uint8_t { @@ -346,10 +366,10 @@ class ESP32BLETracker final : public Component, MONITORING, // Actively monitoring for timeout EXCEEDED_WAIT, // Timeout exceeded, waiting one loop before reboot }; + ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; uint32_t scan_start_time_{0}; /// Precomputed timeout value: scan_duration_ * 2000 uint32_t scan_timeout_ms_{0}; - ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; }; // NOLINTNEXTLINE diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml new file mode 100644 index 0000000000..70760fdea6 --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_explicit.yaml @@ -0,0 +1,19 @@ +esphome: + name: scan-window-explicit + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + scan_parameters: + window: 30ms + +bluetooth_proxy: + active: true + +api: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml new file mode 100644 index 0000000000..4febbfcf3b --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_raised.yaml @@ -0,0 +1,17 @@ +esphome: + name: scan-window-raised + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + +bluetooth_proxy: + active: true + +api: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml new file mode 100644 index 0000000000..5da5601388 --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_scan_only.yaml @@ -0,0 +1,12 @@ +esphome: + name: scan-window-scan-only + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: diff --git a/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml b/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml new file mode 100644 index 0000000000..2b9093164f --- /dev/null +++ b/tests/component_tests/esp32_ble_tracker/config/scan_window_user_set_scan_only.yaml @@ -0,0 +1,14 @@ +esphome: + name: scan-window-user-scan-only + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + +esp32_ble_tracker: + scan_parameters: + connection_scan_window: 20ms diff --git a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py index 8a25f488fa..8612ac6732 100644 --- a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py +++ b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py @@ -12,11 +12,12 @@ arbiter a full-duty scan would starve wifi, so the 30 ms default is kept. from __future__ import annotations from collections.abc import Callable +from pathlib import Path import pytest from esphome import config_validation as cv -from esphome.components.ble_device_base import to_ble_units +from esphome.components.ble_device_base import CONF_CONNECTION_SCAN_WINDOW, to_ble_units from esphome.components.const import CONF_SCAN_PARAMETERS, CONF_WINDOW from esphome.components.esp32 import KEY_IDF_VERSION from esphome.components.esp32_ble_tracker import ( @@ -120,3 +121,103 @@ def test_short_interval_without_window_still_rejected( stage_esp32("5.5.5", wifi=True) with pytest.raises(cv.Invalid, match="needs to be smaller than scan interval"): _scan_params({"scan_parameters": {"interval": "20ms"}}) + + +# The connection-time fallback window: while a GATT connection is active the +# scanner drops from a raised full-duty window back to this value so the +# connection gets guaranteed airtime. + + +def test_raise_arms_connection_scan_window_default( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + params = _scan_params({}) + assert params[CONF_WINDOW] == params[CONF_INTERVAL] + assert to_ble_units(params[CONF_CONNECTION_SCAN_WINDOW]) == 48 + + +def test_user_connection_scan_window_survives_raise( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + params = _scan_params({"scan_parameters": {"connection_scan_window": "60ms"}}) + assert params[CONF_WINDOW] == params[CONF_INTERVAL] + assert to_ble_units(params[CONF_CONNECTION_SCAN_WINDOW]) == 96 + + +def test_unraised_window_gets_no_connection_scan_window_default( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.4", wifi=True) + assert CONF_CONNECTION_SCAN_WINDOW not in _scan_params({}) + + +def test_connection_scan_window_above_interval_rejected( + stage_esp32: Callable[..., None], +) -> None: + stage_esp32("5.5.5", wifi=True) + with pytest.raises( + cv.Invalid, match="connection_scan_window .* needs to be smaller" + ): + _scan_params({"scan_parameters": {"connection_scan_window": "400ms"}}) + + +def test_connection_scan_window_above_window_rejected( + stage_esp32: Callable[..., None], +) -> None: + """A connection window above the (post-raise) window would widen the scan + during connections; the reject runs after the raise so a fallback below a + raised window still validates (covered by the survives-raise test).""" + stage_esp32("5.5.5", wifi=True) + with pytest.raises( + cv.Invalid, match="connection_scan_window .* needs to be smaller" + ): + _scan_params( + {"scan_parameters": {"window": "30ms", "connection_scan_window": "300ms"}} + ) + + +def test_connection_scan_window_truncation_collapse_rejected( + stage_esp32: Callable[..., None], +) -> None: + """A connection window that truncates into the interval's 0.625 ms unit + would silently program a full-duty scan during connections.""" + stage_esp32("5.5.5", wifi=True) + with pytest.raises(cv.Invalid, match="connection_scan_window .* both truncate"): + _scan_params( + { + "scan_parameters": { + "interval": "320.5ms", + "connection_scan_window": "320.2ms", + } + } + ) + + +@pytest.mark.parametrize( + ("config_file", "window_call", "connection_call", "warns"), + [ + # Raised window with GATT clients: the injected fallback is emitted. + ("scan_window_raised.yaml", "set_scan_window(512)", True, False), + # Explicit window: nothing injected. + ("scan_window_explicit.yaml", "set_scan_window(48)", False, False), + # Scan-only build compiles the path out: the injected default is + # dropped silently, a user-set value warns. + ("scan_window_scan_only.yaml", "set_scan_window(512)", False, False), + ("scan_window_user_set_scan_only.yaml", "set_scan_window(512)", False, True), + ], +) +def test_connection_scan_window_codegen( + generate_main: Callable[[str | Path], str], + component_config_path: Callable[[str], Path], + caplog: pytest.LogCaptureFixture, + config_file: str, + window_call: str, + connection_call: bool, + warns: bool, +) -> None: + main_cpp = generate_main(component_config_path(config_file)) + assert window_call in main_cpp + assert ("set_connection_scan_window(48)" in main_cpp) == connection_call + assert ("'connection_scan_window' has no effect" in caplog.text) == warns From 68f3a6b9a56e73ca05e10d7a950e50208b900702 Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Mon, 24 Aug 2026 09:19:29 +1200 Subject: [PATCH 19/21] Bump version to 2026.8.1 --- Doxyfile | 2 +- esphome/const.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Doxyfile b/Doxyfile index ed0670621d..3d9a6f7221 100644 --- a/Doxyfile +++ b/Doxyfile @@ -48,7 +48,7 @@ PROJECT_NAME = ESPHome # could be handy for archiving the generated documentation or if some version # control system is used. -PROJECT_NUMBER = 2026.8.0 +PROJECT_NUMBER = 2026.8.1 # Using the PROJECT_BRIEF tag one can provide an optional one line description # for a project that appears at the top of each page and should give viewer a diff --git a/esphome/const.py b/esphome/const.py index 17ff1e17d9..53da67a4f4 100644 --- a/esphome/const.py +++ b/esphome/const.py @@ -4,7 +4,7 @@ from enum import Enum from esphome.enum import StrEnum -__version__ = "2026.8.0" +__version__ = "2026.8.1" ALLOWED_NAME_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789-_" VALID_SUBSTITUTIONS_CHARACTERS = ( From 76caadc594a7b1988406971ca3b3143f4e789ad5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" <3060199+jesserockz@users.noreply.github.com> Date: Sun, 23 Aug 2026 18:03:44 -0500 Subject: [PATCH 20/21] [noise] Add shared noise component and move api noise primitives (#18490) --- CODEOWNERS | 1 + esphome/components/api/__init__.py | 56 ++--- esphome/components/api/api_connection.cpp | 4 +- .../components/api/api_frame_helper_noise.cpp | 210 +++++------------- .../components/api/api_frame_helper_noise.h | 14 +- esphome/components/api/api_noise_context.h | 37 --- esphome/components/api/api_server.cpp | 2 +- esphome/components/api/api_server.h | 15 +- esphome/components/noise/__init__.py | 69 ++++++ esphome/components/noise/noise.cpp | 88 ++++++++ esphome/components/noise/noise.h | 74 ++++++ esphome/components/noise/noise_handshake.cpp | 139 ++++++++++++ esphome/components/noise/noise_handshake.h | 63 ++++++ esphome/core/defines.h | 1 + platformio.ini | 6 +- tests/benchmarks/components/api/__init__.py | 5 +- tests/benchmarks/components/noise/__init__.py | 7 + tests/component_tests/noise/__init__.py | 0 .../noise/test_encryption_key.py | 37 +++ tests/components/noise/__init__.py | 7 + tests/components/noise/common.yaml | 1 + tests/components/noise/test.esp32-idf.yaml | 2 + tests/components/noise/test.esp8266-ard.yaml | 2 + tests/components/noise/test.host.yaml | 2 + tests/components/noise/test.rp2040-ard.yaml | 2 + .../components/noise/test_noise_handshake.cpp | 199 +++++++++++++++++ .../noise/test_noise_primitives.cpp | 74 ++++++ 27 files changed, 870 insertions(+), 247 deletions(-) delete mode 100644 esphome/components/api/api_noise_context.h create mode 100644 esphome/components/noise/__init__.py create mode 100644 esphome/components/noise/noise.cpp create mode 100644 esphome/components/noise/noise.h create mode 100644 esphome/components/noise/noise_handshake.cpp create mode 100644 esphome/components/noise/noise_handshake.h create mode 100644 tests/benchmarks/components/noise/__init__.py create mode 100644 tests/component_tests/noise/__init__.py create mode 100644 tests/component_tests/noise/test_encryption_key.py create mode 100644 tests/components/noise/__init__.py create mode 100644 tests/components/noise/common.yaml create mode 100644 tests/components/noise/test.esp32-idf.yaml create mode 100644 tests/components/noise/test.esp8266-ard.yaml create mode 100644 tests/components/noise/test.host.yaml create mode 100644 tests/components/noise/test.rp2040-ard.yaml create mode 100644 tests/components/noise/test_noise_handshake.cpp create mode 100644 tests/components/noise/test_noise_primitives.cpp diff --git a/CODEOWNERS b/CODEOWNERS index 9ddbca5c71..3047072ea2 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -381,6 +381,7 @@ esphome/components/nextion/switch/* @senexcrenshaw esphome/components/nextion/text_sensor/* @senexcrenshaw esphome/components/nfc/* @jesserockz @kbx81 esphome/components/noblex/* @AGalfra +esphome/components/noise/* @esphome/core esphome/components/npi19/* @bakerkj esphome/components/nrf52/* @tomaszduda23 esphome/components/number/* @esphome/core diff --git a/esphome/components/api/__init__.py b/esphome/components/api/__init__.py index 0dc4b905bf..53ad0fe5d7 100644 --- a/esphome/components/api/__init__.py +++ b/esphome/components/api/__init__.py @@ -1,4 +1,3 @@ -import base64 import logging from typing import Any @@ -6,6 +5,15 @@ from esphome import automation from esphome.automation import Condition import esphome.codegen as cg from esphome.components.logger import request_log_listener + +# ENCRYPTION_SCHEMA and validate_encryption_key are re-exported for external +# components and downstream consumers that import them from api +from esphome.components.noise import ( # noqa: F401 + ENCRYPTION_SCHEMA, + decode_encryption_key, + encryption_schema, + validate_encryption_key, +) from esphome.config_helpers import get_logger_level import esphome.config_validation as cv from esphome.const import ( @@ -38,6 +46,10 @@ from esphome.core import CORE, ID, CoroPriority, EsphomeError, coroutine_with_pr from esphome.cpp_generator import MockObj, TemplateArgsType from esphome.types import ConfigFragmentType, ConfigType +# Compat alias: downstream consumers (e.g. device-builder) referenced the +# schema by its old private name before it moved to the noise component +_encryption_schema = encryption_schema + _LOGGER = logging.getLogger(__name__) DOMAIN = "api" @@ -46,9 +58,15 @@ CODEOWNERS = ["@esphome/core"] def AUTO_LOAD(config: ConfigType) -> list[str]: - """Conditionally auto-load json only when capture_response is used.""" + """Conditionally auto-load noise (encryption) and json (capture_response).""" base = ["socket"] + # A falsy config is a tooling probe for the maximal set (None from + # dependency resolution, {} from the components-graph platform probe); + # a validated config always carries defaults, never empty + if not config or CONF_ENCRYPTION in config: + base = base + ["noise"] + # Check if any homeassistant.action/homeassistant.service has capture_response: true # This flag is set during config validation in _validate_response_config if not config or CORE.data.get(DOMAIN, {}).get(CONF_CAPTURE_RESPONSE, False): @@ -130,20 +148,6 @@ def _register_provisioning_source(config: ConfigType) -> ConfigType: return config -def validate_encryption_key(value: Any) -> str: - value = cv.string_strict(value) - try: - decoded = base64.b64decode(value, validate=True) - except ValueError as err: - raise cv.Invalid("Invalid key format, please check it's using base64") from err - - if len(decoded) != 32: - raise cv.Invalid("Encryption key must be base64 and 32 bytes long") - - # Return original data for roundtrip conversion - return value - - CONF_SUPPORTS_RESPONSE = "supports_response" # Enum values in api::enums namespace @@ -250,18 +254,6 @@ ACTIONS_SCHEMA = automation.validate_automation( ), ) -ENCRYPTION_SCHEMA = cv.Schema( - { - cv.Optional(CONF_KEY): cv.sensitive(validate_encryption_key), - } -) - - -def _encryption_schema(config: ConfigType | None) -> ConfigType: - if config is None: - config = {} - return ENCRYPTION_SCHEMA(config) - def _consume_api_sockets(config: ConfigType) -> ConfigType: """Register socket needs for API component.""" @@ -297,7 +289,7 @@ CONFIG_SCHEMA = cv.All( CONF_SERVICES, group_of_exclusion=CONF_ACTIONS ): ACTIONS_SCHEMA, cv.Exclusive(CONF_ACTIONS, group_of_exclusion=CONF_ACTIONS): ACTIONS_SCHEMA, - cv.Optional(CONF_ENCRYPTION): _encryption_schema, + cv.Optional(CONF_ENCRYPTION): encryption_schema, cv.Optional(CONF_BATCH_DELAY, default="100ms"): cv.All( cv.positive_time_period_milliseconds, cv.Range(max=cv.TimePeriod(milliseconds=65535)), @@ -484,7 +476,7 @@ async def to_code(config: ConfigType) -> None: if (encryption_config := config.get(CONF_ENCRYPTION, None)) is not None: if key := encryption_config.get(CONF_KEY): - decoded = base64.b64decode(key) + decoded = decode_encryption_key(key) cg.add(var.set_noise_psk(list(decoded))) cg.add_define("USE_API_NOISE_PSK_FROM_YAML") else: @@ -498,10 +490,6 @@ async def to_code(config: ConfigType) -> None: # and plaintext disabled. Only a factory reset can remove it. cg.add_define("USE_API_PLAINTEXT") cg.add_define("USE_API_NOISE") - cg.add_library("esphome/noise-c", "0.1.21") - # Enable optimized memzero/memcmp in libsodium instead of volatile byte loops - cg.add_build_flag("-DHAVE_WEAK_SYMBOLS=1") - cg.add_build_flag("-DHAVE_INLINE_ASM=1") else: cg.add_define("USE_API_PLAINTEXT") diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index c1dded1271..91d13eed65 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -2130,7 +2130,7 @@ bool APIConnection::send_noise_encryption_set_key_response_(const NoiseEncryptio } #endif - psk_t psk{}; + noise::psk_t psk{}; if (msg.key_len == 0) { if (this->parent_->clear_noise_psk(true)) { resp.success = true; @@ -2139,7 +2139,7 @@ bool APIConnection::send_noise_encryption_set_key_response_(const NoiseEncryptio } } else if (base64_decode(msg.key, msg.key_len, psk.data(), psk.size()) != psk.size()) { ESP_LOGW(TAG, "Invalid encryption key length"); - } else if (APINoiseContext::is_all_zeros(psk)) { + } else if (noise::NoiseContext::is_all_zeros(psk)) { // Accepting the reserved provisioning PSK would report success without // enabling encryption (or silently clear an existing key) ESP_LOGW(TAG, "Rejecting all-zero encryption key"); diff --git a/esphome/components/api/api_frame_helper_noise.cpp b/esphome/components/api/api_frame_helper_noise.cpp index 09e3ca2b9e..d7554e62c5 100644 --- a/esphome/components/api/api_frame_helper_noise.cpp +++ b/esphome/components/api/api_frame_helper_noise.cpp @@ -2,9 +2,9 @@ #ifdef USE_API #ifdef USE_API_NOISE #include "api_connection.h" // For ClientInfo struct +#include "esphome/components/noise/noise.h" #include "esphome/core/application.h" #include "esphome/core/entity_base.h" -#include "esphome/core/hal.h" #include "esphome/core/helpers.h" #include "esphome/core/log.h" #include "proto.h" @@ -17,6 +17,14 @@ namespace esphome::api { +using noise::noise_err_to_logstr; + +// api_frame_helper.h keeps its own MAX_HANDSHAKE_SIZE because that header is +// also compiled in plaintext-only builds without the noise component; keep +// the two definitions from drifting apart. +static_assert(MAX_HANDSHAKE_SIZE == noise::MAX_HANDSHAKE_SIZE, + "api and noise component handshake size limits must match"); + static const char *const TAG = "api.noise"; #ifdef USE_ESP8266 static constexpr char PROLOGUE_INIT[] PROGMEM = "NoiseAPIInit"; @@ -51,45 +59,6 @@ static constexpr size_t API_MAX_LOG_BYTES = 168; #define LOG_PACKET_RECEIVED(buffer) ((void) 0) #endif -/// Convert a noise error code to a readable error -const LogString *noise_err_to_logstr(int err) { - if (err == NOISE_ERROR_NO_MEMORY) - return LOG_STR("NO_MEMORY"); - if (err == NOISE_ERROR_UNKNOWN_ID) - return LOG_STR("UNKNOWN_ID"); - if (err == NOISE_ERROR_UNKNOWN_NAME) - return LOG_STR("UNKNOWN_NAME"); - if (err == NOISE_ERROR_MAC_FAILURE) - return LOG_STR("MAC_FAILURE"); - if (err == NOISE_ERROR_NOT_APPLICABLE) - return LOG_STR("NOT_APPLICABLE"); - if (err == NOISE_ERROR_SYSTEM) - return LOG_STR("SYSTEM"); - if (err == NOISE_ERROR_REMOTE_KEY_REQUIRED) - return LOG_STR("REMOTE_KEY_REQUIRED"); - if (err == NOISE_ERROR_LOCAL_KEY_REQUIRED) - return LOG_STR("LOCAL_KEY_REQUIRED"); - if (err == NOISE_ERROR_PSK_REQUIRED) - return LOG_STR("PSK_REQUIRED"); - if (err == NOISE_ERROR_INVALID_LENGTH) - return LOG_STR("INVALID_LENGTH"); - if (err == NOISE_ERROR_INVALID_PARAM) - return LOG_STR("INVALID_PARAM"); - if (err == NOISE_ERROR_INVALID_STATE) - return LOG_STR("INVALID_STATE"); - if (err == NOISE_ERROR_INVALID_NONCE) - return LOG_STR("INVALID_NONCE"); - if (err == NOISE_ERROR_INVALID_PRIVATE_KEY) - return LOG_STR("INVALID_PRIVATE_KEY"); - if (err == NOISE_ERROR_INVALID_PUBLIC_KEY) - return LOG_STR("INVALID_PUBLIC_KEY"); - if (err == NOISE_ERROR_INVALID_FORMAT) - return LOG_STR("INVALID_FORMAT"); - if (err == NOISE_ERROR_INVALID_SIGNATURE) - return LOG_STR("INVALID_SIGNATURE"); - return LOG_STR("UNKNOWN"); -} - /// Initialize the frame helper, returns OK if successful. APIError APINoiseFrameHelper::init() { APIError err = init_common_(); @@ -194,9 +163,9 @@ APIError APINoiseFrameHelper::loop() { */ APIError APINoiseFrameHelper::try_read_frame_() { // read header - if (rx_header_buf_len_ < 3) { + if (rx_header_buf_len_ < noise::FRAME_HEADER_SIZE) { // no header information yet - uint8_t to_read = 3 - rx_header_buf_len_; + uint8_t to_read = static_cast(noise::FRAME_HEADER_SIZE) - rx_header_buf_len_; ssize_t received = this->socket_->read(&rx_header_buf_[rx_header_buf_len_], to_read); APIError err = handle_socket_read_result_(received); if (err != APIError::OK) { @@ -208,7 +177,7 @@ APIError APINoiseFrameHelper::try_read_frame_() { return APIError::WOULD_BLOCK; } - if (rx_header_buf_[0] != 0x01) { + if (rx_header_buf_[0] != noise::FRAME_INDICATOR) { state_ = State::FAILED; HELPER_LOG("Bad indicator byte %u", rx_header_buf_[0]); return APIError::BAD_INDICATOR; @@ -348,15 +317,15 @@ APIError APINoiseFrameHelper::state_action_server_hello_() { return APIError::OK; } APIError APINoiseFrameHelper::state_action_handshake_() { - int action = noise_handshakestate_get_action(this->handshake_); - if (action == NOISE_ACTION_READ_MESSAGE) { + noise::NoiseResponderHandshake::Action action = this->handshake_.action(); + if (action == noise::NoiseResponderHandshake::Action::ACTION_READ) { return this->state_action_handshake_read_(); - } else if (action == NOISE_ACTION_WRITE_MESSAGE) { + } else if (action == noise::NoiseResponderHandshake::Action::ACTION_WRITE) { return this->state_action_handshake_write_(); } // bad state for action this->state_ = State::FAILED; - HELPER_LOG("Bad action for handshake: %d", action); + HELPER_LOG("Bad action for handshake: %d", (int) action); return APIError::HANDSHAKESTATE_BAD_STATE; } APIError APINoiseFrameHelper::state_action_handshake_read_() { @@ -368,20 +337,16 @@ APIError APINoiseFrameHelper::state_action_handshake_read_() { if (this->rx_buf_.empty()) { this->send_explicit_handshake_reject_(LOG_STR("Empty handshake message")); return APIError::BAD_HANDSHAKE_ERROR_BYTE; - } else if (this->rx_buf_[0] != 0x00) { + } else if (this->rx_buf_[0] != noise::HANDSHAKE_STATUS_OK) { HELPER_LOG("Bad handshake error byte: %u", this->rx_buf_[0]); this->send_explicit_handshake_reject_(LOG_STR("Bad handshake error byte")); return APIError::BAD_HANDSHAKE_ERROR_BYTE; } - NoiseBuffer mbuf; - noise_buffer_init(mbuf); - noise_buffer_set_input(mbuf, this->rx_buf_.data() + 1, this->rx_buf_.size() - 1); - int err = noise_handshakestate_read_message(this->handshake_, &mbuf, nullptr); + int err = this->handshake_.read_message(this->rx_buf_.data() + 1, this->rx_buf_.size() - 1); if (err != 0) { // Special handling for MAC failure - this->send_explicit_handshake_reject_(err == NOISE_ERROR_MAC_FAILURE ? LOG_STR("Handshake MAC failure") - : LOG_STR("Handshake error")); + this->send_explicit_handshake_reject_(noise::reject_reason_for(err)); return this->handle_noise_error_(err, LOG_STR("noise_handshakestate_read_message"), APIError::HANDSHAKESTATE_READ_FAILED); } @@ -390,18 +355,16 @@ APIError APINoiseFrameHelper::state_action_handshake_read_() { } APIError APINoiseFrameHelper::state_action_handshake_write_() { uint8_t buffer[65]; - NoiseBuffer mbuf; - noise_buffer_init(mbuf); - noise_buffer_set_output(mbuf, buffer + 1, sizeof(buffer) - 1); + size_t msg_len = 0; - int err = noise_handshakestate_write_message(this->handshake_, &mbuf, nullptr); + int err = this->handshake_.write_message(buffer + 1, sizeof(buffer) - 1, msg_len); APIError aerr = this->handle_noise_error_(err, LOG_STR("noise_handshakestate_write_message"), APIError::HANDSHAKESTATE_WRITE_FAILED); if (aerr != APIError::OK) return aerr; - buffer[0] = 0x00; // success + buffer[0] = noise::HANDSHAKE_STATUS_OK; - aerr = this->write_frame_(buffer, mbuf.size + 1); + aerr = this->write_frame_(buffer, msg_len + 1); if (aerr != APIError::OK) return aerr; return this->check_handshake_finished_(); @@ -409,33 +372,22 @@ APIError APINoiseFrameHelper::state_action_handshake_write_() { void APINoiseFrameHelper::send_explicit_handshake_reject_(const LogString *reason) { // Max reject message: "Bad handshake packet len" (24) + 1 (failure byte) = 25 bytes uint8_t data[32]; - data[0] = 0x01; // failure - -#ifdef USE_STORE_LOG_STR_IN_FLASH - // On ESP8266 with flash strings, we need to use PROGMEM-aware functions - size_t reason_len = strlen_P(reinterpret_cast(reason)); - reason_len = std::min(reason_len, sizeof(data) - 1); - if (reason_len > 0) { - memcpy_P(data + 1, reinterpret_cast(reason), reason_len); - } -#else - // Normal memory access - const char *reason_str = LOG_STR_ARG(reason); - size_t reason_len = strlen(reason_str); - reason_len = std::min(reason_len, sizeof(data) - 1); - if (reason_len > 0) { - // NOLINTNEXTLINE(bugprone-not-null-terminated-result) - binary protocol, not a C string - std::memcpy(data + 1, reason_str, reason_len); - } -#endif - - size_t data_size = reason_len + 1; + static_assert(sizeof(data) >= noise::MAC_FAILURE_PAYLOAD_SIZE, + "reject buffer must fit the MAC failure wire contract"); + size_t data_size = noise::format_reject_payload(data, sizeof(data), reason); // temporarily remove failed state auto orig_state = state_; state_ = State::EXPLICIT_REJECT; - write_frame_(data, data_size); - state_ = orig_state; + APIError aerr = write_frame_(data, data_size); + if (aerr != APIError::OK) { + // Best effort; the reject reason is a diagnosis aid, not a protocol step + ESP_LOGW(TAG, "Sending handshake reject failed: %d", (int) aerr); + } + if (state_ == State::EXPLICIT_REJECT) { + // write_frame_ may have moved the state to FAILED; keep that decision + state_ = orig_state; + } } APIError APINoiseFrameHelper::read_packet(ReadPacketBuffer *buffer) { APIError aerr = this->check_data_state_(); @@ -492,12 +444,10 @@ APIError APINoiseFrameHelper::read_packet(ReadPacketBuffer *buffer) { // Returns APIError::OK on success. APIError APINoiseFrameHelper::encrypt_noise_message_(uint8_t *buf_start, uint16_t payload_size, uint8_t message_type, uint16_t &encrypted_len_out) { - // Write noise header - buf_start[0] = 0x01; // indicator - // buf_start[1], buf_start[2] to be set after encryption + // The noise frame header is written after encryption, when the size is known // Write message header (to be encrypted) - constexpr uint8_t msg_offset = 3; + constexpr uint8_t msg_offset = noise::FRAME_HEADER_SIZE; buf_start[msg_offset] = static_cast(message_type >> 8); // type high byte buf_start[msg_offset + 1] = static_cast(message_type); // type low byte buf_start[msg_offset + 2] = static_cast(payload_size >> 8); // data_len high byte @@ -515,11 +465,10 @@ APIError APINoiseFrameHelper::encrypt_noise_message_(uint8_t *buf_start, uint16_ if (aerr != APIError::OK) return aerr; - // Fill in the encrypted size - buf_start[1] = static_cast(mbuf.size >> 8); - buf_start[2] = static_cast(mbuf.size); + // Fill in the frame header now that the encrypted size is known + noise::write_frame_header(buf_start, static_cast(mbuf.size)); - encrypted_len_out = static_cast(3 + mbuf.size); // indicator + size + encrypted data + encrypted_len_out = static_cast(noise::FRAME_HEADER_SIZE + mbuf.size); return APIError::OK; } @@ -568,21 +517,19 @@ APIError APINoiseFrameHelper::write_protobuf_messages(ProtoWriteBuffer buffer, s } APIError APINoiseFrameHelper::write_frame_(const uint8_t *data, uint16_t len) { - uint8_t header[3]; - header[0] = 0x01; // indicator - header[1] = (uint8_t) (len >> 8); - header[2] = (uint8_t) len; + uint8_t header[noise::FRAME_HEADER_SIZE]; + noise::write_frame_header(header, len); if (len == 0) { - return this->write_raw_buf_(header, 3); + return this->write_raw_buf_(header, noise::FRAME_HEADER_SIZE); } struct iovec iov[2]; iov[0].iov_base = header; - iov[0].iov_len = 3; + iov[0].iov_len = noise::FRAME_HEADER_SIZE; iov[1].iov_base = const_cast(data); iov[1].iov_len = len; - return this->write_raw_iov_(iov, 2, 3 + len); + return this->write_raw_iov_(iov, 2, noise::FRAME_HEADER_SIZE + len); } /** Initiate the data structures for the handshake. @@ -590,45 +537,12 @@ APIError APINoiseFrameHelper::write_frame_(const uint8_t *data, uint16_t len) { * @return 0 on success, -1 on error (check errno) */ APIError APINoiseFrameHelper::init_handshake_() { - int err; - // Noise_NNpsk0_25519_ChaChaPoly_SHA256, built on the stack: - // noise_handshakestate_new_by_id copies it, so a member would waste - // 104 bytes per connection, and a static const would sit in RAM on - // ESP8266 (.rodata is DRAM there). - const NoiseProtocolId nid = { - .prefix_id = NOISE_PREFIX_STANDARD, - .pattern_id = NOISE_PATTERN_NN, - .modifier_ids = {NOISE_MODIFIER_PSK0}, - .dh_id = NOISE_DH_CURVE25519, - .cipher_id = NOISE_CIPHER_CHACHAPOLY, - .hash_id = NOISE_HASH_SHA256, - .hybrid_id = NOISE_DH_NONE, - }; - - err = noise_handshakestate_new_by_id(&handshake_, &nid, NOISE_ROLE_RESPONDER); - APIError aerr = - handle_noise_error_(err, LOG_STR("noise_handshakestate_new_by_id"), APIError::HANDSHAKESTATE_SETUP_FAILED); + int err = this->handshake_.init(this->ctx_.get_psk(), prologue_.data(), prologue_.size()); + APIError aerr = handle_noise_error_(err, LOG_STR("noise_handshake_init"), APIError::HANDSHAKESTATE_SETUP_FAILED); if (aerr != APIError::OK) return aerr; - - const auto &psk = this->ctx_.get_psk(); - err = noise_handshakestate_set_pre_shared_key(handshake_, psk.data(), psk.size()); - aerr = handle_noise_error_(err, LOG_STR("noise_handshakestate_set_pre_shared_key"), - APIError::HANDSHAKESTATE_SETUP_FAILED); - if (aerr != APIError::OK) - return aerr; - - err = noise_handshakestate_set_prologue(handshake_, prologue_.data(), prologue_.size()); - aerr = handle_noise_error_(err, LOG_STR("noise_handshakestate_set_prologue"), APIError::HANDSHAKESTATE_SETUP_FAILED); - if (aerr != APIError::OK) - return aerr; - // set_prologue copies it into handshakestate, so we can get rid of it now + // init copies the prologue into the handshakestate, so we can get rid of it now prologue_.release(); - - err = noise_handshakestate_start(handshake_); - aerr = handle_noise_error_(err, LOG_STR("noise_handshakestate_start"), APIError::HANDSHAKESTATE_SETUP_FAILED); - if (aerr != APIError::OK) - return aerr; return APIError::OK; } @@ -637,15 +551,17 @@ APIError APINoiseFrameHelper::check_handshake_finished_() { assert(state_ == State::HANDSHAKE); #endif - int action = noise_handshakestate_get_action(handshake_); - if (action == NOISE_ACTION_READ_MESSAGE || action == NOISE_ACTION_WRITE_MESSAGE) + noise::NoiseResponderHandshake::Action action = this->handshake_.action(); + if (action == noise::NoiseResponderHandshake::Action::ACTION_READ || + action == noise::NoiseResponderHandshake::Action::ACTION_WRITE) return APIError::OK; - if (action != NOISE_ACTION_SPLIT) { + if (action != noise::NoiseResponderHandshake::Action::ACTION_SPLIT) { state_ = State::FAILED; - HELPER_LOG("Bad action for handshake: %d", action); + HELPER_LOG("Bad action for handshake: %d", (int) action); return APIError::HANDSHAKESTATE_BAD_STATE; } - int err = noise_handshakestate_split(handshake_, &send_cipher_, &recv_cipher_); + // split() also frees the handshake state + int err = this->handshake_.split(send_cipher_, recv_cipher_); APIError aerr = handle_noise_error_(err, LOG_STR("noise_handshakestate_split"), APIError::HANDSHAKESTATE_SPLIT_FAILED); if (aerr != APIError::OK) @@ -654,17 +570,11 @@ APIError APINoiseFrameHelper::check_handshake_finished_() { this->frame_footer_size_ = noise_cipherstate_get_mac_length(send_cipher_); HELPER_LOG("Handshake complete!"); - noise_handshakestate_free(handshake_); - handshake_ = nullptr; state_ = State::DATA; return APIError::OK; } APINoiseFrameHelper::~APINoiseFrameHelper() { - if (handshake_ != nullptr) { - noise_handshakestate_free(handshake_); - handshake_ = nullptr; - } if (send_cipher_ != nullptr) { noise_cipherstate_free(send_cipher_); send_cipher_ = nullptr; @@ -675,16 +585,6 @@ APINoiseFrameHelper::~APINoiseFrameHelper() { } } -extern "C" { -// declare how noise generates random bytes (here with a good HWRNG based on the RF system) -void noise_rand_bytes(void *output, size_t len) { - if (!esphome::random_bytes(reinterpret_cast(output), len)) { - ESP_LOGE(TAG, "Acquiring random bytes failed; rebooting"); - arch_restart(); - } -} -} - } // namespace esphome::api #endif // USE_API_NOISE #endif // USE_API diff --git a/esphome/components/api/api_frame_helper_noise.h b/esphome/components/api/api_frame_helper_noise.h index 46bd366672..05060c77de 100644 --- a/esphome/components/api/api_frame_helper_noise.h +++ b/esphome/components/api/api_frame_helper_noise.h @@ -3,7 +3,7 @@ #ifdef USE_API #ifdef USE_API_NOISE #include "noise/protocol.h" -#include "api_noise_context.h" +#include "esphome/components/noise/noise_handshake.h" namespace esphome::api { @@ -14,9 +14,9 @@ class APINoiseFrameHelper final : public APIFrameHelper { // Pos 1-2: encrypted payload size (16-bit big-endian) // Pos 3-6: encrypted type (16-bit) + data_len (16-bit) // Pos 7+: actual payload data - static constexpr uint8_t HEADER_PADDING = 1 + 2 + 2 + 2; // indicator + size + type + data_len + static constexpr uint8_t HEADER_PADDING = noise::FRAME_HEADER_SIZE + 2 + 2; // frame header + type + data_len - APINoiseFrameHelper(std::unique_ptr socket, APINoiseContext &ctx) + APINoiseFrameHelper(std::unique_ptr socket, noise::NoiseContext &ctx) : APIFrameHelper(std::move(socket)), ctx_(ctx) { frame_header_padding_ = HEADER_PADDING; } @@ -52,13 +52,13 @@ class APINoiseFrameHelper final : public APIFrameHelper { APIError handle_handshake_frame_error_(APIError aerr); APIError handle_noise_error_(int err, const LogString *func_name, APIError api_err); - // Pointers first (4 bytes each) - NoiseHandshakeState *handshake_{nullptr}; + // Pointers first (4 bytes each; the handshake wrapper holds one pointer) + noise::NoiseResponderHandshake handshake_; NoiseCipherState *send_cipher_{nullptr}; NoiseCipherState *recv_cipher_{nullptr}; // Reference to noise context (4 bytes on 32-bit) - APINoiseContext &ctx_; + noise::NoiseContext &ctx_; // Buffer for noise handshake prologue (released after handshake) APIBuffer prologue_; @@ -67,7 +67,7 @@ class APINoiseFrameHelper final : public APIFrameHelper { // Fixed-size header buffer for noise protocol: // 1 byte for indicator + 2 bytes for message size (16-bit value, not varint) // Note: Maximum message size is UINT16_MAX (65535), with a limit of 128 bytes during handshake phase - uint8_t rx_header_buf_[3]; + uint8_t rx_header_buf_[noise::FRAME_HEADER_SIZE]; uint8_t rx_header_buf_len_ = 0; // 4 bytes total, no padding }; diff --git a/esphome/components/api/api_noise_context.h b/esphome/components/api/api_noise_context.h deleted file mode 100644 index 44484ffa2c..0000000000 --- a/esphome/components/api/api_noise_context.h +++ /dev/null @@ -1,37 +0,0 @@ -#pragma once -#include -#include -#include "esphome/core/defines.h" - -namespace esphome::api { - -#ifdef USE_API_NOISE -using psk_t = std::array; - -class APINoiseContext { - public: - // The all-zeros PSK is reserved: it marks the device as unprovisioned and - // doubles as the well-known provisioning PSK that unprovisioned devices - // accept for Noise handshakes (passive-sniffing protection only, no - // authentication). It is never a valid real key. - static bool is_all_zeros(const psk_t &psk) { - uint8_t acc = 0; - for (uint8_t b : psk) { - acc |= b; - } - return acc == 0; - } - void set_psk(psk_t psk) { - this->psk_ = psk; - this->has_psk_ = !is_all_zeros(psk); - } - const psk_t &get_psk() const { return this->psk_; } - bool has_psk() const { return this->has_psk_; } - - protected: - psk_t psk_{}; - bool has_psk_{false}; -}; -#endif // USE_API_NOISE - -} // namespace esphome::api diff --git a/esphome/components/api/api_server.cpp b/esphome/components/api/api_server.cpp index 2d5f9e4155..751f2e4c3b 100644 --- a/esphome/components/api/api_server.cpp +++ b/esphome/components/api/api_server.cpp @@ -588,7 +588,7 @@ bool APIServer::load_and_apply_noise_psk_() { return true; } -bool APIServer::save_noise_psk(psk_t psk, bool make_active) { +bool APIServer::save_noise_psk(noise::psk_t psk, bool make_active) { #ifdef USE_API_NOISE_PSK_FROM_YAML // When PSK is set from YAML, this function should never be called // but if it is, reject the change diff --git a/esphome/components/api/api_server.h b/esphome/components/api/api_server.h index a58e42534b..072a583901 100644 --- a/esphome/components/api/api_server.h +++ b/esphome/components/api/api_server.h @@ -5,7 +5,10 @@ #include "api_buffer.h" // Must precede clients_ so APIConnection is complete for default_delete (libc++). #include "api_connection.h" -#include "api_noise_context.h" +#ifdef USE_API_NOISE +// Only present in the build when the noise component is loaded +#include "esphome/components/noise/noise.h" +#endif #include "api_pb2.h" #include "api_pb2_service.h" #include "esphome/components/socket/socket.h" @@ -37,7 +40,7 @@ class UserServiceDescriptor; #ifdef USE_API_NOISE struct SavedNoisePsk { - psk_t psk; + noise::psk_t psk; } PACKED; // NOLINT #endif @@ -73,10 +76,10 @@ class APIServer final : public Component, APIBuffer &get_shared_buffer_ref() { return shared_write_buffer_; } #ifdef USE_API_NOISE - bool save_noise_psk(psk_t psk, bool make_active = true); + bool save_noise_psk(noise::psk_t psk, bool make_active = true); bool clear_noise_psk(bool make_active = true); - void set_noise_psk(psk_t psk) { this->noise_ctx_.set_psk(psk); } - APINoiseContext &get_noise_ctx() { return this->noise_ctx_; } + void set_noise_psk(noise::psk_t psk) { this->noise_ctx_.set_psk(psk); } + noise::NoiseContext &get_noise_ctx() { return this->noise_ctx_; } #endif // USE_API_NOISE void handle_disconnect(APIConnection *conn); @@ -354,7 +357,7 @@ class APIServer final : public Component, #endif #ifdef USE_API_NOISE - APINoiseContext noise_ctx_; + noise::NoiseContext noise_ctx_; ESPPreferenceObject noise_pref_; #endif // USE_API_NOISE }; diff --git a/esphome/components/noise/__init__.py b/esphome/components/noise/__init__.py new file mode 100644 index 0000000000..e5fcc94332 --- /dev/null +++ b/esphome/components/noise/__init__.py @@ -0,0 +1,69 @@ +import base64 +import binascii +from typing import Any + +import esphome.codegen as cg +import esphome.config_validation as cv +from esphome.const import CONF_KEY +from esphome.types import ConfigType + +CODEOWNERS = ["@esphome/core"] + +noise_ns = cg.esphome_ns.namespace("noise") + +CONFIG_SCHEMA = cv.Schema({}) + + +def validate_encryption_key(value: Any) -> str: + value = cv.string_strict(value) + try: + decoded = base64.b64decode(value, validate=True) + except ValueError as err: + raise cv.Invalid("Invalid key format, please check it's using base64") from err + + if len(decoded) != 32: + raise cv.Invalid("Encryption key must be base64 and 32 bytes long") + + # Return original data for roundtrip conversion + return value + + +def decode_encryption_key(value: str) -> bytes: + """Decode a base64 encryption key to its 32 raw bytes. + + a2b_base64 matches the decode the clients use (aioesphomeapi + decode_noise_psk), so both ends derive the same bytes. The length is + re-checked so a caller cannot turn an unvalidated short decode into a + zero-padded PSK. + """ + try: + decoded = binascii.a2b_base64(value) + except ValueError as err: + raise cv.Invalid("Invalid key format, please check it's using base64") from err + if len(decoded) != 32: + raise cv.Invalid("Encryption key must be base64 and 32 bytes long") + return decoded + + +ENCRYPTION_SCHEMA = cv.Schema( + { + cv.Optional(CONF_KEY): cv.sensitive(validate_encryption_key), + } +) + + +def encryption_schema(config: ConfigType | None) -> ConfigType: + # A bare `encryption:` block is valid; a missing key means the consumer + # falls back to its keyless behavior (api provisioning, ota inheriting + # the api key). + if config is None: + config = {} + return ENCRYPTION_SCHEMA(config) + + +async def to_code(config: ConfigType) -> None: + cg.add_define("USE_NOISE") + cg.add_library("esphome/noise-c", "0.1.21") + # Enable optimized memzero/memcmp in libsodium instead of volatile byte loops + cg.add_build_flag("-DHAVE_WEAK_SYMBOLS=1") + cg.add_build_flag("-DHAVE_INLINE_ASM=1") diff --git a/esphome/components/noise/noise.cpp b/esphome/components/noise/noise.cpp new file mode 100644 index 0000000000..95fab322db --- /dev/null +++ b/esphome/components/noise/noise.cpp @@ -0,0 +1,88 @@ +#include "noise.h" +#ifdef USE_NOISE +#include "esphome/core/log.h" + +#include +#include + +#include + +#ifdef USE_ESP8266 +#include +#endif + +namespace esphome::noise { + +static const char *const TAG = "noise"; + +const LogString *noise_err_to_logstr(int err) { + if (err == NOISE_ERROR_NO_MEMORY) + return LOG_STR("NO_MEMORY"); + if (err == NOISE_ERROR_UNKNOWN_ID) + return LOG_STR("UNKNOWN_ID"); + if (err == NOISE_ERROR_UNKNOWN_NAME) + return LOG_STR("UNKNOWN_NAME"); + if (err == NOISE_ERROR_MAC_FAILURE) + return LOG_STR("MAC_FAILURE"); + if (err == NOISE_ERROR_NOT_APPLICABLE) + return LOG_STR("NOT_APPLICABLE"); + if (err == NOISE_ERROR_SYSTEM) + return LOG_STR("SYSTEM"); + if (err == NOISE_ERROR_REMOTE_KEY_REQUIRED) + return LOG_STR("REMOTE_KEY_REQUIRED"); + if (err == NOISE_ERROR_LOCAL_KEY_REQUIRED) + return LOG_STR("LOCAL_KEY_REQUIRED"); + if (err == NOISE_ERROR_PSK_REQUIRED) + return LOG_STR("PSK_REQUIRED"); + if (err == NOISE_ERROR_INVALID_LENGTH) + return LOG_STR("INVALID_LENGTH"); + if (err == NOISE_ERROR_INVALID_PARAM) + return LOG_STR("INVALID_PARAM"); + if (err == NOISE_ERROR_INVALID_STATE) + return LOG_STR("INVALID_STATE"); + if (err == NOISE_ERROR_INVALID_NONCE) + return LOG_STR("INVALID_NONCE"); + if (err == NOISE_ERROR_INVALID_PRIVATE_KEY) + return LOG_STR("INVALID_PRIVATE_KEY"); + if (err == NOISE_ERROR_INVALID_PUBLIC_KEY) + return LOG_STR("INVALID_PUBLIC_KEY"); + if (err == NOISE_ERROR_INVALID_FORMAT) + return LOG_STR("INVALID_FORMAT"); + if (err == NOISE_ERROR_INVALID_SIGNATURE) + return LOG_STR("INVALID_SIGNATURE"); + return LOG_STR("UNKNOWN"); +} + +const LogString *reject_reason_for(int err) { + return err == NOISE_ERROR_MAC_FAILURE ? LOG_STR("Handshake MAC failure") : LOG_STR("Handshake error"); +} + +size_t format_reject_payload(uint8_t *buf, size_t capacity, const LogString *reason) { + if (capacity == 0) { + // A caller bug; the MAC_FAILURE_PAYLOAD_SIZE static_asserts at the call + // sites make this unreachable, kept as cheap memory safety + ESP_LOGVV(TAG, "Reject buffer has no capacity"); + return 0; + } + buf[0] = HANDSHAKE_STATUS_REJECT; +#ifdef USE_STORE_LOG_STR_IN_FLASH + // On ESP8266 with flash strings, we need to use PROGMEM-aware functions + size_t reason_len = strlen_P(reinterpret_cast(reason)); + reason_len = std::min(reason_len, capacity - 1); + if (reason_len > 0) { + memcpy_P(buf + 1, reinterpret_cast(reason), reason_len); + } +#else + const char *reason_str = LOG_STR_ARG(reason); + size_t reason_len = strlen(reason_str); + reason_len = std::min(reason_len, capacity - 1); + if (reason_len > 0) { + // NOLINTNEXTLINE(bugprone-not-null-terminated-result) - binary protocol, not a C string + std::memcpy(buf + 1, reason_str, reason_len); + } +#endif + return reason_len + 1; +} + +} // namespace esphome::noise +#endif // USE_NOISE diff --git a/esphome/components/noise/noise.h b/esphome/components/noise/noise.h new file mode 100644 index 0000000000..f9da8d35b8 --- /dev/null +++ b/esphome/components/noise/noise.h @@ -0,0 +1,74 @@ +#pragma once +#include "esphome/core/defines.h" +#ifdef USE_NOISE +#include +#include +#include +#include "esphome/core/log.h" + +namespace esphome::noise { + +using psk_t = std::array; + +class NoiseContext { + public: + // The all-zeros PSK is reserved: it marks the device as unprovisioned and + // doubles as the well-known provisioning PSK that unprovisioned devices + // accept for Noise handshakes (passive-sniffing protection only, no + // authentication). It is never a valid real key. + static bool is_all_zeros(const psk_t &psk) { + uint8_t acc = 0; + for (uint8_t b : psk) { + acc |= b; + } + return acc == 0; + } + void set_psk(psk_t psk) { + this->psk_ = psk; + this->has_psk_ = !is_all_zeros(psk); + } + const psk_t &get_psk() const { return this->psk_; } + bool has_psk() const { return this->has_psk_; } + + protected: + psk_t psk_{}; + bool has_psk_{false}; +}; + +/// Convert a noise error code to a readable error +const LogString *noise_err_to_logstr(int err); + +// Shared wire format for the noise transports (api and ota): every frame is +// FRAME_INDICATOR, a 16-bit big-endian payload length, then the payload. +// Handshake payloads start with a status byte; transport payloads end with +// the ChaCha20-Poly1305 MAC. +static constexpr uint8_t FRAME_INDICATOR = 0x01; +static constexpr size_t FRAME_HEADER_SIZE = 3; +static constexpr size_t MAC_SIZE = 16; +static constexpr size_t MAX_HANDSHAKE_SIZE = 128; +static constexpr uint8_t HANDSHAKE_STATUS_OK = 0x00; +static constexpr uint8_t HANDSHAKE_STATUS_REJECT = 0x01; + +inline void write_frame_header(uint8_t *buf, uint16_t payload_len) { + buf[0] = FRAME_INDICATOR; + buf[1] = (uint8_t) (payload_len >> 8); + buf[2] = (uint8_t) payload_len; +} + +/// Fill buf with a handshake reject payload (status byte plus the reason +/// text, PROGMEM aware); returns the payload length. buf needs capacity for +/// the status byte plus the truncated reason. +size_t format_reject_payload(uint8_t *buf, size_t capacity, const LogString *reason); + +/// Reject reason for a failed handshake read. The MAC failure string is a +/// wire contract: clients match it to report a wrong key. +const LogString *reject_reason_for(int err); + +/// Payload size of the MAC failure reject, the one reason string that is a +/// wire contract (sizeof's NUL stands in for the status byte). static_assert +/// reject buffers against this so a wrong key report can never truncate; +/// longer caller-supplied reasons are informational and sized by the caller. +static constexpr size_t MAC_FAILURE_PAYLOAD_SIZE = sizeof("Handshake MAC failure"); + +} // namespace esphome::noise +#endif // USE_NOISE diff --git a/esphome/components/noise/noise_handshake.cpp b/esphome/components/noise/noise_handshake.cpp new file mode 100644 index 0000000000..6d426de012 --- /dev/null +++ b/esphome/components/noise/noise_handshake.cpp @@ -0,0 +1,139 @@ +#include "noise_handshake.h" +#ifdef USE_NOISE +#include "esphome/core/hal.h" +#include "esphome/core/helpers.h" +#include "esphome/core/log.h" + +namespace esphome::noise { + +static const char *const TAG = "noise"; + +// Log the failing noise-c call at the same verbosity the api helper used +// before this class existed; callers only see one collapsed error code. +#define HANDSHAKE_STEP_LOG(func_name, err_code) \ + ESP_LOGVV(TAG, "%s failed: %s", LOG_STR_ARG(LOG_STR(func_name)), LOG_STR_ARG(noise_err_to_logstr(err_code))) + +NoiseResponderHandshake::~NoiseResponderHandshake() { + if (this->handshake_ != nullptr) { + noise_handshakestate_free(this->handshake_); + this->handshake_ = nullptr; + } +} + +int NoiseResponderHandshake::init(const psk_t &psk, const uint8_t *prologue, size_t prologue_len) { + if (this->handshake_ != nullptr) { + noise_handshakestate_free(this->handshake_); + this->handshake_ = nullptr; + } + // Noise_NNpsk0_25519_ChaChaPoly_SHA256, built on the stack: + // noise_handshakestate_new_by_id copies it, so a member would waste + // 104 bytes per connection, and a static const would sit in RAM on + // ESP8266 (.rodata is DRAM there). + const NoiseProtocolId nid = { + .prefix_id = NOISE_PREFIX_STANDARD, + .pattern_id = NOISE_PATTERN_NN, + .modifier_ids = {NOISE_MODIFIER_PSK0}, + .dh_id = NOISE_DH_CURVE25519, + .cipher_id = NOISE_CIPHER_CHACHAPOLY, + .hash_id = NOISE_HASH_SHA256, + .hybrid_id = NOISE_DH_NONE, + }; + + int err = noise_handshakestate_new_by_id(&this->handshake_, &nid, NOISE_ROLE_RESPONDER); + if (err != 0) { + HANDSHAKE_STEP_LOG("noise_handshakestate_new_by_id", err); + return err; + } + err = noise_handshakestate_set_pre_shared_key(this->handshake_, psk.data(), psk.size()); + if (err != 0) { + HANDSHAKE_STEP_LOG("noise_handshakestate_set_pre_shared_key", err); + return this->fail_init_(err); + } + err = noise_handshakestate_set_prologue(this->handshake_, prologue, prologue_len); + if (err != 0) { + HANDSHAKE_STEP_LOG("noise_handshakestate_set_prologue", err); + return this->fail_init_(err); + } + err = noise_handshakestate_start(this->handshake_); + if (err != 0) { + HANDSHAKE_STEP_LOG("noise_handshakestate_start", err); + return this->fail_init_(err); + } + return 0; +} + +/// Release a half-initialized state so a failed init() leaves the object as +/// if init() was never called. +int NoiseResponderHandshake::fail_init_(int err) { + noise_handshakestate_free(this->handshake_); + this->handshake_ = nullptr; + return err; +} + +NoiseResponderHandshake::Action NoiseResponderHandshake::action() const { + if (this->handshake_ == nullptr) { + // A caller bug: init() was never called, or split() already released the state + ESP_LOGVV(TAG, "action() on uninitialized or split handshake"); + return Action::ACTION_FAILED; + } + int raw = noise_handshakestate_get_action(this->handshake_); + switch (raw) { + case NOISE_ACTION_READ_MESSAGE: + return Action::ACTION_READ; + case NOISE_ACTION_WRITE_MESSAGE: + return Action::ACTION_WRITE; + case NOISE_ACTION_SPLIT: + return Action::ACTION_SPLIT; + default: + // Preserve the raw code in debug logs; callers only see the collapsed enum + ESP_LOGVV(TAG, "Unexpected noise action %d", raw); + return Action::ACTION_FAILED; + } +} + +int NoiseResponderHandshake::read_message(uint8_t *data, size_t len) { + NoiseBuffer mbuf; + noise_buffer_init(mbuf); + noise_buffer_set_input(mbuf, data, len); + return noise_handshakestate_read_message(this->handshake_, &mbuf, nullptr); +} + +int NoiseResponderHandshake::write_message(uint8_t *out, size_t capacity, size_t &out_len) { + out_len = 0; + NoiseBuffer mbuf; + noise_buffer_init(mbuf); + noise_buffer_set_output(mbuf, out, capacity); + int err = noise_handshakestate_write_message(this->handshake_, &mbuf, nullptr); + if (err == 0) + out_len = mbuf.size; + return err; +} + +int NoiseResponderHandshake::split(NoiseCipherState *&send_cipher, NoiseCipherState *&recv_cipher) { + // Defined error postcondition: noise-c leaves the out-params unwritten on + // its early error returns, so a caller passing uninitialized locals must + // never see garbage to free + send_cipher = nullptr; + recv_cipher = nullptr; + int err = noise_handshakestate_split(this->handshake_, &send_cipher, &recv_cipher); + if (err != 0) + return err; + noise_handshakestate_free(this->handshake_); + this->handshake_ = nullptr; + return 0; +} + +extern "C" { +// noise-c's only randomness source (the vendored library compiles no rand of +// its own); HWRNG backed. Lives in this TU so every handshake consumer links +// it and the definition can never be dropped from the archive. +void noise_rand_bytes(void *output, size_t len) { + if (!esphome::random_bytes(reinterpret_cast(output), len)) { + ESP_LOGE(TAG, "Acquiring random bytes failed; rebooting"); + arch_restart(); + } +} +} + +} // namespace esphome::noise +#endif // USE_NOISE diff --git a/esphome/components/noise/noise_handshake.h b/esphome/components/noise/noise_handshake.h new file mode 100644 index 0000000000..30596f35c2 --- /dev/null +++ b/esphome/components/noise/noise_handshake.h @@ -0,0 +1,63 @@ +#pragma once +#include "esphome/core/defines.h" +#ifdef USE_NOISE +#include +#include + +#include + +#include "noise.h" + +namespace esphome::noise { + +/** Sans-IO responder side of a Noise_NNpsk0_25519_ChaChaPoly_SHA256 handshake. + * + * Owns only the noise-c handshake state; the caller moves the raw handshake + * messages (no framing) over its own transport, driven by action(): + * read_message() while READ, write_message() while WRITE, then split() to + * take ownership of the transport ciphers. All methods return a noise-c + * error code, 0 on success. Called outside their action() step (before + * init(), after split()) the message methods return a noise-c error rather + * than crashing; the library checks its state argument. + * + * Methods are deliberately small separate functions so callers on tight + * stacks (RP2040 core0 scratch bank) never pay for more than one branch; + * the curve25519 step alone needs ~2KB of stack. + */ +class NoiseResponderHandshake { + public: + // The ACTION_ prefix is macro-collision safety: SDK headers #define bare + // names like READ/WRITE, and macros expand even inside an enum class. + enum class Action : uint8_t { ACTION_READ, ACTION_WRITE, ACTION_SPLIT, ACTION_FAILED }; + + NoiseResponderHandshake() = default; + ~NoiseResponderHandshake(); + // Owns a raw noise-c handshake state; copying would double free it + NoiseResponderHandshake(const NoiseResponderHandshake &) = delete; + NoiseResponderHandshake &operator=(const NoiseResponderHandshake &) = delete; + + /// Create and start the handshake with the given PSK and prologue. A + /// repeated call frees the previous handshake state and starts over. + [[nodiscard]] int init(const psk_t &psk, const uint8_t *prologue, size_t prologue_len); + /// ACTION_FAILED is the catch-all: returned before init(), after split() + /// has released the state, and when noise-c reports a failed handshake. + [[nodiscard]] Action action() const; + /// Process one received handshake message. The buffer is consumed in + /// place: noise-c decrypts into it and zeroes it before returning. + [[nodiscard]] int read_message(uint8_t *data, size_t len); + /// Produce the next handshake message into out; out_len receives its size + /// and is zero on error. + [[nodiscard]] int write_message(uint8_t *out, size_t capacity, size_t &out_len); + /// Hand out the transport ciphers and free the handshake state. The caller + /// owns both cipher states and must free them with noise_cipherstate_free(); + /// both are set to nullptr on error. + [[nodiscard]] int split(NoiseCipherState *&send_cipher, NoiseCipherState *&recv_cipher); + + protected: + int fail_init_(int err); + + NoiseHandshakeState *handshake_{nullptr}; +}; + +} // namespace esphome::noise +#endif // USE_NOISE diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 20aca3776f..5f34437145 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -222,6 +222,7 @@ #define API_MAX_SEND_QUEUE 8 #define MAX_API_CONNECTIONS 6 #define USE_MD5 +#define USE_NOISE #define USE_SHA256 #ifndef USE_RP2 // no MQTT backend or esp_wireguard library on RP2 #define USE_MQTT diff --git a/platformio.ini b/platformio.ini index 4c372cc0bb..b2a36e687c 100644 --- a/platformio.ini +++ b/platformio.ini @@ -45,7 +45,7 @@ lib_deps_base = lib_deps = ${common.lib_deps_base} https://github.com/dudanov/MideaUART.git#eeea6c3e9b4474f067054592b435be1c4e466815 ; midea - esphome/noise-c@0.1.21 ; api + esphome/noise-c@0.1.21 ; noise (api, ota) improv/Improv@1.2.6 ; improv_serial / esp32_improv kikuchan98/pngle@1.1.0 ; online_image ; Using the repository directly, otherwise ESP-IDF can't use the library @@ -244,7 +244,7 @@ lib_deps = ${common:idf-component-libs.lib_deps} ESP32Async/ESPAsyncWebServer@3.9.6 ; web_server_base droscy/esp_wireguard@0.4.5 ; wireguard - esphome/noise-c@0.1.21 ; api + esphome/noise-c@0.1.21 ; noise (api, ota) ESP32Async/AsyncTCP@3.4.5 ; async_tcp DNSServer ; captive_portal heman/AsyncMqttClient-esphome@2.0.0 ; mqtt @@ -641,7 +641,7 @@ build_unflags = extends = common platform = platformio/native lib_deps = - esphome/noise-c@0.1.21 ; used by api + esphome/noise-c@0.1.21 ; used by noise (api, ota) lvgl/lvgl@9.5.0 ; lvgl build_flags = ${common.build_flags} diff --git a/tests/benchmarks/components/api/__init__.py b/tests/benchmarks/components/api/__init__.py index 0565bc5330..12c078911c 100644 --- a/tests/benchmarks/components/api/__init__.py +++ b/tests/benchmarks/components/api/__init__.py @@ -3,8 +3,9 @@ from tests.testing_helpers import ComponentManifestOverride def override_manifest(manifest: ComponentManifestOverride) -> None: - # api must run its to_code to define USE_API, USE_API_PLAINTEXT, - # and add the noise-c library dependency. + # api must run its to_code to define USE_API and USE_API_NOISE. The + # AUTO_LOADed noise component runs its own to_code via the override in + # tests/benchmarks/components/noise/__init__.py. manifest.enable_codegen() original_to_code = manifest.to_code diff --git a/tests/benchmarks/components/noise/__init__.py b/tests/benchmarks/components/noise/__init__.py new file mode 100644 index 0000000000..f430e9dd36 --- /dev/null +++ b/tests/benchmarks/components/noise/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # to_code must run: it defines USE_NOISE and adds the noise-c library + # the api benchmark sources need. + manifest.enable_codegen() diff --git a/tests/component_tests/noise/__init__.py b/tests/component_tests/noise/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/component_tests/noise/test_encryption_key.py b/tests/component_tests/noise/test_encryption_key.py new file mode 100644 index 0000000000..62abae6487 --- /dev/null +++ b/tests/component_tests/noise/test_encryption_key.py @@ -0,0 +1,37 @@ +"""Tests for the shared noise encryption key helpers.""" + +from __future__ import annotations + +import pytest + +from esphome import config_validation as cv +from esphome.components.noise import decode_encryption_key, validate_encryption_key + +KEY = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" + + +def test_validate_encryption_key_roundtrips() -> None: + assert validate_encryption_key(KEY) == KEY + + +@pytest.mark.parametrize("value", ["not-base64!!!", "AAECAw=="]) +def test_validate_encryption_key_rejects_bad_input(value: str) -> None: + with pytest.raises(cv.Invalid): + validate_encryption_key(value) + + +def test_decode_encryption_key_returns_32_bytes() -> None: + assert decode_encryption_key(KEY) == bytes(range(32)) + + +def test_decode_encryption_key_rejects_invalid_base64() -> None: + """The shared helper raises cv.Invalid, not binascii.Error.""" + with pytest.raises(cv.Invalid, match="base64"): + decode_encryption_key("A") + + +def test_decode_encryption_key_rejects_short_decode() -> None: + """a2b_base64 stops at embedded padding; a short decode must not become + a zero padded PSK on the device.""" + with pytest.raises(cv.Invalid, match="32 bytes"): + decode_encryption_key("AAECAw==") diff --git a/tests/components/noise/__init__.py b/tests/components/noise/__init__.py new file mode 100644 index 0000000000..60a5740a83 --- /dev/null +++ b/tests/components/noise/__init__.py @@ -0,0 +1,7 @@ +from tests.testing_helpers import ComponentManifestOverride + + +def override_manifest(manifest: ComponentManifestOverride) -> None: + # to_code must run: it defines USE_NOISE and adds the noise-c library + # the component sources under test need. + manifest.enable_codegen() diff --git a/tests/components/noise/common.yaml b/tests/components/noise/common.yaml new file mode 100644 index 0000000000..35253a35e6 --- /dev/null +++ b/tests/components/noise/common.yaml @@ -0,0 +1 @@ +noise: diff --git a/tests/components/noise/test.esp32-idf.yaml b/tests/components/noise/test.esp32-idf.yaml new file mode 100644 index 0000000000..550ffd1f88 --- /dev/null +++ b/tests/components/noise/test.esp32-idf.yaml @@ -0,0 +1,2 @@ +packages: + noise: !include common.yaml diff --git a/tests/components/noise/test.esp8266-ard.yaml b/tests/components/noise/test.esp8266-ard.yaml new file mode 100644 index 0000000000..550ffd1f88 --- /dev/null +++ b/tests/components/noise/test.esp8266-ard.yaml @@ -0,0 +1,2 @@ +packages: + noise: !include common.yaml diff --git a/tests/components/noise/test.host.yaml b/tests/components/noise/test.host.yaml new file mode 100644 index 0000000000..550ffd1f88 --- /dev/null +++ b/tests/components/noise/test.host.yaml @@ -0,0 +1,2 @@ +packages: + noise: !include common.yaml diff --git a/tests/components/noise/test.rp2040-ard.yaml b/tests/components/noise/test.rp2040-ard.yaml new file mode 100644 index 0000000000..550ffd1f88 --- /dev/null +++ b/tests/components/noise/test.rp2040-ard.yaml @@ -0,0 +1,2 @@ +packages: + noise: !include common.yaml diff --git a/tests/components/noise/test_noise_handshake.cpp b/tests/components/noise/test_noise_handshake.cpp new file mode 100644 index 0000000000..d879a26c43 --- /dev/null +++ b/tests/components/noise/test_noise_handshake.cpp @@ -0,0 +1,199 @@ +#include + +#include + +#include + +#include "esphome/components/noise/noise.h" +#include "esphome/components/noise/noise_handshake.h" + +namespace esphome::noise::testing { + +using Action = NoiseResponderHandshake::Action; + +// A raw noise-c initiator driving the same Noise_NNpsk0_25519_ChaChaPoly_SHA256 +// pattern the responder class implements, so the tests exercise a real +// two-message handshake rather than mirrored calls into the class under test. +class Initiator { + public: + Initiator(const psk_t &psk, const uint8_t *prologue, size_t prologue_len) { + const NoiseProtocolId nid = { + .prefix_id = NOISE_PREFIX_STANDARD, + .pattern_id = NOISE_PATTERN_NN, + .modifier_ids = {NOISE_MODIFIER_PSK0}, + .dh_id = NOISE_DH_CURVE25519, + .cipher_id = NOISE_CIPHER_CHACHAPOLY, + .hash_id = NOISE_HASH_SHA256, + .hybrid_id = NOISE_DH_NONE, + }; + EXPECT_EQ(noise_handshakestate_new_by_id(&this->state_, &nid, NOISE_ROLE_INITIATOR), 0); + EXPECT_EQ(noise_handshakestate_set_pre_shared_key(this->state_, psk.data(), psk.size()), 0); + EXPECT_EQ(noise_handshakestate_set_prologue(this->state_, prologue, prologue_len), 0); + EXPECT_EQ(noise_handshakestate_start(this->state_), 0); + } + ~Initiator() { + if (this->state_ != nullptr) + noise_handshakestate_free(this->state_); + if (this->send_ != nullptr) + noise_cipherstate_free(this->send_); + if (this->recv_ != nullptr) + noise_cipherstate_free(this->recv_); + } + Initiator(const Initiator &) = delete; + Initiator &operator=(const Initiator &) = delete; + + size_t write_message(uint8_t *out, size_t capacity) { + NoiseBuffer mbuf; + noise_buffer_init(mbuf); + noise_buffer_set_output(mbuf, out, capacity); + EXPECT_EQ(noise_handshakestate_write_message(this->state_, &mbuf, nullptr), 0); + return mbuf.size; + } + + int read_message(uint8_t *data, size_t len) { + NoiseBuffer mbuf; + noise_buffer_init(mbuf); + noise_buffer_set_input(mbuf, data, len); + return noise_handshakestate_read_message(this->state_, &mbuf, nullptr); + } + + void split() { EXPECT_EQ(noise_handshakestate_split(this->state_, &this->send_, &this->recv_), 0); } + + NoiseCipherState *send_{nullptr}; + NoiseCipherState *recv_{nullptr}; + + private: + NoiseHandshakeState *state_{nullptr}; +}; + +static const uint8_t PROLOGUE[] = {'t', 'e', 's', 't', 'p', 'r', 'o', 'l', 'o', 'g', 'u', 'e'}; + +static psk_t make_psk(uint8_t seed) { + psk_t psk; + for (size_t i = 0; i < psk.size(); i++) { + psk[i] = static_cast(seed + i); + } + return psk; +} + +TEST(NoiseResponderHandshakeTest, ActionFailedBeforeInit) { + NoiseResponderHandshake handshake; + EXPECT_EQ(handshake.action(), Action::ACTION_FAILED); +} + +TEST(NoiseResponderHandshakeTest, MessageMethodsErrorBeforeInit) { + // The class doc promises a noise-c error, not a crash, when the message + // methods run outside their action() step; pin the library's null check + NoiseResponderHandshake handshake; + uint8_t buf[MAX_HANDSHAKE_SIZE] = {}; + size_t out_len = 0; + EXPECT_NE(handshake.read_message(buf, sizeof(buf)), 0); + EXPECT_NE(handshake.write_message(buf, sizeof(buf), out_len), 0); + // Deliberately non-null: split() documents a nullptr postcondition on + // error, so a caller's uninitialized locals never hold garbage to free + auto *sentinel = reinterpret_cast(0x1); + NoiseCipherState *send_cipher = sentinel; + NoiseCipherState *recv_cipher = sentinel; + EXPECT_NE(handshake.split(send_cipher, recv_cipher), 0); + EXPECT_EQ(send_cipher, nullptr); + EXPECT_EQ(recv_cipher, nullptr); +} + +TEST(NoiseResponderHandshakeTest, FullHandshakeAndTransportRoundTrip) { + const psk_t psk = make_psk(7); + NoiseResponderHandshake responder; + ASSERT_EQ(responder.init(psk, PROLOGUE, sizeof(PROLOGUE)), 0); + EXPECT_EQ(responder.action(), Action::ACTION_READ); + + Initiator initiator(psk, PROLOGUE, sizeof(PROLOGUE)); + uint8_t msg[MAX_HANDSHAKE_SIZE]; + size_t msg_len = initiator.write_message(msg, sizeof(msg)); + ASSERT_GT(msg_len, 0u); + + ASSERT_EQ(responder.read_message(msg, msg_len), 0); + ASSERT_EQ(responder.action(), Action::ACTION_WRITE); + + size_t reply_len = 0; + ASSERT_EQ(responder.write_message(msg, sizeof(msg), reply_len), 0); + ASSERT_GT(reply_len, 0u); + ASSERT_EQ(responder.action(), Action::ACTION_SPLIT); + + ASSERT_EQ(initiator.read_message(msg, reply_len), 0); + initiator.split(); + + NoiseCipherState *send_cipher = nullptr; + NoiseCipherState *recv_cipher = nullptr; + ASSERT_EQ(responder.split(send_cipher, recv_cipher), 0); + ASSERT_NE(send_cipher, nullptr); + ASSERT_NE(recv_cipher, nullptr); + // The handshake state is released by split(); the class reports FAILED after + EXPECT_EQ(responder.action(), Action::ACTION_FAILED); + EXPECT_EQ(static_cast(noise_cipherstate_get_mac_length(send_cipher)), MAC_SIZE); + + // Responder encrypts, initiator decrypts + uint8_t frame[64]; + static constexpr char PLAINTEXT[] = "encrypted ota"; + std::memcpy(frame, PLAINTEXT, sizeof(PLAINTEXT)); + NoiseBuffer mbuf; + noise_buffer_init(mbuf); + noise_buffer_set_inout(mbuf, frame, sizeof(PLAINTEXT), sizeof(frame)); + ASSERT_EQ(noise_cipherstate_encrypt(send_cipher, &mbuf), 0); + EXPECT_EQ(mbuf.size, sizeof(PLAINTEXT) + MAC_SIZE); + + noise_buffer_set_inout(mbuf, frame, mbuf.size, sizeof(frame)); + ASSERT_EQ(noise_cipherstate_decrypt(initiator.recv_, &mbuf), 0); + ASSERT_EQ(mbuf.size, sizeof(PLAINTEXT)); + EXPECT_EQ(std::memcmp(frame, PLAINTEXT, sizeof(PLAINTEXT)), 0); + + noise_cipherstate_free(send_cipher); + noise_cipherstate_free(recv_cipher); +} + +TEST(NoiseResponderHandshakeTest, ReInitRestartsHandshake) { + // The documented retry shape: a repeated init() frees the previous state + // and starts over. The first message under the new key authenticating + // proves the restart took effect; the old state surviving would fail the + // MAC here. + NoiseResponderHandshake responder; + ASSERT_EQ(responder.init(make_psk(7), PROLOGUE, sizeof(PROLOGUE)), 0); + ASSERT_EQ(responder.init(make_psk(9), PROLOGUE, sizeof(PROLOGUE)), 0); + EXPECT_EQ(responder.action(), Action::ACTION_READ); + + Initiator initiator(make_psk(9), PROLOGUE, sizeof(PROLOGUE)); + uint8_t msg[MAX_HANDSHAKE_SIZE]; + size_t msg_len = initiator.write_message(msg, sizeof(msg)); + ASSERT_GT(msg_len, 0u); + EXPECT_EQ(responder.read_message(msg, msg_len), 0); +} + +TEST(NoiseResponderHandshakeTest, WrongPskFailsWithMacFailure) { + NoiseResponderHandshake responder; + ASSERT_EQ(responder.init(make_psk(7), PROLOGUE, sizeof(PROLOGUE)), 0); + + Initiator initiator(make_psk(200), PROLOGUE, sizeof(PROLOGUE)); + uint8_t msg[MAX_HANDSHAKE_SIZE]; + size_t msg_len = initiator.write_message(msg, sizeof(msg)); + ASSERT_GT(msg_len, 0u); + + int err = responder.read_message(msg, msg_len); + EXPECT_EQ(err, NOISE_ERROR_MAC_FAILURE); + EXPECT_EQ(responder.action(), Action::ACTION_FAILED); +} + +TEST(NoiseResponderHandshakeTest, MismatchedPrologueFailsWithMacFailure) { + // The prologue binds the plaintext preamble for downgrade resistance; a + // tampered preamble must fail even with the right key. + const psk_t psk = make_psk(7); + NoiseResponderHandshake responder; + ASSERT_EQ(responder.init(psk, PROLOGUE, sizeof(PROLOGUE)), 0); + + static const uint8_t TAMPERED[] = {'x'}; + Initiator initiator(psk, TAMPERED, sizeof(TAMPERED)); + uint8_t msg[MAX_HANDSHAKE_SIZE]; + size_t msg_len = initiator.write_message(msg, sizeof(msg)); + ASSERT_GT(msg_len, 0u); + + EXPECT_EQ(responder.read_message(msg, msg_len), NOISE_ERROR_MAC_FAILURE); +} + +} // namespace esphome::noise::testing diff --git a/tests/components/noise/test_noise_primitives.cpp b/tests/components/noise/test_noise_primitives.cpp new file mode 100644 index 0000000000..018be9f717 --- /dev/null +++ b/tests/components/noise/test_noise_primitives.cpp @@ -0,0 +1,74 @@ +#include + +#include + +#include + +#include "esphome/components/noise/noise.h" + +namespace esphome::noise::testing { + +TEST(NoiseContextTest, AllZerosPskIsReserved) { + psk_t zeros{}; + EXPECT_TRUE(NoiseContext::is_all_zeros(zeros)); + + psk_t psk{}; + psk[31] = 1; + EXPECT_FALSE(NoiseContext::is_all_zeros(psk)); + + NoiseContext ctx; + EXPECT_FALSE(ctx.has_psk()); + ctx.set_psk(zeros); + EXPECT_FALSE(ctx.has_psk()); + ctx.set_psk(psk); + EXPECT_TRUE(ctx.has_psk()); + EXPECT_EQ(ctx.get_psk(), psk); +} + +TEST(WireFormatTest, FrameHeaderIsIndicatorPlusBigEndianLength) { + uint8_t header[FRAME_HEADER_SIZE]; + write_frame_header(header, 0x1234); + EXPECT_EQ(header[0], FRAME_INDICATOR); + EXPECT_EQ(header[1], 0x12); + EXPECT_EQ(header[2], 0x34); +} + +TEST(WireFormatTest, RejectPayloadCarriesStatusByteAndMacFailureContract) { + // The MAC failure string is a wire contract: clients match it to report a + // wrong key. Format the payload exactly the way the handshake read path does. + uint8_t buf[64]; + size_t len = format_reject_payload(buf, sizeof(buf), reject_reason_for(NOISE_ERROR_MAC_FAILURE)); + static constexpr char EXPECTED[] = "Handshake MAC failure"; + ASSERT_EQ(len, 1 + strlen(EXPECTED)); + EXPECT_EQ(buf[0], HANDSHAKE_STATUS_REJECT); + EXPECT_EQ(memcmp(buf + 1, EXPECTED, strlen(EXPECTED)), 0); + // The exported floor covers the full MAC failure payload exactly + EXPECT_EQ(MAC_FAILURE_PAYLOAD_SIZE, 1 + strlen(EXPECTED)); + + // Any other error maps to the generic reason + len = format_reject_payload(buf, sizeof(buf), reject_reason_for(NOISE_ERROR_INVALID_STATE)); + static constexpr char GENERIC[] = "Handshake error"; + ASSERT_EQ(len, 1 + strlen(GENERIC)); + EXPECT_EQ(memcmp(buf + 1, GENERIC, strlen(GENERIC)), 0); +} + +TEST(WireFormatTest, RejectPayloadTruncatesToCapacity) { + uint8_t buf[8]; + size_t len = format_reject_payload(buf, sizeof(buf), reject_reason_for(NOISE_ERROR_MAC_FAILURE)); + ASSERT_EQ(len, sizeof(buf)); + EXPECT_EQ(buf[0], HANDSHAKE_STATUS_REJECT); + EXPECT_EQ(memcmp(buf + 1, "Handsha", 7), 0); + + // A one-byte buffer still carries the status byte + uint8_t tiny[1]; + len = format_reject_payload(tiny, sizeof(tiny), reject_reason_for(NOISE_ERROR_MAC_FAILURE)); + ASSERT_EQ(len, 1u); + EXPECT_EQ(tiny[0], HANDSHAKE_STATUS_REJECT); + + // A zero-capacity buffer yields no payload and stays untouched + uint8_t none[1] = {0xAA}; + EXPECT_EQ(format_reject_payload(none, 0, reject_reason_for(NOISE_ERROR_MAC_FAILURE)), 0u); + EXPECT_EQ(none[0], 0xAA); +} + +} // namespace esphome::noise::testing From a027d0623166629c921426cb15143b62ddba7501 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 18:12:14 -0500 Subject: [PATCH 21/21] Give the fake env real membership, widen the manifest gate, route zephyr through the shared lexer --- esphome/components/zephyr/library.py | 7 ++- esphome/espidf/component.py | 6 ++- esphome/platformio/extra_script.py | 11 ++++- esphome/platformio/library.py | 46 +++++++++++-------- tests/unit_tests/test_espidf_component.py | 13 ++++++ .../test_platformio_extra_script.py | 21 +++++++++ tests/unit_tests/test_platformio_library.py | 20 +++++++- tests/unit_tests/test_zephyr_library.py | 16 +++++++ 8 files changed, 117 insertions(+), 23 deletions(-) diff --git a/esphome/components/zephyr/library.py b/esphome/components/zephyr/library.py index 0e6551ccf1..b339ae45b0 100644 --- a/esphome/components/zephyr/library.py +++ b/esphome/components/zephyr/library.py @@ -28,6 +28,7 @@ from esphome.platformio.library import ( collect_filtered_files, convert_libraries, ensure_list, + lex_build_flags, split_list_by_condition, ) @@ -80,7 +81,11 @@ def generate_cmakelists_txt(component: ConvertedLibrary) -> str: build_include_dir = build.get("includeDir", DEFAULT_BUILD_INCLUDE_DIR) build_src_filter = ensure_list(build.get("srcFilter", DEFAULT_BUILD_SRC_FILTER)) - build_flags = ensure_list(build.get("flags", DEFAULT_BUILD_FLAGS)) + # The shared lexer re-glues spaced entries and drops bare/empty + # arguments, same as the espidf emitter + build_flags = lex_build_flags( + build.get("flags", DEFAULT_BUILD_FLAGS), component.name + ) src_files = collect_filtered_files( read_path / Path(build_src_dir), build_src_filter diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 4655d1c54d..105413cf44 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -58,8 +58,10 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: """ def escape_entry(p: PathType) -> str: - # In CMakeLists.txt, backslashes need to be escaped - return f'"{str(p)}"'.replace("\\", "\\\\") + # In CMakeLists.txt, backslashes and embedded quotes need escaping + # (a quoted define value reaches here via the shlex round-trip) + escaped = str(p).replace("\\", "\\\\").replace('"', '\\"') + return f'"{escaped}"' def escape_path(p: PathType) -> str: # CMake uses forward slashes for paths on every platform and treats diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index e60a50d746..e04ccd1f55 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -33,7 +33,7 @@ def apply_extra_script( """Run a library's ``extraScript`` and fold its captured env vars into ``build.flags``; ``board_mcu`` is a callable so it resolves lazily.""" extra_script = component.data.get("build", {}).get("extraScript") - if not extra_script: + if extra_script is None or extra_script == "": return if not isinstance(extra_script, str): # A list/dict value would raise an opaque TypeError on the join below @@ -170,6 +170,15 @@ class _FakeSConsEnv: ) return self._vars.get(key, default) + def __contains__(self, key: object) -> bool: + # Without this, "KEY" in env falls back to the legacy sequence + # protocol: __getitem__(0), (1), ... never raises, so it loops + # forever flooding the log + return key in self._vars + + def __iter__(self): + return iter(self._vars) + def __getitem__(self, key: str) -> str: # Scripts also read env["BOARD_MCU"]; an unmodelled subscript # degrades one branch instead of discarding the whole capture diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 6d26fea534..df1d6aa07b 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -656,15 +656,15 @@ def dependency_is_usable( def _valid_dependency_entry(entry: dict, manifest_name: str) -> bool: - """Whether a normalized entry carries a usable name (non-empty string) - and version (string, if present); invalid entries warn naming the - manifest.""" + """Whether a normalized entry carries a usable name (non-empty string), + version (string, if present), and owner (string, if present); invalid + entries warn naming the manifest.""" name = entry.get("name") - if ( - isinstance(name, str) - and name - and ("version" not in entry or isinstance(entry["version"], str)) - ): + owner = entry.get("owner") + name_ok = isinstance(name, str) and name + version_ok = "version" not in entry or isinstance(entry["version"], str) + owner_ok = owner is None or isinstance(owner, str) + if name_ok and version_ok and owner_ok: return True _LOGGER.warning( "Ignoring unrecognized dependency entry %r of %s", entry, manifest_name @@ -683,7 +683,7 @@ def normalize_dependencies( so callers see a uniform list. ``manifest_name`` names the manifest in the warning for entries that cannot be normalized. """ - if not dependencies: + if dependencies is None: return [] if isinstance(dependencies, str): # A plain string is one or more comma-separated names; iterating it @@ -1004,11 +1004,19 @@ def convert_libraries( f"library.properties in {source_dir}" ) - if not isinstance(component.data, dict) or not isinstance( - component.data.get("build", {}), dict - ): - # A bare json.load imposes no shape; every backend dereferences - # data/build, so validate once here and name the library + # A bare json.load imposes no shape; every backend dereferences + # these fields, so validate once here and name the library + malformed = not isinstance(component.data, dict) + if not malformed: + build = component.data.get("build", {}) + malformed = ( + not isinstance(build, dict) + or not isinstance(component.data.get(ESPHOME_DATA_KEY, {}), dict) + or not isinstance(build.get("srcDir", ""), str) + or not isinstance(build.get("includeDir", ""), str) + or not isinstance(build.get("srcFilter", ""), (str, list)) + ) + if malformed: raise EsphomeError(f"Library {key} has a malformed manifest") warn_properties_depends(component.name, component.data) @@ -1018,10 +1026,12 @@ def convert_libraries( # An explicitly requested library fails fast; the routine # cross-platform skip stays at debug, other causes warn if key in top_level_keys: - raise RuntimeError( - f"Requested library {key} is not compatible with " - f"{backend.framework}: {e}" - ) from e + reason = ( + f"is not compatible with {backend.framework}" + if isinstance(e, IncompatiblePlatform) + else "has a malformed manifest" + ) + raise RuntimeError(f"Requested library {key} {reason}: {e}") from e if isinstance(e, IncompatiblePlatform): _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) else: diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index 5a273b987c..0caff8174e 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -294,6 +294,19 @@ def test_generate_cmakelists_txt_multi_token_flag(tmp_component): assert ' "-include"\n "cp_custom_alloc.h"\n' in content +def test_generate_cmakelists_txt_escapes_embedded_quotes(tmp_component): + """A define value carrying a literal quote survives into CMake as an + escaped quote, not a prematurely-terminated string.""" + src_dir = tmp_component.path / "src" + src_dir.mkdir() + (src_dir / "main.c").write_text("int main() {}") + # shlex keeps the backslash-escaped quotes as literal characters + tmp_component.data = {"build": {"flags": ['-DMSG=\\"hi\\"']}} + + content = generate_cmakelists_txt(tmp_component) + assert '"-DMSG=\\"hi\\""' in content + + def test_generate_cmakelists_txt_extra_script_link_flags(tmp_component): """Captured extra-script LINKFLAGS come out as target_link_options, not compile options where they would be silently ineffective.""" diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 980ce29ccb..07f0e78500 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -461,6 +461,27 @@ def test_prepend_inserts_ahead_of_existing(method: str) -> None: assert env.result.libs == ["algobsec", "bsec", "m"] +def test_env_membership_and_iteration(tmp_path) -> None: + """Membership tests and for-loops must use the mapping protocol; the + legacy sequence fallback through __getitem__ would loop forever.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + assert "BOARD_MCU" in env + assert "NOPE" not in env + assert sorted(env) == ["BOARD_MCU", "PIOENV", "PIOPLATFORM"] + + +def test_apply_extra_script_non_string_falsey_raises(tmp_path) -> None: + """A falsey non-string extraScript (false, 0, []) is a malformed + manifest, not an absent script.""" + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": False}} + with pytest.raises(EsphomeError, match="must be a string"): + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + def test_env_get_unknown_key_warns_once(caplog) -> None: """A script branching on an unmodelled env var is diagnosable.""" env = _FakeSConsEnv( diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 0f90ad3e84..1f62513b7b 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -614,10 +614,28 @@ def test_normalize_dependencies_forms(caplog) -> None: assert normalize_dependencies({"Foo": ["1.0", "2.0"]}, "libx") == [] assert normalize_dependencies([{"name": "Foo", "version": 1}], "libx") == [] assert caplog.text.count("unrecognized dependency entry") == 7 + # A non-string owner would stringify into a malformed registry name + assert ( + normalize_dependencies( + [{"name": "Foo", "owner": {"bad": 1}, "version": "1.0"}], "libx" + ) + == [] + ) + # A falsey scalar (0, false) is malformed, not an empty list + assert normalize_dependencies(0, "libx") == [] + assert "Ignoring unrecognized dependencies 0 of libx" in caplog.text @pytest.mark.parametrize( - "manifest", [["not", "a", "manifest"], {"name": "A", "build": "src"}] + "manifest", + [ + ["not", "a", "manifest"], + {"name": "A", "build": "src"}, + {"name": "A", "ESPHOME": "yes"}, + {"name": "A", "build": {"srcDir": 123}}, + {"name": "A", "build": {"includeDir": ["inc"]}}, + {"name": "A", "build": {"srcFilter": {"+": "src"}}}, + ], ) def test_convert_libraries_malformed_manifest_raises( tmp_path, monkeypatch, manifest diff --git a/tests/unit_tests/test_zephyr_library.py b/tests/unit_tests/test_zephyr_library.py index b370fe0c47..0d899ec91d 100644 --- a/tests/unit_tests/test_zephyr_library.py +++ b/tests/unit_tests/test_zephyr_library.py @@ -66,6 +66,22 @@ def test_generate_cmakelists_txt_flags_and_includes(tmp_path): assert "-lm" in out +def test_generate_cmakelists_txt_lexes_spaced_flags(tmp_path): + """A spaced -I entry routes to include dirs instead of landing verbatim + in compile options; same shared lexer as the espidf emitter.""" + c = _make_component(tmp_path) + (tmp_path / "src").mkdir() + (tmp_path / "src" / "a.c").write_text("") + (tmp_path / "include").mkdir() + c.data = {"build": {"flags": "-I include -DBAR=1"}} + + out = generate_cmakelists_txt(c) + + assert str((tmp_path / "include").resolve()).replace("\\", "\\\\") in out + assert "-DBAR=1" in out + assert "-I include" not in out + + def test_generate_zephyr_modules_collects_all_dirs_and_writes(tmp_path, monkeypatch): # Two converted libraries: one top-level, one transitive dependency. The # converter calls backend.emit for both; generate_zephyr_modules must return