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

add configurable MTU #1998

Merged
merged 2 commits into from Apr 13, 2021
Merged

add configurable MTU #1998

merged 2 commits into from Apr 13, 2021

Conversation

fanoush
Copy link
Contributor

@fanoush fanoush commented Apr 12, 2021

  • add configurable MTU to linker scripts and softdevice initialization
  • add triggering MTU and DLE negotiation when being central
  • keep track of effective (negotiated) MTU for central/peripheral connections
  • use effective MTU for nordic uart so we do not send more data if other side supports lower MTU then we
  • allow sdk12 nus service to use larger MTU

@fanoush
Copy link
Contributor Author

fanoush commented Apr 12, 2021

I have tested this with nordic NRF52840 dongle on SDK15 and E104BT5032A nrf52832 board on SDK12. Tested with WebIDE (seems to not break with this) and between each other both as central and peripheral to the other one each having different MTU 131 and 59 (so the effective MTU comes into use). testing larger MTU is visible on notifications - you get more data.

also you can test via https://fanoush.github.io/web-device-cli/ - open developer console - F12 as see console log about guessed MTU (code here https://github.com/fanoush/web-device-cli/blob/master/js/app.js#L128) looks like chrome on windows goes only up to 127

what is missing is to have effective MTU readable for JS code so you know how much you can use

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

This looks awesome - thanks! Maybe the MTU could just be added to getSecurityStatus? I can add that.

Just a quick one before I merge - do you see any issue with me ifdefing BLE_GATTC_EVT_EXCHANGE_MTU_RSP and the effective MTU based on whether the MTU is higher than default or not? It'd just be nice to avoid any extra code if we can avoid it on the really restricted platforms (eg nRF51)

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

was testing different MTUs and how it affect app_ram_base and it is not trivial. First the default 0x2c40 you have there is too high, the real value when current build for SDK12,52832 starts is 0x2828 for me. Then it looks like it jumps mostly by 0x20 but sometime it is less and sometimes more, here are some values that should work (did not test 55-57, they are approximated from 54 and 58).

default:0x2828 50:2bd8 51:2c00 52:2c20 53:2c40 54:2c68 55:2c88? 56:2ca8? 57:2cc8? 58:2ce8 59:2d18 60:2d38 61:2d58 77:0x2f88

as for good MTU size to pick - the iphone uses formula 23+x*27 as per table in https://devzone.nordicsemi.com/f/nordic-q-a/44825/ios-mtu-size-why-only-185-bytes the rationale being that it best fits into connection interval (not sure why 27 and not 23). That would give numbers like 50,77,104,131 but I guess it doesn't matter much and one could also pick MTU which exactly covers use case needed (like 59 with those unicycles using 56 byte protocol) and uses minimum of memory. For 52840 I like 131 as -3 it is 128 which is nicely rounded.

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

Just a quick one before I merge - do you see any issue with me ifdefing BLE_GATTC_EVT_EXCHANGE_MTU_RSP and the effective MTU based on whether the MTU is higher than default or not? It'd just be nice to avoid any extra code if we can avoid it on the really restricted platforms (eg nRF51)

yes, was thinking about it too, if there is no central this can be removed too and perhaps also the effective mtu variables. Was thinking about making some define EXTENSIBLE_MTU defined from (NRF_BLE_MAX_MTU_SIZE > GATT_MTU_SIZE_DEFAULT) and then ifdef out all mtu stuff based on that

feel free to play with it and change as you wish.

could just be added to getSecurityStatus

well it is not about security, can connection itself have such properties? connection being perhaps https://www.espruino.com/Reference#BluetoothDevice ? because it can be different per connection - each device negotiates different value

the https://www.espruino.com/Reference#l_NRF_getSecurityStatus is not per connection so with both central and peripheral I am not sure what it is supposed to return, I'd deprecate that one :-) Also in future we can have more connections than two

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

Ok, great - yes, I'll do an EXTENSIBLE_MTU thing.

There's a BluetoothDevice getSecurityStatus as well ;) But yes, it's not ideal. I guess longer term there may be more info (connection interval/etc) so we probably don't want just properties in there, but a getInfo function...

How sure are you about this? It looks fine but I've been meaning to push a 2v09 version out this week, and I'm wondering if I should do that and then merge this?

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

... having said that, it'd be a really nice addition to have in the new version :)

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

I've tested it only with android phone, windows 10 computer, 52832 board and nrf52840 dongle over few days in all directions. It also works for those guys with those unicycles so far. Also if you do not add MTU to any board file and ifdef MTU stuff as you suggest there is no change - but then you can wait with merge :-) Not sure if you have more (IOT?) devices yourself that would need or benefit from larger MTU to work fine? Definitely more testing with real devices could be useful. But if you don't have more devices to test then users must test this. Can you have second build with larger MTU for some supported boards so people could revert to that if it break their devices? In theory it can happen with any broken BLE device.

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

Also as for compatibility did you read softdevice 3.1.0 release notes? You use softdevice 3.0.0 in builds but it is not downloadable from nordic site anymore. They list only 3.1.0 there https://www.nordicsemi.com/Software-and-tools/Software/S132/Download

