deviceplug / btleplug

Rust Cross-Platform Host-Side Bluetooth LE Access Library
Other
818 stars 151 forks source link

Should bluez Peripheral be private? #73

Closed alexeden closed 4 years ago

alexeden commented 4 years ago

I'm trying to interface with a Bluetooth drone controller from code running on a Raspberry Pi. I've got this struct (omitting all the unneeded details):

#[allow(unused_imports)]
#[cfg(any(target_os = "windows", target_os = "macos"))]
use btleplug::corebluetooth::peripheral::Peripheral;

#[allow(unused_imports)]
#[cfg(target_os = "linux")]
use btleplug::bluez::adapter::peripheral::Peripheral;

pub struct T1d {
  device: Peripheral,
}

Compiles fine on my mac, but the linux-equivalent of Peripheral in the bluez module is private, so it fails on the Pi.

Is it intentionally private? I should note, and I cannot stress this enough, I'm new to rust and have very little idea of what I'm doing. Any help is appreciated!

qdot commented 4 years ago

First off, don't worry, this is actually super confusing. As the README kinda says (and maybe I should put in there more directly), this is a bunch of squished together implementations that still need sorting out.

So actually, the Mac impl of Peripheral should be private, heh. You don't wanna bring in the platform peripheral implementation, but rather the peripheral trait.

For an example of what you want to bring in with a cross platform case, see https://github.com/buttplugio/buttplug-rs/blob/master/buttplug/src/server/comm_managers/btleplug/mod.rs#L24. I handle win/mac/linux there. It's a little weird but it works.

Ideally these platform-specific use statements should be handled in our library too. I'll add that as an issue, I just keep forgetting to do it. >.>

This is sort of the tl;dr answer, so lemme know if you'd like me to explain further.

ChrisCA commented 4 years ago

I also stumbled upon this for my usecase and I solved it by making the linux specific Peripheral public. Same as alexeden I wasn't sure about whether its private by design or because of fresh copy-pasta. Initinally I planned to use the trait Peripheral abstraction but with my (limited) Rust skills, I didn't managed to create what I need as boxing the trait object wasn't possible for me.

alexeden commented 4 years ago

@qdot Understood. Thanks for the quick response! This is probably more of a general-rust question, so feel free to defer me to the documentation: I tried using this as a reference, which seems to do exactly what I'm going for. The difference being that in my implementation, I'm getting the peripheral from within the new function, which gives me this error:

29 | impl<T: ApiPeripheral> T1d<T> {
   |      - this type parameter
...
65 |     Ok(Self { device: periph })
   |                       ^^^^^^ expected type parameter `T`, found struct `btleplug::corebluetooth::peripheral::Peripheral`
   |
   = note: expected type parameter `T`
                      found struct `btleplug::corebluetooth::peripheral::Peripheral`
qdot commented 4 years ago

Yeah, don't make Peripheral implementations public. That's not the way to go here, but figuring out how to do this does take some Rust style thinking to get the monomorphization correct.

To use peripherals, all we care about is the trait. The compiler will happily substitute the concrete type in via monomorphizing with the trait bounds, though. For instance, here's where I store a peripheral in one of my libraries:

https://github.com/buttplugio/buttplug-rs/blob/master/buttplug/src/server/comm_managers/btleplug/btleplug_device_impl.rs#L33

I use Option here because I may not have my peripheral 'til later, but Option isn't Sized, so the issues with boxing don't happen here and I get T back from Option inner type calls. This just requires binding T to Peripheral + 'static, which means "A type that has the Peripheral trait and lasts the lifetime of the process". When this is compiled, the actual private struct that has the Peripheral trait will be swapped in by the compiler and directly accessed, so you don't have to dynamic call through something like a Box.

Does that make sense?

qdot commented 4 years ago

BTW, if you want to poke me about this in real time, I have a discord server for supporting this library, link is in the README, or I'm on the Rust official and community discord servers.

alexeden commented 4 years ago

Totally does make sense, and that was my initial thought (that I shouldn't have to use a concrete type of a Peripheral... I can just use the trait and the compiler will know what's up). So now I'm thinking I probably just had a tiny but breaking detail somewhere in the code that was causing issues. Or because I wasn't wrapping it in an Option (or anything else for that matter). I'll reach out if my flailing persists. Thanks again!

qdot commented 4 years ago

A heads up for @ChrisCA since I just fixed this with @alexeden and I gave the wrong info because I was thinking about storage types, not return types.

If you have a function that returns a peripheral in an Option wrapper, you'll want to do something like

fn maybe_a_peripheral() -> Option<impl Peripheral>

Using the type specification (i.e. fn maybe_a_peripheral<T: Peripheral>() -> Option<T>) causes conflicts between the return type (which is a Real Peripheral, i.e. corebluetooth::Peripheral) and what the trait bound expected (just Peripheral, which is unsized and therefore unreturnable anyways). impl Peripheral gets you the resolution you want without boxing, even within the Option wrapper (since it'll just resolve out the type at compile).

ChrisCA commented 4 years ago

@qdot Thanks for your effort, but I've to admit that I didn't manage to achieve what I originally wanted. I even failed to provide a minimal sample. How can I achieve to "store" peripheral trait objects in a Vec? I can follow the example you provided up here but finally, I always end up at a point where I would need to define the type of my collection where it would be necessary, with the knowledge I have atm, to have access to the platform specific structs. Same as Alex, I and up with:

error[E0308]: mismatched types
  --> src\bt\bt_win.rs:50:50
   |
21 | pub fn init_bluetooth<T: Peripheral>(
   |                       - this type parameter
...
50 |                 cl_peripherals.extend_collection(&p);
   |                                                  ^^ expected type parameter `T`, found struct `btleplug::winrtble::peripheral::Peripheral`
   |
   = note: expected reference `&T`
              found reference `&btleplug::winrtble::peripheral::Peripheral`

If this is more of a general rust topic, I'm also happy with a short hint where I could do further read up on this topic. (even so my search so far wasn't very successful)

qdot commented 4 years ago

@ChrisCA Where/how is your peripheral collection defined? We keep Peripheral collections in btleplug internally, for instance, the peripherals map in the adapter manager: https://github.com/deviceplug/btleplug/blob/master/src/api/adapter_manager.rs#L20

So it is possible, but how you set up your types can sometimes be a little tricky.

ChrisCA commented 4 years ago

@qdot Thanks for the sample link with the adapter manager. It's represents my situation very well so let use this as a minimal sample. Please note that I'm on Windows now. If I want to setup an AdapterManager I run the new() method:

use btleplug::api::*;

fn main(){
    let manager = AdapterManager::new();
}

cargo check says:

error[E0283]: type annotations needed for `btleplug::api::AdapterManager<PeripheralType>`
 --> src\main.rs:4:19
  |
4 |     let manager = AdapterManager::new();
  |         -------   ^^^^^^^^^^^^^^^^^^^ cannot infer type for type parameter `PeripheralType`
  |         |
  |         consider giving `manager` the explicit type `btleplug::api::AdapterManager<PeripheralType>`, where the type parameter `PeripheralType` is specified
  |
  = note: cannot satisfy `_: btleplug::api::Peripheral`
  = note: required by `btleplug::api::AdapterManager::<PeripheralType>::new`

Until this point I can follow the logic as we created a generic type with the peripheral trait restriction. But how should I achieve an equivalent of this:

use btleplug::api::*;

fn main(){
 let manager: AdapterManager<btleplug::winrtble::peripheral::Peripheral> = AdapterManager::new();
}

If btleplug::winrtble::peripheral::Peripheral is private?

qdot commented 4 years ago

@ChrisCA Well, for that example specifically, it's because AdapterManager should be pub(crate), not pub. AdapterManager is made to be held by adapter objects, there's nothing you can do with it outside of the crate. I'll file a bug to fix this, it just went in the "api" module 'cause that's where most of the traits and stuff were, wasn't thinking about the exposure level.

qdot commented 4 years ago

Now filed as #74

ChrisCA commented 4 years ago

@qdot I understand. As I said, my setup is very similar to the one of the AdapterManager. This gives me the feeling that what I implement is one level below the scope of this library. I will grind a little bit more through other's code using btleplug. Maybe I can get a better approach than what I try to achieve at the moment. But thanks again for your input. :-)

qdot commented 4 years ago

@ChrisCA Yeah, def feel free to file issues, yell at me on twitter/discord, etc if you have more questions. I realize the API here right now is really odd, so it does take some working around sometimes.

It'll be better, someday. I hope. Maybe.