Various API updates #166
Various API updates #166
Conversation
Also move utils modules to separate files.
Also, a better name for string_to_maybe_string, and avoid the dead code warning for str_to_nsstring which is only used in tests.
By re-exporting the platform specific `Manager` and `Adapter` saves the user of the api from doing so. By type-aliasing `Adapter `as `ConnectedAdapter` for mac and windows, and adding a no-op `connect` the user doesn't have to care that only linux has a difference between the two.
the crate _serde_bytes_ is added as it's needed for Vec<u8>
Corrected the inverse store order. Added more display and parse options.
Is replaced by general tests on BDAddr.
| @@ -284,6 +284,9 @@ pub trait Central: Send + Sync + Clone { | |||
|
|
|||
| /// Returns a particular [`Peripheral`] by its address if it has been discovered. | |||
| async fn peripheral(&self, address: BDAddr) -> Result<Self::Peripheral>; | |||
|
|
|||
| /// Add a [`Peripheral`] from a MAC address without a scan result. Not supported on all Bluetooth systems. | |||
| async fn add_peripheral(&self, address: BDAddr) -> Result<Self::Peripheral>; | |||
There was a problem hiding this comment.
What does it mean to add a peripheral? Add to what?
There was a problem hiding this comment.
So, let me explain the rationale behind this.
On Android, scanning for Bluetooth devices requires permission to access location information (though it looks like they're fixing this in Android 12.) To make this less cumbersome for developers, Android provides the Companion Device Manager API, which basically tells the Android OS "scan for Bluetooth devices yourself, prompt the user to select one, and tell me which one they selected." Unlike the scanning API, this does not require location permissions.
Once the app obtains this handle (which contains the MAC address information), either by prompting the user through the Companion Device Manager or by saving the MAC address and then retrieving it from storage later on, it can then connect to the device simply by providing the MAC address:
BluetoothDevice device = BluetoothAdapter.getDefaultAdapter().getRemoteDevice("12:34:56:78:90:AB");This new API is intended to make this functionality possible for btleplug clients on Android.
There was a problem hiding this comment.
This also becomes an interesting question in WebBluetooth with the upcoming getDevices() (or whatever it is) API, which I believe may allow similar?
There was a problem hiding this comment.
Would the app call the Companion Device Manager API directly, without btleplug involvement? And does this need to be a separate method from peripheral, just above? Either way, what you're really doing is getting a peripheral which the system already knows about, it's just a question of whether you are able to know about it beforehand. Perhaps they could be the same method?
There was a problem hiding this comment.
Would the app call the Companion Device Manager API directly, without btleplug involvement?
Yes. The Companion Device Manager involves the use of an Activity and other things which are far above the level of btleplug.
My Android port also has an Adapter::report_scan_result() method, which takes an android.bluetooth.le.ScanResult (which can be obtained from the CDM) and applies it to the device as if the information in it had come from its own scan.
There was a problem hiding this comment.
Either way, what you're really doing is getting a peripheral which the system already knows about, it's just a question of whether you are able to know about it beforehand. Perhaps they could be the same method?
In this branch, Adapter::peripheral() assumes the peripheral is already in the cache, while Adapter::add_peripheral() adds it if it's not already there. I suppose they could be merged into one method.
There was a problem hiding this comment.
I suppose they could be merged into one method.
On second thought, I think it's better to be able to make the distinction between "get the peripheral only if it's already in the cache" and "add it by MAC address if it doesn't already exist". I'd prefer to keep add_peripheral() after all (I'm open to feedback on better names.)
| @@ -147,7 +147,7 @@ pub struct PeripheralProperties { | |||
| /// The address of this peripheral | |||
| pub address: BDAddr, | |||
| /// The type of address (either random or public) | |||
| pub address_type: AddressType, | |||
| pub address_type: Option<AddressType>, | |||
There was a problem hiding this comment.
Having an address without an AddressType seems problematic, isn't it required to know how to interpret the address?
There was a problem hiding this comment.
The problem is that not all Bluetooth systems report the address type. Android does not, and in fact, neither do WinRTBLE and CoreBluetooth - out of the systems we use, the only one that does is BlueZ. Rather than pretending to know the type and guessing, BTLEPlug should simply be honest that it doesn't know, and let the client decide what to do with that information.
I've updated WinRTBLE and CoreBluetooth to reflect this.
| @@ -186,7 +186,7 @@ pub trait Peripheral: Send + Sync + Clone + Debug { | |||
|
|
|||
| /// Returns the set of properties associated with the peripheral. These may be updated over time | |||
| /// as additional advertising reports are received. | |||
| async fn properties(&self) -> Result<PeripheralProperties>; | |||
| async fn properties(&self) -> Result<Option<PeripheralProperties>>; | |||
There was a problem hiding this comment.
If you don't know the properties yet, can't you just use PeripheralProperties::default()? The fields (other than the address) are all either optional or can be empty or 0 anyway.
There was a problem hiding this comment.
My rationale behind this was to honestly report "we don't know yet" instead of "it's empty". Though, now that I look more closely at PeripheralProperties, I do see the has_scan_response field, which I guess reports this already. Still, in order to conform to Rust conventions, I'd argue in favor of making PeripheralProperties an Option and removing the has_scan_response field.
There was a problem hiding this comment.
I'm in agreement on Option versus default. I'd rather know the surrounding state than just getting back something with nothing in it and not knowing if that's what the device reported.
Codecov Report
@@ Coverage Diff @@
## dev #166 +/- ##
==========================================
+ Coverage 14.75% 20.54% +5.78%
==========================================
Files 24 12 -12
Lines 3836 2473 -1363
==========================================
- Hits 566 508 -58
+ Misses 3270 1965 -1305
Continue to review full report at Codecov.
|
Some Bluetooth systems allow you to obtain a handle to a peripheral without getting a scan result from it, just by providing a MAC address. Make properties() optional to reflect the fact that the properties may not be known.
Not all Bluetooth systems provide information about the address type. Make it optional, and return None instead of pretending to know.
Some Bluetooth systems allow you to obtain a handle to a peripheral without getting a scan result from it, just by providing a MAC address. Add an optional method to do this.
Converting an Error into a String loses information such as source and downcasting. Use a Box<dyn Error> instead to preserve this information.
|
I realize everything might not be perfect here, but perfect has currently been the enemy of done for 6 months now and I'd like to slide this into 0.8.0 so we can possibly add android as either 0.8.x or maybe 0.9 if there's enough stuff in front of it to need surface changes. Cherry-picking onto dev and closing. |
These changes are being made to accommodate the Android Bluetooth API.