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.

Sign up for GitHub

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

Jump to bottom

Implement nickel_bluetooth action #152

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Implement nickel_bluetooth action #152

merged 4 commits into from
Dec 15, 2023

Conversation

onatbas
Copy link
Contributor

@onatbas onatbas commented Dec 14, 2023

Implementation of a new action "nickel_bluetooth" to turn Bluetooth on and off via NickelMenu.

Available commands are:
enable
disable
toggle

I've been testing/using this modified version of NickelMenu on my Clara 2E and It should be compatible with all Bluetooth devices.

This should address issue #142

Usage examples:
menu_item :main :Bluetooth (enable) :nickel_bluetooth :enable
menu_item :main :Bluetooth (disable) :nickel_bluetooth :disable
menu_item :main :Bluetooth (toggle) :nickel_bluetooth :toggle

@onatbas onatbas requested a review from pgaskin as a code owner December 14, 2023 04:15
@onatbas
Implement nickel_bluetooth action
b3fb59c 
Available commands are:
	enable
	disable
	toggle
pgaskin
pgaskin requested changes Dec 14, 2023
View reviewed changes
Copy link
Owner

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

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

Look okay overall, not tested (I don't have a Kobo with bluetooth support).

A few minor formatting nitpicks (I should add clang-format at some point).

Also, add the //libnickel symbol checks with the version constraints for CI.

src/action_cc.cc Outdated Show resolved Hide resolved
src/action_cc.cc Outdated Show resolved Hide resolved
src/action_cc.cc Outdated Show resolved Hide resolved
@pgaskin pgaskin linked an issue Dec 14, 2023 that may be closed by this pull request
How to add the option to turn on and off bluetooth #142
Closed
@onatbas
Copy link
Contributor Author

onatbas commented Dec 14, 2023

Thanks. I identified a few problems with the internal Bluetooth driver: in certain conditions, the connection doesn't get established if BT is turned on and off too quickly. I added a few actions and parameters to this fix this and currently testing the changes.
In addition to addressing your comments, I shall submit my changes by the end of today.

@onatbas
Implemented check, scan options for bluetooth
fee1243 
Due to inconsistent handling of bluetooth connections, turning
the connections off will trigger an automatic device scan.

Same way, if bluetooth is turned off before a connection hasn't
been established yet, the following request may not be
successful. Ongoing scans are stopped before turning bluetooth
off.

If bluetooth is enabled but no devices are found, a scan can
now be initiated.

Changes:
	Stop scan if no connection before Bluetooth off
	nickel_bluetooth symbol checks against 4.34.20097
	Initiate bluetooth scan on turn on
	scan is defined
	Code flow is refactor for readability
@onatbas
Copy link
Contributor Author

onatbas commented Dec 15, 2023

I have completed my checks and happy with the current implementation of this feature. The //libnickel checks could probably go even earlier versions but, I haven’t had the chance to validate all versions.
let me know if there are other changes, style adjustments I may have missed.

@onatbas onatbas requested a review from pgaskin December 15, 2023 02:54
pgaskin
pgaskin approved these changes Dec 15, 2023
View reviewed changes
Copy link
Owner

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

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

Looks perfect; I like how you organized the actions. I can't test it as I don't have one which supports it, but it's simple enough, I'll probably just merge it.

pgaskin
pgaskin reviewed Dec 15, 2023
View reviewed changes
src/action_cc.cc Outdated Show resolved Hide resolved
@onatbas
Copy link
Contributor Author

onatbas commented Dec 15, 2023

I'm glad you like it. It's an old pattern called "bitmasking" or "bit flags." The method was going toward multiple booleans, and this pattern is intended for that problem.

It is indeed very simple as you quite rightly put it. Here's a sample from my Clara 2E. (Bluetooth device blinks when it's disconnected/scanning. When the blinking stops, it means connection established.)
https://github.com/pgaskin/NickelMenu/assets/714795/f6f404ee-7f92-45ad-90be-0bd51cd4ea35

@pgaskin pgaskin merged commit 2e58194 into pgaskin:master Dec 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgaskin pgaskin pgaskin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to add the option to turn on and off bluetooth
2 participants
@onatbas @pgaskin