fix(ble): Fix connection issues in Bluedroid and GAP naming in NimBLE#12318
fix(ble): Fix connection issues in Bluedroid and GAP naming in NimBLE#12318lucasssvaz wants to merge 3 commits intoespressif:masterfrom
Conversation
👋 Hello lucasssvaz, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
lucasssvaz
changed the title
Test Results 90 files 90 suites 30m 47s ⏱️ Results for commit 3e8b9b4. ♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR enhances BLE connection reliability and robustness by adding retry logic for transient stack errors, improving async event synchronization in advertising, fixing NimBLE device name persistence, and adding enhanced logging throughout the connection flow.
Changes:
- Added retry logic to BLEClient::connect() to handle transient ESP_ERR_INVALID_STATE errors with configurable retries and delays, plus additional validation and scan stopping before connection attempts
- Improved semaphore-based synchronization in BLEAdvertising::start() and event handling to ensure proper async operation completion for advertising and scan response data configuration
- Fixed NimBLE server device name by re-setting it after ble_gatts_start() to prevent reversion to SDK default
- Made isScanning() method available for both Bluedroid and NimBLE stacks by moving it from NimBLE-specific to common API
- Added comprehensive debug and info-level logging for connection lifecycle events
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/BLE/src/BLEClient.cpp | Added connection retry logic, address/state validation, automatic scan stopping, enhanced error handling and logging |
| libraries/BLE/src/BLEAdvertising.cpp | Improved async semaphore handling for advertising/scan response data configuration with explicit take/wait/give patterns and debug logging |
| libraries/BLE/src/BLEServer.cpp | Re-sets device name after starting NimBLE GATT server and adds logging for connection/registration events |
| libraries/BLE/src/BLEScan.h | Moved isScanning() method declaration from NimBLE-only section to common API |
| libraries/BLE/src/BLEScan.cpp | Added isScanning() implementation for Bluedroid stack using m_stopped state variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // Other errors, don't retry | ||
| log_e("esp_ble_gattc_open: rc=%d %s", errRc, GeneralUtils::errorToString(errRc)); | ||
| BLEDevice::removePeerDevice(m_appId, true); | ||
| esp_ble_gattc_app_unregister(m_gattc_if); | ||
| m_gattc_if = ESP_GATT_IF_NONE; | ||
| return false; |
There was a problem hiding this comment.
The semaphore m_semaphoreOpenEvt is taken at line 485 but is not released before returning false when a non-retryable error occurs. This will leave the semaphore in a taken state and could cause deadlocks or hangs in subsequent connection attempts. The semaphore should be given back with an appropriate error value before cleanup and return.
| // Check if we exhausted retries | ||
| if (errRc != ESP_OK) { | ||
| log_e("esp_ble_gattc_open: rc=%d %s", errRc, GeneralUtils::errorToString(errRc)); | ||
| log_e("esp_ble_gattc_open failed after %d retries: rc=%d %s", maxRetries, errRc, GeneralUtils::errorToString(errRc)); | ||
| BLEDevice::removePeerDevice(m_appId, true); | ||
| esp_ble_gattc_app_unregister(m_gattc_if); | ||
| m_gattc_if = ESP_GATT_IF_NONE; | ||
| return false; |
There was a problem hiding this comment.
The semaphore m_semaphoreOpenEvt is taken at line 485 in the retry loop but is not released before returning false when retries are exhausted. This will leave the semaphore in a taken state and could cause deadlocks or hangs in subsequent connection attempts. The semaphore should be given back with an appropriate error value before cleanup and return.
1 participant
Description of Change
This pull request introduces several improvements to the BLE library, focusing on connection reliability, async event handling, and logging. The most significant updates include adding retry logic to BLE client connections, improving semaphore and async handling for advertising, and enhancing logging for easier debugging. Additionally, the device name is now properly set for NimBLE servers after initialization, and a method to check scan status has been added.
Connection Reliability and Robustness:
BLEClient::connect()to handle transient BLE stack errors (e.g., busy state), with configurable retries and delays. The method now also validates addresses, checks for existing connections, stops active scans before connecting, and improves error handling and cleanup. [1] [2] [3]Advertising and Async Handling:
BLEAdvertising::start()by explicitly taking and waiting on semaphores during advertising and scan response data configuration, ensuring proper synchronization with BLE stack events. Added debug logs for configuration steps.BLEAdvertising::handleGAPEvent(), restored and enhanced semaphore signaling and added debug logging for advertising and scan response data completion events.NimBLE Server Initialization:
Logging Improvements:
API Enhancements:
BLEScan::isScanning()method (and declaration in the header) to allow checking if scanning is active. [1] [2] [3]Test Scenarios
Tested locally with ESP32 and ESP32-C6