From 2ad266582f61db675f0bd4b84b9b1927bfd211b8 Mon Sep 17 00:00:00 2001 From: Gustavo Ambrozio Date: Mon, 23 Jun 2025 00:40:07 -1000 Subject: [PATCH 01/20] [online_image] Allow suppressing update on url change (#8885) --- esphome/components/online_image/__init__.py | 5 +++++ esphome/components/online_image/online_image.h | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/esphome/components/online_image/__init__.py b/esphome/components/online_image/__init__.py index 9380cf1b1b..3f15db6e50 100644 --- a/esphome/components/online_image/__init__.py +++ b/esphome/components/online_image/__init__.py @@ -34,6 +34,7 @@ MULTI_CONF = True CONF_ON_DOWNLOAD_FINISHED = "on_download_finished" CONF_PLACEHOLDER = "placeholder" +CONF_UPDATE = "update" _LOGGER = logging.getLogger(__name__) @@ -167,6 +168,7 @@ SET_URL_SCHEMA = cv.Schema( { cv.GenerateID(): cv.use_id(OnlineImage), cv.Required(CONF_URL): cv.templatable(cv.url), + cv.Optional(CONF_UPDATE, default=True): cv.templatable(bool), } ) @@ -188,6 +190,9 @@ async def online_image_action_to_code(config, action_id, template_arg, args): if CONF_URL in config: template_ = await cg.templatable(config[CONF_URL], args, cg.std_string) cg.add(var.set_url(template_)) + if CONF_UPDATE in config: + template_ = await cg.templatable(config[CONF_UPDATE], args, bool) + cg.add(var.set_update(template_)) return var diff --git a/esphome/components/online_image/online_image.h b/esphome/components/online_image/online_image.h index 6ed9c7956f..6a2144538f 100644 --- a/esphome/components/online_image/online_image.h +++ b/esphome/components/online_image/online_image.h @@ -201,9 +201,12 @@ template class OnlineImageSetUrlAction : public Action { public: OnlineImageSetUrlAction(OnlineImage *parent) : parent_(parent) {} TEMPLATABLE_VALUE(std::string, url) + TEMPLATABLE_VALUE(bool, update) void play(Ts... x) override { this->parent_->set_url(this->url_.value(x...)); - this->parent_->update(); + if (this->update_.value(x...)) { + this->parent_->update(); + } } protected: From a1aebe6a2cec45290a7c8e51f58b32285ae0cdaa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 11:57:49 +0200 Subject: [PATCH 02/20] Eliminate memory fragmentation with BLE event pool (#9101) --- esphome/components/esp32_ble/ble.cpp | 55 ++-- esphome/components/esp32_ble/ble.h | 2 + esphome/components/esp32_ble/ble_event.h | 275 +++++++++++------- esphome/components/esp32_ble/ble_event_pool.h | 72 +++++ esphome/components/esp32_ble/queue.h | 25 +- 5 files changed, 288 insertions(+), 141 deletions(-) create mode 100644 esphome/components/esp32_ble/ble_event_pool.h diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index 8adef79d2f..5a66f11d0f 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -1,6 +1,7 @@ #ifdef USE_ESP32 #include "ble.h" +#include "ble_event_pool.h" #include "esphome/core/application.h" #include "esphome/core/log.h" @@ -23,9 +24,6 @@ namespace esp32_ble { static const char *const TAG = "esp32_ble"; -static RAMAllocator EVENT_ALLOCATOR( // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) - RAMAllocator::ALLOW_FAILURE | RAMAllocator::ALLOC_INTERNAL); - void ESP32BLE::setup() { global_ble = this; ESP_LOGCONFIG(TAG, "Running setup"); @@ -349,9 +347,8 @@ void ESP32BLE::loop() { default: break; } - // Destructor will clean up external allocations for GATTC/GATTS - ble_event->~BLEEvent(); - EVENT_ALLOCATOR.deallocate(ble_event, 1); + // Return the event to the pool + this->ble_event_pool_.release(ble_event); ble_event = this->ble_events_.pop(); } if (this->advertising_ != nullptr) { @@ -359,37 +356,41 @@ void ESP32BLE::loop() { } // Log dropped events periodically - size_t dropped = this->ble_events_.get_and_reset_dropped_count(); + uint16_t dropped = this->ble_events_.get_and_reset_dropped_count(); if (dropped > 0) { - ESP_LOGW(TAG, "Dropped %zu BLE events due to buffer overflow", dropped); + ESP_LOGW(TAG, "Dropped %u BLE events due to buffer overflow", dropped); } } +// Helper function to load new event data based on type +void load_ble_event(BLEEvent *event, esp_gap_ble_cb_event_t e, esp_ble_gap_cb_param_t *p) { + event->load_gap_event(e, p); +} + +void load_ble_event(BLEEvent *event, esp_gattc_cb_event_t e, esp_gatt_if_t i, esp_ble_gattc_cb_param_t *p) { + event->load_gattc_event(e, i, p); +} + +void load_ble_event(BLEEvent *event, esp_gatts_cb_event_t e, esp_gatt_if_t i, esp_ble_gatts_cb_param_t *p) { + event->load_gatts_event(e, i, p); +} + template void enqueue_ble_event(Args... args) { - // Check if queue is full before allocating - if (global_ble->ble_events_.full()) { - // Queue is full, drop the event + // Allocate an event from the pool + BLEEvent *event = global_ble->ble_event_pool_.allocate(); + if (event == nullptr) { + // No events available - queue is full or we're out of memory global_ble->ble_events_.increment_dropped_count(); return; } - BLEEvent *new_event = EVENT_ALLOCATOR.allocate(1); - if (new_event == nullptr) { - // Memory too fragmented to allocate new event. Can only drop it until memory comes back - global_ble->ble_events_.increment_dropped_count(); - return; - } - new (new_event) BLEEvent(args...); + // Load new event data (replaces previous event) + load_ble_event(event, args...); - // Push the event - since we're the only producer and we checked full() above, - // this should always succeed unless we have a bug - if (!global_ble->ble_events_.push(new_event)) { - // This should not happen in SPSC queue with single producer - ESP_LOGE(TAG, "BLE queue push failed unexpectedly"); - new_event->~BLEEvent(); - EVENT_ALLOCATOR.deallocate(new_event, 1); - } -} // NOLINT(clang-analyzer-unix.Malloc) + // Push the event to the queue + global_ble->ble_events_.push(event); + // Push always succeeds because we're the only producer and the pool ensures we never exceed queue size +} // Explicit template instantiations for the friend function template void enqueue_ble_event(esp_gap_ble_cb_event_t, esp_ble_gap_cb_param_t *); diff --git a/esphome/components/esp32_ble/ble.h b/esphome/components/esp32_ble/ble.h index 58c064a2ef..9fe996086e 100644 --- a/esphome/components/esp32_ble/ble.h +++ b/esphome/components/esp32_ble/ble.h @@ -12,6 +12,7 @@ #include "esphome/core/helpers.h" #include "ble_event.h" +#include "ble_event_pool.h" #include "queue.h" #ifdef USE_ESP32 @@ -148,6 +149,7 @@ class ESP32BLE : public Component { BLEComponentState state_{BLE_COMPONENT_STATE_OFF}; LockFreeQueue ble_events_; + BLEEventPool ble_event_pool_; BLEAdvertising *advertising_{}; esp_ble_io_cap_t io_cap_{ESP_IO_CAP_NONE}; uint32_t advertising_cycle_time_{}; diff --git a/esphome/components/esp32_ble/ble_event.h b/esphome/components/esp32_ble/ble_event.h index f51095effd..30118d2afd 100644 --- a/esphome/components/esp32_ble/ble_event.h +++ b/esphome/components/esp32_ble/ble_event.h @@ -51,6 +51,13 @@ static_assert(offsetof(esp_ble_gap_cb_param_t, scan_stop_cmpl.status) == // - GATTC/GATTS events: We heap-allocate and copy the entire param struct, ensuring // the data remains valid even after the BLE callback returns. The original // param pointer from ESP-IDF is only valid during the callback. +// +// CRITICAL DESIGN NOTE: +// The heap allocations for GATTC/GATTS events are REQUIRED for memory safety. +// DO NOT attempt to optimize by removing these allocations or storing pointers +// to the original ESP-IDF data. The ESP-IDF callback data has a different lifetime +// than our event processing, and accessing it after the callback returns would +// result in use-after-free bugs and crashes. class BLEEvent { public: // NOLINTNEXTLINE(readability-identifier-naming) @@ -63,123 +70,72 @@ class BLEEvent { // Constructor for GAP events - no external allocations needed BLEEvent(esp_gap_ble_cb_event_t e, esp_ble_gap_cb_param_t *p) { this->type_ = GAP; - this->event_.gap.gap_event = e; - - if (p == nullptr) { - return; // Invalid event, but we can't log in header file - } - - // Only copy the data we actually use for each GAP event type - switch (e) { - case ESP_GAP_BLE_SCAN_RESULT_EVT: - // Copy only the fields we use from scan results - memcpy(this->event_.gap.scan_result.bda, p->scan_rst.bda, sizeof(esp_bd_addr_t)); - this->event_.gap.scan_result.ble_addr_type = p->scan_rst.ble_addr_type; - this->event_.gap.scan_result.rssi = p->scan_rst.rssi; - this->event_.gap.scan_result.adv_data_len = p->scan_rst.adv_data_len; - this->event_.gap.scan_result.scan_rsp_len = p->scan_rst.scan_rsp_len; - this->event_.gap.scan_result.search_evt = p->scan_rst.search_evt; - memcpy(this->event_.gap.scan_result.ble_adv, p->scan_rst.ble_adv, - ESP_BLE_ADV_DATA_LEN_MAX + ESP_BLE_SCAN_RSP_DATA_LEN_MAX); - break; - - case ESP_GAP_BLE_SCAN_PARAM_SET_COMPLETE_EVT: - this->event_.gap.scan_complete.status = p->scan_param_cmpl.status; - break; - - case ESP_GAP_BLE_SCAN_START_COMPLETE_EVT: - this->event_.gap.scan_complete.status = p->scan_start_cmpl.status; - break; - - case ESP_GAP_BLE_SCAN_STOP_COMPLETE_EVT: - this->event_.gap.scan_complete.status = p->scan_stop_cmpl.status; - break; - - default: - // We only handle 4 GAP event types, others are dropped - break; - } + this->init_gap_data_(e, p); } // Constructor for GATTC events - uses heap allocation - // Creates a copy of the param struct since the original is only valid during the callback + // IMPORTANT: The heap allocation is REQUIRED and must not be removed as an optimization. + // The param pointer from ESP-IDF is only valid during the callback execution. + // Since BLE events are processed asynchronously in the main loop, we must create + // our own copy to ensure the data remains valid until the event is processed. BLEEvent(esp_gattc_cb_event_t e, esp_gatt_if_t i, esp_ble_gattc_cb_param_t *p) { this->type_ = GATTC; - this->event_.gattc.gattc_event = e; - this->event_.gattc.gattc_if = i; - - if (p == nullptr) { - this->event_.gattc.gattc_param = nullptr; - this->event_.gattc.data = nullptr; - return; // Invalid event, but we can't log in header file - } - - // Heap-allocate param and data - // Heap allocation is used because GATTC/GATTS events are rare (<1% of events) - // while GAP events (99%) are stored inline to minimize memory usage - this->event_.gattc.gattc_param = new esp_ble_gattc_cb_param_t(*p); - - // Copy data for events that need it - switch (e) { - case ESP_GATTC_NOTIFY_EVT: - this->event_.gattc.data = new std::vector(p->notify.value, p->notify.value + p->notify.value_len); - this->event_.gattc.gattc_param->notify.value = this->event_.gattc.data->data(); - break; - case ESP_GATTC_READ_CHAR_EVT: - case ESP_GATTC_READ_DESCR_EVT: - this->event_.gattc.data = new std::vector(p->read.value, p->read.value + p->read.value_len); - this->event_.gattc.gattc_param->read.value = this->event_.gattc.data->data(); - break; - default: - this->event_.gattc.data = nullptr; - break; - } + this->init_gattc_data_(e, i, p); } // Constructor for GATTS events - uses heap allocation - // Creates a copy of the param struct since the original is only valid during the callback + // IMPORTANT: The heap allocation is REQUIRED and must not be removed as an optimization. + // The param pointer from ESP-IDF is only valid during the callback execution. + // Since BLE events are processed asynchronously in the main loop, we must create + // our own copy to ensure the data remains valid until the event is processed. BLEEvent(esp_gatts_cb_event_t e, esp_gatt_if_t i, esp_ble_gatts_cb_param_t *p) { this->type_ = GATTS; - this->event_.gatts.gatts_event = e; - this->event_.gatts.gatts_if = i; - - if (p == nullptr) { - this->event_.gatts.gatts_param = nullptr; - this->event_.gatts.data = nullptr; - return; // Invalid event, but we can't log in header file - } - - // Heap-allocate param and data - // Heap allocation is used because GATTC/GATTS events are rare (<1% of events) - // while GAP events (99%) are stored inline to minimize memory usage - this->event_.gatts.gatts_param = new esp_ble_gatts_cb_param_t(*p); - - // Copy data for events that need it - switch (e) { - case ESP_GATTS_WRITE_EVT: - this->event_.gatts.data = new std::vector(p->write.value, p->write.value + p->write.len); - this->event_.gatts.gatts_param->write.value = this->event_.gatts.data->data(); - break; - default: - this->event_.gatts.data = nullptr; - break; - } + this->init_gatts_data_(e, i, p); } // Destructor to clean up heap allocations - ~BLEEvent() { - switch (this->type_) { - case GATTC: - delete this->event_.gattc.gattc_param; - delete this->event_.gattc.data; - break; - case GATTS: - delete this->event_.gatts.gatts_param; - delete this->event_.gatts.data; - break; - default: - break; + ~BLEEvent() { this->cleanup_heap_data(); } + + // Default constructor for pre-allocation in pool + BLEEvent() : type_(GAP) {} + + // Clean up any heap-allocated data + void cleanup_heap_data() { + if (this->type_ == GAP) { + return; } + if (this->type_ == GATTC) { + delete this->event_.gattc.gattc_param; + delete this->event_.gattc.data; + this->event_.gattc.gattc_param = nullptr; + this->event_.gattc.data = nullptr; + return; + } + if (this->type_ == GATTS) { + delete this->event_.gatts.gatts_param; + delete this->event_.gatts.data; + this->event_.gatts.gatts_param = nullptr; + this->event_.gatts.data = nullptr; + } + } + + // Load new event data for reuse (replaces previous event data) + void load_gap_event(esp_gap_ble_cb_event_t e, esp_ble_gap_cb_param_t *p) { + this->cleanup_heap_data(); + this->type_ = GAP; + this->init_gap_data_(e, p); + } + + void load_gattc_event(esp_gattc_cb_event_t e, esp_gatt_if_t i, esp_ble_gattc_cb_param_t *p) { + this->cleanup_heap_data(); + this->type_ = GATTC; + this->init_gattc_data_(e, i, p); + } + + void load_gatts_event(esp_gatts_cb_event_t e, esp_gatt_if_t i, esp_ble_gatts_cb_param_t *p) { + this->cleanup_heap_data(); + this->type_ = GATTS; + this->init_gatts_data_(e, i, p); } // Disable copy to prevent double-delete @@ -224,6 +180,119 @@ class BLEEvent { esp_gap_ble_cb_event_t gap_event_type() const { return event_.gap.gap_event; } const BLEScanResult &scan_result() const { return event_.gap.scan_result; } esp_bt_status_t scan_complete_status() const { return event_.gap.scan_complete.status; } + + private: + // Initialize GAP event data + void init_gap_data_(esp_gap_ble_cb_event_t e, esp_ble_gap_cb_param_t *p) { + this->event_.gap.gap_event = e; + + if (p == nullptr) { + return; // Invalid event, but we can't log in header file + } + + // Copy data based on event type + switch (e) { + case ESP_GAP_BLE_SCAN_RESULT_EVT: + memcpy(this->event_.gap.scan_result.bda, p->scan_rst.bda, sizeof(esp_bd_addr_t)); + this->event_.gap.scan_result.ble_addr_type = p->scan_rst.ble_addr_type; + this->event_.gap.scan_result.rssi = p->scan_rst.rssi; + this->event_.gap.scan_result.adv_data_len = p->scan_rst.adv_data_len; + this->event_.gap.scan_result.scan_rsp_len = p->scan_rst.scan_rsp_len; + this->event_.gap.scan_result.search_evt = p->scan_rst.search_evt; + memcpy(this->event_.gap.scan_result.ble_adv, p->scan_rst.ble_adv, + ESP_BLE_ADV_DATA_LEN_MAX + ESP_BLE_SCAN_RSP_DATA_LEN_MAX); + break; + + case ESP_GAP_BLE_SCAN_PARAM_SET_COMPLETE_EVT: + this->event_.gap.scan_complete.status = p->scan_param_cmpl.status; + break; + + case ESP_GAP_BLE_SCAN_START_COMPLETE_EVT: + this->event_.gap.scan_complete.status = p->scan_start_cmpl.status; + break; + + case ESP_GAP_BLE_SCAN_STOP_COMPLETE_EVT: + this->event_.gap.scan_complete.status = p->scan_stop_cmpl.status; + break; + + default: + // We only handle 4 GAP event types, others are dropped + break; + } + } + + // Initialize GATTC event data + void init_gattc_data_(esp_gattc_cb_event_t e, esp_gatt_if_t i, esp_ble_gattc_cb_param_t *p) { + this->event_.gattc.gattc_event = e; + this->event_.gattc.gattc_if = i; + + if (p == nullptr) { + this->event_.gattc.gattc_param = nullptr; + this->event_.gattc.data = nullptr; + return; // Invalid event, but we can't log in header file + } + + // Heap-allocate param and data + // Heap allocation is used because GATTC/GATTS events are rare (<1% of events) + // while GAP events (99%) are stored inline to minimize memory usage + // IMPORTANT: This heap allocation provides clear ownership semantics: + // - The BLEEvent owns the allocated memory for its lifetime + // - The data remains valid from the BLE callback context until processed in the main loop + // - Without this copy, we'd have use-after-free bugs as ESP-IDF reuses the callback memory + this->event_.gattc.gattc_param = new esp_ble_gattc_cb_param_t(*p); + + // Copy data for events that need it + // The param struct contains pointers (e.g., notify.value) that point to temporary buffers. + // We must copy this data to ensure it remains valid when the event is processed later. + switch (e) { + case ESP_GATTC_NOTIFY_EVT: + this->event_.gattc.data = new std::vector(p->notify.value, p->notify.value + p->notify.value_len); + this->event_.gattc.gattc_param->notify.value = this->event_.gattc.data->data(); + break; + case ESP_GATTC_READ_CHAR_EVT: + case ESP_GATTC_READ_DESCR_EVT: + this->event_.gattc.data = new std::vector(p->read.value, p->read.value + p->read.value_len); + this->event_.gattc.gattc_param->read.value = this->event_.gattc.data->data(); + break; + default: + this->event_.gattc.data = nullptr; + break; + } + } + + // Initialize GATTS event data + void init_gatts_data_(esp_gatts_cb_event_t e, esp_gatt_if_t i, esp_ble_gatts_cb_param_t *p) { + this->event_.gatts.gatts_event = e; + this->event_.gatts.gatts_if = i; + + if (p == nullptr) { + this->event_.gatts.gatts_param = nullptr; + this->event_.gatts.data = nullptr; + return; // Invalid event, but we can't log in header file + } + + // Heap-allocate param and data + // Heap allocation is used because GATTC/GATTS events are rare (<1% of events) + // while GAP events (99%) are stored inline to minimize memory usage + // IMPORTANT: This heap allocation provides clear ownership semantics: + // - The BLEEvent owns the allocated memory for its lifetime + // - The data remains valid from the BLE callback context until processed in the main loop + // - Without this copy, we'd have use-after-free bugs as ESP-IDF reuses the callback memory + this->event_.gatts.gatts_param = new esp_ble_gatts_cb_param_t(*p); + + // Copy data for events that need it + // The param struct contains pointers (e.g., write.value) that point to temporary buffers. + // We must copy this data to ensure it remains valid when the event is processed later. + switch (e) { + case ESP_GATTS_WRITE_EVT: + this->event_.gatts.data = new std::vector(p->write.value, p->write.value + p->write.len); + this->event_.gatts.gatts_param->write.value = this->event_.gatts.data->data(); + break; + default: + this->event_.gatts.data = nullptr; + break; + } + } }; // BLEEvent total size: 84 bytes (80 byte union + 1 byte type + 3 bytes padding) diff --git a/esphome/components/esp32_ble/ble_event_pool.h b/esphome/components/esp32_ble/ble_event_pool.h new file mode 100644 index 0000000000..ef123b1325 --- /dev/null +++ b/esphome/components/esp32_ble/ble_event_pool.h @@ -0,0 +1,72 @@ +#pragma once + +#ifdef USE_ESP32 + +#include +#include +#include "ble_event.h" +#include "queue.h" +#include "esphome/core/helpers.h" + +namespace esphome { +namespace esp32_ble { + +// BLE Event Pool - On-demand pool of BLEEvent objects to avoid heap fragmentation +// Events are allocated on first use and reused thereafter, growing to peak usage +template class BLEEventPool { + public: + BLEEventPool() : total_created_(0) {} + + ~BLEEventPool() { + // Clean up any remaining events in the free list + BLEEvent *event; + while ((event = this->free_list_.pop()) != nullptr) { + delete event; + } + } + + // Allocate an event from the pool + // Returns nullptr if pool is full + BLEEvent *allocate() { + // Try to get from free list first + BLEEvent *event = this->free_list_.pop(); + if (event != nullptr) + return event; + + // Need to create a new event + if (this->total_created_ >= SIZE) { + // Pool is at capacity + return nullptr; + } + + // Use internal RAM for better performance + RAMAllocator allocator(RAMAllocator::ALLOC_INTERNAL); + event = allocator.allocate(1); + + if (event == nullptr) { + // Memory allocation failed + return nullptr; + } + + // Placement new to construct the object + new (event) BLEEvent(); + this->total_created_++; + return event; + } + + // Return an event to the pool for reuse + void release(BLEEvent *event) { + if (event != nullptr) { + this->free_list_.push(event); + } + } + + private: + LockFreeQueue free_list_; // Free events ready for reuse + uint8_t total_created_; // Total events created (high water mark) +}; + +} // namespace esp32_ble +} // namespace esphome + +#endif diff --git a/esphome/components/esp32_ble/queue.h b/esphome/components/esp32_ble/queue.h index 56d2efd18b..75bf1eef25 100644 --- a/esphome/components/esp32_ble/queue.h +++ b/esphome/components/esp32_ble/queue.h @@ -18,7 +18,7 @@ namespace esphome { namespace esp32_ble { -template class LockFreeQueue { +template class LockFreeQueue { public: LockFreeQueue() : head_(0), tail_(0), dropped_count_(0) {} @@ -26,8 +26,8 @@ template class LockFreeQueue { if (element == nullptr) return false; - size_t current_tail = tail_.load(std::memory_order_relaxed); - size_t next_tail = (current_tail + 1) % SIZE; + uint8_t current_tail = tail_.load(std::memory_order_relaxed); + uint8_t next_tail = (current_tail + 1) % SIZE; if (next_tail == head_.load(std::memory_order_acquire)) { // Buffer full @@ -41,7 +41,7 @@ template class LockFreeQueue { } T *pop() { - size_t current_head = head_.load(std::memory_order_relaxed); + uint8_t current_head = head_.load(std::memory_order_relaxed); if (current_head == tail_.load(std::memory_order_acquire)) { return nullptr; // Empty @@ -53,27 +53,30 @@ template class LockFreeQueue { } size_t size() const { - size_t tail = tail_.load(std::memory_order_acquire); - size_t head = head_.load(std::memory_order_acquire); + uint8_t tail = tail_.load(std::memory_order_acquire); + uint8_t head = head_.load(std::memory_order_acquire); return (tail - head + SIZE) % SIZE; } - size_t get_and_reset_dropped_count() { return dropped_count_.exchange(0, std::memory_order_relaxed); } + uint16_t get_and_reset_dropped_count() { return dropped_count_.exchange(0, std::memory_order_relaxed); } void increment_dropped_count() { dropped_count_.fetch_add(1, std::memory_order_relaxed); } bool empty() const { return head_.load(std::memory_order_acquire) == tail_.load(std::memory_order_acquire); } bool full() const { - size_t next_tail = (tail_.load(std::memory_order_relaxed) + 1) % SIZE; + uint8_t next_tail = (tail_.load(std::memory_order_relaxed) + 1) % SIZE; return next_tail == head_.load(std::memory_order_acquire); } protected: T *buffer_[SIZE]; - std::atomic head_; - std::atomic tail_; - std::atomic dropped_count_; + // Atomic: written by producer (push/increment), read+reset by consumer (get_and_reset) + std::atomic dropped_count_; // 65535 max - more than enough for drop tracking + // Atomic: written by consumer (pop), read by producer (push) to check if full + std::atomic head_; + // Atomic: written by producer (push), read by consumer (pop) to check if empty + std::atomic tail_; }; } // namespace esp32_ble From 24587fe875fabbd061b92fc100c0846d3512fab1 Mon Sep 17 00:00:00 2001 From: Edward Firmo <94725493+edwardtfn@users.noreply.github.com> Date: Thu, 19 Jun 2025 03:41:20 +0200 Subject: [PATCH 03/20] [nextion] Fix command spacing double timing and response blocking issues (#9134) --- esphome/components/nextion/nextion.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/esphome/components/nextion/nextion.cpp b/esphome/components/nextion/nextion.cpp index 3de32bfde9..24c31713bc 100644 --- a/esphome/components/nextion/nextion.cpp +++ b/esphome/components/nextion/nextion.cpp @@ -33,6 +33,7 @@ bool Nextion::send_command_(const std::string &command) { #ifdef USE_NEXTION_COMMAND_SPACING if (!this->ignore_is_setup_ && !this->command_pacer_.can_send()) { + ESP_LOGN(TAG, "Command spacing: delaying command '%s'", command.c_str()); return false; } #endif // USE_NEXTION_COMMAND_SPACING @@ -43,10 +44,6 @@ bool Nextion::send_command_(const std::string &command) { const uint8_t to_send[3] = {0xFF, 0xFF, 0xFF}; this->write_array(to_send, sizeof(to_send)); -#ifdef USE_NEXTION_COMMAND_SPACING - this->command_pacer_.mark_sent(); -#endif // USE_NEXTION_COMMAND_SPACING - return true; } @@ -377,12 +374,6 @@ void Nextion::process_nextion_commands_() { size_t commands_processed = 0; #endif // USE_NEXTION_MAX_COMMANDS_PER_LOOP -#ifdef USE_NEXTION_COMMAND_SPACING - if (!this->command_pacer_.can_send()) { - return; // Will try again in next loop iteration - } -#endif - size_t to_process_length = 0; std::string to_process; @@ -430,6 +421,7 @@ void Nextion::process_nextion_commands_() { } #ifdef USE_NEXTION_COMMAND_SPACING this->command_pacer_.mark_sent(); // Here is where we should mark the command as sent + ESP_LOGN(TAG, "Command spacing: marked command sent at %u ms", millis()); #endif break; case 0x02: // invalid Component ID or name was used From 6dfb9eba619b57be2c56dcf480d408fc9b09f1c4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 19 Jun 2025 05:03:09 +0200 Subject: [PATCH 04/20] Fix missing BLE GAP events causing RSSI sensor and beacon failures (#9138) --- esphome/components/esp32_ble/ble.cpp | 97 ++++++++++++++---- esphome/components/esp32_ble/ble_event.h | 120 ++++++++++++++++++++--- 2 files changed, 188 insertions(+), 29 deletions(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index 5a66f11d0f..cf63ad34d7 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -324,23 +324,69 @@ void ESP32BLE::loop() { } case BLEEvent::GAP: { esp_gap_ble_cb_event_t gap_event = ble_event->event_.gap.gap_event; - if (gap_event == ESP_GAP_BLE_SCAN_RESULT_EVT) { - // Use the new scan event handler - no memcpy! - for (auto *scan_handler : this->gap_scan_event_handlers_) { - scan_handler->gap_scan_event_handler(ble_event->scan_result()); - } - } else if (gap_event == ESP_GAP_BLE_SCAN_PARAM_SET_COMPLETE_EVT || - gap_event == ESP_GAP_BLE_SCAN_START_COMPLETE_EVT || - gap_event == ESP_GAP_BLE_SCAN_STOP_COMPLETE_EVT) { - // All three scan complete events have the same structure with just status - // The scan_complete struct matches ESP-IDF's layout exactly, so this reinterpret_cast is safe - // This is verified at compile-time by static_assert checks in ble_event.h - // The struct already contains our copy of the status (copied in BLEEvent constructor) - ESP_LOGV(TAG, "gap_event_handler - %d", gap_event); - for (auto *gap_handler : this->gap_event_handlers_) { - gap_handler->gap_event_handler( - gap_event, reinterpret_cast(&ble_event->event_.gap.scan_complete)); - } + switch (gap_event) { + case ESP_GAP_BLE_SCAN_RESULT_EVT: + // Use the new scan event handler - no memcpy! + for (auto *scan_handler : this->gap_scan_event_handlers_) { + scan_handler->gap_scan_event_handler(ble_event->scan_result()); + } + break; + + // Scan complete events + case ESP_GAP_BLE_SCAN_PARAM_SET_COMPLETE_EVT: + case ESP_GAP_BLE_SCAN_START_COMPLETE_EVT: + case ESP_GAP_BLE_SCAN_STOP_COMPLETE_EVT: + // All three scan complete events have the same structure with just status + // The scan_complete struct matches ESP-IDF's layout exactly, so this reinterpret_cast is safe + // This is verified at compile-time by static_assert checks in ble_event.h + // The struct already contains our copy of the status (copied in BLEEvent constructor) + ESP_LOGV(TAG, "gap_event_handler - %d", gap_event); + for (auto *gap_handler : this->gap_event_handlers_) { + gap_handler->gap_event_handler( + gap_event, reinterpret_cast(&ble_event->event_.gap.scan_complete)); + } + break; + + // Advertising complete events + case ESP_GAP_BLE_ADV_DATA_SET_COMPLETE_EVT: + case ESP_GAP_BLE_SCAN_RSP_DATA_SET_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_START_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_STOP_COMPLETE_EVT: + // All advertising complete events have the same structure with just status + ESP_LOGV(TAG, "gap_event_handler - %d", gap_event); + for (auto *gap_handler : this->gap_event_handlers_) { + gap_handler->gap_event_handler( + gap_event, reinterpret_cast(&ble_event->event_.gap.adv_complete)); + } + break; + + // RSSI complete event + case ESP_GAP_BLE_READ_RSSI_COMPLETE_EVT: + ESP_LOGV(TAG, "gap_event_handler - %d", gap_event); + for (auto *gap_handler : this->gap_event_handlers_) { + gap_handler->gap_event_handler( + gap_event, reinterpret_cast(&ble_event->event_.gap.read_rssi_complete)); + } + break; + + // Security events + case ESP_GAP_BLE_AUTH_CMPL_EVT: + case ESP_GAP_BLE_SEC_REQ_EVT: + case ESP_GAP_BLE_PASSKEY_NOTIF_EVT: + case ESP_GAP_BLE_PASSKEY_REQ_EVT: + case ESP_GAP_BLE_NC_REQ_EVT: + ESP_LOGV(TAG, "gap_event_handler - %d", gap_event); + for (auto *gap_handler : this->gap_event_handlers_) { + gap_handler->gap_event_handler( + gap_event, reinterpret_cast(&ble_event->event_.gap.security)); + } + break; + + default: + // Unknown/unhandled event + ESP_LOGW(TAG, "Unhandled GAP event type in loop: %d", gap_event); + break; } break; } @@ -399,11 +445,26 @@ template void enqueue_ble_event(esp_gattc_cb_event_t, esp_gatt_if_t, esp_ble_gat void ESP32BLE::gap_event_handler(esp_gap_ble_cb_event_t event, esp_ble_gap_cb_param_t *param) { switch (event) { - // Only queue the 4 GAP events we actually handle + // Queue GAP events that components need to handle + // Scanning events - used by esp32_ble_tracker case ESP_GAP_BLE_SCAN_RESULT_EVT: case ESP_GAP_BLE_SCAN_PARAM_SET_COMPLETE_EVT: case ESP_GAP_BLE_SCAN_START_COMPLETE_EVT: case ESP_GAP_BLE_SCAN_STOP_COMPLETE_EVT: + // Advertising events - used by esp32_ble_beacon and esp32_ble server + case ESP_GAP_BLE_ADV_DATA_SET_COMPLETE_EVT: + case ESP_GAP_BLE_SCAN_RSP_DATA_SET_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_START_COMPLETE_EVT: + case ESP_GAP_BLE_ADV_STOP_COMPLETE_EVT: + // Connection events - used by ble_client + case ESP_GAP_BLE_READ_RSSI_COMPLETE_EVT: + // Security events - used by ble_client and bluetooth_proxy + case ESP_GAP_BLE_AUTH_CMPL_EVT: + case ESP_GAP_BLE_SEC_REQ_EVT: + case ESP_GAP_BLE_PASSKEY_NOTIF_EVT: + case ESP_GAP_BLE_PASSKEY_REQ_EVT: + case ESP_GAP_BLE_NC_REQ_EVT: enqueue_ble_event(event, param); return; diff --git a/esphome/components/esp32_ble/ble_event.h b/esphome/components/esp32_ble/ble_event.h index 30118d2afd..dd3ec3da42 100644 --- a/esphome/components/esp32_ble/ble_event.h +++ b/esphome/components/esp32_ble/ble_event.h @@ -24,16 +24,45 @@ static_assert(sizeof(esp_ble_gap_cb_param_t::ble_scan_stop_cmpl_evt_param) == si "ESP-IDF scan_stop_cmpl structure has unexpected size"); // Verify the status field is at offset 0 (first member) -static_assert(offsetof(esp_ble_gap_cb_param_t, scan_param_cmpl.status) == - offsetof(esp_ble_gap_cb_param_t, scan_param_cmpl), +static_assert(offsetof(esp_ble_gap_cb_param_t, scan_param_cmpl.status) == 0, "status must be first member of scan_param_cmpl"); -static_assert(offsetof(esp_ble_gap_cb_param_t, scan_start_cmpl.status) == - offsetof(esp_ble_gap_cb_param_t, scan_start_cmpl), +static_assert(offsetof(esp_ble_gap_cb_param_t, scan_start_cmpl.status) == 0, "status must be first member of scan_start_cmpl"); -static_assert(offsetof(esp_ble_gap_cb_param_t, scan_stop_cmpl.status) == - offsetof(esp_ble_gap_cb_param_t, scan_stop_cmpl), +static_assert(offsetof(esp_ble_gap_cb_param_t, scan_stop_cmpl.status) == 0, "status must be first member of scan_stop_cmpl"); +// Compile-time verification for advertising complete events +static_assert(sizeof(esp_ble_gap_cb_param_t::ble_adv_data_cmpl_evt_param) == sizeof(esp_bt_status_t), + "ESP-IDF adv_data_cmpl structure has unexpected size"); +static_assert(sizeof(esp_ble_gap_cb_param_t::ble_scan_rsp_data_cmpl_evt_param) == sizeof(esp_bt_status_t), + "ESP-IDF scan_rsp_data_cmpl structure has unexpected size"); +static_assert(sizeof(esp_ble_gap_cb_param_t::ble_adv_data_raw_cmpl_evt_param) == sizeof(esp_bt_status_t), + "ESP-IDF adv_data_raw_cmpl structure has unexpected size"); +static_assert(sizeof(esp_ble_gap_cb_param_t::ble_adv_start_cmpl_evt_param) == sizeof(esp_bt_status_t), + "ESP-IDF adv_start_cmpl structure has unexpected size"); +static_assert(sizeof(esp_ble_gap_cb_param_t::ble_adv_stop_cmpl_evt_param) == sizeof(esp_bt_status_t), + "ESP-IDF adv_stop_cmpl structure has unexpected size"); + +// Verify the status field is at offset 0 for advertising events +static_assert(offsetof(esp_ble_gap_cb_param_t, adv_data_cmpl.status) == 0, + "status must be first member of adv_data_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, scan_rsp_data_cmpl.status) == 0, + "status must be first member of scan_rsp_data_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, adv_data_raw_cmpl.status) == 0, + "status must be first member of adv_data_raw_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, adv_start_cmpl.status) == 0, + "status must be first member of adv_start_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, adv_stop_cmpl.status) == 0, + "status must be first member of adv_stop_cmpl"); + +// Compile-time verification for RSSI complete event structure +static_assert(offsetof(esp_ble_gap_cb_param_t, read_rssi_cmpl.status) == 0, + "status must be first member of read_rssi_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, read_rssi_cmpl.rssi) == sizeof(esp_bt_status_t), + "rssi must immediately follow status in read_rssi_cmpl"); +static_assert(offsetof(esp_ble_gap_cb_param_t, read_rssi_cmpl.remote_addr) == sizeof(esp_bt_status_t) + sizeof(int8_t), + "remote_addr must follow rssi in read_rssi_cmpl"); + // Received GAP, GATTC and GATTS events are only queued, and get processed in the main loop(). // This class stores each event with minimal memory usage. // GAP events (99% of traffic) don't have the vector overhead. @@ -67,6 +96,17 @@ class BLEEvent { GATTS, }; + // Type definitions for cleaner method signatures + struct StatusOnlyData { + esp_bt_status_t status; + }; + + struct RSSICompleteData { + esp_bt_status_t status; + int8_t rssi; + esp_bd_addr_t remote_addr; + }; + // Constructor for GAP events - no external allocations needed BLEEvent(esp_gap_ble_cb_event_t e, esp_ble_gap_cb_param_t *p) { this->type_ = GAP; @@ -147,12 +187,21 @@ class BLEEvent { struct gap_event { esp_gap_ble_cb_event_t gap_event; union { - BLEScanResult scan_result; // 73 bytes + BLEScanResult scan_result; // 73 bytes - Used by: esp32_ble_tracker // This matches ESP-IDF's scan complete event structures // All three (scan_param_cmpl, scan_start_cmpl, scan_stop_cmpl) have identical layout - struct { - esp_bt_status_t status; - } scan_complete; // 1 byte + // Used by: esp32_ble_tracker + StatusOnlyData scan_complete; // 1 byte + // Advertising complete events all have same structure + // Used by: esp32_ble_beacon, esp32_ble server components + // ADV_DATA_SET, SCAN_RSP_DATA_SET, ADV_DATA_RAW_SET, ADV_START, ADV_STOP + StatusOnlyData adv_complete; // 1 byte + // RSSI complete event + // Used by: ble_client (ble_rssi_sensor component) + RSSICompleteData read_rssi_complete; // 8 bytes + // Security events - we store the full security union + // Used by: ble_client (automation), bluetooth_proxy, esp32_ble_client + esp_ble_sec_t security; // Variable size, but fits within scan_result size }; } gap; // 80 bytes total @@ -180,6 +229,9 @@ class BLEEvent { esp_gap_ble_cb_event_t gap_event_type() const { return event_.gap.gap_event; } const BLEScanResult &scan_result() const { return event_.gap.scan_result; } esp_bt_status_t scan_complete_status() const { return event_.gap.scan_complete.status; } + esp_bt_status_t adv_complete_status() const { return event_.gap.adv_complete.status; } + const RSSICompleteData &read_rssi_complete() const { return event_.gap.read_rssi_complete; } + const esp_ble_sec_t &security() const { return event_.gap.security; } private: // Initialize GAP event data @@ -215,8 +267,47 @@ class BLEEvent { this->event_.gap.scan_complete.status = p->scan_stop_cmpl.status; break; + // Advertising complete events - all have same structure with just status + // Used by: esp32_ble_beacon, esp32_ble server components + case ESP_GAP_BLE_ADV_DATA_SET_COMPLETE_EVT: + this->event_.gap.adv_complete.status = p->adv_data_cmpl.status; + break; + case ESP_GAP_BLE_SCAN_RSP_DATA_SET_COMPLETE_EVT: + this->event_.gap.adv_complete.status = p->scan_rsp_data_cmpl.status; + break; + case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT: // Used by: esp32_ble_beacon + this->event_.gap.adv_complete.status = p->adv_data_raw_cmpl.status; + break; + case ESP_GAP_BLE_ADV_START_COMPLETE_EVT: // Used by: esp32_ble_beacon + this->event_.gap.adv_complete.status = p->adv_start_cmpl.status; + break; + case ESP_GAP_BLE_ADV_STOP_COMPLETE_EVT: // Used by: esp32_ble_beacon + this->event_.gap.adv_complete.status = p->adv_stop_cmpl.status; + break; + + // RSSI complete event + // Used by: ble_client (ble_rssi_sensor) + case ESP_GAP_BLE_READ_RSSI_COMPLETE_EVT: + this->event_.gap.read_rssi_complete.status = p->read_rssi_cmpl.status; + this->event_.gap.read_rssi_complete.rssi = p->read_rssi_cmpl.rssi; + memcpy(this->event_.gap.read_rssi_complete.remote_addr, p->read_rssi_cmpl.remote_addr, sizeof(esp_bd_addr_t)); + break; + + // Security events - copy the entire security union + // Used by: ble_client, bluetooth_proxy, esp32_ble_client + case ESP_GAP_BLE_AUTH_CMPL_EVT: // Used by: bluetooth_proxy, esp32_ble_client + case ESP_GAP_BLE_SEC_REQ_EVT: // Used by: esp32_ble_client + case ESP_GAP_BLE_PASSKEY_NOTIF_EVT: // Used by: ble_client automation + case ESP_GAP_BLE_PASSKEY_REQ_EVT: // Used by: ble_client automation + case ESP_GAP_BLE_NC_REQ_EVT: // Used by: ble_client automation + memcpy(&this->event_.gap.security, &p->ble_security, sizeof(esp_ble_sec_t)); + break; + default: - // We only handle 4 GAP event types, others are dropped + // We only store data for GAP events that components currently use + // Unknown events still get queued and logged in ble.cpp:375 as + // "Unhandled GAP event type in loop" - this helps identify new events + // that components might need in the future break; } } @@ -295,6 +386,13 @@ class BLEEvent { } }; +// Verify the gap_event struct hasn't grown beyond expected size +// The gap member in the union should be 80 bytes (including the gap_event enum) +static_assert(sizeof(decltype(((BLEEvent *) nullptr)->event_.gap)) <= 80, "gap_event struct has grown beyond 80 bytes"); + +// Verify esp_ble_sec_t fits within our union +static_assert(sizeof(esp_ble_sec_t) <= 73, "esp_ble_sec_t is larger than BLEScanResult"); + // BLEEvent total size: 84 bytes (80 byte union + 1 byte type + 3 bytes padding) } // namespace esp32_ble From 2f2ecadae7c2d6d18f8cb77da488f10461d8c073 Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Mon, 23 Jun 2025 15:41:06 +1200 Subject: [PATCH 05/20] [config validation] Add more ip address / network validators (#9181) --- esphome/config_validation.py | 45 +++++++++++++++++++++++++++++++++++- esphome/yaml_util.py | 3 ++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/esphome/config_validation.py b/esphome/config_validation.py index 964f533215..bf69b81bb5 100644 --- a/esphome/config_validation.py +++ b/esphome/config_validation.py @@ -3,7 +3,15 @@ from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime -from ipaddress import AddressValueError, IPv4Address, ip_address +from ipaddress import ( + AddressValueError, + IPv4Address, + IPv4Network, + IPv6Address, + IPv6Network, + ip_address, + ip_network, +) import logging import os import re @@ -1176,6 +1184,14 @@ def ipv4address(value): return address +def ipv6address(value): + try: + address = IPv6Address(value) + except AddressValueError as exc: + raise Invalid(f"{value} is not a valid IPv6 address") from exc + return address + + def ipv4address_multi_broadcast(value): address = ipv4address(value) if not (address.is_multicast or (address == IPv4Address("255.255.255.255"))): @@ -1193,6 +1209,33 @@ def ipaddress(value): return address +def ipv4network(value): + """Validate that the value is a valid IPv4 network.""" + try: + network = IPv4Network(value, strict=False) + except ValueError as exc: + raise Invalid(f"{value} is not a valid IPv4 network") from exc + return network + + +def ipv6network(value): + """Validate that the value is a valid IPv6 network.""" + try: + network = IPv6Network(value, strict=False) + except ValueError as exc: + raise Invalid(f"{value} is not a valid IPv6 network") from exc + return network + + +def ipnetwork(value): + """Validate that the value is a valid IP network.""" + try: + network = ip_network(value, strict=False) + except ValueError as exc: + raise Invalid(f"{value} is not a valid IP network") from exc + return network + + def _valid_topic(value): """Validate that this is a valid topic name/filter.""" if value is None: # Used to disable publishing and subscribing diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index 78deec8e65..bd1806affc 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -5,7 +5,7 @@ import fnmatch import functools import inspect from io import BytesIO, TextIOBase, TextIOWrapper -from ipaddress import _BaseAddress +from ipaddress import _BaseAddress, _BaseNetwork import logging import math import os @@ -621,6 +621,7 @@ ESPHomeDumper.add_multi_representer(str, ESPHomeDumper.represent_stringify) ESPHomeDumper.add_multi_representer(int, ESPHomeDumper.represent_int) ESPHomeDumper.add_multi_representer(float, ESPHomeDumper.represent_float) ESPHomeDumper.add_multi_representer(_BaseAddress, ESPHomeDumper.represent_stringify) +ESPHomeDumper.add_multi_representer(_BaseNetwork, ESPHomeDumper.represent_stringify) ESPHomeDumper.add_multi_representer(MACAddress, ESPHomeDumper.represent_stringify) ESPHomeDumper.add_multi_representer(TimePeriod, ESPHomeDumper.represent_stringify) ESPHomeDumper.add_multi_representer(Lambda, ESPHomeDumper.represent_lambda) From 5d6e690c12024ef7712a7bc24fec94755c8f72e0 Mon Sep 17 00:00:00 2001 From: rwrozelle Date: Sun, 22 Jun 2025 23:41:29 -0400 Subject: [PATCH 06/20] Fixes for setup of OpenThread either using TLV or entering Credentials directly (#9157) Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com> --- esphome/components/openthread/__init__.py | 10 +++++----- esphome/components/openthread/openthread.cpp | 6 +++--- esphome/components/openthread/tlv.py | 7 +++++++ tests/components/openthread/test.esp32-c6-idf.yaml | 2 ++ 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/esphome/components/openthread/__init__.py b/esphome/components/openthread/__init__.py index 5b1ea491e3..393c47e720 100644 --- a/esphome/components/openthread/__init__.py +++ b/esphome/components/openthread/__init__.py @@ -46,7 +46,7 @@ def set_sdkconfig_options(config): add_idf_sdkconfig_option("CONFIG_OPENTHREAD_NETWORK_PANID", config[CONF_PAN_ID]) add_idf_sdkconfig_option("CONFIG_OPENTHREAD_NETWORK_CHANNEL", config[CONF_CHANNEL]) add_idf_sdkconfig_option( - "CONFIG_OPENTHREAD_NETWORK_MASTERKEY", f"{config[CONF_NETWORK_KEY]:X}" + "CONFIG_OPENTHREAD_NETWORK_MASTERKEY", f"{config[CONF_NETWORK_KEY]:X}".lower() ) if network_name := config.get(CONF_NETWORK_NAME): @@ -54,14 +54,14 @@ def set_sdkconfig_options(config): if (ext_pan_id := config.get(CONF_EXT_PAN_ID)) is not None: add_idf_sdkconfig_option( - "CONFIG_OPENTHREAD_NETWORK_EXTPANID", f"{ext_pan_id:X}" + "CONFIG_OPENTHREAD_NETWORK_EXTPANID", f"{ext_pan_id:X}".lower() ) if (mesh_local_prefix := config.get(CONF_MESH_LOCAL_PREFIX)) is not None: add_idf_sdkconfig_option( - "CONFIG_OPENTHREAD_MESH_LOCAL_PREFIX", f"{mesh_local_prefix:X}" + "CONFIG_OPENTHREAD_MESH_LOCAL_PREFIX", f"{mesh_local_prefix}".lower() ) if (pskc := config.get(CONF_PSKC)) is not None: - add_idf_sdkconfig_option("CONFIG_OPENTHREAD_NETWORK_PSKC", f"{pskc:X}") + add_idf_sdkconfig_option("CONFIG_OPENTHREAD_NETWORK_PSKC", f"{pskc:X}".lower()) if CONF_FORCE_DATASET in config: if config[CONF_FORCE_DATASET]: @@ -98,7 +98,7 @@ _CONNECTION_SCHEMA = cv.Schema( cv.Optional(CONF_EXT_PAN_ID): cv.hex_int, cv.Optional(CONF_NETWORK_NAME): cv.string_strict, cv.Optional(CONF_PSKC): cv.hex_int, - cv.Optional(CONF_MESH_LOCAL_PREFIX): cv.hex_int, + cv.Optional(CONF_MESH_LOCAL_PREFIX): cv.ipv6network, } ) diff --git a/esphome/components/openthread/openthread.cpp b/esphome/components/openthread/openthread.cpp index f40a56952a..24b3c23960 100644 --- a/esphome/components/openthread/openthread.cpp +++ b/esphome/components/openthread/openthread.cpp @@ -137,7 +137,7 @@ void OpenThreadSrpComponent::setup() { // Copy the mdns services to our local instance so that the c_str pointers remain valid for the lifetime of this // component this->mdns_services_ = this->mdns_->get_services(); - ESP_LOGW(TAG, "Setting up SRP services. count = %d\n", this->mdns_services_.size()); + ESP_LOGD(TAG, "Setting up SRP services. count = %d\n", this->mdns_services_.size()); for (const auto &service : this->mdns_services_) { otSrpClientBuffersServiceEntry *entry = otSrpClientBuffersAllocateService(instance); if (!entry) { @@ -185,11 +185,11 @@ void OpenThreadSrpComponent::setup() { if (error != OT_ERROR_NONE) { ESP_LOGW(TAG, "Failed to add service: %s", otThreadErrorToString(error)); } - ESP_LOGW(TAG, "Added service: %s", full_service.c_str()); + ESP_LOGD(TAG, "Added service: %s", full_service.c_str()); } otSrpClientEnableAutoStartMode(instance, srp_start_callback, nullptr); - ESP_LOGW(TAG, "Finished SRP setup"); + ESP_LOGD(TAG, "Finished SRP setup"); } void *OpenThreadSrpComponent::pool_alloc_(size_t size) { diff --git a/esphome/components/openthread/tlv.py b/esphome/components/openthread/tlv.py index 45c8c47227..4a7d21c47d 100644 --- a/esphome/components/openthread/tlv.py +++ b/esphome/components/openthread/tlv.py @@ -1,5 +1,6 @@ # Sourced from https://gist.github.com/agners/0338576e0003318b63ec1ea75adc90f9 import binascii +import ipaddress from esphome.const import CONF_CHANNEL @@ -37,6 +38,12 @@ def parse_tlv(tlv) -> dict: if tag in TLV_TYPES: if tag == 3: output[TLV_TYPES[tag]] = val.decode("utf-8") + elif tag == 7: + mesh_local_prefix = binascii.hexlify(val).decode("utf-8") + mesh_local_prefix_str = f"{mesh_local_prefix}0000000000000000" + ipv6_bytes = bytes.fromhex(mesh_local_prefix_str) + ipv6_address = ipaddress.IPv6Address(ipv6_bytes) + output[TLV_TYPES[tag]] = f"{ipv6_address}/64" else: output[TLV_TYPES[tag]] = int.from_bytes(val) return output diff --git a/tests/components/openthread/test.esp32-c6-idf.yaml b/tests/components/openthread/test.esp32-c6-idf.yaml index 482fd1a453..f53b323bec 100644 --- a/tests/components/openthread/test.esp32-c6-idf.yaml +++ b/tests/components/openthread/test.esp32-c6-idf.yaml @@ -8,4 +8,6 @@ openthread: pan_id: 0x8f28 ext_pan_id: 0xd63e8e3e495ebbc3 pskc: 0xc23a76e98f1a6483639b1ac1271e2e27 + mesh_local_prefix: fd53:145f:ed22:ad81::/64 force_dataset: true + From 649936200e8390db5c91bb1450f5265d48d14852 Mon Sep 17 00:00:00 2001 From: myhomeiot <70070601+myhomeiot@users.noreply.github.com> Date: Mon, 23 Jun 2025 06:42:20 +0300 Subject: [PATCH 07/20] Restore access to BLEScanResult as get_scan_result (#9148) --- esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp | 1 + esphome/components/esp32_ble_tracker/esp32_ble_tracker.h | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp index c5906779f1..a1fb727dd0 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp @@ -522,6 +522,7 @@ optional ESPBLEiBeacon::from_manufacturer_data(const ServiceData } void ESPBTDevice::parse_scan_rst(const BLEScanResult &scan_result) { + this->scan_result_ = &scan_result; for (uint8_t i = 0; i < ESP_BD_ADDR_LEN; i++) this->address_[i] = scan_result.bda[i]; this->address_type_ = static_cast(scan_result.ble_addr_type); diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h index 16a100fb47..892f76f49c 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h @@ -85,6 +85,9 @@ class ESPBTDevice { const std::vector &get_service_datas() const { return service_datas_; } + // Exposed through a function for use in lambdas + const BLEScanResult &get_scan_result() const { return *scan_result_; } + bool resolve_irk(const uint8_t *irk) const; optional get_ibeacon() const { @@ -111,6 +114,7 @@ class ESPBTDevice { std::vector service_uuids_{}; std::vector manufacturer_datas_{}; std::vector service_datas_{}; + const BLEScanResult *scan_result_{nullptr}; }; class ESP32BLETracker; From 22e360d4792b95449e2a93de1a7f956e9f9bb381 Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Mon, 23 Jun 2025 23:32:22 +1200 Subject: [PATCH 08/20] Bump version to 2025.6.1 --- Doxyfile | 2 +- esphome/const.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Doxyfile b/Doxyfile index d1e950c548..a4dd65bf45 100644 --- a/Doxyfile +++ b/Doxyfile @@ -48,7 +48,7 @@ PROJECT_NAME = ESPHome # could be handy for archiving the generated documentation or if some version # control system is used. -PROJECT_NUMBER = 2025.6.0 +PROJECT_NUMBER = 2025.6.1 # Using the PROJECT_BRIEF tag one can provide an optional one line description # for a project that appears at the top of each page and should give viewer a diff --git a/esphome/const.py b/esphome/const.py index 95a5dbe218..3ef82aff5b 100644 --- a/esphome/const.py +++ b/esphome/const.py @@ -1,6 +1,6 @@ """Constants used by esphome.""" -__version__ = "2025.6.0" +__version__ = "2025.6.1" ALLOWED_NAME_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789-_" VALID_SUBSTITUTIONS_CHARACTERS = ( From ac942e0670e9afe9e8f8ca6903f3788320c674b9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Jun 2025 19:58:32 +0200 Subject: [PATCH 09/20] Bump aioesphomeapi from 33.1.0 to 33.1.1 (#9187) --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 01bbfa91c0..3f306fe4fa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ platformio==6.1.18 # When updating platformio, also update /docker/Dockerfile esptool==4.9.0 click==8.1.7 esphome-dashboard==20250514.0 -aioesphomeapi==33.1.0 +aioesphomeapi==33.1.1 zeroconf==0.147.0 puremagic==1.29 ruamel.yaml==0.18.14 # dashboard_import From a35e476be5b0ec14ae04695c5906c583977df59c Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Mon, 23 Jun 2025 13:31:20 -0600 Subject: [PATCH 10/20] [opt3001] New component (#6625) Co-authored-by: Keith Burzinski --- CODEOWNERS | 1 + esphome/components/opt3001/__init__.py | 0 esphome/components/opt3001/opt3001.cpp | 122 ++++++++++++++++++ esphome/components/opt3001/opt3001.h | 27 ++++ esphome/components/opt3001/sensor.py | 35 +++++ tests/components/opt3001/common.yaml | 10 ++ tests/components/opt3001/test.esp32-ard.yaml | 5 + .../components/opt3001/test.esp32-c3-ard.yaml | 5 + .../components/opt3001/test.esp32-c3-idf.yaml | 5 + tests/components/opt3001/test.esp32-idf.yaml | 5 + .../components/opt3001/test.esp8266-ard.yaml | 5 + tests/components/opt3001/test.rp2040-ard.yaml | 5 + 12 files changed, 225 insertions(+) create mode 100644 esphome/components/opt3001/__init__.py create mode 100644 esphome/components/opt3001/opt3001.cpp create mode 100644 esphome/components/opt3001/opt3001.h create mode 100644 esphome/components/opt3001/sensor.py create mode 100644 tests/components/opt3001/common.yaml create mode 100644 tests/components/opt3001/test.esp32-ard.yaml create mode 100644 tests/components/opt3001/test.esp32-c3-ard.yaml create mode 100644 tests/components/opt3001/test.esp32-c3-idf.yaml create mode 100644 tests/components/opt3001/test.esp32-idf.yaml create mode 100644 tests/components/opt3001/test.esp8266-ard.yaml create mode 100644 tests/components/opt3001/test.rp2040-ard.yaml diff --git a/CODEOWNERS b/CODEOWNERS index ebbc8732ea..83d64a8850 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -323,6 +323,7 @@ esphome/components/one_wire/* @ssieb esphome/components/online_image/* @clydebarrow @guillempages esphome/components/opentherm/* @olegtarasov esphome/components/openthread/* @mrene +esphome/components/opt3001/* @ccutrer esphome/components/ota/* @esphome/core esphome/components/output/* @esphome/core esphome/components/packet_transport/* @clydebarrow diff --git a/esphome/components/opt3001/__init__.py b/esphome/components/opt3001/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/esphome/components/opt3001/opt3001.cpp b/esphome/components/opt3001/opt3001.cpp new file mode 100644 index 0000000000..2d65f1090d --- /dev/null +++ b/esphome/components/opt3001/opt3001.cpp @@ -0,0 +1,122 @@ +#include "opt3001.h" +#include "esphome/core/log.h" + +namespace esphome { +namespace opt3001 { + +static const char *const TAG = "opt3001.sensor"; + +static const uint8_t OPT3001_REG_RESULT = 0x00; +static const uint8_t OPT3001_REG_CONFIGURATION = 0x01; +// See datasheet for full description of each bit. +static const uint16_t OPT3001_CONFIGURATION_RANGE_FULL = 0b1100000000000000; +static const uint16_t OPT3001_CONFIGURATION_CONVERSION_TIME_800 = 0b100000000000; +static const uint16_t OPT3001_CONFIGURATION_CONVERSION_MODE_MASK = 0b11000000000; +static const uint16_t OPT3001_CONFIGURATION_CONVERSION_MODE_SINGLE_SHOT = 0b01000000000; +static const uint16_t OPT3001_CONFIGURATION_CONVERSION_MODE_SHUTDOWN = 0b00000000000; +// tl;dr: Configure an automatic-ranged, 800ms single shot reading, +// with INT processing disabled +static const uint16_t OPT3001_CONFIGURATION_FULL_RANGE_ONE_SHOT = OPT3001_CONFIGURATION_RANGE_FULL | + OPT3001_CONFIGURATION_CONVERSION_TIME_800 | + OPT3001_CONFIGURATION_CONVERSION_MODE_SINGLE_SHOT; +static const uint16_t OPT3001_CONVERSION_TIME_800 = 825; // give it 25 extra ms; it seems to not be ready quite often + +/* +opt3001 properties: + +- e (exponent) = high 4 bits of result register +- m (mantissa) = low 12 bits of result register +- formula: (0.01 * 2^e) * m lx + +*/ + +void OPT3001Sensor::read_result_(const std::function &f) { + // ensure the single shot flag is clear, indicating it's done + uint16_t raw_value; + if (this->read(reinterpret_cast(&raw_value), 2) != i2c::ERROR_OK) { + ESP_LOGW(TAG, "Reading configuration register failed"); + f(NAN); + return; + } + raw_value = i2c::i2ctohs(raw_value); + + if ((raw_value & OPT3001_CONFIGURATION_CONVERSION_MODE_MASK) != OPT3001_CONFIGURATION_CONVERSION_MODE_SHUTDOWN) { + // not ready; wait 10ms and try again + ESP_LOGW(TAG, "Data not ready; waiting 10ms"); + this->set_timeout("opt3001_wait", 10, [this, f]() { read_result_(f); }); + return; + } + + if (this->read_register(OPT3001_REG_RESULT, reinterpret_cast(&raw_value), 2) != i2c::ERROR_OK) { + ESP_LOGW(TAG, "Reading result register failed"); + f(NAN); + return; + } + raw_value = i2c::i2ctohs(raw_value); + + uint8_t exponent = raw_value >> 12; + uint16_t mantissa = raw_value & 0b111111111111; + + double lx = 0.01 * pow(2.0, double(exponent)) * double(mantissa); + f(float(lx)); +} + +void OPT3001Sensor::read_lx_(const std::function &f) { + // turn on (after one-shot sensor automatically powers down) + uint16_t start_measurement = i2c::htoi2cs(OPT3001_CONFIGURATION_FULL_RANGE_ONE_SHOT); + if (this->write_register(OPT3001_REG_CONFIGURATION, reinterpret_cast(&start_measurement), 2) != + i2c::ERROR_OK) { + ESP_LOGW(TAG, "Triggering one shot measurement failed"); + f(NAN); + return; + } + + this->set_timeout("read", OPT3001_CONVERSION_TIME_800, [this, f]() { + if (this->write(&OPT3001_REG_CONFIGURATION, 1, true) != i2c::ERROR_OK) { + ESP_LOGW(TAG, "Starting configuration register read failed"); + f(NAN); + return; + } + + this->read_result_(f); + }); +} + +void OPT3001Sensor::dump_config() { + LOG_SENSOR("", "OPT3001", this); + LOG_I2C_DEVICE(this); + if (this->is_failed()) { + ESP_LOGE(TAG, ESP_LOG_MSG_COMM_FAIL); + } + + LOG_UPDATE_INTERVAL(this); +} + +void OPT3001Sensor::update() { + // Set a flag and skip just in case the sensor isn't responding, + // and we just keep waiting for it in read_result_. + // This way we don't end up with potentially boundless "threads" + // using up memory and eventually crashing the device + if (this->updating_) { + return; + } + this->updating_ = true; + + this->read_lx_([this](float val) { + this->updating_ = false; + + if (std::isnan(val)) { + this->status_set_warning(); + this->publish_state(NAN); + return; + } + ESP_LOGD(TAG, "'%s': Illuminance=%.1flx", this->get_name().c_str(), val); + this->status_clear_warning(); + this->publish_state(val); + }); +} + +float OPT3001Sensor::get_setup_priority() const { return setup_priority::DATA; } + +} // namespace opt3001 +} // namespace esphome diff --git a/esphome/components/opt3001/opt3001.h b/esphome/components/opt3001/opt3001.h new file mode 100644 index 0000000000..ae3fde5c54 --- /dev/null +++ b/esphome/components/opt3001/opt3001.h @@ -0,0 +1,27 @@ +#pragma once + +#include "esphome/core/component.h" +#include "esphome/components/sensor/sensor.h" +#include "esphome/components/i2c/i2c.h" + +namespace esphome { +namespace opt3001 { + +/// This class implements support for the i2c-based OPT3001 ambient light sensor. +class OPT3001Sensor : public sensor::Sensor, public PollingComponent, public i2c::I2CDevice { + public: + void dump_config() override; + void update() override; + float get_setup_priority() const override; + + protected: + // checks if one-shot is complete before reading the result and returning it + void read_result_(const std::function &f); + // begins a one-shot measurement + void read_lx_(const std::function &f); + + bool updating_{false}; +}; + +} // namespace opt3001 +} // namespace esphome diff --git a/esphome/components/opt3001/sensor.py b/esphome/components/opt3001/sensor.py new file mode 100644 index 0000000000..a5bbf0e8dd --- /dev/null +++ b/esphome/components/opt3001/sensor.py @@ -0,0 +1,35 @@ +import esphome.codegen as cg +import esphome.config_validation as cv +from esphome.components import i2c, sensor +from esphome.const import ( + DEVICE_CLASS_ILLUMINANCE, + STATE_CLASS_MEASUREMENT, + UNIT_LUX, +) + +DEPENDENCIES = ["i2c"] +CODEOWNERS = ["@ccutrer"] + +opt3001_ns = cg.esphome_ns.namespace("opt3001") + +OPT3001Sensor = opt3001_ns.class_( + "OPT3001Sensor", sensor.Sensor, cg.PollingComponent, i2c.I2CDevice +) + +CONFIG_SCHEMA = ( + sensor.sensor_schema( + OPT3001Sensor, + unit_of_measurement=UNIT_LUX, + accuracy_decimals=1, + device_class=DEVICE_CLASS_ILLUMINANCE, + state_class=STATE_CLASS_MEASUREMENT, + ) + .extend(cv.polling_component_schema("60s")) + .extend(i2c.i2c_device_schema(0x44)) +) + + +async def to_code(config): + var = await sensor.new_sensor(config) + await cg.register_component(var, config) + await i2c.register_i2c_device(var, config) diff --git a/tests/components/opt3001/common.yaml b/tests/components/opt3001/common.yaml new file mode 100644 index 0000000000..dab4f824f8 --- /dev/null +++ b/tests/components/opt3001/common.yaml @@ -0,0 +1,10 @@ +i2c: + - id: i2c_opt3001 + scl: ${scl_pin} + sda: ${sda_pin} + +sensor: + - platform: opt3001 + name: Living Room Brightness + address: 0x44 + update_interval: 30s diff --git a/tests/components/opt3001/test.esp32-ard.yaml b/tests/components/opt3001/test.esp32-ard.yaml new file mode 100644 index 0000000000..63c3bd6afd --- /dev/null +++ b/tests/components/opt3001/test.esp32-ard.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO16 + sda_pin: GPIO17 + +<<: !include common.yaml diff --git a/tests/components/opt3001/test.esp32-c3-ard.yaml b/tests/components/opt3001/test.esp32-c3-ard.yaml new file mode 100644 index 0000000000..ee2c29ca4e --- /dev/null +++ b/tests/components/opt3001/test.esp32-c3-ard.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO5 + sda_pin: GPIO4 + +<<: !include common.yaml diff --git a/tests/components/opt3001/test.esp32-c3-idf.yaml b/tests/components/opt3001/test.esp32-c3-idf.yaml new file mode 100644 index 0000000000..ee2c29ca4e --- /dev/null +++ b/tests/components/opt3001/test.esp32-c3-idf.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO5 + sda_pin: GPIO4 + +<<: !include common.yaml diff --git a/tests/components/opt3001/test.esp32-idf.yaml b/tests/components/opt3001/test.esp32-idf.yaml new file mode 100644 index 0000000000..63c3bd6afd --- /dev/null +++ b/tests/components/opt3001/test.esp32-idf.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO16 + sda_pin: GPIO17 + +<<: !include common.yaml diff --git a/tests/components/opt3001/test.esp8266-ard.yaml b/tests/components/opt3001/test.esp8266-ard.yaml new file mode 100644 index 0000000000..ee2c29ca4e --- /dev/null +++ b/tests/components/opt3001/test.esp8266-ard.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO5 + sda_pin: GPIO4 + +<<: !include common.yaml diff --git a/tests/components/opt3001/test.rp2040-ard.yaml b/tests/components/opt3001/test.rp2040-ard.yaml new file mode 100644 index 0000000000..ee2c29ca4e --- /dev/null +++ b/tests/components/opt3001/test.rp2040-ard.yaml @@ -0,0 +1,5 @@ +substitutions: + scl_pin: GPIO5 + sda_pin: GPIO4 + +<<: !include common.yaml From d4e978369a9f376b5e072431aba356f778139e52 Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Tue, 24 Jun 2025 19:56:30 +1200 Subject: [PATCH 11/20] Store reference to device on EntityBase This is so we can get the name of the device to use as part of the object id and to internally set the name for logging. --- esphome/core/entity_base.cpp | 38 ++++++++++++++++++------------------ esphome/core/entity_base.h | 15 +++++++++++--- esphome/cpp_helpers.py | 10 ++++++---- 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/esphome/core/entity_base.cpp b/esphome/core/entity_base.cpp index 791b6615a1..cf91e17a6a 100644 --- a/esphome/core/entity_base.cpp +++ b/esphome/core/entity_base.cpp @@ -11,7 +11,14 @@ const StringRef &EntityBase::get_name() const { return this->name_; } void EntityBase::set_name(const char *name) { this->name_ = StringRef(name); if (this->name_.empty()) { - this->name_ = StringRef(App.get_friendly_name()); +#ifdef USE_DEVICES + if (this->device_ != nullptr) { + this->name_ = StringRef(this->device_->get_name()); + } else +#endif + { + this->name_ = StringRef(App.get_friendly_name()); + } this->flags_.has_own_name = false; } else { this->flags_.has_own_name = true; @@ -29,16 +36,21 @@ void EntityBase::set_icon(const char *icon) { this->icon_c_str_ = icon; } // Entity Object ID std::string EntityBase::get_object_id() const { + std::string suffix = ""; +#ifdef USE_DEVICES + if (this->device_ != nullptr) { + suffix = "@" + str_sanitize(str_snake_case(this->device_->get_name())); + } +#endif // Check if `App.get_friendly_name()` is constant or dynamic. if (!this->flags_.has_own_name && App.is_name_add_mac_suffix_enabled()) { // `App.get_friendly_name()` is dynamic. - return str_sanitize(str_snake_case(App.get_friendly_name())); - } else { - // `App.get_friendly_name()` is constant. + return str_sanitize(str_snake_case(App.get_friendly_name())) + suffix; + } else { // `App.get_friendly_name()` is constant. if (this->object_id_c_str_ == nullptr) { - return ""; + return suffix; } - return this->object_id_c_str_; + return this->object_id_c_str_ + suffix; } } void EntityBase::set_object_id(const char *object_id) { @@ -47,19 +59,7 @@ void EntityBase::set_object_id(const char *object_id) { } // Calculate Object ID Hash from Entity Name -void EntityBase::calc_object_id_() { - // Check if `App.get_friendly_name()` is constant or dynamic. - if (!this->flags_.has_own_name && App.is_name_add_mac_suffix_enabled()) { - // `App.get_friendly_name()` is dynamic. - const auto object_id = str_sanitize(str_snake_case(App.get_friendly_name())); - // FNV-1 hash - this->object_id_hash_ = fnv1_hash(object_id); - } else { - // `App.get_friendly_name()` is constant. - // FNV-1 hash - this->object_id_hash_ = fnv1_hash(this->object_id_c_str_); - } -} +void EntityBase::calc_object_id_() { this->object_id_hash_ = fnv1_hash(this->get_object_id()); } uint32_t EntityBase::get_object_id_hash() { return this->object_id_hash_; } diff --git a/esphome/core/entity_base.h b/esphome/core/entity_base.h index 4bd04a9b1c..4819b66108 100644 --- a/esphome/core/entity_base.h +++ b/esphome/core/entity_base.h @@ -6,6 +6,10 @@ #include "helpers.h" #include "log.h" +#ifdef USE_DEVICES +#include "device.h" +#endif + namespace esphome { enum EntityCategory : uint8_t { @@ -53,8 +57,13 @@ class EntityBase { #ifdef USE_DEVICES // Get/set this entity's device id - uint32_t get_device_id() const { return this->device_id_; } - void set_device_id(const uint32_t device_id) { this->device_id_ = device_id; } + uint32_t get_device_id() const { + if (this->device_ == nullptr) { + return 0; // No device set, return 0 + } + return this->device_->get_device_id(); + } + void set_device(Device *device) { this->device_ = device; } #endif // Check if this entity has state @@ -74,7 +83,7 @@ class EntityBase { const char *icon_c_str_{nullptr}; uint32_t object_id_hash_{}; #ifdef USE_DEVICES - uint32_t device_id_{}; + Device *device_{}; #endif // Bit-packed flags to save memory (1 byte instead of 5) diff --git a/esphome/cpp_helpers.py b/esphome/cpp_helpers.py index 8d5440f591..e50be56092 100644 --- a/esphome/cpp_helpers.py +++ b/esphome/cpp_helpers.py @@ -17,7 +17,7 @@ from esphome.core import CORE, ID, coroutine from esphome.coroutine import FakeAwaitable from esphome.cpp_generator import add, get_variable from esphome.cpp_types import App -from esphome.helpers import fnv1a_32bit_hash, sanitize, snake_case +from esphome.helpers import sanitize, snake_case from esphome.types import ConfigFragmentType, ConfigType from esphome.util import Registry, RegistryEntry @@ -99,6 +99,11 @@ async def register_parented(var, value): async def setup_entity(var, config): """Set up generic properties of an Entity""" + if CONF_DEVICE_ID in config: + device_id: ID = config[CONF_DEVICE_ID] + device = await get_variable(device_id) + add(var.set_device(device)) + add(var.set_name(config[CONF_NAME])) if not config[CONF_NAME]: add(var.set_object_id(sanitize(snake_case(CORE.friendly_name)))) @@ -111,9 +116,6 @@ async def setup_entity(var, config): add(var.set_icon(config[CONF_ICON])) if CONF_ENTITY_CATEGORY in config: add(var.set_entity_category(config[CONF_ENTITY_CATEGORY])) - if CONF_DEVICE_ID in config: - device_id: ID = config[CONF_DEVICE_ID] - add(var.set_device_id(fnv1a_32bit_hash(device_id.id))) def extract_registry_entry_config( From e370872ec1baeb48b864daee44a6a00f7aee0e23 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 16:13:34 +0200 Subject: [PATCH 12/20] fix conflicts --- .../alarm_control_panel/__init__.py | 2 +- esphome/components/binary_sensor/__init__.py | 2 +- esphome/components/button/__init__.py | 2 +- esphome/components/climate/__init__.py | 2 +- esphome/components/cover/__init__.py | 2 +- esphome/components/datetime/__init__.py | 2 +- esphome/components/esp32_camera/__init__.py | 2 +- esphome/components/event/__init__.py | 2 +- esphome/components/fan/__init__.py | 2 +- esphome/components/light/__init__.py | 2 +- esphome/components/lock/__init__.py | 2 +- esphome/components/media_player/__init__.py | 2 +- esphome/components/number/__init__.py | 2 +- esphome/components/select/__init__.py | 2 +- esphome/components/sensor/__init__.py | 2 +- esphome/components/switch/__init__.py | 2 +- esphome/components/text/__init__.py | 2 +- esphome/components/text_sensor/__init__.py | 2 +- esphome/components/update/__init__.py | 2 +- esphome/components/valve/__init__.py | 2 +- esphome/core/__init__.py | 4 + esphome/core/entity_base.cpp | 12 +- esphome/cpp_helpers.py | 61 ++++++++- tests/unit_tests/conftest.py | 9 ++ tests/unit_tests/test_duplicate_entities.py | 129 ++++++++++++++++++ 25 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 tests/unit_tests/test_duplicate_entities.py diff --git a/esphome/components/alarm_control_panel/__init__.py b/esphome/components/alarm_control_panel/__init__.py index e88050132a..3c35076de9 100644 --- a/esphome/components/alarm_control_panel/__init__.py +++ b/esphome/components/alarm_control_panel/__init__.py @@ -190,7 +190,7 @@ ALARM_CONTROL_PANEL_CONDITION_SCHEMA = maybe_simple_id( async def setup_alarm_control_panel_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "alarm_control_panel") for conf in config.get(CONF_ON_STATE, []): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) await automation.build_automation(trigger, [], conf) diff --git a/esphome/components/binary_sensor/__init__.py b/esphome/components/binary_sensor/__init__.py index bc26c09622..b34477d30a 100644 --- a/esphome/components/binary_sensor/__init__.py +++ b/esphome/components/binary_sensor/__init__.py @@ -521,7 +521,7 @@ BINARY_SENSOR_SCHEMA.add_extra(cv.deprecated_schema_constant("binary_sensor")) async def setup_binary_sensor_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "binary_sensor") if (device_class := config.get(CONF_DEVICE_CLASS)) is not None: cg.add(var.set_device_class(device_class)) diff --git a/esphome/components/button/__init__.py b/esphome/components/button/__init__.py index 892bf62f3a..c63073dd38 100644 --- a/esphome/components/button/__init__.py +++ b/esphome/components/button/__init__.py @@ -87,7 +87,7 @@ BUTTON_SCHEMA.add_extra(cv.deprecated_schema_constant("button")) async def setup_button_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "button") for conf in config.get(CONF_ON_PRESS, []): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) diff --git a/esphome/components/climate/__init__.py b/esphome/components/climate/__init__.py index 52938a17d0..ff00565abf 100644 --- a/esphome/components/climate/__init__.py +++ b/esphome/components/climate/__init__.py @@ -273,7 +273,7 @@ CLIMATE_SCHEMA.add_extra(cv.deprecated_schema_constant("climate")) async def setup_climate_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "climate") visual = config[CONF_VISUAL] if (min_temp := visual.get(CONF_MIN_TEMPERATURE)) is not None: diff --git a/esphome/components/cover/__init__.py b/esphome/components/cover/__init__.py index 9fe7593eab..c7aec6493b 100644 --- a/esphome/components/cover/__init__.py +++ b/esphome/components/cover/__init__.py @@ -154,7 +154,7 @@ COVER_SCHEMA.add_extra(cv.deprecated_schema_constant("cover")) async def setup_cover_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "cover") if (device_class := config.get(CONF_DEVICE_CLASS)) is not None: cg.add(var.set_device_class(device_class)) diff --git a/esphome/components/datetime/__init__.py b/esphome/components/datetime/__init__.py index 24fbf5a1ec..42b29227c3 100644 --- a/esphome/components/datetime/__init__.py +++ b/esphome/components/datetime/__init__.py @@ -133,7 +133,7 @@ def datetime_schema(class_: MockObjClass) -> cv.Schema: async def setup_datetime_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "datetime") if (mqtt_id := config.get(CONF_MQTT_ID)) is not None: mqtt_ = cg.new_Pvariable(mqtt_id, var) diff --git a/esphome/components/esp32_camera/__init__.py b/esphome/components/esp32_camera/__init__.py index 05522265ae..68ba1ae549 100644 --- a/esphome/components/esp32_camera/__init__.py +++ b/esphome/components/esp32_camera/__init__.py @@ -284,7 +284,7 @@ SETTERS = { async def to_code(config): var = cg.new_Pvariable(config[CONF_ID]) - await setup_entity(var, config) + await setup_entity(var, config, "camera") await cg.register_component(var, config) for key, setter in SETTERS.items(): diff --git a/esphome/components/event/__init__.py b/esphome/components/event/__init__.py index e7ab489a25..1ff0d4e3d5 100644 --- a/esphome/components/event/__init__.py +++ b/esphome/components/event/__init__.py @@ -88,7 +88,7 @@ EVENT_SCHEMA.add_extra(cv.deprecated_schema_constant("event")) async def setup_event_core_(var, config, *, event_types: list[str]): - await setup_entity(var, config) + await setup_entity(var, config, "event") for conf in config.get(CONF_ON_EVENT, []): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) diff --git a/esphome/components/fan/__init__.py b/esphome/components/fan/__init__.py index c6ff938cd6..bebf760b0b 100644 --- a/esphome/components/fan/__init__.py +++ b/esphome/components/fan/__init__.py @@ -225,7 +225,7 @@ def validate_preset_modes(value): async def setup_fan_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "fan") cg.add(var.set_restore_mode(config[CONF_RESTORE_MODE])) diff --git a/esphome/components/light/__init__.py b/esphome/components/light/__init__.py index a013029fc2..902d661eb5 100644 --- a/esphome/components/light/__init__.py +++ b/esphome/components/light/__init__.py @@ -207,7 +207,7 @@ def validate_color_temperature_channels(value): async def setup_light_core_(light_var, output_var, config): - await setup_entity(light_var, config) + await setup_entity(light_var, config, "light") cg.add(light_var.set_restore_mode(config[CONF_RESTORE_MODE])) diff --git a/esphome/components/lock/__init__.py b/esphome/components/lock/__init__.py index 0fb67e3948..aa1061de53 100644 --- a/esphome/components/lock/__init__.py +++ b/esphome/components/lock/__init__.py @@ -94,7 +94,7 @@ LOCK_SCHEMA.add_extra(cv.deprecated_schema_constant("lock")) async def _setup_lock_core(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "lock") for conf in config.get(CONF_ON_LOCK, []): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) diff --git a/esphome/components/media_player/__init__.py b/esphome/components/media_player/__init__.py index ef76419de3..c01bd24890 100644 --- a/esphome/components/media_player/__init__.py +++ b/esphome/components/media_player/__init__.py @@ -81,7 +81,7 @@ IsAnnouncingCondition = media_player_ns.class_( async def setup_media_player_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "media_player") for conf in config.get(CONF_ON_STATE, []): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) await automation.build_automation(trigger, [], conf) diff --git a/esphome/components/number/__init__.py b/esphome/components/number/__init__.py index 2567d9ffe1..65a00bfe2f 100644 --- a/esphome/components/number/__init__.py +++ b/esphome/components/number/__init__.py @@ -237,7 +237,7 @@ NUMBER_SCHEMA.add_extra(cv.deprecated_schema_constant("number")) async def setup_number_core_( var, config, *, min_value: float, max_value: float, step: float ): - await setup_entity(var, config) + await setup_entity(var, config, "number") cg.add(var.traits.set_min_value(min_value)) cg.add(var.traits.set_max_value(max_value)) diff --git a/esphome/components/select/__init__.py b/esphome/components/select/__init__.py index e14a9351a0..c3f8abec8f 100644 --- a/esphome/components/select/__init__.py +++ b/esphome/components/select/__init__.py @@ -89,7 +89,7 @@ SELECT_SCHEMA.add_extra(cv.deprecated_schema_constant("select")) async def setup_select_core_(var, config, *, options: list[str]): - await setup_entity(var, config) + await setup_entity(var, config, "select") cg.add(var.traits.set_options(options)) diff --git a/esphome/components/sensor/__init__.py b/esphome/components/sensor/__init__.py index 1ad3cfabee..749b7992b8 100644 --- a/esphome/components/sensor/__init__.py +++ b/esphome/components/sensor/__init__.py @@ -787,7 +787,7 @@ async def build_filters(config): async def setup_sensor_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "sensor") if (device_class := config.get(CONF_DEVICE_CLASS)) is not None: cg.add(var.set_device_class(device_class)) diff --git a/esphome/components/switch/__init__.py b/esphome/components/switch/__init__.py index 0211c648fc..322d547e95 100644 --- a/esphome/components/switch/__init__.py +++ b/esphome/components/switch/__init__.py @@ -131,7 +131,7 @@ SWITCH_SCHEMA.add_extra(cv.deprecated_schema_constant("switch")) async def setup_switch_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "switch") if (inverted := config.get(CONF_INVERTED)) is not None: cg.add(var.set_inverted(inverted)) diff --git a/esphome/components/text/__init__.py b/esphome/components/text/__init__.py index 40b3a90d6b..fc1b3d1b05 100644 --- a/esphome/components/text/__init__.py +++ b/esphome/components/text/__init__.py @@ -94,7 +94,7 @@ async def setup_text_core_( max_length: int | None, pattern: str | None, ): - await setup_entity(var, config) + await setup_entity(var, config, "text") cg.add(var.traits.set_min_length(min_length)) cg.add(var.traits.set_max_length(max_length)) diff --git a/esphome/components/text_sensor/__init__.py b/esphome/components/text_sensor/__init__.py index c7ac17c35a..38f0ae451e 100644 --- a/esphome/components/text_sensor/__init__.py +++ b/esphome/components/text_sensor/__init__.py @@ -186,7 +186,7 @@ async def build_filters(config): async def setup_text_sensor_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "text_sensor") if (device_class := config.get(CONF_DEVICE_CLASS)) is not None: cg.add(var.set_device_class(device_class)) diff --git a/esphome/components/update/__init__.py b/esphome/components/update/__init__.py index 09b0698903..061dd4589f 100644 --- a/esphome/components/update/__init__.py +++ b/esphome/components/update/__init__.py @@ -87,7 +87,7 @@ UPDATE_SCHEMA.add_extra(cv.deprecated_schema_constant("update")) async def setup_update_core_(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "update") if device_class_config := config.get(CONF_DEVICE_CLASS): cg.add(var.set_device_class(device_class_config)) diff --git a/esphome/components/valve/__init__.py b/esphome/components/valve/__init__.py index a6f1428cd2..98c96f9afc 100644 --- a/esphome/components/valve/__init__.py +++ b/esphome/components/valve/__init__.py @@ -132,7 +132,7 @@ VALVE_SCHEMA.add_extra(cv.deprecated_schema_constant("valve")) async def _setup_valve_core(var, config): - await setup_entity(var, config) + await setup_entity(var, config, "valve") if device_class_config := config.get(CONF_DEVICE_CLASS): cg.add(var.set_device_class(device_class_config)) diff --git a/esphome/core/__init__.py b/esphome/core/__init__.py index bc98ff54db..00c1db33ee 100644 --- a/esphome/core/__init__.py +++ b/esphome/core/__init__.py @@ -522,6 +522,9 @@ class EsphomeCore: # Dict to track platform entity counts for pre-allocation # 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 + # Key: (device_id, platform, object_id), Value: count of duplicates + self.unique_ids: dict[tuple[int, str, str], int] = {} # Whether ESPHome was started in verbose mode self.verbose = False # Whether ESPHome was started in quiet mode @@ -553,6 +556,7 @@ class EsphomeCore: self.loaded_integrations = set() self.component_ids = set() self.platform_counts = defaultdict(int) + self.unique_ids = {} PIN_SCHEMA_REGISTRY.reset() @property diff --git a/esphome/core/entity_base.cpp b/esphome/core/entity_base.cpp index cf91e17a6a..7b86130f2f 100644 --- a/esphome/core/entity_base.cpp +++ b/esphome/core/entity_base.cpp @@ -36,21 +36,15 @@ void EntityBase::set_icon(const char *icon) { this->icon_c_str_ = icon; } // Entity Object ID std::string EntityBase::get_object_id() const { - std::string suffix = ""; -#ifdef USE_DEVICES - if (this->device_ != nullptr) { - suffix = "@" + str_sanitize(str_snake_case(this->device_->get_name())); - } -#endif // Check if `App.get_friendly_name()` is constant or dynamic. if (!this->flags_.has_own_name && App.is_name_add_mac_suffix_enabled()) { // `App.get_friendly_name()` is dynamic. - return str_sanitize(str_snake_case(App.get_friendly_name())) + suffix; + return str_sanitize(str_snake_case(App.get_friendly_name())); } else { // `App.get_friendly_name()` is constant. if (this->object_id_c_str_ == nullptr) { - return suffix; + return ""; } - return this->object_id_c_str_ + suffix; + return this->object_id_c_str_; } } void EntityBase::set_object_id(const char *object_id) { diff --git a/esphome/cpp_helpers.py b/esphome/cpp_helpers.py index e50be56092..ee91ac6132 100644 --- a/esphome/cpp_helpers.py +++ b/esphome/cpp_helpers.py @@ -15,7 +15,7 @@ from esphome.const import ( ) from esphome.core import CORE, ID, coroutine from esphome.coroutine import FakeAwaitable -from esphome.cpp_generator import add, get_variable +from esphome.cpp_generator import MockObj, add, get_variable from esphome.cpp_types import App from esphome.helpers import sanitize, snake_case from esphome.types import ConfigFragmentType, ConfigType @@ -97,18 +97,65 @@ async def register_parented(var, value): add(var.set_parent(paren)) -async def setup_entity(var, config): - """Set up generic properties of an Entity""" +async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: + """Set up generic properties of an Entity. + + This function handles duplicate entity names by automatically appending + a suffix (_2, _3, etc.) when multiple entities have the same object_id + within the same platform and device combination. + + Args: + var: The entity variable to set up + config: Configuration dictionary containing entity settings + platform: The platform name (e.g., "sensor", "binary_sensor") + """ + # Get device info + device_id: int = 0 if CONF_DEVICE_ID in config: - device_id: ID = config[CONF_DEVICE_ID] - device = await get_variable(device_id) + device_id_obj: ID = config[CONF_DEVICE_ID] + device: MockObj = await get_variable(device_id_obj) add(var.set_device(device)) + # Use the device's ID hash as device_id + from esphome.helpers import fnv1a_32bit_hash + + device_id = fnv1a_32bit_hash(device_id_obj.id) add(var.set_name(config[CONF_NAME])) + + # Calculate base object_id + base_object_id: str if not config[CONF_NAME]: - add(var.set_object_id(sanitize(snake_case(CORE.friendly_name)))) + # Use the friendly name if available, otherwise use the device name + if CORE.friendly_name: + base_object_id = sanitize(snake_case(CORE.friendly_name)) + else: + base_object_id = sanitize(snake_case(CORE.name)) + _LOGGER.debug( + "Entity has empty name, using '%s' as object_id base", base_object_id + ) else: - add(var.set_object_id(sanitize(snake_case(config[CONF_NAME])))) + base_object_id = sanitize(snake_case(config[CONF_NAME])) + + # Handle duplicates + # Check for duplicates + unique_key: tuple[int, str, str] = (device_id, platform, base_object_id) + if unique_key in CORE.unique_ids: + # Found duplicate, add suffix + count = CORE.unique_ids[unique_key] + 1 + CORE.unique_ids[unique_key] = count + object_id = f"{base_object_id}_{count}" + _LOGGER.info( + "Duplicate %s entity '%s' found. Renaming to '%s'", + platform, + config[CONF_NAME], + object_id, + ) + else: + # First occurrence + CORE.unique_ids[unique_key] = 1 + object_id = base_object_id + + add(var.set_object_id(object_id)) add(var.set_disabled_by_default(config[CONF_DISABLED_BY_DEFAULT])) if CONF_INTERNAL in config: add(var.set_internal(config[CONF_INTERNAL])) diff --git a/tests/unit_tests/conftest.py b/tests/unit_tests/conftest.py index 955869b799..aac5a642f6 100644 --- a/tests/unit_tests/conftest.py +++ b/tests/unit_tests/conftest.py @@ -14,6 +14,8 @@ import sys import pytest +from esphome.core import CORE + here = Path(__file__).parent # Configure location of package root @@ -21,6 +23,13 @@ package_root = here.parent.parent sys.path.insert(0, package_root.as_posix()) +@pytest.fixture(autouse=True) +def reset_core(): + """Reset CORE after each test.""" + yield + CORE.reset() + + @pytest.fixture def fixture_path() -> Path: """ diff --git a/tests/unit_tests/test_duplicate_entities.py b/tests/unit_tests/test_duplicate_entities.py new file mode 100644 index 0000000000..ab075a02fc --- /dev/null +++ b/tests/unit_tests/test_duplicate_entities.py @@ -0,0 +1,129 @@ +"""Test duplicate entity object ID handling.""" + +import pytest + +from esphome.core import CORE +from esphome.helpers import sanitize, snake_case + + +@pytest.fixture +def setup_test_device() -> None: + """Set up test device configuration.""" + CORE.name = "test-device" + CORE.friendly_name = "Test Device" + + +def test_unique_key_generation() -> None: + """Test that unique keys are generated correctly.""" + # Test with no device + key1: tuple[int, str, str] = (0, "binary_sensor", "temperature") + assert key1 == (0, "binary_sensor", "temperature") + + # Test with device + key2: tuple[int, str, str] = (12345, "sensor", "humidity") + assert key2 == (12345, "sensor", "humidity") + + +def test_duplicate_tracking() -> None: + """Test that duplicates are tracked correctly.""" + # First occurrence + key: tuple[int, str, str] = (0, "sensor", "temperature") + assert key not in CORE.unique_ids + + CORE.unique_ids[key] = 1 + assert CORE.unique_ids[key] == 1 + + # Second occurrence + count: int = CORE.unique_ids[key] + 1 + CORE.unique_ids[key] = count + assert CORE.unique_ids[key] == 2 + + +def test_object_id_sanitization() -> None: + """Test that object IDs are properly sanitized.""" + # Test various inputs + assert sanitize(snake_case("Temperature Sensor")) == "temperature_sensor" + assert sanitize(snake_case("Living Room Light!")) == "living_room_light_" + assert sanitize(snake_case("Test-Device")) == "test-device" + assert sanitize(snake_case("")) == "" + + +def test_suffix_generation() -> None: + """Test that suffixes are generated correctly.""" + base_id: str = "temperature" + + # No suffix for first occurrence + object_id_1: str = base_id + assert object_id_1 == "temperature" + + # Add suffix for duplicates + count: int = 2 + object_id_2: str = f"{base_id}_{count}" + assert object_id_2 == "temperature_2" + + count = 3 + object_id_3: str = f"{base_id}_{count}" + assert object_id_3 == "temperature_3" + + +def test_different_platforms_same_name() -> None: + """Test that same name on different platforms doesn't conflict.""" + # Simulate two entities with same name on different platforms + key1: tuple[int, str, str] = (0, "binary_sensor", "status") + key2: tuple[int, str, str] = (0, "text_sensor", "status") + + # They should be different keys + assert key1 != key2 + + # Track them separately + CORE.unique_ids[key1] = 1 + CORE.unique_ids[key2] = 1 + + # Both should be at count 1 (no conflict) + assert CORE.unique_ids[key1] == 1 + assert CORE.unique_ids[key2] == 1 + + +def test_different_devices_same_name_platform() -> None: + """Test that same name+platform on different devices doesn't conflict.""" + # Simulate two entities with same name and platform but different devices + key1: tuple[int, str, str] = (12345, "sensor", "temperature") + key2: tuple[int, str, str] = (67890, "sensor", "temperature") + + # They should be different keys + assert key1 != key2 + + # Track them separately + CORE.unique_ids[key1] = 1 + CORE.unique_ids[key2] = 1 + + # Both should be at count 1 (no conflict) + assert CORE.unique_ids[key1] == 1 + assert CORE.unique_ids[key2] == 1 + + +def test_empty_name_handling(setup_test_device: None) -> None: + """Test handling of entities with empty names.""" + # When name is empty, it should use the device name + empty_name: str = "" + base_id: str + if not empty_name: + if CORE.friendly_name: + base_id = sanitize(snake_case(CORE.friendly_name)) + else: + base_id = sanitize(snake_case(CORE.name)) + + assert base_id == "test_device" # Uses friendly name + + +def test_reset_clears_unique_ids() -> None: + """Test that CORE.reset() clears the unique_ids tracking.""" + # Add some tracked IDs + CORE.unique_ids[(0, "sensor", "test")] = 2 + CORE.unique_ids[(0, "binary_sensor", "test")] = 3 + + assert len(CORE.unique_ids) == 2 + + # Reset should clear them + CORE.reset() + assert len(CORE.unique_ids) == 0 From c3776240b632571eed36829f192f91071bbaba0b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:03:23 +0200 Subject: [PATCH 13/20] fixes --- esphome/cpp_helpers.py | 21 +- esphome/entity.py | 41 ++++ .../fixtures/duplicate_entities.yaml | 118 +++++++++++ tests/integration/test_duplicate_entities.py | 187 ++++++++++++++++++ .../test_get_base_entity_object_id.py | 140 +++++++++++++ 5 files changed, 497 insertions(+), 10 deletions(-) create mode 100644 esphome/entity.py create mode 100644 tests/integration/fixtures/duplicate_entities.yaml create mode 100644 tests/integration/test_duplicate_entities.py create mode 100644 tests/unit_tests/test_get_base_entity_object_id.py diff --git a/esphome/cpp_helpers.py b/esphome/cpp_helpers.py index ee91ac6132..a1289485ca 100644 --- a/esphome/cpp_helpers.py +++ b/esphome/cpp_helpers.py @@ -17,7 +17,7 @@ from esphome.core import CORE, ID, coroutine from esphome.coroutine import FakeAwaitable from esphome.cpp_generator import MockObj, add, get_variable from esphome.cpp_types import App -from esphome.helpers import sanitize, snake_case +from esphome.entity import get_base_entity_object_id from esphome.types import ConfigFragmentType, ConfigType from esphome.util import Registry, RegistryEntry @@ -122,19 +122,14 @@ async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: add(var.set_name(config[CONF_NAME])) - # Calculate base object_id - base_object_id: str + # Calculate base object_id using the same logic as C++ + # This must match the C++ behavior in esphome/core/entity_base.cpp + base_object_id = get_base_entity_object_id(config[CONF_NAME], CORE.friendly_name) + if not config[CONF_NAME]: - # Use the friendly name if available, otherwise use the device name - if CORE.friendly_name: - base_object_id = sanitize(snake_case(CORE.friendly_name)) - else: - base_object_id = sanitize(snake_case(CORE.name)) _LOGGER.debug( "Entity has empty name, using '%s' as object_id base", base_object_id ) - else: - base_object_id = sanitize(snake_case(config[CONF_NAME])) # Handle duplicates # Check for duplicates @@ -156,6 +151,12 @@ async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: object_id = base_object_id add(var.set_object_id(object_id)) + _LOGGER.debug( + "Setting object_id '%s' for entity '%s' on platform '%s'", + object_id, + config[CONF_NAME], + platform, + ) add(var.set_disabled_by_default(config[CONF_DISABLED_BY_DEFAULT])) if CONF_INTERNAL in config: add(var.set_internal(config[CONF_INTERNAL])) diff --git a/esphome/entity.py b/esphome/entity.py new file mode 100644 index 0000000000..732822d0ff --- /dev/null +++ b/esphome/entity.py @@ -0,0 +1,41 @@ +"""Entity-related helper functions.""" + +from esphome.core import CORE +from esphome.helpers import sanitize, snake_case + + +def get_base_entity_object_id(name: str, friendly_name: str | None) -> str: + """Calculate the base object ID for an entity that will be set via set_object_id(). + + This function calculates what object_id_c_str_ should be set to in C++. + + The C++ EntityBase::get_object_id() (entity_base.cpp lines 38-49) works as: + - If !has_own_name && is_name_add_mac_suffix_enabled(): + return str_sanitize(str_snake_case(App.get_friendly_name())) // Dynamic + - Else: + return object_id_c_str_ ?? "" // What we set via set_object_id() + + Since we're calculating what to pass to set_object_id(), we always need to + generate the object_id the same way, regardless of name_add_mac_suffix setting. + + Args: + name: The entity name (empty string if no name) + friendly_name: The friendly name from CORE.friendly_name + + Returns: + The base object ID to use for duplicate checking and to pass to set_object_id() + """ + + if name: + # Entity has its own name (has_own_name will be true) + base_str = name + elif friendly_name: + # Entity has empty name (has_own_name will be false) + # Calculate what the object_id should be + # C++ uses App.get_friendly_name() which returns friendly_name or device name + base_str = friendly_name + else: + # Fallback to device name + base_str = CORE.name + + return sanitize(snake_case(base_str)) diff --git a/tests/integration/fixtures/duplicate_entities.yaml b/tests/integration/fixtures/duplicate_entities.yaml new file mode 100644 index 0000000000..0f831db90d --- /dev/null +++ b/tests/integration/fixtures/duplicate_entities.yaml @@ -0,0 +1,118 @@ +esphome: + name: duplicate-entities-test + # Define devices to test multi-device duplicate handling + devices: + - id: controller_1 + name: Controller 1 + - id: controller_2 + name: Controller 2 + +host: +api: # Port will be automatically injected +logger: + +# Create duplicate entities across different scenarios + +# Scenario 1: Multiple sensors with same name on same device (should get _2, _3, _4) +sensor: + - platform: template + name: Temperature + lambda: return 1.0; + update_interval: 0.1s + + - platform: template + name: Temperature + lambda: return 2.0; + update_interval: 0.1s + + - platform: template + name: Temperature + lambda: return 3.0; + update_interval: 0.1s + + - platform: template + name: Temperature + lambda: return 4.0; + update_interval: 0.1s + + # Scenario 2: Device-specific duplicates using device_id configuration + - platform: template + name: Device Temperature + device_id: controller_1 + lambda: return 10.0; + update_interval: 0.1s + + - platform: template + name: Device Temperature + device_id: controller_1 + lambda: return 11.0; + update_interval: 0.1s + + - platform: template + name: Device Temperature + device_id: controller_1 + lambda: return 12.0; + update_interval: 0.1s + + # Different device, same name - should not conflict + - platform: template + name: Device Temperature + device_id: controller_2 + lambda: return 20.0; + update_interval: 0.1s + +# Scenario 3: Binary sensors (different platform, same name) +binary_sensor: + - platform: template + name: Temperature + lambda: return true; + + - platform: template + name: Temperature + lambda: return false; + + - platform: template + name: Temperature + lambda: return true; + + # Scenario 5: Binary sensors on devices + - platform: template + name: Device Temperature + device_id: controller_1 + lambda: return true; + + - platform: template + name: Device Temperature + device_id: controller_2 + lambda: return false; + +# Scenario 6: Test with special characters that need sanitization +text_sensor: + - platform: template + name: "Status Message!" + lambda: return {"status1"}; + update_interval: 0.1s + + - platform: template + name: "Status Message!" + lambda: return {"status2"}; + update_interval: 0.1s + + - platform: template + name: "Status Message!" + lambda: return {"status3"}; + update_interval: 0.1s + +# Scenario 7: More switch duplicates +switch: + - platform: template + name: "Power Switch" + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "Power Switch" + lambda: return true; + turn_on_action: [] + turn_off_action: [] diff --git a/tests/integration/test_duplicate_entities.py b/tests/integration/test_duplicate_entities.py new file mode 100644 index 0000000000..edbcb9799c --- /dev/null +++ b/tests/integration/test_duplicate_entities.py @@ -0,0 +1,187 @@ +"""Integration test for duplicate entity handling.""" + +from __future__ import annotations + +import asyncio + +from aioesphomeapi import EntityInfo +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_duplicate_entities( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that duplicate entity names are automatically suffixed with _2, _3, _4.""" + async with run_compiled(yaml_config), api_client_connected() as client: + # Get device info + device_info = await client.device_info() + assert device_info is not None + + # Get devices + devices = device_info.devices + assert len(devices) >= 2, f"Expected at least 2 devices, got {len(devices)}" + + # Find our test devices + controller_1 = next((d for d in devices if d.name == "Controller 1"), None) + controller_2 = next((d for d in devices if d.name == "Controller 2"), None) + + assert controller_1 is not None, "Controller 1 device not found" + assert controller_2 is not None, "Controller 2 device not found" + + # Get entity list + entities = await client.list_entities_services() + all_entities: list[EntityInfo] = [] + for entity_list in entities[0]: + if hasattr(entity_list, "object_id"): + all_entities.append(entity_list) + + # Group entities by type for easier testing + sensors = [e for e in all_entities if e.__class__.__name__ == "SensorInfo"] + binary_sensors = [ + e for e in all_entities if e.__class__.__name__ == "BinarySensorInfo" + ] + text_sensors = [ + e for e in all_entities if e.__class__.__name__ == "TextSensorInfo" + ] + switches = [e for e in all_entities if e.__class__.__name__ == "SwitchInfo"] + + # Scenario 1: Check sensors with duplicate "Temperature" names + temp_sensors = [s for s in sensors if s.name == "Temperature"] + temp_object_ids = sorted([s.object_id for s in temp_sensors]) + + # Should have temperature, temperature_2, temperature_3, temperature_4 + assert len(temp_object_ids) >= 4, ( + f"Expected at least 4 temperature sensors, got {len(temp_object_ids)}" + ) + assert "temperature" in temp_object_ids, ( + "First temperature sensor should not have suffix" + ) + assert "temperature_2" in temp_object_ids, ( + "Second temperature sensor should be temperature_2" + ) + assert "temperature_3" in temp_object_ids, ( + "Third temperature sensor should be temperature_3" + ) + assert "temperature_4" in temp_object_ids, ( + "Fourth temperature sensor should be temperature_4" + ) + + # Scenario 2: Check device-specific sensors don't conflict + device_temp_sensors = [s for s in sensors if s.name == "Device Temperature"] + + # Group by device + controller_1_temps = [ + s + for s in device_temp_sensors + if getattr(s, "device_id", None) == controller_1.device_id + ] + controller_2_temps = [ + s + for s in device_temp_sensors + if getattr(s, "device_id", None) == controller_2.device_id + ] + + # Controller 1 should have device_temperature, device_temperature_2, device_temperature_3 + c1_object_ids = sorted([s.object_id for s in controller_1_temps]) + assert len(c1_object_ids) >= 3, ( + f"Expected at least 3 sensors on controller_1, got {len(c1_object_ids)}" + ) + assert "device_temperature" in c1_object_ids, ( + "First device sensor should not have suffix" + ) + assert "device_temperature_2" in c1_object_ids, ( + "Second device sensor should be device_temperature_2" + ) + assert "device_temperature_3" in c1_object_ids, ( + "Third device sensor should be device_temperature_3" + ) + + # Controller 2 should have only device_temperature (no suffix) + c2_object_ids = [s.object_id for s in controller_2_temps] + assert len(c2_object_ids) >= 1, ( + f"Expected at least 1 sensor on controller_2, got {len(c2_object_ids)}" + ) + assert "device_temperature" in c2_object_ids, ( + "Controller 2 sensor should not have suffix" + ) + + # Scenario 3: Check binary sensors (different platform, same name) + temp_binary = [b for b in binary_sensors if b.name == "Temperature"] + binary_object_ids = sorted([b.object_id for b in temp_binary]) + + # Should have temperature, temperature_2, temperature_3 (no conflict with sensor platform) + assert len(binary_object_ids) >= 3, ( + f"Expected at least 3 binary sensors, got {len(binary_object_ids)}" + ) + assert "temperature" in binary_object_ids, ( + "First binary sensor should not have suffix" + ) + assert "temperature_2" in binary_object_ids, ( + "Second binary sensor should be temperature_2" + ) + assert "temperature_3" in binary_object_ids, ( + "Third binary sensor should be temperature_3" + ) + + # Scenario 4: Check text sensors with special characters + status_sensors = [t for t in text_sensors if t.name == "Status Message!"] + status_object_ids = sorted([t.object_id for t in status_sensors]) + + # Special characters should be sanitized to _ + assert len(status_object_ids) >= 3, ( + f"Expected at least 3 status sensors, got {len(status_object_ids)}" + ) + assert "status_message_" in status_object_ids, ( + "First status sensor should be status_message_" + ) + assert "status_message__2" in status_object_ids, ( + "Second status sensor should be status_message__2" + ) + assert "status_message__3" in status_object_ids, ( + "Third status sensor should be status_message__3" + ) + + # Scenario 5: Check switches with duplicate names + power_switches = [s for s in switches if s.name == "Power Switch"] + power_object_ids = sorted([s.object_id for s in power_switches]) + + # Should have power_switch, power_switch_2 + assert len(power_object_ids) >= 2, ( + f"Expected at least 2 power switches, got {len(power_object_ids)}" + ) + assert "power_switch" in power_object_ids, ( + "First power switch should be power_switch" + ) + assert "power_switch_2" in power_object_ids, ( + "Second power switch should be power_switch_2" + ) + + # Verify we can get states for all entities (ensures they're functional) + loop = asyncio.get_running_loop() + states_future: asyncio.Future[bool] = loop.create_future() + state_count = 0 + expected_count = ( + len(sensors) + len(binary_sensors) + len(text_sensors) + len(switches) + ) + + def on_state(state) -> None: + nonlocal state_count + state_count += 1 + if state_count >= expected_count and not states_future.done(): + states_future.set_result(True) + + client.subscribe_states(on_state) + + # Wait for all entity states + try: + await asyncio.wait_for(states_future, timeout=10.0) + except asyncio.TimeoutError: + pytest.fail( + f"Did not receive all entity states within 10 seconds. " + f"Expected {expected_count}, received {state_count}" + ) diff --git a/tests/unit_tests/test_get_base_entity_object_id.py b/tests/unit_tests/test_get_base_entity_object_id.py new file mode 100644 index 0000000000..aeea862d78 --- /dev/null +++ b/tests/unit_tests/test_get_base_entity_object_id.py @@ -0,0 +1,140 @@ +"""Test get_base_entity_object_id function matches C++ behavior.""" + +from esphome.core import CORE +from esphome.entity import get_base_entity_object_id +from esphome.helpers import sanitize, snake_case + + +class TestGetBaseEntityObjectId: + """Test that get_base_entity_object_id matches C++ EntityBase::get_object_id behavior.""" + + def test_with_entity_name(self) -> None: + """Test when entity has its own name - should use entity name.""" + # Simple name + assert ( + get_base_entity_object_id("Temperature Sensor", None) + == "temperature_sensor" + ) + assert ( + get_base_entity_object_id("Temperature Sensor", "Device Name") + == "temperature_sensor" + ) + + # Name with special characters + assert ( + get_base_entity_object_id("Temp!@#$%^&*()Sensor", None) + == "temp__________sensor" + ) + assert get_base_entity_object_id("Temp-Sensor_123", None) == "temp-sensor_123" + + # Already snake_case + assert ( + get_base_entity_object_id("temperature_sensor", None) + == "temperature_sensor" + ) + + # Mixed case + assert ( + get_base_entity_object_id("TemperatureSensor", None) == "temperaturesensor" + ) + assert ( + get_base_entity_object_id("TEMPERATURE SENSOR", None) + == "temperature_sensor" + ) + + def test_empty_name_with_friendly_name(self) -> None: + """Test when entity has empty name - should use friendly name.""" + # C++ behavior: when has_own_name is false, uses App.get_friendly_name() + assert get_base_entity_object_id("", "Friendly Device") == "friendly_device" + assert ( + get_base_entity_object_id("", "Kitchen Controller") == "kitchen_controller" + ) + assert get_base_entity_object_id("", "Test-Device_123") == "test-device_123" + + # Special characters in friendly name + assert get_base_entity_object_id("", "Device!@#$%") == "device_____" + + def test_empty_name_no_friendly_name(self) -> None: + """Test when entity has empty name and no friendly name - should use device name.""" + # Save original values + original_name = getattr(CORE, "name", None) + + try: + # Test with CORE.name set + CORE.name = "device-name" + assert get_base_entity_object_id("", None) == "device-name" + + CORE.name = "Test Device" + assert get_base_entity_object_id("", None) == "test_device" + + finally: + # Restore original value + if original_name is not None: + CORE.name = original_name + + def test_edge_cases(self) -> None: + """Test edge cases.""" + # Only spaces + assert get_base_entity_object_id(" ", None) == "___" + + # Unicode characters (should be replaced) + assert get_base_entity_object_id("Température", None) == "temp_rature" + assert get_base_entity_object_id("测试", None) == "__" + + # Empty string with empty friendly name (empty friendly name is treated as None) + # Falls back to CORE.name + original_name = getattr(CORE, "name", None) + try: + CORE.name = "device" + assert get_base_entity_object_id("", "") == "device" + finally: + if original_name is not None: + CORE.name = original_name + + # Very long name (should work fine) + long_name = "a" * 100 + " " + "b" * 100 + expected = "a" * 100 + "_" + "b" * 100 + assert get_base_entity_object_id(long_name, None) == expected + + def test_matches_cpp_helpers(self) -> None: + """Test that the logic matches using snake_case and sanitize directly.""" + test_cases = [ + ("Temperature Sensor", "temperature_sensor"), + ("Living Room Light", "living_room_light"), + ("Test-Device_123", "test-device_123"), + ("Special!@#Chars", "special___chars"), + ("UPPERCASE NAME", "uppercase_name"), + ("lowercase name", "lowercase_name"), + ("Mixed Case Name", "mixed_case_name"), + (" Spaces ", "___spaces___"), + ] + + for name, expected in test_cases: + # For non-empty names, verify our function produces same result as direct snake_case + sanitize + assert get_base_entity_object_id(name, None) == sanitize(snake_case(name)) + assert get_base_entity_object_id(name, None) == expected + + # Empty name is handled specially - it doesn't just use sanitize(snake_case("")) + # Instead it falls back to friendly_name or CORE.name + assert sanitize(snake_case("")) == "" # Direct conversion gives empty string + # But our function returns a fallback + original_name = getattr(CORE, "name", None) + try: + CORE.name = "device" + assert get_base_entity_object_id("", None) == "device" # Uses device name + finally: + if original_name is not None: + CORE.name = original_name + + def test_name_add_mac_suffix_behavior(self) -> None: + """Test behavior related to name_add_mac_suffix. + + In C++, when name_add_mac_suffix is enabled and entity has no name, + get_object_id() returns str_sanitize(str_snake_case(App.get_friendly_name())) + dynamically. Our function always returns the same result since we're + calculating the base for duplicate tracking. + """ + # The function should always return the same result regardless of + # name_add_mac_suffix setting, as we're calculating the base object_id + assert get_base_entity_object_id("", "Test Device") == "test_device" + assert get_base_entity_object_id("Entity Name", "Test Device") == "entity_name" From 2f8e07302b64c81871a5c695e6e533e50f9ceabb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:10:06 +0200 Subject: [PATCH 14/20] Update esphome/core/entity_base.cpp --- esphome/core/entity_base.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/esphome/core/entity_base.cpp b/esphome/core/entity_base.cpp index 7b86130f2f..6afd02ff65 100644 --- a/esphome/core/entity_base.cpp +++ b/esphome/core/entity_base.cpp @@ -40,7 +40,8 @@ std::string EntityBase::get_object_id() const { if (!this->flags_.has_own_name && App.is_name_add_mac_suffix_enabled()) { // `App.get_friendly_name()` is dynamic. return str_sanitize(str_snake_case(App.get_friendly_name())); - } else { // `App.get_friendly_name()` is constant. + } else { + // `App.get_friendly_name()` is constant. if (this->object_id_c_str_ == nullptr) { return ""; } From 8c2b141049d86a55b1d8ff7da151b7910c2f98f3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:41:40 +0200 Subject: [PATCH 15/20] cleanup --- esphome/cpp_helpers.py | 81 +-- esphome/entity.py | 100 ++- .../fixtures/duplicate_entities.yaml | 93 +++ tests/integration/test_duplicate_entities.py | 79 +++ tests/unit_tests/test_duplicate_entities.py | 129 ---- tests/unit_tests/test_entity.py | 590 ++++++++++++++++++ .../test_get_base_entity_object_id.py | 140 ----- 7 files changed, 863 insertions(+), 349 deletions(-) delete mode 100644 tests/unit_tests/test_duplicate_entities.py create mode 100644 tests/unit_tests/test_entity.py delete mode 100644 tests/unit_tests/test_get_base_entity_object_id.py diff --git a/esphome/cpp_helpers.py b/esphome/cpp_helpers.py index a1289485ca..746a006348 100644 --- a/esphome/cpp_helpers.py +++ b/esphome/cpp_helpers.py @@ -1,12 +1,6 @@ import logging from esphome.const import ( - CONF_DEVICE_ID, - CONF_DISABLED_BY_DEFAULT, - CONF_ENTITY_CATEGORY, - CONF_ICON, - CONF_INTERNAL, - CONF_NAME, CONF_SAFE_MODE, CONF_SETUP_PRIORITY, CONF_TYPE_ID, @@ -15,9 +9,11 @@ from esphome.const import ( ) from esphome.core import CORE, ID, coroutine from esphome.coroutine import FakeAwaitable -from esphome.cpp_generator import MockObj, add, get_variable +from esphome.cpp_generator import add, get_variable from esphome.cpp_types import App -from esphome.entity import get_base_entity_object_id +from esphome.entity import ( # noqa: F401 # pylint: disable=unused-import + setup_entity, # Import for backward compatibility +) from esphome.types import ConfigFragmentType, ConfigType from esphome.util import Registry, RegistryEntry @@ -97,75 +93,6 @@ async def register_parented(var, value): add(var.set_parent(paren)) -async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: - """Set up generic properties of an Entity. - - This function handles duplicate entity names by automatically appending - a suffix (_2, _3, etc.) when multiple entities have the same object_id - within the same platform and device combination. - - Args: - var: The entity variable to set up - config: Configuration dictionary containing entity settings - platform: The platform name (e.g., "sensor", "binary_sensor") - """ - # Get device info - device_id: int = 0 - if CONF_DEVICE_ID in config: - device_id_obj: ID = config[CONF_DEVICE_ID] - device: MockObj = await get_variable(device_id_obj) - add(var.set_device(device)) - # Use the device's ID hash as device_id - from esphome.helpers import fnv1a_32bit_hash - - device_id = fnv1a_32bit_hash(device_id_obj.id) - - add(var.set_name(config[CONF_NAME])) - - # Calculate base object_id using the same logic as C++ - # This must match the C++ behavior in esphome/core/entity_base.cpp - base_object_id = get_base_entity_object_id(config[CONF_NAME], CORE.friendly_name) - - if not config[CONF_NAME]: - _LOGGER.debug( - "Entity has empty name, using '%s' as object_id base", base_object_id - ) - - # Handle duplicates - # Check for duplicates - unique_key: tuple[int, str, str] = (device_id, platform, base_object_id) - if unique_key in CORE.unique_ids: - # Found duplicate, add suffix - count = CORE.unique_ids[unique_key] + 1 - CORE.unique_ids[unique_key] = count - object_id = f"{base_object_id}_{count}" - _LOGGER.info( - "Duplicate %s entity '%s' found. Renaming to '%s'", - platform, - config[CONF_NAME], - object_id, - ) - else: - # First occurrence - CORE.unique_ids[unique_key] = 1 - object_id = base_object_id - - add(var.set_object_id(object_id)) - _LOGGER.debug( - "Setting object_id '%s' for entity '%s' on platform '%s'", - object_id, - config[CONF_NAME], - platform, - ) - add(var.set_disabled_by_default(config[CONF_DISABLED_BY_DEFAULT])) - if CONF_INTERNAL in config: - add(var.set_internal(config[CONF_INTERNAL])) - if CONF_ICON in config: - add(var.set_icon(config[CONF_ICON])) - if CONF_ENTITY_CATEGORY in config: - add(var.set_entity_category(config[CONF_ENTITY_CATEGORY])) - - def extract_registry_entry_config( registry: Registry, full_config: ConfigType, diff --git a/esphome/entity.py b/esphome/entity.py index 732822d0ff..fa7f1ab7d9 100644 --- a/esphome/entity.py +++ b/esphome/entity.py @@ -1,10 +1,26 @@ """Entity-related helper functions.""" -from esphome.core import CORE +import logging + +from esphome.const import ( + CONF_DEVICE_ID, + CONF_DISABLED_BY_DEFAULT, + CONF_ENTITY_CATEGORY, + CONF_ICON, + CONF_INTERNAL, + CONF_NAME, +) +from esphome.core import CORE, ID +from esphome.cpp_generator import MockObj, add, get_variable from esphome.helpers import sanitize, snake_case +from esphome.types import ConfigType + +_LOGGER = logging.getLogger(__name__) -def get_base_entity_object_id(name: str, friendly_name: str | None) -> str: +def get_base_entity_object_id( + name: str, friendly_name: str | None, device_name: str | None = None +) -> str: """Calculate the base object ID for an entity that will be set via set_object_id(). This function calculates what object_id_c_str_ should be set to in C++. @@ -21,6 +37,7 @@ def get_base_entity_object_id(name: str, friendly_name: str | None) -> str: Args: name: The entity name (empty string if no name) friendly_name: The friendly name from CORE.friendly_name + device_name: The device name if entity is on a sub-device Returns: The base object ID to use for duplicate checking and to pass to set_object_id() @@ -29,9 +46,12 @@ def get_base_entity_object_id(name: str, friendly_name: str | None) -> str: if name: # Entity has its own name (has_own_name will be true) base_str = name + elif device_name: + # Entity has empty name and is on a sub-device + # C++ EntityBase::set_name() uses device->get_name() when device is set + base_str = device_name elif friendly_name: # Entity has empty name (has_own_name will be false) - # Calculate what the object_id should be # C++ uses App.get_friendly_name() which returns friendly_name or device name base_str = friendly_name else: @@ -39,3 +59,77 @@ def get_base_entity_object_id(name: str, friendly_name: str | None) -> str: base_str = CORE.name return sanitize(snake_case(base_str)) + + +async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: + """Set up generic properties of an Entity. + + This function handles duplicate entity names by automatically appending + a suffix (_2, _3, etc.) when multiple entities have the same object_id + within the same platform and device combination. + + Args: + var: The entity variable to set up + config: Configuration dictionary containing entity settings + platform: The platform name (e.g., "sensor", "binary_sensor") + """ + # Get device info + device_id: int = 0 + device_name: str | None = None + if CONF_DEVICE_ID in config: + device_id_obj: ID = config[CONF_DEVICE_ID] + device: MockObj = await get_variable(device_id_obj) + add(var.set_device(device)) + # Use the device's ID hash as device_id + from esphome.helpers import fnv1a_32bit_hash + + device_id = fnv1a_32bit_hash(device_id_obj.id) + # Get device name for object ID calculation + device_name = device_id_obj.id + + add(var.set_name(config[CONF_NAME])) + + # Calculate base object_id using the same logic as C++ + # This must match the C++ behavior in esphome/core/entity_base.cpp + base_object_id = get_base_entity_object_id( + config[CONF_NAME], CORE.friendly_name, device_name + ) + + if not config[CONF_NAME]: + _LOGGER.debug( + "Entity has empty name, using '%s' as object_id base", base_object_id + ) + + # Handle duplicates + # Check for duplicates + unique_key: tuple[int, str, str] = (device_id, platform, base_object_id) + if unique_key in CORE.unique_ids: + # Found duplicate, add suffix + count = CORE.unique_ids[unique_key] + 1 + CORE.unique_ids[unique_key] = count + object_id = f"{base_object_id}_{count}" + _LOGGER.info( + "Duplicate %s entity '%s' found. Renaming to '%s'", + platform, + config[CONF_NAME], + object_id, + ) + else: + # First occurrence + CORE.unique_ids[unique_key] = 1 + object_id = base_object_id + + add(var.set_object_id(object_id)) + _LOGGER.debug( + "Setting object_id '%s' for entity '%s' on platform '%s'", + object_id, + config[CONF_NAME], + platform, + ) + add(var.set_disabled_by_default(config[CONF_DISABLED_BY_DEFAULT])) + if CONF_INTERNAL in config: + add(var.set_internal(config[CONF_INTERNAL])) + if CONF_ICON in config: + add(var.set_icon(config[CONF_ICON])) + if CONF_ENTITY_CATEGORY in config: + add(var.set_entity_category(config[CONF_ENTITY_CATEGORY])) diff --git a/tests/integration/fixtures/duplicate_entities.yaml b/tests/integration/fixtures/duplicate_entities.yaml index 0f831db90d..17332fe4b2 100644 --- a/tests/integration/fixtures/duplicate_entities.yaml +++ b/tests/integration/fixtures/duplicate_entities.yaml @@ -86,6 +86,22 @@ binary_sensor: device_id: controller_2 lambda: return false; + # Issue #6953: Empty names on binary sensors + - platform: template + name: "" + lambda: return true; + - platform: template + name: "" + lambda: return false; + + - platform: template + name: "" + lambda: return true; + + - platform: template + name: "" + lambda: return false; + # Scenario 6: Test with special characters that need sanitization text_sensor: - platform: template @@ -116,3 +132,80 @@ switch: lambda: return true; turn_on_action: [] turn_off_action: [] + + # Scenario 8: Issue #6953 - Multiple entities with empty names + # Empty names on main device - should use device name with suffixes + - platform: template + name: "" + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "" + lambda: return true; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "" + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + # Scenario 9: Issue #6953 - Empty names on sub-devices + # Empty names on sub-device - should use sub-device name with suffixes + - platform: template + name: "" + device_id: controller_1 + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "" + device_id: controller_1 + lambda: return true; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "" + device_id: controller_1 + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + # Empty names on different sub-device + - platform: template + name: "" + device_id: controller_2 + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "" + device_id: controller_2 + lambda: return true; + turn_on_action: [] + turn_off_action: [] + + # Scenario 10: Issue #6953 - Duplicate "xyz" names + - platform: template + name: "xyz" + lambda: return false; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "xyz" + lambda: return true; + turn_on_action: [] + turn_off_action: [] + + - platform: template + name: "xyz" + lambda: return false; + turn_on_action: [] + turn_off_action: [] diff --git a/tests/integration/test_duplicate_entities.py b/tests/integration/test_duplicate_entities.py index edbcb9799c..ba40e6bd23 100644 --- a/tests/integration/test_duplicate_entities.py +++ b/tests/integration/test_duplicate_entities.py @@ -161,6 +161,85 @@ async def test_duplicate_entities( "Second power switch should be power_switch_2" ) + # Scenario 6: Check empty names on main device (Issue #6953) + empty_binary = [b for b in binary_sensors if b.name == ""] + empty_binary_ids = sorted([b.object_id for b in empty_binary]) + + # Should use device name "duplicate-entities-test" (sanitized, not snake_case) + assert len(empty_binary_ids) >= 4, ( + f"Expected at least 4 empty name binary sensors, got {len(empty_binary_ids)}" + ) + assert "duplicate-entities-test" in empty_binary_ids, ( + "First empty binary sensor should use device name" + ) + assert "duplicate-entities-test_2" in empty_binary_ids, ( + "Second empty binary sensor should be duplicate-entities-test_2" + ) + assert "duplicate-entities-test_3" in empty_binary_ids, ( + "Third empty binary sensor should be duplicate-entities-test_3" + ) + assert "duplicate-entities-test_4" in empty_binary_ids, ( + "Fourth empty binary sensor should be duplicate-entities-test_4" + ) + + # Scenario 7: Check empty names on sub-devices (Issue #6953) + empty_switches = [s for s in switches if s.name == ""] + + # Group by device + c1_empty_switches = [ + s + for s in empty_switches + if getattr(s, "device_id", None) == controller_1.device_id + ] + c2_empty_switches = [ + s + for s in empty_switches + if getattr(s, "device_id", None) == controller_2.device_id + ] + main_empty_switches = [ + s + for s in empty_switches + if getattr(s, "device_id", None) + not in [controller_1.device_id, controller_2.device_id] + ] + + # Controller 1 empty switches should use "controller_1" + c1_empty_ids = sorted([s.object_id for s in c1_empty_switches]) + assert len(c1_empty_ids) >= 3, ( + f"Expected at least 3 empty switches on controller_1, got {len(c1_empty_ids)}" + ) + assert "controller_1" in c1_empty_ids, "First should be controller_1" + assert "controller_1_2" in c1_empty_ids, "Second should be controller_1_2" + assert "controller_1_3" in c1_empty_ids, "Third should be controller_1_3" + + # Controller 2 empty switches + c2_empty_ids = sorted([s.object_id for s in c2_empty_switches]) + assert len(c2_empty_ids) >= 2, ( + f"Expected at least 2 empty switches on controller_2, got {len(c2_empty_ids)}" + ) + assert "controller_2" in c2_empty_ids, "First should be controller_2" + assert "controller_2_2" in c2_empty_ids, "Second should be controller_2_2" + + # Main device empty switches + main_empty_ids = sorted([s.object_id for s in main_empty_switches]) + assert len(main_empty_ids) >= 3, ( + f"Expected at least 3 empty switches on main device, got {len(main_empty_ids)}" + ) + assert "duplicate-entities-test" in main_empty_ids + assert "duplicate-entities-test_2" in main_empty_ids + assert "duplicate-entities-test_3" in main_empty_ids + + # Scenario 8: Check "xyz" duplicates (Issue #6953) + xyz_switches = [s for s in switches if s.name == "xyz"] + xyz_ids = sorted([s.object_id for s in xyz_switches]) + + assert len(xyz_ids) >= 3, ( + f"Expected at least 3 xyz switches, got {len(xyz_ids)}" + ) + assert "xyz" in xyz_ids, "First xyz switch should be xyz" + assert "xyz_2" in xyz_ids, "Second xyz switch should be xyz_2" + assert "xyz_3" in xyz_ids, "Third xyz switch should be xyz_3" + # Verify we can get states for all entities (ensures they're functional) loop = asyncio.get_running_loop() states_future: asyncio.Future[bool] = loop.create_future() diff --git a/tests/unit_tests/test_duplicate_entities.py b/tests/unit_tests/test_duplicate_entities.py deleted file mode 100644 index ab075a02fc..0000000000 --- a/tests/unit_tests/test_duplicate_entities.py +++ /dev/null @@ -1,129 +0,0 @@ -"""Test duplicate entity object ID handling.""" - -import pytest - -from esphome.core import CORE -from esphome.helpers import sanitize, snake_case - - -@pytest.fixture -def setup_test_device() -> None: - """Set up test device configuration.""" - CORE.name = "test-device" - CORE.friendly_name = "Test Device" - - -def test_unique_key_generation() -> None: - """Test that unique keys are generated correctly.""" - # Test with no device - key1: tuple[int, str, str] = (0, "binary_sensor", "temperature") - assert key1 == (0, "binary_sensor", "temperature") - - # Test with device - key2: tuple[int, str, str] = (12345, "sensor", "humidity") - assert key2 == (12345, "sensor", "humidity") - - -def test_duplicate_tracking() -> None: - """Test that duplicates are tracked correctly.""" - # First occurrence - key: tuple[int, str, str] = (0, "sensor", "temperature") - assert key not in CORE.unique_ids - - CORE.unique_ids[key] = 1 - assert CORE.unique_ids[key] == 1 - - # Second occurrence - count: int = CORE.unique_ids[key] + 1 - CORE.unique_ids[key] = count - assert CORE.unique_ids[key] == 2 - - -def test_object_id_sanitization() -> None: - """Test that object IDs are properly sanitized.""" - # Test various inputs - assert sanitize(snake_case("Temperature Sensor")) == "temperature_sensor" - assert sanitize(snake_case("Living Room Light!")) == "living_room_light_" - assert sanitize(snake_case("Test-Device")) == "test-device" - assert sanitize(snake_case("")) == "" - - -def test_suffix_generation() -> None: - """Test that suffixes are generated correctly.""" - base_id: str = "temperature" - - # No suffix for first occurrence - object_id_1: str = base_id - assert object_id_1 == "temperature" - - # Add suffix for duplicates - count: int = 2 - object_id_2: str = f"{base_id}_{count}" - assert object_id_2 == "temperature_2" - - count = 3 - object_id_3: str = f"{base_id}_{count}" - assert object_id_3 == "temperature_3" - - -def test_different_platforms_same_name() -> None: - """Test that same name on different platforms doesn't conflict.""" - # Simulate two entities with same name on different platforms - key1: tuple[int, str, str] = (0, "binary_sensor", "status") - key2: tuple[int, str, str] = (0, "text_sensor", "status") - - # They should be different keys - assert key1 != key2 - - # Track them separately - CORE.unique_ids[key1] = 1 - CORE.unique_ids[key2] = 1 - - # Both should be at count 1 (no conflict) - assert CORE.unique_ids[key1] == 1 - assert CORE.unique_ids[key2] == 1 - - -def test_different_devices_same_name_platform() -> None: - """Test that same name+platform on different devices doesn't conflict.""" - # Simulate two entities with same name and platform but different devices - key1: tuple[int, str, str] = (12345, "sensor", "temperature") - key2: tuple[int, str, str] = (67890, "sensor", "temperature") - - # They should be different keys - assert key1 != key2 - - # Track them separately - CORE.unique_ids[key1] = 1 - CORE.unique_ids[key2] = 1 - - # Both should be at count 1 (no conflict) - assert CORE.unique_ids[key1] == 1 - assert CORE.unique_ids[key2] == 1 - - -def test_empty_name_handling(setup_test_device: None) -> None: - """Test handling of entities with empty names.""" - # When name is empty, it should use the device name - empty_name: str = "" - base_id: str - if not empty_name: - if CORE.friendly_name: - base_id = sanitize(snake_case(CORE.friendly_name)) - else: - base_id = sanitize(snake_case(CORE.name)) - - assert base_id == "test_device" # Uses friendly name - - -def test_reset_clears_unique_ids() -> None: - """Test that CORE.reset() clears the unique_ids tracking.""" - # Add some tracked IDs - CORE.unique_ids[(0, "sensor", "test")] = 2 - CORE.unique_ids[(0, "binary_sensor", "test")] = 3 - - assert len(CORE.unique_ids) == 2 - - # Reset should clear them - CORE.reset() - assert len(CORE.unique_ids) == 0 diff --git a/tests/unit_tests/test_entity.py b/tests/unit_tests/test_entity.py new file mode 100644 index 0000000000..6cdf5369ae --- /dev/null +++ b/tests/unit_tests/test_entity.py @@ -0,0 +1,590 @@ +"""Test get_base_entity_object_id function matches C++ behavior.""" + +from collections.abc import Generator +from typing import Any + +import pytest + +from esphome import entity +from esphome.const import CONF_DEVICE_ID, CONF_DISABLED_BY_DEFAULT, CONF_ICON, CONF_NAME +from esphome.core import CORE, ID +from esphome.cpp_generator import MockObj +from esphome.entity import get_base_entity_object_id, setup_entity +from esphome.helpers import sanitize, snake_case + + +@pytest.fixture(autouse=True) +def restore_core_state() -> Generator[None, None, None]: + """Save and restore CORE state for tests.""" + original_name = CORE.name + original_friendly_name = CORE.friendly_name + yield + CORE.name = original_name + CORE.friendly_name = original_friendly_name + + +def test_with_entity_name() -> None: + """Test when entity has its own name - should use entity name.""" + # Simple name + assert get_base_entity_object_id("Temperature Sensor", None) == "temperature_sensor" + assert ( + get_base_entity_object_id("Temperature Sensor", "Device Name") + == "temperature_sensor" + ) + # Even with device name, entity name takes precedence + assert ( + get_base_entity_object_id("Temperature Sensor", "Device Name", "Sub Device") + == "temperature_sensor" + ) + + # Name with special characters + assert ( + get_base_entity_object_id("Temp!@#$%^&*()Sensor", None) + == "temp__________sensor" + ) + assert get_base_entity_object_id("Temp-Sensor_123", None) == "temp-sensor_123" + + # Already snake_case + assert get_base_entity_object_id("temperature_sensor", None) == "temperature_sensor" + + # Mixed case + assert get_base_entity_object_id("TemperatureSensor", None) == "temperaturesensor" + assert get_base_entity_object_id("TEMPERATURE SENSOR", None) == "temperature_sensor" + + +def test_empty_name_with_device_name() -> None: + """Test when entity has empty name and is on a sub-device - should use device name.""" + # C++ behavior: when has_own_name is false and device is set, uses device->get_name() + assert ( + get_base_entity_object_id("", "Friendly Device", "Sub Device 1") + == "sub_device_1" + ) + assert ( + get_base_entity_object_id("", "Kitchen Controller", "controller_1") + == "controller_1" + ) + assert get_base_entity_object_id("", None, "Test-Device_123") == "test-device_123" + + +def test_empty_name_with_friendly_name() -> None: + """Test when entity has empty name and no device - should use friendly name.""" + # C++ behavior: when has_own_name is false, uses App.get_friendly_name() + assert get_base_entity_object_id("", "Friendly Device") == "friendly_device" + assert get_base_entity_object_id("", "Kitchen Controller") == "kitchen_controller" + assert get_base_entity_object_id("", "Test-Device_123") == "test-device_123" + + # Special characters in friendly name + assert get_base_entity_object_id("", "Device!@#$%") == "device_____" + + +def test_empty_name_no_friendly_name() -> None: + """Test when entity has empty name and no friendly name - should use device name.""" + # Test with CORE.name set + CORE.name = "device-name" + assert get_base_entity_object_id("", None) == "device-name" + + CORE.name = "Test Device" + assert get_base_entity_object_id("", None) == "test_device" + + +def test_edge_cases() -> None: + """Test edge cases.""" + # Only spaces + assert get_base_entity_object_id(" ", None) == "___" + + # Unicode characters (should be replaced) + assert get_base_entity_object_id("Température", None) == "temp_rature" + assert get_base_entity_object_id("测试", None) == "__" + + # Empty string with empty friendly name (empty friendly name is treated as None) + # Falls back to CORE.name + CORE.name = "device" + assert get_base_entity_object_id("", "") == "device" + + # Very long name (should work fine) + long_name = "a" * 100 + " " + "b" * 100 + expected = "a" * 100 + "_" + "b" * 100 + assert get_base_entity_object_id(long_name, None) == expected + + +def test_matches_cpp_helpers() -> None: + """Test that the logic matches using snake_case and sanitize directly.""" + test_cases = [ + ("Temperature Sensor", "temperature_sensor"), + ("Living Room Light", "living_room_light"), + ("Test-Device_123", "test-device_123"), + ("Special!@#Chars", "special___chars"), + ("UPPERCASE NAME", "uppercase_name"), + ("lowercase name", "lowercase_name"), + ("Mixed Case Name", "mixed_case_name"), + (" Spaces ", "___spaces___"), + ] + + for name, expected in test_cases: + # For non-empty names, verify our function produces same result as direct snake_case + sanitize + assert get_base_entity_object_id(name, None) == sanitize(snake_case(name)) + assert get_base_entity_object_id(name, None) == expected + + # Empty name is handled specially - it doesn't just use sanitize(snake_case("")) + # Instead it falls back to friendly_name or CORE.name + assert sanitize(snake_case("")) == "" # Direct conversion gives empty string + # But our function returns a fallback + CORE.name = "device" + assert get_base_entity_object_id("", None) == "device" # Uses device name + + +def test_name_add_mac_suffix_behavior() -> None: + """Test behavior related to name_add_mac_suffix. + + In C++, when name_add_mac_suffix is enabled and entity has no name, + get_object_id() returns str_sanitize(str_snake_case(App.get_friendly_name())) + dynamically. Our function always returns the same result since we're + calculating the base for duplicate tracking. + """ + # The function should always return the same result regardless of + # name_add_mac_suffix setting, as we're calculating the base object_id + assert get_base_entity_object_id("", "Test Device") == "test_device" + assert get_base_entity_object_id("Entity Name", "Test Device") == "entity_name" + + +def test_priority_order() -> None: + """Test the priority order: entity name > device name > friendly name > CORE.name.""" + CORE.name = "core-device" + + # 1. Entity name has highest priority + assert ( + get_base_entity_object_id("Entity Name", "Friendly Name", "Device Name") + == "entity_name" + ) + + # 2. Device name is next priority (when entity name is empty) + assert ( + get_base_entity_object_id("", "Friendly Name", "Device Name") == "device_name" + ) + + # 3. Friendly name is next (when entity and device names are empty) + assert get_base_entity_object_id("", "Friendly Name", None) == "friendly_name" + + # 4. CORE.name is last resort + assert get_base_entity_object_id("", None, None) == "core-device" + + +def test_real_world_examples() -> None: + """Test real-world entity naming scenarios.""" + # Common ESPHome entity names + test_cases = [ + # name, friendly_name, device_name, expected + ("Living Room Light", None, None, "living_room_light"), + ("", "Kitchen Controller", None, "kitchen_controller"), + ( + "", + "ESP32 Device", + "controller_1", + "controller_1", + ), # Device name takes precedence + ("GPIO2 Button", None, None, "gpio2_button"), + ("WiFi Signal", "My Device", None, "wifi_signal"), + ("", None, "esp32_node", "esp32_node"), + ("Front Door Sensor", "Home Assistant", "door_controller", "front_door_sensor"), + ] + + for name, friendly_name, device_name, expected in test_cases: + result = get_base_entity_object_id(name, friendly_name, device_name) + assert result == expected, ( + f"Failed for {name=}, {friendly_name=}, {device_name=}: {result=}, {expected=}" + ) + + +def test_issue_6953_scenarios() -> None: + """Test specific scenarios from issue #6953.""" + # Scenario 1: Multiple empty names on main device with name_add_mac_suffix + # The Python code calculates the base, C++ might append MAC suffix dynamically + CORE.name = "device-name" + CORE.friendly_name = "Friendly Device" + + # All empty names should resolve to same base + assert get_base_entity_object_id("", CORE.friendly_name) == "friendly_device" + assert get_base_entity_object_id("", CORE.friendly_name) == "friendly_device" + assert get_base_entity_object_id("", CORE.friendly_name) == "friendly_device" + + # Scenario 2: Empty names on sub-devices + assert ( + get_base_entity_object_id("", "Main Device", "controller_1") == "controller_1" + ) + assert ( + get_base_entity_object_id("", "Main Device", "controller_2") == "controller_2" + ) + + # Scenario 3: xyz duplicates + assert get_base_entity_object_id("xyz", None) == "xyz" + assert get_base_entity_object_id("xyz", "Device") == "xyz" + + +# Tests for setup_entity function + + +@pytest.fixture +def setup_test_environment() -> Generator[list[str], None, None]: + """Set up test environment for setup_entity tests.""" + # Reset CORE state + CORE.reset() + CORE.name = "test-device" + CORE.friendly_name = "Test Device" + # Store original add function + + original_add = entity.add + # Track what gets added + added_expressions = [] + + def mock_add(expression: Any) -> Any: + added_expressions.append(str(expression)) + return original_add(expression) + + # Patch add function in entity module + entity.add = mock_add + yield added_expressions + # Clean up + entity.add = original_add + CORE.reset() + + +def extract_object_id_from_expressions(expressions: list[str]) -> str | None: + """Extract the object ID that was set from the generated expressions.""" + for expr in expressions: + # Look for set_object_id calls + if ".set_object_id(" in expr: + # Extract the ID from something like: var.set_object_id("temperature_2") + start = expr.find('"') + 1 + end = expr.rfind('"') + if start > 0 and end > start: + return expr[start:end] + return None + + +@pytest.mark.asyncio +async def test_setup_entity_no_duplicates(setup_test_environment: list[str]) -> None: + """Test setup_entity with unique names.""" + + added_expressions = setup_test_environment + + # Create mock entities + var1 = MockObj("sensor1") + var2 = MockObj("sensor2") + + # Set up first entity + config1 = { + CONF_NAME: "Temperature", + CONF_DISABLED_BY_DEFAULT: False, + } + await setup_entity(var1, config1, "sensor") + + # Get object ID from first entity + object_id1 = extract_object_id_from_expressions(added_expressions) + assert object_id1 == "temperature" + + # Clear for next entity + added_expressions.clear() + + # Set up second entity with different name + config2 = { + CONF_NAME: "Humidity", + CONF_DISABLED_BY_DEFAULT: False, + } + await setup_entity(var2, config2, "sensor") + + # Get object ID from second entity + object_id2 = extract_object_id_from_expressions(added_expressions) + assert object_id2 == "humidity" + + +@pytest.mark.asyncio +async def test_setup_entity_with_duplicates(setup_test_environment: list[str]) -> None: + """Test setup_entity with duplicate names.""" + + added_expressions = setup_test_environment + + # Create mock entities + entities = [MockObj(f"sensor{i}") for i in range(4)] + + # Set up entities with same name + config = { + CONF_NAME: "Temperature", + CONF_DISABLED_BY_DEFAULT: False, + } + + object_ids = [] + for var in entities: + added_expressions.clear() + await setup_entity(var, config, "sensor") + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) + + # Check that object IDs were set with proper suffixes + assert object_ids[0] == "temperature" + assert object_ids[1] == "temperature_2" + assert object_ids[2] == "temperature_3" + assert object_ids[3] == "temperature_4" + + +@pytest.mark.asyncio +async def test_setup_entity_different_platforms( + setup_test_environment: list[str], +) -> None: + """Test that same name on different platforms doesn't conflict.""" + + added_expressions = setup_test_environment + + # Create mock entities + sensor = MockObj("sensor1") + binary_sensor = MockObj("binary_sensor1") + text_sensor = MockObj("text_sensor1") + + config = { + CONF_NAME: "Status", + CONF_DISABLED_BY_DEFAULT: False, + } + + # Set up entities on different platforms + platforms = [ + (sensor, "sensor"), + (binary_sensor, "binary_sensor"), + (text_sensor, "text_sensor"), + ] + + object_ids = [] + for var, platform in platforms: + added_expressions.clear() + await setup_entity(var, config, platform) + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) + + # All should get base object ID without suffix + assert all(obj_id == "status" for obj_id in object_ids) + + +@pytest.mark.asyncio +async def test_setup_entity_with_devices(setup_test_environment: list[str]) -> None: + """Test that same name on different devices doesn't conflict.""" + + added_expressions = setup_test_environment + + # Create mock devices + device1_id = ID("device1", type="Device") + device2_id = ID("device2", type="Device") + + device1 = MockObj("device1_obj") + device2 = MockObj("device2_obj") + + # Mock get_variable to return our devices + original_get_variable = entity.get_variable + + async def mock_get_variable(device_id: ID) -> MockObj: + if device_id == device1_id: + return device1 + elif device_id == device2_id: + return device2 + return await original_get_variable(device_id) + + entity.get_variable = mock_get_variable + + try: + # Create sensors with same name on different devices + sensor1 = MockObj("sensor1") + sensor2 = MockObj("sensor2") + + config1 = { + CONF_NAME: "Temperature", + CONF_DEVICE_ID: device1_id, + CONF_DISABLED_BY_DEFAULT: False, + } + + config2 = { + CONF_NAME: "Temperature", + CONF_DEVICE_ID: device2_id, + CONF_DISABLED_BY_DEFAULT: False, + } + + # Get object IDs + object_ids = [] + for var, config in [(sensor1, config1), (sensor2, config2)]: + added_expressions.clear() + await setup_entity(var, config, "sensor") + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) + + # Both should get base object ID without suffix (different devices) + assert object_ids[0] == "temperature" + assert object_ids[1] == "temperature" + + finally: + entity.get_variable = original_get_variable + + +@pytest.mark.asyncio +async def test_setup_entity_empty_name(setup_test_environment: list[str]) -> None: + """Test setup_entity with empty entity name.""" + + added_expressions = setup_test_environment + + var = MockObj("sensor1") + + config = { + CONF_NAME: "", + CONF_DISABLED_BY_DEFAULT: False, + } + + await setup_entity(var, config, "sensor") + + object_id = extract_object_id_from_expressions(added_expressions) + # Should use friendly name + assert object_id == "test_device" + + +@pytest.mark.asyncio +async def test_setup_entity_empty_name_duplicates( + setup_test_environment: list[str], +) -> None: + """Test setup_entity with multiple empty names.""" + + added_expressions = setup_test_environment + + entities = [MockObj(f"sensor{i}") for i in range(3)] + + config = { + CONF_NAME: "", + CONF_DISABLED_BY_DEFAULT: False, + } + + object_ids = [] + for var in entities: + added_expressions.clear() + await setup_entity(var, config, "sensor") + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) + + # Should use device name with suffixes + assert object_ids[0] == "test_device" + assert object_ids[1] == "test_device_2" + assert object_ids[2] == "test_device_3" + + +@pytest.mark.asyncio +async def test_setup_entity_special_characters( + setup_test_environment: list[str], +) -> None: + """Test setup_entity with names containing special characters.""" + + added_expressions = setup_test_environment + + entities = [MockObj(f"sensor{i}") for i in range(3)] + + config = { + CONF_NAME: "Temperature Sensor!", + CONF_DISABLED_BY_DEFAULT: False, + } + + object_ids = [] + for var in entities: + added_expressions.clear() + await setup_entity(var, config, "sensor") + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) + + # Special characters should be sanitized + assert object_ids[0] == "temperature_sensor_" + assert object_ids[1] == "temperature_sensor__2" + assert object_ids[2] == "temperature_sensor__3" + + +@pytest.mark.asyncio +async def test_setup_entity_with_icon(setup_test_environment: list[str]) -> None: + """Test setup_entity sets icon correctly.""" + + added_expressions = setup_test_environment + + var = MockObj("sensor1") + + config = { + CONF_NAME: "Temperature", + CONF_DISABLED_BY_DEFAULT: False, + CONF_ICON: "mdi:thermometer", + } + + await setup_entity(var, config, "sensor") + + # Check icon was set + icon_set = any( + ".set_icon(" in expr and "mdi:thermometer" in expr for expr in added_expressions + ) + assert icon_set + + +@pytest.mark.asyncio +async def test_setup_entity_disabled_by_default( + setup_test_environment: list[str], +) -> None: + """Test setup_entity sets disabled_by_default correctly.""" + + added_expressions = setup_test_environment + + var = MockObj("sensor1") + + config = { + CONF_NAME: "Temperature", + CONF_DISABLED_BY_DEFAULT: True, + } + + await setup_entity(var, config, "sensor") + + # Check disabled_by_default was set + disabled_set = any( + ".set_disabled_by_default(true)" in expr.lower() for expr in added_expressions + ) + assert disabled_set + + +@pytest.mark.asyncio +async def test_setup_entity_mixed_duplicates(setup_test_environment: list[str]) -> None: + """Test complex duplicate scenario with multiple platforms and devices.""" + + added_expressions = setup_test_environment + + # Track results + results = [] + + # 3 sensors named "Status" + for i in range(3): + added_expressions.clear() + var = MockObj(f"sensor_status_{i}") + await setup_entity( + var, {CONF_NAME: "Status", CONF_DISABLED_BY_DEFAULT: False}, "sensor" + ) + object_id = extract_object_id_from_expressions(added_expressions) + results.append(("sensor", object_id)) + + # 2 binary_sensors named "Status" + for i in range(2): + added_expressions.clear() + var = MockObj(f"binary_sensor_status_{i}") + await setup_entity( + var, {CONF_NAME: "Status", CONF_DISABLED_BY_DEFAULT: False}, "binary_sensor" + ) + object_id = extract_object_id_from_expressions(added_expressions) + results.append(("binary_sensor", object_id)) + + # 1 text_sensor named "Status" + added_expressions.clear() + var = MockObj("text_sensor_status") + await setup_entity( + var, {CONF_NAME: "Status", CONF_DISABLED_BY_DEFAULT: False}, "text_sensor" + ) + object_id = extract_object_id_from_expressions(added_expressions) + results.append(("text_sensor", object_id)) + + # Check results - each platform has its own namespace + assert results[0] == ("sensor", "status") # sensor + assert results[1] == ("sensor", "status_2") # sensor + assert results[2] == ("sensor", "status_3") # sensor + assert results[3] == ("binary_sensor", "status") # binary_sensor (new namespace) + assert results[4] == ("binary_sensor", "status_2") # binary_sensor + assert results[5] == ("text_sensor", "status") # text_sensor (new namespace) diff --git a/tests/unit_tests/test_get_base_entity_object_id.py b/tests/unit_tests/test_get_base_entity_object_id.py deleted file mode 100644 index aeea862d78..0000000000 --- a/tests/unit_tests/test_get_base_entity_object_id.py +++ /dev/null @@ -1,140 +0,0 @@ -"""Test get_base_entity_object_id function matches C++ behavior.""" - -from esphome.core import CORE -from esphome.entity import get_base_entity_object_id -from esphome.helpers import sanitize, snake_case - - -class TestGetBaseEntityObjectId: - """Test that get_base_entity_object_id matches C++ EntityBase::get_object_id behavior.""" - - def test_with_entity_name(self) -> None: - """Test when entity has its own name - should use entity name.""" - # Simple name - assert ( - get_base_entity_object_id("Temperature Sensor", None) - == "temperature_sensor" - ) - assert ( - get_base_entity_object_id("Temperature Sensor", "Device Name") - == "temperature_sensor" - ) - - # Name with special characters - assert ( - get_base_entity_object_id("Temp!@#$%^&*()Sensor", None) - == "temp__________sensor" - ) - assert get_base_entity_object_id("Temp-Sensor_123", None) == "temp-sensor_123" - - # Already snake_case - assert ( - get_base_entity_object_id("temperature_sensor", None) - == "temperature_sensor" - ) - - # Mixed case - assert ( - get_base_entity_object_id("TemperatureSensor", None) == "temperaturesensor" - ) - assert ( - get_base_entity_object_id("TEMPERATURE SENSOR", None) - == "temperature_sensor" - ) - - def test_empty_name_with_friendly_name(self) -> None: - """Test when entity has empty name - should use friendly name.""" - # C++ behavior: when has_own_name is false, uses App.get_friendly_name() - assert get_base_entity_object_id("", "Friendly Device") == "friendly_device" - assert ( - get_base_entity_object_id("", "Kitchen Controller") == "kitchen_controller" - ) - assert get_base_entity_object_id("", "Test-Device_123") == "test-device_123" - - # Special characters in friendly name - assert get_base_entity_object_id("", "Device!@#$%") == "device_____" - - def test_empty_name_no_friendly_name(self) -> None: - """Test when entity has empty name and no friendly name - should use device name.""" - # Save original values - original_name = getattr(CORE, "name", None) - - try: - # Test with CORE.name set - CORE.name = "device-name" - assert get_base_entity_object_id("", None) == "device-name" - - CORE.name = "Test Device" - assert get_base_entity_object_id("", None) == "test_device" - - finally: - # Restore original value - if original_name is not None: - CORE.name = original_name - - def test_edge_cases(self) -> None: - """Test edge cases.""" - # Only spaces - assert get_base_entity_object_id(" ", None) == "___" - - # Unicode characters (should be replaced) - assert get_base_entity_object_id("Température", None) == "temp_rature" - assert get_base_entity_object_id("测试", None) == "__" - - # Empty string with empty friendly name (empty friendly name is treated as None) - # Falls back to CORE.name - original_name = getattr(CORE, "name", None) - try: - CORE.name = "device" - assert get_base_entity_object_id("", "") == "device" - finally: - if original_name is not None: - CORE.name = original_name - - # Very long name (should work fine) - long_name = "a" * 100 + " " + "b" * 100 - expected = "a" * 100 + "_" + "b" * 100 - assert get_base_entity_object_id(long_name, None) == expected - - def test_matches_cpp_helpers(self) -> None: - """Test that the logic matches using snake_case and sanitize directly.""" - test_cases = [ - ("Temperature Sensor", "temperature_sensor"), - ("Living Room Light", "living_room_light"), - ("Test-Device_123", "test-device_123"), - ("Special!@#Chars", "special___chars"), - ("UPPERCASE NAME", "uppercase_name"), - ("lowercase name", "lowercase_name"), - ("Mixed Case Name", "mixed_case_name"), - (" Spaces ", "___spaces___"), - ] - - for name, expected in test_cases: - # For non-empty names, verify our function produces same result as direct snake_case + sanitize - assert get_base_entity_object_id(name, None) == sanitize(snake_case(name)) - assert get_base_entity_object_id(name, None) == expected - - # Empty name is handled specially - it doesn't just use sanitize(snake_case("")) - # Instead it falls back to friendly_name or CORE.name - assert sanitize(snake_case("")) == "" # Direct conversion gives empty string - # But our function returns a fallback - original_name = getattr(CORE, "name", None) - try: - CORE.name = "device" - assert get_base_entity_object_id("", None) == "device" # Uses device name - finally: - if original_name is not None: - CORE.name = original_name - - def test_name_add_mac_suffix_behavior(self) -> None: - """Test behavior related to name_add_mac_suffix. - - In C++, when name_add_mac_suffix is enabled and entity has no name, - get_object_id() returns str_sanitize(str_snake_case(App.get_friendly_name())) - dynamically. Our function always returns the same result since we're - calculating the base for duplicate tracking. - """ - # The function should always return the same result regardless of - # name_add_mac_suffix setting, as we're calculating the base object_id - assert get_base_entity_object_id("", "Test Device") == "test_device" - assert get_base_entity_object_id("Entity Name", "Test Device") == "entity_name" From 418e248e5eca848d06f4bad8483d3edaa7a533b6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:51:05 +0200 Subject: [PATCH 16/20] cleanup --- esphome/entity.py | 3 +- tests/unit_tests/test_entity.py | 170 ++++++++++++++++---------------- 2 files changed, 88 insertions(+), 85 deletions(-) diff --git a/esphome/entity.py b/esphome/entity.py index fa7f1ab7d9..3fa2d62b4d 100644 --- a/esphome/entity.py +++ b/esphome/entity.py @@ -12,7 +12,7 @@ from esphome.const import ( ) from esphome.core import CORE, ID from esphome.cpp_generator import MockObj, add, get_variable -from esphome.helpers import sanitize, snake_case +from esphome.helpers import fnv1a_32bit_hash, sanitize, snake_case from esphome.types import ConfigType _LOGGER = logging.getLogger(__name__) @@ -81,7 +81,6 @@ async def setup_entity(var: MockObj, config: ConfigType, platform: str) -> None: device: MockObj = await get_variable(device_id_obj) add(var.set_device(device)) # Use the device's ID hash as device_id - from esphome.helpers import fnv1a_32bit_hash device_id = fnv1a_32bit_hash(device_id_obj.id) # Get device name for object ID calculation diff --git a/tests/unit_tests/test_entity.py b/tests/unit_tests/test_entity.py index 6cdf5369ae..3033b52a65 100644 --- a/tests/unit_tests/test_entity.py +++ b/tests/unit_tests/test_entity.py @@ -1,6 +1,7 @@ """Test get_base_entity_object_id function matches C++ behavior.""" from collections.abc import Generator +import re from typing import Any import pytest @@ -107,9 +108,9 @@ def test_edge_cases() -> None: assert get_base_entity_object_id(long_name, None) == expected -def test_matches_cpp_helpers() -> None: - """Test that the logic matches using snake_case and sanitize directly.""" - test_cases = [ +@pytest.mark.parametrize( + ("name", "expected"), + [ ("Temperature Sensor", "temperature_sensor"), ("Living Room Light", "living_room_light"), ("Test-Device_123", "test-device_123"), @@ -118,13 +119,17 @@ def test_matches_cpp_helpers() -> None: ("lowercase name", "lowercase_name"), ("Mixed Case Name", "mixed_case_name"), (" Spaces ", "___spaces___"), - ] + ], +) +def test_matches_cpp_helpers(name: str, expected: str) -> None: + """Test that the logic matches using snake_case and sanitize directly.""" + # For non-empty names, verify our function produces same result as direct snake_case + sanitize + assert get_base_entity_object_id(name, None) == sanitize(snake_case(name)) + assert get_base_entity_object_id(name, None) == expected - for name, expected in test_cases: - # For non-empty names, verify our function produces same result as direct snake_case + sanitize - assert get_base_entity_object_id(name, None) == sanitize(snake_case(name)) - assert get_base_entity_object_id(name, None) == expected +def test_empty_name_fallback() -> None: + """Test empty name handling which falls back to friendly_name or CORE.name.""" # Empty name is handled specially - it doesn't just use sanitize(snake_case("")) # Instead it falls back to friendly_name or CORE.name assert sanitize(snake_case("")) == "" # Direct conversion gives empty string @@ -169,10 +174,9 @@ def test_priority_order() -> None: assert get_base_entity_object_id("", None, None) == "core-device" -def test_real_world_examples() -> None: - """Test real-world entity naming scenarios.""" - # Common ESPHome entity names - test_cases = [ +@pytest.mark.parametrize( + ("name", "friendly_name", "device_name", "expected"), + [ # name, friendly_name, device_name, expected ("Living Room Light", None, None, "living_room_light"), ("", "Kitchen Controller", None, "kitchen_controller"), @@ -186,13 +190,14 @@ def test_real_world_examples() -> None: ("WiFi Signal", "My Device", None, "wifi_signal"), ("", None, "esp32_node", "esp32_node"), ("Front Door Sensor", "Home Assistant", "door_controller", "front_door_sensor"), - ] - - for name, friendly_name, device_name, expected in test_cases: - result = get_base_entity_object_id(name, friendly_name, device_name) - assert result == expected, ( - f"Failed for {name=}, {friendly_name=}, {device_name=}: {result=}, {expected=}" - ) + ], +) +def test_real_world_examples( + name: str, friendly_name: str | None, device_name: str | None, expected: str +) -> None: + """Test real-world entity naming scenarios.""" + result = get_base_entity_object_id(name, friendly_name, device_name) + assert result == expected def test_issue_6953_scenarios() -> None: @@ -226,15 +231,14 @@ def test_issue_6953_scenarios() -> None: @pytest.fixture def setup_test_environment() -> Generator[list[str], None, None]: """Set up test environment for setup_entity tests.""" - # Reset CORE state - CORE.reset() + # Set CORE state for tests CORE.name = "test-device" CORE.friendly_name = "Test Device" # Store original add function original_add = entity.add # Track what gets added - added_expressions = [] + added_expressions: list[str] = [] def mock_add(expression: Any) -> Any: added_expressions.append(str(expression)) @@ -245,19 +249,16 @@ def setup_test_environment() -> Generator[list[str], None, None]: yield added_expressions # Clean up entity.add = original_add - CORE.reset() def extract_object_id_from_expressions(expressions: list[str]) -> str | None: """Extract the object ID that was set from the generated expressions.""" for expr in expressions: - # Look for set_object_id calls - if ".set_object_id(" in expr: - # Extract the ID from something like: var.set_object_id("temperature_2") - start = expr.find('"') + 1 - end = expr.rfind('"') - if start > 0 and end > start: - return expr[start:end] + # Look for set_object_id calls with regex to handle various formats + # Matches: var.set_object_id("temperature_2") or var.set_object_id('temperature_2') + match = re.search(r'\.set_object_id\(["\'](.*?)["\']\)', expr) + if match: + return match.group(1) return None @@ -312,7 +313,7 @@ async def test_setup_entity_with_duplicates(setup_test_environment: list[str]) - CONF_DISABLED_BY_DEFAULT: False, } - object_ids = [] + object_ids: list[str] = [] for var in entities: added_expressions.clear() await setup_entity(var, config, "sensor") @@ -351,7 +352,7 @@ async def test_setup_entity_different_platforms( (text_sensor, "text_sensor"), ] - object_ids = [] + object_ids: list[str] = [] for var, platform in platforms: added_expressions.clear() await setup_entity(var, config, platform) @@ -362,62 +363,67 @@ async def test_setup_entity_different_platforms( assert all(obj_id == "status" for obj_id in object_ids) -@pytest.mark.asyncio -async def test_setup_entity_with_devices(setup_test_environment: list[str]) -> None: - """Test that same name on different devices doesn't conflict.""" +@pytest.fixture +def mock_get_variable() -> Generator[dict[ID, MockObj], None, None]: + """Mock get_variable to return test devices.""" + devices = {} + original_get_variable = entity.get_variable + async def _mock_get_variable(device_id: ID) -> MockObj: + if device_id in devices: + return devices[device_id] + return await original_get_variable(device_id) + + entity.get_variable = _mock_get_variable + yield devices + # Clean up + entity.get_variable = original_get_variable + + +@pytest.mark.asyncio +async def test_setup_entity_with_devices( + setup_test_environment: list[str], mock_get_variable: dict[ID, MockObj] +) -> None: + """Test that same name on different devices doesn't conflict.""" added_expressions = setup_test_environment # Create mock devices device1_id = ID("device1", type="Device") device2_id = ID("device2", type="Device") - device1 = MockObj("device1_obj") device2 = MockObj("device2_obj") - # Mock get_variable to return our devices - original_get_variable = entity.get_variable + # Register devices with the mock + mock_get_variable[device1_id] = device1 + mock_get_variable[device2_id] = device2 - async def mock_get_variable(device_id: ID) -> MockObj: - if device_id == device1_id: - return device1 - elif device_id == device2_id: - return device2 - return await original_get_variable(device_id) + # Create sensors with same name on different devices + sensor1 = MockObj("sensor1") + sensor2 = MockObj("sensor2") - entity.get_variable = mock_get_variable + config1 = { + CONF_NAME: "Temperature", + CONF_DEVICE_ID: device1_id, + CONF_DISABLED_BY_DEFAULT: False, + } - try: - # Create sensors with same name on different devices - sensor1 = MockObj("sensor1") - sensor2 = MockObj("sensor2") + config2 = { + CONF_NAME: "Temperature", + CONF_DEVICE_ID: device2_id, + CONF_DISABLED_BY_DEFAULT: False, + } - config1 = { - CONF_NAME: "Temperature", - CONF_DEVICE_ID: device1_id, - CONF_DISABLED_BY_DEFAULT: False, - } + # Get object IDs + object_ids: list[str] = [] + for var, config in [(sensor1, config1), (sensor2, config2)]: + added_expressions.clear() + await setup_entity(var, config, "sensor") + object_id = extract_object_id_from_expressions(added_expressions) + object_ids.append(object_id) - config2 = { - CONF_NAME: "Temperature", - CONF_DEVICE_ID: device2_id, - CONF_DISABLED_BY_DEFAULT: False, - } - - # Get object IDs - object_ids = [] - for var, config in [(sensor1, config1), (sensor2, config2)]: - added_expressions.clear() - await setup_entity(var, config, "sensor") - object_id = extract_object_id_from_expressions(added_expressions) - object_ids.append(object_id) - - # Both should get base object ID without suffix (different devices) - assert object_ids[0] == "temperature" - assert object_ids[1] == "temperature" - - finally: - entity.get_variable = original_get_variable + # Both should get base object ID without suffix (different devices) + assert object_ids[0] == "temperature" + assert object_ids[1] == "temperature" @pytest.mark.asyncio @@ -455,7 +461,7 @@ async def test_setup_entity_empty_name_duplicates( CONF_DISABLED_BY_DEFAULT: False, } - object_ids = [] + object_ids: list[str] = [] for var in entities: added_expressions.clear() await setup_entity(var, config, "sensor") @@ -483,7 +489,7 @@ async def test_setup_entity_special_characters( CONF_DISABLED_BY_DEFAULT: False, } - object_ids = [] + object_ids: list[str] = [] for var in entities: added_expressions.clear() await setup_entity(var, config, "sensor") @@ -513,10 +519,9 @@ async def test_setup_entity_with_icon(setup_test_environment: list[str]) -> None await setup_entity(var, config, "sensor") # Check icon was set - icon_set = any( - ".set_icon(" in expr and "mdi:thermometer" in expr for expr in added_expressions + assert any( + 'sensor1.set_icon("mdi:thermometer")' in expr for expr in added_expressions ) - assert icon_set @pytest.mark.asyncio @@ -537,10 +542,9 @@ async def test_setup_entity_disabled_by_default( await setup_entity(var, config, "sensor") # Check disabled_by_default was set - disabled_set = any( - ".set_disabled_by_default(true)" in expr.lower() for expr in added_expressions + assert any( + "sensor1.set_disabled_by_default(true)" in expr for expr in added_expressions ) - assert disabled_set @pytest.mark.asyncio @@ -550,7 +554,7 @@ async def test_setup_entity_mixed_duplicates(setup_test_environment: list[str]) added_expressions = setup_test_environment # Track results - results = [] + results: list[tuple[str, str]] = [] # 3 sensors named "Status" for i in range(3): From d89ee2df423c0132fc13a0214dc0addc8fff7bb6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:52:13 +0200 Subject: [PATCH 17/20] Update esphome/core/application.h --- esphome/core/application.h | 1 - 1 file changed, 1 deletion(-) diff --git a/esphome/core/application.h b/esphome/core/application.h index 160a7b35ca..17270ca459 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -109,7 +109,6 @@ class Application { this->name_ = name; this->friendly_name_ = friendly_name; } - // area is now handled through the areas system this->comment_ = comment; this->compilation_time_ = compilation_time; } From ac0b0b652ead8f911a077c4f3620f7853d90fb3c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 17:55:58 +0200 Subject: [PATCH 18/20] cleanup --- tests/integration/test_duplicate_entities.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_duplicate_entities.py b/tests/integration/test_duplicate_entities.py index ba40e6bd23..9b30d2db5a 100644 --- a/tests/integration/test_duplicate_entities.py +++ b/tests/integration/test_duplicate_entities.py @@ -37,8 +37,7 @@ async def test_duplicate_entities( entities = await client.list_entities_services() all_entities: list[EntityInfo] = [] for entity_list in entities[0]: - if hasattr(entity_list, "object_id"): - all_entities.append(entity_list) + all_entities.append(entity_list) # Group entities by type for easier testing sensors = [e for e in all_entities if e.__class__.__name__ == "SensorInfo"] @@ -242,7 +241,7 @@ async def test_duplicate_entities( # Verify we can get states for all entities (ensures they're functional) loop = asyncio.get_running_loop() - states_future: asyncio.Future[bool] = loop.create_future() + states_future: asyncio.Future[None] = loop.create_future() state_count = 0 expected_count = ( len(sensors) + len(binary_sensors) + len(text_sensors) + len(switches) @@ -252,7 +251,7 @@ async def test_duplicate_entities( nonlocal state_count state_count += 1 if state_count >= expected_count and not states_future.done(): - states_future.set_result(True) + states_future.set_result(None) client.subscribe_states(on_state) From 66201be5ca9febebf1d31238fca7bd28ed1e175e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 18:00:10 +0200 Subject: [PATCH 19/20] preen --- tests/unit_tests/core/test_config.py | 11 ++--------- tests/unit_tests/test_entity.py | 5 ++++- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/unit_tests/core/test_config.py b/tests/unit_tests/core/test_config.py index 372c1df7ee..55cc1f3027 100644 --- a/tests/unit_tests/core/test_config.py +++ b/tests/unit_tests/core/test_config.py @@ -28,13 +28,6 @@ def yaml_file(tmp_path: Path) -> Callable[[str], str]: return _yaml_file -@pytest.fixture(autouse=True) -def reset_core(): - """Reset CORE after each test.""" - yield - CORE.reset() - - def load_config_from_yaml( yaml_file: Callable[[str], str], yaml_content: str ) -> Config | None: @@ -61,7 +54,7 @@ def load_config_from_fixture( def test_validate_area_config_with_string() -> None: """Test that string area config is converted to structured format.""" - result: dict[str, Any] = validate_area_config("Living Room") + result = validate_area_config("Living Room") assert isinstance(result, dict) assert "id" in result @@ -80,7 +73,7 @@ def test_validate_area_config_with_dict() -> None: "name": "Test Area", } - result: dict[str, Any] = validate_area_config(input_config) + result = validate_area_config(input_config) assert result == input_config assert result["id"] == area_id diff --git a/tests/unit_tests/test_entity.py b/tests/unit_tests/test_entity.py index 3033b52a65..1b0c648be4 100644 --- a/tests/unit_tests/test_entity.py +++ b/tests/unit_tests/test_entity.py @@ -13,6 +13,9 @@ from esphome.cpp_generator import MockObj from esphome.entity import get_base_entity_object_id, setup_entity from esphome.helpers import sanitize, snake_case +# Pre-compiled regex pattern for extracting object IDs from expressions +OBJECT_ID_PATTERN = re.compile(r'\.set_object_id\(["\'](.*?)["\']\)') + @pytest.fixture(autouse=True) def restore_core_state() -> Generator[None, None, None]: @@ -256,7 +259,7 @@ def extract_object_id_from_expressions(expressions: list[str]) -> str | None: for expr in expressions: # Look for set_object_id calls with regex to handle various formats # Matches: var.set_object_id("temperature_2") or var.set_object_id('temperature_2') - match = re.search(r'\.set_object_id\(["\'](.*?)["\']\)', expr) + match = OBJECT_ID_PATTERN.search(expr) if match: return match.group(1) return None From ac3598f12af5468bd07dc178767f0393bc8c3a51 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Jun 2025 18:07:58 +0200 Subject: [PATCH 20/20] cleanup --- tests/unit_tests/core/test_config.py | 1 - tests/unit_tests/test_entity.py | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit_tests/core/test_config.py b/tests/unit_tests/core/test_config.py index 55cc1f3027..ba8436b7a7 100644 --- a/tests/unit_tests/core/test_config.py +++ b/tests/unit_tests/core/test_config.py @@ -198,7 +198,6 @@ def test_device_with_invalid_area_id( # Check for the specific error message in stdout captured = capsys.readouterr() - print(captured.out) assert ( "Couldn't find ID 'nonexistent_area'. Please check you have defined an ID with that name in your configuration." in captured.out diff --git a/tests/unit_tests/test_entity.py b/tests/unit_tests/test_entity.py index 1b0c648be4..62ce7406ff 100644 --- a/tests/unit_tests/test_entity.py +++ b/tests/unit_tests/test_entity.py @@ -259,8 +259,7 @@ def extract_object_id_from_expressions(expressions: list[str]) -> str | None: for expr in expressions: # Look for set_object_id calls with regex to handle various formats # Matches: var.set_object_id("temperature_2") or var.set_object_id('temperature_2') - match = OBJECT_ID_PATTERN.search(expr) - if match: + if match := OBJECT_ID_PATTERN.search(expr): return match.group(1) return None