adafruit / Adafruit_CircuitPython_BLE

Bluetooth Low Energy (BLE) library for CircuitPython
MIT License
128 stars 58 forks source link

Please increase buffers size for Nordic UART Service from 64 to 256 bytes #127

Closed box8 closed 3 years ago

box8 commented 3 years ago

In nRF5 SDK 17.0.2 in ble_nus.h BLE_NUS_MAX_DATA_LEN defined as:

define OPCODE_LENGTH 1

define HANDLE_LENGTH 2

...

define BLE_NUS_MAX_DATA_LEN (NRF_SDH_BLE_GATT_MAX_MTU_SIZE - OPCODE_LENGTH - HANDLE_LENGTH)

BLE 4.2 allows 247 bytes for NRF_SDH_BLE_GATT_MAX_MTU_SIZE, so BLE_NUS_MAX_DATA_LEN = 244

Current version of UARTService just drops extra bytes (64+) from received packet.

dhalbert commented 3 years ago

We would be happy to entertain a PR, if you feel up to it.

@tannewt BTW, do you see dropping of bytes in the REPL serial, when, say, pasting a long string into the REPl?

box8 commented 3 years ago

I'm currently working on nRF52832 firmware, NUS transfer implemented as described in https://www.programmersought.com/article/17621618211/ Method 2+ -- burst of ble_nus_data_send when BLE_NUS_EVT_TX_RDY event received. The amount of data sent in one burst is ~950 bytes, so 256 byte buffer is not enough anyway. Implementing https://github.com/adafruit/Adafruit_CircuitPython_BLE/issues/37 would be better.

tannewt commented 3 years ago

First, we aren't using BLE_NUS_MAX_DATA_LEN internally at all.

For the serial BLE in CircuitPython, I'm creating my own incoming characteristic buffer and outgoing packet buffer. I haven't tested it extensively so I'm not sure if we'll see reliability issues. I imagine we will in extreme cases but the nordic uart service doesn't manage control flow. So, I'm not sure how you improve that. The file transfer service does its own control flow to make sure peripheral side buffers aren't overrun.

The Method 2+ thing looks to be managing the outgoing buffer, NOT the incoming buffer on the receiver. In CircuitPython, PacketBuffer will do this for you already.

37 is the better long term fix but it's low priority for us. @box8 I'd recommend copying the definitions and changing the buffer size for yourself. That'll be simplest and doable now.