notify / sendValue() silently truncates output data, thoughts on how to surface this from the library?

[chmorgan]

I've had this annoying issue for several months where notify payloads were corrupted json but reading the characteristic worked correctly in my iOS app.

Looks like this is something nimble is doing internally, it's fitting the payload into the MTU and dropping the rest off the end.

What was tough was that I check all return values from esp-nimble-cpp and nothing ever returned a return value to indicate an issue.

It would be great if esp-nimble-cpp had some mechanism to indicate that this truncation was occurring, perhaps through an additional payload check in sendValue() where the return value could be an enum instead of a bool where if truncation occurred you'd return a SENT_TRUNCATED value or something.

Thoughts on this issue?

[h2zero]

I think changing the return value to the number of bytes sent would be a good way to handle this indication.
However it is best to pre-handle this in the app by checking the MTU size first, notifications are limited to the MTU-3.

[chmorgan]

@h2zero I'd have no idea that this was a limitation and I've been working with the ble for years, I figured no errors returned == everything worked...

If you are ok with returning the number of bytes sent I'll whip up a patch to do so. How to handle the api breakage?

[h2zero]

Yeah, as much as I try to hide it here there is still an element of BLE inner workings knowledge required.

I don't know if that is the best way to deal with this as notify() could be sent to more than one client, what value should be returned then?

Any change such as this would be a breaking change, so would be reserved for a 3.x release.
Maybe a function NimBLEServer::getMinMTU which would return the smallest MTU of all the clients connected so the app could limit the data to that would be better?

[chmorgan]

I think you'd want the library to indicate the input data was limited / truncated by the underlying system / library.

Doesn't nimble provide a way to get the MTU size? maybe it reports the smallest size of all clients?

[h2zero]

A simple way to do this in your app:

auto peers = pServer->getPeerDevices();
size_t maxLen = 517;
for (auto peer : peers) {
    size_t peerMTU = pServer->getPeerMTU();
    if (peerMTU < maxLen) {
        maxLen = peerMTU - 3;
    }
}

[chmorgan]

I don't disagree. I'd still propose that the library should, at the cost of a size retrieval and comparison, perform this check for users. It would have been hugely valuable to me to see the api that reports this and have the option to use it.

What about a notify_v2() or notifyWithResult() or some variant like that which reports a truncation? existing users call notify() (which internally runs the same code path as notifyWithResult() but drops the result).

[h2zero]

The issue is still how to report it, if 3 clients receive the notification from the single call but one has an MTU of 27 another of 180 and another of 517 for instance and you send 512 bytes, what value should be returned? All 3 clients just received different amounts of data.

[chmorgan]

My understanding is that today each client would receive different amounts based on their MTU.

In terms of reporting, could we report that some truncation occurred and then the caller could iterate over the connections checking each of their MTUs?

One alternate option would be to return a std::array<> of connection handles, MTUs and bytes accepted but this is very heavy in terms of stack usage.

[h2zero]

I'm not a big fan of returning a complex type, too hard for the user to parse and adjust for. I'm thinking something could be done with the onStatus callback, I'll think on it.

[chmorgan]

I'm happy to implement along any lines that you think are suitable. A callback that is optional could be an approach (if that's what you are referring to.)

Note that in my comment above the complex type would only be returned on the new "Notify with status" method so normal users would remain able to see true/false without any of that complexity.

I'm not seriously suggesting though that we return a std::array<> of connection handle responses, that seems excessive and wasteful.