Refactor BDAddr #81

MattesWhite · 2020-09-28T14:37:08Z

This PR follows the discussion of #68.

At last I found some time to work on this. For now I refactored BDAddr as a show case what I intend to do. If the implementation is approved I'll start working on UUID. Of course I'll include the changed suggested in #80 as well.

Initially, I wanted to write the parsers with nom but I recognized that there is a bunch of nom v4 code in the crate so I settled for a 'hand-crafted' version. Another solution would be to use regex but I didn't wanted to pull in another dependency without asking.

qdot · 2020-10-05T05:11:27Z

I definitely like the direction this is going, though do note that your current version is failing on macOS and Windows.

I think you're fine to move on to UUID, though I did bring in #80 (as I'm about to do a quick point release to fix Windows UWP reads), but you can integrate that in as you need.

The nom dependency thing is killing me too, I'm really hoping to get back to this one day and possibly just move us to actual dbus usage. :(

I don't think bringing in regex would be a huge issue? But being an ex browser dev I'm also kind of oblivious to compile times and all that, and the library I use this for is... pretty violently monolithic so I have like 30+ dependencies there. :)

MattesWhite · 2020-10-09T07:20:18Z

The BDAddr part is now finalized. All pipelines should only fail because of legacy code by now.

A question of style: Do you prefer having bigger modules (src/api/mod.rs has now more than 500 lines without tests) or smaller once, e.g. put BDAddr and UUID into submodules and re-export them? Personally, I'd prefer the latter but I'll align with your choice of style.

Once, I did a refactoring from nom v4 to nom v5 so I'm willing to take care of this. However, the issue of having no RSSI values available (#76) is way more pressing for me. In addition, there is already worked on nom v6 which probably will improve support for custom errors so waiting a little bit and solving #48 first is maybe not that bad.

For now I'll focus on refactoring UUIDs.

qdot · 2020-10-10T05:37:50Z

Looks like everything built on the latest commit, so that's positive.

And smaller modules, please! The big modules came in with the libraries I imported, I'm much more a fan of submodules and re-exporting.

For the nom stuff, it's really hard to say how much that matters other than the conflicting dependencies, especially if we start looking toward dbus again for bluez. The RSSI is now important for me too (My library that depends on this now advertises RSSI relay capabilities, and yet, I have no RSSI here... >.> ), so hopefully that can be looked at soon.

MattesWhite · 2020-10-12T11:38:03Z

So first work on UUIDs is done. I decided that its most practical to use uuid::Uuid wherever possible but I kept the short-value variant which has nice benefits like use parse and format functions of uuid crate.

I can't guarantee that the Windows version still works or that I didn't introduce bugs. I tried to adapt what was possible but when I the need to reverse bytes here and there makes my head hurt. In addition, I have only a Linux machine to test, so I don't even know if it compiles. Is there a way to do (at least) a cargo check for another OS? All in all, it would be really nice if someone could verify the code on a Windows machine.

qdot · 2020-10-12T22:02:41Z

After the issues I had with the macOS read/write patches, I'll be testing this throughly across all platforms before landing, because it directly affects users of my libraries that depend on this. 👍

So far, according to CI it looks like the only thing catching you on windows is a &u16 to u32 miscast, so not too horrible?

MattesWhite · 2020-10-13T06:48:16Z

Yes the fixing errors caught by the pipelines is easy but I don't wanted to fix each error separately, push a single commit, wait for CI, iterate and so on. This also costs pipeline time, I think? What really bothers me is that I often read that Windows GUIDs are little endian and the uuid::Uuid is big endian so I can't guarantee that the conversion functions do the right thing. And writing tests without IDE support is not very pleasant.

qdot · 2020-10-13T07:10:20Z

Well, looks like you pass CI now at least. And I do all my development on windows (and the vast number of users of this library are also on windows, and probably don't know they're using this library), so like I said, I'll at least give it a runthru before landing. :)

johnmave126 · 2020-11-12T22:21:43Z

Actually, how about using eui48 for mac address? That crate seems pretty slim and seems to be treated as de-facto mac address crate in crate like tokio-postgres.

qwandor · 2021-02-20T00:59:08Z

