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

Remove Central and simpify/unify API #24

Merged
merged 16 commits into from
Dec 9, 2020
Merged

Remove Central and simpify/unify API #24

merged 16 commits into from
Dec 9, 2020

Conversation

twyatt
Copy link
Member

@twyatt twyatt commented Dec 9, 2020

Decouples components from Central paradigm (Central has been removed).

API was simplified and unified by making the following changes:

  • MacOS target holds an internal CentralManager
  • Android target uses AndroidX startup to register an Initializer (and obtain application Context)

Scanner is no longer scoped, so Peripheral remains the only Coroutine scoped component. This allows library consumers more flexibility about the lifecycle of the various components.

Updated README can be viewed here.

@twyatt twyatt changed the title Twyatt/scope Remove Central and simpify/unify API Dec 9, 2020
@twyatt twyatt marked this pull request as ready for review December 9, 2020 11:06
@twyatt

This comment has been minimized.

README.md Outdated
Comment on lines 21 to 23
val example = scanner()
.advertisements
.first { it.name?.startsWith("Example") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention that all advertising peripherals will be in the flow, and filtering is done after they've been scanned? (as opposed to passing a filter to the scanner itself, like it is traditionally done in Android?)

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, yes, all filtering is to be done at the Flow level. Not ideal, but good enough for MVP.

The plan is to add additional configuration support to Scanner (follow the builder pattern), similar to how Json works in kotlinx.serialization.

So, for platforms that support it, filtering could be done at the scanner level via an API similar to:

val scanner = Scanner {
    services = arrayOf(...)
    priority = Low
}

See also: #22

core/src/androidMain/kotlin/Peripheral.kt Outdated Show resolved Hide resolved
import kotlin.js.Promise

private val bluetooth: Bluetooth
get() = js("window.navigator.bluetooth") ?: error("Bluetooth unavailable")
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works because we're targeting js().browser() correct? Supporting non-browser based JS environments would require a different approach, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Good point. I don't believe this will work for Node.js.
Looks like Noble library does it via some kind of Node.js bindings mechanism.

https://github.com/noble/noble/blob/c4cd18a7a429bb832f6c4ca793be3faf9af884e9/lib/webbluetooth/bindings.js#L28-L45

...something to look into in the future. Tracking via #25.

@cedrickcooke

This comment has been minimized.

@twyatt
Copy link
Member Author

twyatt commented Dec 9, 2020

Complete with the exact name match on the interface.

🤦 I'm not sure what I was doing wrong, but when I tried this earlier it wasn't compiling. Seems to be working now, this was my preferred approach. So, I'm glad it seems to be working now. 😄

README.md Outdated Show resolved Hide resolved
@twyatt twyatt requested a review from sdonn3 December 9, 2020 22:18
@twyatt twyatt requested a review from sdonn3 December 9, 2020 22:23
@@ -27,6 +28,10 @@ import org.w3c.dom.events.Event as JsEvent

private const val GATT_SERVER_DISCONNECTED = "gattserverdisconnected"

public actual fun CoroutineScope.peripheral(
advertisement: Advertisement,
): Peripheral = peripheral(advertisement.bluetoothDevice)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same implementation as the Android version's Peripheral implementation. I haven't seen much expected/actual implementations. Is it possible for disparate platforms to share implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this particular case. The bluetoothDevice property name is the same on both platforms, but the type is the platform specific type for each platform, so being able to have this as a single function in common is not possible unless the BluetoothDevice is defined as expect/actual.

Unfortunately, since the platforms diverge a bit in their representation of the underlying bluetooth device, it's probably better to have this function duplicated than deal with the potential complexity of trying to unify the bluetooth device platform representations.

@twyatt twyatt merged commit 58b0525 into main Dec 9, 2020
@twyatt twyatt deleted the twyatt/scope branch December 9, 2020 23:01
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

Successfully merging this pull request may close these issues.

5 participants