From release notes it looks like there is possibly some known buffer corruption/exploit in 3.0 (reason for removal from downloads?) and also they added lot of stuff related to DLE including It is now possible to completely disable the DLE feature. so maybe it breaks with some devices even if properly implemented on our side. I have actually tested with 3.1.0 in the beginning but then went to 3.0 just to be sure it works there too and didn't see any difference. So in addition to MTU and connection interval info (which all could be also passed to NRF.requestDevice or device.gatt.connect() in future as preferred values for opening the connection?) DLE and PDU size could be perhaps set or seen there too.

EDIT: oh now I see you actually can pass connection interval to https://www.espruino.com/Reference#l_BluetoothRemoteGATTServer_connect already. Nice so requested MTU (and possibly DLE flag to not initiate it on SD >3) could go there too in future.

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

Thanks! That's very interesting. And yes, it'd be great to be able request a specific MTU - but I can't imagine too many times where you wouldn't want the maximum?

Just filed an issue for 3.1.0 softdevice at #1999

It should be compatible with the existing SDK12 builds? All I need to do is swap it over? Definitely seems like a good idea.

re. DLE - I wonder if the issue is just that people screw up allocation of it in their code and then the softdevice blindly sends a bit of your application. Maybe some folks want to disable it just to be sure

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

Thanks! That's very interesting. And yes, it'd be great to be able request a specific MTU - but I can't imagine too many times where you wouldn't want the maximum?

not sure about MTU but regarding DLE it may be needed for compatibility, some time ago I've seen this https://software-dl.ti.com/lprf/simplelink_cc2640r2_latest/docs/blestack/ble_user_guide/html/ble-stack-3.x/data-length-extensions.html#sec-interoperability-with-peers so maybe makes sense with some older devices to request MTU 23 and then our code could avoid all that MTU and DLE negotiation. Or higher MTU could still make sense without DLE (if that breaks it) so the packets are fragmented but you can still get more data on GATT level.

Also I guess it may be related to selected connection interval - with longer interval you can fit more or larger packets, with shorter interval maybe shorter packets may make sense as mentioned near the end of https://punchthrough.com/maximizing-ble-throughput-part-3-data-length-extension-dle-2/ Also not sure if in noisy/crowded conditions shorter packets could get better success. Don't know really, but I can imagine it can be sometimes useful to change it :-)

It should be compatible with the existing SDK12 builds? All I need to do is swap it over?

yes, I used it randomly instead of 3.0 and seem to just work, however it also needs fixing DFU package generation in board files so it allows newer version too - 'DFU_SETTINGS=--application-version 0xff --hw-version 52 --sd-req 0x8C,0x91'

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

I just merged in 3.1.0...

First the default 0x2c40 you have there is too high, the real value when current build for SDK12,52832 starts is 0x2828 for me

Great! So we can actually increase the MTU in all the Espruino parts to 53 without losing any RAM at all?

How did you find those figures? Just trial and error, or did you enable logging and see what was being reported back as an error?

@gfwilliams gfwilliams merged commit 2792f36 into espruino:master Apr 13, 2021
1 check passed
@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

Sorry, I'm being dumb.

From sd_ble_enable:

p_app_ram_base - Pointer to a variable containing the start address of the application RAM region (APP_RAM_BASE). On return, this will contain the minimum start address of the application RAM region required by the SoftDevice for this configuration.

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

Great! So we can actually increase the MTU in all the Espruino parts to 53 without losing any RAM at all?

Yes that's what it tells me in my build, so MTU of 53 should work with same value of 0x2c40. With -DDEBUG_APP_RAM_BASE defined you should see 0x2828 in process.env.APP_RAM_BASE in current build without increasing MTU.

@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

Thanks for all your work on this - it's awesome! Seems to work perfectly on the devices I have tried so far.

I added a few hacky ifdef things to avoid adding code when this isn't enabled, but I'll have more of a play with this over the coming days and will add it in the 2v09 release.

It's interesting that the connection process with the IDE is significantly faster with this. I guess it's just the extra speed of transferring process.env. I'll make some changes to the IDE now to take advantage of the MTU

gfwilliams added a commit to espruino/EspruinoTools that referenced this pull request Apr 13, 2021
…ck from characteristics that is over that. Takes advantage of espruino/Espruino#1998
gfwilliams added a commit to espruino/EspruinoWebIDE that referenced this pull request Apr 13, 2021
…ck from characteristics that is over that. Takes advantage of espruino/Espruino#1998

Fix bug with relay creating the correct code
@gfwilliams
Copy link
Member

gfwilliams commented Apr 13, 2021

Ok, https://espruino.github.io/EspruinoWebIDE/ should now use the increased MTU.

Next up the Puck.js/UART.js libs could be updated...

@fanoush
Copy link
Contributor Author

fanoush commented Apr 13, 2021

Ok, https://espruino.github.io/EspruinoWebIDE/ should now use the increased MTU.

works quite well :-) I can feel the difference, seeing reset() or process.env, or E.dumpxxx - it just shows without waiting, also tried to upload same 35KB binary file to storage - with old fw and old ide it took 49 seconds to upload, with new one (and mtu 61) it took 20 seconds :-)
Also downloading - clicking eye button in storage dialog - 19 seconds vs 7 seconds for same file, nice :-)
Also with nrf52840 dongle (MTU 131) uploading same file to storage took 11 seconds however downloading/view took 7 seconds so upload scales with MTU but in this case download does not when compared to MTU 61.

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.

None yet

2 participants