@MattesWhite Hi! Any chance you could rebase the BDAddr changes against the latest dev branch? The diff is a bit hard to follow at the moment.

I've done something a bit similar for UUIDs in #123, but using the type from the uuid crate directly in the API and internally, which makes equality work properly, and then using an extension trait and a helper function to convert to and from the short form where needed.

NZSmartie · 2021-03-07T01:07:24Z

I've rebased it on to the current dev branch and omitted all UUID related changes. It did drop the useful UUID constants you had defined, but I'm sure we could add that in another PR, that way this PR only focuses on BDAddr

NZSmartie · 2021-03-07T01:11:03Z

src/corebluetooth/adapter.rs

+    let b: [u8; 6] = uuid.as_bytes()[0..6].try_into().unwrap();
+    BDAddr::try_from(b).unwrap()


One of refactors that didn't quite survive the rebase was this. It may be worth reviewing as I ended up applying a dirty fix so that macOS/iOS would still build

src/winrtble/utils.rs

MattesWhite · 2021-03-08T16:07:26Z

Hi all,

sorry for the late response. Lately I don't have much time to monitor the progress of btleplug. However, I'm very exited to see the progress on async 👍 .

Feel free to edit this PR as much as you like. I'm happy that at least some pieces of my code end up in the crate. If there are any questions I'll be happy to help.

src/api/mod.rs

src/lib.rs

src/api/bdaddr.rs

qwandor · 2021-03-08T20:08:17Z

src/api/bdaddr.rs

+        Ok(Self { address })
+    }
+    /// Writes the address without delimiters.
+    pub fn write_flat(&self, f: &mut impl fmt::Write) -> fmt::Result {


Is this actually used anywhere? I'd be inclined to leave it and to_string_flat out unless you have a usecase in mind.

Actually this was my initial motivation to write this PR (see #68 (comment)). I'm working on a 'Bluetooth gateway' where the BDAddr of devices is used as the identifier in a REST API. As the default : in MAC addresses is also a valid separator in URIs I use this flat format. eui48 provides a similar format.

Don't know if this is enough to justify its existence in btleplug.

At least I renamed it to _no_delim instead of _flat

src/api/bdaddr.rs

qwandor · 2021-03-08T20:11:29Z

src/api/bdaddr.rs

+        let mut address = [0; 6];
+        for i in (0..12).step_by(2) {
+            let part = &s[i..i + 2];
+            address[i / 2] = u8::from_str_radix(part, 16).expect("Checked upfront");


Rather than checking twice, remove the is_ascii_hexdigit check above, and return the error here instead.

qwandor · 2021-03-08T20:12:11Z

src/api/bdaddr.rs

+impl From<u64> for BDAddr {
+    fn from(int: u64) -> Self {
+        let mut cpy = [0; 6];
+        let slice = int.to_be_bytes(); // Reverse order to have MSB on index 0


Check that the top two bytes are 0.

Is this really a problem/error? Would you rather use TryFrom?

I think that's better. Done this myself.

qwandor · 2021-03-08T20:17:49Z

I think that this is generally an improvement, once the a bunch of small things are fixed. However, we could consider instead using one of the existing crates which provides a standard MAC address type, such as eui48 or macaddr. I haven't had a chance yet to see which of the two is a better choice.

src/api/bdaddr.rs

Corrected the inverse store order. Added more display and parse options.

Is replaced by general tests on BDAddr.

MattesWhite marked this pull request as ready for review October 13, 2020 06:48

darrinsmart mentioned this pull request Feb 14, 2021

Panic via Uuid::from_str() (Corebluetooth, notifications) #119

Closed

NZSmartie force-pushed the refactor-uuid-and-bdaddr branch from cde5f12 to 39000b2 Compare March 7, 2021 01:05

NZSmartie changed the base branch from master to dev March 7, 2021 01:05

NZSmartie changed the title ~~Refactor BDAddr and UUID~~ Refactor BDAddr Mar 7, 2021

NZSmartie force-pushed the refactor-uuid-and-bdaddr branch from 39000b2 to a1309d7 Compare March 7, 2021 01:09

NZSmartie reviewed Mar 7, 2021

View reviewed changes

qwandor reviewed Mar 7, 2021

View reviewed changes

src/winrtble/utils.rs Show resolved Hide resolved