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(ble): Add the ability to enable and disable advertising #2546

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ReFil
Copy link
Contributor

@ReFil ReFil commented Oct 10, 2024

Currently advertising cannot be disabled, In some circumstances it may be desirable to disable advertising to the host e.g. seeking consistency between an endpoint switch and percieved board behaviour.

This adds 3 new options to &bt, ADV_ON, ADV_OFF and ADV_TOG as well as the metadata and documentation for the behaviour. ble.c has been updated with a static flag that gets checked in update_advertising() and doesn't permit any adveritising state other than ZMK_ADV_NONE unless it's set. it's defaulted to true so standard board behavior shouldn't change.

The ability to get the current adverising state has also been added, it could have utility in custom display widgets.

Tested on a development board, I've noticed some hosts cache the advertising info so it still shows up as a connectible device if the switch is used whilst the bluetooth devices menu is open, but it still cannot be connected to. Using an app like nRF connect or a different host it can be seen that the device properly stops advertising

@ReFil ReFil requested review from a team as code owners October 10, 2024 13:31
@Nick-Munnich
Copy link
Contributor

Regarding docs I think there might end up being some confusion when people see "This turns on advertising", use the behavior expecting to be able to connect a new device yet nothing happens because they haven't selected an unused profile.

@ReFil
Copy link
Contributor Author

ReFil commented Oct 10, 2024

I see your point, it would still advertise if it's in an occupied profile but wouldn't pair and work properly. Perhaps it would be better worded as "Permits advertising (note that other conditions must be met for the device to be visible)"?

@Nick-Munnich
Copy link
Contributor

That still sounds like it's a bit unclear to some. What would you consider to be the difference between what you've defined as BT_ADV_OFF and a hypothetical BT_OFF command?

@ReFil
Copy link
Contributor Author

ReFil commented Oct 10, 2024

Functionally there's not really any difference I suppose

@Nick-Munnich
Copy link
Contributor

I think renaming and rephrasing it appropriately would be both clearer and make it usable by non-tech people, then. Sometimes a user might want to just turn off BT after all.

@ReFil
Copy link
Contributor Author

ReFil commented Oct 10, 2024

Hmm, I'm not sure about describing it as turning off Bluetooth when it doesn't do that. My concern is users may wonder if it affects split connections if it's described as shutting off Bluetooth

Copy link
Contributor

@Nick-Munnich Nick-Munnich left a comment

Choose a reason for hiding this comment

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

True, so that's not an accurate description. Hmm.

docs/docs/keymaps/behaviors/bluetooth.md Outdated Show resolved Hide resolved
ReFil and others added 2 commits October 22, 2024 13:20
Currently advertising cannot be disabled, In some circumstances it may be desirable to disable advertising to the host e.g. seeking consistency between an endpoint switch and percieved board behaviour

Update behavior_bt.c

Revert "Update behavior_bt.c"

This reverts commit d822523.
@caksoylar caksoylar added core Core functionality/behavior of ZMK bluetooth Bluetooth related items labels Oct 27, 2024
Copy link
Contributor

@petejohanson petejohanson 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 working on this!

app/include/dt-bindings/zmk/bt.h Outdated Show resolved Hide resolved
app/include/zmk/ble.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bluetooth Bluetooth related items core Core functionality/behavior of ZMK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants