Skip to content
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

Incorrect notified value on fast changes #688

Open
paolosanchi opened this issue Sep 14, 2024 · 10 comments
Open

Incorrect notified value on fast changes #688

paolosanchi opened this issue Sep 14, 2024 · 10 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@paolosanchi
Copy link

paolosanchi commented Sep 14, 2024

Describe the bug
On consecutive notifications of a characteristic, only the latest value is received multiple times.

I have a bluetooth device (esp32) that uses a characteristic to submit text messages via notification.
It may happen that N messages are sent one suddenly after the other.
In such case the startNotifications 's callback may be called N times with the latest value, and the previous messages are lost.

I could verify this behaviour on an android pad with android 8 sdk 26 (Huawei MediaPad 5 Lite).
I'm sure that the device (that sends the notifications) works correctly, because this bug doesn't happen other devices (android, ios and web).

Based on the code, the issue seems come from the sdk:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {

Side note:
I understand right now the difference from PROPERTY_NOTIFY and PROPERTY_INDICATE
https://community.nxp.com/t5/Wireless-Connectivity-Knowledge/Indication-and-Notification/ta-p/1129270

So I've tried to change the characteristic property from PROPERTY_NOTIFY to PROPERTY_INDICATE, but the android onCharacteristicChanged() event is not fired anymore.

To Reproduce
Steps to reproduce the behavior:

  1. Register to startnotifications() to listen for value changes of a characteristic.
  2. Notify different values N times from the device's characteristic in a fast sequence.
  3. Only the latest value is received for N times. The previous value are lost.

Expected behavior
Every value change notification should reach the client.

Plugin version:

  • @capacitor-community/bluetooth-le: 6.0.1

Smartphone (please complete the following information):

  • Device: Huawei MediaPad 5 Lite
  • OS: Android 8
  • Version: SDK 26

Additional context
Add any other context about the problem here.

@paolosanchi paolosanchi added the bug Something isn't working label Sep 14, 2024
@peitschie
Copy link
Collaborator

Hi @paolosanchi

Are you saying this issue only occurs with Android 8 + the Huawei device? You mention that the bug doesn't happen on other devices, including some other android devices.

Something to try to see if the notifications become more reliable is to explicitly request a higher connection priority via https://github.com/capacitor-community/bluetooth-le/blob/main/README.md#requestconnectionpriority

This can sometimes force Android to wake-up more actively and process packets. This increases battery usage, but may stop the lower parts of the stack from discarding the notification.

NOTIFY can be slightly lossy, as it has no requirement for acknowledgement at the GATT layer. If the notifications are important for the phone to receive, you definitely want to be using INDICATE instead of NOTIFY. However, this requires firmware support to implement, as the firmware must advertise the property as INDICATE as well, and perform retries as necessary.

@paolosanchi
Copy link
Author

paolosanchi commented Sep 16, 2024

Yes, only on this device, which memory is actually almost full (maybe it is slowed down somehow).

Thanks for the requestconnectionpriority advice, I'll check it out, I've also tried the INDICATE property, but it seems I got no ACK from the device. I should test it deeper. I have control over the firmware, I could actually provide an arduino sketch for ESP32 if necessary.

The strange fact, is that for each notification the related onCharacteristicChanged() is fired. So NONE of the notification is lost. The fact is that the VALUE is overwritten by the last notification, and the other values are lost (not the event).

Because of this, It doesn't seems not related to priority or indicate.

@NaterGator
Copy link
Contributor

@paolosanchi you may want to try the changes in #701 for this. I found the conversion functions woefully under-performant for high throughput notification use cases. It also sounds like there may be some platform-level issue at play here, but your application will be able to actually handle the incoming notifications much more quickly and may actually process them before the next notification comes in, avoiding the platform level issue.

@paolosanchi
Copy link
Author

paolosanchi commented Oct 25, 2024

I've tested it and I'm sorry @NaterGator, this change doesn't fix my specific issue. Thanks for notifying it, anyway.

@NaterGator
Copy link
Contributor

I'm sorry to hear that @paolosanchi... worth a shot!

@peitschie
Copy link
Collaborator

@paolosanchi I've been digging through the code a bit to see if I can understand where this might happen...

If you still have the setup there to debug, can you try replacing the startNotifications method in your existing code in BluetoothLe.kt with this and see if this makes any difference?

    fun startNotifications(call: PluginCall) {
        val context = CoroutineScope(newSingleThreadContext("MyOwnThread"))
        val device = getDevice(call) ?: return
        val characteristic = getCharacteristic(call) ?: return
        device.setNotifications(characteristic.first, characteristic.second, true, { response ->
            context.launch {
                val key =
                    "notification|${device.getId()}|${(characteristic.first)}|${(characteristic.second)}"
                val ret = JSObject()
                ret.put("value", response.value)
                try {
                    notifyListeners(key, ret)
                } catch (e: ConcurrentModificationException) {
                    Logger.error(TAG, "Error in notifyListeners: ${e.localizedMessage}", e)
                }
            }
        }, { response ->
            run {
                if (response.success) {
                    call.resolve()
                } else {
                    call.reject(response.value)
                }
            }
        })
    }

Note, you'll need to add these two imports up the top of this file:

import kotlinx.coroutines.launch
import kotlinx.coroutines.newSingleThreadContext

@paolosanchi
Copy link
Author

@peitschie thanks for digging through this. I'm sorry, but it doesn't solve.
To compile it I had to add also import kotlinx.coroutines.CoroutineScope

The fact is that in debug the data at the following line is already wrong:


Note that Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU is false, because TIRAMISU is 33 and I'm running on a 26 SDK

@peitschie
Copy link
Collaborator

Under debug, this wouldn't surprise me, as debugging will introduce additional performance penalties on what you're inspecting here. Given this only happens under a high notification rate, debugging will make the problem worse 🙂

The hope is that using launch instead of run above can free up the notification callback a bit more quickly... hopefully enough to mostly keep up.

From what you're observing though, it does seem likely that we won't be able to fix this unfortunately. Under debug at least, by the time the characteristic changed callback has been raised, a newer value has already been written there.

@paolosanchi
Copy link
Author

As I understand, in the ble protocol the value is sent along with the notification. The debugging and performance should not influence this behavior, and this is why I think it's a framework issue that may have been fixed with latest versions.

By the way, I've tested it also without the debugger attached and it still doesn't work.

I've already "solved" this issue with a retry mechanics.

@peitschie
Copy link
Collaborator

I'll flag this with won't fix, as I agree with you that this is an Android issue, not a plugin issue that can be solved. The newer callback methods in current Android avoid this issue completely by supplying the notification value separately.

Thanks for the ongoing data @paolosanchi ... it's much appreciated and beneficial for others who find similar quirks.

@peitschie peitschie added the wontfix This will not be worked on label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants