From a28214caec53b9a4596df6b418fac354e8e13fd7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 29 Nov 2022 19:56:31 -1000 Subject: [PATCH] Fix bluetooth remote connections not picking the best path (#82957) --- .../components/bluetooth/wrappers.py | 68 ++++++------------ tests/components/bluetooth/test_models.py | 69 ++++++++++++++++--- .../keymitt_ble/test_config_flow.py | 55 ++++++++------- 3 files changed, 113 insertions(+), 79 deletions(-) diff --git a/homeassistant/components/bluetooth/wrappers.py b/homeassistant/components/bluetooth/wrappers.py index 9565886c9a3..ca4f2314b65 100644 --- a/homeassistant/components/bluetooth/wrappers.py +++ b/homeassistant/components/bluetooth/wrappers.py @@ -12,7 +12,7 @@ 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_retry_connector import NO_RSSI_VALUE, freshen_ble_device +from bleak_retry_connector import NO_RSSI_VALUE from homeassistant.core import CALLBACK_TYPE, callback as hass_callback from homeassistant.helpers.frame import report @@ -182,25 +182,17 @@ class HaBleakClientWrapper(BleakClient): async def connect(self, **kwargs: Any) -> bool: """Connect to the specified GATT server.""" - if ( - not self._backend - or not self.__ble_device - or not self._async_get_backend_for_ble_device(self.__ble_device) - ): - assert models.MANAGER is not None - wrapped_backend = ( - self._async_get_backend() or self._async_get_fallback_backend() - ) - self.__ble_device = ( - await freshen_ble_device(wrapped_backend.device) - or wrapped_backend.device - ) - self._backend = wrapped_backend.client( - self.__ble_device, - disconnected_callback=self.__disconnected_callback, - timeout=self.__timeout, - hass=models.MANAGER.hass, - ) + assert models.MANAGER is not None + ( + wrapped_backend, + self.__ble_device, + ) = self._async_get_best_available_backend_and_device() + self._backend = wrapped_backend.client( + self.__ble_device, + disconnected_callback=self.__disconnected_callback, + timeout=self.__timeout, + hass=models.MANAGER.hass, + ) return await super().connect(**kwargs) @hass_callback @@ -224,29 +216,14 @@ class HaBleakClientWrapper(BleakClient): return _HaWrappedBleakBackend(ble_device, connector.client) @hass_callback - def _async_get_backend(self) -> _HaWrappedBleakBackend | None: - """Get the bleak backend for the given address.""" - assert models.MANAGER is not None - address = self.__address - ble_device = models.MANAGER.async_ble_device_from_address(address, True) - if ble_device is None: - raise BleakError(f"No device found for address {address}") + def _async_get_best_available_backend_and_device( + self, + ) -> tuple[_HaWrappedBleakBackend, BLEDevice]: + """Get a best available backend and device for the given address. - if backend := self._async_get_backend_for_ble_device(ble_device): - return backend - - return None - - @hass_callback - def _async_get_fallback_backend(self) -> _HaWrappedBleakBackend: - """Get a fallback backend for the given address.""" - # - # The preferred backend cannot currently connect the device - # because it is likely out of connection slots. - # - # We need to try all backends to find one that can - # connect to the device. - # + 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( @@ -258,10 +235,9 @@ class HaBleakClientWrapper(BleakClient): or NO_RSSI_VALUE, reverse=True, ): - if backend := self._async_get_backend_for_ble_device( - device_advertisement_data[0] - ): - return backend + ble_device = device_advertisement_data[0] + if backend := self._async_get_backend_for_ble_device(ble_device): + return backend, ble_device raise BleakError( f"No backend with an available connection slot that can reach address {address} was found" diff --git a/tests/components/bluetooth/test_models.py b/tests/components/bluetooth/test_models.py index 05f67b97daa..612f33a68bd 100644 --- a/tests/components/bluetooth/test_models.py +++ b/tests/components/bluetooth/test_models.py @@ -60,22 +60,73 @@ async def test_wrapped_bleak_client_set_disconnected_callback_after_connected( hass, enable_bluetooth, one_adapter ): """Test wrapped bleak client can set a disconnected callback after connected.""" + manager = _get_manager() + + switchbot_proxy_device_has_connection_slot = BLEDevice( + "44:44:33:11:23:45", + "wohand", + { + "connector": HaBluetoothConnector( + MockBleakClient, "mock_bleak_client", lambda: True + ), + "path": "/org/bluez/hci0/dev_44_44_33_11_23_45", + }, + rssi=-40, + ) + switchbot_proxy_device_adv_has_connection_slot = generate_advertisement_data( + local_name="wohand", + service_uuids=[], + manufacturer_data={1: b"\x01"}, + rssi=-40, + ) switchbot_device = BLEDevice( - "44:44:33:11:23:45", "wohand", {"path": "/org/bluez/hci0/dev_44_44_33_11_23_45"} + "44:44:33:11:23:45", + "wohand", + {"path": "/org/bluez/hci0/dev_44_44_33_11_23_45"}, ) switchbot_adv = generate_advertisement_data( - local_name="wohand", service_uuids=[], manufacturer_data={1: b"\x01"} + local_name="wohand", service_uuids=[], manufacturer_data={1: b"\x01"}, rssi=-100 ) - inject_advertisement(hass, switchbot_device, switchbot_adv) - client = HaBleakClientWrapper(switchbot_device) - with patch( - "bleak.backends.bluezdbus.client.BleakClientBlueZDBus.connect" - ) as connect: + + inject_advertisement_with_source( + hass, switchbot_device, switchbot_adv, "00:00:00:00:00:01" + ) + inject_advertisement_with_source( + hass, + switchbot_proxy_device_has_connection_slot, + switchbot_proxy_device_adv_has_connection_slot, + "esp32_has_connection_slot", + ) + + class FakeScanner(BaseHaScanner): + @property + def discovered_devices_and_advertisement_data( + self, + ) -> dict[str, tuple[BLEDevice, AdvertisementData]]: + """Return a list of discovered devices.""" + return { + switchbot_proxy_device_has_connection_slot.address: ( + switchbot_proxy_device_has_connection_slot, + switchbot_proxy_device_adv_has_connection_slot, + ) + } + + async def async_get_device_by_address(self, address: str) -> BLEDevice | None: + """Return a list of discovered devices.""" + if address == switchbot_proxy_device_has_connection_slot.address: + return switchbot_proxy_device_has_connection_slot + return None + + scanner = FakeScanner(hass, "esp32", "esp32") + cancel = manager.async_register_scanner(scanner, True) + + client = HaBleakClientWrapper(switchbot_proxy_device_has_connection_slot) + with patch("bleak.backends.bluezdbus.client.BleakClientBlueZDBus.connect"): await client.connect() - assert len(connect.mock_calls) == 1 - assert client._backend is not None + assert client.is_connected is True client.set_disconnected_callback(lambda client: None) await client.disconnect() + cancel() async def test_ble_device_with_proxy_client_out_of_connections( diff --git a/tests/components/keymitt_ble/test_config_flow.py b/tests/components/keymitt_ble/test_config_flow.py index 81e6a0be8e7..7729f71f08c 100644 --- a/tests/components/keymitt_ble/test_config_flow.py +++ b/tests/components/keymitt_ble/test_config_flow.py @@ -1,6 +1,6 @@ """Test the MicroBot config flow.""" -from unittest.mock import ANY, patch +from unittest.mock import ANY, AsyncMock, patch from homeassistant.config_entries import SOURCE_BLUETOOTH, SOURCE_USER from homeassistant.const import CONF_ACCESS_TOKEN, CONF_ADDRESS @@ -9,7 +9,6 @@ from homeassistant.data_entry_flow import FlowResultType from . import ( SERVICE_INFO, USER_INPUT, - MockMicroBotApiClient, MockMicroBotApiClientFail, patch_async_setup_entry, ) @@ -19,6 +18,13 @@ from tests.common import MockConfigEntry DOMAIN = "keymitt_ble" +def patch_microbot_api(): + """Patch MicroBot API.""" + return patch( + "homeassistant.components.keymitt_ble.config_flow.MicroBotApiClient", AsyncMock + ) + + async def test_bluetooth_discovery(hass): """Test discovery via bluetooth with a valid device.""" result = await hass.config_entries.flow.async_init( @@ -29,7 +35,7 @@ async def test_bluetooth_discovery(hass): assert result["type"] == FlowResultType.FORM assert result["step_id"] == "init" - with patch_async_setup_entry() as mock_setup_entry: + with patch_async_setup_entry() as mock_setup_entry, patch_microbot_api(): result = await hass.config_entries.flow.async_configure( result["flow_id"], USER_INPUT, @@ -51,13 +57,14 @@ async def test_bluetooth_discovery_already_setup(hass): unique_id="aa:bb:cc:dd:ee:ff", ) entry.add_to_hass(hass) - result = await hass.config_entries.flow.async_init( - DOMAIN, - context={"source": SOURCE_BLUETOOTH}, - data=SERVICE_INFO, - ) - assert result["type"] == FlowResultType.ABORT - assert result["reason"] == "already_configured" + with patch_microbot_api(): + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": SOURCE_BLUETOOTH}, + data=SERVICE_INFO, + ) + assert result["type"] == FlowResultType.ABORT + assert result["reason"] == "already_configured" async def test_user_setup(hass): @@ -74,19 +81,18 @@ async def test_user_setup(hass): assert result["step_id"] == "init" assert result["errors"] == {} - result2 = await hass.config_entries.flow.async_configure( - result["flow_id"], - USER_INPUT, - ) + with patch_microbot_api(): + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + USER_INPUT, + ) + await hass.async_block_till_done() assert result2["type"] == FlowResultType.FORM assert result2["step_id"] == "link" assert result2["errors"] is None - with patch( - "homeassistant.components.keymitt_ble.config_flow.MicroBotApiClient", - MockMicroBotApiClient, - ), patch_async_setup_entry() as mock_setup_entry: + with patch_microbot_api(), patch_async_setup_entry() as mock_setup_entry: result3 = await hass.config_entries.flow.async_configure( result2["flow_id"], USER_INPUT, @@ -124,7 +130,7 @@ async def test_user_setup_already_configured(hass): async def test_user_no_devices(hass): """Test the user initiated form with valid mac.""" - with patch( + with patch_microbot_api(), patch( "homeassistant.components.keymitt_ble.config_flow.async_discovered_service_info", return_value=[], ): @@ -138,7 +144,7 @@ async def test_user_no_devices(hass): async def test_no_link(hass): """Test the user initiated form with invalid response.""" - with patch( + with patch_microbot_api(), patch( "homeassistant.components.keymitt_ble.config_flow.async_discovered_service_info", return_value=[SERVICE_INFO], ): @@ -149,10 +155,11 @@ async def test_no_link(hass): assert result["step_id"] == "init" assert result["errors"] == {} - result2 = await hass.config_entries.flow.async_configure( - result["flow_id"], - USER_INPUT, - ) + with patch_microbot_api(): + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + USER_INPUT, + ) assert result2["type"] == FlowResultType.FORM assert result2["step_id"] == "link"