Improve reliability of bluetooth active connections (#4049)

Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com>

esphome/components/bluetooth_proxy/bluetooth_proxy.cpp

@@ -128,6 +128,11 @@ BluetoothConnection *BluetoothProxy::get_connection_(uint64_t address, bool rese
   for (auto *connection : this->connections_) {
     if (connection->get_address() == 0) {
       connection->set_address(address);
+      // All connections must start at INIT
+      // We only set the state if we allocate the connection
+      // to avoid a race where multiple connection attempts
+      // are made.
+      connection->set_state(espbt::ClientState::INIT);
       return connection;
     }
   }
@@ -144,8 +149,19 @@ void BluetoothProxy::bluetooth_device_request(const api::BluetoothDeviceRequest
         api::global_api_server->send_bluetooth_device_connection(msg.address, false);
         return;
       }
-      ESP_LOGV(TAG, "[%d] [%s] Searching to connect", connection->get_connection_index(),
+      if (connection->state() == espbt::ClientState::CONNECTED ||
-               connection->address_str().c_str());
+          connection->state() == espbt::ClientState::ESTABLISHED) {
+        ESP_LOGW(TAG, "[%d] [%s] Connection already established", connection->get_connection_index(),
+                 connection->address_str().c_str());
+        api::global_api_server->send_bluetooth_device_connection(msg.address, true);
+        api::global_api_server->send_bluetooth_connections_free(this->get_bluetooth_connections_free(),
+                                                                this->get_bluetooth_connections_limit());
+        return;
+      } else if (connection->state() != espbt::ClientState::INIT) {
+        ESP_LOGW(TAG, "[%d] [%s] Connection already in progress", connection->get_connection_index(),
+                 connection->address_str().c_str());
+        return;
+      }
       connection->set_state(espbt::ClientState::SEARCHING);
       api::global_api_server->send_bluetooth_connections_free(this->get_bluetooth_connections_free(),
                                                               this->get_bluetooth_connections_limit());

esphome/components/esp32_ble_client/ble_client_base.cpp

@@ -23,7 +23,9 @@ void BLEClientBase::setup() {
 }
 
 void BLEClientBase::loop() {
-  if (this->state_ == espbt::ClientState::DISCOVERED) {
+  // READY_TO_CONNECT means we have discovered the device
+  // and the scanner has been stopped by the tracker.
+  if (this->state_ == espbt::ClientState::READY_TO_CONNECT) {
     this->connect();
   }
 }
@@ -51,7 +53,8 @@ bool BLEClientBase::parse_device(const espbt::ESPBTDevice &device) {
 }
 
 void BLEClientBase::connect() {
-  ESP_LOGI(TAG, "[%d] [%s] Attempting BLE connection", this->connection_index_, this->address_str_.c_str());
+  ESP_LOGI(TAG, "[%d] [%s] 0x%02x Attempting BLE connection", this->connection_index_, this->address_str_.c_str(),
+           this->remote_addr_type_);
   auto ret = esp_ble_gattc_open(this->gattc_if_, this->remote_bda_, this->remote_addr_type_, true);
   if (ret) {
     ESP_LOGW(TAG, "[%d] [%s] esp_ble_gattc_open error, status=%d", this->connection_index_, this->address_str_.c_str(),
@@ -63,6 +66,8 @@ void BLEClientBase::connect() {
 }
 
 void BLEClientBase::disconnect() {
+  if (this->state_ == espbt::ClientState::IDLE || this->state_ == espbt::ClientState::DISCONNECTING)
+    return;
   ESP_LOGI(TAG, "[%d] [%s] Disconnecting.", this->connection_index_, this->address_str_.c_str());
   auto err = esp_ble_gattc_close(this->gattc_if_, this->conn_id_);
   if (err != ESP_OK) {
@@ -70,9 +75,12 @@ void BLEClientBase::disconnect() {
              err);
   }
 
-  if (this->state_ == espbt::ClientState::SEARCHING) {
+  if (this->state_ == espbt::ClientState::SEARCHING || this->state_ == espbt::ClientState::READY_TO_CONNECT ||
+      this->state_ == espbt::ClientState::DISCOVERED) {
     this->set_address(0);
     this->set_state(espbt::ClientState::IDLE);
+  } else {
+    this->set_state(espbt::ClientState::DISCONNECTING);
   }
 }
 

esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp

@@ -66,7 +66,11 @@ void ESP32BLETracker::setup() {
 #endif
 
   if (this->scan_continuous_) {
-    this->start_scan_(true);
+    if (xSemaphoreTake(this->scan_end_lock_, 0L)) {
+      this->start_scan_(true);
+    } else {
+      ESP_LOGW(TAG, "Cannot start scan!");
+    }
   }
 }
 
@@ -83,86 +87,106 @@ void ESP32BLETracker::loop() {
     ble_event = this->ble_events_.pop();
   }
 
-  if (this->scanner_idle_) {
+  int connecting = 0;
-    return;
+  int discovered = 0;
-  }
+  int searching = 0;
-
-  bool connecting = false;
   for (auto *client : this->clients_) {
-    if (client->state() == ClientState::CONNECTING || client->state() == ClientState::DISCOVERED)
+    switch (client->state()) {
-      connecting = true;
+      case ClientState::DISCOVERED:
-  }
+        discovered++;
-
+        break;
-  if (!connecting && xSemaphoreTake(this->scan_end_lock_, 0L)) {
+      case ClientState::SEARCHING:
-    xSemaphoreGive(this->scan_end_lock_);
+        searching++;
-    if (this->scan_continuous_) {
+        break;
-      this->start_scan_(false);
+      case ClientState::CONNECTING:
-    } else if (xSemaphoreTake(this->scan_end_lock_, 0L) && !this->scanner_idle_) {
+      case ClientState::READY_TO_CONNECT:
-      xSemaphoreGive(this->scan_end_lock_);
+        connecting++;
-      this->end_of_scan_();
+        break;
-      return;
+      default:
+        break;
     }
   }
+  bool promote_to_connecting = discovered && !searching && !connecting;
 
-  if (xSemaphoreTake(this->scan_result_lock_, 5L / portTICK_PERIOD_MS)) {
+  if (!this->scanner_idle_) {
-    uint32_t index = this->scan_result_index_;
+    if (this->scan_result_index_ &&  // if it looks like we have a scan result we will take the lock
-    xSemaphoreGive(this->scan_result_lock_);
+        xSemaphoreTake(this->scan_result_lock_, 5L / portTICK_PERIOD_MS)) {
+      uint32_t index = this->scan_result_index_;
+      if (index) {
+        if (index >= 16) {
+          ESP_LOGW(TAG, "Too many BLE events to process. Some devices may not show up.");
+        }
+        for (size_t i = 0; i < index; i++) {
+          ESPBTDevice device;
+          device.parse_scan_rst(this->scan_result_buffer_[i]);
 
-    if (index >= 16) {
+          bool found = false;
-      ESP_LOGW(TAG, "Too many BLE events to process. Some devices may not show up.");
+          for (auto *listener : this->listeners_) {
-    }
+            if (listener->parse_device(device))
-    for (size_t i = 0; i < index; i++) {
+              found = true;
-      ESPBTDevice device;
+          }
-      device.parse_scan_rst(this->scan_result_buffer_[i]);
 
-      bool found = false;
+          for (auto *client : this->clients_) {
-      for (auto *listener : this->listeners_) {
+            if (client->parse_device(device)) {
-        if (listener->parse_device(device))
+              found = true;
-          found = true;
+              if (!connecting && client->state() == ClientState::DISCOVERED) {
-      }
+                promote_to_connecting = true;
-
+              }
-      for (auto *client : this->clients_) {
-        if (client->parse_device(device)) {
-          found = true;
-          if (client->state() == ClientState::DISCOVERED) {
-            esp_ble_gap_stop_scanning();
-#ifdef USE_ARDUINO
-            constexpr TickType_t block_time = 10L / portTICK_PERIOD_MS;
-#else
-            constexpr TickType_t block_time = 0L;  // PR #3594
-#endif
-            if (xSemaphoreTake(this->scan_end_lock_, block_time)) {
-              xSemaphoreGive(this->scan_end_lock_);
             }
           }
+
+          if (!found && !this->scan_continuous_) {
+            this->print_bt_device_info(device);
+          }
         }
+        this->scan_result_index_ = 0;
       }
-
-      if (!found) {
-        this->print_bt_device_info(device);
-      }
-    }
-
-    if (xSemaphoreTake(this->scan_result_lock_, 10L / portTICK_PERIOD_MS)) {
-      this->scan_result_index_ = 0;
       xSemaphoreGive(this->scan_result_lock_);
     }
+
+    if (this->scan_set_param_failed_) {
+      ESP_LOGE(TAG, "Scan set param failed: %d", this->scan_set_param_failed_);
+      esp_ble_gap_stop_scanning();
+      this->scan_set_param_failed_ = ESP_BT_STATUS_SUCCESS;
+    }
+
+    if (this->scan_start_failed_) {
+      ESP_LOGE(TAG, "Scan start failed: %d", this->scan_start_failed_);
+      esp_ble_gap_stop_scanning();
+      this->scan_start_failed_ = ESP_BT_STATUS_SUCCESS;
+    }
+
+    if (!connecting && xSemaphoreTake(this->scan_end_lock_, 0L)) {
+      if (this->scan_continuous_) {
+        if (!promote_to_connecting) {
+          this->start_scan_(false);
+        }
+      } else if (!this->scanner_idle_) {
+        this->end_of_scan_();
+        return;
+      }
+    }
   }
 
-  if (this->scan_set_param_failed_) {
+  // If there is a discovered client and no connecting
-    ESP_LOGE(TAG, "Scan set param failed: %d", this->scan_set_param_failed_);
+  // clients and no clients using the scanner to search for
-    this->scan_set_param_failed_ = ESP_BT_STATUS_SUCCESS;
+  // devices, then stop scanning and promote the discovered
-  }
+  // client to ready to connect.
-
+  if (promote_to_connecting) {
-  if (this->scan_start_failed_) {
+    for (auto *client : this->clients_) {
-    ESP_LOGE(TAG, "Scan start failed: %d", this->scan_start_failed_);
+      if (client->state() == ClientState::DISCOVERED) {
-    this->scan_start_failed_ = ESP_BT_STATUS_SUCCESS;
+        ESP_LOGD(TAG, "Pausing scan to make connection...");
+        esp_ble_gap_stop_scanning();
+        // We only want to promote one client at a time.
+        client->set_state(ClientState::READY_TO_CONNECT);
+        break;
+      }
+    }
   }
 }
 
 void ESP32BLETracker::start_scan() {
   if (xSemaphoreTake(this->scan_end_lock_, 0L)) {
-    xSemaphoreGive(this->scan_end_lock_);
     this->start_scan_(true);
   } else {
     ESP_LOGW(TAG, "Scan requested when a scan is already in progress. Ignoring.");
@@ -256,8 +280,9 @@ bool ESP32BLETracker::ble_setup() {
 }
 
 void ESP32BLETracker::start_scan_(bool first) {
-  if (!xSemaphoreTake(this->scan_end_lock_, 0L)) {
+  // The lock must be held when calling this function.
-    ESP_LOGW(TAG, "Cannot start scan!");
+  if (xSemaphoreTake(this->scan_end_lock_, 0L)) {
+    ESP_LOGE(TAG, "start_scan called without holding scan_end_lock_");
     return;
   }
 
@@ -284,8 +309,9 @@ void ESP32BLETracker::start_scan_(bool first) {
 }
 
 void ESP32BLETracker::end_of_scan_() {
-  if (!xSemaphoreTake(this->scan_end_lock_, 0L)) {
+  // The lock must be held when calling this function.
-    ESP_LOGW(TAG, "Cannot clean up end of scan!");
+  if (xSemaphoreTake(this->scan_end_lock_, 0L)) {
+    ESP_LOGE(TAG, "end_of_scan_ called without holding the scan_end_lock_");
     return;
   }
 
@@ -337,6 +363,9 @@ void ESP32BLETracker::gap_scan_set_param_complete_(const esp_ble_gap_cb_param_t:
 
 void ESP32BLETracker::gap_scan_start_complete_(const esp_ble_gap_cb_param_t::ble_scan_start_cmpl_evt_param &param) {
   this->scan_start_failed_ = param.status;
+  if (param.status != ESP_BT_STATUS_SUCCESS) {
+    xSemaphoreGive(this->scan_end_lock_);
+  }
 }
 
 void ESP32BLETracker::gap_scan_stop_complete_(const esp_ble_gap_cb_param_t::ble_scan_stop_cmpl_evt_param &param) {

esphome/components/esp32_ble_tracker/esp32_ble_tracker.h

@@ -145,12 +145,18 @@ class ESPBTDeviceListener {
 };
 
 enum class ClientState {
+  // Connection is allocated
+  INIT,
+  // Client is disconnecting
+  DISCONNECTING,
   // Connection is idle, no device detected.
   IDLE,
   // Searching for device.
   SEARCHING,
   // Device advertisement found.
   DISCOVERED,
+  // Device is discovered and the scanner is stopped
+  READY_TO_CONNECT,
   // Connection in progress.
   CONNECTING,
   // Initial connection established.