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

Conversation

twyatt
Copy link
Member

@twyatt twyatt commented May 21, 2020

Changes onConnectionStateChange property to be backed by StateFlow instead of ConflatedBroadcastChannel:

Conceptually state flow is similar to ConflatedBroadcastChannel and is designed to completely replace ConflatedBroadcastChannel in the future.

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #66 into develop will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop      #66      +/-   ##
=============================================
+ Coverage      80.46%   80.70%   +0.23%     
  Complexity        36       36              
=============================================
  Files             13       13              
  Lines            256      254       -2     
  Branches          28       26       -2     
=============================================
- Hits             206      205       -1     
+ Misses            43       42       -1     
  Partials           7        7              
Impacted Files Coverage Δ Complexity Δ
core/src/main/java/gatt/CoroutinesGatt.kt 50.00% <100.00%> (ø) 10.00 <1.00> (ø)
core/src/main/java/gatt/GattCallback.kt 84.61% <100.00%> (+0.83%) 17.00 <1.00> (ø)
core/src/main/java/gatt/GattConnection.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
core/src/main/java/gatt/Events.kt 81.69% <0.00%> (+1.40%) 0.00% <0.00%> (ø%)

@twyatt twyatt added this to the 1.0.0 milestone May 21, 2020
@twyatt twyatt merged commit 5da3384 into develop May 21, 2020
@twyatt twyatt deleted the twyatt/onConnectionStateChange/StateFlow branch May 21, 2020 16:55
Comment on lines +30 to +32
private val _onConnectionStateChange = MutableStateFlow<OnConnectionStateChange?>(null)
val onConnectionStateChange: Flow<OnConnectionStateChange> =
_onConnectionStateChange.filterNotNull()
Copy link

@AdamGrzybkowski AdamGrzybkowski May 21, 2020

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

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)

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

Successfully merging this pull request may close these issues.

3 participants