-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Add implementation for WebBluetooth #204
base: dev
Are you sure you want to change the base?
Conversation
You're going to need to undo the revert commits. Tokio doesn't have runtime support for WASM, but utilities are fine, you'll just need to fix up the attributes we bring tokio in with. I've been running a WASM library for quite a while using the tokio stream strategy here, it works fine. |
Sure. As I wrote this is just an overview of the required changes.
Do you have any open source examples for that? When I add the Do you have any feedback beside the obvious todos? What do you think about e.g. the |
src/wasm/adapter.rs
Outdated
} | ||
} | ||
|
||
pub(crate) async fn add_inital_peripherals(&self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: initial
src/wasm/adapter.rs
Outdated
} | ||
|
||
#[macro_export] | ||
macro_rules! spawn_local_and_wait { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a macro rather than just a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had some troubles to get it compiled and didn't check the details, since the whole PR is still WIP and I want to get some feedback about the general idea first. I want to know how to implement the specifics for WebBluetooth first, before cleaning everything up. The main issue is still the additional requirement for a list of supported service UUID, which would need some additional API changes and if my changes with PeripheralId
would be accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like PeripheralId
could work, though it should be an opaque wrapper type on all platforms rather than sometimes an alias for BDAddr. Actually on MacOS we currently make up a fake MAC address (see uuid_to_bdaddr
) because CoreBluetooth doesn't give us the MAC address. Having an opaque peripheral ID type might also let me simplify some parts of the Linux implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting the id
-String
from WebBluetooth to a BDAddr
like for MacOS doesn't seam a good idea to me. What do you suggest as an "opaque wrapper"? A String
everywhere? Or a Vec<u8>
? Or something like:
struct PeripheralId {
#[cfg(not(target_arch = "wasm32"))]
inner: BDAddr,
#[cfg(target_arch = "wasm32")]
inner: String,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean a struct with a private field, something like that, but defined under each platform module and then re-exported. So in src/wasm/mod.rs
you'd have something like:
struct PeripheralId(String);
or whatever you like, while src/corebluetooth/mod.rs
we might have
struct PeripheralId(Uuid);
and then it would be re-exported with a common name in src/platform.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would lead to some kind of layering-violation, since the platform
sources depend on the api
sources, which need the PeripheralId
. What do you suggest to resolve this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for reference; I currently depend on being able to convert a BDAddr into a u64 and back for more practical marshalling of addresses across an ffi boundary, between C# and Rust for a Unity plugin.
A more open-ended vec/string representation would make things a little awkward here since it potentially introduces more fiddly memory mangement sending addresses to C#. So long as the address is hashable I could hide this fairly easily by maintaining my own integer addresses that map to the real address but maybe it could even be useful if btleplug would keep backend address representation an implementation detail and always just return a u64 address - and then just leave it up to the backend to map that to something. For winrt, corebluetooth, bluez the native address can be packed in a u64, or wasm a hash table could be used in the backend to map the string ids to unique integers that get handed out to applications?
Arguably that's kind of what we already have I suppose, and I guess wasm could have a backend index of string addresses and look up a BDaddr from that to hand out for the public API. A unique BDAddr can be generated from a monotonically incremented u64 (BDAddr already has conversion to/from u64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The macOS implementation already requires the usage of 128bit UUIDs, so the existing implementation is not 100% correct.
- What's the benefit of maintaining the map in btleplug than in the ffi wrapper? What's the problem with wrapping the new
PeripheralId
in ffi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof yeah actually the current uuid mapping for the corebluetooth backend isn't great :)
pub(crate) fn uuid_to_bdaddr(uuid: &str) -> BDAddr {
let b: [u8; 6] = uuid.as_bytes()[0..6].try_into().unwrap();
BDAddr::try_from(b).unwrap()
}
That's just taking 48 bits from the string representation of the uuid :)
There's no particularly special benefit to having a mapping like I was suggesting inside btleplug; I was just thinking that if the wasm backend is the only backend that needs anything more than a 64bit ID it could be convenient to maintain that ability to reliably convert addresses to u64. Seeing that the corebluetooth backend can't really convert device IDs to a u64 I'm not so sure about this constraint now.
Looking at the Android API (which I'm hoping to use later) I see they simply report the device address as a string. They document that it's the mac address and imply you can rely on it being in a standard format but it also doesn't look like a really strong guarantee and it's maybe not good to assume that the string can be decoded:
public String getAddress ()
Returns the hardware address of this BluetoothDevice.
For example, "00:11:22:AA:BB:CC"
(https://developer.android.com/reference/android/bluetooth/BluetoothDevice#getAddress())
Tbh for my use case I could also quite easily just not pass the address to Unity/C# since it's not really something that needs to be shown to the user. I create a handle for the peripheral itself which serves as a unique ID for interacting with the api over ffi. If needs be for showing the IDs to the user then passing a string across isn't that big of a deal.
I marked the |
3c6d529
to
97185e3
Compare
@paroga As someone who's taken an interest in this PR for some BLE Earbuds I want to make a web app for, I'd set the tokio dependency for each target except |
@shymega I personally would like to avoid Maybe @qdot could have a look at the current PR, so we first get it at least into the |
This PR shows the required changes to add support for WebBluetooth via the
wasm32
target.tokio
has no support for thewasm32
target (This is the reason for the two reverting commits).Peripheral::known_services()
).Since additional changes will be required for real support, the purpose of the first version to get feedback for the next review round. Therefore the code isn't 100% tidied up yet.