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
add configurable MTU #1998
Conversation
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
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 |
This looks awesome - thanks! Maybe the MTU could just be added to Just a quick one before I merge - do you see any issue with me ifdefing |
was testing different MTUs and how it affect app_ram_base and it is not trivial. First the default 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. |
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.
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 |
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 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? |
... having said that, it'd be a really nice addition to have in the new version :) |
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. |
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 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. |
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 |
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 :-)
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 - |
I just merged in 3.1.0...
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? |
Sorry, I'm being dumb. From sd_ble_enable:
|
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. |
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 |
…ck from characteristics that is over that. Takes advantage of espruino/Espruino#1998
…ck from characteristics that is over that. Takes advantage of espruino/Espruino#1998 Fix bug with relay creating the correct code
Ok, https://espruino.github.io/EspruinoWebIDE/ should now use the increased MTU. Next up the Puck.js/UART.js libs could be updated... |
…boards to 53 byte (from 23) (#1998)
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 :-) |