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