Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Have onConnectionStateChange backed by StateFlow #66

Merged
merged 1 commit into from
May 21, 2020
Merged

Have onConnectionStateChange backed by StateFlow #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/gatt/CoroutinesGatt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class CoroutinesGatt internal constructor(
) : Gatt {

@FlowPreview
override val onConnectionStateChange = callback.onConnectionStateChange.asFlow()
override val onConnectionStateChange = callback.onConnectionStateChange

@FlowPreview
override val onCharacteristicChanged = callback.onCharacteristicChanged.asFlow()
Expand Down
15 changes: 12 additions & 3 deletions core/src/main/java/gatt/GattCallback.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package com.juul.able.gatt

import android.bluetooth.BluetoothGatt
import android.bluetooth.BluetoothGatt.GATT_SUCCESS
import android.bluetooth.BluetoothGattCallback
import android.bluetooth.BluetoothGattCharacteristic
import android.bluetooth.BluetoothGattDescriptor
Expand All @@ -17,13 +18,19 @@ import kotlinx.coroutines.channels.BroadcastChannel
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.channels.Channel.Factory.BUFFERED
import kotlinx.coroutines.channels.Channel.Factory.CONFLATED
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.runBlocking

internal class GattCallback(
private val dispatcher: ExecutorCoroutineDispatcher
) : BluetoothGattCallback() {

val onConnectionStateChange = BroadcastChannel<OnConnectionStateChange>(CONFLATED)
private val _onConnectionStateChange = MutableStateFlow<OnConnectionStateChange?>(null)
val onConnectionStateChange: Flow<OnConnectionStateChange> =
_onConnectionStateChange.filterNotNull()
Comment on lines +30 to +32
Copy link

@AdamGrzybkowski AdamGrzybkowski May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twyatt adding nullable field looks a bit hacky just to use StateFlow
you probably want ShareFlow but this one isn't out there yet

Copy link
Member Author

@twyatt twyatt May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it isn't ideal. I was torn on the best approach.

I considered exposing connection state as a simple sealed class as is being done in KeepAliveGatt:

sealed class State {
    object Connecting : State()
    object Connected : State()
    object Disconnecting : State()
    object Disconnected : State()
    data class Closed(val cause: Throwable?) : State()
}

But I worried this would diverge too much from the current onConnectionStateChange (in 0.7.x) that carries GATT status (e.g. GATT_SUCCESS) in addition to the connection state.

Being that OnConnectionStateChange holds both the GATT status and connection state does make it more like an event than a state; so at first, does make SharedFlow seem like a better fit.

I ultimately settled on StateFlow because I saw the "connection state" as the primary attribute of the OnConnectionStateChange, so I justified it as more of a state than event.

Making it nullable was necessary to simplify handling of the initial connection sequence:

https://github.com/JuulLabs-OSS/able/blob/5da33841058463f052a1489ddae6a7b23c072119/core/src/main/java/gatt/GattConnection.kt#L57-L69

During the connection sequence, if we see a STATE_DISCONNECTED then we can consider the connection attempt as failed. If the StateFlow were non-null and we started it with a STATE_DISCONNECTED then it would require more complexity to properly handle that case. Whereas making _onConnectionStateChange (internally) nullable means we can represent the absence of a connection state as null (before Android has called BluetoothGattCallback. onCharacteristicChanged).

Although it may appear a bit awkward, it was was suggested as a possible approach in: Kotlin/kotlinx.coroutines#2034 (comment)


val onCharacteristicChanged = BroadcastChannel<OnCharacteristicChanged>(BUFFERED)
val onResponse = Channel<Any>(CONFLATED)

Expand All @@ -38,9 +45,11 @@ internal class GattCallback(
if (isClosed.compareAndSet(false, true)) {
Able.verbose { "Closing GattCallback belonging to device ${gatt.device}" }
onDisconnecting() // Duplicate call in case Android skips STATE_DISCONNECTING.
onConnectionStateChange.close()
gatt.close()

_onConnectionStateChange.value =
OnConnectionStateChange(GATT_SUCCESS, STATE_DISCONNECTED)

// todo: Remove when https://github.com/Kotlin/kotlinx.coroutines/issues/261 is fixed.
dispatcher.close()
}
Expand All @@ -53,7 +62,7 @@ internal class GattCallback(
) {
val event = OnConnectionStateChange(status, newState)
Able.debug { "← $event" }
onConnectionStateChange.offer(event)
_onConnectionStateChange.value = event

when (newState) {
STATE_DISCONNECTING -> onDisconnecting()
Expand Down
16 changes: 4 additions & 12 deletions core/src/main/java/gatt/GattConnection.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import android.bluetooth.BluetoothProfile.STATE_DISCONNECTED
import com.juul.able.Able
import kotlinx.coroutines.FlowPreview
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.firstOrNull
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.onEach

/**
Expand Down Expand Up @@ -60,18 +60,10 @@ internal suspend fun GattConnection.suspendUntilConnectionState(state: GattConne
.onEach { event ->
Able.verbose { "← Received $event while waiting for ${state.asGattConnectionStateString()}" }
if (event.status != GATT_SUCCESS) throw GattStatusFailure(event)
if (event.newState == STATE_DISCONNECTED && state != STATE_DISCONNECTED) throw ConnectionLost()
}
.firstOrNull { (_, newState) -> newState == state }
.also {
if (it == null) { // Upstream Channel closed due to STATE_DISCONNECTED.
if (state == STATE_DISCONNECTED) {
Able.info { "Reached (implicit) STATE_DISCONNECTED" }
} else {
throw ConnectionLost()
}
}
}
?.also { (_, newState) ->
.first { (_, newState) -> newState == state }
.also { (_, newState) ->
Able.info { "Reached ${newState.asGattConnectionStateString()}" }
}
}
2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ ext.versions = [
ext.deps = [
kotlin: [
stdlib: "org.jetbrains.kotlin:kotlin-stdlib",
coroutines: "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.5",
coroutines: "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.7",
junit: "org.jetbrains.kotlin:kotlin-test-junit",
],

Expand Down