From 4c2874a32b08001e27feb1d2b1970bdaca4a58ec Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 Aug 2025 15:37:01 -0500 Subject: [PATCH 1/6] [esphome] Fix OTA watchdog resets during port scanning and network delays (#10152) --- .../components/esphome/ota/ota_esphome.cpp | 227 ++++++++++++------ esphome/components/esphome/ota/ota_esphome.h | 9 +- 2 files changed, 158 insertions(+), 78 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 4cc82b9094..5217e9c61f 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -19,7 +19,9 @@ namespace esphome { static const char *const TAG = "esphome.ota"; -static constexpr u_int16_t OTA_BLOCK_SIZE = 8192; +static constexpr uint16_t OTA_BLOCK_SIZE = 8192; +static constexpr uint32_t OTA_SOCKET_TIMEOUT_HANDSHAKE = 10000; // milliseconds for initial handshake +static constexpr uint32_t OTA_SOCKET_TIMEOUT_DATA = 90000; // milliseconds for data transfer void ESPHomeOTAComponent::setup() { #ifdef USE_OTA_STATE_CALLBACK @@ -28,19 +30,19 @@ void ESPHomeOTAComponent::setup() { this->server_ = socket::socket_ip_loop_monitored(SOCK_STREAM, 0); // monitored for incoming connections if (this->server_ == nullptr) { - ESP_LOGW(TAG, "Could not create socket"); + this->log_socket_error_("creation"); this->mark_failed(); return; } int enable = 1; int err = this->server_->setsockopt(SOL_SOCKET, SO_REUSEADDR, &enable, sizeof(int)); if (err != 0) { - ESP_LOGW(TAG, "Socket unable to set reuseaddr: errno %d", err); + this->log_socket_error_("reuseaddr"); // we can still continue } err = this->server_->setblocking(false); if (err != 0) { - ESP_LOGW(TAG, "Socket unable to set nonblocking mode: errno %d", err); + this->log_socket_error_("non-blocking"); this->mark_failed(); return; } @@ -49,21 +51,21 @@ void ESPHomeOTAComponent::setup() { socklen_t sl = socket::set_sockaddr_any((struct sockaddr *) &server, sizeof(server), this->port_); if (sl == 0) { - ESP_LOGW(TAG, "Socket unable to set sockaddr: errno %d", errno); + this->log_socket_error_("set sockaddr"); this->mark_failed(); return; } err = this->server_->bind((struct sockaddr *) &server, sizeof(server)); if (err != 0) { - ESP_LOGW(TAG, "Socket unable to bind: errno %d", errno); + this->log_socket_error_("bind"); this->mark_failed(); return; } err = this->server_->listen(4); if (err != 0) { - ESP_LOGW(TAG, "Socket unable to listen: errno %d", errno); + this->log_socket_error_("listen"); this->mark_failed(); return; } @@ -83,17 +85,93 @@ void ESPHomeOTAComponent::dump_config() { } void ESPHomeOTAComponent::loop() { - // Skip handle_() call if no client connected and no incoming connections + // Skip handle_handshake_() call if no client connected and no incoming connections // This optimization reduces idle loop overhead when OTA is not active - // Note: No need to check server_ for null as the component is marked failed in setup() if server_ creation fails + // Note: No need to check server_ for null as the component is marked failed in setup() + // if server_ creation fails if (this->client_ != nullptr || this->server_->ready()) { - this->handle_(); + this->handle_handshake_(); } } static const uint8_t FEATURE_SUPPORTS_COMPRESSION = 0x01; -void ESPHomeOTAComponent::handle_() { +void ESPHomeOTAComponent::handle_handshake_() { + /// Handle the initial OTA handshake. + /// + /// This method is non-blocking and will return immediately if no data is available. + /// It waits for the first magic byte (0x6C) before proceeding to handle_data_(). + /// A 10-second timeout is enforced from initial connection. + + if (this->client_ == nullptr) { + // We already checked server_->ready() in loop(), so we can accept directly + struct sockaddr_storage source_addr; + socklen_t addr_len = sizeof(source_addr); + int enable = 1; + + this->client_ = this->server_->accept_loop_monitored((struct sockaddr *) &source_addr, &addr_len); + if (this->client_ == nullptr) + return; + int err = this->client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); + if (err != 0) { + this->log_socket_error_("nodelay"); + this->cleanup_connection_(); + return; + } + err = this->client_->setblocking(false); + if (err != 0) { + this->log_socket_error_("non-blocking"); + this->cleanup_connection_(); + return; + } + this->log_start_("handshake"); + this->client_connect_time_ = App.get_loop_component_start_time(); + } + + // Check for handshake timeout + uint32_t now = App.get_loop_component_start_time(); + if (now - this->client_connect_time_ > OTA_SOCKET_TIMEOUT_HANDSHAKE) { + ESP_LOGW(TAG, "Handshake timeout"); + this->cleanup_connection_(); + return; + } + + // Try to read first byte of magic bytes + uint8_t first_byte; + ssize_t read = this->client_->read(&first_byte, 1); + + if (read == -1 && (errno == EAGAIN || errno == EWOULDBLOCK)) { + return; // No data yet, try again next loop + } + + if (read <= 0) { + // Error or connection closed + if (read == -1) { + this->log_socket_error_("reading first byte"); + } else { + ESP_LOGW(TAG, "Remote closed during handshake"); + } + this->cleanup_connection_(); + return; + } + + // Got first byte, check if it's the magic byte + if (first_byte != 0x6C) { + ESP_LOGW(TAG, "Invalid initial byte: 0x%02X", first_byte); + this->cleanup_connection_(); + return; + } + + // First byte is valid, continue with data handling + this->handle_data_(); +} + +void ESPHomeOTAComponent::handle_data_() { + /// Handle the OTA data transfer and update process. + /// + /// This method is blocking and will not return until the OTA update completes, + /// fails, or times out. It handles authentication, receives the firmware data, + /// writes it to flash, and reboots on success. ota::OTAResponseTypes error_code = ota::OTA_RESPONSE_ERROR_UNKNOWN; bool update_started = false; size_t total = 0; @@ -108,38 +186,14 @@ void ESPHomeOTAComponent::handle_() { size_t size_acknowledged = 0; #endif - if (this->client_ == nullptr) { - // We already checked server_->ready() in loop(), so we can accept directly - struct sockaddr_storage source_addr; - socklen_t addr_len = sizeof(source_addr); - this->client_ = this->server_->accept((struct sockaddr *) &source_addr, &addr_len); - if (this->client_ == nullptr) - return; - } - - int enable = 1; - int err = this->client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); - if (err != 0) { - ESP_LOGW(TAG, "Socket could not enable TCP nodelay, errno %d", errno); - this->client_->close(); - this->client_ = nullptr; - return; - } - - ESP_LOGD(TAG, "Starting update from %s", this->client_->getpeername().c_str()); - this->status_set_warning(); -#ifdef USE_OTA_STATE_CALLBACK - this->state_callback_.call(ota::OTA_STARTED, 0.0f, 0); -#endif - - if (!this->readall_(buf, 5)) { - ESP_LOGW(TAG, "Reading magic bytes failed"); + // Read remaining 4 bytes of magic (we already read the first byte 0x6C in handle_handshake_) + if (!this->readall_(buf, 4)) { + this->log_read_error_("magic bytes"); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } - // 0x6C, 0x26, 0xF7, 0x5C, 0x45 - if (buf[0] != 0x6C || buf[1] != 0x26 || buf[2] != 0xF7 || buf[3] != 0x5C || buf[4] != 0x45) { - ESP_LOGW(TAG, "Magic bytes do not match! 0x%02X-0x%02X-0x%02X-0x%02X-0x%02X", buf[0], buf[1], buf[2], buf[3], - buf[4]); + // Check remaining magic bytes: 0x26, 0xF7, 0x5C, 0x45 + if (buf[0] != 0x26 || buf[1] != 0xF7 || buf[2] != 0x5C || buf[3] != 0x45) { + ESP_LOGW(TAG, "Magic bytes mismatch! 0x6C-0x%02X-0x%02X-0x%02X-0x%02X", buf[0], buf[1], buf[2], buf[3]); error_code = ota::OTA_RESPONSE_ERROR_MAGIC; goto error; // NOLINT(cppcoreguidelines-avoid-goto) } @@ -153,7 +207,7 @@ void ESPHomeOTAComponent::handle_() { // Read features - 1 byte if (!this->readall_(buf, 1)) { - ESP_LOGW(TAG, "Reading features failed"); + this->log_read_error_("features"); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } ota_features = buf[0]; // NOLINT @@ -232,7 +286,7 @@ void ESPHomeOTAComponent::handle_() { // Read size, 4 bytes MSB first if (!this->readall_(buf, 4)) { - ESP_LOGW(TAG, "Reading size failed"); + this->log_read_error_("size"); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } ota_size = 0; @@ -242,6 +296,17 @@ void ESPHomeOTAComponent::handle_() { } ESP_LOGV(TAG, "Size is %u bytes", ota_size); + // Now that we've passed authentication and are actually + // starting the update, set the warning status and notify + // listeners. This ensures that port scanners do not + // accidentally trigger the update process. + this->log_start_("update"); + this->status_set_warning(); +#ifdef USE_OTA_STATE_CALLBACK + this->state_callback_.call(ota::OTA_STARTED, 0.0f, 0); +#endif + + // This will block for a few seconds as it locks flash error_code = backend->begin(ota_size); if (error_code != ota::OTA_RESPONSE_OK) goto error; // NOLINT(cppcoreguidelines-avoid-goto) @@ -253,7 +318,7 @@ void ESPHomeOTAComponent::handle_() { // Read binary MD5, 32 bytes if (!this->readall_(buf, 32)) { - ESP_LOGW(TAG, "Reading binary MD5 checksum failed"); + this->log_read_error_("MD5 checksum"); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } sbuf[32] = '\0'; @@ -270,23 +335,22 @@ void ESPHomeOTAComponent::handle_() { ssize_t read = this->client_->read(buf, requested); if (read == -1) { if (errno == EAGAIN || errno == EWOULDBLOCK) { - App.feed_wdt(); - delay(1); + this->yield_and_feed_watchdog_(); continue; } - ESP_LOGW(TAG, "Error receiving data for update, errno %d", errno); + ESP_LOGW(TAG, "Read error, errno %d", errno); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } else if (read == 0) { // $ man recv // "When a stream socket peer has performed an orderly shutdown, the return value will // be 0 (the traditional "end-of-file" return)." - ESP_LOGW(TAG, "Remote end closed connection"); + ESP_LOGW(TAG, "Remote closed connection"); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } error_code = backend->write(buf, read); if (error_code != ota::OTA_RESPONSE_OK) { - ESP_LOGW(TAG, "Error writing binary data to flash!, error_code: %d", error_code); + ESP_LOGW(TAG, "Flash write error, code: %d", error_code); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } total += read; @@ -307,8 +371,7 @@ void ESPHomeOTAComponent::handle_() { this->state_callback_.call(ota::OTA_IN_PROGRESS, percentage, 0); #endif // feed watchdog and give other tasks a chance to run - App.feed_wdt(); - yield(); + this->yield_and_feed_watchdog_(); } } @@ -318,7 +381,7 @@ void ESPHomeOTAComponent::handle_() { error_code = backend->end(); if (error_code != ota::OTA_RESPONSE_OK) { - ESP_LOGW(TAG, "Error ending update! error_code: %d", error_code); + ESP_LOGW(TAG, "Error ending update! code: %d", error_code); goto error; // NOLINT(cppcoreguidelines-avoid-goto) } @@ -328,12 +391,11 @@ void ESPHomeOTAComponent::handle_() { // Read ACK if (!this->readall_(buf, 1) || buf[0] != ota::OTA_RESPONSE_OK) { - ESP_LOGW(TAG, "Reading back acknowledgement failed"); + this->log_read_error_("ack"); // do not go to error, this is not fatal } - this->client_->close(); - this->client_ = nullptr; + this->cleanup_connection_(); delay(10); ESP_LOGI(TAG, "Update complete"); this->status_clear_warning(); @@ -346,8 +408,7 @@ void ESPHomeOTAComponent::handle_() { error: buf[0] = static_cast(error_code); this->writeall_(buf, 1); - this->client_->close(); - this->client_ = nullptr; + this->cleanup_connection_(); if (backend != nullptr && update_started) { backend->abort(); @@ -364,28 +425,24 @@ bool ESPHomeOTAComponent::readall_(uint8_t *buf, size_t len) { uint32_t at = 0; while (len - at > 0) { uint32_t now = millis(); - if (now - start > 1000) { - ESP_LOGW(TAG, "Timed out reading %d bytes of data", len); + if (now - start > OTA_SOCKET_TIMEOUT_DATA) { + ESP_LOGW(TAG, "Timeout reading %d bytes", len); return false; } ssize_t read = this->client_->read(buf + at, len - at); if (read == -1) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { - App.feed_wdt(); - delay(1); - continue; + if (errno != EAGAIN && errno != EWOULDBLOCK) { + ESP_LOGW(TAG, "Error reading %d bytes, errno %d", len, errno); + return false; } - ESP_LOGW(TAG, "Failed to read %d bytes of data, errno %d", len, errno); - return false; } else if (read == 0) { ESP_LOGW(TAG, "Remote closed connection"); return false; } else { at += read; } - App.feed_wdt(); - delay(1); + this->yield_and_feed_watchdog_(); } return true; @@ -395,25 +452,21 @@ bool ESPHomeOTAComponent::writeall_(const uint8_t *buf, size_t len) { uint32_t at = 0; while (len - at > 0) { uint32_t now = millis(); - if (now - start > 1000) { - ESP_LOGW(TAG, "Timed out writing %d bytes of data", len); + if (now - start > OTA_SOCKET_TIMEOUT_DATA) { + ESP_LOGW(TAG, "Timeout writing %d bytes", len); return false; } ssize_t written = this->client_->write(buf + at, len - at); if (written == -1) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { - App.feed_wdt(); - delay(1); - continue; + if (errno != EAGAIN && errno != EWOULDBLOCK) { + ESP_LOGW(TAG, "Error writing %d bytes, errno %d", len, errno); + return false; } - ESP_LOGW(TAG, "Failed to write %d bytes of data, errno %d", len, errno); - return false; } else { at += written; } - App.feed_wdt(); - delay(1); + this->yield_and_feed_watchdog_(); } return true; } @@ -421,5 +474,25 @@ bool ESPHomeOTAComponent::writeall_(const uint8_t *buf, size_t len) { float ESPHomeOTAComponent::get_setup_priority() const { return setup_priority::AFTER_WIFI; } uint16_t ESPHomeOTAComponent::get_port() const { return this->port_; } void ESPHomeOTAComponent::set_port(uint16_t port) { this->port_ = port; } + +void ESPHomeOTAComponent::log_socket_error_(const char *msg) { ESP_LOGW(TAG, "Socket %s: errno %d", msg, errno); } + +void ESPHomeOTAComponent::log_read_error_(const char *what) { ESP_LOGW(TAG, "Read %s failed", what); } + +void ESPHomeOTAComponent::log_start_(const char *phase) { + ESP_LOGD(TAG, "Starting %s from %s", phase, this->client_->getpeername().c_str()); +} + +void ESPHomeOTAComponent::cleanup_connection_() { + this->client_->close(); + this->client_ = nullptr; + this->client_connect_time_ = 0; +} + +void ESPHomeOTAComponent::yield_and_feed_watchdog_() { + App.feed_wdt(); + delay(1); +} + } // namespace esphome #endif diff --git a/esphome/components/esphome/ota/ota_esphome.h b/esphome/components/esphome/ota/ota_esphome.h index e0d09ff37e..8397b86528 100644 --- a/esphome/components/esphome/ota/ota_esphome.h +++ b/esphome/components/esphome/ota/ota_esphome.h @@ -27,15 +27,22 @@ class ESPHomeOTAComponent : public ota::OTAComponent { uint16_t get_port() const; protected: - void handle_(); + void handle_handshake_(); + void handle_data_(); bool readall_(uint8_t *buf, size_t len); bool writeall_(const uint8_t *buf, size_t len); + void log_socket_error_(const char *msg); + void log_read_error_(const char *what); + void log_start_(const char *phase); + void cleanup_connection_(); + void yield_and_feed_watchdog_(); #ifdef USE_OTA_PASSWORD std::string password_; #endif // USE_OTA_PASSWORD uint16_t port_; + uint32_t client_connect_time_{0}; std::unique_ptr server_; std::unique_ptr client_; From 9aa21956c8af931f529d51bfb972d49292daa375 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 Aug 2025 15:41:08 -0500 Subject: [PATCH 2/6] [api] Optimize single vector writes to use write() instead of writev() (#10193) --- esphome/components/api/api_frame_helper.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index 6ca38e80ed..dee3af2ac3 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -156,7 +156,9 @@ APIError APIFrameHelper::write_raw_(const struct iovec *iov, int iovcnt, uint16_ } // Try to send directly if no buffered data - ssize_t sent = this->socket_->writev(iov, iovcnt); + // Optimize for single iovec case (common for plaintext API) + ssize_t sent = + (iovcnt == 1) ? this->socket_->write(iov[0].iov_base, iov[0].iov_len) : this->socket_->writev(iov, iovcnt); if (sent == -1) { APIError err = this->handle_socket_write_error_(); From 42aee53dde435611cf152f5fc88d53bf60ca9d48 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 Aug 2025 15:47:46 -0500 Subject: [PATCH 3/6] [bluetooth_proxy] Replace dynamic vector with fixed array for BLE advertisements (#10174) --- esphome/components/api/api.proto | 2 +- esphome/components/api/api_options.proto | 1 + esphome/components/api/api_pb2.cpp | 8 +- esphome/components/api/api_pb2.h | 5 +- esphome/components/api/api_pb2_dump.cpp | 4 +- .../components/bluetooth_proxy/__init__.py | 6 + .../bluetooth_proxy/bluetooth_proxy.cpp | 57 ++-------- .../bluetooth_proxy/bluetooth_proxy.h | 4 +- esphome/core/defines.h | 1 + script/api_protobuf/api_protobuf.py | 106 +++++++++++++++--- 10 files changed, 124 insertions(+), 70 deletions(-) diff --git a/esphome/components/api/api.proto b/esphome/components/api/api.proto index 9d77ecdfa8..6b19f2026a 100644 --- a/esphome/components/api/api.proto +++ b/esphome/components/api/api.proto @@ -1438,7 +1438,7 @@ message BluetoothLERawAdvertisementsResponse { option (ifdef) = "USE_BLUETOOTH_PROXY"; option (no_delay) = true; - repeated BluetoothLERawAdvertisement advertisements = 1; + repeated BluetoothLERawAdvertisement advertisements = 1 [(fixed_array_with_length_define) = "BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE"]; } enum BluetoothDeviceRequestType { diff --git a/esphome/components/api/api_options.proto b/esphome/components/api/api_options.proto index ed0e0d7455..50c43b96fd 100644 --- a/esphome/components/api/api_options.proto +++ b/esphome/components/api/api_options.proto @@ -30,6 +30,7 @@ extend google.protobuf.FieldOptions { optional bool no_zero_copy = 50008 [default=false]; optional bool fixed_array_skip_zero = 50009 [default=false]; optional string fixed_array_size_define = 50010; + optional string fixed_array_with_length_define = 50011; // container_pointer: Zero-copy optimization for repeated fields. // diff --git a/esphome/components/api/api_pb2.cpp b/esphome/components/api/api_pb2.cpp index 5dddc79b49..476e3c88d0 100644 --- a/esphome/components/api/api_pb2.cpp +++ b/esphome/components/api/api_pb2.cpp @@ -1843,12 +1843,14 @@ void BluetoothLERawAdvertisement::calculate_size(ProtoSize &size) const { size.add_length(1, this->data_len); } void BluetoothLERawAdvertisementsResponse::encode(ProtoWriteBuffer buffer) const { - for (auto &it : this->advertisements) { - buffer.encode_message(1, it, true); + for (uint16_t i = 0; i < this->advertisements_len; i++) { + buffer.encode_message(1, this->advertisements[i], true); } } void BluetoothLERawAdvertisementsResponse::calculate_size(ProtoSize &size) const { - size.add_repeated_message(1, this->advertisements); + for (uint16_t i = 0; i < this->advertisements_len; i++) { + size.add_message_object_force(1, this->advertisements[i]); + } } bool BluetoothDeviceRequest::decode_varint(uint32_t field_id, ProtoVarInt value) { switch (field_id) { diff --git a/esphome/components/api/api_pb2.h b/esphome/components/api/api_pb2.h index d43d3c61b7..edf839be55 100644 --- a/esphome/components/api/api_pb2.h +++ b/esphome/components/api/api_pb2.h @@ -1788,11 +1788,12 @@ class BluetoothLERawAdvertisement : public ProtoMessage { class BluetoothLERawAdvertisementsResponse : public ProtoMessage { public: static constexpr uint8_t MESSAGE_TYPE = 93; - static constexpr uint8_t ESTIMATED_SIZE = 34; + static constexpr uint8_t ESTIMATED_SIZE = 136; #ifdef HAS_PROTO_MESSAGE_DUMP const char *message_name() const override { return "bluetooth_le_raw_advertisements_response"; } #endif - std::vector advertisements{}; + std::array advertisements{}; + uint16_t advertisements_len{0}; void encode(ProtoWriteBuffer buffer) const override; void calculate_size(ProtoSize &size) const override; #ifdef HAS_PROTO_MESSAGE_DUMP diff --git a/esphome/components/api/api_pb2_dump.cpp b/esphome/components/api/api_pb2_dump.cpp index b212353ad8..7af322f96d 100644 --- a/esphome/components/api/api_pb2_dump.cpp +++ b/esphome/components/api/api_pb2_dump.cpp @@ -1534,9 +1534,9 @@ void BluetoothLERawAdvertisement::dump_to(std::string &out) const { } void BluetoothLERawAdvertisementsResponse::dump_to(std::string &out) const { MessageDumpHelper helper(out, "BluetoothLERawAdvertisementsResponse"); - for (const auto &it : this->advertisements) { + for (uint16_t i = 0; i < this->advertisements_len; i++) { out.append(" advertisements: "); - it.dump_to(out); + this->advertisements[i].dump_to(out); out.append("\n"); } } diff --git a/esphome/components/bluetooth_proxy/__init__.py b/esphome/components/bluetooth_proxy/__init__.py index fb7f7a37c0..112faa27e5 100644 --- a/esphome/components/bluetooth_proxy/__init__.py +++ b/esphome/components/bluetooth_proxy/__init__.py @@ -118,6 +118,12 @@ async def to_code(config): connection_count = len(config.get(CONF_CONNECTIONS, [])) cg.add_define("BLUETOOTH_PROXY_MAX_CONNECTIONS", connection_count) + # Define batch size for BLE advertisements + # Each advertisement is up to 80 bytes when packaged (including protocol overhead) + # 16 advertisements × 80 bytes (worst case) = 1280 bytes out of ~1320 bytes usable payload + # This achieves ~97% WiFi MTU utilization while staying under the limit + cg.add_define("BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE", 16) + for connection_conf in config.get(CONF_CONNECTIONS, []): connection_var = cg.new_Pvariable(connection_conf[CONF_ID]) await cg.register_component(connection_var, connection_conf) diff --git a/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp b/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp index 04b85fc3f0..723466a5ff 100644 --- a/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp +++ b/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp @@ -11,12 +11,8 @@ namespace esphome::bluetooth_proxy { static const char *const TAG = "bluetooth_proxy"; -// Batch size for BLE advertisements to maximize WiFi efficiency -// Each advertisement is up to 80 bytes when packaged (including protocol overhead) -// Most advertisements are 20-30 bytes, allowing even more to fit per packet -// 16 advertisements × 80 bytes (worst case) = 1280 bytes out of ~1320 bytes usable payload -// This achieves ~97% WiFi MTU utilization while staying under the limit -static constexpr size_t FLUSH_BATCH_SIZE = 16; +// BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE is defined during code generation +// It sets the batch size for BLE advertisements to maximize WiFi efficiency // Verify BLE advertisement data array size matches the BLE specification (31 bytes adv + 31 bytes scan response) static_assert(sizeof(((api::BluetoothLERawAdvertisement *) nullptr)->data) == 62, @@ -25,13 +21,6 @@ static_assert(sizeof(((api::BluetoothLERawAdvertisement *) nullptr)->data) == 62 BluetoothProxy::BluetoothProxy() { global_bluetooth_proxy = this; } void BluetoothProxy::setup() { - // Reserve capacity but start with size 0 - // Reserve 50% since we'll grow naturally and flush at FLUSH_BATCH_SIZE - this->response_.advertisements.reserve(FLUSH_BATCH_SIZE / 2); - - // Don't pre-allocate pool - let it grow only if needed in busy environments - // Many devices in quiet areas will never need the overflow pool - this->connections_free_response_.limit = BLUETOOTH_PROXY_MAX_CONNECTIONS; this->connections_free_response_.free = BLUETOOTH_PROXY_MAX_CONNECTIONS; @@ -88,33 +77,21 @@ bool BluetoothProxy::parse_devices(const esp32_ble::BLEScanResult *scan_results, auto &result = scan_results[i]; uint8_t length = result.adv_data_len + result.scan_rsp_len; - // Check if we need to expand the vector - if (this->advertisement_count_ >= advertisements.size()) { - if (this->advertisement_pool_.empty()) { - // No room in pool, need to allocate - advertisements.emplace_back(); - } else { - // Pull from pool - advertisements.push_back(std::move(this->advertisement_pool_.back())); - this->advertisement_pool_.pop_back(); - } - } - // Fill in the data directly at current position - auto &adv = advertisements[this->advertisement_count_]; + auto &adv = advertisements[this->response_.advertisements_len]; adv.address = esp32_ble::ble_addr_to_uint64(result.bda); adv.rssi = result.rssi; adv.address_type = result.ble_addr_type; adv.data_len = length; std::memcpy(adv.data, result.ble_adv, length); - this->advertisement_count_++; + this->response_.advertisements_len++; ESP_LOGV(TAG, "Queuing raw packet from %02X:%02X:%02X:%02X:%02X:%02X, length %d. RSSI: %d dB", result.bda[0], result.bda[1], result.bda[2], result.bda[3], result.bda[4], result.bda[5], length, result.rssi); - // Flush if we have reached FLUSH_BATCH_SIZE - if (this->advertisement_count_ >= FLUSH_BATCH_SIZE) { + // Flush if we have reached BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE + if (this->response_.advertisements_len >= BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE) { this->flush_pending_advertisements(); } } @@ -123,27 +100,17 @@ bool BluetoothProxy::parse_devices(const esp32_ble::BLEScanResult *scan_results, } void BluetoothProxy::flush_pending_advertisements() { - if (this->advertisement_count_ == 0 || !api::global_api_server->is_connected() || this->api_connection_ == nullptr) + if (this->response_.advertisements_len == 0 || !api::global_api_server->is_connected() || + this->api_connection_ == nullptr) return; - auto &advertisements = this->response_.advertisements; - - // Return any items beyond advertisement_count_ to the pool - if (advertisements.size() > this->advertisement_count_) { - // Move unused items back to pool - this->advertisement_pool_.insert(this->advertisement_pool_.end(), - std::make_move_iterator(advertisements.begin() + this->advertisement_count_), - std::make_move_iterator(advertisements.end())); - - // Resize to actual count - advertisements.resize(this->advertisement_count_); - } - // Send the message this->api_connection_->send_message(this->response_, api::BluetoothLERawAdvertisementsResponse::MESSAGE_TYPE); - // Reset count - existing items will be overwritten in next batch - this->advertisement_count_ = 0; + ESP_LOGV(TAG, "Sent batch of %u BLE advertisements", this->response_.advertisements_len); + + // Reset the length for the next batch + this->response_.advertisements_len = 0; } void BluetoothProxy::dump_config() { diff --git a/esphome/components/bluetooth_proxy/bluetooth_proxy.h b/esphome/components/bluetooth_proxy/bluetooth_proxy.h index 21695d9819..bc8d3ed762 100644 --- a/esphome/components/bluetooth_proxy/bluetooth_proxy.h +++ b/esphome/components/bluetooth_proxy/bluetooth_proxy.h @@ -150,7 +150,6 @@ class BluetoothProxy : public esp32_ble_tracker::ESPBTDeviceListener, public Com std::array connections_{}; // BLE advertisement batching - std::vector advertisement_pool_; api::BluetoothLERawAdvertisementsResponse response_; // Group 3: 4-byte types @@ -161,9 +160,8 @@ class BluetoothProxy : public esp32_ble_tracker::ESPBTDeviceListener, public Com // Group 4: 1-byte types grouped together bool active_; - uint8_t advertisement_count_{0}; uint8_t connection_count_{0}; - // 3 bytes used, 1 byte padding + // 2 bytes used, 2 bytes padding }; extern BluetoothProxy *global_bluetooth_proxy; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 7631ff54f3..01f6811e05 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -148,6 +148,7 @@ #define USE_BLUETOOTH_PROXY #define BLUETOOTH_PROXY_MAX_CONNECTIONS 3 +#define BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE 16 #define USE_CAPTIVE_PORTAL #define USE_ESP32_BLE #define USE_ESP32_BLE_CLIENT diff --git a/script/api_protobuf/api_protobuf.py b/script/api_protobuf/api_protobuf.py index fa2f87d98d..3396e5ad05 100755 --- a/script/api_protobuf/api_protobuf.py +++ b/script/api_protobuf/api_protobuf.py @@ -339,6 +339,11 @@ def create_field_type_info( ) -> TypeInfo: """Create the appropriate TypeInfo instance for a field, handling repeated fields and custom options.""" if field.label == 3: # repeated + # Check if this repeated field has fixed_array_with_length_define option + if ( + fixed_size := get_field_opt(field, pb.fixed_array_with_length_define) + ) is not None: + return FixedArrayWithLengthRepeatedType(field, fixed_size) # Check if this repeated field has fixed_array_size option if (fixed_size := get_field_opt(field, pb.fixed_array_size)) is not None: return FixedArrayRepeatedType(field, fixed_size) @@ -1052,7 +1057,7 @@ def _generate_array_dump_content( """ o = f"for (const auto {'' if is_bool else '&'}it : {field_name}) {{\n" # Check if underlying type can use dump_field - if type(ti).can_use_dump_field(): + if ti.can_use_dump_field(): # For types that have dump_field overloads, use them with extra indent o += f' dump_field(out, "{name}", {ti.dump_field_value("it")}, 4);\n' else: @@ -1084,6 +1089,12 @@ class FixedArrayRepeatedType(TypeInfo): validate_field_type(field.type, field.name) self._ti: TypeInfo = TYPE_INFO[field.type](field) + def _encode_element(self, element: str) -> str: + """Helper to generate encode statement for a single element.""" + if isinstance(self._ti, EnumType): + return f"buffer.{self._ti.encode_func}({self.number}, static_cast({element}), true);" + return f"buffer.{self._ti.encode_func}({self.number}, {element}, true);" + @property def cpp_type(self) -> str: return f"std::array<{self._ti.cpp_type}, {self.array_size}>" @@ -1111,19 +1122,13 @@ class FixedArrayRepeatedType(TypeInfo): @property def encode_content(self) -> str: - # Helper to generate encode statement for a single element - def encode_element(element: str) -> str: - if isinstance(self._ti, EnumType): - return f"buffer.{self._ti.encode_func}({self.number}, static_cast({element}), true);" - return f"buffer.{self._ti.encode_func}({self.number}, {element}, true);" - # If skip_zero is enabled, wrap encoding in a zero check if self.skip_zero: if self.is_define: # When using a define, we need to use a loop-based approach o = f"for (const auto &it : this->{self.field_name}) {{\n" o += " if (it != 0) {\n" - o += f" {encode_element('it')}\n" + o += f" {self._encode_element('it')}\n" o += " }\n" o += "}" return o @@ -1132,7 +1137,7 @@ class FixedArrayRepeatedType(TypeInfo): [f"this->{self.field_name}[{i}] != 0" for i in range(self.array_size)] ) encode_lines = [ - f" {encode_element(f'this->{self.field_name}[{i}]')}" + f" {self._encode_element(f'this->{self.field_name}[{i}]')}" for i in range(self.array_size) ] return f"if ({non_zero_checks}) {{\n" + "\n".join(encode_lines) + "\n}" @@ -1140,23 +1145,23 @@ class FixedArrayRepeatedType(TypeInfo): # When using a define, always use loop-based approach if self.is_define: o = f"for (const auto &it : this->{self.field_name}) {{\n" - o += f" {encode_element('it')}\n" + o += f" {self._encode_element('it')}\n" o += "}" return o # Unroll small arrays for efficiency if self.array_size == 1: - return encode_element(f"this->{self.field_name}[0]") + return self._encode_element(f"this->{self.field_name}[0]") if self.array_size == 2: return ( - encode_element(f"this->{self.field_name}[0]") + self._encode_element(f"this->{self.field_name}[0]") + "\n " - + encode_element(f"this->{self.field_name}[1]") + + self._encode_element(f"this->{self.field_name}[1]") ) # Use loops for larger arrays o = f"for (const auto &it : this->{self.field_name}) {{\n" - o += f" {encode_element('it')}\n" + o += f" {self._encode_element('it')}\n" o += "}" return o @@ -1230,6 +1235,66 @@ class FixedArrayRepeatedType(TypeInfo): return underlying_size * self.array_size +class FixedArrayWithLengthRepeatedType(FixedArrayRepeatedType): + """Special type for fixed-size repeated fields with variable length tracking. + + Similar to FixedArrayRepeatedType but generates an additional length field + to track how many elements are actually in use. Only encodes/sends elements + up to the current length. + + Fixed arrays with length are only supported for encoding (SOURCE_SERVER) since + we cannot control how many items we receive when decoding. + """ + + @property + def public_content(self) -> list[str]: + # Return both the array and the length field + return [ + f"{self.cpp_type} {self.field_name}{{}};", + f"uint16_t {self.field_name}_len{{0}};", + ] + + @property + def encode_content(self) -> str: + # Always use a loop up to the current length + o = f"for (uint16_t i = 0; i < this->{self.field_name}_len; i++) {{\n" + o += f" {self._encode_element(f'this->{self.field_name}[i]')}\n" + o += "}" + return o + + @property + def dump_content(self) -> str: + # Dump only the active elements + o = f"for (uint16_t i = 0; i < this->{self.field_name}_len; i++) {{\n" + # Check if underlying type can use dump_field + if self._ti.can_use_dump_field(): + o += f' dump_field(out, "{self.name}", {self._ti.dump_field_value(f"this->{self.field_name}[i]")}, 4);\n' + else: + o += f' out.append(" {self.name}: ");\n' + o += indent(self._ti.dump(f"this->{self.field_name}[i]")) + "\n" + o += ' out.append("\\n");\n' + o += "}" + return o + + def get_size_calculation(self, name: str, force: bool = False) -> str: + # Calculate size only for active elements + o = f"for (uint16_t i = 0; i < {name}_len; i++) {{\n" + o += f" {self._ti.get_size_calculation(f'{name}[i]', True)}\n" + o += "}" + return o + + def get_estimated_size(self) -> int: + # For fixed arrays with length, estimate based on typical usage + # Assume on average half the array is used + underlying_size = self._ti.get_estimated_size() + if self.is_define: + # When using a define, estimate 8 elements as typical + return underlying_size * 8 + return underlying_size * ( + self.array_size // 2 if self.array_size > 2 else self.array_size + ) + + class RepeatedTypeInfo(TypeInfo): def __init__(self, field: descriptor.FieldDescriptorProto) -> None: super().__init__(field) @@ -1711,6 +1776,19 @@ def build_message_type( f"since we cannot trust or control the number of items received from clients." ) + # Validate that fixed_array_with_length_define is only used in encode-only messages + if ( + needs_decode + and field.label == 3 + and get_field_opt(field, pb.fixed_array_with_length_define) is not None + ): + raise ValueError( + f"Message '{desc.name}' uses fixed_array_with_length_define on field '{field.name}' " + f"but has source={SOURCE_NAMES[source]}. " + f"Fixed arrays with length are only supported for SOURCE_SERVER (encode-only) messages " + f"since we cannot trust or control the number of items received from clients." + ) + ti = create_field_type_info(field, needs_decode, needs_encode) # Skip field declarations for fields that are in the base class From c14c4fb658dae18deb53fe425ea18a476f5572e9 Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Tue, 12 Aug 2025 09:12:54 +1200 Subject: [PATCH 4/6] [substitutions] Add some safe built-in functions to jinja parsing (#10178) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- esphome/components/substitutions/jinja.py | 24 ++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/esphome/components/substitutions/jinja.py b/esphome/components/substitutions/jinja.py index cf393d2a5d..c6e40a668d 100644 --- a/esphome/components/substitutions/jinja.py +++ b/esphome/components/substitutions/jinja.py @@ -25,6 +25,24 @@ def has_jinja(st): return detect_jinja_re.search(st) is not None +# SAFE_GLOBAL_FUNCTIONS defines a allowlist of built-in functions that are considered safe to expose +# in Jinja templates or other sandboxed evaluation contexts. Only functions that do not allow +# arbitrary code execution, file access, or other security risks are included. +# +# The following functions are considered safe: +# - ord: Converts a character to its Unicode code point integer. +# - chr: Converts an integer to its corresponding Unicode character. +# - len: Returns the length of a sequence or collection. +# +# These functions were chosen because they are pure, have no side effects, and do not provide access +# to the file system, environment, or other potentially sensitive resources. +SAFE_GLOBAL_FUNCTIONS = { + "ord": ord, + "chr": chr, + "len": len, +} + + class JinjaStr(str): """ Wraps a string containing an unresolved Jinja expression, @@ -66,7 +84,11 @@ class Jinja: self.env.add_extension("jinja2.ext.do") self.env.globals["math"] = math # Inject entire math module self.context_vars = {**context_vars} - self.env.globals = {**self.env.globals, **self.context_vars} + self.env.globals = { + **self.env.globals, + **self.context_vars, + **SAFE_GLOBAL_FUNCTIONS, + } def expand(self, content_str): """ From 82b7c1224c17d533db47c92ace97e27a11433eed Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 Aug 2025 16:58:51 -0500 Subject: [PATCH 5/6] [core] Improve entity duplicate validation error messages (#10184) --- esphome/config.py | 4 +- esphome/core/__init__.py | 22 +++++-- esphome/core/entity_helpers.py | 37 ++++++++++- esphome/types.py | 12 ++++ tests/unit_tests/core/test_entity_helpers.py | 66 ++++++++++++++++++- .../entity_conflict_components.yaml | 20 ++++++ 6 files changed, 151 insertions(+), 10 deletions(-) create mode 100644 tests/unit_tests/fixtures/core/entity_helpers/entity_conflict_components.yaml diff --git a/esphome/config.py b/esphome/config.py index cf7a232d8e..ecd0cbb048 100644 --- a/esphome/config.py +++ b/esphome/config.py @@ -627,13 +627,15 @@ class SchemaValidationStep(ConfigValidationStep): def __init__( self, domain: str, path: ConfigPath, conf: ConfigType, comp: ComponentManifest ): + self.domain = domain self.path = path self.conf = conf self.comp = comp def run(self, result: Config) -> None: token = path_context.set(self.path) - with result.catch_error(self.path): + # The domain already contains the full component path (e.g., "sensor.template", "sensor.uptime") + with CORE.component_context(self.domain), result.catch_error(self.path): if self.comp.is_platform: # Remove 'platform' key for validation input_conf = OrderedDict(self.conf) diff --git a/esphome/core/__init__.py b/esphome/core/__init__.py index 39c6c3def1..9df5da1c78 100644 --- a/esphome/core/__init__.py +++ b/esphome/core/__init__.py @@ -1,4 +1,5 @@ from collections import defaultdict +from contextlib import contextmanager import logging import math import os @@ -38,7 +39,7 @@ from esphome.util import OrderedDict if TYPE_CHECKING: from ..cpp_generator import MockObj, MockObjClass, Statement - from ..types import ConfigType + from ..types import ConfigType, EntityMetadata _LOGGER = logging.getLogger(__name__) @@ -571,14 +572,16 @@ class EsphomeCore: # Key: platform name (e.g. "sensor", "binary_sensor"), Value: count self.platform_counts: defaultdict[str, int] = defaultdict(int) # Track entity unique IDs to handle duplicates - # Set of (device_id, platform, sanitized_name) tuples - self.unique_ids: set[tuple[str, str, str]] = set() + # Dict mapping (device_id, platform, sanitized_name) -> entity metadata + self.unique_ids: dict[tuple[str, str, str], EntityMetadata] = {} # Whether ESPHome was started in verbose mode self.verbose = False # Whether ESPHome was started in quiet mode self.quiet = False # A list of all known ID classes self.id_classes = {} + # The current component being processed during validation + self.current_component: str | None = None def reset(self): from esphome.pins import PIN_SCHEMA_REGISTRY @@ -604,9 +607,20 @@ class EsphomeCore: self.loaded_integrations = set() self.component_ids = set() self.platform_counts = defaultdict(int) - self.unique_ids = set() + self.unique_ids = {} + self.current_component = None PIN_SCHEMA_REGISTRY.reset() + @contextmanager + def component_context(self, component: str): + """Context manager to set the current component being processed.""" + old_component = self.current_component + self.current_component = component + try: + yield + finally: + self.current_component = old_component + @property def address(self) -> str | None: if self.config is None: diff --git a/esphome/core/entity_helpers.py b/esphome/core/entity_helpers.py index cc388ffb4c..107b9fd739 100644 --- a/esphome/core/entity_helpers.py +++ b/esphome/core/entity_helpers.py @@ -16,7 +16,7 @@ from esphome.core import CORE, ID from esphome.cpp_generator import MockObj, add, get_variable import esphome.final_validate as fv from esphome.helpers import sanitize, snake_case -from esphome.types import ConfigType +from esphome.types import ConfigType, EntityMetadata _LOGGER = logging.getLogger(__name__) @@ -214,14 +214,45 @@ def entity_duplicate_validator(platform: str) -> Callable[[ConfigType], ConfigTy # Check for duplicates unique_key = (device_id, platform, name_key) if unique_key in CORE.unique_ids: + # Get the existing entity metadata + existing = CORE.unique_ids[unique_key] + existing_name = existing.get("name", entity_name) + existing_device = existing.get("device_id", "") + existing_id = existing.get("entity_id", "unknown") + + # Build detailed error message device_prefix = f" on device '{device_id}'" if device_id else "" + existing_device_prefix = ( + f" on device '{existing_device}'" if existing_device else "" + ) + existing_component = existing.get("component", "unknown") + + # Provide more context about where the duplicate was found + conflict_msg = ( + f"Conflicts with entity '{existing_name}'{existing_device_prefix}" + ) + if existing_id != "unknown": + conflict_msg += f" (id: {existing_id})" + if existing_component != "unknown": + conflict_msg += f" from component '{existing_component}'" + raise cv.Invalid( f"Duplicate {platform} entity with name '{entity_name}' found{device_prefix}. " + f"{conflict_msg}. " f"Each entity on a device must have a unique name within its platform." ) - # Add to tracking set - CORE.unique_ids.add(unique_key) + # Store metadata about this entity + entity_metadata: EntityMetadata = { + "name": entity_name, + "device_id": device_id, + "platform": platform, + "entity_id": str(config.get(CONF_ID, "unknown")), + "component": CORE.current_component or "unknown", + } + + # Add to tracking dict + CORE.unique_ids[unique_key] = entity_metadata return config return validator diff --git a/esphome/types.py b/esphome/types.py index f68f503993..62499a953c 100644 --- a/esphome/types.py +++ b/esphome/types.py @@ -1,5 +1,7 @@ """This helper module tracks commonly used types in the esphome python codebase.""" +from typing import TypedDict + from esphome.core import ID, EsphomeCore, Lambda ConfigFragmentType = ( @@ -16,3 +18,13 @@ ConfigFragmentType = ( ConfigType = dict[str, ConfigFragmentType] CoreType = EsphomeCore ConfigPathType = str | int + + +class EntityMetadata(TypedDict): + """Metadata stored for each entity to help with duplicate detection.""" + + name: str + device_id: str + platform: str + entity_id: str + component: str diff --git a/tests/unit_tests/core/test_entity_helpers.py b/tests/unit_tests/core/test_entity_helpers.py index c639ad94b2..2157bc20a9 100644 --- a/tests/unit_tests/core/test_entity_helpers.py +++ b/tests/unit_tests/core/test_entity_helpers.py @@ -12,6 +12,7 @@ from esphome.const import ( CONF_DEVICE_ID, CONF_DISABLED_BY_DEFAULT, CONF_ICON, + CONF_ID, CONF_INTERNAL, CONF_NAME, ) @@ -511,12 +512,18 @@ def test_entity_duplicate_validator() -> None: validated1 = validator(config1) assert validated1 == config1 assert ("", "sensor", "temperature") in CORE.unique_ids + # Check metadata was stored + metadata = CORE.unique_ids[("", "sensor", "temperature")] + assert metadata["name"] == "Temperature" + assert metadata["platform"] == "sensor" # Second entity with different name should pass config2 = {CONF_NAME: "Humidity"} validated2 = validator(config2) assert validated2 == config2 assert ("", "sensor", "humidity") in CORE.unique_ids + metadata2 = CORE.unique_ids[("", "sensor", "humidity")] + assert metadata2["name"] == "Humidity" # Duplicate entity should fail config3 = {CONF_NAME: "Temperature"} @@ -540,11 +547,15 @@ def test_entity_duplicate_validator_with_devices() -> None: validated1 = validator(config1) assert validated1 == config1 assert ("device1", "sensor", "temperature") in CORE.unique_ids + metadata1 = CORE.unique_ids[("device1", "sensor", "temperature")] + assert metadata1["device_id"] == "device1" config2 = {CONF_NAME: "Temperature", CONF_DEVICE_ID: device2} validated2 = validator(config2) assert validated2 == config2 assert ("device2", "sensor", "temperature") in CORE.unique_ids + metadata2 = CORE.unique_ids[("device2", "sensor", "temperature")] + assert metadata2["device_id"] == "device2" # Duplicate on same device should fail config3 = {CONF_NAME: "Temperature", CONF_DEVICE_ID: device1} @@ -595,6 +606,54 @@ def test_entity_different_platforms_yaml_validation( assert result is not None +def test_entity_duplicate_validator_error_message() -> None: + """Test that duplicate entity error messages include helpful metadata.""" + # Create validator for sensor platform + validator = entity_duplicate_validator("sensor") + + # Set current component to simulate validation context for uptime sensor + CORE.current_component = "sensor.uptime" + + # First entity should pass + config1 = {CONF_NAME: "Battery", CONF_ID: ID("battery_1")} + validated1 = validator(config1) + assert validated1 == config1 + + # Reset component to simulate template sensor + CORE.current_component = "sensor.template" + + # Duplicate entity should fail with detailed error + config2 = {CONF_NAME: "Battery", CONF_ID: ID("battery_2")} + with pytest.raises( + Invalid, + match=r"Duplicate sensor entity with name 'Battery' found.*" + r"Conflicts with entity 'Battery' \(id: battery_1\) from component 'sensor\.uptime'", + ): + validator(config2) + + # Clean up + CORE.current_component = None + + +def test_entity_conflict_between_components_yaml( + yaml_file: Callable[[str], str], capsys: pytest.CaptureFixture[str] +) -> None: + """Test that conflicts between different components show helpful error messages.""" + result = load_config_from_fixture( + yaml_file, "entity_conflict_components.yaml", FIXTURES_DIR + ) + assert result is None + + # Check for the enhanced error message + captured = capsys.readouterr() + # The error should mention both the conflict and which component created it + assert "Duplicate sensor entity with name 'Battery' found" in captured.out + # Should mention it conflicts with an entity from a specific sensor platform + assert "from component 'sensor." in captured.out + # Should show it's a conflict between wifi_signal and template + assert "sensor.wifi_signal" in captured.out or "sensor.template" in captured.out + + def test_entity_duplicate_validator_internal_entities() -> None: """Test that internal entities are excluded from duplicate name validation.""" # Create validator for sensor platform @@ -612,14 +671,17 @@ def test_entity_duplicate_validator_internal_entities() -> None: validated2 = validator(config2) assert validated2 == config2 # Internal entity should not be added to unique_ids - assert len([k for k in CORE.unique_ids if k == ("", "sensor", "temperature")]) == 1 + # Count how many times the key appears (should still be 1) + count = sum(1 for k in CORE.unique_ids if k == ("", "sensor", "temperature")) + assert count == 1 # Another internal entity with same name should also pass config3 = {CONF_NAME: "Temperature", CONF_INTERNAL: True} validated3 = validator(config3) assert validated3 == config3 # Still only one entry in unique_ids (from the non-internal entity) - assert len([k for k in CORE.unique_ids if k == ("", "sensor", "temperature")]) == 1 + count = sum(1 for k in CORE.unique_ids if k == ("", "sensor", "temperature")) + assert count == 1 # Non-internal entity with same name should fail config4 = {CONF_NAME: "Temperature"} diff --git a/tests/unit_tests/fixtures/core/entity_helpers/entity_conflict_components.yaml b/tests/unit_tests/fixtures/core/entity_helpers/entity_conflict_components.yaml new file mode 100644 index 0000000000..6a1df0f7b4 --- /dev/null +++ b/tests/unit_tests/fixtures/core/entity_helpers/entity_conflict_components.yaml @@ -0,0 +1,20 @@ +esphome: + name: test-device + +esp32: + board: esp32dev + +# Uptime sensor +sensor: + - platform: uptime + name: "Battery" + id: uptime_battery + +# Template sensor also named "Battery" - this should conflict + - platform: template + name: "Battery" + id: template_battery + lambda: |- + return 95.0; + unit_of_measurement: "%" + update_interval: 60s From ff52869b4c0d6552d17187fe773a702a8f162677 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 11 Aug 2025 17:10:38 -0500 Subject: [PATCH 6/6] [api] Add constexpr optimizations to protobuf encoding (#10192) --- esphome/components/api/proto.h | 76 ++++++++++++++++------------------ 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/esphome/components/api/proto.h b/esphome/components/api/proto.h index 5c174b679c..0e5ec61050 100644 --- a/esphome/components/api/proto.h +++ b/esphome/components/api/proto.h @@ -15,6 +15,23 @@ namespace esphome::api { +// Helper functions for ZigZag encoding/decoding +inline constexpr uint32_t encode_zigzag32(int32_t value) { + return (static_cast(value) << 1) ^ (static_cast(value >> 31)); +} + +inline constexpr uint64_t encode_zigzag64(int64_t value) { + return (static_cast(value) << 1) ^ (static_cast(value >> 63)); +} + +inline constexpr int32_t decode_zigzag32(uint32_t value) { + return (value & 1) ? static_cast(~(value >> 1)) : static_cast(value >> 1); +} + +inline constexpr int64_t decode_zigzag64(uint64_t value) { + return (value & 1) ? static_cast(~(value >> 1)) : static_cast(value >> 1); +} + /* * StringRef Ownership Model for API Protocol Messages * =================================================== @@ -87,33 +104,25 @@ class ProtoVarInt { return {}; // Incomplete or invalid varint } - uint16_t as_uint16() const { return this->value_; } - uint32_t as_uint32() const { return this->value_; } - uint64_t as_uint64() const { return this->value_; } - bool as_bool() const { return this->value_; } - int32_t as_int32() const { + constexpr uint16_t as_uint16() const { return this->value_; } + constexpr uint32_t as_uint32() const { return this->value_; } + constexpr uint64_t as_uint64() const { return this->value_; } + constexpr bool as_bool() const { return this->value_; } + constexpr int32_t as_int32() const { // Not ZigZag encoded return static_cast(this->as_int64()); } - int64_t as_int64() const { + constexpr int64_t as_int64() const { // Not ZigZag encoded return static_cast(this->value_); } - int32_t as_sint32() const { + constexpr int32_t as_sint32() const { // with ZigZag encoding - if (this->value_ & 1) { - return static_cast(~(this->value_ >> 1)); - } else { - return static_cast(this->value_ >> 1); - } + return decode_zigzag32(static_cast(this->value_)); } - int64_t as_sint64() const { + constexpr int64_t as_sint64() const { // with ZigZag encoding - if (this->value_ & 1) { - return static_cast(~(this->value_ >> 1)); - } else { - return static_cast(this->value_ >> 1); - } + return decode_zigzag64(this->value_); } /** * Encode the varint value to a pre-allocated buffer without bounds checking. @@ -309,22 +318,10 @@ class ProtoWriteBuffer { this->encode_uint64(field_id, static_cast(value), force); } void encode_sint32(uint32_t field_id, int32_t value, bool force = false) { - uint32_t uvalue; - if (value < 0) { - uvalue = ~(value << 1); - } else { - uvalue = value << 1; - } - this->encode_uint32(field_id, uvalue, force); + this->encode_uint32(field_id, encode_zigzag32(value), force); } void encode_sint64(uint32_t field_id, int64_t value, bool force = false) { - uint64_t uvalue; - if (value < 0) { - uvalue = ~(value << 1); - } else { - uvalue = value << 1; - } - this->encode_uint64(field_id, uvalue, force); + this->encode_uint64(field_id, encode_zigzag64(value), force); } void encode_message(uint32_t field_id, const ProtoMessage &value, bool force = false); std::vector *get_buffer() const { return buffer_; } @@ -395,7 +392,7 @@ class ProtoSize { * @param value The uint32_t value to calculate size for * @return The number of bytes needed to encode the value */ - static inline uint32_t varint(uint32_t value) { + static constexpr uint32_t varint(uint32_t value) { // Optimized varint size calculation using leading zeros // Each 7 bits requires one byte in the varint encoding if (value < 128) @@ -419,7 +416,7 @@ class ProtoSize { * @param value The uint64_t value to calculate size for * @return The number of bytes needed to encode the value */ - static inline uint32_t varint(uint64_t value) { + static constexpr uint32_t varint(uint64_t value) { // Handle common case of values fitting in uint32_t (vast majority of use cases) if (value <= UINT32_MAX) { return varint(static_cast(value)); @@ -450,7 +447,7 @@ class ProtoSize { * @param value The int32_t value to calculate size for * @return The number of bytes needed to encode the value */ - static inline uint32_t varint(int32_t value) { + static constexpr uint32_t varint(int32_t value) { // Negative values are sign-extended to 64 bits in protocol buffers, // which always results in a 10-byte varint for negative int32 if (value < 0) { @@ -466,7 +463,7 @@ class ProtoSize { * @param value The int64_t value to calculate size for * @return The number of bytes needed to encode the value */ - static inline uint32_t varint(int64_t value) { + static constexpr uint32_t varint(int64_t value) { // For int64_t, we convert to uint64_t and calculate the size // This works because the bit pattern determines the encoding size, // and we've handled negative int32 values as a special case above @@ -480,7 +477,7 @@ class ProtoSize { * @param type The wire type value (from the WireType enum in the protobuf spec) * @return The number of bytes needed to encode the field ID and wire type */ - static inline uint32_t field(uint32_t field_id, uint32_t type) { + static constexpr uint32_t field(uint32_t field_id, uint32_t type) { uint32_t tag = (field_id << 3) | (type & 0b111); return varint(tag); } @@ -607,9 +604,8 @@ class ProtoSize { */ inline void add_sint32_force(uint32_t field_id_size, int32_t value) { // Always calculate size when force is true - // ZigZag encoding for sint32: (n << 1) ^ (n >> 31) - uint32_t zigzag = (static_cast(value) << 1) ^ (static_cast(value >> 31)); - total_size_ += field_id_size + varint(zigzag); + // ZigZag encoding for sint32 + total_size_ += field_id_size + varint(encode_zigzag32(value)); } /**