Version 6.0.2 breaks bytesToString() and startNotification()

[paolosanchi]

Describe the bug
This commit seems to break startNotification() on js side:
perf: improve byte to hex string conversion optimizations (#701)

The issue is caused by bytesToString(), because it returns a hex string without spaces between the hex couple of values.

To Reproduce
Steps to reproduce the behavior:

val value = bytesToString(data)
val originalData = stringToBytes(value)

originalData is different from data

This is what I have debugging

data: [77, 83, 71, 95, 73, 78, 73, 84, 95, 79, 75, 58, 48, 10]
originalData: [77]
value: "4D53475F494E49545F4F4B3A300A"

Expected behavior
value shoud be like this instead:
"4D 53 47 5F 49 4E 49 54 5F 4F 4B 3A 30 0A"

the issue resides in here:

bluetooth-le/android/src/main/java/com/capacitorjs/community/plugins/bluetoothle/Conversion.kt

Line 21 in 8e99165

return result.concatToString()

because the following function from the kotlin framework StringsJVM.kt returns the string without spaces:

public actual fun CharArray.concatToString(): String {
    return String(this)
}

Screenshots
see above

Plugin version:

• @capacitor-community/bluetooth-le:6.0.2

Desktop:
not applicable

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

Proposed solution

fun bytesToString(bytes: ByteArray): String {
    return bytes.toHexString().chunked(2).joinToString(" ")
}

or even better

fun bytesToString(bytes: ByteArray): String {
  return bytes.joinToString(separator = " ") { byte -> "%02X".format(byte) }
}

I can't verify if this invalidates the performance enhancements gained by the above commit

[peitschie]

Hi @paolosanchi

Just to confirm, are you finding this change in internal behaviour is causing an observable difference when used as part of BleClient.startNotifications?

[paolosanchi]

@peitschie I've found this issue because the callback of startNotification() on the js side receives always an empty string.
Indeed, the following function expect be bytes in the string to be separated by a space

bluetooth-le/src/conversion.ts

Line 19 in 8e99165

return numbersToDataView(value.split('').map((s) => s.charCodeAt(0)));

[peitschie]

The code here is a bit different to what you're describing 🙂

The real flow here is:

1. startNotification receives a hex string here: https://github.com/capacitor-community/bluetooth-le/blob/main/src/bleClient.ts#L670
2. It calls this.convertValue, which forwards on to hexStringDataToView: https://github.com/capacitor-community/bluetooth-le/blob/main/src/bleClient.ts#L706
3. hexStringDataToView was carefully refactored to process a non-space separated hex string: https://github.com/capacitor-community/bluetooth-le/blob/main/src/conversion.ts#L44

I've just tested this again locally, and can confirm that for me at least, the proper values are coming through:

As you can see... the hexStringToDataView is correctly handling the space-less hex-encoded bye array.

Are you able to confirm the conversion.ts implementation embedded in your app matches this code, i.e., there's not a stale version of the plugin source somewhere in your setup?

Can you share an example of a full payload you're transferring, so I can see if there's any issues with the size or data contained?

[paolosanchi]

@peitschie my bad, you are right. I've got this issue because I use ionic's Live Update, and I had the version 6.0.2 for android and 6.0.1 for web (I forgot to commit the package.json).

It is fine if the old web version is not compatible with the new one for android, the opposite would not be good, wich this is not the case.

While reporting the js code reference, I've been confused by textToDataView() wich uses .split(' '), but the function is not used anymore (I did a quick search on the repo).
The code with .split(' ') I've found in debug was relative to the old version, and I simply was not aware of it.

That said, the issue on the android side may still cause unwanted behaviour since stringToBytes() cannot handle strings without spaces, and stringToBytes().
However, it seems to be used only in write(), and it is the user that set directly the uid. I mean, it doesn't seem to be a regression.

My issue was not a bug, and it is solved on my side. If you think it doesn't worth to fix stringToBytes() I guess it's fine.
Sorry for the false alarm and thank you for the prompt reply.

[peitschie]

My issue was not a bug, and it is solved on my side. [...] Sorry for the false alarm and thank you for the prompt reply.

Fantastic to hear that you've found a solution. I appreciate you taking the time to detail what you encountered here!

That said, the issue on the android side may still cause unwanted behaviour since stringToBytes() cannot handle strings without spaces, and stringToBytes().

Yeah... on close inspection, although the behaviour in stringToBytes is not buggy, it's definitely a bit inconsistent.

There is no bug/regression here currently, because the conversion pair is dataViewToHexString (link) on the JS which does include a space.

It does feel a bit inconsistent though that we put spaces between hex pairs going JS => native, but don't put these on the way out going native => JS.

I'll close this particular issue out, but will raise a new one for the inconsistency between these interfaces.