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 support for transferring badge config via Bluetooth Low Energy #16

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

hlxid
Copy link
Contributor

@hlxid hlxid commented Jul 30, 2024

Adds a new crate feature ble that allows for transfering badge configurations over Bluetooth Low Energy instead of USB. Expands the CLI to allow for specification of the transport protocol, either USB or BLE.

The BLE library uses async rust requiring to add tokio as a async runtime to the CLI.

Summary by Sourcery

This pull request introduces support for transferring badge configurations via Bluetooth Low Energy (BLE) by adding a new crate feature ble. The CLI has been expanded to allow users to specify the transport protocol (USB or BLE). Additionally, the tokio async runtime has been integrated to support the asynchronous operations required by the BLE library. Documentation has been updated to reflect these changes.

  • New Features:
    • Added support for transferring badge configurations via Bluetooth Low Energy (BLE) by introducing a new crate feature ble.
    • Expanded the CLI to allow specification of the transport protocol, either USB or BLE.
  • Enhancements:
    • Integrated the tokio async runtime to support asynchronous operations required by the BLE library.
  • Documentation:
    • Updated the README to include instructions for using the new BLE transport mode, including necessary permissions for macOS.

Adds a new crate feature `ble` that allows for transfering badge
configurations over Bluetooth Low Energy instead of USB.
Expands the CLI to allow for specification of the transport protocol,
either USB or BLE.

The BLE library uses async rust requiring to add tokio as a async runtime
to the CLI.
Copy link

sourcery-ai bot commented Jul 30, 2024

Reviewer's Guide by Sourcery

This pull request adds support for transferring badge configurations via Bluetooth Low Energy (BLE) in addition to the existing USB option. It introduces a new crate feature ble, updates the CLI to allow specifying the transport protocol, and adds the tokio async runtime to handle asynchronous BLE operations. The src/ble.rs file implements the BLE communication logic, including device discovery and payload writing.

File-Level Changes

Files Changes
src/main.rs
src/lib.rs
src/ble.rs
Introduced support for Bluetooth Low Energy (BLE) as a transport protocol, including new command-line arguments, an asynchronous runtime, and a dedicated BLE module.
README.md Updated documentation to reflect the new BLE transport option and its usage.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @hlxid - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/ble.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mgjm mgjm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ❤️. BLE support was on my wishlist.

I have written a few improvement suggestions. In general please have look at all ? operators. Every time you return an error from a library like btleplug try to add a custom context to the error. This helps to narrow down where an error originated from (kind of like a stack trace). I have given one example suggestion, but this can be applied to all ? operators.

Note: All suggestions are just written in GitHub, they still need cargo fmt and maybe some use statements are missing.

I can't test the BLE functionality myself. Did you test this feature on real hardware with an actual badge? And on which OS?

Cargo.toml Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@mgjm
Copy link
Collaborator

mgjm commented Jul 30, 2024

The CI pipeline failed on linux. The dbus-1 library seems to be missing. Try to add the libdbus-1-dev package in .github/workflows/ci.yaml at line 54 and 69.

@hlxid
Copy link
Contributor Author

hlxid commented Jul 31, 2024

Thank you very much for your extensive review!
All addressed points should be fixed with my latest commit. Didn't catch that btleplug uses tokio anyway and also thank you very much for the anyhow context suggestion.

I have tested this PR with a real badge on two different Arch Linux machines. I can also test it on Windows and macOS if you want.

Copy link
Collaborator

@mgjm mgjm left a comment

Choose a reason for hiding this comment

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

Awesome. I've just added some last suggestions.

And it would be great if you could test it on Windows and MacOS too.

Note: I haven't tested the code on Windows or MacOS at all. There could be unrelated bugs. If you have the time, you could also test the USB transport on those systems.

src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
src/ble.rs Outdated Show resolved Hide resolved
@hlxid
Copy link
Contributor Author

hlxid commented Aug 1, 2024

Thanks again. I've also removed the service UUID check as it is done through the ScanFilter now and would be redundant.

I've tested this now on macOS where writing to the badge works but afterwards the CLI just decides to hang indefinitely and not exit as expected. USB works flawlessly :)
I will be able to look into the hang issue after writing on macOS and test on Windows sometimes in the beginning of next week.

@mgjm
Copy link
Collaborator

mgjm commented Aug 1, 2024

LGTM

I've moved the MacOS and Windows testing to a separate issue: #20

@mgjm mgjm merged commit 71f9ab3 into fossasia:main Aug 1, 2024
13 checks passed
@hlxid hlxid deleted the feat/ble branch August 2, 2024 05:11
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.

2 participants