BlueZ: Replace raw HCI sockets with BlueZ over DBus #105

NZSmartie · 2021-01-10T08:22:13Z

This is a complete rewrite of the "bluez" platform specific code.

A lot has happened and I did my best to commend code to explain how/why a well as keep commits subjective so that changes can be understood in context.

Things I want to do before merging:

~~Verify I didn't break the existing API~~ (Maintaining a backwards compatible API is a lot harder than I thought and ends up with a lot of messy dead code)
Ensure both passive and active discovery work as intended
Ensure the code isn't too terrible looking
Test this library against multiple versions of BlueZ to see what fails where

Closes #60, #69

NZSmartie · 2021-01-11T00:40:43Z

I had a look through other open issues and found two related issues that will be resolved with this PR

NZSmartie · 2021-01-17T02:57:27Z

@qdot, can you add libdbus-1-dev to Linux's installed packages on the CI pipeline?

NZSmartie · 2021-01-17T23:04:18Z

I've done what I can to test this PR, and I'm now relying on others to help test it. Since it works for me™, I'm happy to submit future PRs to fix bugs or other issues that I've yet to face

As well as some other house cleaning 🧹

I also had a misplaced `!`

NZSmartie · 2021-01-19T01:31:08Z

just rebased on to master

lnicola · 2021-01-21T18:01:55Z

Also closes #52.

qdot · 2021-01-21T18:08:44Z

Awesome, thanks for the updates. I am definitely trying to get to reviewing this, hopefully in the next day or two.

qdot · 2021-01-21T18:09:00Z

