From b5ccd55f4ed8cf9c756505a0b2c1767f5d957be5 Mon Sep 17 00:00:00 2001 From: Javier Peletier Date: Sat, 25 Apr 2026 19:06:58 +0200 Subject: [PATCH 1/2] [packages] Fix premature substitution of vars in remote package files (#15997) Co-authored-by: J. Nick Koston Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- esphome/components/packages/__init__.py | 21 ++- esphome/yaml_util.py | 11 +- .../component_tests/packages/test_packages.py | 130 ++++++++++++++++++ tests/unit_tests/test_yaml_util.py | 62 ++++++++- 4 files changed, 219 insertions(+), 5 deletions(-) diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index b6ec0067c9..1b9e03d88f 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -378,9 +378,8 @@ def _substitute_package_definition( Local package contents are left untouched — they will be substituted later during the main substitution pass. """ - if isinstance(package_config, str) or ( - isinstance(package_config, dict) and is_remote_package(package_config) - ): + + def do_substitute(package_config: dict | str) -> dict | str: # Collect undefined-variable errors (rather than raising strict) so the # path walked through a remote-package dict is preserved and the user # sees which field (url / path / ref / ...) referenced the undefined @@ -394,6 +393,22 @@ def _substitute_package_definition( errors=errors, ) raise_first_undefined(errors, "package definition") + return package_config + + if isinstance(package_config, str): + return do_substitute(package_config) + + if isinstance(package_config, dict) and is_remote_package(package_config): + # Mark vars as literal to avoid substituting variables in the vars block itself, since they are meant to be + # passed as-is to the package YAML and may contain their own substitution expressions that should not + # be prematurely evaluated here. + if CONF_FILES in package_config: + for file_def in package_config[CONF_FILES]: + if isinstance(file_def, dict) and CONF_VARS in file_def: + file_def[CONF_VARS] = yaml_util.make_literal(file_def[CONF_VARS]) + + package_config = do_substitute(package_config) + return package_config diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index 42da27ec14..3cfc9c4b15 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -113,6 +113,15 @@ def make_data_base( return value +def make_literal(value: Any) -> ESPLiteralValue | Any: + """Wrap a value in an ESPLiteralValue object.""" + try: + return add_class_to_obj(value, ESPLiteralValue) + except TypeError: + # Adding class failed, ignore error + return value + + def add_context(value: Any, context_vars: dict[str, Any] | None) -> Any: """Tags a list/string/dict value with context vars that must be applied to it and its children during the substitution pass. If no vars are given, no tagging is done. @@ -525,7 +534,7 @@ class ESPHomeLoaderMixin: obj = self.construct_sequence(node) elif isinstance(node, yaml.MappingNode): obj = self.construct_mapping(node) - return add_class_to_obj(obj, ESPLiteralValue) + return make_literal(obj) @_add_data_ref def construct_extend(self, node: yaml.Node) -> Extend: diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index af4b6db796..13a6da9f2c 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -1491,3 +1491,133 @@ def test_substitute_package_definition_includes_source_location(tmp_path: Path) line, col = int(match.group(1)), int(match.group(2)) assert line == 2, f"expected 1-based line 2, got {line} (err={err!r})" assert col >= 1, f"expected 1-based column ≥ 1, got {col} (err={err!r})" + + +def test_substitute_package_definition_vars_preserved_literally() -> None: + """``vars:`` blocks in remote-package files are not substituted prematurely. + + Variable references inside ``vars:`` may resolve to substitutions + contributed by sibling packages that have not yet been loaded, so they + must be passed through untouched and resolved later by the package YAML. + """ + pkg = { + CONF_URL: "https://github.com/esphome/non-existant-repo", + CONF_REF: "main", + CONF_FILES: [ + { + CONF_PATH: "common/somefile.yaml", + CONF_VARS: {"pin": "${PIN}"}, + }, + ], + } + # Note: PIN is intentionally NOT in the context — it is meant to + # be resolved later, when the package YAML is processed. + result = _substitute_package_definition(pkg, ContextVars()) + + assert result[CONF_FILES][0][CONF_VARS] == {"pin": "${PIN}"} + + +def test_substitute_package_definition_other_fields_still_substituted() -> None: + """Marking ``vars:`` literal does not stop substitution of url/ref/path.""" + ctx = ContextVars({"branch": "release", "org": "esphome"}) + pkg = { + CONF_URL: "https://github.com/${org}/firmware", + CONF_REF: "${branch}", + CONF_FILES: [ + { + CONF_PATH: "common/sensor.yaml", + CONF_VARS: {"pin": "${PIN}"}, + }, + ], + } + result = _substitute_package_definition(pkg, ctx) + + assert result[CONF_URL] == "https://github.com/esphome/firmware" + assert result[CONF_REF] == "release" + # vars passed through unchanged + assert result[CONF_FILES][0][CONF_VARS] == {"pin": "${PIN}"} + + +def test_substitute_package_definition_without_vars_unaffected() -> None: + """Files entries without a ``vars:`` block continue to work.""" + ctx = ContextVars({"branch": "main"}) + pkg = { + CONF_URL: "https://github.com/esphome/firmware", + CONF_REF: "${branch}", + CONF_FILES: [ + {CONF_PATH: "file1.yaml"}, + "file2.yaml", + ], + } + result = _substitute_package_definition(pkg, ctx) + + assert result[CONF_REF] == "main" + assert result[CONF_FILES][0] == {CONF_PATH: "file1.yaml"} + assert result[CONF_FILES][1] == "file2.yaml" + + +@patch("esphome.yaml_util.load_yaml") +@patch("pathlib.Path.is_file") +@patch("esphome.git.clone_or_update") +def test_remote_package_vars_resolved_against_sibling_package_substitutions( + mock_clone_or_update, mock_is_file, mock_load_yaml +) -> None: + """A ``vars:`` reference in one remote package can resolve to a + substitution defined in a sibling remote package. + + A higher-priority package declares ``substitutions:`` (e.g. ``SENSOR_PIN: 5``) and a + lower-priority package's ``files: -> vars:`` references that substitution. + Because packages are processed highest-priority first and ``vars:`` is now + preserved literally during package-definition processing, the substitution + is resolved correctly when the package YAML itself is loaded. + """ + mock_clone_or_update.return_value = (Path("/tmp/noexists"), MagicMock()) + mock_is_file.return_value = True + + # Two YAML files mocked from the "remote" repo: + # - platform.yaml exports a substitution ``SENSOR_PIN`` + # - sensor.yaml uses ``${pin}`` (which is bound from ``vars:`` to + # ``${SENSOR_PIN}`` and resolved against the merged substitutions). + mock_load_yaml.side_effect = [ + # Order matches reverse-priority traversal (highest priority first). + OrderedDict( + { + CONF_SUBSTITUTIONS: {"SENSOR_PIN": "GPIO5"}, + } + ), + OrderedDict( + { + CONF_SENSOR: [ + { + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_1, + "pin": "${pin}", + } + ], + } + ), + ] + + config = { + CONF_PACKAGES: { + "special_sensor": { + CONF_URL: "https://github.com/esphome/non-existant-repo", + CONF_FILES: [ + { + CONF_PATH: "sensor.yaml", + CONF_VARS: {"pin": "${SENSOR_PIN}"}, + }, + ], + CONF_REFRESH: "1d", + }, + "platform": { + CONF_URL: "https://github.com/esphome/non-existant-repo", + CONF_FILES: ["platform.yaml"], + CONF_REFRESH: "1d", + }, + } + } + + actual = packages_pass(config) + + assert actual[CONF_SENSOR][0]["pin"] == "GPIO5" diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index e3aa2a16f5..3815ac1d75 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -11,7 +11,13 @@ from esphome.config_helpers import Extend, Remove import esphome.config_validation as cv from esphome.core import DocumentLocation, DocumentRange, EsphomeError from esphome.util import OrderedDict -from esphome.yaml_util import ESPHomeDataBase, format_path, make_data_base +from esphome.yaml_util import ( + ESPHomeDataBase, + ESPLiteralValue, + format_path, + make_data_base, + make_literal, +) @pytest.fixture(autouse=True) @@ -891,3 +897,57 @@ def test_format_path_empty_path_with_located_current_obj(): obj = _located("${var}", "main.yaml", 0, 0) result = format_path([], obj) assert result == "In: in main.yaml 1:1" + + +def test_make_literal_wraps_dict() -> None: + """A dict is wrapped so it becomes an ESPLiteralValue instance.""" + value = {"key": "${var}"} + result = make_literal(value) + assert isinstance(result, ESPLiteralValue) + assert isinstance(result, dict) + assert result == {"key": "${var}"} + + +def test_make_literal_wraps_list() -> None: + """A list is wrapped so it becomes an ESPLiteralValue instance.""" + value = ["${var}", "plain"] + result = make_literal(value) + assert isinstance(result, ESPLiteralValue) + assert isinstance(result, list) + assert result == ["${var}", "plain"] + + +def test_make_literal_wraps_string() -> None: + """A string is wrapped so it becomes an ESPLiteralValue instance.""" + result = make_literal("${var}") + assert isinstance(result, ESPLiteralValue) + assert result == "${var}" + + +def test_make_literal_returns_already_wrapped_value_unchanged() -> None: + """Wrapping a value that is already an ESPLiteralValue returns it as-is.""" + value = make_literal({"key": "value"}) + assert isinstance(value, ESPLiteralValue) + result = make_literal(value) + assert result is value + + +def test_make_literal_returns_none_unchanged() -> None: + """Values whose class cannot be augmented (e.g. ``None``) are returned as-is.""" + result = make_literal(None) + assert result is None + + +def test_make_literal_blocks_substitution() -> None: + """A value wrapped with make_literal is skipped by the substitution pass.""" + value = make_literal({"pin": "${PIN}"}) + result = substitutions.substitute( + value, + path=[], + parent_context=substitutions.ContextVars(), + strict_undefined=False, + ) + # The literal block must remain untouched, even though the variable is + # undefined in the context. + assert result == {"pin": "${PIN}"} + assert isinstance(result, ESPLiteralValue) From 15cb2c0580a4bd985f4e19b809bdb0eda6c0af9d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 25 Apr 2026 12:39:32 -0500 Subject: [PATCH 2/2] [output] Gate FloatOutput power scaling fields behind USE_OUTPUT_FLOAT_POWER_SCALING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The min_power / max_power / zero_means_zero scaling support on FloatOutput costs 12 bytes per instance (max_power_, min_power_, zero_means_zero_ + alignment padding) on every PWM channel, DAC channel, LEDC output, and dimmer-chip channel — even on configs that never touch the feature. Repo-wide usage is ~17 YAML lines, mostly in test fixtures and a couple of LED-driver chip tests; the runtime set_min_power / set_max_power actions added in #8934 have no usage outside the action's own test. Add USE_OUTPUT_FLOAT_POWER_SCALING and gate the fields and scaling math in FloatOutput::set_level() behind it, mirroring the USE_POWER_SUPPLY pattern already used in BinaryOutput. Python codegen flips the define on whenever: - a min_power / max_power / zero_means_zero key is set on any output, or - a non-default zero_means_zero value is provided, or - an output.set_min_power / output.set_max_power action is registered The action class templates (SetMinPowerAction, SetMaxPowerAction) are also gated on the same define so their non-dependent member access on FloatOutput::set_min_power doesn't fail to parse when the methods aren't compiled in. zero_means_zero_ now has a default initializer (was UB before — it was always written from setup, but only because the schema default forced it). For configs without scaling: 12 B .bss saved per FloatOutput instance, plus a small flash saving from the elided multiply/subtract in set_level(). For configs with scaling: behavior is unchanged. Verified on tests/components/esp8266_pwm (no scaling): pstorage 0x28 → 0x1c per output (40 B → 28 B). Verified on tests/components/output (uses set_min_power/set_max_power actions): builds correctly with the define on. --- esphome/components/output/__init__.py | 9 ++++++++- esphome/components/output/automation.h | 3 +++ esphome/components/output/float_output.cpp | 4 ++++ esphome/components/output/float_output.h | 19 ++++++++++++++++++- esphome/core/defines.h | 1 + 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/esphome/components/output/__init__.py b/esphome/components/output/__init__.py index 36798f2d7f..dd2b5e9372 100644 --- a/esphome/components/output/__init__.py +++ b/esphome/components/output/__init__.py @@ -54,10 +54,15 @@ async def setup_output_platform_(obj, config): power_supply_ = await cg.get_variable(config[CONF_POWER_SUPPLY]) cg.add(obj.set_power_supply(power_supply_)) if CONF_MAX_POWER in config: + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_max_power(config[CONF_MAX_POWER])) if CONF_MIN_POWER in config: + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_min_power(config[CONF_MIN_POWER])) - if CONF_ZERO_MEANS_ZERO in config: + # Only emit (and pull in the scaling field) when explicitly enabled — the + # C++ default initializer covers the False case, saving 4 B per instance. + if config.get(CONF_ZERO_MEANS_ZERO): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") cg.add(obj.set_zero_means_zero(config[CONF_ZERO_MEANS_ZERO])) @@ -121,6 +126,7 @@ async def output_set_level_to_code(config, action_id, template_arg, args): synchronous=True, ) async def output_set_min_power_to_code(config, action_id, template_arg, args): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") paren = await cg.get_variable(config[CONF_ID]) var = cg.new_Pvariable(action_id, template_arg, paren) template_ = await cg.templatable(config[CONF_MIN_POWER], args, cg.float_) @@ -140,6 +146,7 @@ async def output_set_min_power_to_code(config, action_id, template_arg, args): synchronous=True, ) async def output_set_max_power_to_code(config, action_id, template_arg, args): + cg.add_define("USE_OUTPUT_FLOAT_POWER_SCALING") paren = await cg.get_variable(config[CONF_ID]) var = cg.new_Pvariable(action_id, template_arg, paren) template_ = await cg.templatable(config[CONF_MAX_POWER], args, cg.float_) diff --git a/esphome/components/output/automation.h b/esphome/components/output/automation.h index 3279378129..537226a143 100644 --- a/esphome/components/output/automation.h +++ b/esphome/components/output/automation.h @@ -2,6 +2,7 @@ #include "esphome/core/component.h" #include "esphome/core/automation.h" +#include "esphome/core/defines.h" #include "esphome/components/output/binary_output.h" #include "esphome/components/output/float_output.h" @@ -40,6 +41,7 @@ template class SetLevelAction : public Action { FloatOutput *output_; }; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING template class SetMinPowerAction : public Action { public: SetMinPowerAction(FloatOutput *output) : output_(output) {} @@ -63,6 +65,7 @@ template class SetMaxPowerAction : public Action { protected: FloatOutput *output_; }; +#endif // USE_OUTPUT_FLOAT_POWER_SCALING } // namespace output } // namespace esphome diff --git a/esphome/components/output/float_output.cpp b/esphome/components/output/float_output.cpp index 46014e0903..f6c75fbef6 100644 --- a/esphome/components/output/float_output.cpp +++ b/esphome/components/output/float_output.cpp @@ -7,6 +7,7 @@ namespace output { static const char *const TAG = "output.float"; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING void FloatOutput::set_max_power(float max_power) { this->max_power_ = clamp(max_power, this->min_power_, 1.0f); // Clamp to MIN>=MAX>=1.0 } @@ -14,6 +15,7 @@ void FloatOutput::set_max_power(float max_power) { void FloatOutput::set_min_power(float min_power) { this->min_power_ = clamp(min_power, 0.0f, this->max_power_); // Clamp to 0.0>=MIN>=MAX } +#endif void FloatOutput::set_level(float state) { state = clamp(state, 0.0f, 1.0f); @@ -26,8 +28,10 @@ void FloatOutput::set_level(float state) { } #endif +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING if (state != 0.0f || !this->zero_means_zero_) // regardless of min_power_, 0.0 means off state = (state * (this->max_power_ - this->min_power_)) + this->min_power_; +#endif if (this->is_inverted()) state = 1.0f - state; diff --git a/esphome/components/output/float_output.h b/esphome/components/output/float_output.h index 5225f88c66..e0a7a9530f 100644 --- a/esphome/components/output/float_output.h +++ b/esphome/components/output/float_output.h @@ -1,11 +1,13 @@ #pragma once #include "esphome/core/component.h" +#include "esphome/core/defines.h" #include "binary_output.h" namespace esphome { namespace output { +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING #define LOG_FLOAT_OUTPUT(this) \ LOG_BINARY_OUTPUT(this) \ if (this->max_power_ != 1.0f) { \ @@ -14,6 +16,9 @@ namespace output { if (this->min_power_ != 0.0f) { \ ESP_LOGCONFIG(TAG, " Min Power: %.1f%%", this->min_power_ * 100.0f); \ } +#else +#define LOG_FLOAT_OUTPUT(this) LOG_BINARY_OUTPUT(this) +#endif /** Base class for all output components that can output a variable level, like PWM. * @@ -30,6 +35,7 @@ namespace output { */ class FloatOutput : public BinaryOutput { public: +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING /** Set the maximum power output of this component. * * All values are multiplied by max_power - min_power and offset to min_power to get the adjusted value. @@ -51,6 +57,7 @@ class FloatOutput : public BinaryOutput { * @param zero_means_zero True if a 0 state should mean 0 and not min_power. */ void set_zero_means_zero(bool zero_means_zero) { this->zero_means_zero_ = zero_means_zero; } +#endif /** Set the level of this float output, this is called from the front-end. * @@ -69,20 +76,30 @@ class FloatOutput : public BinaryOutput { // ========== INTERNAL METHODS ========== // (In most use cases you won't need these) +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING /// Get the maximum power output. float get_max_power() const { return this->max_power_; } /// Get the minimum power output. float get_min_power() const { return this->min_power_; } +#else + /// Get the maximum power output. + float get_max_power() const { return 1.0f; } + + /// Get the minimum power output. + float get_min_power() const { return 0.0f; } +#endif protected: /// Implement BinarySensor's write_enabled; this should never be called. void write_state(bool state) override; virtual void write_state(float state) = 0; +#ifdef USE_OUTPUT_FLOAT_POWER_SCALING float max_power_{1.0f}; float min_power_{0.0f}; - bool zero_means_zero_; + bool zero_means_zero_{false}; +#endif }; } // namespace output diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 80247f69da..154211d825 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -132,6 +132,7 @@ #define USE_NEXTION_WAVEFORM #define USE_NUMBER #define USE_OUTPUT +#define USE_OUTPUT_FLOAT_POWER_SCALING #define USE_POWER_SUPPLY #define USE_PREFERENCES_SYNC_EVERY_LOOP #define USE_QR_CODE