ttu / ruuvitag-sensor

Python package for communicating with RuuviTag BLE Sensor and for decoding sensor data from broadcasted data
https://ttu.github.io/ruuvitag-sensor/
MIT License
196 stars 78 forks source link

fix: pass Bluetooth device info to BleakScanner #238

Closed terop closed 8 months ago

terop commented 8 months ago

This enables the use of non-default Bluetooth device use with the Bleak communication back-end.

Fixes issue https://github.com/ttu/ruuvitag-sensor/issues/237.

terop commented 8 months ago

Python 3.7 pipeline does not want to pass, see https://github.com/ttu/ruuvitag-sensor/actions/runs/7906642579/job/21581949199?pr=238. @ttu what should be done to make it pass?

ttu commented 8 months ago

Python 3.7 & black causing problems again... Maybe it is time to drop the support for 3.7 in the next major release.

❯ black --version
black, 24.2.0 (compiled: yes)
Python (CPython) 3.11.5

❯ black .
reformatted /Users/ttu/src/github/ruuvitag-sensor/ruuvitag_sensor/ruuvi.py

All done! ✨ 🍰 ✨
1 file reformatted, 42 files left unchanged.
❯ black --version
black, 23.3.0 (compiled: no)
Python (CPython) 3.7.17

❯ black .
All done! ✨ 🍰 ✨
43 files left unchanged.

I tried to add to ´pyproject.toml`, but didn't help

[tool.black]
target_version = ['py37', 'py38']

I will try to check some proper solution for this or if can't come up with anything I will just disable black for py3.7.

ttu commented 8 months ago

Black has dropped support for py37 a while ago. Black is now locked to last supported version in #240

terop commented 8 months ago

Thanks Tomi! Could you check the comment I put?

terop commented 8 months ago

Python 3.7 & black causing problems again... Maybe it is time to drop the support for 3.7 in the next major release.

❯ black --version
black, 24.2.0 (compiled: yes)
Python (CPython) 3.11.5

❯ black .
reformatted /Users/ttu/src/github/ruuvitag-sensor/ruuvitag_sensor/ruuvi.py

All done! ✨ 🍰 ✨
1 file reformatted, 42 files left unchanged.
❯ black --version
black, 23.3.0 (compiled: no)
Python (CPython) 3.7.17

❯ black .
All done! ✨ 🍰 ✨
43 files left unchanged.

I tried to add to ´pyproject.toml`, but didn't help

[tool.black]
target_version = ['py37', 'py38']

I will try to check some proper solution for this or if can't come up with anything I will just disable black for py3.7.

I agree with dropping Python 3.7 as it is has already dropped from official Python support. Also all major distributions, including Debian, have long since shipped with a version newer than 3.7.

ttu commented 8 months ago

Locally I got some mymy errors. No idea why I got those errors locally, but on CI everything passed fine.

❯ mypy ./ruuvitag_sensor
ruuvitag_sensor/adapters/development/dev_bleak_scanner.py:30: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
ruuvitag_sensor/adapters/development/dev_bleak_scanner.py:31: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument "scanning_mode" to "BleakScanner" has incompatible type "str"; expected "Literal['active', 'passive']"  [arg-type]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument 3 to "BleakScanner" has incompatible type "**Dict[str, str]"; expected "Optional[List[str]]"  [arg-type]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument 3 to "BleakScanner" has incompatible type "**Dict[str, str]"; expected "BlueZScannerArgs"  [arg-type]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument 3 to "BleakScanner" has incompatible type "**Dict[str, str]"; expected "CBScannerArgs"  [arg-type]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument 3 to "BleakScanner" has incompatible type "**Dict[str, str]"; expected "Optional[Type[BaseBleakScanner]]"  [arg-type]
Found 5 errors in 1 file (checked 18 source files)

I added some typing to _get_scanner everything passed then fine:

from typing import AsyncGenerator, List, Literal, Tuple