(Note: This is permission to yell at me in these here comments if I don't get to it soon :3 )

qdot

This is all looking really good! I've just got a few comments I'd like addressed, then it can land.

qdot · 2021-01-23T17:45:14Z

src/bluez/adapter/mod.rs

+                let lock = listener.lock();
+                while lock.process(Duration::from_secs(0)).unwrap() {}
+                drop(lock);
+                thread::sleep(Duration::from_millis(100));


Where'd the 100ms come from here? BLE packet timings can be less than that, so this may lag the system some. I think it could be reduced, would just like to know if there was a source for this.

Also, as the Drop trait needs to kill this, this should be a cancellable sleep. Maybe change it to a condition with a timed wait? That way you aren't blocking the join in the Drop.

no source for this. I was going for a way to quickly process any messages there may be, and then sleeping on the thread for 100ms, so that other threads have a chance to use the listener to send/receive

Looking back at my code and later learning about parking_lot I was dealing with mutex fairness, where, the while loop was constantly locking the mutex, not giving any other threads a chance to get a lock, and thus, anything that needed listener would block.

I'll see if parking_lot::ReentrantMutex promotes fairness

qdot · 2021-01-23T17:45:56Z

src/bluez/adapter/mod.rs

+
+                    if args.interfaces.iter().any(|s| s == BLUEZ_INTERFACE_DEVICE) {
+                        adapter.remove_device(&path).unwrap();
+                    } else if args.interfaces.iter().any(|s| s == BLUEZ_INTERFACE_SERVICE) {


What should happen here? Can you at least comment the null blocks here and below to let us know whether these are null for a reason or just still need to be filled in?

qdot · 2021-01-23T17:47:31Z

src/bluez/adapter/mod.rs

-            handle_map: Arc::new(DashMap::new()),
-            manager: AdapterManager::new(),
-        };
+    pub fn is_up(&self) -> Result<bool> {


Is there a situation where the adapter would be powered, but not up? Or would this throw if the adapter instance was for a device that has been disconnected?

qdot · 2021-01-23T17:49:42Z

Cargo.toml

@@ -36,6 +37,7 @@ uuid = "0.8.2"
 serde = { version = "1.0.119", features = ["derive"], default-features = false, optional = true }
 dashmap = "4.0.2"
 futures = "0.3.12"
+parking_lot = "0.11.1"

 [dependencies.nom]


Can we kill the nom dep here now?

qdot · 2021-01-23T17:58:25Z

src/bluez/adapter/mod.rs

+            .map(|(path, attribute)| Ok(self.add_attribute(path.as_str().unwrap(), attribute)?))
+            .collect::<Result<()>>()?;
+
+        // TODO: Descriptors are nested behind characteristics, and their UUID may be used more than once.


I think there's an issue about descriptor requests, we should probably call this out there. I haven't even looked at other platforms yet.

qdot · 2021-01-23T18:11:08Z

src/bluez/adapter/mod.rs

    }

-    pub fn connect(&self) -> Result<ConnectedAdapter> {
-        ConnectedAdapter::new(self)
+    fn active(&self, _enabled: bool) {


Here and elsewhere, can we get some comments on the methods that need to be filled in? Makes it easier to file help wanted bugs or at least say issues are claimed later, if we know what needs to be filled in.

qdot · 2021-01-23T18:12:10Z

src/bluez/adapter/peripheral.rs

-    connection_tx: Arc<Mutex<Sender<u16>>>,
-    connection_rx: Arc<Mutex<Receiver<u16>>>,
-    message_queue: Arc<Mutex<VecDeque<ACLData>>>,
+    attributes_map: Arc<Mutex<HashMap<u16, (String, Handle, Characteristic)>>>,


If you wanted to lose the Mutex here, you could switch to DashMap. I use it heavily up in Buttplug, works great. That said, with everything else so lock heavy here, that's really just the lightest of nits and could be a followup.

qdot · 2021-01-23T18:18:26Z

src/bluez/adapter/peripheral.rs

+                    if let (Some(id), Some(data)) = (iter.next(), iter.next()) {
+                        // This API is terrible.. why can't I just get an array out, why is it all wrapped in a Variant?
+                        let data: Vec<u8> = data
+                            .as_iter() // 🎶 The Variant is connected to the


I like this, please go through the rest of your patch and make your comments rhyme.

qdot · 2021-01-23T18:22:07Z

src/bluez/adapter/peripheral.rs

+            .characteristics
+            .lock()
+            .unwrap()
+            .clone()


Very nit: You might be able to use cloned() after your map (and use refs up to that point), save the copy then discard? But it honestly doesn't matter much in this context, and I can't remember off the top of my head where cloned() comes in.

qwandor · 2021-01-23T18:33:59Z

src/bluez/adapter/peripheral.rs

+                            .as_iter() // Lets convert the
+                            .unwrap() // integers to a
+                            .map(|b| b.as_u64().unwrap() as u8) // array of bytes...
+                            .collect(); // I got too lazy to make it rhyme... 🎶


You can do this more nicely with dbus::arg::prop_cast, or if you pass --prop-newtype to dbus-codegen-rust it will give you a type-safe wrapper for the HashMap, and you can do OrgBluezDevice1Properties::manufacturer_data to get a HashMap<u16, Variant<Box<dyn RefArg>>>, and then you can just map the values with dbus::arg::cast::<Vec<u8>>(&v.0).

From memory, I gave this a try from the very start. but I think because i'm passing in a subset of the type that dbus::arg::cast/prop_cast are expecting, i get compile errors. I'll have another go and see if I've missed something.

Ah, you may also need to make sure you have at least version 0.9.1 of the dbus crate, to include the fix from diwic/dbus-rs#306.

I've sent you NZSmartie#1 to fix this.

Thanks @qwandor! Though i'm a bit sad to see my silly rhyming comment go away 😢

We need to keep some standards in this code, so I'm at least going to need some methods in iambic pentameter going forward.

qdot · 2021-01-23T18:50:38Z

@qwandor Thanks for the comment!

If you or anyone else has additional guidance on the Bluez side, please feel free to add it here. I was mostly reviewing for structure, I'm far more familiar with the CoreBluetooth and UWP sides of the library, so I can't really comment much on the Bluez.

…g into feature/bluez-dbus-qwandor

qdot · 2021-02-03T20:03:14Z

Ok, well, I'm cool with the changes and will land this, just gotta figure out why changing the base branch to dev borkt things because master and dev are on the same HEAD. I'd also like to keep my fast-forward only rule so I may rebase out the merge that comes in here. So, in short: Your code will be into the dev branch probably this evening, but may look slightly different than your commit lineup here, so you may need to reset if you had changes on your dev.

qdot · 2021-02-04T02:15:11Z

Ok, I just brought this in with the merges for now, turns out the prehistory of rumble is just lousy with them anyways.

NZSmartie force-pushed the feature/bluez-dbus branch from 9dd3f2c to d3ec17e Compare January 11, 2021 00:00

NZSmartie mentioned this pull request Jan 11, 2021

DeviceLost event is never fired. #60

Closed

NZSmartie force-pushed the feature/bluez-dbus branch 4 times, most recently from dfef5db to e3d2fb5 Compare January 17, 2021 06:45

NZSmartie changed the title ~~[WIP] BlueZ: Replace raw HCI sockets with BlueZ over DBus~~ BlueZ: Replace raw HCI sockets with BlueZ over DBus Jan 17, 2021

NZSmartie marked this pull request as ready for review January 17, 2021 23:03

NZSmartie and others added 19 commits January 19, 2021 14:25

BlueZ: Begin BlueZ over DBus re-write

4977be9

BlueZ: Prune old HCI code

fbfc51d

BlueZ: Add dbus introspection output

506ba54

BlueZ: Add build script and renaming some things

15099b8

BlueZ: Add Cargo.toml comments

31465eb

BlueZ: More progress

4ba9b8c

BlueZ: Use a seperate DBus connection to listen for signals

c1edfa1

BlueZ: Add perihperal characteristics from DBus

41e427f

BlueZ: Report back all discovered characteristics

e7e96ff

BlueZ: Update introspection scripts to get characteristics

8fe6d0a

BlueZ: Clean up logging and always try to scan

82b94aa

BlueZ: Supprt reading/writing

9f80965

BlueZ: Fixed characteristic discovery and implmement read_by_type

2131ae9

As well as some other house cleaning 🧹

BlueZ: Keep dbus and dbus-codegen at version parity

54871c3

BlueZ: Rename characteristic to attribute where appropriate

264ebc6

BlueZ: Correctly hand properties changd on peripherals

44e332e

I also had a misplaced `!`

BlueZ: Improve connecting and char discovery

72558f9

BlueZ: Ignore blocked devices

0734056

BlueZ: 🧹 Code cleanup

56ff58d

NZSmartie added 2 commits January 19, 2021 14:30

BlueZ: Remove unused package

5696626

BlueZ: Fix broken example

182c5e2

NZSmartie force-pushed the feature/bluez-dbus branch from 0bab260 to 182c5e2 Compare January 19, 2021 01:30

NZSmartie added 2 commits January 19, 2021 14:39

BlueZ: Generate code in build dir and include to avoid tyle checker

f81d196

BlueZ: 🧹 Code cleanup

e9732a0

lnicola mentioned this pull request Jan 21, 2021

Unable to scan for BTLE devices with Intel 5.1 BT adapter #52

Closed

qdot requested changes Jan 23, 2021

View reviewed changes

qwandor reviewed Jan 23, 2021

View reviewed changes

qwandor and others added 10 commits January 30, 2021 13:16

Update to latest dbus-codegen and use propnewtype option.

e563bb4

Use typed wrapper for device properties.

50d569b

Use typed wrapper for characteristic properties.

d270b41

Use generated string constants.

ffc0017

Fix typo in comment.

1048bf8

BlueZ: Rename is_up to is_powered

1ffbc23

BlueZ: Replace thread::sleep() with CondVar for quicker thread joins

539587f

BlueZ: Remove nom dependency

88b00e5

BlueZ: Use invoke_handlers

167238a

Merge branch 'feature/bluez-dbus' of git://github.com/qwandor/btleplu…

838d220

…g into feature/bluez-dbus-qwandor

NZSmartie requested a review from qdot February 2, 2021 09:02

qdot changed the base branch from master to dev February 3, 2021 19:58

qdot approved these changes Feb 3, 2021

View reviewed changes

qdot merged commit 838d220 into deviceplug:dev Feb 4, 2021

NZSmartie mentioned this pull request Feb 24, 2021

0.6.x passive bluetooth scan support #122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BlueZ: Replace raw HCI sockets with BlueZ over DBus #105

BlueZ: Replace raw HCI sockets with BlueZ over DBus #105

NZSmartie commented Jan 10, 2021 •

edited

Loading

NZSmartie commented Jan 11, 2021

NZSmartie commented Jan 17, 2021

NZSmartie commented Jan 17, 2021

NZSmartie commented Jan 19, 2021

lnicola commented Jan 21, 2021

qdot commented Jan 21, 2021

qdot commented Jan 21, 2021

qdot left a comment

qdot Jan 23, 2021

NZSmartie Jan 23, 2021

NZSmartie Jan 24, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qdot Jan 23, 2021

qwandor Jan 23, 2021

NZSmartie Jan 23, 2021

qwandor Jan 23, 2021

qwandor Jan 30, 2021

NZSmartie Feb 2, 2021

qdot Feb 3, 2021

qdot commented Jan 23, 2021

qdot commented Feb 3, 2021

qdot commented Feb 4, 2021

BlueZ: Replace raw HCI sockets with BlueZ over DBus #105

BlueZ: Replace raw HCI sockets with BlueZ over DBus #105

Conversation

NZSmartie commented Jan 10, 2021 • edited Loading

NZSmartie commented Jan 11, 2021

NZSmartie commented Jan 17, 2021

NZSmartie commented Jan 17, 2021

NZSmartie commented Jan 19, 2021

lnicola commented Jan 21, 2021

qdot commented Jan 21, 2021

qdot commented Jan 21, 2021

qdot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdot commented Jan 23, 2021

qdot commented Feb 3, 2021

qdot commented Feb 4, 2021

NZSmartie commented Jan 10, 2021 •

edited

Loading