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

Set transport flag to BluetoothDevice.TRANSPORT_LE by default #61

Closed
degill opened this issue Jan 22, 2021 · 11 comments · Fixed by #90
Closed

Set transport flag to BluetoothDevice.TRANSPORT_LE by default #61

degill opened this issue Jan 22, 2021 · 11 comments · Fixed by #90
Assignees
Milestone

Comments

@degill
Copy link
Contributor

degill commented Jan 22, 2021

Hello there :)

Currently I am still using able, or better: I am using a slightly customised version of it where I set the transport flag of the connectGatt method to BluetoothDevice.TRANSPORT_LE.

I was wondering what the reasoning behind this decision is. Using BluetoothDevice.TRANSPORT_LE might reduce the appearances of the infamous 133 error. Other then anecdotal once I sadly have no proof to back up this claim.

I saw that you want to make this a customisable parameter, still I am wondering why in Able you didn't use the flag and here you use TRANSPORT_AUTO.

Also: Great library, really looking forward to incorporating Kable at one point :)

@twyatt
Copy link
Member

twyatt commented Jan 22, 2021

Hello there :)

Hi! 👋

I am wondering why in Able you didn't use the flag and here you use TRANSPORT_AUTO.

For Able, we never had a need to customize it, so just never focused on that code.

Android BLE is notoriously error prone when it comes to characteristic (and friends) objects because the Android system shares with you the same instances of BluetoothGattCharacteristic that it updates when values change, so data can essentially change underneath you and is very hard to debug. In Able, tried to work around this by storing a reference to the data separately as soon as things like "on characteristic change" events came in. This essentially fixes the problem but in very rare instances there is still a small window of time it could be changed before that separate reference is saved.

In newer versions of Android, you can use a Handler for the gatt callback threads. This allows all I/O to occur on the same thread and should completely eliminate the potential data race conditions.

In Kable, we wanted to utilize Handler to squash those potential hard to debug bugs. In order to use a Handler it required using method signatures that also needed the transport parameter. We just went with what we thought was the default (TRANSPORT_AUTO).

This particular parameter was introduced in Kable to fix a separate issue. 😄

Set transport flag to BluetoothDevice.TRANSPORT_LE by default

We'll have to look into it, but we'll want to have the default be whatever the default/expected behavior would be across all the supported platforms (or as consistent as we can get that).

I saw that you want to make this a customizable parameter

Definitely, so even if the default isn't what you'd typically use, you'll be able to configure it to your needs.

Also: Great library, really looking forward to incorporating Kable at one point :)

Thanks, really appreciate the kind words. We're planning to write a small migration guide soon that might help you out with the transition (#50).

@twyatt twyatt self-assigned this Jan 22, 2021
@twyatt twyatt added this to the 0.3.0 milestone Jan 22, 2021
@twyatt
Copy link
Member

twyatt commented Jan 22, 2021

It's a bit difficult to find conclusive details about how Apple and JavaScript handle this, but it seems Apple (Core Bluetooth) will "choose to use the BR/EDR transport if available" and "In the CB [Core Bluetooth] API there is no indication whatsoever that BR/EDR is used. Also, there's no way to specify which transport to use". — NordicSemiconductor/IOS-nRF-Connect#66 (comment)

The JavaScript documentation talked about support for BR/EDR but (similar to Core Bluetooth) there doesn't seem to be a way to choose.

Since it seems that Apple and JavaScript both exhibit a behavior akin to TRANSPORT_AUTO (with no option otherwise) I'd like to have that remain the default. This allows for multiplatform library consumers to not have to diverge from their common code for this setting (i.e. the default will be the same for all platforms).

I'll expose a way on the API to configure this option for Android platform/consumers.


Happy to reconsider if you can find evidence of it being configurable and/or a different default on JavaScript and Apple.

@degill
Copy link
Contributor Author

degill commented Jan 27, 2021

Sadly I cannot provide any info on how JavaScript or Apple handles this. I also don't have a full picture of what the different modes will do for different devices and in different situations. My only thought was that since this is a library targeting a BLE connection, it could default to whats best for each platform when it comes to BLE as long as behaviour more or less stays the same. And it seems that using TANSPORT_LE results in more reliable connection attempts on android (but again, I have only anecdotal evidence and some blog post here or there).

On one hand I can totally understand the goal of having all platforms behave the same but on the other hand I could see most consumers will want to change this flag for their android implementations.

Sidenote: For what its worth, Nordic only uses TRANSPORT_LE.

@twyatt
Copy link
Member

twyatt commented Jan 28, 2021

@degill I definitely hear you. Also totally understand only having "anecdotal evidence", BLE is such a complex beast (especially on Android) that many times people just gain experience with getting things to work a specific way (without actually being able to prove why it works better, due to things being so complex and hard to definitively prove).

I'll mull over this a bit more before making a final decision.

Not sure how to approach this, but if you happen to come up with some way (ideally via some test code w/ results) to prove that TRANSPORT_LE is more reliable then that would help make the decision. Or perhaps digging through the Android source code to find out what Android is doing differently under-the-hood when TRANSPORT_AUTO is used vs. TRANSPORT_LE (to help prove why TRANSPORT_LE might be more reliable) that would also be helpful. If I can find some time I'll also try to find the source code for this.

@degill
Copy link
Contributor Author

degill commented Jan 29, 2021

If I find the time I will try to do some more research. The complexity and the lack of docs and explanations is just a bummer.

Regarding the prove you mentioned: I can (sadly) not share our current project with you (it wouldn't help much either I guess because we develop our own hardware) but I definitely had a dramatic decrease in failed connection attempts with a 133 when I switched to the custom version of Able that only differs in this flag. Maybe this is just a problem with our own hardware? Maybe its because of the flag? I don't know. Right now only every 30th connection attempt fails and that is probably due to me brute forcing it, before it was like every 5th or 10th.

@twyatt twyatt modified the milestones: 0.3.0, 0.4.0 Feb 26, 2021
@twyatt
Copy link
Member

twyatt commented Mar 4, 2021

Looking through the Android source, TRANSPORT_AUTO selects what transport is used based on device_type.

btif_gatt_client.cc#L320-340:

  // Determine transport
  if (transport_p != BT_TRANSPORT_AUTO) {
    transport = transport_p;
  } else {
    switch (device_type) {
      case BT_DEVICE_TYPE_BREDR:
        transport = BT_TRANSPORT_BR_EDR;
        break;

      case BT_DEVICE_TYPE_BLE:
        transport = BT_TRANSPORT_LE;
        break;

      case BT_DEVICE_TYPE_DUMO:
        if (transport_p == BT_TRANSPORT_LE)
          transport = BT_TRANSPORT_LE;
        else
          transport = BT_TRANSPORT_BR_EDR;
        break;
    }
  }

Determining where device_type comes from led me down a deep rabbit hole that never yielded a clear answer. 😢

btif_config.cc#L399-413:

Screen Shot 2021-03-03 at 10 21 02 PM

Skimming through various parts of the source for how transport is used does indicate that its value impacts a lot of logic within the Android BLE system.

I had originally assumed that the device_type was based on how the peripheral identified itself, but the source code neither confirmed nor denied that assumption.

...I'll continue to do a bit of research. I'll also see what I can find out about the other platforms (Core Bluetooth and Web Bluetooth) to determine their default behavior.

@solvek
Copy link

solvek commented Apr 16, 2021

I am also for setting default transport to TRANSPORT_LE
I have created a fork of the project, changed default transport but currently strugling with compiling the library by myself.
How can I include my kable fork to my project?

@twyatt
Copy link
Member

twyatt commented Apr 16, 2021

@solvek Thanks for voicing interest in this issue. I've been swamped with other work related tasks, but I'll try to dedicate some time to Kable this weekend to get this (and hopefully a few other issues) wrapped up for the next release.

In the mean time, to use your fork of Kable in your project: there are a number of ways of approaching it, but for quick testing and local usage, performing a local Maven install might be what you're looking for?

./gradlew publishToMavenLocal

The above command will publish to your Maven local as version main-SNAPSHOT unless you specify a version manually, e.g. ./gradlew publishToMavenLocal -PVERSION_NAME=0.0.1-SNAPSHOT (SNAPSHOT suffix is necessary to disable publication signing).

Then in your Gradle project where you want to use your forked version:

repositories {
    mavenLocal()
    // ..
}

// depending on your project configuration, you may need to place this in the appropriate sourceSet directive
dependencies {
    implementation("com.juul.kable:core:main-SNAPSHOT")
}

@twyatt
Copy link
Member

twyatt commented Apr 17, 2021

A SNAPSHOT build is now available with support for configuring the transport.

repositories {
    maven("https://oss.sonatype.org/content/repositories/snapshots")
}

dependencies {
    implementation("com.juul.kable:core:0.4.1-issue-61-SNAPSHOT")
}

The CoroutineScope.peripheral extension function now has optional transport and phy parameters. Example usage:

import com.juul.kable.Transport.Le
import com.juul.kable.Phy.Le1M

val peripheral = scope.peripheral(advertisement, transport = Le, phy = Le1M)
peripheral.connect()

The transport default is currently Auto. Further investigation is needed to how Core Bluetooth (e.g. iOS) handles the transport distinction (before providing a similar API for Apple targets).

For now, Auto was chosen as the default to remain consistent with Android's BluetoothDevice.connectGatt behavior (when using signature without transport parameter) and the behavior of previous Kable versions. The default may be changed to Transport.Le in a future of version of Kable.


Please give the SNAPSHOT build a try and report any issues with it. Thank you!

@twyatt
Copy link
Member

twyatt commented Apr 27, 2021

📢   UPDATE: Transport.Le will be the default for Android platform (when not specified).

Looked into it a bit more, it appears that Web Bluetooth doesn't support non-LE profiles. With Core Bluetooth (e.g. iOS) not allowing for customization of the transport it didn't feel as necessary to default to Auto.

In other words, Transport.Le felt the best for keeping parity between the behavior of all the platforms supported (that and the feedback that it is more reliable on Android 😄 ).

@twyatt
Copy link
Member

twyatt commented Apr 28, 2021

Support for configuring transport (with default as LE) added in 0.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants