apache / mynewt-nimble

Apache mynewt
https://mynewt.apache.org/
Apache License 2.0
709 stars 402 forks source link

GATTS long reads behavior #1090

Open rojer opened 3 years ago

rojer commented 3 years ago

In general, I like that NimBLE tries to hide the complexity of long reads and writes from the user, automatically servicing them within the stack, it's a welcome change from Bluedroid that spills all this onto the user of the API and expects the user to deal with all the complications.

However, currently observed behavior for long reads is that NimBLE will repeatedly request the value from the attribute callback while responding to central's requests and sending MTU chunks at a time (at least i suspect that's what's happening). However, if the value of the characteristic happens to change between reads, incorrect value will be returned to the client. As far as i can tell, there is current;y no way for the API user to tell if a particular BLE_GATT_ACCESS_OP_READ_CHR is part of the same long read or not - for example, read offset is not provided to the user, so user application cannot work this around either.

IMO, NimBLE should buffer the long response and serve long reads from internally buffered copy of the response and not rely on the value staying the same.

ivanholmes commented 2 years ago

I ran in to this as well. My callback contained a function that gathered data from an external source which would run multiple times and crash the microcontroller. I must say I found it quite confusing that this was happening, although I have now had a practical education on one of the inner workings of BLE, which is a good thing I suppose!

I think @rojer's idea of buffering the data is a good one. It would also be useful to be able to tell inside the callback function if it is being run for the first time in a long read, the last time, or somewhere in the middle. This would allow the implementation of characteristics that iterate through a data structure, which would be useful in my application.

macksal commented 10 months ago

Running into this sporadically. It is a confusing behaviour that the offset reads are handled, but the developer still needs to consider this case.

Buffering internally in Nimble would be good, but I suppose that could be an equally nasty surprise in a low resource environment.

@ivanholmes @rojer did you end up with any workarounds? I'm thinking of caching the value when it's first read, and returning the cached value if it's read again within some time period (e.g. 5 seconds).

rojer commented 10 months ago

@macksal yep, in my library i buffer the full response and wait until NimBLE reads it out in MTU-1 byte sized chunks (here). this seems to be working so far but could break if nimble decides to change their behavior.

sjanc commented 10 months ago

Hi,

This is integral trait of long reads in GATT. Specification doesn't treat long read as atomic and thus server never knows what (if any) GATT procedure is executed by the client. It is up to higher layers (ie profiles) to define and handle this.

This is from Core Spec 5.4 Vol 3 Part F 3.4.4.5 "ATT_READ_BLOB_REQ" "Note: The value of a Long Attribute may change between the server receiving one ATT_READ_BLOB_REQ PDU and the next ATT_READ_BLOB_REQ PDU. A higher layer specification should be aware of this and define appropriate behavior."

rojer commented 10 months ago

that's fine, the problem is there is no way to do that currently. it's literally impossible to tell if the read being serviced is a part of long read or not. for example, my higher layer decision is to present consistent view to the client but how am i to tell the NimBLE api reads apart? right now i'm relying on MTU-1 read behavior, which is less than ideal. even as much as passing an offset for the read would be sufficient (as bluedroid does).

sjanc commented 10 months ago

ahh ok, I see now, I think we should be able to extend ble_gatt_access_ctxt with needed info without breaking API compatiblity

vandy commented 3 months ago

@ivanholmes, @macksal, @rojer, I believe buffering the data inside the nimble stack is not an option here. Because it's up to the client to decide whether it wants to and will read the whole attribute or not, so there is no guarantee that after the first read request there would be others to consume the whole attribute.

As provided in the comments offset field in ble_gatt_access_ctxt struct could indicate the long attribute read procedure. I've made a pull request on this #1846.