from bleak import BleakScanner
from bleak.backends.scanner import AdvertisementData, AdvertisementDataCallback, BLEDevice

from ruuvitag_sensor.adapters import BleCommunicationAsync
from ruuvitag_sensor.adapters.utils import rssi_to_hex
from ruuvitag_sensor.ruuvi_types import MacAndRawData, RawData

MAC_REGEX = "[0-9a-f]{2}([:])[0-9a-f]{2}(\\1[0-9a-f]{2}){4}$"

def _get_scanner(detection_callback: AdvertisementDataCallback, bt_device: str = ""):
    # NOTE: On Linux - bleak.exc.BleakError: passive scanning mode requires bluez or_patterns
    # NOTE: On macOS - bleak.exc.BleakError: macOS does not support passive scanning
    scanning_mode: Literal["active", "passive"] = "passive" if sys.platform.startswith("win") else "active"

    if "bleak_dev" in os.environ.get("RUUVI_BLE_ADAPTER", "").lower():
        # pylint: disable=import-outside-toplevel
        from ruuvitag_sensor.adapters.development.dev_bleak_scanner import DevBleakScanner

        return DevBleakScanner(detection_callback, scanning_mode)

    additional_arguments = {"adapter": bt_device} if bt_device else {}
    return BleakScanner(detection_callback=detection_callback, scanning_mode=scanning_mode, kwargs=additional_arguments)

AdvertisementDataCallback was not actually required but added it anyway. Couldn't test this on Linux, so does this still work on your setup? If it works, you could add those type infos to _get_scanner.

terop commented 8 months ago

This is quite interesting as now I get the same mypy errors with the code that is in this PR. I cannot understand why this error did not appear to me earlier nor why it is not seen in CI.

Unfortunately the code (return BleakScanner(detection_callback=detection_callback, scanning_mode=scanning_mode, kwargs=additional_arguments)) does not work because the last argument to BleakScanner constructor is a kwargs argument which must be called with the unpacking operator (**) to work. Thus code you are suggesting cannot be be used. However what does work is

return BleakScanner(detection_callback=detection_callback, scanning_mode=scanning_mode,
                    adapter=bt_device if bt_device else "hci0")

but here we have a problem with what to pass as default value which would work on all platforms as Bluetooth device names most likely are not the same on all platforms. Considering this it might be easiest to go back to the original code with two different calls to the BleakScanner constructor depending on the value of the bt_device parameter.

ttu commented 8 months ago

Yes, you are correct, I didn't think this through as we do not want to pass argument named kwargs, but adapter etc.

From the BleakScanner solution it doesn't seem that windows and macOS solutions are using adapter argument, but better to take the safe route and go with your initial solution with 2 different constructor calls as you suggested.

terop commented 8 months ago

Changed back to the original code but now Pylint on Python 3.7 is not happy.

terop commented 8 months ago

I got Pylint to pass on 3.7 by removing the Literal["active", "passive"] part from line 21 in bleak_ble.py. @ttu Is this an acceptable solution or should something else be done?

ttu commented 8 months ago

Sorry for delay, busy school vacation week.

I still go few mypy errors locally. Again no idea why those don't appear during CI. Maybe you could add # type: ignore[arg-type] and then this is good to go to master 😄

❯ mypy ./ruuvitag_sensor
ruuvitag_sensor/adapters/development/dev_bleak_scanner.py:30: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
ruuvitag_sensor/adapters/development/dev_bleak_scanner.py:31: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
ruuvitag_sensor/adapters/bleak_ble.py:30: error: Argument "scanning_mode" to "BleakScanner" has incompatible type "str"; expected "Literal['active', 'passive']"  [arg-type]
ruuvitag_sensor/adapters/bleak_ble.py:32: error: Argument "scanning_mode" to "BleakScanner" has incompatible type "str"; expected "Literal['active', 'passive']"  [arg-type]
Found 2 errors in 1 file (checked 18 source files)
terop commented 8 months ago

Comments added.