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

Feat: CentralState #366

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Feat: CentralState #366

merged 5 commits into from
Sep 11, 2024

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Feb 19, 2024

Hi,

I'm working on bluetooth integration for trezor-suite app and i would like to use btleplug as a bridge between the electron app and bluetooth device.
To make it work i would like to contribute and address some missing features and issues

CentralState

Implementation of #280 for macos, windows and linux.

I wasn't sure should i extend existing adapter_info method or just add new adapter_state, just for clarity i've added new function. I'm leaving this decision to you.
Also i'm not sure about the naming adapter_state() -> CentralState i didn't want to mess up current naming convention.
Feel free to nitpick anything.

Tested on Windows 10, macOS sonoma, linux nixOS

Commits

@qwandor qwandor changed the base branch from master to dev February 19, 2024 15:22
Copy link
Collaborator

@qwandor qwandor left a comment

Choose a reason for hiding this comment

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

Looks good for BlueZ and CoreBluetooth, apart from some minor comments.

@@ -22,6 +23,13 @@ impl Adapter {
}
}

fn get_central_state(state: bool) -> CentralState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Call the parameter powered rather than state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done bcf31a7

} if id == adapter_id => match adapter_event {
AdapterEvent::Powered { powered } => {
let state = get_central_state(powered);
Some(CentralEvent::StateUpdate(state.into()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need the .into() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess i dont need it :) fixed bcf31a7

@szymonlesisz
Copy link
Contributor Author

sorry for the force-push, too many fixes for cargo fmt

@qwandor qwandor requested a review from qdot February 19, 2024 16:07
@Yongle-Fu
Copy link

wait this feature merged 😁

@szymonlesisz
Copy link
Contributor Author

szymonlesisz commented Sep 10, 2024

Hi,
i've rebased over recent dev branch and resolve conflicts with Use objc2-core-bluetooth

is there anything else i can do to push things forward?

im expecting more conflicts with #388

src/winrtble/manager.rs Outdated Show resolved Hide resolved
src/winrtble/adapter.rs Outdated Show resolved Hide resolved
@szymonlesisz
Copy link
Contributor Author

szymonlesisz commented Sep 10, 2024

ff96d68 and 20dc206 are just format fixes

3e09084 removed clone plus added radio.StateChanged error handler, not sure if this was the reason but now im receiving StateChanged once as expected (reference to note in the description)

pls let me know when to squash my fixups

Copy link
Collaborator

@qwandor qwandor left a comment

Choose a reason for hiding this comment

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

Looks good to me once the build failure is fixed. Go ahead and squash your fixups when you're ready.

@qwandor qwandor merged commit 0eb6188 into deviceplug:dev Sep 11, 2024
4 checks passed
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.

3 participants