PR feedback

.github/workflows/python-build-test.yml

@@ -43,18 +43,28 @@ jobs:
         inv build.mkdocs
   build-rust:
     runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        python-version: [ "3.8", "3.9", "3.10" ]
+      fail-fast: false
     steps:
       - name: Check out from Git
         uses: actions/checkout@v3
-      - name: Set up Python
+      - name: Set up Python ${{ matrix.python-version }}
         uses: actions/setup-python@v4
         with:
-          python-version: '3.10'
+          python-version: ${{ matrix.python-version }}
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip
           python -m pip install ".[build,test,development,documentation]"
       - name: Install Rust toolchain
         uses: actions-rust-lang/setup-rust-toolchain@v1
+        with:
+          components: clippy,rustfmt
+      - name: Rust Lints
+        run: cd rust && cargo fmt --check && cargo clippy --all-targets -- --deny warnings
+      - name: Rust Build
+        run: cd rust && cargo build --all-targets
       - name: Rust Tests
-        run: cd rust && cargo build --all-targets && cargo test
+        run: cd rust && cargo test

rust/README.md

@@ -10,7 +10,7 @@ See the `examples` directory for usage.
 # Usage
 
 Set up a virtualenv for Bumble, or otherwise have an isolated Python environment
-fur Bumble and its dependencies.
+for Bumble and its dependencies.
 
 Due to Python being
 [picky about how its sys path is set up](https://github.com/PyO3/pyo3/issues/1741,

rust/examples/battery_client.rs

@@ -33,7 +33,7 @@
 
 use bumble::wrapper::{
     device::{Device, Peer},
-    profile::BatteryService,
+    profile::BatteryServiceProxy,
     transport::Transport,
     PyObjectExt,
 };
@@ -63,12 +63,11 @@ async fn main() -> PyResult<()> {
 
     let conn = device.connect(&cli.target_addr).await?;
     let mut peer = Peer::new(conn)?;
-    peer.discover_services().await?;
+    for mut s in peer.discover_services().await? {
-    for mut s in peer.services()? {
         s.discover_characteristics().await?;
     }
     let battery_service = peer
-        .create_service_proxy::<BatteryService>()?
+        .create_service_proxy::<BatteryServiceProxy>()?
         .ok_or(anyhow::anyhow!("No battery service found"))?;
 
     let mut battery_level_char = battery_service

rust/examples/broadcast.rs

@@ -13,11 +13,13 @@
 // limitations under the License.
 
 use anyhow::anyhow;
-use bumble::wrapper::{
+use bumble::{
     adv::{AdvertisementDataBuilder, CommonDataType},
-    device::Device,
+    wrapper::{
-    logging::{bumble_env_logging_level, py_logging_basic_config},
+        device::Device,
-    transport::Transport,
+        logging::{bumble_env_logging_level, py_logging_basic_config},
+        transport::Transport,
+    },
 };
 use clap::Parser as _;
 use pyo3::PyResult;
@@ -61,7 +63,7 @@ async fn main() -> PyResult<()> {
         )
         .map_err(|e| anyhow!(e))?;
 
-    device.set_advertisement(adv_data)?;
+    device.set_advertising_data(adv_data)?;
     device.power_on().await?;
 
     println!("Advertising...");

rust/examples/scanner.rs

@@ -12,11 +12,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//! Counterpart to the Python example `run_scanner.py`
+//! Counterpart to the Python example `run_scanner.py`.
+//!
+//! Device deduplication is done here rather than relying on the controller's filtering to provide
+//! for additional features, like the ability to make deduplication time-bounded.
 
-use bumble::wrapper::{
+use bumble::{
-    adv::CommonDataType, core::AdvertisementDataUnit, device::Device, hci::AddressType,
+    adv::CommonDataType,
-    transport::Transport,
+    wrapper::{
+        core::AdvertisementDataUnit, device::Device, hci::AddressType, transport::Transport,
+    },
 };
 use clap::Parser as _;
 use itertools::Itertools;

rust/examples/usb_probe.rs

@@ -89,7 +89,7 @@ fn main() -> anyhow::Result<()> {
             transport_names.push(basic_transport_name.clone());
         } else {
             transport_names.push(format!(
-                "{}/{}",
+                "{}#{}",
                 basic_transport_name,
                 device_serials_by_id
                     .get(&device_id)
@@ -124,39 +124,39 @@ fn main() -> anyhow::Result<()> {
                 Style::new().red()
             };
             println!(
-                "{:26 }{}",
+                "{:26}{}",
                 "  Bumble Transport Names:".blue(),
                 transport_names.iter().map(|n| n.style(style)).join(" or ")
             )
         }
         println!(
-            "{:26 }{:03}/{:03}",
+            "{:26}{:03}/{:03}",
             "  Bus/Device:".green(),
             device.bus_number(),
             device.address()
         );
         println!(
-            "{:26 }{}",
+            "{:26}{}",
             "  Class:".green(),
             class_info.formatted_class_name()
         );
         println!(
-            "{:26 }{}",
+            "{:26}{}",
             "  Subclass/Protocol:".green(),
             class_info.formatted_subclass_protocol()
         );
         if let Some(s) = serial {
-            println!("{:26 }{}", "  Serial:".green(), s);
+            println!("{:26}{}", "  Serial:".green(), s);
             device_serials_by_id
                 .entry(device_id)
                 .or_insert(HashSet::new())
                 .insert(s);
         }
         if let Some(m) = mfg {
-            println!("{:26 }{}", "  Manufacturer:".green(), m);
+            println!("{:26}{}", "  Manufacturer:".green(), m);
         }
         if let Some(p) = product {
-            println!("{:26 }{}", "  Product:".green(), p);
+            println!("{:26}{}", "  Product:".green(), p);
         }
 
         if cli.verbose {
@@ -205,15 +205,15 @@ fn print_device_details<T: UsbContext>(
     for i in 0..device_desc.num_configurations() {
         println!("  Configuration {}", i + 1);
         for interface in device.config_descriptor(i)?.interfaces() {
-            let descriptors: Vec<_> = interface.descriptors().collect();
+            let interface_descriptors: Vec<_> = interface.descriptors().collect();
-            for d in &descriptors {
+            for d in &interface_descriptors {
                 let class_info =
                     ClassInfo::new(d.class_code(), d.sub_class_code(), d.protocol_code());
 
                 println!(
                     "      Interface: {}{} ({}, {})",
                     interface.number(),
-                    if descriptors.len() > 1 {
+                    if interface_descriptors.len() > 1 {
                         format!("/{}", d.setting_number())
                     } else {
                         String::new()

rust/src/adv.rs

@@ -1,9 +1,7 @@
-//! Advertisements
+//! BLE advertisements.
 
-use crate::wrapper::{
+use crate::wrapper::assigned_numbers::{COMPANY_IDS, SERVICE_IDS};
-    assigned_numbers::{COMPANY_IDS, SERVICE_IDS},
+use crate::wrapper::core::{Uuid128, Uuid16, Uuid32};
-    core::{Uuid128, Uuid16, Uuid32},
-};
 use itertools::Itertools;
 use nom::{combinator, multi, number};
 use std::fmt;
@@ -151,7 +149,7 @@ impl CommonDataType {
     /// Apply type-specific human-oriented formatting to data, if any is applicable
     pub fn format_data(&self, data: &[u8]) -> Option<String> {
         match self {
-            Self::Flags => Some(Flags::matching(data).map(|f| format!("{:?}", f)).join(", ")),
+            Self::Flags => Some(Flags::matching(data).map(|f| format!("{:?}", f)).join(",")),
             Self::CompleteListOf16BitServiceClassUuids
             | Self::IncompleteListOf16BitServiceClassUuids
             | Self::ListOf16BitServiceSolicitationUuids => {
@@ -396,30 +394,12 @@ pub enum Flags {
 
 impl fmt::Debug for Flags {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
+        write!(f, "{}", self.short_name())
-            Flags::LeLimited => write!(f, "LE Limited"),
-            Flags::LeDiscoverable => write!(f, "LE General"),
-            Flags::NoBrEdr => write!(f, "No BR/EDR"),
-            Flags::BrEdrController => write!(f, "BR/EDR C"),
-            Flags::BrEdrHost => write!(f, "BR/EDR H"),
-        }
-    }
-}
-
-impl fmt::Display for Flags {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Flags::LeLimited => write!(f, "LE Limited Discoverable Mode"),
-            Flags::LeDiscoverable => write!(f, "LE General Discoverable Mode"),
-            Flags::NoBrEdr => write!(f, "BR/EDR Not Supported"),
-            Flags::BrEdrController => write!(f, "Simultaneous LE and BR/EDR (Controller)"),
-            Flags::BrEdrHost => write!(f, "Simultaneous LE and BR/EDR (Host)"),
-        }
     }
 }
 
 impl Flags {
-    /// Iterates over the flags that are present in the provided `flags` byte.
+    /// Iterates over the flags that are present in the provided `flags` bytes.
     pub fn matching(flags: &[u8]) -> impl Iterator<Item = Self> + '_ {
         // The encoding is not clear from the spec: do we look at the first byte? or the last?
         // In practice it's only one byte.
@@ -437,4 +417,30 @@ impl Flags {
             mask & first_byte > 0
         })
     }
+
+    /// An abbreviated form of the flag name.
+    ///
+    /// See [Flags::name] for the full name.
+    pub fn short_name(&self) -> &'static str {
+        match self {
+            Flags::LeLimited => "LE Limited",
+            Flags::LeDiscoverable => "LE General",
+            Flags::NoBrEdr => "No BR/EDR",
+            Flags::BrEdrController => "BR/EDR C",
+            Flags::BrEdrHost => "BR/EDR H",
+        }
+    }
+
+    /// The human-readable name of the flag.
+    ///
+    /// See [Flags::short_name] for a shorter string for use if compactness is important.
+    pub fn name(&self) -> &'static str {
+        match self {
+            Flags::LeLimited => "LE Limited Discoverable Mode",
+            Flags::LeDiscoverable => "LE General Discoverable Mode",
+            Flags::NoBrEdr => "BR/EDR Not Supported",
+            Flags::BrEdrController => "Simultaneous LE and BR/EDR (Controller)",
+            Flags::BrEdrHost => "Simultaneous LE and BR/EDR (Host)",
+        }
+    }
 }

rust/src/lib.rs

@@ -22,8 +22,10 @@
 //! typically do, making it good for prototyping, experimentation, test tools, etc.
 //!
 //! Bumble is primarily written in Python. Rust types that wrap the Python API, which is currently
-//! all of them, are in the [wrapper] module.
+//! the bulk of the code, are in the [wrapper] module.
 
 #![deny(missing_docs, unsafe_code)]
 
 pub mod wrapper;
+
+pub mod adv;

rust/src/wrapper/core.rs

@@ -14,7 +14,7 @@
 
 //! Core types
 
-use crate::wrapper::adv::CommonDataTypeCode;
+use crate::adv::CommonDataTypeCode;
 use lazy_static::lazy_static;
 use nom::{bytes, combinator};
 use pyo3::{intern, PyObject, PyResult, Python};

rust/src/wrapper/device.rs

@@ -14,14 +14,15 @@
 
 //! Devices and connections to them
 
-use crate::wrapper::{
+use crate::{
     adv::AdvertisementDataBuilder,
-    core::AdvertisingData,
+    wrapper::{
-    gatt::Service,
+        core::AdvertisingData,
-    gatt_client::ProfileServiceProxy,
+        gatt_client::{ProfileServiceProxy, ServiceProxy},
-    hci::Address,
+        hci::Address,
-    transport::{Sink, Source},
+        transport::{Sink, Source},
-    ClosureCallback,
+        ClosureCallback,
+    },
 };
 use pyo3::types::PyDict;
 use pyo3::{intern, types::PyModule, PyObject, PyResult, Python, ToPyObject};
@@ -111,7 +112,7 @@ impl Device {
     }
 
     /// Set the advertisement data to be used when [Device::start_advertising] is called.
-    pub fn set_advertisement(&mut self, adv_data: AdvertisementDataBuilder) -> PyResult<()> {
+    pub fn set_advertising_data(&mut self, adv_data: AdvertisementDataBuilder) -> PyResult<()> {
         Python::with_gil(|py| {
             self.0.setattr(
                 py,
@@ -166,29 +167,34 @@ impl Peer {
     }
 
     /// Populates the peer's cache of services.
-    pub async fn discover_services(&mut self) -> PyResult<()> {
+    ///
+    /// Returns the discovered services.
+    pub async fn discover_services(&mut self) -> PyResult<Vec<ServiceProxy>> {
         Python::with_gil(|py| {
             self.0
                 .call_method0(py, intern!(py, "discover_services"))
                 .and_then(|coroutine| pyo3_asyncio::tokio::into_future(coroutine.as_ref(py)))
         })?
         .await
-        .map(|_| ())
+        .and_then(|list| {
+            Python::with_gil(|py| {
+                list.as_ref(py)
+                    .iter()?
+                    .map(|r| r.map(|h| ServiceProxy(h.to_object(py))))
+                    .collect()
+            })
+        })
     }
 
     /// Returns a snapshot of the Services currently in the peer's cache
-    pub fn services(&self) -> PyResult<Vec<Service>> {
+    pub fn services(&self) -> PyResult<Vec<ServiceProxy>> {
         Python::with_gil(|py| {
-            let list = self.0.getattr(py, intern!(py, "services"))?;
+            self.0
-
+                .getattr(py, intern!(py, "services"))?
-            // there's probably a better way to do this
-            Ok(list
                 .as_ref(py)
                 .iter()?
-                .collect::<Result<Vec<_>, _>>()?
+                .map(|r| r.map(|h| ServiceProxy(h.to_object(py))))
-                .into_iter()
+                .collect()
-                .map(|any| Service(any.to_object(py)))
-                .collect::<Vec<_>>())
         })
     }
 

rust/src/wrapper/gatt.rs

@@ -1,67 +0,0 @@
-// Copyright 2023 Google LLC
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-//! GATT
-
-use crate::wrapper::ClosureCallback;
-use pyo3::{intern, types::PyTuple, PyObject, PyResult, Python};
-
-/// A GATT service
-pub struct Service(pub(crate) PyObject);
-
-impl Service {
-    /// Discover the characteristics in this service.
-    ///
-    /// Populates an internal cache of characteristics in this service.
-    pub async fn discover_characteristics(&mut self) -> PyResult<()> {
-        Python::with_gil(|py| {
-            self.0
-                .call_method0(py, intern!(py, "discover_characteristics"))
-                .and_then(|coroutine| pyo3_asyncio::tokio::into_future(coroutine.as_ref(py)))
-        })?
-        .await
-        .map(|_| ())
-    }
-}
-
-/// A GATT characteristic
-pub struct Characteristic(pub(crate) PyObject);
-
-impl Characteristic {
-    /// Subscribe to changes to the characteristic, executing `callback` for each new value
-    pub async fn subscribe(
-        &mut self,
-        callback: impl Fn(Python, &PyTuple) -> PyResult<()> + Send + 'static,
-    ) -> PyResult<()> {
-        let boxed = ClosureCallback::new(move |py, args, _kwargs| callback(py, args));
-
-        Python::with_gil(|py| {
-            self.0
-                .call_method1(py, intern!(py, "subscribe"), (boxed,))
-                .and_then(|obj| pyo3_asyncio::tokio::into_future(obj.as_ref(py)))
-        })?
-        .await
-        .map(|_| ())
-    }
-
-    /// Read the current value of the characteristic
-    pub async fn read_value(&self) -> PyResult<PyObject> {
-        Python::with_gil(|py| {
-            self.0
-                .call_method0(py, intern!(py, "read_value"))
-                .and_then(|obj| pyo3_asyncio::tokio::into_future(obj.as_ref(py)))
-        })?
-        .await
-    }
-}

rust/src/wrapper/gatt_client.rs

@@ -14,7 +14,58 @@
 
 //! GATT client support
 
-use pyo3::PyObject;
+use crate::wrapper::ClosureCallback;
+use pyo3::types::PyTuple;
+use pyo3::{intern, PyObject, PyResult, Python};
+
+/// A GATT service on a remote device
+pub struct ServiceProxy(pub(crate) PyObject);
+
+impl ServiceProxy {
+    /// Discover the characteristics in this service.
+    ///
+    /// Populates an internal cache of characteristics in this service.
+    pub async fn discover_characteristics(&mut self) -> PyResult<()> {
+        Python::with_gil(|py| {
+            self.0
+                .call_method0(py, intern!(py, "discover_characteristics"))
+                .and_then(|coroutine| pyo3_asyncio::tokio::into_future(coroutine.as_ref(py)))
+        })?
+        .await
+        .map(|_| ())
+    }
+}
+
+/// A GATT characteristic on a remote device
+pub struct CharacteristicProxy(pub(crate) PyObject);
+
+impl CharacteristicProxy {
+    /// Subscribe to changes to the characteristic, executing `callback` for each new value
+    pub async fn subscribe(
+        &mut self,
+        callback: impl Fn(Python, &PyTuple) -> PyResult<()> + Send + 'static,
+    ) -> PyResult<()> {
+        let boxed = ClosureCallback::new(move |py, args, _kwargs| callback(py, args));
+
+        Python::with_gil(|py| {
+            self.0
+                .call_method1(py, intern!(py, "subscribe"), (boxed,))
+                .and_then(|obj| pyo3_asyncio::tokio::into_future(obj.as_ref(py)))
+        })?
+        .await
+        .map(|_| ())
+    }
+
+    /// Read the current value of the characteristic
+    pub async fn read_value(&self) -> PyResult<PyObject> {
+        Python::with_gil(|py| {
+            self.0
+                .call_method0(py, intern!(py, "read_value"))
+                .and_then(|obj| pyo3_asyncio::tokio::into_future(obj.as_ref(py)))
+        })?
+        .await
+    }
+}
 
 /// Equivalent to the Python `ProfileServiceProxy`.
 pub trait ProfileServiceProxy {

rust/src/wrapper/mod.rs

@@ -28,11 +28,9 @@ use pyo3::{
 };
 pub use pyo3_asyncio;
 
-pub mod adv;
 pub mod assigned_numbers;
 pub mod core;
 pub mod device;
-pub mod gatt;
 pub mod gatt_client;
 pub mod hci;
 pub mod logging;

rust/src/wrapper/profile.rs

@@ -14,15 +14,15 @@
 
 //! GATT profiles
 
-use crate::wrapper::{gatt::Characteristic, gatt_client::ProfileServiceProxy};
+use crate::wrapper::gatt_client::{CharacteristicProxy, ProfileServiceProxy};
 use pyo3::{intern, PyObject, PyResult, Python};
 
 /// Exposes the battery GATT service
-pub struct BatteryService(PyObject);
+pub struct BatteryServiceProxy(PyObject);
 
-impl BatteryService {
+impl BatteryServiceProxy {
     /// Get the battery level, if available
-    pub fn battery_level(&self) -> PyResult<Option<Characteristic>> {
+    pub fn battery_level(&self) -> PyResult<Option<CharacteristicProxy>> {
         Python::with_gil(|py| {
             self.0
                 .getattr(py, intern!(py, "battery_level"))
@@ -30,14 +30,14 @@ impl BatteryService {
                     if level.is_none(py) {
                         None
                     } else {
-                        Some(Characteristic(level))
+                        Some(CharacteristicProxy(level))
                     }
                 })
         })
     }
 }
 
-impl ProfileServiceProxy for BatteryService {
+impl ProfileServiceProxy for BatteryServiceProxy {
     const PROXY_CLASS_MODULE: &'static str = "bumble.profiles.battery_service";
     const PROXY_CLASS_NAME: &'static str = "BatteryServiceProxy";