Experimental: update a remote characteristic from a notification

[Jeroen88]

Experimental: some peripherals, like the Xiaomi LYWSD03MMC, update a remote characteristic with a notification (off spec). This PR takes the updated value from the notification event to update the characteristic.

A few thought about this PR:

• NimBLERemoteCharacteristic::readValue() detects if it needs to update m_value by checking it's length. Because ::readValue() initiates m_value with the empty string, and the BLE_GAP_EVENT_NOTIFY_RX updates the value if the peripheral sends a notification, this is okay (though a bit shaky)
• It may be possible that the characteristic is registered for notify. I think it is possible that the notify updates the characteristic, thus preventing ::readValue() to update it through a regular call. This case is hypothetical I think: why should you try to get the same value through a ::readValue() and a notification? And if someone does so, it stays the value of the same characteristic (although the two values could differ because of a small gap in time, but again, this time gap is also very small)

In conclusion, I think this PR is acceptable, depending mostly on if @h2zero wants to accept an off specification (but existing in the real world) case.

[h2zero]

My initial thoughts on this is it may negatively impact other use cases. For example some peripherals may have large characteristics, say 500 bytes, if the MTU is not large enough to send all the data for this it could use notifications to signal the central to read the value. As far as I know there is no rule that the peripheral must notify the actual value of the characteristic.

The other issue, especially on the esp32 due to being dual core, is the app may be performing an operation on the current value at the same time a notification comes and overwrites it, not good.

I'll have to think on this a bit.

[chegewara]

May i ask/suggest something?
I never liked solution with BLEValue, because bluedroid stack already keep characteristic value and it was waste of heap. IMO better is to use pointers to returned value, update it and call callbacks when event is fired.
Its up to developer if he want to have copy of this value in case it will be overwritten with notifications before he use its value.

[h2zero]

@chegewara Yes I think that would be a better way to deal with the values in bluedroid, however NimBLE does not store the values internally, it just passes a buffer to a callback and then releases it. At least that's my understanding, I will look at it to be sure.

[Jeroen88]

The other issue, especially on the esp32 due to being dual core, is the app may be performing an operation on the current value at the same time a notification comes and overwrites it, not good.

I am not sure, but I believe your NimBLE library will run on one core only. Also the chance that the peripheral sends the same characteristic (almost)multitasking is low / nihil? But otherwise semaphores can be used.

For example some peripherals may have large characteristics, say 500 bytes, if the MTU is not large enough to send all the data for this it could use notifications to signal the central to read the value.

Maybe I do not understand you, but if someone wants to read a characteristic, be it with or without this modification, it should fit into the MCU. If it does not, it can't be read in the first place

@chegewara I love pointers and references too!

[chegewara]

In case NimBLE is not keeping values internally i would suggest to use uint8_t* to store raw data and let developer interpret data, eventually add few basic values returned like getInt, getString, getFloat etc. I learned that using std::string, which is handy, also is having too big overhead. Its just my 5 cents.

[h2zero]

I am not sure, but I believe your NimBLE library will run on one core only. Also the chance that the peripheral sends the same characteristic (almost)multitasking is low / nihil? But otherwise semaphores can be used.

@Jeroen88 What I mean here is if the app is doing something with the current value, maybe printing it in a loop or whatever and a notification arrives in the middle of that process and changes the data it could trigger an exception. Also the callback handleGapEvent() is in callback context so it is executed in the same core as the NimBLE host task i.e. Core 0, while the app is running in Core 1.

Maybe I do not understand you, but if someone wants to read a characteristic, be it with or without this modification, it should fit into the MCU. If it does not, it can't be read in the first place

Characteristics larger than MTU can be read via a long read operation. Notification size is limited to MTU-3 bytes though.

@chegewara I agree and have started to transition to that in the client-long-read-write branch. Need more time to do the work on this though. I also wanted to keep compatibility with the original library intact so this uses the same strategy right now.

[chegewara]

@h2zero Im not using this library, yet, but i think you are doing great job portining nimble and trying to keep backward compatibility with old ble library. Do you have paypal?

[h2zero]

Thanks! @chegewara, yes I do, how much do I owe you? 😄

[Jeroen88]

What I mean here is if the app is doing something with the current value, maybe printing it in a loop or whatever and a notification arrives in the middle of that process and changes the data it could trigger an exception. Also the callback handleGapEvent() is in callback context so it is executed in the same core as the NimBLE host task i.e. Core 0, while the app is running in Core 1.

@h2zero I doubt that. I have been runnning reading out the sensor with the adaptation for 17 hours now, reading the value every 20 sec, that is more than 3,000 readouts without any problem!

Characteristics larger than MTU can be read via a long read operation. Notification size is limited to MTU-3 bytes though.

@h2zero I have no knowledge of this. As I understand it now, a characteristic larger than 512 bytes can be read with a multiple read, and that is called a long read? If so this could also be accommodated in the notify scheme, but I doubt it will be necessary. :)

[chegewara]

Thanks! @chegewara, yes I do, how much do I owe you? smile

I am willing to buy you a beer

[chegewara]

@h2zero I have no knowledge of this. As I understand it now, a characteristic larger than 512 bytes can be read with a multiple read, and that is called a long read? If so this could also be accommodated in the notify scheme, but I doubt it will be necessary. :)

Long read is when characteristic value length is longer than MTU - 1 IIRC (or -2). I am using a lot OTA over BLE with android and iOS. On android i can setup MTU up to 517, on iOS its 257 IIRC, to match L2CAP max message length.

I didnt look at this PR code earlier, sorry. This is how i think it should look like:

• remote characteristic value should be updated on client on notification received, here we can discuss if we want to update value before or after sending notification callback (making notification callback with return bool we could prevent to store/overwrite local value with new one, giving option to validate data for example),
• remote characteristic should be updated the same way as with notifications.
  Its may not be consistent with old library, but may give us better control over code.

[h2zero]

@chegewara Thank you! a beer sounds really good right now 🍻. link

I didnt look at this PR code earlier, sorry. This is how i think it should look like:

• remote characteristic value should be updated on client on notification received, here we can discuss if we want to update value before or after sending notification callback (making notification callback with return bool we could prevent to store/overwrite local value with new one, giving option to validate data for example),
• remote characteristic should be updated the same way as with notifications.
  Its may not be consistent with old library, but may give us better control over code.

I agree with the premise of this PR and these points make sense. I have to give this some thought as I would like to keep the app in control of handling the data as much as possible. I also don't want to stray too far from the original library, as the intent there seems that the app should store/use the data without the library interfering.

@h2zero I doubt that. I have been runnning reading out the sensor with the adaptation for 17 hours now, reading the value every 20 sec, that is more than 3,000 readouts without any problem!

@Jeroen88 Yes I wasn't thinking about this properly, if the app stored it's own copy of the data there would not be an issue, I was more concerned with if the app was using a pointer to the data as in the other PR you made.

I'll play with this on the weekend.

[chegewara]

Beer on its way, or maybe few.

[h2zero]

@chegewara Much appreciated! Nice weather here for beer on the deck tonight 😄.

[Jeroen88]

I'll play with this on the weekend.

Nice :D

[h2zero]

Played around with this a bit and I think it could work. However it should be done within a larger rework patch, as @chegewara pointed out, handling the data in std::string isn't ideal.

I'm thinking the data should just be stored in a uint8_t array and readValue() could continue to return a string but just create it from that data.

• There would be a new read() method that readValue() could call to maintain backward compatibility.
• readRawData() would use the new read() method and return a pointer to the array.
• There should be a getData() method that does not call read() and just returns the data we already have so we can retrieve the data sent in the notifications which update the data array.

This would have to be done after the long read/write branch is merged to avoid conflicts.

Any thoughts?

[Jeroen88]

Yes I think that is indeed the right way forward!

[Jeroen88]

Replaced by PR #49