From f238f9331272c25bf073b921f443e62e75c73c6c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 22 Dec 2025 11:37:51 -1000 Subject: [PATCH] [core] Move comment to PROGMEM on ESP8266 (#12554) --- esphome/components/web_server/web_server.cpp | 4 +- esphome/core/application.h | 23 +++--- esphome/core/build_info_data.h | 2 + esphome/core/config.py | 3 +- esphome/writer.py | 25 +++++-- tests/dummy_main.cpp | 2 +- tests/unit_tests/test_writer.py | 76 +++++++++++++++++--- 7 files changed, 105 insertions(+), 30 deletions(-) diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index ece9d6512..b0731f335 100644 --- a/esphome/components/web_server/web_server.cpp +++ b/esphome/components/web_server/web_server.cpp @@ -287,7 +287,9 @@ std::string WebServer::get_config_json() { JsonObject root = builder.root(); root[ESPHOME_F("title")] = App.get_friendly_name().empty() ? App.get_name() : App.get_friendly_name(); - root[ESPHOME_F("comment")] = App.get_comment_ref(); + char comment_buffer[ESPHOME_COMMENT_SIZE]; + App.get_comment_string(comment_buffer); + root[ESPHOME_F("comment")] = comment_buffer; #if defined(USE_WEBSERVER_OTA_DISABLED) || !defined(USE_WEBSERVER_OTA) root[ESPHOME_F("ota")] = false; // Note: USE_WEBSERVER_OTA_DISABLED only affects web_server, not captive_portal #else diff --git a/esphome/core/application.h b/esphome/core/application.h index d2146a6c1..13461b3eb 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -12,6 +12,7 @@ #include "esphome/core/hal.h" #include "esphome/core/helpers.h" #include "esphome/core/preferences.h" +#include "esphome/core/progmem.h" #include "esphome/core/scheduler.h" #include "esphome/core/string_ref.h" #include "esphome/core/version.h" @@ -107,8 +108,7 @@ static const uint32_t TEARDOWN_TIMEOUT_REBOOT_MS = 1000; // 1 second for quick class Application { public: - void pre_setup(const std::string &name, const std::string &friendly_name, const char *comment, - bool name_add_mac_suffix) { + void pre_setup(const std::string &name, const std::string &friendly_name, bool name_add_mac_suffix) { arch_init(); this->name_add_mac_suffix_ = name_add_mac_suffix; if (name_add_mac_suffix) { @@ -127,7 +127,6 @@ class Application { this->name_ = name; this->friendly_name_ = friendly_name; } - this->comment_ = comment; } #ifdef USE_DEVICES @@ -264,10 +263,19 @@ class Application { return ""; } - /// Get the comment of this Application set by pre_setup(). - std::string get_comment() const { return this->comment_; } - /// Get the comment as StringRef (avoids allocation) - StringRef get_comment_ref() const { return StringRef(this->comment_); } + /// Copy the comment string into the provided buffer + /// Buffer must be ESPHOME_COMMENT_SIZE bytes (compile-time enforced) + void get_comment_string(std::span buffer) { + ESPHOME_strncpy_P(buffer.data(), ESPHOME_COMMENT_STR, buffer.size()); + buffer[buffer.size() - 1] = '\0'; + } + + /// Get the comment of this Application as a string + std::string get_comment() { + char buffer[ESPHOME_COMMENT_SIZE]; + this->get_comment_string(buffer); + return std::string(buffer); + } bool is_name_add_mac_suffix_enabled() const { return this->name_add_mac_suffix_; } @@ -513,7 +521,6 @@ class Application { // Pointer-sized members first Component *current_component_{nullptr}; - const char *comment_{nullptr}; // std::vector (3 pointers each: begin, end, capacity) // Partitioned vector design for looping components diff --git a/esphome/core/build_info_data.h b/esphome/core/build_info_data.h index 5e424ffac..02bb465e4 100644 --- a/esphome/core/build_info_data.h +++ b/esphome/core/build_info_data.h @@ -7,4 +7,6 @@ #define ESPHOME_CONFIG_HASH 0x12345678U // NOLINT #define ESPHOME_BUILD_TIME 1700000000 // NOLINT +#define ESPHOME_COMMENT_SIZE 1 // NOLINT static const char ESPHOME_BUILD_TIME_STR[] = "2024-01-01 00:00:00 +0000"; +static const char ESPHOME_COMMENT_STR[] = ""; diff --git a/esphome/core/config.py b/esphome/core/config.py index 507a39b40..5e32b9380 100644 --- a/esphome/core/config.py +++ b/esphome/core/config.py @@ -209,7 +209,7 @@ CONFIG_SCHEMA = cv.All( cv.Required(CONF_NAME): cv.valid_name, cv.Optional(CONF_FRIENDLY_NAME, ""): cv.All(cv.string, cv.Length(max=120)), cv.Optional(CONF_AREA): validate_area_config, - cv.Optional(CONF_COMMENT): cv.string, + cv.Optional(CONF_COMMENT): cv.All(cv.string, cv.Length(max=255)), cv.Required(CONF_BUILD_PATH): cv.string, cv.Optional(CONF_PLATFORMIO_OPTIONS, default={}): cv.Schema( { @@ -505,7 +505,6 @@ async def to_code(config: ConfigType) -> None: cg.App.pre_setup( config[CONF_NAME], config[CONF_FRIENDLY_NAME], - config.get(CONF_COMMENT, ""), config[CONF_NAME_ADD_MAC_SUFFIX], ) ) diff --git a/esphome/writer.py b/esphome/writer.py index 183fff873..9ae40e417 100644 --- a/esphome/writer.py +++ b/esphome/writer.py @@ -21,6 +21,7 @@ from esphome.const import ( from esphome.core import CORE, EsphomeError from esphome.helpers import ( copy_file_if_changed, + cpp_string_escape, get_str_env, is_ha_addon, read_file, @@ -271,7 +272,7 @@ def copy_src_tree(): "esphome", "core", "build_info_data.h" ) build_info_json_path = CORE.relative_build_path("build_info.json") - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() # Defensively force a rebuild if the build_info files don't exist, or if # there was a config change which didn't actually cause a source change @@ -292,7 +293,9 @@ def copy_src_tree(): if sources_changed: write_file( build_info_data_h_path, - generate_build_info_data_h(config_hash, build_time, build_time_str), + generate_build_info_data_h( + config_hash, build_time, build_time_str, comment + ), ) write_file( build_info_json_path, @@ -332,31 +335,39 @@ def generate_version_h(): ) -def get_build_info() -> tuple[int, int, str]: +def get_build_info() -> tuple[int, int, str, str]: """Calculate build_info values from current config. Returns: - Tuple of (config_hash, build_time, build_time_str) + Tuple of (config_hash, build_time, build_time_str, comment) """ config_hash = CORE.config_hash build_time = int(time.time()) build_time_str = time.strftime("%Y-%m-%d %H:%M:%S %z", time.localtime(build_time)) - return config_hash, build_time, build_time_str + comment = CORE.comment or "" + return config_hash, build_time, build_time_str, comment def generate_build_info_data_h( - config_hash: int, build_time: int, build_time_str: str + config_hash: int, build_time: int, build_time_str: str, comment: str ) -> str: - """Generate build_info_data.h header with config hash and build time.""" + """Generate build_info_data.h header with config hash, build time, and comment.""" + # cpp_string_escape returns '"escaped"', slice off the quotes since template has them + escaped_comment = cpp_string_escape(comment)[1:-1] + # +1 for null terminator + comment_size = len(comment) + 1 return f"""#pragma once // Auto-generated build_info data #define ESPHOME_CONFIG_HASH 0x{config_hash:08x}U // NOLINT #define ESPHOME_BUILD_TIME {build_time} // NOLINT +#define ESPHOME_COMMENT_SIZE {comment_size} // NOLINT #ifdef USE_ESP8266 #include static const char ESPHOME_BUILD_TIME_STR[] PROGMEM = "{build_time_str}"; +static const char ESPHOME_COMMENT_STR[] PROGMEM = "{escaped_comment}"; #else static const char ESPHOME_BUILD_TIME_STR[] = "{build_time_str}"; +static const char ESPHOME_COMMENT_STR[] = "{escaped_comment}"; #endif """ diff --git a/tests/dummy_main.cpp b/tests/dummy_main.cpp index 5849f4eb9..e6fe73380 100644 --- a/tests/dummy_main.cpp +++ b/tests/dummy_main.cpp @@ -12,7 +12,7 @@ using namespace esphome; void setup() { - App.pre_setup("livingroom", "LivingRoom", "comment", false); + App.pre_setup("livingroom", "LivingRoom", false); auto *log = new logger::Logger(115200, 512); // NOLINT log->pre_setup(); log->set_uart_selection(logger::UART_SELECTION_UART0); diff --git a/tests/unit_tests/test_writer.py b/tests/unit_tests/test_writer.py index 06a7d5dbd..f354d71bb 100644 --- a/tests/unit_tests/test_writer.py +++ b/tests/unit_tests/test_writer.py @@ -1186,8 +1186,9 @@ def test_get_build_info_new_build( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "Test comment" - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0x12345678 assert isinstance(build_time, int) @@ -1195,6 +1196,7 @@ def test_get_build_info_new_build( assert isinstance(build_time_str, str) # Verify build_time_str format matches expected pattern assert len(build_time_str) >= 19 # e.g., "2025-12-15 16:27:44 +0000" + assert comment == "Test comment" @patch("esphome.writer.CORE") @@ -1206,6 +1208,7 @@ def test_get_build_info_always_returns_current_time( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "" # Create existing build_info.json with matching config_hash and version existing_build_time = 1700000000 @@ -1222,7 +1225,7 @@ def test_get_build_info_always_returns_current_time( ) with patch("esphome.writer.__version__", "2025.1.0-dev"): - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0x12345678 # get_build_info now always returns current time @@ -1240,6 +1243,7 @@ def test_get_build_info_config_changed( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0xABCDEF00 # Different from existing + mock_core.comment = "" # Create existing build_info.json with different config_hash existing_build_time = 1700000000 @@ -1255,7 +1259,7 @@ def test_get_build_info_config_changed( ) with patch("esphome.writer.__version__", "2025.1.0-dev"): - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0xABCDEF00 assert build_time != existing_build_time # New time generated @@ -1271,6 +1275,7 @@ def test_get_build_info_version_changed( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "" # Create existing build_info.json with different version existing_build_time = 1700000000 @@ -1286,7 +1291,7 @@ def test_get_build_info_version_changed( ) with patch("esphome.writer.__version__", "2025.1.0-dev"): # New version - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0x12345678 assert build_time != existing_build_time # New time generated @@ -1302,11 +1307,12 @@ def test_get_build_info_invalid_json( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "" # Create invalid JSON file build_info_path.write_text("not valid json {{{") - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0x12345678 assert isinstance(build_time, int) @@ -1322,12 +1328,13 @@ def test_get_build_info_missing_keys( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "" # Create JSON with missing keys build_info_path.write_text(json.dumps({"config_hash": 0x12345678})) with patch("esphome.writer.__version__", "2025.1.0-dev"): - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() assert config_hash == 0x12345678 assert isinstance(build_time, int) @@ -1343,8 +1350,9 @@ def test_get_build_info_build_time_str_format( build_info_path = tmp_path / "build_info.json" mock_core.relative_build_path.return_value = build_info_path mock_core.config_hash = 0x12345678 + mock_core.comment = "" - config_hash, build_time, build_time_str = get_build_info() + config_hash, build_time, build_time_str, comment = get_build_info() # Verify the format matches "%Y-%m-%d %H:%M:%S %z" # e.g., "2025-12-15 16:27:44 +0000" @@ -1357,36 +1365,73 @@ def test_generate_build_info_data_h_format() -> None: config_hash = 0x12345678 build_time = 1700000000 build_time_str = "2023-11-14 22:13:20 +0000" + comment = "Test comment" - result = generate_build_info_data_h(config_hash, build_time, build_time_str) + result = generate_build_info_data_h( + config_hash, build_time, build_time_str, comment + ) assert "#pragma once" in result assert "#define ESPHOME_CONFIG_HASH 0x12345678U" in result assert "#define ESPHOME_BUILD_TIME 1700000000" in result + assert "#define ESPHOME_COMMENT_SIZE 13" in result # len("Test comment") + 1 assert 'ESPHOME_BUILD_TIME_STR[] = "2023-11-14 22:13:20 +0000"' in result + assert 'ESPHOME_COMMENT_STR[] = "Test comment"' in result def test_generate_build_info_data_h_esp8266_progmem() -> None: """Test generate_build_info_data_h includes PROGMEM for ESP8266.""" - result = generate_build_info_data_h(0xABCDEF01, 1700000000, "test") + result = generate_build_info_data_h(0xABCDEF01, 1700000000, "test", "comment") # Should have ESP8266 PROGMEM conditional assert "#ifdef USE_ESP8266" in result assert "#include " in result assert "PROGMEM" in result + # Both build time and comment should have PROGMEM versions + assert 'ESPHOME_BUILD_TIME_STR[] PROGMEM = "test"' in result + assert 'ESPHOME_COMMENT_STR[] PROGMEM = "comment"' in result def test_generate_build_info_data_h_hash_formatting() -> None: """Test generate_build_info_data_h formats hash with leading zeros.""" # Test with small hash value that needs leading zeros - result = generate_build_info_data_h(0x00000001, 0, "test") + result = generate_build_info_data_h(0x00000001, 0, "test", "") assert "#define ESPHOME_CONFIG_HASH 0x00000001U" in result # Test with larger hash value - result = generate_build_info_data_h(0xFFFFFFFF, 0, "test") + result = generate_build_info_data_h(0xFFFFFFFF, 0, "test", "") assert "#define ESPHOME_CONFIG_HASH 0xffffffffU" in result +def test_generate_build_info_data_h_comment_escaping() -> None: + r"""Test generate_build_info_data_h properly escapes special characters in comment. + + Uses cpp_string_escape which outputs octal escapes for special characters: + - backslash (ASCII 92) -> \134 + - double quote (ASCII 34) -> \042 + - newline (ASCII 10) -> \012 + """ + # Test backslash escaping (ASCII 92 = octal 134) + result = generate_build_info_data_h(0, 0, "test", "backslash\\here") + assert 'ESPHOME_COMMENT_STR[] = "backslash\\134here"' in result + + # Test quote escaping (ASCII 34 = octal 042) + result = generate_build_info_data_h(0, 0, "test", 'has "quotes"') + assert 'ESPHOME_COMMENT_STR[] = "has \\042quotes\\042"' in result + + # Test newline escaping (ASCII 10 = octal 012) + result = generate_build_info_data_h(0, 0, "test", "line1\nline2") + assert 'ESPHOME_COMMENT_STR[] = "line1\\012line2"' in result + + +def test_generate_build_info_data_h_empty_comment() -> None: + """Test generate_build_info_data_h handles empty comment.""" + result = generate_build_info_data_h(0, 0, "test", "") + + assert "#define ESPHOME_COMMENT_SIZE 1" in result # Just null terminator + assert 'ESPHOME_COMMENT_STR[] = ""' in result + + @patch("esphome.writer.CORE") @patch("esphome.writer.iter_components") @patch("esphome.writer.walk_files") @@ -1445,6 +1490,7 @@ def test_copy_src_tree_writes_build_info_files( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "Test comment" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [("core", mock_component)] @@ -1466,6 +1512,8 @@ def test_copy_src_tree_writes_build_info_files( assert "#define ESPHOME_CONFIG_HASH 0xdeadbeefU" in build_info_h_content assert "#define ESPHOME_BUILD_TIME" in build_info_h_content assert "ESPHOME_BUILD_TIME_STR" in build_info_h_content + assert "#define ESPHOME_COMMENT_SIZE" in build_info_h_content + assert "ESPHOME_COMMENT_STR" in build_info_h_content # Verify build_info.json was written build_info_json_path = build_path / "build_info.json" @@ -1517,6 +1565,7 @@ def test_copy_src_tree_detects_config_hash_change( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF # Different from existing + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [] @@ -1578,6 +1627,7 @@ def test_copy_src_tree_detects_version_change( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [] @@ -1627,6 +1677,7 @@ def test_copy_src_tree_handles_invalid_build_info_json( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [] @@ -1700,6 +1751,7 @@ def test_copy_src_tree_build_info_timestamp_behavior( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [("test", mock_component)] @@ -1794,6 +1846,7 @@ def test_copy_src_tree_detects_removed_source_file( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = [] # No components = file should be removed @@ -1855,6 +1908,7 @@ def test_copy_src_tree_ignores_removed_generated_file( mock_core.relative_build_path.side_effect = lambda *args: build_path.joinpath(*args) mock_core.defines = [] mock_core.config_hash = 0xDEADBEEF + mock_core.comment = "" mock_core.target_platform = "test_platform" mock_core.config = {} mock_iter_components.return_value = []