From 8c70e5aaaddbc689a0a1dd50242a0a48da2d5605 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 23 Dec 2022 15:48:47 -1000 Subject: [PATCH] Try the next best adapter after a BLE connection fails (#84512) * Try the next best adapter after a BLE connection fails * add cover * tweak * tweak * Update homeassistant/components/bluetooth/wrappers.py * bump * small tweak * tweak logic --- homeassistant/components/bluetooth/manager.py | 23 ++-- .../components/bluetooth/manifest.json | 2 +- .../components/bluetooth/wrappers.py | 107 ++++++++++++++---- homeassistant/package_constraints.txt | 2 +- requirements_all.txt | 2 +- requirements_test_all.txt | 2 +- tests/components/bluetooth/test_wrappers.py | 73 +++++++++++- 7 files changed, 171 insertions(+), 40 deletions(-) diff --git a/homeassistant/components/bluetooth/manager.py b/homeassistant/components/bluetooth/manager.py index 7dd993b7d5c..748b685d866 100644 --- a/homeassistant/components/bluetooth/manager.py +++ b/homeassistant/components/bluetooth/manager.py @@ -217,20 +217,19 @@ class BluetoothManager: uninstall_multiple_bleak_catcher() @hass_callback - def async_get_discovered_devices_and_advertisement_data_by_address( + def async_get_scanner_discovered_devices_and_advertisement_data_by_address( self, address: str, connectable: bool - ) -> list[tuple[BLEDevice, AdvertisementData]]: - """Get devices and advertisement_data by address.""" + ) -> list[tuple[BaseHaScanner, BLEDevice, AdvertisementData]]: + """Get scanner, devices, and advertisement_data by address.""" types_ = (True,) if connectable else (True, False) - return [ - device_advertisement_data - for device_advertisement_data in ( - scanner.discovered_devices_and_advertisement_data.get(address) - for type_ in types_ - for scanner in self._get_scanners_by_type(type_) - ) - if device_advertisement_data is not None - ] + results: list[tuple[BaseHaScanner, BLEDevice, AdvertisementData]] = [] + for type_ in types_: + for scanner in self._get_scanners_by_type(type_): + if device_advertisement_data := scanner.discovered_devices_and_advertisement_data.get( + address + ): + results.append((scanner, *device_advertisement_data)) + return results @hass_callback def _async_all_discovered_addresses(self, connectable: bool) -> Iterable[str]: diff --git a/homeassistant/components/bluetooth/manifest.json b/homeassistant/components/bluetooth/manifest.json index d4f60621778..b20a3f17b50 100644 --- a/homeassistant/components/bluetooth/manifest.json +++ b/homeassistant/components/bluetooth/manifest.json @@ -7,7 +7,7 @@ "quality_scale": "internal", "requirements": [ "bleak==0.19.2", - "bleak-retry-connector==2.12.1", + "bleak-retry-connector==2.13.0", "bluetooth-adapters==0.15.2", "bluetooth-auto-recovery==1.0.3", "bluetooth-data-tools==0.3.1", diff --git a/homeassistant/components/bluetooth/wrappers.py b/homeassistant/components/bluetooth/wrappers.py index 64a010fed4d..a2c417ca382 100644 --- a/homeassistant/components/bluetooth/wrappers.py +++ b/homeassistant/components/bluetooth/wrappers.py @@ -5,13 +5,18 @@ import asyncio from collections.abc import Callable import contextlib from dataclasses import dataclass +from functools import partial import logging from typing import TYPE_CHECKING, Any, Final from bleak import BleakClient, BleakError from bleak.backends.client import BaseBleakClient, get_platform_client_backend_type from bleak.backends.device import BLEDevice -from bleak.backends.scanner import AdvertisementDataCallback, BaseBleakScanner +from bleak.backends.scanner import ( + AdvertisementData, + AdvertisementDataCallback, + BaseBleakScanner, +) from bleak_retry_connector import ( NO_RSSI_VALUE, ble_device_description, @@ -23,6 +28,7 @@ from homeassistant.core import CALLBACK_TYPE, callback as hass_callback from homeassistant.helpers.frame import report from . import models +from .base_scanner import BaseHaScanner FILTER_UUIDS: Final = "UUIDs" _LOGGER = logging.getLogger(__name__) @@ -37,6 +43,7 @@ class _HaWrappedBleakBackend: """Wrap bleak backend to make it usable by Home Assistant.""" device: BLEDevice + scanner: BaseHaScanner client: type[BaseBleakClient] source: str | None @@ -140,6 +147,33 @@ class HaBleakScannerWrapper(BaseBleakScanner): asyncio.get_running_loop().call_soon_threadsafe(self._detection_cancel) +def _rssi_sorter_with_connection_failure_penalty( + scanner_device_advertisement_data: tuple[ + BaseHaScanner, BLEDevice, AdvertisementData + ], + connection_failure_count: dict[BaseHaScanner, int], + rssi_diff: int, +) -> float: + """Get a sorted list of scanner, device, advertisement data adjusting for previous connection failures. + + When a connection fails, we want to try the next best adapter so we + apply a penalty to the RSSI value to make it less likely to be chosen + for every previous connection failure. + + We use the 51% of the RSSI difference between the first and second + best adapter as the penalty. This ensures we will always try the + best adapter twice before moving on to the next best adapter since + the first failure may be a transient service resolution issue. + """ + scanner, _, advertisement_data = scanner_device_advertisement_data + base_rssi = advertisement_data.rssi or NO_RSSI_VALUE + if connect_failures := connection_failure_count.get(scanner): + if connect_failures > 1 and not rssi_diff: + rssi_diff = 1 + return base_rssi - (rssi_diff * connect_failures * 0.51) + return base_rssi + + class HaBleakClientWrapper(BleakClient): """Wrap the BleakClient to ensure it does not shutdown our scanner. @@ -171,6 +205,7 @@ class HaBleakClientWrapper(BleakClient): self.__address = address_or_ble_device self.__disconnected_callback = disconnected_callback self.__timeout = timeout + self.__connect_failures: dict[BaseHaScanner, int] = {} self._backend: BaseBleakClient | None = None # type: ignore[assignment] @property @@ -197,12 +232,13 @@ class HaBleakClientWrapper(BleakClient): async def connect(self, **kwargs: Any) -> bool: """Connect to the specified GATT server.""" assert models.MANAGER is not None - wrapped_backend = self._async_get_best_available_backend_and_device() + manager = models.MANAGER + wrapped_backend = self._async_get_best_available_backend_and_device(manager) self._backend = wrapped_backend.client( wrapped_backend.device, disconnected_callback=self.__disconnected_callback, timeout=self.__timeout, - hass=models.MANAGER.hass, + hass=manager.hass, ) if debug_logging := _LOGGER.isEnabledFor(logging.DEBUG): # Only lookup the description if we are going to log it @@ -215,8 +251,12 @@ class HaBleakClientWrapper(BleakClient): finally: # If we failed to connect and its a local adapter (no source) # we release the connection slot - if not connected and not wrapped_backend.source: - models.MANAGER.async_release_connection_slot(wrapped_backend.device) + if not connected: + self.__connect_failures[wrapped_backend.scanner] = ( + self.__connect_failures.get(wrapped_backend.scanner, 0) + 1 + ) + if not wrapped_backend.source: + manager.async_release_connection_slot(wrapped_backend.device) if debug_logging: _LOGGER.debug("%s: Connected (last rssi: %s)", description, rssi) @@ -224,7 +264,7 @@ class HaBleakClientWrapper(BleakClient): @hass_callback def _async_get_backend_for_ble_device( - self, manager: BluetoothManager, ble_device: BLEDevice + self, manager: BluetoothManager, scanner: BaseHaScanner, ble_device: BLEDevice ) -> _HaWrappedBleakBackend | None: """Get the backend for a BLEDevice.""" if not (source := device_source(ble_device)): @@ -233,41 +273,68 @@ class HaBleakClientWrapper(BleakClient): if not manager.async_allocate_connection_slot(ble_device): return None cls = get_platform_client_backend_type() - return _HaWrappedBleakBackend(ble_device, cls, source) + return _HaWrappedBleakBackend(ble_device, scanner, cls, source) # Make sure the backend can connect to the device # as some backends have connection limits - if ( - not (scanner := manager.async_scanner_by_source(source)) - or not scanner.connector - or not scanner.connector.can_connect() - ): + if not scanner.connector or not scanner.connector.can_connect(): return None - return _HaWrappedBleakBackend(ble_device, scanner.connector.client, source) + return _HaWrappedBleakBackend( + ble_device, scanner, scanner.connector.client, source + ) @hass_callback def _async_get_best_available_backend_and_device( - self, + self, manager: BluetoothManager ) -> _HaWrappedBleakBackend: """Get a best available backend and device for the given address. This method will return the backend with the best rssi that has a free connection slot. """ - assert models.MANAGER is not None address = self.__address - device_advertisement_datas = models.MANAGER.async_get_discovered_devices_and_advertisement_data_by_address( + scanner_device_advertisement_datas = manager.async_get_scanner_discovered_devices_and_advertisement_data_by_address( address, True ) - for device_advertisement_data in sorted( - device_advertisement_datas, - key=lambda device_advertisement_data: device_advertisement_data[1].rssi + sorted_scanner_device_advertisement_datas = sorted( + scanner_device_advertisement_datas, + key=lambda scanner_device_advertisement_data: scanner_device_advertisement_data[ + 2 + ].rssi or NO_RSSI_VALUE, reverse=True, + ) + + # If we have connection failures we adjust the rssi sorting + # to prefer the adapter/scanner with the less failures so + # we don't keep trying to connect with an adapter + # that is failing + if ( + self.__connect_failures + and len(sorted_scanner_device_advertisement_datas) > 1 ): + # We use the rssi diff between to the top two + # to adjust the rssi sorter so that each failure + # will reduce the rssi sorter by the diff amount + rssi_diff = ( + sorted_scanner_device_advertisement_datas[0][2].rssi + - sorted_scanner_device_advertisement_datas[1][2].rssi + ) + adjusted_rssi_sorter = partial( + _rssi_sorter_with_connection_failure_penalty, + connection_failure_count=self.__connect_failures, + rssi_diff=rssi_diff, + ) + sorted_scanner_device_advertisement_datas = sorted( + scanner_device_advertisement_datas, + key=adjusted_rssi_sorter, + reverse=True, + ) + + for (scanner, ble_device, _) in sorted_scanner_device_advertisement_datas: if backend := self._async_get_backend_for_ble_device( - models.MANAGER, device_advertisement_data[0] + manager, scanner, ble_device ): return backend diff --git a/homeassistant/package_constraints.txt b/homeassistant/package_constraints.txt index 9cccf280e3f..4c3accd7015 100644 --- a/homeassistant/package_constraints.txt +++ b/homeassistant/package_constraints.txt @@ -10,7 +10,7 @@ atomicwrites-homeassistant==1.4.1 attrs==22.1.0 awesomeversion==22.9.0 bcrypt==3.1.7 -bleak-retry-connector==2.12.1 +bleak-retry-connector==2.13.0 bleak==0.19.2 bluetooth-adapters==0.15.2 bluetooth-auto-recovery==1.0.3 diff --git a/requirements_all.txt b/requirements_all.txt index 01341f8ded4..8a8daba6fdf 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -428,7 +428,7 @@ bimmer_connected==0.10.4 bizkaibus==0.1.1 # homeassistant.components.bluetooth -bleak-retry-connector==2.12.1 +bleak-retry-connector==2.13.0 # homeassistant.components.bluetooth bleak==0.19.2 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 32d67a1db3d..51762667025 100644 --- a/requirements_test_all.txt +++ b/requirements_test_all.txt @@ -352,7 +352,7 @@ bellows==0.34.5 bimmer_connected==0.10.4 # homeassistant.components.bluetooth -bleak-retry-connector==2.12.1 +bleak-retry-connector==2.13.0 # homeassistant.components.bluetooth bleak==0.19.2 diff --git a/tests/components/bluetooth/test_wrappers.py b/tests/components/bluetooth/test_wrappers.py index d7ba073b7b3..dd051ebb1fe 100644 --- a/tests/components/bluetooth/test_wrappers.py +++ b/tests/components/bluetooth/test_wrappers.py @@ -43,6 +43,10 @@ class FakeScanner(BaseHaRemoteScanner): ) self._details: dict[str, str | HaBluetoothConnector] = {} + def __repr__(self) -> str: + """Return the representation.""" + return f"FakeScanner({self.name})" + def inject_advertisement( self, device: BLEDevice, advertisement_data: AdvertisementData ) -> None: @@ -65,6 +69,7 @@ class BaseFakeBleakClient: def __init__(self, address_or_ble_device: Union[BLEDevice, str], **kwargs): """Initialize the fake bleak client.""" self._device_path = "/dev/test" + self._device = address_or_ble_device self._address = address_or_ble_device.address async def disconnect(self, *args, **kwargs): @@ -100,7 +105,7 @@ class FakeBleakClientRaisesOnConnect(BaseFakeBleakClient): def _generate_ble_device_and_adv_data( - interface: str, mac: str + interface: str, mac: str, rssi: int ) -> tuple[BLEDevice, AdvertisementData]: """Generate a BLE device with adv data.""" return ( @@ -110,7 +115,7 @@ def _generate_ble_device_and_adv_data( delegate="", details={"path": f"/org/bluez/{interface}/dev_{mac}"}, ), - generate_advertisement_data(), + generate_advertisement_data(rssi=rssi), ) @@ -158,13 +163,13 @@ def _generate_scanners_with_fake_devices(hass): hci0_device_advs = {} for i in range(10): device, adv_data = _generate_ble_device_and_adv_data( - "hci0", f"00:00:00:00:00:{i:02x}" + "hci0", f"00:00:00:00:00:{i:02x}", rssi=-60 ) hci0_device_advs[device.address] = (device, adv_data) hci1_device_advs = {} for i in range(10): device, adv_data = _generate_ble_device_and_adv_data( - "hci1", f"00:00:00:00:00:{i:02x}" + "hci1", f"00:00:00:00:00:{i:02x}", rssi=-80 ) hci1_device_advs[device.address] = (device, adv_data) @@ -296,3 +301,63 @@ async def test_release_slot_on_connect_exception( cancel_hci0() cancel_hci1() + + +async def test_we_switch_adapters_on_failure( + hass, + two_adapters, + enable_bluetooth, + install_bleak_catcher, +): + """Ensure we try the next best adapter after a failure.""" + hci0_device_advs, cancel_hci0, cancel_hci1 = _generate_scanners_with_fake_devices( + hass + ) + ble_device = hci0_device_advs["00:00:00:00:00:01"][0] + client = bleak.BleakClient(ble_device) + + class FakeBleakClientFailsHCI0Only(BaseFakeBleakClient): + """Fake bleak client that fails to connect.""" + + async def connect(self, *args, **kwargs): + """Connect.""" + if "/hci0/" in self._device.details["path"]: + return False + return True + + with patch( + "homeassistant.components.bluetooth.wrappers.get_platform_client_backend_type", + return_value=FakeBleakClientFailsHCI0Only, + ): + assert await client.connect() is False + + with patch( + "homeassistant.components.bluetooth.wrappers.get_platform_client_backend_type", + return_value=FakeBleakClientFailsHCI0Only, + ): + assert await client.connect() is False + + # After two tries we should switch to hci1 + with patch( + "homeassistant.components.bluetooth.wrappers.get_platform_client_backend_type", + return_value=FakeBleakClientFailsHCI0Only, + ): + assert await client.connect() is True + + # ..and we remember that hci1 works as long as the client doesn't change + with patch( + "homeassistant.components.bluetooth.wrappers.get_platform_client_backend_type", + return_value=FakeBleakClientFailsHCI0Only, + ): + assert await client.connect() is True + + # If we replace the client, we should try hci0 again + client = bleak.BleakClient(ble_device) + + with patch( + "homeassistant.components.bluetooth.wrappers.get_platform_client_backend_type", + return_value=FakeBleakClientFailsHCI0Only, + ): + assert await client.connect() is False + cancel_hci0() + cancel_hci1()