Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pass Bluetooth device info to BleakScanner #238

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

terop
Copy link
Contributor

@terop terop commented Feb 14, 2024

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

Fixes issue #237.

@terop terop force-pushed the pass_bt_device_to_bleak branch 2 times, most recently from 842c6e5 to 8b3385d Compare February 14, 2024 19:54
@terop
Copy link
Contributor Author

terop commented Feb 14, 2024

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
Copy link
Owner

ttu commented Feb 17, 2024

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
Copy link
Owner

ttu commented Feb 17, 2024

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

@terop terop force-pushed the pass_bt_device_to_bleak branch 2 times, most recently from bb47c62 to c38c677 Compare February 17, 2024 07:25
@terop
Copy link
Contributor Author

terop commented Feb 17, 2024

Thanks Tomi!
Could you check the comment I put?

@terop terop force-pushed the pass_bt_device_to_bleak branch from c38c677 to 8b955c1 Compare February 17, 2024 13:12
@terop
Copy link
Contributor Author

terop commented Feb 17, 2024

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
Copy link
Owner

ttu commented Feb 18, 2024

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
Copy link
Contributor Author

terop commented Feb 18, 2024

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
Copy link
Owner

ttu commented Feb 19, 2024

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 terop force-pushed the pass_bt_device_to_bleak branch from 8b955c1 to cdcdd03 Compare February 19, 2024 05:33
@terop
Copy link
Contributor Author

terop commented Feb 19, 2024

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

@terop terop force-pushed the pass_bt_device_to_bleak branch 2 times, most recently from bcfee11 to 8247a7d Compare February 19, 2024 18:35
@terop
Copy link
Contributor Author

terop commented Feb 19, 2024

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
Copy link
Owner

ttu commented Feb 24, 2024

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)

This enables the use of non-default Bluetooth device use with
the Bleak communication back-end.
@terop terop force-pushed the pass_bt_device_to_bleak branch from 8247a7d to 3588b6b Compare February 24, 2024 11:04
@terop
Copy link
Contributor Author

terop commented Feb 24, 2024

Comments added.

@ttu ttu merged commit e74eabf into ttu:master Feb 26, 2024
6 checks passed
@terop terop deleted the pass_bt_device_to_bleak branch February 26, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants