From fb132b44211509e82c6c85d139f6d009662bce1d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 01:01:35 -0500 Subject: [PATCH] [improv_serial][esp32_improv] Use buffer based RPC response builder (#18793) --- .../esp32_improv/esp32_improv_component.cpp | 54 +++++---- .../esp32_improv/esp32_improv_component.h | 3 +- .../components/improv_base/improv_base.cpp | 18 +++ esphome/components/improv_base/improv_base.h | 6 + .../improv_serial/improv_serial_component.cpp | 110 ++++++++++++------ .../improv_serial/improv_serial_component.h | 23 +++- esphome/components/network/ip_address.h | 2 + esphome/core/defines.h | 5 +- tests/components/improv_base/benchmark.yaml | 6 + .../improv_base/rpc_response_builder_test.cpp | 102 ++++++++++++++++ .../improv_serial/common-uart0.yaml | 2 + .../fixtures/improv_serial_uart.yaml | 2 + tests/integration/test_improv_serial_uart.py | 15 ++- 13 files changed, 280 insertions(+), 68 deletions(-) create mode 100644 tests/components/improv_base/benchmark.yaml create mode 100644 tests/components/improv_base/rpc_response_builder_test.cpp diff --git a/esphome/components/esp32_improv/esp32_improv_component.cpp b/esphome/components/esp32_improv/esp32_improv_component.cpp index 6e3a4ef526..4756fba637 100644 --- a/esphome/components/esp32_improv/esp32_improv_component.cpp +++ b/esphome/components/esp32_improv/esp32_improv_component.cpp @@ -1,5 +1,7 @@ #include "esp32_improv_component.h" +#include + #include "esphome/components/bytebuffer/bytebuffer.h" #include "esphome/components/esp32_ble/ble.h" #include "esphome/components/esp32_ble_server/ble_2902.h" @@ -19,7 +21,13 @@ using namespace bytebuffer; static const char *const TAG = "esp32_improv.component"; static constexpr size_t IMPROV_MAX_LOG_BYTES = 128; -static const char *const ESPHOME_MY_LINK = "https://my.home-assistant.io/redirect/config_flow_start?domain=esphome"; +static constexpr char ESPHOME_MY_LINK[] = "https://my.home-assistant.io/redirect/config_flow_start?domain=esphome"; +// command + data length + trailing byte +static constexpr size_t RPC_RESPONSE_OVERHEAD = 3; +// Reserves the ESPHOME_MY_LINK entry; a maximal next URL displaces only the +// lower value web server URL +static constexpr size_t MAX_NEXT_URL_LEN = + improv::RPC_RESPONSE_MAX_SIZE - RPC_RESPONSE_OVERHEAD - 1 - sizeof(ESPHOME_MY_LINK); static constexpr uint16_t STOP_ADVERTISING_DELAY = 10000; // Delay (ms) before stopping service to allow BLE clients to read the final state static constexpr uint16_t NAME_ADVERTISING_INTERVAL = 60000; // Advertise name every 60 seconds @@ -285,8 +293,9 @@ void ESP32ImprovComponent::set_error_(improv::Error error) { } } -void ESP32ImprovComponent::send_response_(std::vector &&response) { - this->rpc_response_->set_value(std::move(response)); +void ESP32ImprovComponent::send_response_(std::span response) { + // The BLE characteristic owns its value, so one exact-size copy is required here + this->rpc_response_->set_value(std::vector(response.begin(), response.end())); if (this->state_ != improv::STATE_STOPPED) this->rpc_response_->notify(); } @@ -430,40 +439,35 @@ void ESP32ImprovComponent::check_wifi_connection_() { this->connecting_sta_ = {}; this->cancel_timeout("wifi-connect-timeout"); - // Build URL list with minimal allocations - // Maximum 3 URLs: custom next_url + ESPHOME_MY_LINK + webserver URL - std::string url_strings[3]; - size_t url_count = 0; + // Build the URL list directly into a stack buffer with no heap allocation + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::WIFI_SETTINGS); #ifdef USE_ESP32_IMPROV_NEXT_URL // Add next_url if configured (should be first per Improv BLE spec) - { - char url_buffer[384]; - size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); - if (len > 0) { - url_strings[url_count++] = std::string(url_buffer, len); - } - } + this->add_next_url_(builder, MAX_NEXT_URL_LEN); #endif - // Add default URLs for backward compatibility - url_strings[url_count++] = ESPHOME_MY_LINK; + // Add default URLs for backward compatibility; MAX_NEXT_URL_LEN reserves this + // entry's space, so it always fits + builder.add_string(ESPHOME_MY_LINK, sizeof(ESPHOME_MY_LINK) - 1); #ifdef USE_WEBSERVER for (auto &ip : wifi::global_wifi_component->wifi_sta_ip_addresses()) { if (ip.is_ip4()) { - // "http://" (7) + IPv4 max (15) + ":" (1) + port max (5) + null = 29 - char url_buffer[32]; - memcpy(url_buffer, "http://", 7); // NOLINT(bugprone-not-null-terminated-result) - str_to null-terminates - ip.str_to(url_buffer + 7); - size_t len = strlen(url_buffer); - snprintf(url_buffer + len, sizeof(url_buffer) - len, ":%d", USE_WEBSERVER_PORT); - url_strings[url_count++] = url_buffer; + char ip_buf[network::IP_ADDRESS_BUFFER_SIZE]; + ip.str_to(ip_buf); + // "http://" (7) + IP (40) + ":" (1) + port (5) + null (1) = 54 + char webserver_url[7 + network::IP_ADDRESS_BUFFER_SIZE + 1 + 5 + 1]; + size_t len = + buf_append_printf(webserver_url, sizeof(webserver_url), 0, "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); + if (!builder.add_string(webserver_url, len)) { + ESP_LOGW(TAG, "Response full; URL dropped"); + } break; } } #endif - this->send_response_(improv::build_rpc_response(improv::WIFI_SETTINGS, - std::vector(url_strings, url_strings + url_count))); + this->send_response_(builder.finish()); } else if (this->is_active() && this->state_ != improv::STATE_PROVISIONED) { ESP_LOGD(TAG, "WiFi provisioned externally"); } diff --git a/esphome/components/esp32_improv/esp32_improv_component.h b/esphome/components/esp32_improv/esp32_improv_component.h index d948dba3b3..414948c977 100644 --- a/esphome/components/esp32_improv/esp32_improv_component.h +++ b/esphome/components/esp32_improv/esp32_improv_component.h @@ -22,6 +22,7 @@ #include "esphome/components/output/binary_output.h" #endif +#include #include #ifdef USE_ESP32 @@ -109,7 +110,7 @@ class ESP32ImprovComponent final : public Component, public improv_base::ImprovB void set_state_(improv::State state, bool update_advertising = true); void set_error_(improv::Error error); improv::State get_initial_state_() const; - void send_response_(std::vector &&response); + void send_response_(std::span response); void process_incoming_data_(); void on_wifi_connect_timeout_(); void check_wifi_connection_(); diff --git a/esphome/components/improv_base/improv_base.cpp b/esphome/components/improv_base/improv_base.cpp index fa1b855d6c..1babeb5b5a 100644 --- a/esphome/components/improv_base/improv_base.cpp +++ b/esphome/components/improv_base/improv_base.cpp @@ -4,10 +4,13 @@ #include "esphome/components/network/util.h" #include "esphome/core/application.h" #include "esphome/core/defines.h" +#include "esphome/core/log.h" namespace esphome::improv_base { #if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) +static const char *const TAG = "improv_base"; + static constexpr const char DEVICE_NAME_PLACEHOLDER[] = "{{device_name}}"; static constexpr size_t DEVICE_NAME_PLACEHOLDER_LEN = sizeof(DEVICE_NAME_PLACEHOLDER) - 1; static constexpr const char IP_ADDRESS_PLACEHOLDER[] = "{{ip_address}}"; @@ -62,6 +65,21 @@ size_t ImprovBase::get_formatted_next_url_(char *buffer, size_t buffer_size) { *out = '\0'; return out - buffer; } + +void ImprovBase::add_next_url_(improv::RpcResponseBuilder &builder, size_t max_len) { + // The builder rejects strings above 254 bytes, so anything longer than this + // buffer could never be sent anyway + char url_buffer[256]; + size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); + if (len == 0) { + return; + } + // max_len is the transport's budget for this entry; skipping an over-long URL + // here keeps the rest of the response sendable instead of oversizing the frame + if (len > max_len || !builder.add_string(url_buffer, len)) { + ESP_LOGW(TAG, "Next URL too long; skipping"); + } +} #endif } // namespace esphome::improv_base diff --git a/esphome/components/improv_base/improv_base.h b/esphome/components/improv_base/improv_base.h index 9dded85a46..352bb75d5f 100644 --- a/esphome/components/improv_base/improv_base.h +++ b/esphome/components/improv_base/improv_base.h @@ -3,6 +3,10 @@ #include #include "esphome/core/defines.h" +#if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) +#include +#endif + namespace esphome::improv_base { class ImprovBase { @@ -15,6 +19,8 @@ class ImprovBase { #if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) /// Format next_url_ into buffer, replacing placeholders. Returns length written. size_t get_formatted_next_url_(char *buffer, size_t buffer_size); + /// Append the formatted next_url to the RPC response, warning if it does not fit. + void add_next_url_(improv::RpcResponseBuilder &builder, size_t max_len); const char *next_url_{nullptr}; #endif }; diff --git a/esphome/components/improv_serial/improv_serial_component.cpp b/esphome/components/improv_serial/improv_serial_component.cpp index 9c7745ee0a..0fb18e9b0d 100644 --- a/esphome/components/improv_serial/improv_serial_component.cpp +++ b/esphome/components/improv_serial/improv_serial_component.cpp @@ -9,6 +9,8 @@ #include "esphome/components/logger/logger.h" #include "esphome/components/wifi/scan_list.h" +#include + namespace esphome::improv_serial { static const char *const TAG = "improv_serial"; @@ -61,8 +63,7 @@ void ImprovSerialComponent::loop() { this->cancel_timeout("wifi-connect-timeout"); this->set_state_(improv::STATE_PROVISIONED); - std::vector url = this->build_rpc_settings_response_(improv::WIFI_SETTINGS); - this->send_response_(url); + this->send_settings_response_(improv::WIFI_SETTINGS); } } } @@ -142,16 +143,11 @@ void ImprovSerialComponent::write_data_(const uint8_t *data, const size_t size) #endif } -std::vector ImprovSerialComponent::build_rpc_settings_response_(improv::Command command) { - std::vector urls; +void ImprovSerialComponent::send_settings_response_(improv::Command command) { + std::array buf; + improv::RpcResponseBuilder builder(buf, command); #ifdef USE_IMPROV_SERIAL_NEXT_URL - { - char url_buffer[384]; - size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); - if (len > 0) { - urls.emplace_back(url_buffer, len); - } - } + this->add_next_url_(builder, MAX_NEXT_URL_LEN); #endif #ifdef USE_WEBSERVER for (auto &ip : wifi::global_wifi_component->wifi_sta_ip_addresses()) { @@ -160,25 +156,63 @@ std::vector ImprovSerialComponent::build_rpc_settings_response_(improv: ip.str_to(ip_buf); // "http://" (7) + IP (40) + ":" (1) + port (5) + null (1) = 54 char webserver_url[7 + network::IP_ADDRESS_BUFFER_SIZE + 1 + 5 + 1]; - snprintf(webserver_url, sizeof(webserver_url), "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); - urls.emplace_back(webserver_url); + // buf_append_printf keeps the format string in flash on ESP8266 + size_t len = + buf_append_printf(webserver_url, sizeof(webserver_url), 0, "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); + if (!builder.add_string(webserver_url, len)) { + ESP_LOGW(TAG, "Response full; URL dropped"); + } break; } } #endif - std::vector data = improv::build_rpc_response(command, urls, false); - return data; + this->send_response_(builder.finish(false)); } -std::vector ImprovSerialComponent::build_version_info_() { +void ImprovSerialComponent::send_version_info_() { +// Entry cost per field is sizeof(lit): a length byte plus the string #ifdef ESPHOME_PROJECT_NAME - std::vector infos = {ESPHOME_PROJECT_NAME, ESPHOME_PROJECT_VERSION, ESPHOME_VARIANT, App.get_name()}; + static constexpr size_t INFO_ENTRIES_LEN = + sizeof(ESPHOME_PROJECT_NAME) + sizeof(ESPHOME_PROJECT_VERSION) + sizeof(ESPHOME_VARIANT); #else - std::vector infos = {"ESPHome", ESPHOME_VERSION, ESPHOME_VARIANT, App.get_name()}; + static constexpr size_t INFO_ENTRIES_LEN = sizeof("ESPHome") + sizeof(ESPHOME_VERSION) + sizeof(ESPHOME_VARIANT); #endif - std::vector data = improv::build_rpc_response(improv::GET_DEVICE_INFO, infos, false); - return data; -}; + static_assert(INFO_ENTRIES_LEN < MAX_SERIAL_PAYLOAD, + "esphome project name and version too long for the improv_serial device info frame"); + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::GET_DEVICE_INFO); +#ifdef USE_ESP8266 + // Keep each literal in flash and copy it through an exact size stack buffer, + // so a long project name or version can never be truncated +#define IMPROV_ADD_INFO(lit) \ + do { \ + static const char progmem_str[] PROGMEM = lit; \ + char tmp[sizeof(lit)]; \ + progmem_memcpy(tmp, progmem_str, sizeof(lit)); \ + builder.add_string(tmp, sizeof(lit) - 1); \ + } while (0) +#else + // Literals are directly flash mapped on all other platforms +#define IMPROV_ADD_INFO(lit) builder.add_string(lit, sizeof(lit) - 1) +#endif +#ifdef ESPHOME_PROJECT_NAME + IMPROV_ADD_INFO(ESPHOME_PROJECT_NAME); + IMPROV_ADD_INFO(ESPHOME_PROJECT_VERSION); +#else + IMPROV_ADD_INFO("ESPHome"); + IMPROV_ADD_INFO(ESPHOME_VERSION); +#endif + IMPROV_ADD_INFO(ESPHOME_VARIANT); +#undef IMPROV_ADD_INFO + // Only the device name length is unknown at compile time + const auto &name = App.get_name(); + if (INFO_ENTRIES_LEN + 1 + name.size() <= MAX_SERIAL_PAYLOAD) { + builder.add_string(name.c_str(), name.size()); + } else { + ESP_LOGW(TAG, "Response full; device name dropped"); + } + this->send_response_(builder.finish(false)); +} bool ImprovSerialComponent::parse_improv_serial_byte_(uint8_t byte) { size_t at = this->rx_buffer_.size(); @@ -229,32 +263,35 @@ bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command } this->set_state_(this->state_); if (this->state_ == improv::STATE_PROVISIONED) { - std::vector url = this->build_rpc_settings_response_(improv::GET_CURRENT_STATE); - this->send_response_(url); + this->send_settings_response_(improv::GET_CURRENT_STATE); } return true; case improv::GET_DEVICE_INFO: { - std::vector info = this->build_version_info_(); - this->send_response_(info); + this->send_version_info_(); return true; } case improv::GET_WIFI_NETWORKS: { const auto &results = wifi::global_wifi_component->get_scan_result(); + std::array buf; for (const auto &scan : results) { bool with_auth = false; if (!wifi::should_show_scan_entry(results, scan, with_auth)) continue; // Send each ssid separately to avoid overflowing the buffer char rssi_buf[5]; // int8_t: -128 to 127, max 4 chars + null - *int8_to_str(rssi_buf, scan.get_rssi()) = '\0'; - std::vector data = improv::build_rpc_response( - improv::GET_WIFI_NETWORKS, {scan.get_ssid().str(), rssi_buf, YESNO(with_auth)}, false); - this->send_response_(data); + char *rssi_end = int8_to_str(rssi_buf, scan.get_rssi()); + *rssi_end = '\0'; + improv::RpcResponseBuilder builder(buf, improv::GET_WIFI_NETWORKS); + // SSID(32) + RSSI(4) + YESNO(3) entries always fit the payload + const auto &ssid = scan.get_ssid(); + builder.add_string(ssid.c_str(), ssid.size()); + builder.add_string(rssi_buf, rssi_end - rssi_buf); + builder.add_string(YESNO(with_auth)); + this->send_response_(builder.finish(false)); } // Send empty response to signify the end of the list. - std::vector data = - improv::build_rpc_response(improv::GET_WIFI_NETWORKS, std::vector{}, false); - this->send_response_(data); + improv::RpcResponseBuilder builder(buf, improv::GET_WIFI_NETWORKS); + this->send_response_(builder.finish(false)); return true; } default: { @@ -282,7 +319,14 @@ void ImprovSerialComponent::set_error_(improv::Error error) { this->write_data_(); } -void ImprovSerialComponent::send_response_(std::vector &response) { +void ImprovSerialComponent::send_response_(std::span response) { + // The serial frame length field is a single byte + if (response.size() > MAX_SERIAL_RESPONSE) { + ESP_LOGE(TAG, "Response too long"); + // Fail fast instead of leaving the client to wait out its timeout + this->set_error_(improv::ERROR_UNKNOWN); + return; + } this->tx_header_[TX_TYPE_IDX] = TYPE_RPC_RESPONSE; this->write_data_(response.data(), response.size()); } diff --git a/esphome/components/improv_serial/improv_serial_component.h b/esphome/components/improv_serial/improv_serial_component.h index 5a4eaaa945..692873bbb6 100644 --- a/esphome/components/improv_serial/improv_serial_component.h +++ b/esphome/components/improv_serial/improv_serial_component.h @@ -8,6 +8,7 @@ #include "esphome/core/helpers.h" #ifdef USE_WIFI #include +#include #include #ifdef USE_IMPROV_SERIAL_UART @@ -47,6 +48,22 @@ enum ImprovSerialType : uint8_t { static const uint16_t IMPROV_SERIAL_TIMEOUT = 100; static const uint8_t IMPROV_SERIAL_VERSION = 1; +// The serial frame length field is one byte +static constexpr size_t MAX_SERIAL_RESPONSE = 255; +// command + data length + trailing byte +static constexpr size_t RPC_RESPONSE_OVERHEAD = 3; +static constexpr size_t MAX_SERIAL_PAYLOAD = MAX_SERIAL_RESPONSE - RPC_RESPONSE_OVERHEAD; +#ifdef USE_WEBSERVER +// length byte + "http://" + IPv4 + ":" + port +static constexpr size_t WEBSERVER_URL_RESERVE = 1 + 7 + 15 + 1 + 5; +#else +static constexpr size_t WEBSERVER_URL_RESERVE = 0; +#endif +// Entry budget minus its own length byte +static constexpr size_t MAX_NEXT_URL_LEN = MAX_SERIAL_PAYLOAD - WEBSERVER_URL_RESERVE - 1; + +static_assert(MAX_SERIAL_RESPONSE <= improv::RPC_RESPONSE_MAX_SIZE, "builder buffer too small for the frame"); + class ImprovSerialComponent final : public Component, public improv_base::ImprovBase { public: void setup() override; @@ -66,11 +83,11 @@ class ImprovSerialComponent final : public Component, public improv_base::Improv void set_state_(improv::State state); void send_current_state_(improv::State state); void set_error_(improv::Error error); - void send_response_(std::vector &response); + void send_response_(std::span response); void on_wifi_connect_timeout_(); - std::vector build_rpc_settings_response_(improv::Command command); - std::vector build_version_info_(); + void send_settings_response_(improv::Command command); + void send_version_info_(); ESPHOME_ALWAYS_INLINE optional read_byte_() { optional byte; diff --git a/esphome/components/network/ip_address.h b/esphome/components/network/ip_address.h index f7aa7daf99..28f83cc4fa 100644 --- a/esphome/components/network/ip_address.h +++ b/esphome/components/network/ip_address.h @@ -188,6 +188,8 @@ struct IPAddress { } IPAddress(const std::string &in_address) { inet_aton(in_address.c_str(), &ip_addr_); } IPAddress(const ip_addr_t *other_ip) { ip_addr_ = *other_ip; } + bool is_ip4() const { return true; } + bool is_ip6() const { return false; } /// Write IP address to buffer. Buffer must be at least IP_ADDRESS_BUFFER_SIZE bytes. char *str_to(char *buf) const { inet_ntop(AF_INET, &ip_addr_, buf, IP_ADDRESS_BUFFER_SIZE); diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 993b9dce75..90ecfea72a 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -81,8 +81,6 @@ #define USE_HTTP_REQUEST_OTA_WATCHDOG_TIMEOUT 8000 // NOLINT #define USE_I2S_AUDIO_SPDIF_MODE #define USE_IMAGE -#define USE_IMPROV_SERIAL -#define USE_IMPROV_SERIAL_NEXT_URL #define USE_INFRARED #define USE_IR_RF #define USE_JSON @@ -223,6 +221,9 @@ #define USE_API_USER_DEFINED_ACTION_RESPONSES_JSON #define API_MAX_SEND_QUEUE 8 #define MAX_API_CONNECTIONS 6 +// The Improv library is not in the Zephyr tidy environment +#define USE_IMPROV_SERIAL +#define USE_IMPROV_SERIAL_NEXT_URL #define USE_MD5 #define USE_NOISE #define USE_SHA256 diff --git a/tests/components/improv_base/benchmark.yaml b/tests/components/improv_base/benchmark.yaml new file mode 100644 index 0000000000..8781e614a1 --- /dev/null +++ b/tests/components/improv_base/benchmark.yaml @@ -0,0 +1,6 @@ +# The builder test compares against the Improv library's build_rpc_response, +# so the library must be part of the unit test build. +# Keep the version in sync with the pin in esphome/components/improv_base/__init__.py. +esphome: + libraries: + - improv/Improv@1.2.7 diff --git a/tests/components/improv_base/rpc_response_builder_test.cpp b/tests/components/improv_base/rpc_response_builder_test.cpp new file mode 100644 index 0000000000..d9d0ad90d8 --- /dev/null +++ b/tests/components/improv_base/rpc_response_builder_test.cpp @@ -0,0 +1,102 @@ +#include + +#include +#include +#include +#include +#include + +#include + +namespace esphome::improv_base::testing { + +namespace { + +std::vector build_with_builder(improv::Command command, const std::vector &datum, + bool add_checksum) { + std::array buf; + improv::RpcResponseBuilder builder(buf, command); + for (const auto &str : datum) { + EXPECT_TRUE(builder.add_string(str.c_str(), str.size())); + } + auto out = builder.finish(add_checksum); + return {out.begin(), out.end()}; +} + +} // namespace + +// The serial path sends builder output where build_rpc_response bytes went before, +// so the two must match exactly, including the trailing 0x00 when checksums are off. +TEST(RpcResponseBuilder, ByteIdenticalToBuildRpcResponse) { + const std::vector device_info = {"ESPHome", "2026.9.0", "ESP32", "test-device"}; + const std::vector network = {"MySSID", "-67", "YES"}; + const std::vector empty = {}; + const std::vector max_payload = {std::string(254, 'x')}; + + for (bool add_checksum : {false, true}) { + for (const auto *datum : {&device_info, &network, &empty, &max_payload}) { + EXPECT_EQ(build_with_builder(improv::GET_DEVICE_INFO, *datum, add_checksum), + improv::build_rpc_response(improv::GET_DEVICE_INFO, *datum, add_checksum)); + } + } +} + +// Golden bytes independent of the library: command, data length, string entries, +// then the trailing byte (0x00 without checksum, additive checksum with). +TEST(RpcResponseBuilder, GoldenBytes) { + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {}, false), (std::vector{0x04, 0x00, 0x00})); + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {"ab"}, false), + (std::vector{0x04, 0x03, 0x02, 'a', 'b', 0x00})); + // Checksum: 0x04 + 0x03 + 0x02 + 'a' + 'b' = 0xCC + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {"ab"}, true), + (std::vector{0x04, 0x03, 0x02, 'a', 'b', 0xCC})); +} + +// esp32_improv calls finish() and build_rpc_response() with no checksum flag, +// so the two defaults must agree +TEST(RpcResponseBuilder, DefaultChecksumFlagMatches) { + const std::vector urls = {"https://example.com"}; + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::WIFI_SETTINGS); + for (const auto &str : urls) { + EXPECT_TRUE(builder.add_string(str.c_str(), str.size())); + } + auto out = builder.finish(); + EXPECT_EQ(std::vector(out.begin(), out.end()), improv::build_rpc_response(improv::WIFI_SETTINGS, urls)); +} + +TEST(RpcResponseBuilder, PayloadBudget) { + std::array buf; + + // 254 byte string fills the payload exactly; a second entry no longer fits + improv::RpcResponseBuilder full(buf, improv::GET_DEVICE_INFO); + const std::string big(254, 'x'); + EXPECT_TRUE(full.add_string(big.c_str(), big.size())); + EXPECT_FALSE(full.add_string("y", 1)); + + // 255 byte string can never fit (its length byte would exceed the budget) + improv::RpcResponseBuilder over(buf, improv::GET_DEVICE_INFO); + const std::string too_big(255, 'y'); + EXPECT_FALSE(over.add_string(too_big.c_str(), too_big.size())); + // A wildly out of range length must not wrap the position arithmetic + EXPECT_FALSE(over.add_string("z", static_cast(-1))); + auto out = over.finish(false); + EXPECT_EQ(std::vector(out.begin(), out.end()), (std::vector{0x03, 0x00, 0x00})); +} + +TEST(RpcResponseBuilder, FinishIsIdempotent) { + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::GET_DEVICE_INFO); + EXPECT_TRUE(builder.add_string("abc", 3)); + auto first = builder.finish(true); + const std::vector expected(first.begin(), first.end()); + + EXPECT_FALSE(builder.add_string("late", 4)); + auto again = builder.finish(true); + EXPECT_EQ(std::vector(again.begin(), again.end()), expected); + // The checksum flag on a later call is ignored + auto no_checksum = builder.finish(false); + EXPECT_EQ(std::vector(no_checksum.begin(), no_checksum.end()), expected); +} + +} // namespace esphome::improv_base::testing diff --git a/tests/components/improv_serial/common-uart0.yaml b/tests/components/improv_serial/common-uart0.yaml index 7b7730fd46..45bf1e5c33 100644 --- a/tests/components/improv_serial/common-uart0.yaml +++ b/tests/components/improv_serial/common-uart0.yaml @@ -5,4 +5,6 @@ wifi: logger: hardware_uart: UART0 +# next_url compiles the USE_IMPROV_SERIAL_NEXT_URL branch and add_next_url_ improv_serial: + next_url: https://example.com/?device_name={{device_name}}&ip_address={{ip_address}} diff --git a/tests/integration/fixtures/improv_serial_uart.yaml b/tests/integration/fixtures/improv_serial_uart.yaml index 75ffe97809..daa41c6a95 100644 --- a/tests/integration/fixtures/improv_serial_uart.yaml +++ b/tests/integration/fixtures/improv_serial_uart.yaml @@ -38,3 +38,5 @@ uart_mock: improv_serial: uart_id: mock_uart + # Deterministic on host: only the device name placeholder is used + next_url: https://example.com/?device={{device_name}} diff --git a/tests/integration/test_improv_serial_uart.py b/tests/integration/test_improv_serial_uart.py index 7dad5f74bd..75fb3263d6 100644 --- a/tests/integration/test_improv_serial_uart.py +++ b/tests/integration/test_improv_serial_uart.py @@ -133,8 +133,15 @@ async def test_improv_serial_uart( ) await waiter.wait_for("save_wifi_sta ssid=NewNet") await waiter.wait_for("uart_mock", f"TX 12 bytes: {state_frame_hex(0x04)}") - # Settings RPC response with no URLs: payload [0x01, 0x00, 0x00] and footer - await waiter.wait_for("uart_mock", "TX 3 bytes: 01:00:00") - await waiter.wait_for( - "uart_mock", f"TX 2 bytes: {rpc_footer_hex(bytes([0x01, 0x00, 0x00]))}" + # Settings RPC response carries the formatted next_url and its footer + next_url = b"https://example.com/?device=improv-uart" + payload = ( + bytes([CMD_WIFI_SETTINGS, len(next_url) + 1, len(next_url)]) + + next_url + + b"\x00" ) + await waiter.wait_for( + "uart_mock", + f"TX {len(payload)} bytes: " + ":".join(f"{b:02X}" for b in payload), + ) + await waiter.wait_for("uart_mock", f"TX 2 bytes: {rpc_footer_hex(payload)}")