Added support for Google´s Bumble Bluetooth Controller stack #1681

vChavezB · 2024-11-01T19:40:08Z

The backend supports direct use with Bumble. The HCI Controller is managed by the Bumble stack and the transport layer can be defined by the user (e.g. VHCI, Serial, TCP, android-netsim).

Use cases for this backend are:

Bluetooth Functional tests without Hardware. Example of Bluetooth stacks that
support virtualization are Android Emulator and Zephyr RTOS.
Virtual connection between different HCI Controllers that are not in the same
radio network (virtual or physical).

To enable this backend the env. variable BLEAK_BUMBLE must be set.

At the moment this is an experimental backend and would like to get some feedback from users and the devs of bleak :)

vChavezB · 2024-11-01T22:44:35Z

Fixed some linting and doc issues.

JPHutchins · 2024-11-01T22:47:31Z

It looks like you may not have updated the lock file. Please run poetry lock --no-update, commit, and push. thanks!

JPHutchins

Looks amazing!

Starting with a first round of type safety. While Bleak does not have 100% type coverage, I won't support any new python - in any context - that does not have 100% type coverage. That's just my opinion, and I'm happy to PR for it if it's too much of a lift.

I'll stay tuned to this and test it out on HW ASAP!

JPHutchins · 2024-11-01T22:45:57Z

bleak/backends/bumble/__init__.py

+def get_default_transport():
+    return "tcp-server:_:1234"


Can it be:

def get_default_transport() -> str: return "tcp-server:_:1234"

or better:

def get_default_transport() -> Literal["tcp-server:_:1234"]: return "tcp-server:_:1234"

Why does it need a function?

JPHutchins · 2024-11-01T22:50:13Z

bleak/backends/bumble/__init__.py

+from bumble.link import LocalLink
+from bumble.transport import open_transport
+
+transports = {}


What are these? Bumble has poor type safety, but it seems we can get some of it:

async def open_transport(name: str) -> Transport: """ Open a transport by name. The name must be <type>:<metadata><parameters> Where <parameters> depend on the type (and may be empty for some types), and <metadata> is either omitted, or a ,-separated list of <key>=<value> pairs, enclosed in []. If there are not metadata or parameter, the : after the <type> may be omitted. Examples: * usb:0 * usb:[driver=rtk]0 * android-netsim The supported types are: * serial * udp * tcp-client * tcp-server * ws-client * ws-server * pty * file * vhci * hci-socket * usb * pyusb * android-emulator * android-netsim """

Likely this API should not use strings. Aren't these really enums?

Like why are these serialized to strings? OMG, it parses them:

scheme, *tail = name.split(':', 1) spec = tail[0] if tail else None metadata = None if spec: # Metadata may precede the spec if spec.startswith('['): metadata_str, *tail = spec[1:].split(']') spec = tail[0] if tail else None metadata = dict([entry.split('=') for entry in metadata_str.split(',')])

🤦‍♀️

When Guido left Google he should have taken Python with him...

I think bumble uses directly strings. See here.

Actually there are hidden imports here which is going to be a problem for compiling binaries... lazy imports are nice until they aren't.

I think bumble uses directly strings. See here.

Agreed - that API is bumming me out. It should deserialize to some well defined types and then pass those to open the transport. I see it was done this way to allow user input from the command line into this function.

Looking into this more, I don't seen any reason to use the open_transport function. Possibly can do away with the stuff in the __init__.py file entirely. Can the user be responsible to import the transport from Bumble and inject it themselves?

I checked the Transports from bumble and I am not sure its trivial. The implementation specific transports are defined locally inside the function that opens each transport.

Example for the TCP Server

https://github.com/google/bumble/blob/b78f8951430e8b4c0f9ef14818666dbede97b009/bumble/transport/tcp_server.py#L65

This requires a PR to the bumble project to allow users to access the Transport implementations or copy paste all transports and make them accessible while its not possible directly from the bumble project.

What I could do in the meantime is add enums to the strings of the transport with argument adapter and a separate argument string for the specific parameters that the transport needs.

JPHutchins · 2024-11-01T22:51:44Z

bleak/backends/bumble/__init__.py

+from bumble.transport import open_transport
+
+transports = {}
+link = LocalLink()


I don't know if these mutable objects are intended to export to the entire application. If they must be at file scope, then some safety can be added, for example:

_link: Final = LocalLink()

JPHutchins · 2024-11-01T22:53:12Z

bleak/backends/bumble/__init__.py

+def get_link():
+    # Assume all transports are linked
+    return link


def get_link() -> LocalLink: # Assume all transports are linked return link

I think I see that these getters are intended to protect the global object.

JPHutchins · 2024-11-01T22:54:55Z

bleak/backends/bumble/__init__.py

+async def start_transport(transport: str):
+    if transport not in transports.keys():
+        transports[transport] = await open_transport(transport)
+        Controller(
+            "ext",
+            host_source=transports[transport].source,
+            host_sink=transports[transport].sink,
+            link=link,
+        )
+
+    return transports[transport]


async def start_transport(transport: str) -> Transport:

JPHutchins · 2024-11-01T23:03:02Z

bleak/backends/bumble/client.py

+            return self.services
+
+        new_services = BleakGATTServiceCollection()
+        self._peer = Peer(self._connection)


This is a type error; self._peer already has static type None - is it something OR nothing or is it always guaranteed to be something? Should it be a mutable variable that is global to the class or should it be an immutable variable local to the function it's used in?

JPHutchins · 2024-11-01T23:04:04Z

bleak/backends/bumble/client.py

+
+        """
+        if not isinstance(char_specifier, BleakGATTCharacteristic):
+            characteristic = self.services.get_characteristic(char_specifier)


Type error - services can be None.

JPHutchins · 2024-11-01T23:04:15Z

bleak/backends/bumble/client.py

+            (bytearray) The read data.
+
+        """
+        descriptor = self.services.get_descriptor(handle)


Type error; services can be None

JPHutchins · 2024-11-01T23:04:27Z

bleak/backends/bumble/client.py

+        """
+        descriptor = self.services.get_descriptor(handle)
+        if not descriptor:
+            raise BleakError("Descriptor {} was not found!".format(handle))


Prefer f strings

JPHutchins · 2024-11-01T23:04:39Z

bleak/backends/bumble/client.py

+        if not descriptor:
+            raise BleakError("Descriptor {} was not found!".format(handle))
+        val = await descriptor.obj.read_value()
+        logger.debug("Read Descriptor {0} : {1}".format(handle, val))


Prefer f-strings

JPHutchins · 2024-11-01T23:15:42Z

Re: the overall API. If this is merged, I think that it should be importable as a separate class. BleakClient can remain as an OS-selected class, but BumbleClient should be separately importable. BumbleClient would be fully compatible with BleakClient - that is, they implement the same interface, Protocol, ABC, whatever you want to call it.

vChavezB · 2024-11-01T23:37:33Z

I agree, the Bumble client is largely OS independent (except for the transports that use the Linux HCI device). So it could be imported separately.

Thanks for the feedback on type safety! It really helps to remove ambiguity in Python and help other developers understand the intent of the code. I will apply the suggestions you mentioned.

JPHutchins · 2024-11-01T23:40:55Z

I agree, the Bumble client is largely OS independent (except for the transports that use the Linux HCI device). So it could be imported separately.

Thanks for the feedback on type safety! It really helps to remove ambiguity in Python and help other developers understand the intent of the code. I will apply the suggestions you mentioned.

Glad you like it! I was annoyed when I first saw it being required in projects but now I'm a "kool-aid drinker" 🤣! It does in fact eliminate an entire class of runtime bugs that Python applications tend to suffer from.

You'll need mypy or pyright LSPs/extensions running to really see anything. By default, if a function has no return type specified, the linters ignore the typing in the function body. Best to simply specify a return type for all functions, even if it is None.

vChavezB · 2024-11-02T11:45:20Z

I have applied some of the suggestions from @JPHutchins. I will also check the typesafety of the changes in this PR.

dlech

This looks really nice!

I think it will be useful for people who have devices that don't work so well with the various OS Bluetooth stacks. And it could be useful for providing a test backend for anyone that needs some serious testing.

I think that it should be importable as a separate class.

I disagree with this suggestion. I would like to be able to run any script with or without the Bumble backend, so no code changes should be required to use the Bumble backend. I also don't want a bunch of features being added that aren't supported on other backends, so having a separate class should not be necessary.

docs/backends/android.rst

dlech · 2024-11-03T22:34:58Z

bleak/backends/bumble/characteristic.py

+from bleak.backends.descriptor import BleakGATTDescriptor
+
+
+class BleakGATTCharacteristicBumble(BleakGATTCharacteristic):


I've been hoping we could get rid of the subclasses of BleakGATTService, BleakGATTCharacteristic and BleakGATTDescriptor and just use that class directly everywhere since the subclasses are 90% duplicate code.

Not sure if that is something that is feasible to do before this PR or not.

Currently I put this PR in Draft to get some feedback. If you plan to refactor the classes I am open to rebase my fork and change the backend.

bleak/backends/bumble/client.py

dlech · 2024-11-03T22:39:58Z

bleak/backends/bumble/client.py

+        await self._dev.power_on()
+        await self._dev.connect(self.address)
+
+        self.services: BleakGATTServiceCollection = await self.get_services()


If getting services fails, we should disconnect.

What do you mean by disconnect?

should I do wrap it around a try statement ?

or do you mean if the BLE device does not have any services we should disconnect?

I did not find any similar logic in the backends that refer to check if the service fails. The only one is the WinRT where this is raised and is specific for Windows:

bleak/bleak/backends/winrt/client.py

Line 796 in e01e264

raise

From bumble I always get a List of services, if not, then its empty and thus will have an empty BleakGATTServiceCollection

dlech · 2024-11-03T22:41:38Z

bleak/backends/bumble/client.py

+
+        """
+        await start_transport(self._adapter)
+        await self._dev.power_on()


Hmm... power isn't something Bleak has had to deal with before. Since up to now, we have only used OS APIs, it has been up to the OS to control power.

Likely this should be moved to a new adapter class. See #1060

This is because Bumble does not interact with the OS. It has its own HCI stack, so I have to "turn on" the Bumble BLE device, which internally resets the bumble HCI controller.

https://github.com/google/bumble/blob/b78f8951430e8b4c0f9ef14818666dbede97b009/bumble/device.py#L2199

If there is a BleakAdapter proposed I am glad to use it for this PR

bleak/backends/bumble/utils.py

dlech · 2024-11-03T23:08:25Z

docs/backends/bumble.rst

+
+.. note::
+
+    To select this backend set the environmental variable ``BLEAK_BUMBLE``.


The environment variable should contain the transport info.

The idea is that users should just have to set the environment variable and use any Bleak script with the Bumble backend without having to modify any code.

My idea was that I could use bumble for functional testing with a virtual link. The user can then modify their code to select the appropiate transport.

Also what happens if the user wants to use both bumble and their physical OS HCI Controller.

Example:

Native HCI from Linux : hci0

HCI Serial controller that does not work well with Linux : bumble over serial

I opted for the bumble backend with an env. variable for tests only. it would be good to exactly define the use cases of bumble for bleak.

Do we accept bumble as a virtual transport to extend the limitations of OS drivers and hardware issues?

Should bumble cohabitate with the backend of each native OS ?

Use case for this is when you still want to use the native hardware from your OS and as mentioned a serial/USB controller.

Should bumble be flexible to use multiple transports?

This allows for example to connect an application running over Android simulator, a VHCI (linux), a serial HCI Controller and real hardware on the same network. This would for example enhance functional cross-platform testing of applications.

I will add an env. variable to define use of bumble + transport options. In addition, I will just mention in the docs that if a specific backend must be selected then it can be changed over the BleakClient argument backend 👍

dlech · 2024-11-03T23:12:18Z

docs/backends/bumble.rst

+The backend enables support of multiple |bumble_transport| to communicate with
+a physical or virtual HCI controller.
+
+Use cases for this backend are:


I think the serial/USB use case is worth mentioning here too. (Probably as number 1)

Do you mean using an HCI controller that communicates over serial/USB?

Just some context, for me the number 1 use case is to virtually test Zephyr OS firmware applications with bleak. I have an application that uses Bleak and connects to a real Zephyr BLE Peripheral Device. I wanted to use bumble as backend to compile my Zephyr application for linux and use the virtual TCP HCI channel. This allows me to verify that both my firmware and bleak application work without hardware.

Should I write the use case you mention as:

Use of a serial/USB HCI Controller that is not supported natively by an OS

or is there something else particular for this use case?

dlech · 2024-11-03T23:15:59Z

pyproject.toml

@@ -38,6 +38,7 @@ bleak-winrt = { version = "^1.2.0", markers = "platform_system=='Windows'", pyth
 "winrt-Windows.Foundation.Collections" = { version = "^2", markers = "platform_system=='Windows'", python = ">=3.12" }
 "winrt-Windows.Storage.Streams" = { version = "^2", markers = "platform_system=='Windows'", python = ">=3.12" }
 dbus-fast = { version = ">=1.83.0, < 3", markers = "platform_system == 'Linux'" }
+bumble = "^0.0.201"


I think this should be an optional dependency (i.e. only installed if pip install bleak[bumble]). Otherwise it is going to bring in a lot of other dependencies that most people don't need.

yes, seems fine by me. Then I would add this in the docs for the backend on how to install.

tests/bleak/backends/bumble/test_scanner.py

vChavezB · 2024-11-04T12:46:19Z

Resolved some of the reviews pending is:

Wait for Answer to Added support for Google´s Bumble Bluetooth Controller stack #1681 (comment)
Wait for PR for Bleak adapter ? add BleakAdapter class #1060
Wait for PR (?) to get Rid of BleakGattChar, BleakGatt Svc , see Added support for Google´s Bumble Bluetooth Controller stack #1681 (comment)
Wait for comment on detailed description of USB/Serial use case. See Added support for Google´s Bumble Bluetooth Controller stack #1681 (comment)
~~Define exactly expected use cases for bumble Added support for Google´s Bumble Bluetooth Controller stack #1681 (comment)~~

JPHutchins · 2024-11-04T20:03:37Z

I disagree with this suggestion. I would like to be able to run any script with or without the Bumble backend, so no code changes should be required to use the Bumble backend. I also don't want a bunch of features being added that aren't supported on other backends, so having a separate class should not be necessary.

I think that this works for me. My application can simply specify the Bumble backend, right?

bleak/bleak/__init__.py

Line 517 in e01e264

backend: Optional[Type[BaseBleakClient]] = None,

Obviously I cannot ask users to be setting environment variables 🤣.

Note to self: investigate import times due to use of runtime vs compile time OS checks:

bleak/bleak/backends/client.py

Lines 250 to 274 in e01e264

    
           def get_platform_client_backend_type() -> Type[BaseBleakClient]: 
        
               """ 
        
               Gets the platform-specific :class:`BaseBleakClient` type. 
        
               """ 
        
               if os.environ.get("P4A_BOOTSTRAP") is not None: 
        
                   from bleak.backends.p4android.client import BleakClientP4Android 
        
                   return BleakClientP4Android 
        
               if platform.system() == "Linux": 
        
                   from bleak.backends.bluezdbus.client import BleakClientBlueZDBus 
        
                   return BleakClientBlueZDBus 
        
               if platform.system() == "Darwin": 
        
                   from bleak.backends.corebluetooth.client import BleakClientCoreBluetooth 
        
                   return BleakClientCoreBluetooth 
        
               if platform.system() == "Windows": 
        
                   from bleak.backends.winrt.client import BleakClientWinRT 
        
                   return BleakClientWinRT 
        
               raise BleakError(f"Unsupported platform: {platform.system()}")

dlech · 2024-11-04T20:28:29Z

My application can simply specify the Bumble backend, right?

Yes, if you have an application that requires the Bumble backend and doesn't work with other backend, that is fine.

Obviously I cannot ask users to be setting environment variables 🤣.

Of course! But your application could set the environment variable itself after it starts too. But the backend arg seems like the more straightforward way to do it.

vChavezB · 2024-11-04T20:37:29Z

Well, in this case, if for bleak we want an easy way for users to replace the backend with bumble, then a variable like BLEAK_BUMBLE_TRANSPORT can be defined (or whatever name is more convenient).

And then if we don't want the env variable, then the user can pass the backend option 👍

I can then mention these two options in the docs.

JPHutchins · 2024-11-04T21:06:22Z

bleak/backends/bumble/__init__.py

+async def start_transport(transport: BumbleTransport) -> None:
+    transport_cmd = str(transport)
+    if transport_cmd not in transports.keys():
+        transports[transport_cmd] = await open_transport(transport_cmd)


I appreciate all the work done to make this less error-prone; but I'd like to understand why the user cannot simply import their "open transport function" - like open_serial_transport, open_udp_transport, etc.

It's taking the burden off of Bleak to maintain. Because the function is awaitable and has side effects, it would likely be passed as a partial or lambda (a closure):

async def start_transport( open_transport_func: partial[Coroutine[None, None, Transport]] ) -> Transport: await open_transport_func() ... my_transport = partial(open_serial_transport, "my serial transport args") ... await start_transport(my_transport)

The specific transport functions are still poorly typed with serialized strings as arguments, but at least now they could change upstream without Bleak having any responsibility to maintain them.

I am using the open_transport instead of specific open_transport_xyz because all the bumble examples and applications use this function. So I assumed this was the official Bumble way 🐝 .

Couldnt we map the possible transports functions to an Enum, then the user does not need to pass a partial or callback but just an enum plus string arguments.

Yes, that would be cool, but still requires maintenance here when upstream changes. The closure is perhaps confusing, and an alternative would be that start_transport takes the open_transport_func and spec as args. If one of the open transport functions upstream changes its number of args, this could be a problem, but it feels like they are using serialized strings with the goal of the function signature being general.

The backend supports direct use with Bumble. The HCI Controller is managed by the Bumble stack and the transport layer can be defined by the user (e.g. VHCI, Serial, TCP, android-netsim).

vChavezB marked this pull request as draft November 1, 2024 19:40

vChavezB force-pushed the bumble branch 11 times, most recently from b76d7f3 to 62411ce Compare November 1, 2024 22:41

vChavezB force-pushed the bumble branch from 62411ce to 6c42c4d Compare November 1, 2024 22:57

JPHutchins reviewed Nov 1, 2024

View reviewed changes

vChavezB force-pushed the bumble branch 2 times, most recently from dd907d4 to 7cf2739 Compare November 2, 2024 11:38

vChavezB force-pushed the bumble branch 5 times, most recently from 7a1fc78 to d49fe6f Compare November 2, 2024 22:16

dlech reviewed Nov 3, 2024

View reviewed changes

vChavezB force-pushed the bumble branch from d49fe6f to 31b1df6 Compare November 4, 2024 12:25

vChavezB force-pushed the bumble branch from 31b1df6 to 6274de4 Compare November 4, 2024 12:50

JPHutchins reviewed Nov 4, 2024

View reviewed changes

Added support for Google´s Bumble Bluetooth Controller stack

daf9234

The backend supports direct use with Bumble. The HCI Controller is managed by the Bumble stack and the transport layer can be defined by the user (e.g. VHCI, Serial, TCP, android-netsim).

vChavezB force-pushed the bumble branch from 6274de4 to daf9234 Compare November 4, 2024 22:02

		from bleak.backends.descriptor import BleakGATTDescriptor


		class BleakGATTCharacteristicBumble(BleakGATTCharacteristic):


		.. note::

		To select this backend set the environmental variable ``BLEAK_BUMBLE``.

Added support for Google´s Bumble Bluetooth Controller stack #1681

Are you sure you want to change the base?

Added support for Google´s Bumble Bluetooth Controller stack #1681

Conversation

vChavezB commented Nov 1, 2024 • edited Loading

vChavezB commented Nov 1, 2024

JPHutchins commented Nov 1, 2024

JPHutchins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPHutchins Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPHutchins commented Nov 1, 2024

vChavezB commented Nov 1, 2024

JPHutchins commented Nov 1, 2024 • edited Loading

vChavezB commented Nov 2, 2024

dlech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB commented Nov 4, 2024 • edited Loading

JPHutchins commented Nov 4, 2024

dlech commented Nov 4, 2024

vChavezB commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vChavezB commented Nov 1, 2024 •

edited

Loading

JPHutchins Nov 1, 2024 •

edited

Loading

vChavezB Nov 2, 2024 •

edited

Loading

JPHutchins commented Nov 1, 2024 •

edited

Loading

vChavezB Nov 4, 2024 •

edited

Loading

vChavezB Nov 4, 2024 •

edited

Loading

vChavezB Nov 4, 2024 •

edited

Loading

vChavezB Nov 4, 2024 •

edited

Loading

vChavezB commented Nov 4, 2024 •

edited

Loading