Adding mtu_size client #525

es617 · 2021-04-23T19:02:59Z

Adding a simple method to the macOS client to get the current ATT MTU size.
The ATT MTU size includes the ATT header, while CoreBluetooth returns the maximum ATT payload size, hence the +3 in the getMtuSize method.

dlech · 2021-04-28T14:39:54Z

Could you also add this function for the other backends?

es617 · 2021-04-28T21:33:54Z

@dlech I added support for the other three backends. A few notes:

The windows implementation seems pretty straightforward. The only question is whether the value returned is the ATT MTU size or the max ATT payload size. Reading the specs, it should be the ATT MTU size (including the header). Unfortunately, I haven't been able to test on Windows yet.

1b) If I get to test on Windows, how do I test the two different win backends?

The BlueZ implementation is not the greatest, but it's the only option available as far as I can see. It will only work if the server has a characteristic with "notify" or "write wo response" property.
I tried to keep the API the same among the different backends; in reality, only the BlueZ backend requires a "characteristic" to be passed. The other backends will just ignore that argument. Not sure what is the recommended approach for BLEAK in these situations. Please advise!

dlech · 2021-04-28T22:00:13Z

Thanks for the effort!

I think for Windows, right now the backend depends on the Python runtime version, so if you don't want to install different versions of Python, you probably just have to edit Bleak to be sure to load the one you want.

On BlueZ, I agree this is probably the best we can do. Instead of making the user have to provide a characteristic, would it be possible to go through all of the cached characteristics and chose the "best" one? I think the "best" one would be one where we can call AquireWrite (write without response property set) and fall back to AquireNotify if no characteristic is available for AquireWrite. (Devices could start sending notifications plus it will fail if notifications are already enabled, so it would be best to avoid AquireNotify.)

Also, we should close the file descriptor returned by these D-Bus methods, otherwise it will probably block other operations.

It would also be good to cache the returned MTU value on the BlueZ backend so that we only have to call the complex lookup once per connection.

dlech · 2021-04-28T22:08:11Z

Another idea:

Make this a property instead of an async method. On Windows and Mac, this property will just work. On Linux, this property will return an error unless the user first calls a special method to fetch and cache the MTU value. This way, the user has full control over which characteristic to use (and possibly a choice of using AquireWrite or AquireNotify). Then if BlueZ ever adds a proper MTU property, we won't be stuck with an unnecessary async method in Bleak.

es617 · 2021-04-30T19:20:09Z

Thanks for the feedback!

I still need to run some tests, but here for early review.

Now using a property for all the clients
BlueZ has default mtu size set to 23. After service discovery we try to get the actual MTU size. If that fails, we stick with 23. Not sure whether to return an error or lie with a smaller (conservative) MTU size. In any case, this behavior should be documented.

To be tested:

what happens when we call get_mtu_size prior to bonding and we have encrypted characteristics.
Windows clients

es617 · 2021-05-05T20:31:51Z

Pushed another round of changes.
Unfortunately, the "get mtu size" method can't be called right after connection. If the device is not bonded calling AcquireNotify or AcquireWrite on an encrypted characteristic causes the device to disconnect.

The only option we have is to let the user call the method manually (now called discover_mtu_size, to distinguish from "get"), passing the UUID of a characteristic and the preferred property to use for the discovery.

dlech

This looks good to me. Just one minor comment.

It would be nice to have an example program too, but we can save that for a separate PR.

dlech · 2021-05-05T20:41:24Z

bleak/backends/bluezdbus/client.py

+        elif char_property is "write-without-response":
+            member = "AcquireWrite"
+        else:
+            raise BleakError(


This should probably be ValueError rather than BleakError.

dlech · 2021-05-05T21:16:51Z

I also just noticed that we are setting negotiate_unix_fd based on the BlueZ version, so I'm wondering if trying to close the file descriptor will cause a problem when negotiate_unix_fd=False.

es617 · 2021-05-12T00:58:50Z

I also just noticed that we are setting negotiate_unix_fd based on the BlueZ version, so I'm wondering if trying to close the file descriptor will cause a problem when negotiate_unix_fd=False.

Good catch. I tried with negotiate_unix_fd=False and it gets stuck at the reply = await self._bus.call. I am not sure what is going on there. Any idea?

Also, things aren't looking great for negotiate_unix_fd=True as well. Turns out that calling client.start_notify after client.discover_mtu_size fails with bleak.exc.BleakDBusError: [org.bluez.Error.NotPermitted] Notify acquired. I suspect the os.close(fd) isn't working, and I noticed that the fd we get in discover_mtu_size has value 0, which it can't be right (?!). Any suggestion on this one?

If those can't be easily addressed, I'll probably revert the BlueZ changes, and go back to the simple MacOS and Windows only solution (still pending windows testing).

dlech · 2021-05-12T03:35:02Z

I would be happy with BlueZ just returning 23 for now along with logging a warning that it is not properly implemented.

Then we can look at fixing dbus-next and/or adding an mtu property to BlueZ to eventually get this working right.

es617 · 2021-05-13T17:20:45Z

thanks! I just pushed the simplified implementation for bluez. I'll test on windows and then this should be good to go

dlech

After making the suggested change (and merging the fix from #537), the winrt backend works.

dlech · 2021-05-13T18:27:17Z

bleak/backends/bluezdbus/client.py

@@ -570,6 +570,14 @@ def is_connected(self) -> bool:
            False if self._bus is None else self._properties.get("Connected", False)
        )

+    @property
+    def mtu_size(self) -> bool:


return type is int, not bool

dlech · 2021-05-13T18:27:45Z

bleak/backends/corebluetooth/client.py

@@ -125,6 +125,12 @@ def is_connected(self) -> bool:
            False if manager is None else manager.isConnected
        )

+    @property
+    def mtu_size(self) -> bool:


return type should be int

dlech · 2021-05-13T18:28:09Z

bleak/backends/dotnet/client.py

@@ -305,6 +305,11 @@ def is_connected(self) -> bool:
            else self._requester.ConnectionStatus == BluetoothConnectionStatus.Connected
        )

+    @property
+    def mtu_size(self) -> bool:


return type should be int

dlech · 2021-05-13T18:28:14Z

bleak/backends/winrt/client.py

@@ -280,6 +280,11 @@ def is_connected(self) -> bool:
            == BluetoothConnectionStatus.CONNECTED
        )

+    @property
+    def mtu_size(self) -> bool:


return type should be int

dlech · 2021-05-13T18:28:51Z

bleak/backends/winrt/client.py

+    @property
+    def mtu_size(self) -> bool:
+        """Get ATT MTU size for active connection"""
+        return self._session.MaxPduSize


the winrt API uses lower_snake_case, so max_pdu_size here

dlech · 2021-05-13T20:08:27Z

Hmm.... I'm playing with this on Windows more and I just got the wrong MTU (517 instead of 23 as expected for the device I'm using). So I guess that connect() returns before the OS has negotiated the MTU. Not sure what to do about that other than document it.

Windows has a MaxPduSizeChanged event but I'm not sure that I would trust it to always be called when a device is connected and to only be called once. If it did work this way, we could wait for the event when connecting.

dlech · 2021-05-13T20:34:06Z

After some more experimentation, it seems that the device isn't connecting properly or is becoming disconnected before I read the mtu, so I think we can ignore my previous comment.

UPDATE: Doh, I had another BLE device advertising that I didn't know was on. 🤦

es617 · 2021-05-14T00:42:48Z

thanks for the feedback and for testing on Windows!
Could you check if the max_pdu_size (or MaxPduSize) include the ATT payload (like returning 23, instead of 20)? It's not clear from the specs. Just to be sure we are consistent between the different backends.

dlech · 2021-05-14T00:59:22Z

I have confirmed that MaxPduSize does return the actual MTU.

es617 · 2021-05-14T13:46:12Z

thanks for the approve! I don't see a merge button ("Only those with write access to this repository can merge pull requests."), should someone else merge this?

dlech · 2021-05-14T14:27:43Z

merged, thanks!

Adding get_mtu_size to macOS client

90bc4dc

Enrico Santagati added 2 commits April 28, 2021 17:24

Adding win and bluez support

3cf9294

running black

d61a246

Enrico Santagati added 2 commits April 30, 2021 15:14

Using property instead of method for mtu size

357654e

run black

a58f717

es617 changed the title ~~Adding get_mtu_size to macOS client~~ Adding mtu_size client Apr 30, 2021

Enrico Santagati added 2 commits May 5, 2021 16:27

require manual discovery of MTU size on linux

b363ce4

update changelog

1bbfeb8

flake8 check

353b864

dlech reviewed May 5, 2021

View reviewed changes

pan- mentioned this pull request May 7, 2021

Add FOTA Service mock example ARMmbed/mbed-os-example-ble#370

Merged

simplified bluez implementation

793ff2d

dlech requested changes May 13, 2021

View reviewed changes

Addressing PR comments

f33d994

dlech approved these changes May 14, 2021

View reviewed changes

dlech merged commit 62f45b0 into hbldh:develop May 14, 2021

dlech mentioned this pull request May 19, 2021

Fixed write without response on BlueZ < 5.51. #542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding mtu_size client #525

Adding mtu_size client #525

es617 commented Apr 23, 2021

dlech commented Apr 28, 2021

es617 commented Apr 28, 2021

dlech commented Apr 28, 2021

dlech commented Apr 28, 2021

es617 commented Apr 30, 2021

es617 commented May 5, 2021

dlech left a comment

dlech May 5, 2021

dlech commented May 5, 2021

es617 commented May 12, 2021

dlech commented May 12, 2021

es617 commented May 13, 2021

dlech left a comment

dlech May 13, 2021

dlech May 13, 2021

dlech May 13, 2021

dlech May 13, 2021

dlech May 13, 2021

dlech commented May 13, 2021

dlech commented May 13, 2021 •

edited

Loading

es617 commented May 14, 2021

dlech commented May 14, 2021

es617 commented May 14, 2021

dlech commented May 14, 2021

Adding mtu_size client #525

Adding mtu_size client #525

Conversation

es617 commented Apr 23, 2021

dlech commented Apr 28, 2021

es617 commented Apr 28, 2021

dlech commented Apr 28, 2021

dlech commented Apr 28, 2021

es617 commented Apr 30, 2021

es617 commented May 5, 2021

dlech left a comment

Choose a reason for hiding this comment

dlech May 5, 2021

Choose a reason for hiding this comment

dlech commented May 5, 2021

es617 commented May 12, 2021

dlech commented May 12, 2021

es617 commented May 13, 2021

dlech left a comment

Choose a reason for hiding this comment

dlech May 13, 2021

Choose a reason for hiding this comment

dlech May 13, 2021

Choose a reason for hiding this comment

dlech May 13, 2021

Choose a reason for hiding this comment

dlech May 13, 2021

Choose a reason for hiding this comment

dlech May 13, 2021

Choose a reason for hiding this comment

dlech commented May 13, 2021

dlech commented May 13, 2021 • edited Loading

es617 commented May 14, 2021

dlech commented May 14, 2021

es617 commented May 14, 2021

dlech commented May 14, 2021

dlech commented May 13, 2021 •

edited

Loading