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

Add type PeripheralId #205

Merged
merged 2 commits into from
Sep 2, 2021
Merged

Add type PeripheralId #205

merged 2 commits into from
Sep 2, 2021

Conversation

paroga
Copy link
Contributor

@paroga paroga commented Aug 27, 2021

As requested by @qwandor in #204 (comment).

I changed the MacOS and Windows platform only as much as necessary, since I don't have a machine to test it.

src/api/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@qwandor qwandor left a comment

Choose a reason for hiding this comment

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

@qdot I think this is a good idea in general, and looks useful both for CoreBluetooth and WebBluetooth. Thoughts?

src/api/mod.rs Outdated Show resolved Hide resolved
src/corebluetooth/adapter.rs Outdated Show resolved Hide resolved
src/winrtble/peripheral.rs Outdated Show resolved Hide resolved
src/corebluetooth/peripheral.rs Outdated Show resolved Hide resolved
@paroga paroga force-pushed the pid branch 3 times, most recently from da4f29c to c83818d Compare August 27, 2021 20:55
@qwandor qwandor requested a review from qdot August 27, 2021 21:12
@paroga paroga force-pushed the pid branch 2 times, most recently from 0536db6 to 8cdbc9a Compare August 28, 2021 06:40
@rib
Copy link
Contributor

rib commented Aug 28, 2021

I wonder if the public api should even just reduce this down to letting you query the address as a string.

Although in general UIs will prefer to just show the name of a device the address is still a piece of information that can make sense to present in a UI to let someone disambiguate between multiple of the same device.

From an application's pov I guess a string ends up being the only really useful representation of the address?

Internally backends may want to have a less ambiguous representation of the address, e.g. BDAddr or a Uuid but that could just be an implementation detail?

I suppose the benefits of having an opaque struct are that 1) the backend might be able to have a slightly more optimal index of peripherals by address and 2) it makes sure you can only look up a device by an address that btleplug told you about (since you have no way of describing an address yourself).

If going with an opaque struct I think it could at least be good to ensure the struct implements fmt::Display for being able to show the address to a user if necessary.

@paroga
Copy link
Contributor Author

paroga commented Aug 28, 2021

If going with an opaque struct I think it could at least be good to ensure the struct implements fmt::Display for being able to show the address to a user if necessary.

You can always use adapter.peripheral(&your_peripheral_id)?.properties()?.local_name to get additional infos for the UI. Everything else would not show any reasonable data to the user.

@rib
Copy link
Contributor

rib commented Aug 28, 2021

Right the device name is what you would primarily show but I think the address can also be useful from a user pov to disambiguate if you have multiple devices that are the same.

@rib
Copy link
Contributor

rib commented Aug 28, 2021

(at least for me the only use case I have for the address being exposed in the public api is for showing to the user, so I'm not really sure what other use cases there are for the address besides that purpose. In terms of opaquely identifying a device doesn't having a Peripheral already do that?)

@qwandor
Copy link
Collaborator

qwandor commented Aug 28, 2021

@rib I think it makes sense to expose the address as an address, on platforms where it's available. If you want to convert it to a string to show to the user, sure, there's a method to do that. The thing to bear in mind is that it isn't actually available on all platforms. The address we were exposing on Mac OS was totally bogus.

I don't think it makes much sense for the opaque struct (i.e. PeripheralId) to implement Display, because it's not necessarily anything that makes sense to show to the user. On Windows it is an address, on Mac OS it's an arbitrary UUID that the OS assigns because it doesn't want to expose addresses to applications. On Linux it's currently an address, but I'm probably going to send a PR after this is merged to change it to be the D-Bus path as that will allow some optimisations.

Copy link
Contributor

@qdot qdot left a comment

Choose a reason for hiding this comment

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

Yeah I'm ok with the general structure of this. The only thing I might change is making PeripheralId an enum that could hold either BDAddr or Uuid instead of defining it per platform, but that would only be in the service of making tests easier to write and execute on single platforms when developing. That said we really don't have that many tests at the moment, so it's not a big deal, and could always be done later.

Some platforms do not use a BDAddr to identify a specific device. This new
type allows different platforms to use their own identifier.
@qwandor qwandor merged commit 15b45b9 into deviceplug:dev Sep 2, 2021
@paroga paroga deleted the pid branch September 4, 2021 09:13
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.

4 participants