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

Connect with timeout waits unnecessarily long #70

Open
johanlunds opened this issue Sep 9, 2022 · 2 comments
Open

Connect with timeout waits unnecessarily long #70

johanlunds opened this issue Sep 9, 2022 · 2 comments

Comments

@johanlunds
Copy link

johanlunds commented Sep 9, 2022

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
Copy link
Author

johanlunds commented Sep 9, 2022

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
Copy link
Author

johanlunds commented Sep 10, 2022

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

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.

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

No branches or pull requests

1 participant