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

Peripheral blocking scope from finishing #378

Open
tomnovotny7 opened this issue Sep 8, 2022 · 2 comments
Open

Peripheral blocking scope from finishing #378

tomnovotny7 opened this issue Sep 8, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@tomnovotny7
Copy link

Bug report

When using scope.peripheral(...) it will block scope from finishing.

I am developing an Android application and I've tried versions 0.18.0, 0.17.2, 0.16.1 and unreleased version 0.18.0-issue-359-1-SNAPSHOT was crashing when using different contexts.

How to reproduce

A coroutine is not finished unless the job is manually canceled.

val device: BluetoothDevice = ...

viewModelScope.launch {
  val peripheral = peripheral(device)

  // some work, connecting and disconnection or nothing
}

Even worse when using a different context:

val device: BluetoothDevice = ...

viewModelScope.launch {
  withContext(Dispatchers.IO) {
    val peripheral = peripheral(device)

    // some work, connecting and disconnection or nothing
  }
  
  // unreachable code
}

Expected behavior

Either should not block scope and be closed automatically or should provide peripheral.close() method for closing the peripheral.

@twyatt twyatt added the bug Something isn't working label Sep 8, 2022
@twyatt
Copy link
Member

twyatt commented Sep 10, 2022

Thanks for the issue report! It is a good reminder that this portion of Kable is overdue for a redesign.

Short-term: I think providing peripheral.close() makes sense.

Long-term: I'd like to remove the reliance on a CoroutineScope within a Peripheral.

The nitty gritty details are: Initially CoroutineScope within a Peripheral was used as a crutch, to make passing data across threads simpler/feasible with the old memory model. I believe that with the new memory model, Kable can lean on the threads of the callbacks that are providing the various triggers that need to be acted on (rather than spinning up coroutines). For example, on Apple, we spin up a coroutine to monitor when BLE turns off (to disconnect peripherals) but we should be able to explicitly invoke the appropriate methods from the callbacks themselves (rather than emitting to a flow, to be collected and acted on). In the Android world, a coroutine is spun up to pass along characteristic change events from the current connection (which can change over time); instead of using the Peripheral's CoroutineScope, Kable should be using the scope of the collector of characteristic change flow (and it should switch to grabbing events from the current connection, which may change).

twyatt added a commit that referenced this issue Feb 3, 2023
As identified in #378, `Peripheral` objects eagerly lean on the
`CoroutineScope` receiver provided at `Peripheral` creation. This isn't
necessary and this is a small step in removing the crutches that were
being relied on.
twyatt added a commit that referenced this issue Feb 3, 2023
As identified in #378, `Peripheral` objects eagerly lean on the
`CoroutineScope` receiver provided at `Peripheral` creation. This isn't
necessary and this is a small step in removing the crutches that were
being relied on.
twyatt added a commit that referenced this issue Mar 13, 2023
As identified in #378, `Peripheral` objects eagerly lean on the
`CoroutineScope` receiver provided at `Peripheral` creation. This isn't
necessary and this is a small step in removing the crutches that were
being relied on.
@twyatt
Copy link
Member

twyatt commented Sep 25, 2023

I'm hoping to have the Android side of this issue resolved in 0.29.0, the other platforms will be fixed in later versions (I'll keep this issue open until problem is resolved on all platforms).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants