fix(ble): Fix connection issues in Bluedroid and GAP naming in NimBLE (#12318)

* fix(ble): Fix GAP name persistance with NimBLE

* fix(ble): Await for Adv data in Bluedroid and match nimble

* Fix possible deadlocks

libraries/BLE/src/BLEAdvertising.cpp

@@ -702,18 +702,24 @@ bool BLEAdvertising::start() {
 
   if (!m_customAdvData) {
     // Set the configuration for advertising.
+    // This is an async operation - we must wait for ESP_GAP_BLE_ADV_DATA_SET_COMPLETE_EVT
     m_advData.set_scan_rsp = false;
     m_advData.include_name = !m_scanResp;
     m_advData.include_txpower = !m_scanResp;
+    m_semaphoreSetAdv.take("config_adv_data");
     errRc = ::esp_ble_gap_config_adv_data(&m_advData);
     if (errRc != ESP_OK) {
       log_e("<< esp_ble_gap_config_adv_data: rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
+      m_semaphoreSetAdv.give();
       return false;
     }
+    m_semaphoreSetAdv.wait("config_adv_data");
+    log_d("Advertising data configured");
   }
 
   if (!m_customScanResponseData && m_scanResp) {
     // Set the configuration for scan response.
+    // This is an async operation - we must wait for ESP_GAP_BLE_SCAN_RSP_DATA_SET_COMPLETE_EVT
     memcpy(&m_scanRespData, &m_advData, sizeof(esp_ble_adv_data_t));  // Copy the content of m_advData.
     m_scanRespData.set_scan_rsp = true;                               // Define this struct as scan response data
     m_scanRespData.include_name = true;                               // Caution: This may lead to a crash if the device name has more than 29 characters
@@ -721,11 +727,15 @@ bool BLEAdvertising::start() {
     m_scanRespData.appearance = 0;  // If defined the 'Appearance' attribute is already included in the advertising data
     m_scanRespData.flag = 0;        // 'Flags' attribute should no be included in the scan response
 
+    m_semaphoreSetAdv.take("config_scan_rsp");
     errRc = ::esp_ble_gap_config_adv_data(&m_scanRespData);
     if (errRc != ESP_OK) {
       log_e("<< esp_ble_gap_config_adv_data (Scan response): rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
+      m_semaphoreSetAdv.give();
       return false;
     }
+    m_semaphoreSetAdv.wait("config_scan_rsp");
+    log_d("Scan response data configured");
   }
 
   // If we had services to advertise then we previously allocated some storage for them.
@@ -765,17 +775,19 @@ void BLEAdvertising::handleGAPEvent(esp_gap_ble_cb_event_t event, esp_ble_gap_cb
   switch (event) {
     case ESP_GAP_BLE_ADV_DATA_SET_COMPLETE_EVT:
     {
-      // m_semaphoreSetAdv.give();
+      log_d("Advertising data set complete, status=%d", param->adv_data_cmpl.status);
+      m_semaphoreSetAdv.give();
       break;
     }
     case ESP_GAP_BLE_SCAN_RSP_DATA_SET_COMPLETE_EVT:
     {
-      // m_semaphoreSetAdv.give();
+      log_d("Scan response data set complete, status=%d", param->scan_rsp_data_cmpl.status);
+      m_semaphoreSetAdv.give();
       break;
     }
     case ESP_GAP_BLE_ADV_START_COMPLETE_EVT:
     {
-      // m_semaphoreSetAdv.give();
+      log_d("Advertising start complete, status=%d", param->adv_start_cmpl.status);
       break;
     }
     case ESP_GAP_BLE_ADV_STOP_COMPLETE_EVT:

libraries/BLE/src/BLEClient.cpp

@@ -420,7 +420,34 @@ void BLEClientCallbacks::onDisconnect(BLEClient *pClient) {
  * @return True on success.
  */
 bool BLEClient::connect(BLEAddress address, uint8_t type, uint32_t timeoutMs) {
-  log_v(">> connect(%s)", address.toString().c_str());
+  log_i(">> connect(%s)", address.toString().c_str());
+
+  // Configuration for retry logic
+  const int maxRetries = 3;
+  const int retryDelayMs = 100;
+  int retryCount = 0;
+  esp_err_t errRc;
+  uint32_t rc;
+
+  // Validate address
+  if (address == BLEAddress("")) {
+    log_e("Invalid peer address (NULL)");
+    return false;
+  }
+
+  // Check if already connected
+  if (m_isConnected) {
+    log_e("Client already connected to %s", m_peerAddress.toString().c_str());
+    return false;
+  }
+
+  // Stop any active scan before connecting - scanning can interfere with connection establishment
+  BLEScan *pScan = BLEDevice::getScan();
+  if (pScan != nullptr && pScan->isScanning()) {
+    log_i("Stopping scan before connecting...");
+    pScan->stop();
+    delay(50);  // Give the BLE stack time to settle after stopping scan
+  }
 
   // We need the connection handle that we get from registering the application.  We register the app
   // and then block on its completion.  When the event has arrived, we will have the handle.
@@ -429,14 +456,15 @@ bool BLEClient::connect(BLEAddress address, uint8_t type, uint32_t timeoutMs) {
   m_semaphoreRegEvt.take("connect");
 
   // clearServices(); // we dont need to delete services since every client is unique?
-  esp_err_t errRc = ::esp_ble_gattc_app_register(m_appId);
+  log_d("Registering GATT client app (appId=%d)...", m_appId);
+  errRc = ::esp_ble_gattc_app_register(m_appId);
   if (errRc != ESP_OK) {
     log_e("esp_ble_gattc_app_register: rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
     BLEDevice::removePeerDevice(m_appId, true);
     return false;
   }
 
-  uint32_t rc = m_semaphoreRegEvt.wait("connect");
+  rc = m_semaphoreRegEvt.wait("connect");
 
   if (rc != ESP_GATT_OK) {
     // fixes ESP_GATT_NO_RESOURCES error mostly
@@ -448,32 +476,79 @@ bool BLEClient::connect(BLEAddress address, uint8_t type, uint32_t timeoutMs) {
     return false;
   }
 
+  log_d("GATT client registered (gattc_if=%d)", m_gattc_if);
   m_peerAddress = address;
 
-  // Perform the open connection request against the target BLE Server.
-  m_semaphoreOpenEvt.take("connect");
-  errRc = ::esp_ble_gattc_open(
-    m_gattc_if,
-    getPeerAddress().getNative(),  // address
-    (esp_ble_addr_type_t)type,     // Note: This was added on 2018-04-03 when the latest ESP-IDF was detected to have changed the signature.
-    1                              // direct connection <-- maybe needs to be changed in case of direct indirect connection???
-  );
-  if (errRc != ESP_OK) {
-    log_e("esp_ble_gattc_open: rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
-    BLEDevice::removePeerDevice(m_appId, true);
-    return false;
-  }
+  // Perform the open connection request against the target BLE Server with retry logic.
+  // This helps handle cases where the BLE stack is busy or needs time to settle.
+  do {
+    m_semaphoreOpenEvt.take("connect");
+    log_d("Opening connection to %s (attempt %d/%d)...", address.toString().c_str(), retryCount + 1, maxRetries + 1);
+    errRc = ::esp_ble_gattc_open(
+      m_gattc_if,
+      getPeerAddress().getNative(),  // address
+      (esp_ble_addr_type_t)type,     // Note: This was added on 2018-04-03 when the latest ESP-IDF was detected to have changed the signature.
+      1                              // direct connection <-- maybe needs to be changed in case of direct indirect connection???
+    );
 
-  bool got_sem = m_semaphoreOpenEvt.timedWait("connect", timeoutMs);  // Wait for the connection to complete.
-  rc = m_semaphoreOpenEvt.value();
-  // check the status of the connection and cleanup in case of failure
-  if (!got_sem || rc != ESP_GATT_OK) {
+    if (errRc == ESP_OK) {
+      log_d("esp_ble_gattc_open returned OK, waiting for connection event...");
+      break;  // Success, exit retry loop
+    }
+
+    // Handle specific error cases that may benefit from retry
+    if (errRc == ESP_ERR_INVALID_STATE) {
+      // BLE stack may be busy, retry after delay
+      log_w("esp_ble_gattc_open: stack busy (ESP_ERR_INVALID_STATE), retry %d/%d", retryCount + 1, maxRetries);
+      m_semaphoreOpenEvt.give(ESP_GATT_ERROR);  // Release semaphore before retry
+      delay(retryDelayMs);
+      retryCount++;
+    } else {
+      // Other errors, don't retry
+      log_e("esp_ble_gattc_open: rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
+      m_semaphoreOpenEvt.give(ESP_GATT_ERROR);  // Release semaphore before cleanup
+      BLEDevice::removePeerDevice(m_appId, true);
+      esp_ble_gattc_app_unregister(m_gattc_if);
+      m_gattc_if = ESP_GATT_IF_NONE;
+      return false;
+    }
+  } while (retryCount < maxRetries);
+
+  // Check if we exhausted retries
+  if (errRc != ESP_OK) {
+    log_e("esp_ble_gattc_open failed after %d retries: rc=%d %s", maxRetries, errRc, GeneralUtils::errorToString(errRc));
+    m_semaphoreOpenEvt.give(ESP_GATT_ERROR);  // Release semaphore before cleanup
     BLEDevice::removePeerDevice(m_appId, true);
     esp_ble_gattc_app_unregister(m_gattc_if);
     m_gattc_if = ESP_GATT_IF_NONE;
+    return false;
   }
-  log_v("<< connect(), rc=%d", rc == ESP_GATT_OK);
-  return rc == ESP_GATT_OK;
+
+  log_d("Waiting for connection event (timeout=%d ms)...", timeoutMs);
+  bool got_sem = m_semaphoreOpenEvt.timedWait("connect", timeoutMs);  // Wait for the connection to complete.
+  rc = m_semaphoreOpenEvt.value();
+
+  // check the status of the connection and cleanup in case of failure
+  if (!got_sem) {
+    log_e("Connection timeout after %d ms to %s (no OPEN event received)", timeoutMs, address.toString().c_str());
+    // Cancel any pending connection attempt
+    esp_ble_gap_disconnect(getPeerAddress().getNative());
+    BLEDevice::removePeerDevice(m_appId, true);
+    esp_ble_gattc_app_unregister(m_gattc_if);
+    m_gattc_if = ESP_GATT_IF_NONE;
+    return false;
+  }
+
+  if (rc != ESP_GATT_OK) {
+    log_e("Connection failed to %s, status=%d %s", address.toString().c_str(), rc, GeneralUtils::errorToString(rc));
+    BLEDevice::removePeerDevice(m_appId, true);
+    esp_ble_gattc_app_unregister(m_gattc_if);
+    m_gattc_if = ESP_GATT_IF_NONE;
+    return false;
+  }
+
+  log_i("Connected to %s", address.toString().c_str());
+  return true;
 }  // connect
 
 /**

libraries/BLE/src/BLEScan.cpp

@@ -567,6 +567,14 @@ bool BLEScan::stop() {
   return true;
 }  // stop
 
+/**
+ * @brief Get the status of the scanner.
+ * @return true if scanning is active.
+ */
+bool BLEScan::isScanning() {
+  return !m_stopped;
+}  // isScanning
+
 #endif  // CONFIG_BLUEDROID_ENABLED
 
 /***************************************************************************

libraries/BLE/src/BLEScan.h

@@ -121,6 +121,7 @@ public:
   void erase(BLEAddress address);
   BLEScanResults *getResults();
   void clearResults();
+  bool isScanning();
 
   /***************************************************************************
    *                       Bluedroid public declarations                     *
@@ -141,7 +142,6 @@ public:
 
 #if defined(CONFIG_NIMBLE_ENABLED)
   void setDuplicateFilter(bool enabled);
-  bool isScanning();
   void clearDuplicateCache();
 #endif
 

libraries/BLE/src/BLEServer.cpp

@@ -189,6 +189,17 @@ void BLEServer::start() {
     return;
   }
 
+  // Re-set the device name after ble_gatts_start() because ble_svc_gap_init()
+  // (called in createServer) resets it to the default "nimble" from sdkconfig.
+  // The GAP service device name must be set after the GATT server is started.
+  String deviceName = BLEDevice::getDeviceName();
+  if (deviceName.length() > 0) {
+    rc = ble_svc_gap_device_name_set(deviceName.c_str());
+    if (rc != 0) {
+      log_e("ble_svc_gap_device_name_set: rc=%d %s", rc, BLEUtils::returnCodeToString(rc));
+    }
+  }
+
 #if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
   ble_gatts_show_local();
 #endif
@@ -475,6 +486,7 @@ void BLEServer::handleGATTServerEvent(esp_gatts_cb_event_t event, esp_gatt_if_t
     //
     case ESP_GATTS_CONNECT_EVT:
     {
+      log_i("Client connected, conn_id=%d", param->connect.conn_id);
       m_connId = param->connect.conn_id;
       addPeerDevice((void *)this, false, m_connId);
       if (m_pServerCallbacks != nullptr) {
@@ -514,6 +526,7 @@ void BLEServer::handleGATTServerEvent(esp_gatts_cb_event_t event, esp_gatt_if_t
     // we also want to start advertising again.
     case ESP_GATTS_DISCONNECT_EVT:
     {
+      log_i("Client disconnected, conn_id=%d, reason=%d", param->disconnect.conn_id, param->disconnect.reason);
       if (m_pServerCallbacks != nullptr) {  // If we have callbacks, call now.
         m_pServerCallbacks->onDisconnect(this);
         m_pServerCallbacks->onDisconnect(this, param);
@@ -563,6 +576,7 @@ void BLEServer::handleGATTServerEvent(esp_gatts_cb_event_t event, esp_gatt_if_t
     //
     case ESP_GATTS_REG_EVT:
     {
+      log_i("GATT server registered, status=%d, app_id=%d, gatts_if=%d", param->reg.status, param->reg.app_id, gatts_if);
       m_gatts_if = gatts_if;
       m_semaphoreRegisterAppEvt.give();  // Unlock the mutex waiting for the registration of the app.
       break;