Support custom BLE device name discovery #87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR introduces a configurable BLE device name by adding a CLI flag and refactoring the BLE scanning and connection APIs to accept a dynamic name instead of a hardcoded constant. The device_name parameter is threaded through enumeration, filtering, and payload writing paths. Sequence diagram for BLE device discovery with custom device namesequenceDiagram
actor User
participant CLI as CLI
participant Main as main.rs
participant Device as Device (ble.rs)
User->>CLI: Run command with --device_name
CLI->>Main: Parse Args (device_name)
Main->>Device: Device::single(device_name)
Device->>Device: enumerate(device_name)
Device->>Device: enumerate_duration(duration, device_name)
Device->>Device: from_peripheral(peripheral, device_name)
Device-->>Main: Return matching device
Main-->>CLI: Continue with device operations
Class diagram for updated Device BLE APIclassDiagram
class Device {
+async fn enumerate(device_name: &str) -> Result<Vec<Self>>
+async fn enumerate_duration(scan_duration: Duration, device_name: &str) -> Result<Vec<Self>>
+async fn single(device_name: Option<&str>) -> Result<Self>
+async fn from_peripheral(peripheral: Peripheral, device_name: &str) -> Option<Self>
}
Class diagram for main.rs argument and payload flowclassDiagram
class Args {
+Option<String> device_name
+TransportProtocol transport
+bool list_devices
}
class TransportProtocol
class PayloadBuffer
Args --> TransportProtocol
Args --> PayloadBuffer
class Main {
+fn main() -> Result<()>
+fn write_payload(transport: &TransportProtocol, device_name: Option<String>, payload: PayloadBuffer) -> Result<(), anyhow::Error>
+fn list_devices(transport: &TransportProtocol) -> Result<()>
+fn gnerate_payload(args: &mut Args) -> Result<PayloadBuffer>
}
Main --> Args
Main --> TransportProtocol
Main --> PayloadBuffer
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @weberval - I've reviewed your changes - here's some feedback:
- The new
device_nameparameter is currently taken as&Stringin multiple methods and is derived via&device_name.unwrap_or(...), which can lead to invalid references to temporaries; consider accepting&strorimpl AsRef<str>(or an ownedString) and restructuring to avoid borrowing from dropped values. - Propagating
device_namethroughenumerate,enumerate_duration, andfrom_peripheralmakes the API less ergonomic; consider encapsulating the BLE scan criteria (including device name) in a single config struct or using default arguments to reduce parameter plumbing. - In
single, consuming anOption<String>only to immediately unwrap and reference it is both surprising and error-prone; it’d be clearer to accept either an ownedStringorOption<&str>and clone the fallback name when needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `device_name` parameter is currently taken as `&String` in multiple methods and is derived via `&device_name.unwrap_or(...)`, which can lead to invalid references to temporaries; consider accepting `&str` or `impl AsRef<str>` (or an owned `String`) and restructuring to avoid borrowing from dropped values.
- Propagating `device_name` through `enumerate`, `enumerate_duration`, and `from_peripheral` makes the API less ergonomic; consider encapsulating the BLE scan criteria (including device name) in a single config struct or using default arguments to reduce parameter plumbing.
- In `single`, consuming an `Option<String>` only to immediately unwrap and reference it is both surprising and error-prone; it’d be clearer to accept either an owned `String` or `Option<&str>` and clone the fallback name when needed.
## Individual Comments
### Comment 1
<location> `src/ble.rs:76` </location>
<code_context>
/// Returns all badges that are in BLE range and are in Bluetooth transfer mode.
- pub async fn enumerate() -> Result<Vec<Self>> {
- Self::enumerate_duration(Duration::from_secs(2)).await
+ pub async fn enumerate(device_name: &String) -> Result<Vec<Self>> {
+ Self::enumerate_duration(Duration::from_secs(2), device_name).await
}
</code_context>
<issue_to_address>
Consider using &str instead of &String for device_name parameter.
Using &str enables the function to accept both String and string literals, improving flexibility and adhering to Rust conventions.
Suggested implementation:
```rust
pub async fn enumerate(device_name: &str) -> Result<Vec<Self>> {
Self::enumerate_duration(Duration::from_secs(2), device_name).await
}
```
```rust
pub async fn enumerate_duration(
scan_duration: Duration,
device_name: &str,
) -> Result<Vec<Self>> {
```
</issue_to_address>
### Comment 2
<location> `src/ble.rs:85` </location>
<code_context>
/// # Panics
/// This function panics if it is unable to access the Bluetooth adapter.
- pub async fn enumerate_duration(scan_duration: Duration) -> Result<Vec<Self>> {
+ pub async fn enumerate_duration(
+ scan_duration: Duration,
+ device_name: &String,
+ ) -> Result<Vec<Self>> {
// Run device scan
</code_context>
<issue_to_address>
Use &str for device_name parameter for consistency and ergonomics.
This change improves usability, as device names are usually passed as string slices.
Suggested implementation:
```rust
pub async fn enumerate(device_name: &str) -> Result<Vec<Self>> {
Self::enumerate_duration(Duration::from_secs(2), device_name).await
}
```
```rust
pub async fn enumerate_duration(
scan_duration: Duration,
device_name: &str,
) -> Result<Vec<Self>> {
```
```rust
{
if let Some(badge) = Self::from_peripheral(p, device_name).await {
led_badges.push(badge);
}
}
```
If there are any other places in the codebase where `enumerate` or `enumerate_duration` are called, ensure that the argument passed for `device_name` is a `&str` (e.g., use `"my_device"` or `my_string_variable.as_str()` if you have a `String`).
</issue_to_address>
### Comment 3
<location> `src/ble.rs:120` </location>
<code_context>
}
- async fn from_peripheral(peripheral: Peripheral) -> Option<Self> {
+ async fn from_peripheral(peripheral: Peripheral, device_name: &String) -> Option<Self> {
// The existance of the service with the correct UUID
// exists is already checked by the scan filter.
</code_context>
<issue_to_address>
Switch device_name parameter to &str for better ergonomics.
This update will enhance API usability across all call sites.
Suggested implementation:
```rust
if let Some(badge) = Self::from_peripheral(p, device_name).await {
```
```rust
async fn from_peripheral(peripheral: Peripheral, device_name: &str) -> Option<Self> {
```
```rust
if local_name == device_name {
```
</issue_to_address>
### Comment 4
<location> `src/ble.rs:140` </location>
<code_context>
/// or if multiple devices would match.
- pub async fn single() -> Result<Self> {
- let mut devices = Self::enumerate()
+ pub async fn single(device_name: Option<String>) -> Result<Self> {
+ let device_name = &device_name.unwrap_or(BADGE_BLE_DEVICE_NAME.to_string());
+ let mut devices = Self::enumerate(device_name)
</code_context>
<issue_to_address>
Consider accepting Option<&str> instead of Option<String> for device_name.
This change reduces allocations and increases flexibility, as the function only needs a borrowed value.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
pub async fn single(device_name: Option<String>) -> Result<Self> {
let device_name = &device_name.unwrap_or(BADGE_BLE_DEVICE_NAME.to_string());
let mut devices = Self::enumerate(device_name)
=======
pub async fn single(device_name: Option<&str>) -> Result<Self> {
let device_name = device_name.unwrap_or(BADGE_BLE_DEVICE_NAME);
let mut devices = Self::enumerate(device_name)
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `src/ble.rs:141` </location>
<code_context>
- pub async fn single() -> Result<Self> {
- let mut devices = Self::enumerate()
+ pub async fn single(device_name: Option<String>) -> Result<Self> {
+ let device_name = &device_name.unwrap_or(BADGE_BLE_DEVICE_NAME.to_string());
+ let mut devices = Self::enumerate(device_name)
.await
</code_context>
<issue_to_address>
Avoid allocating a new String for BADGE_BLE_DEVICE_NAME if not necessary.
If BADGE_BLE_DEVICE_NAME is a &'static str, use as_ref() or pass &str directly to avoid unnecessary String allocation.
Suggested implementation:
```rust
pub async fn single(device_name: Option<&str>) -> Result<Self> {
```
```rust
let device_name = device_name.as_deref().unwrap_or(BADGE_BLE_DEVICE_NAME);
let mut devices = Self::enumerate(device_name)
```
If `Self::from_peripheral` and other downstream functions expect a `String` or `&String`, you should update their signatures and usages to accept `&str` instead, to maintain consistency and avoid unnecessary allocations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
weberval
changed the title
sourcery-ai
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #56
Summary by Sourcery
Allow customizing the BLE device name used for discovery by adding a new CLI flag and updating the BLE enumeration API to accept and use the provided name.
New Features:
Enhancements: