diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index d1ee7af4ea..e22d43c0cc 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -306,7 +306,7 @@ void ESP32BLE::loop() { case BLEEvent::GATTS: { esp_gatts_cb_event_t event = ble_event->event_.gatts.gatts_event; esp_gatt_if_t gatts_if = ble_event->event_.gatts.gatts_if; - esp_ble_gatts_cb_param_t *param = ble_event->event_.gatts.gatts_param; + esp_ble_gatts_cb_param_t *param = &ble_event->event_.gatts.gatts_param; ESP_LOGV(TAG, "gatts_event [esp_gatt_if: %d] - %d", gatts_if, event); for (auto *gatts_handler : this->gatts_event_handlers_) { gatts_handler->gatts_event_handler(event, gatts_if, param); @@ -316,7 +316,7 @@ void ESP32BLE::loop() { case BLEEvent::GATTC: { esp_gattc_cb_event_t event = ble_event->event_.gattc.gattc_event; esp_gatt_if_t gattc_if = ble_event->event_.gattc.gattc_if; - esp_ble_gattc_cb_param_t *param = ble_event->event_.gattc.gattc_param; + esp_ble_gattc_cb_param_t *param = &ble_event->event_.gattc.gattc_param; ESP_LOGV(TAG, "gattc_event [esp_gatt_if: %d] - %d", gattc_if, event); for (auto *gattc_handler : this->gattc_event_handlers_) { gattc_handler->gattc_event_handler(event, gattc_if, param); diff --git a/esphome/components/esp32_ble/ble_event.h b/esphome/components/esp32_ble/ble_event.h index 2c0ab1d34e..299fd7705f 100644 --- a/esphome/components/esp32_ble/ble_event.h +++ b/esphome/components/esp32_ble/ble_event.h @@ -61,10 +61,24 @@ static_assert(offsetof(esp_ble_gap_cb_param_t, read_rssi_cmpl.rssi) == sizeof(es 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"); +// Param struct sizes on ESP32 +static constexpr size_t GATTC_PARAM_SIZE = 28; +static constexpr size_t GATTS_PARAM_SIZE = 32; + +// Maximum size for inline storage of data +// GATTC: 80 - 28 (param) - 8 (other fields) = 44 bytes for data +// GATTS: 80 - 32 (param) - 8 (other fields) = 40 bytes for data +static constexpr size_t GATTC_INLINE_DATA_SIZE = 44; +static constexpr size_t GATTS_INLINE_DATA_SIZE = 40; + +// Verify param struct sizes +static_assert(sizeof(esp_ble_gattc_cb_param_t) == GATTC_PARAM_SIZE, "GATTC param size unexpected"); +static_assert(sizeof(esp_ble_gatts_cb_param_t) == GATTS_PARAM_SIZE, "GATTS param size unexpected"); + // 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 heap allocation overhead. -// GATTC/GATTS events use heap allocation for their param and data. +// GATTC/GATTS events use heap allocation for their param and inline storage for small data. // // Event flow: // 1. ESP-IDF BLE stack calls our static handlers in the BLE task context @@ -111,21 +125,21 @@ class BLEEvent { this->init_gap_data_(e, p); } - // Constructor for GATTC events - uses heap allocation - // 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. + // Constructor for GATTC events - param stored inline, data may use heap + // IMPORTANT: We MUST copy the param struct because the pointer from ESP-IDF + // is only valid during the callback execution. Since BLE events are processed + // asynchronously in the main loop, we store our own copy inline 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->init_gattc_data_(e, i, p); } - // Constructor for GATTS events - uses heap allocation - // 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. + // Constructor for GATTS events - param stored inline, data may use heap + // IMPORTANT: We MUST copy the param struct because the pointer from ESP-IDF + // is only valid during the callback execution. Since BLE events are processed + // asynchronously in the main loop, we store our own copy inline 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->init_gatts_data_(e, i, p); @@ -135,27 +149,32 @@ class BLEEvent { ~BLEEvent() { this->release(); } // Default constructor for pre-allocation in pool - BLEEvent() : type_(GAP) {} + BLEEvent() : event_{}, type_(GAP) {} // Invoked on return to EventPool - clean up any heap-allocated data void release() { - 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; - this->event_.gattc.data_len = 0; - 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; - this->event_.gatts.data_len = 0; + switch (this->type_) { + case GAP: + // GAP events don't have heap allocations + break; + case GATTC: + // Param is now stored inline, only delete heap data if it was heap-allocated + if (!this->event_.gattc.is_inline && this->event_.gattc.data.heap_data != nullptr) { + delete[] this->event_.gattc.data.heap_data; + } + // Clear critical fields to prevent issues if type changes + this->event_.gattc.is_inline = false; + this->event_.gattc.data.heap_data = nullptr; + break; + case GATTS: + // Param is now stored inline, only delete heap data if it was heap-allocated + if (!this->event_.gatts.is_inline && this->event_.gatts.data.heap_data != nullptr) { + delete[] this->event_.gatts.data.heap_data; + } + // Clear critical fields to prevent issues if type changes + this->event_.gatts.is_inline = false; + this->event_.gatts.data.heap_data = nullptr; + break; } } @@ -207,22 +226,30 @@ class BLEEvent { // NOLINTNEXTLINE(readability-identifier-naming) struct gattc_event { - esp_gattc_cb_event_t gattc_event; - esp_gatt_if_t gattc_if; - esp_ble_gattc_cb_param_t *gattc_param; // Heap-allocated - uint8_t *data; // Heap-allocated raw buffer (manually managed) - uint16_t data_len; // Track size separately - } gattc; + esp_ble_gattc_cb_param_t gattc_param; // Stored inline (28 bytes) + esp_gattc_cb_event_t gattc_event; // 4 bytes + union { + uint8_t *heap_data; // 4 bytes when heap-allocated + uint8_t inline_data[GATTC_INLINE_DATA_SIZE]; // 44 bytes when stored inline + } data; // 44 bytes total + uint16_t data_len; // 2 bytes + esp_gatt_if_t gattc_if; // 1 byte + bool is_inline; // 1 byte - true when data is stored inline + } gattc; // Total: 80 bytes // NOLINTNEXTLINE(readability-identifier-naming) struct gatts_event { - esp_gatts_cb_event_t gatts_event; - esp_gatt_if_t gatts_if; - esp_ble_gatts_cb_param_t *gatts_param; // Heap-allocated - uint8_t *data; // Heap-allocated raw buffer (manually managed) - uint16_t data_len; // Track size separately - } gatts; - } event_; // 80 bytes + esp_ble_gatts_cb_param_t gatts_param; // Stored inline (32 bytes) + esp_gatts_cb_event_t gatts_event; // 4 bytes + union { + uint8_t *heap_data; // 4 bytes when heap-allocated + uint8_t inline_data[GATTS_INLINE_DATA_SIZE]; // 40 bytes when stored inline + } data; // 40 bytes total + uint16_t data_len; // 2 bytes + esp_gatt_if_t gatts_if; // 1 byte + bool is_inline; // 1 byte - true when data is stored inline + } gatts; // Total: 80 bytes + } event_; // 80 bytes ble_event_t type_; @@ -236,6 +263,29 @@ class BLEEvent { const esp_ble_sec_t &security() const { return event_.gap.security; } private: + // Helper to copy data with inline storage optimization + template + void copy_data_with_inline_storage_(EventStruct &event, const uint8_t *src_data, uint16_t len, + uint8_t **param_value_ptr) { + event.data_len = len; + if (len > 0) { + if (len <= InlineSize) { + event.is_inline = true; + memcpy(event.data.inline_data, src_data, len); + *param_value_ptr = event.data.inline_data; + } else { + event.is_inline = false; + event.data.heap_data = new uint8_t[len]; + memcpy(event.data.heap_data, src_data, len); + *param_value_ptr = event.data.heap_data; + } + } else { + event.is_inline = false; + event.data.heap_data = nullptr; + *param_value_ptr = nullptr; + } + } + // 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; @@ -320,48 +370,37 @@ class BLEEvent { this->event_.gattc.gattc_if = i; if (p == nullptr) { - this->event_.gattc.gattc_param = nullptr; - this->event_.gattc.data = nullptr; + // Zero out the param struct when null + memset(&this->event_.gattc.gattc_param, 0, sizeof(this->event_.gattc.gattc_param)); + this->event_.gattc.is_inline = false; + this->event_.gattc.data.heap_data = nullptr; this->event_.gattc.data_len = 0; 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 param struct inline (no heap allocation!) + // GATTC/GATTS events are rare (<1% of events) but we can still store them inline + // along with small data payloads, eliminating all heap allocations for typical BLE operations + // CRITICAL: This copy is REQUIRED for memory safety - the ESP-IDF param pointer + // is only valid during the callback and will be reused/freed after we return + this->event_.gattc.gattc_param = *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_len = p->notify.value_len; - if (p->notify.value_len > 0) { - this->event_.gattc.data = new uint8_t[p->notify.value_len]; - memcpy(this->event_.gattc.data, p->notify.value, p->notify.value_len); - } else { - this->event_.gattc.data = nullptr; - } - this->event_.gattc.gattc_param->notify.value = this->event_.gattc.data; + copy_data_with_inline_storage_event_.gattc), GATTC_INLINE_DATA_SIZE>( + this->event_.gattc, p->notify.value, p->notify.value_len, &this->event_.gattc.gattc_param.notify.value); break; case ESP_GATTC_READ_CHAR_EVT: case ESP_GATTC_READ_DESCR_EVT: - this->event_.gattc.data_len = p->read.value_len; - if (p->read.value_len > 0) { - this->event_.gattc.data = new uint8_t[p->read.value_len]; - memcpy(this->event_.gattc.data, p->read.value, p->read.value_len); - } else { - this->event_.gattc.data = nullptr; - } - this->event_.gattc.gattc_param->read.value = this->event_.gattc.data; + copy_data_with_inline_storage_event_.gattc), GATTC_INLINE_DATA_SIZE>( + this->event_.gattc, p->read.value, p->read.value_len, &this->event_.gattc.gattc_param.read.value); break; default: - this->event_.gattc.data = nullptr; + this->event_.gattc.is_inline = false; + this->event_.gattc.data.heap_data = nullptr; this->event_.gattc.data_len = 0; break; } @@ -373,37 +412,32 @@ class BLEEvent { this->event_.gatts.gatts_if = i; if (p == nullptr) { - this->event_.gatts.gatts_param = nullptr; - this->event_.gatts.data = nullptr; + // Zero out the param struct when null + memset(&this->event_.gatts.gatts_param, 0, sizeof(this->event_.gatts.gatts_param)); + this->event_.gatts.is_inline = false; + this->event_.gatts.data.heap_data = nullptr; this->event_.gatts.data_len = 0; 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 param struct inline (no heap allocation!) + // GATTC/GATTS events are rare (<1% of events) but we can still store them inline + // along with small data payloads, eliminating all heap allocations for typical BLE operations + // CRITICAL: This copy is REQUIRED for memory safety - the ESP-IDF param pointer + // is only valid during the callback and will be reused/freed after we return + this->event_.gatts.gatts_param = *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_len = p->write.len; - if (p->write.len > 0) { - this->event_.gatts.data = new uint8_t[p->write.len]; - memcpy(this->event_.gatts.data, p->write.value, p->write.len); - } else { - this->event_.gatts.data = nullptr; - } - this->event_.gatts.gatts_param->write.value = this->event_.gatts.data; + copy_data_with_inline_storage_event_.gatts), GATTS_INLINE_DATA_SIZE>( + this->event_.gatts, p->write.value, p->write.len, &this->event_.gatts.gatts_param.write.value); break; default: - this->event_.gatts.data = nullptr; + this->event_.gatts.is_inline = false; + this->event_.gatts.data.heap_data = nullptr; this->event_.gatts.data_len = 0; break; } @@ -414,6 +448,15 @@ class BLEEvent { // 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 GATTC and GATTS structs don't exceed GAP struct size +// This ensures the union size is determined by GAP (the most common event type) +static_assert(sizeof(decltype(((BLEEvent *) nullptr)->event_.gattc)) <= + sizeof(decltype(((BLEEvent *) nullptr)->event_.gap)), + "gattc_event struct exceeds gap_event size - union size would increase"); +static_assert(sizeof(decltype(((BLEEvent *) nullptr)->event_.gatts)) <= + sizeof(decltype(((BLEEvent *) nullptr)->event_.gap)), + "gatts_event struct exceeds gap_event size - union size would increase"); + // 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");