From c4620283f554ed373f84989ced320ec41f6a0e26 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 28 Jul 2022 12:37:56 -1000 Subject: [PATCH] Check the bluetoothctl version with asyncio.create_subprocess_exec --- CHANGELOG.rst | 1 + bleak/__init__.py | 4 - bleak/backends/bluezdbus/__init__.py | 29 +---- bleak/backends/bluezdbus/client.py | 28 ++--- bleak/backends/bluezdbus/version.py | 62 +++++++++++ requirements_dev.txt | 2 + tests/bleak/backends/bluezdbus/__init__.py | 0 .../bleak/backends/bluezdbus/test_version.py | 105 ++++++++++++++++++ 8 files changed, 185 insertions(+), 46 deletions(-) create mode 100644 bleak/backends/bluezdbus/version.py create mode 100644 tests/bleak/backends/bluezdbus/__init__.py create mode 100644 tests/bleak/backends/bluezdbus/test_version.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 508049dfa..bd02a296b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -32,6 +32,7 @@ Changed * ``BleakScanner()`` arg ``scanning_mode`` is no longer Windows-only and is no longer keyword-only. * All ``BleakScanner()`` instances in BlueZ backend now use common D-Bus object manager. * Deprecated ``filters`` kwarg in ``BleakScanner`` in BlueZ backend. +* BlueZ version is now checked on first connection instead of import to avoid blocking the event loop. Fixed ----- diff --git a/bleak/__init__.py b/bleak/__init__.py index 62e5c0b3e..3001dfdf4 100644 --- a/bleak/__init__.py +++ b/bleak/__init__.py @@ -12,7 +12,6 @@ import asyncio from bleak.__version__ import __version__ # noqa: F401 -from bleak.backends.bluezdbus import check_bluez_version from bleak.exc import BleakError _on_rtd = os.environ.get("READTHEDOCS") == "True" @@ -38,9 +37,6 @@ BleakClientP4Android as BleakClient, ) # noqa: F401 elif platform.system() == "Linux": - if not _on_ci and not check_bluez_version(5, 43): - raise BleakError("Bleak requires BlueZ >= 5.43.") - from bleak.backends.bluezdbus.scanner import ( BleakScannerBlueZDBus as BleakScanner, ) # noqa: F401 diff --git a/bleak/backends/bluezdbus/__init__.py b/bleak/backends/bluezdbus/__init__.py index ec049b4fa..688b9dfdb 100644 --- a/bleak/backends/bluezdbus/__init__.py +++ b/bleak/backends/bluezdbus/__init__.py @@ -1,28 +1 @@ -import re -import subprocess - -from ...exc import BleakError - - -def check_bluez_version(major: int, minor: int) -> bool: - """ - Checks the BlueZ version. - - Returns: - ``True`` if the BlueZ major version is equal to *major* and the minor - version is greater than or equal to *minor*, otherwise ``False``. - """ - # lazy-get the version and store it so we only have to run subprocess once - if not hasattr(check_bluez_version, "version"): - p = subprocess.Popen(["bluetoothctl", "--version"], stdout=subprocess.PIPE) - out, _ = p.communicate() - s = re.search(b"(\\d+).(\\d+)", out.strip(b"'")) - - if not s: - raise BleakError(f"Could not determine BlueZ version: {out.decode()}") - - setattr(check_bluez_version, "version", tuple(map(int, s.groups()))) - - bluez_major, bluez_minor = getattr(check_bluez_version, "version") - - return bluez_major == major and bluez_minor >= minor +"""BlueZ backend.""" diff --git a/bleak/backends/bluezdbus/client.py b/bleak/backends/bluezdbus/client.py index 39c605b2b..54d3b04b6 100644 --- a/bleak/backends/bluezdbus/client.py +++ b/bleak/backends/bluezdbus/client.py @@ -15,7 +15,7 @@ from dbus_next.message import Message from dbus_next.signature import Variant -from bleak.backends.bluezdbus import check_bluez_version, defs +from bleak.backends.bluezdbus import defs from bleak.backends.bluezdbus.characteristic import BleakGATTCharacteristicBlueZDBus from bleak.backends.bluezdbus.descriptor import BleakGATTDescriptorBlueZDBus from bleak.backends.bluezdbus.scanner import BleakScannerBlueZDBus @@ -30,7 +30,7 @@ from bleak.backends.device import BLEDevice from bleak.backends.service import BleakGATTServiceCollection from bleak.exc import BleakDBusError, BleakError - +from .version import BlueZFeatures logger = logging.getLogger(__name__) @@ -78,12 +78,6 @@ def __init__(self, address_or_ble_device: Union[BLEDevice, str], **kwargs): # used to override mtu_size property self._mtu_size: Optional[int] = None - # BlueZ version features - self._can_write_without_response = check_bluez_version(5, 46) - self._write_without_response_workaround_needed = not check_bluez_version(5, 51) - self._hides_battery_characteristic = check_bluez_version(5, 48) - self._hides_device_name_characteristic = check_bluez_version(5, 48) - # Connectivity methods async def connect(self, **kwargs) -> bool: @@ -105,6 +99,10 @@ async def connect(self, **kwargs) -> bool: if self.is_connected: raise BleakError("Client is already connected") + if not BlueZFeatures.checked_bluez_version: + await BlueZFeatures.check_bluez_version() + if not BlueZFeatures.supported_version: + raise BleakError("Bleak requires BlueZ >= 5.43.") # A Discover must have been run before connecting to any devices. # Find the desired device before trying to connect. timeout = kwargs.get("timeout", self._timeout) @@ -625,8 +623,9 @@ async def read_gatt_char( if not characteristic: # Special handling for BlueZ >= 5.48, where Battery Service (0000180f-0000-1000-8000-00805f9b34fb:) # has been moved to interface org.bluez.Battery1 instead of as a regular service. - if str(char_specifier) == "00002a19-0000-1000-8000-00805f9b34fb" and ( - self._hides_battery_characteristic + if ( + str(char_specifier) == "00002a19-0000-1000-8000-00805f9b34fb" + and BlueZFeatures.hides_battery_characteristic ): reply = await self._bus.call( Message( @@ -647,8 +646,9 @@ async def read_gatt_char( ) ) return value - if str(char_specifier) == "00002a00-0000-1000-8000-00805f9b34fb" and ( - self._hides_device_name_characteristic + if ( + str(char_specifier) == "00002a00-0000-1000-8000-00805f9b34fb" + and BlueZFeatures.hides_device_name_characteristic ): # Simulate regular characteristics read to be consistent over all platforms. value = bytearray(self._properties["Name"].encode("ascii")) @@ -780,9 +780,9 @@ async def write_gatt_char( ) # See docstring for details about this handling. - if not response and not self._can_write_without_response: + if not response and not BlueZFeatures.can_write_without_response: raise BleakError("Write without response requires at least BlueZ 5.46") - if response or not self._write_without_response_workaround_needed: + if response or not BlueZFeatures.write_without_response_workaround_needed: # TODO: Add OnValueUpdated handler for response=True? reply = await self._bus.call( Message( diff --git a/bleak/backends/bluezdbus/version.py b/bleak/backends/bluezdbus/version.py new file mode 100644 index 000000000..94572f1f6 --- /dev/null +++ b/bleak/backends/bluezdbus/version.py @@ -0,0 +1,62 @@ +import asyncio +import contextlib +import logging +import re +from typing import Optional + +logger = logging.getLogger(__name__) + + +async def _get_bluetoothctl_version(): + """Get the version of bluetoothctl.""" + with contextlib.suppress(Exception): + proc = await asyncio.create_subprocess_exec( + "bluetoothctl", "--version", stdout=asyncio.subprocess.PIPE + ) + out = await proc.stdout.read() + version = re.search(b"(\\d+).(\\d+)", out.strip(b"'")) + await proc.wait() + return version + return None + + +class BlueZFeatures: + """Check which features are supported by the BlueZ backend.""" + + checked_bluez_version = False + supported_version = True + can_write_without_response = True + write_without_response_workaround_needed = False + hides_battery_characteristic = True + hides_device_name_characteristic = True + _check_bluez_event: Optional[asyncio.Event] = None + + @classmethod + async def check_bluez_version(cls) -> None: + """Check the bluez version.""" + if cls._check_bluez_event: + # If there is already a check in progress + # it wins, wait for it instead + await cls._check_bluez_event.wait() + return + cls._check_bluez_event = asyncio.Event() + version_output = await _get_bluetoothctl_version() + if version_output: + major, minor = tuple(map(int, version_output.groups())) + cls.supported_version = major == 5 and minor >= 34 + cls.can_write_without_response = major == 5 and minor >= 46 + cls.write_without_response_workaround_needed = not ( + major == 5 and minor >= 51 + ) + cls.hides_battery_characteristic = major == 5 and minor >= 48 + cls.hides_device_name_characteristic = major == 5 and minor >= 48 + else: + # Its possible they may be running inside a container where + # bluetoothctl is not available and they only have access to the + # BlueZ D-Bus API. + logging.warning( + "Could not determine BlueZ version, bluetoothctl not available, assuming 5.51+" + ) + + cls._check_bluez_event.set() + cls.checked_bluez_version = True diff --git a/requirements_dev.txt b/requirements_dev.txt index f32868930..ab6174ca3 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -1,3 +1,4 @@ +asynctest>=0.13.0 pip>=18.0 bump2version==1.0.1 wheel>=0.32.2 @@ -11,5 +12,6 @@ Sphinx>=1.7.7 sphinx-rtd-theme>=1.0.0 PyYAML>=3.13 pytest>=3.9.2 +pytest-asyncio>=0.19.0 pytest-cov>=2.5.1 typing-extensions>=4.2.0 diff --git a/tests/bleak/backends/bluezdbus/__init__.py b/tests/bleak/backends/bluezdbus/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/bleak/backends/bluezdbus/test_version.py b/tests/bleak/backends/bluezdbus/test_version.py new file mode 100644 index 000000000..8c1ab72ea --- /dev/null +++ b/tests/bleak/backends/bluezdbus/test_version.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python + +"""Tests for `bleak.backends.bluezdbus.version` package.""" + +import sys +from unittest.mock import Mock, patch + +import pytest + +if sys.version_info[:2] < (3, 8): + from asynctest.mock import CoroutineMock as AsyncMock +else: + from unittest.mock import AsyncMock + +from bleak.backends.bluezdbus.version import BlueZFeatures + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "version,can_write_without_response,write_without_response_workaround_needed,hides_battery_characteristic,hides_device_name_characteristic", + [ + (b"bluetoothctl: 5.34", False, False, False, False), + (b"bluetoothctl: 5.46", True, False, False, False), + (b"bluetoothctl: 5.48", True, False, True, True), + (b"bluetoothctl: 5.51", True, True, True, True), + (b"bluetoothctl: 5.63", True, True, True, True), + (b"", True, True, True, True), + ], +) +async def test_bluez_version( + version, + can_write_without_response, + write_without_response_workaround_needed, + hides_battery_characteristic, + hides_device_name_characteristic, +): + """Test we can determine supported feature from bluetoothctl.""" + mock_proc = Mock( + wait=AsyncMock(), stdout=Mock(read=AsyncMock(return_value=version)) + ) + with patch( + "bleak.backends.bluezdbus.version.asyncio.create_subprocess_exec", + AsyncMock(return_value=mock_proc), + ): + BlueZFeatures._check_bluez_event = None + await BlueZFeatures.check_bluez_version() + assert BlueZFeatures.checked_bluez_version is True + assert BlueZFeatures.can_write_without_response == can_write_without_response + assert ( + not BlueZFeatures.write_without_response_workaround_needed + == write_without_response_workaround_needed + ) + assert BlueZFeatures.hides_battery_characteristic == hides_battery_characteristic + assert ( + BlueZFeatures.hides_device_name_characteristic + == hides_device_name_characteristic + ) + + +@pytest.mark.asyncio +async def test_bluez_version_only_happens_once(): + """Test we can determine supported feature from bluetoothctl.""" + BlueZFeatures.checked_bluez_version = False + BlueZFeatures._check_bluez_event = None + mock_proc = Mock( + wait=AsyncMock(), + stdout=Mock(read=AsyncMock(return_value=b"bluetoothctl: 5.46")), + ) + with patch( + "bleak.backends.bluezdbus.version.asyncio.create_subprocess_exec", + AsyncMock(return_value=mock_proc), + ): + await BlueZFeatures.check_bluez_version() + + assert BlueZFeatures.checked_bluez_version is True + + with patch( + "bleak.backends.bluezdbus.version.asyncio.create_subprocess_exec", + side_effect=Exception, + ): + await BlueZFeatures.check_bluez_version() + + assert BlueZFeatures.checked_bluez_version is True + + +@pytest.mark.asyncio +async def test_exception_checking_bluez_features_does_not_block_forever(): + """Test an exception while checking BlueZ features does not stall a second check.""" + BlueZFeatures.checked_bluez_version = False + BlueZFeatures._check_bluez_event = None + with patch( + "bleak.backends.bluezdbus.version.asyncio.create_subprocess_exec", + side_effect=OSError, + ): + await BlueZFeatures.check_bluez_version() + + assert BlueZFeatures.checked_bluez_version is True + + with patch( + "bleak.backends.bluezdbus.version.asyncio.create_subprocess_exec", + side_effect=OSError, + ): + await BlueZFeatures.check_bluez_version() + + assert BlueZFeatures.checked_bluez_version is True