Add support for pairing callbacks#1864
Conversation
dlech
left a comment
There was a problem hiding this comment.
Thanks for getting this moving again. I've made some preliminary comments. It will take me some time to go back and look at the previous discussions to refresh my memory on all of the details that have been looked at before. And I'll need some time to put something together for testing.
In the meantime, it looks like there is perhaps some preliminary refactoring that could be split out into a separate pull request to keep things moving.
| negotiate_unix_fd=True, | ||
| auth=get_dbus_authenticator(), | ||
| ).connect() | ||
| if not self._bus.connected: |
There was a problem hiding this comment.
By not creating a new MessageBus here, it changes the behavoir when when people disconnect and reconnect without stopping notifications first.
I'm not sure we can do this without breaking existing users.
There was a problem hiding this comment.
This comment has not been addressed. The implementation does not match the comment added at line 97 either.
|
@tgagneret-embedded Thanks for picking this back up. We have a lot of Home Assistant users who are really excited about having this feature. As soon as this gets going, I'll work on adding support to ESPHome as well. |
|
@tgagneret-embedded I opened a PR against your |
dff7640 to
f76bda0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1864 +/- ##
===========================================
+ Coverage 34.60% 34.83% +0.23%
===========================================
Files 37 39 +2
Lines 3849 3990 +141
Branches 476 484 +8
===========================================
+ Hits 1332 1390 +58
- Misses 2490 2568 +78
- Partials 27 32 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
I'm not sure why you changed https://github.com/tgagneret-embedded/bleak/pull/1/files#diff-aaae8a8eb8ab5923670dec887dfaf4d90aacacf551882aab9a51165920eae38d but I reverted it back since it does not work. Could you give me more detail to make sure it works correctly ? Thanks |
@dlech requested this change in this review comment: #1864 (comment) |
I understand, maybe the naming is not correct. In my case I get the complete Device1 and not only one property of Device1. |
|
@tgagneret-embedded I opened another PR based on your current |
|
Co-authored-by: Lennard Beers <l.beers@outlook.de>
484fbc4 to
9455b14
Compare
|
FYI, we now have some new test infrastructure that it should be possible to write tests and get full coverage for anything we add here (since this is BlueZ-only). You don't even need real Bluetooth hardware for this, so it should make it a lot easier now. But that does mean that I will be expecting tests to cover possibilities like Bleak initiating the pairing request, the peripheral initiating the pairing request, different paring types, etc. |
| member="Set", | ||
| signature="ssv", | ||
| body=[defs.DEVICE_INTERFACE, "Trusted", Variant("b", True)], | ||
| if callbacks is None: |
There was a problem hiding this comment.
Why does this depend on callbacks?
Maybe we should move this after the call to "Pair"?
|
|
||
| from dbus_fast import DBusError, Message | ||
| from dbus_fast.aio import MessageBus | ||
| from dbus_fast.service import ServiceInterface, method |
There was a problem hiding this comment.
| from dbus_fast.service import ServiceInterface, method | |
| from dbus_fast.service import ServiceInterface, method as dbus_method |
The API was recently renamed in dbus-fast. We can still import method so that we don't have to update the dependency just yet, but the rest of the code should use @dbus_method() so that we don't have to change that in the future.
|
|
||
| ble_device = await self._create_ble_device(device) | ||
|
|
||
| task = asyncio.create_task(self._callbacks.confirm_passkey(ble_device, passkey)) |
There was a problem hiding this comment.
Could use a comment explaining that we have to create a task so that it can be canceled by a different callback.
|
|
||
| if not result: | ||
| raise DBusError("org.bluez.Error.Rejected", "user rejected") | ||
| else: |
There was a problem hiding this comment.
Don't need else since if always raises.
| raise DBusError("org.bluez.Error.Rejected", "user rejected") | ||
| else: | ||
| manager = await get_global_bluez_manager() | ||
| # Set device as trusted. |
There was a problem hiding this comment.
This seems out of place here.
| ----- | ||
|
|
||
| This module contains types associated with the BlueZ D-Bus `agent api | ||
| <https://github.com/bluez/bluez/blob/master/doc/agent-api.txt>`. |
There was a problem hiding this comment.
| <https://github.com/bluez/bluez/blob/master/doc/agent-api.txt>`. | |
| <https://github.com/bluez/bluez/blob/master/doc/agent-api.txt>`_. |
I think that is the correct rst format, but I always forget and have to look it up.
| import contextlib | ||
| import logging | ||
| import os | ||
| from typing import Set, no_type_check |
There was a problem hiding this comment.
| from typing import Set, no_type_check | |
| from typing import no_type_check |
Can use set instead since we dropped support for Python < 3.9.
| @method() | ||
| @no_type_check | ||
| async def AuthorizeService(self, device: "o", uuid: "s"): # noqa: F821 N802 | ||
| logger.debug("AuthorizeService %s", device, uuid) |
There was a problem hiding this comment.
| logger.debug("AuthorizeService %s", device, uuid) | |
| logger.debug("AuthorizeService %s %s", device, uuid) |
| async def RequestPasskey(self, device: "o") -> "u": # noqa: F821 N802 | ||
| logger.debug("RequestPasskey %s", device) | ||
|
|
||
| ble_device = await self._create_ble_device(device) |
There was a problem hiding this comment.
Hmm... this is slightly dangerous because "Cancel" could be called if this actually awaits. In that case we would miss the cancellation.
Although the only time this should actually do that is if we lost the D-Bus connection, which means we have bigger problems anyway.
I suppose just adding a comment explaining this would suffice.
Alternative could be to pass manager to the constructor so that we have a reference to use without awaiting.
| negotiate_unix_fd=True, | ||
| auth=get_dbus_authenticator(), | ||
| ).connect() | ||
| if not self._bus.connected: |
There was a problem hiding this comment.
This comment has not been addressed. The implementation does not match the comment added at line 97 either.
| This module contains types associated with the BlueZ D-Bus `agent api | ||
| <https://github.com/bluez/bluez/blob/master/doc/agent-api.txt>`. | ||
| """ | ||
|
|
There was a problem hiding this comment.
| import sys | |
| from typing import TYPE_CHECKING | |
| from bleak.args.bluez import BlueZNotifyArgs | |
| if TYPE_CHECKING: | |
| if sys.platform != "linux": | |
| assert False, "This backend is only available on Linux" | |
Needed to avoid type checking failures on other platforms.
| ) | ||
| args = parser.parse_args() | ||
|
|
||
| asyncio.run(main(args.address, args.name, args.unpair, args.auto)) |
There was a problem hiding this comment.
| asyncio.run(main(args.address, args.name, args.unpair, args.auto)) | |
| asyncio.run(main(args)) |
Then we don't have to change it every time we add an arg.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser("pairing_agent.py") |
There was a problem hiding this comment.
| parser = argparse.ArgumentParser("pairing_agent.py") | |
| parser = argparse.ArgumentParser(namespace=Args()) |
Similar to other examples.
| if address: | ||
| await BleakClient(address).unpair() | ||
| elif name: | ||
| await BleakClient(name).unpair() |
There was a problem hiding this comment.
| if address: | |
| await BleakClient(address).unpair() | |
| elif name: | |
| await BleakClient(name).unpair() | |
| await BleakClient(address or name).unpair() |
| print(f"could not find device with name '{name}'") | ||
| return | ||
| else: | ||
| raise ValueError("Either --name or --address must be provided") |
There was a problem hiding this comment.
Could instead assert_never() here since add_mutually_exclusive_group() should already validate this.
| def __init__(self) -> None: | ||
| self.session: PromptSession[str] = PromptSession() | ||
|
|
||
| async def request_passkey(self, device: BLEDevice) -> str: |
There was a problem hiding this comment.
I like to use @override to show we are intentionally implementing a base class method.
| if auto: | ||
| print("connecting and pairing...") | ||
|
|
||
| async with BleakClient( |
There was a problem hiding this comment.
I would expect we would want to catch the same exceptions here as below.
Another thing we could do first (in a separate PR) is add some thorough tests for the existing pairing so that we can be sure we don't break anything that was already working with these changes. There are a couple of basic pairing tests in #1888, but we could split those out to a separate file and expand on that. |
|
I also have a feeling that it is going to be important to merge 9472a10 first to be able to handle a device disconnecting while attempting to pair. Otherwise, there are all sorts of For this I'm thinking of starting a |
|
@dlech any news about this feature? when will be merged? |
|
This PR needs more work before we can merge it. Now that we have integration tests, there is no reason to not have full coverage on this. There has been quite a bit of other activity in Bleak that has been keeping me busy, so someone else will need to do the work here or we can just keep waiting. |
Add bleak.pairing with the pairing API: a PairingCallbacks NamedTuple of optional async handlers, the IOCapability enum, and io_capability() which derives the advertised capability from which callbacks are provided. With no callbacks the result is NoInputNoOutput (Just Works). Each callback receives its arguments directly (the device, plus the passkey for Numeric Comparison and Passkey Entry); there are no wrapper request types. io_capability() treats a confirm handler as implying a display, so confirm combined with another handler upgrades the capability (e.g. confirm + display_passkey is DisplayYesNo). This is the foundation the BlueZ and WinRT pairing implementations build on. Supersedes the bleak.agent module from hbldh#1864 / hbldh#1990 (renamed to bleak.pairing; "agent" is a BlueZ-ism). Part of the pairing series; foundation for pairing_callbacks plumbing, the backend implementations, and the BleakClient argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the org.bluez.Agent1 implementation (agent.py) bound to a PairingCallbacks and registered for the duration of the Pair call via the bluez_agent async context manager; the advertised IO capability is derived from the provided callbacks. _pair wraps Pair in the agent and trusts the device only after a successful pairing (fixing the prior leave-Trusted-on- failure REVISIT), and pairing D-Bus errors map to BleakPairingCancelledError / BleakPairingFailedError via _assert_pairing_reply. The agent's D-Bus methods are typed with dbus_fast.annotations aliases instead of @no_type_check string-signature shims, so pin dbus-fast >=4.0.0,<5.0.0 (Annotated support; <5 avoids the v5 STRUCT marshaling change). tests/backends/bluezdbus/test_agent.py covers the agent dispatch, the no-callback error paths, and capability derivation. The ceremony integration tests land with the BleakClient argument in a later PR. Implements the BlueZ side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#1380 / hbldh#1434 / hbldh#758. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the org.bluez.Agent1 implementation (agent.py) bound to a PairingCallbacks and registered for the duration of the Pair call via the bluez_agent async context manager; the advertised IO capability is derived from the provided callbacks. _pair wraps Pair in the agent and trusts the device only after a successful pairing (fixing the prior leave-Trusted-on-failure REVISIT), and pairing D-Bus errors map to BleakPairingCancelledError / BleakPairingFailedError via _assert_pairing_reply. The agent's _run wrapper is generic over the callback return type and owns the ensure_future so a peer Cancel can abort any in-flight callback. The agent's D-Bus methods are typed with dbus_fast.annotations aliases instead of @no_type_check string-signature shims, so pin dbus-fast >=4.0.0,<5.0.0 (Annotated support; <5 avoids the v5 STRUCT marshaling change). tests/backends/bluezdbus/test_agent.py covers the agent dispatch, the no-callback error paths, and capability derivation. tests/integration/test_client_pairing.py adds the bumble ceremony tests (Just Works, Numeric Comparison, Passkey Entry both ways, unpair), runnable on BlueZ VHCI in CI and over a USB HCI controller. Implements the BlueZ side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#1380 / hbldh#1434 / hbldh#758. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rework BleakClientWinRT.pair() to take part in the negotiated pairing kind via a PairingCallbacks. _pairing_kinds() derives the accepted DevicePairingKinds from the callbacks; _accept_pairing() dispatches each PairingRequested event (Just Works / Numeric Comparison / Passkey Entry in both directions) with an exhaustive match, validating the entered passkey against MAX_PASSKEY and bridging the WinRT callback thread to the event loop via a deferral. pair() accepts an explicit callbacks argument and maps PAIRING_CANCELED / REJECTED_BY_HANDLER to BleakPairingCancelledError, other failures to BleakPairingFailedError. The PairingRequested handler's response task is tracked and cancelled when pair() unwinds, so a mid-ceremony timeout/cancel completes the deferral instead of orphaning it (addresses the lifecycle question raised on hbldh#1990). tests/backends/winrt/test_pairing.py covers _pairing_kinds derivation and _accept_pairing dispatch for every kind. Implements the WinRT side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#373 / hbldh#700 / hbldh#1880. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lock in the current BlueZ and WinRT pair()/unpair() contract before the pairing-agent series reworks it, so later refactors cannot silently regress today's behavior. - winrt: idempotent already-paired return, can_pair guard, Just Works success + handler detach, result-status failure, the deprecated protection_level keyword, and unpair status handling. - bluez: already-paired return, Trusted-then-Pair + resolvable-private- address refresh, D-Bus failure propagation (no agent is registered on develop, so a real Pair fails, cf. hbldh#1380 / hbldh#1434), and unpair RemoveDevice / DoesNotExist mapping. - integration: a connect/disconnect/unpair smoke test for the existing unpair path. Expands the integration harness from hbldh#1888. Addresses dlech's request on hbldh#1990 to split the work and add tests for existing pairing functions first. Part of the pairing series continuing hbldh#1864. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add bleak.pairing with the pairing API: a PairingCallbacks NamedTuple of optional async handlers, the IOCapability enum, and io_capability() which derives the advertised capability from which callbacks are provided. With no callbacks the result is NoInputNoOutput (Just Works). Each callback receives its arguments directly (the device, plus the passkey for Numeric Comparison and Passkey Entry); there are no wrapper request types. io_capability() treats a confirm handler as implying a display, so confirm combined with another handler upgrades the capability (e.g. confirm + display_passkey is DisplayYesNo). This is the foundation the BlueZ and WinRT pairing implementations build on. Supersedes the bleak.agent module from hbldh#1864 / hbldh#1990 (renamed to bleak.pairing; "agent" is a BlueZ-ism). Part of the pairing series; foundation for pairing_callbacks plumbing, the backend implementations, and the BleakClient argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the org.bluez.Agent1 implementation (agent.py) bound to a PairingCallbacks and registered for the duration of the Pair call via the bluez_agent async context manager; the advertised IO capability is derived from the provided callbacks. _pair wraps Pair in the agent and trusts the device only after a successful pairing (fixing the prior leave-Trusted-on-failure REVISIT), and pairing D-Bus errors map to BleakPairingCancelledError / BleakPairingFailedError via _assert_pairing_reply. The agent's _run wrapper is generic over the callback return type and owns the ensure_future so a peer Cancel can abort any in-flight callback. The agent's D-Bus methods are typed with dbus_fast.annotations aliases instead of @no_type_check string-signature shims, so pin dbus-fast >=4.0.0,<5.0.0 (Annotated support; <5 avoids the v5 STRUCT marshaling change). tests/backends/bluezdbus/test_agent.py covers the agent dispatch, the no-callback error paths, and capability derivation. tests/integration/test_client_pairing.py adds the bumble ceremony tests (Just Works, Numeric Comparison, Passkey Entry both ways, unpair), runnable on BlueZ VHCI in CI and over a USB HCI controller. Implements the BlueZ side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#1380 / hbldh#1434 / hbldh#758. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rework BleakClientWinRT.pair() to take part in the negotiated pairing kind via a PairingCallbacks. _pairing_kinds() derives the accepted DevicePairingKinds from the callbacks; _accept_pairing() dispatches each PairingRequested event (Just Works / Numeric Comparison / Passkey Entry in both directions) with an exhaustive match, validating the entered passkey against MAX_PASSKEY and bridging the WinRT callback thread to the event loop via a deferral. pair() accepts an explicit callbacks argument and maps PAIRING_CANCELED / REJECTED_BY_HANDLER to BleakPairingCancelledError, other failures to BleakPairingFailedError. The PairingRequested handler's response task is tracked and cancelled when pair() unwinds, so a mid-ceremony timeout/cancel completes the deferral instead of orphaning it (addresses the lifecycle question raised on hbldh#1990). tests/backends/winrt/test_pairing.py covers _pairing_kinds derivation and _accept_pairing dispatch for every kind. Implements the WinRT side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#373 / hbldh#700 / hbldh#1880. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the org.bluez.Agent1 implementation (agent.py) bound to a PairingCallbacks and registered for the duration of the Pair call via the bluez_agent async context manager; the advertised IO capability is derived from the provided callbacks. _pair wraps Pair in the agent and trusts the device only after a successful pairing (fixing the prior leave-Trusted-on-failure REVISIT), and pairing D-Bus errors map to BleakPairingCancelledError / BleakPairingFailedError via _assert_pairing_reply. The agent's _run wrapper is generic over the callback return type and owns the ensure_future so a peer Cancel can abort any in-flight callback. The agent's D-Bus methods are typed with dbus_fast.annotations aliases instead of @no_type_check string-signature shims, so pin dbus-fast >=4.0.0,<5.0.0 (Annotated support; <5 avoids the v5 STRUCT marshaling change). tests/backends/bluezdbus/test_agent.py covers the agent dispatch, the no-callback error paths, and capability derivation. tests/integration/test_client_pairing.py adds the bumble ceremony tests (Just Works, Numeric Comparison, Passkey Entry both ways, unpair), runnable on BlueZ VHCI in CI and over a USB HCI controller. Implements the BlueZ side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#1380 / hbldh#1434 / hbldh#758. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rework BleakClientWinRT.pair() to take part in the negotiated pairing kind via a PairingCallbacks. _pairing_kinds() derives the accepted DevicePairingKinds from the callbacks; _accept_pairing() dispatches each PairingRequested event (Just Works / Numeric Comparison / Passkey Entry in both directions) with an exhaustive match, validating the entered passkey against MAX_PASSKEY and bridging the WinRT callback thread to the event loop via a deferral. pair() accepts an explicit callbacks argument and maps PAIRING_CANCELED / REJECTED_BY_HANDLER to BleakPairingCancelledError, other failures to BleakPairingFailedError. The PairingRequested handler's response task is tracked and cancelled when pair() unwinds, so a mid-ceremony timeout/cancel completes the deferral instead of orphaning it (addresses the lifecycle question raised on hbldh#1990). tests/backends/winrt/test_pairing.py covers _pairing_kinds derivation and _accept_pairing dispatch for every kind. Implements the WinRT side of the pairing series (hbldh#1990); continues hbldh#1864 and advances hbldh#373 / hbldh#700 / hbldh#1880. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This is the foundation the BlueZ and WinRT pairing implementations build on. Supersedes the bleak.agent module from hbldh#1864 / hbldh#1990 (renamed to bleak.pairing; "agent" is a BlueZ-ism). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This is the foundation the BlueZ and WinRT pairing implementations build on. Supersedes the bleak.agent module from hbldh#1864 / hbldh#1990 (renamed to bleak.pairing; "agent" is a BlueZ-ism). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add support for pairing callbacks.
This is based on: