BleGattCoroutines icon indicating copy to clipboard operation
BleGattCoroutines copied to clipboard

Connect with timeout waits unnecessarily long

Open johanlunds opened this issue 3 years ago • 2 comments

Hi,

My issue is that sometimes connecting fails quickly with status = 133. What happens then is that connect and withTimeout waits unnecessarily long.

I have connect code with timeout and stateChanges flow code as recommended. Like this:

var connection: GattConnection? = null

suspend fun connect() {
    val connection = GattConnection(...)
    this.connection = connection
    
    // Listen for disconnect events (and other state changes):
    MainScope().launch {
        connection.stateChanges.collect {
            if (it.newState == BluetoothProfile.STATE_DISCONNECTED) {
                disconnect() // Update UI...
            }
        }
    }
    
    val connected = try {
        withTimeout(5000) {
            connection.connect()
        }
        true
    } catch (e: TimeoutCancellationException) {
        false
    } catch (e: CancellationException) {
        false
    } catch (e: GattException) {
        false
    }

    if (connected) {
        // Connect succeeded! Update UI here...
    } else {
        // Connect failed! Update UI here...
    }
}

fun disconnect() {
    connection?.close()
    // Update UI here...
}

What happens for me with status 133 is this:

  1. A state change is triggered quickly with status 133, so disconnect() above is executed.
  2. withTimeout will wait the full 5 seconds
  3. Then after 5 seconds it times out with a TimeoutCancellationException
  4. The code // Connect failed! Update UI here... above is executed

Ideally I'd like connect and withTimeout to cancel immediately and the connect call to fail immediately when the connect fails.

Am I doing something wrong?

Do you have any recommendations how the code should be instead?

Thank you for a very nice library.

johanlunds avatar Sep 09 '22 20:09 johanlunds

I managed to get some code working now that does what I want it to (I think it's correct at least), but it feels a bit hacky and I'm not sure this is the best way:

val scope = CoroutineScope(Dispatchers.Main)
val connect = scope.async {
    withTimeout(5000) {
        connection.connect()
    }
}
val disconnected = scope.async {
    connection.stateChanges.first {
        it.newState == BluetoothProfile.STATE_DISCONNECTED
    }
}
// Wait on the first job to complete:
val connected = select<Boolean> {
    connect.onAwait { true }
    disconnected.onAwait { false }
}
if (connected) {
    // Connected!
} else {
    // Connect failed due to disconnect...
}

I would love feedback on how to improve this!

johanlunds avatar Sep 09 '22 22:09 johanlunds

Here is how a change to GattConnection.connect() to make it fail fast could look like:

https://github.com/Beepiz/BleGattCoroutines/compare/main...johanlunds:johanlunds-connect-change

-        isConnectedFlow.first { connected -> connected }
+        
+        // Alternative #1: Use isConnectedFlow with accompanying change below: 
+        val isConnected = isConnectedFlow.first()
+        if (!isConnected) throw GattException("Connect operation failed, got disconnected.")
     private val callback = object : BluetoothGattCallback() {
         override fun onConnectionStateChange(gatt: BG, status: Int, newState: Int) {
-            when (status) {
-                STATUS_SUCCESS -> isConnected = newState == BluetoothProfile.STATE_CONNECTED
-            }
+            // Alternative #1: Change the logic here to also set isConnected on failure statuses, something like:
+            isConnected = (status == STATUS_SUCCESS && newState == BluetoothProfile.STATE_CONNECTED)

There's probably a reason it works like it does today that escapes me, so please feel free to let me know.

johanlunds avatar Sep 10 '22 10:09 johanlunds