intercreate / smpclient

Simple Management Protocol (SMP) Client for remotely managing MCU firmware
Apache License 2.0
9 stars 6 forks source link

BLE Incorrect MTU #21

Closed tomaszduda23 closed 6 months ago

tomaszduda23 commented 6 months ago

I've tested version 2.0.1 a few more times. It looks that MTU is calculated incorrectly sometimes. The log after a few runs look like:

INFO smp_characteristic.max_write_without_response_size=20
INFO smp_characteristic.max_write_without_response_size=495

It seems that this should be still added

        # BlueZ doesn't have a proper way to get the MTU, so we have this hack.
        # If this doesn't work for you, you can set the client._mtu_size attribute
        # to override the value instead.
        if self._client._backend.__class__.__name__ == "BleakClientBlueZDBus":
            await self._client._backend._acquire_mtu()
JPHutchins commented 6 months ago

There is a Windows-specific workaround, but it looks like it may be required for Linux as well... either way, I'd like to hear more from Bleak about the "best way".

https://github.com/intercreate/smpclient/blob/0f47207c8d44b843a599d23ff7dd202dcdac25c2/smpclient/transport/ble.py#L73-L85

JPHutchins commented 6 months ago

Hmmm, BlueZ gets it here: https://github.com/hbldh/bleak/blob/b8149a5b5504eaacbd45ee56c1339fa4395e1345/bleak/backends/bluezdbus/manager.py#L697-L699

tomaszduda23 commented 6 months ago

Right now it defaults to 20 %50 of times. Calling this explicit works 100% time https://github.com/tomaszduda23/smpclient/blob/d25c8035ae2858fd41a106058297b619d58fbcb5/smpclient/transport/ble.py#L84 It seems that there is a race in bleak. Perhaps value is set only once and sometimes it happens before MTU negotiation is completed. Could you consider adding this code temporary? It is main blocker for me know.

JPHutchins commented 6 months ago

@tomaszduda23 Agreed that we need the fix now, I will add it back!

I’ll be advocating for Bleak to add acquire_mtu() to the public interface.

tomaszduda23 commented 6 months ago

I've tested 3.0.1 and MTU works correctly :+1: