feat(battery): Split battery reporting over BLE GATT #1243

Katona · 2022-04-13T16:27:27Z

This PR implements #764.

The changes are relatively simple:

the central registers on battery level changes of peripheral over BLE GATT.
then it 'relays' the peripheral battery level changes to the host using BLE GATT again. :)
for this to work, the central no longer relies on the Battery Service of Zephyr since that assumes one battery only. Instead, it defines its own BAS with two battery level characteristics.

What I tested

I tested only on my wireless Corne. For the test I used the Web Bluetooth API since that was the easiest to setup for me. Here is a link to a video demonstrating it.

What I didn't test

I didn't test non split BLE keyboards since I don't have any.
Tested only using macOS. I don't know how other OSes display battery level when multiple one are present in one device.

As indicated in #764 the OSes doesn't seem to be prepared to devices with more than one battery (macOS certainly not), but not all is lost in my opinion. I still believe this PR is useful since:

it is actually standard (the BLE standard allows it).
it is not breaking anything: OSes (macOS) ignore the second battery, so battery level reporting still works for central and, eventually OSes might support it (not likely, though).
we can develop utilities which can display both batteries in the system tray or menu.

Katona · 2022-04-15T12:45:39Z

I just realized I can also share the test app relatively easily: https://cdpn.io/pen/debug/ExodqEG?authentication_hash=bYAdyOGvRaRk

Clicking on Pair Device will bring up a window where you have to select Corne from the list (I have to switch to a new profile to appear there) then pair. After that you can check the logs in the developer console (F12 on Chrome) where the battery level change events should be displayed.

Nicell · 2022-04-15T17:42:08Z

I just realized I can also share the test app relatively easily: https://cdpn.io/pen/debug/ExodqEG?authentication_hash=bYAdyOGvRaRk

This link expired, could you send the non-debug link? Are we aware of any operating systems that utilize two BAS services? Conversely, does this change still work on OSs that don't? It would be nice to test this on a real OS that can show the two battery levels.

Katona · 2022-04-15T20:54:41Z

@Nicell I hosted it here: https://zmk-split-battery-report.herokuapp.com/ (JS playgrounds usually run the app in an iframe which can not access the Bluetooth API, so I tried to share the debug link instead).

As I said, I have only access to macOS Monterey which still displays only one battery, the central one. This at least means it doesn't break existing battery level reporting, but to be honest, I am not sure if it's a macOS limitation or a bug in my implementation, since Apple Airpods have proper battery level reporting: https://support.apple.com/en-us/HT207012#status. But of course, Airpods might be treated specially, I will try to investigate it further.

#764 claims that Windows 11 don't support it either, I have no info about linux unfortunately.

tadfisher · 2022-04-16T01:59:55Z

Apple has its own peripheral communication protocol that allows for multiple battery level reporting. I know my Sony earbuds support it.

Katona · 2022-04-16T11:17:03Z

Which is probably proprietary and we don't get the chance to implement it (without some certification). But I still think this PR makes sense even with zero OS support, we could create small apps which would display the battery level in system tray/menu, or is this overkill?

tadfisher · 2022-04-17T00:20:48Z

Actually it's pretty well-documented: https://developer.apple.com/accessories/Accessory-Design-Guidelines.pdf

Problem is, it's for devices using the hands-free profile (using HFP AT commands), so it's of little use for HID-over-GATT devices like keyboards.

Katona · 2022-04-17T21:16:26Z

@tadfisher You're right, I was not aware of that.

Katona · 2022-04-19T07:06:12Z

@Nicell Based on the discussion above, I think the best is to assume that no OS is supporting this at the moment (and to be honest, it's unlikely that they ever will be) but, as I said, it is possible to create some utility which can display the battery level (and other things as well) of both sides. I know it's not ideal, but I think it's feasible.

petejohanson

Thanks for working on this! I haven't had a chance to fully review all the details, but adding a few high level questions/comments.

petejohanson · 2022-05-23T14:14:46Z

app/include/zmk/events/battery_state_changed.h

+ZMK_EVENT_DECLARE(zmk_battery_state_changed);
+
+struct zmk_peripheral_battery_state_changed {


Hmm.... We already have the concept of position events having a "source" (see app/include/zmk/events/position_state_changed.h). If we're now going to have yet another event that is either from local or peripheral device, we might want to pull that out in a more generalized place, and simply supplement the existin battery state changed event w/ a source field as well. Thoughts?

I agree, I need some time to understand to how that's implemented/used.

petejohanson · 2022-05-23T14:17:09Z

app/src/battery_split.c

+    BT_GATT_CHARACTERISTIC(BT_UUID_BAS_BATTERY_LEVEL, BT_GATT_CHRC_READ | BT_GATT_CHRC_NOTIFY,
+                           BT_GATT_PERM_READ, read_blvl, NULL, &last_state_of_peripheral_charge),
+    BT_GATT_CCC(blvl_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE),
+    BT_GATT_CUD("Peripheral", BT_GATT_PERM_READ));


As we prepare to support multiple peripherals (see #1282 ) I think we might want to get that in first, and then work on getting this to runtime generate all the necessary characteristics based on how many configured peripherals there are?)

Yeah, I saw that and came to the same conclusion. Anyways, I need to create the utility which can display it meaningfully, currently I only have the test project based on the Bluetooth Web API :)

petejohanson · 2022-06-15T05:38:03Z

app/CMakeLists.txt

    target_sources(app PRIVATE src/behaviors/behavior_bt.c)
    target_sources(app PRIVATE src/ble.c)
    target_sources(app PRIVATE src/hog.c)
+    if (CONFIG_ZMK_SPLIT_BLE_ROLE_CENTRAL)


We moved to the more agnostic CONFIG_ZMK_SPLIT_ROLE_CENTRAL now:

Suggested change

if (CONFIG_ZMK_SPLIT_BLE_ROLE_CENTRAL)

if (CONFIG_ZMK_SPLIT_ROLE_CENTRAL)

Katona · 2022-06-24T15:32:03Z

app/Kconfig

@@ -157,6 +156,13 @@ config BT_PERIPHERAL_PREF_LATENCY
 config BT_PERIPHERAL_PREF_TIMEOUT
 	default 400

+if ((!ZMK_SPLIT_ROLE_CENTRAL) || (!ZMK_SPLIT)) 


Maybe there is a cleaner way to write this.

Depends on your definition of 'cleaner', but you could also do this

if (!(ZMK_SPLIT && ZMK_SPLIT_ROLE_CENTRAL))

or just remove the parenthesis around the two terms

if ( !ZMK_SPLIT || !ZMK_SPLIT_ROLE_CENTRAL )

is pretty clean

Katona · 2022-06-24T15:33:32Z

Making this PR more attractive:

I created a (very) POC macOS app which displays the battery levels in the menubar.

zhenchaopro · 2022-06-29T16:33:29Z

Making this PR more attractive:

I created a (very) POC macOS app which displays the battery levels in the menubar.

Awesome! Would it be possible to make it as a SwiftBar plugin?

Katona · 2022-06-30T16:50:13Z

@Steve-Fan probably yes, but as I said it's just a POC at the moment :)

zhenchaopro · 2022-07-01T02:06:48Z

@Katona Great! What API to use to get the battery info in MacOS? I'd love do some test.

Katona · 2022-07-01T07:51:29Z

@Steve-Fan I am using the Core Bluetooth API at the moment, but it's not too convenient in my opinion. I plan to use RxBluetoothKit, that seems better.

petejohanson

Couple other items I noticed.

app/Kconfig

kien242 · 2022-07-20T10:55:27Z

Hi sir, there is a bug i would like to report. When I build the firmware for Corne keyboard, I can't build for the main side when the OLED feature is enabled. it gives error : "battery_status.c:61: undefined reference to ` bt_bas_get_battery_level ' ". The slave side built normally.

petejohanson · 2022-07-20T15:54:27Z

Hi sir, there is a bug i would like to report. When I build the firmware for Corne keyboard, I can't build for the main side when the OLED feature is enabled. it gives error : "battery_status.c:61: undefined reference to ` bt_bas_get_battery_level ' ". The slave side built normally.

Yeah, this makes sense. We'll need a solution for a reasonable API abstraction for the consumers to show battery state, like for the display widget.

Katona · 2022-07-20T19:58:01Z

@kien242 Thanks for reporting it! I think I was able to fix this, but I don't have the necessary hardware to test it. @petejohanson is there way to test things when the required hardware is not available?

kien242 · 2022-07-21T03:06:38Z

@Katona, Thank you for fixing that bug. One wish of mine is probably a bit annoying to you, but Can you make a widget to display the slave's battery on the master's screen? I really wish there was that widget for a split keyboard with only one screen on the main side.

Katona · 2022-07-21T21:42:47Z

@kien242 Actually, I am working on making the display superfluous altogether :) Maybe it's just me, but I think there is no real use case of the screen, apart from maybe debugging.

kien242 · 2022-07-23T03:08:19Z

@kien242 Actually, I am working on making the display superfluous altogether :) Maybe it's just me, but I think there is no real use case of the screen, apart from maybe debugging.

@Katona, yes sir, but It seems that the widget is not working as expected, when I try with lily58's firmware, the OLED screen always says the battery capacity is 0

abonham · 2022-08-05T03:24:45Z

Can confirm it works like a charm on macOS 12, using a Ferris Sweep + nice!nano v2.0 x 2.

WarpspeedSCP · 2022-08-18T06:42:07Z

Yup, can confirm this working well on linux as well. problem is blueman also ignores the additional battery report.

If anyone is planning on using this code and rebasing on zmk master, I recommend creating a new branch from zmk master and manually patching any edited files. there are some conflicting changes in this branch which prevent a clean rebase. You will experience horrible cmake errors if you just try to rebase.

Maksim-Isakau · 2022-11-13T23:09:36Z

The feature is a must-have, thanks for this PR @Katona! Having dedicated displays for battery feels like a complete waste of energy.

Are we waiting for #836 to be merged before this PR can go further?

If anyone is looking for a Windows app, I made one based on Katona's POC: https://github.com/Fukkei/zmk-split-battery

snprajwal · 2023-04-06T19:43:50Z

Is this PR still in the pipeline to be added to the next ZMK release? I would love to have this feature available in the firmware by default (happy ZMK user with a Ferricy here)

Katona · 2023-04-12T11:24:22Z

@snprajwal The plan is/was to wait #836 to be merged first, then we can proceed with this. I am trying to keep this up-to-date with main, though.

In split_central_update_indicators_callback, it seems that the peripheral is sometimes considered connected before the GATT characteristics have been discovered. If this is the case, the update_hid_indicators handle will not yet be set, and must not be written to.

Babbie · 2023-09-20T20:07:23Z

Now that #836 has been merged, what is the plan for this PR? Would love to have this feature!

Katona · 2023-09-21T09:25:38Z

@Babbie I am planning to update the branch or or create a new PR depending on how big the changes are.

petejohanson · 2023-11-10T05:04:02Z

@Katona Any chance you've got time to work on addressing the review comments and merge conflicts on this? If not, will you be upset if I lend a hand?

Katona · 2023-11-10T09:22:54Z

@petejohanson I'll try to do it in the next few days, I'll let you know if I can't.

Urie96 · 2023-11-17T07:56:52Z

I added a zmk behavior in my fork to output the battery level of both sides, which ungracefully provides a way to view the peripheral battery level without relying on host software. 😂

Katona · 2023-11-24T20:28:24Z

@Katona Any chance you've got time to work on addressing the review comments and merge conflicts on this? If not, will you be upset if I lend a hand?

@petejohanson Sorry but I won't be able to finish this in the near future, so you can take this over.

* Add ability to fetch and report peripheral battery levels on split centrals. * Add additional support for adding a new Battery Level service to split centrals that exposes fetched peripheral battery levels to connected hosts. Co-authored-by: Peter Johanson <peter@peterjohanson.com>

petejohanson · 2024-01-03T17:28:19Z

Marking this closed now that #2045 is merged. Thanks @Katona for all the work on this and on the client application!

tomatolog · 2024-01-05T00:29:30Z

It is great to see that feature pushed into main branch.
However could someone could point to some usage example - maybe someone already changed the shield code and could provide the example?

Nicell added enhancement New feature or request split labels Apr 13, 2022

Nicell self-requested a review April 13, 2022 16:31

petejohanson reviewed May 23, 2022

View reviewed changes

petejohanson requested changes Jun 15, 2022

View reviewed changes

Katona commented Jun 24, 2022

View reviewed changes

Katona requested a review from petejohanson June 26, 2022 19:20

petejohanson requested changes Jul 11, 2022

View reviewed changes

app/Kconfig Outdated Show resolved Hide resolved

Katona mentioned this pull request Jan 22, 2023

Battery level is zero C0% P0% Katona/zmk-ble#2

Open

zhenchaopro mentioned this pull request Feb 10, 2023

Bluetooth security failing on windows machine #805

Open

equiman mentioned this pull request Mar 2, 2023

Not update after charging Maksim-Isakau/zmk-split-battery#4

Open

chrisandreae added a commit to moergo-sc/zmk that referenced this pull request Jul 30, 2023

Patch for zmkfirmware#1243: follow linter requirements

ff12237

petejohanson mentioned this pull request Nov 29, 2023

feat(ble): Support peripheral battery levels. #2045

Merged

petejohanson force-pushed the split_battery_service branch from b47fe6a to 05f7cd1 Compare November 29, 2023 22:56

petejohanson requested a review from a team as a code owner November 29, 2023 22:56

petejohanson force-pushed the split_battery_service branch from 05f7cd1 to b47fe6a Compare November 29, 2023 22:59

petejohanson force-pushed the split_battery_service branch from b47fe6a to e834a9e Compare December 9, 2023 20:47

petejohanson closed this Jan 3, 2024

lesshonor mentioned this pull request Feb 16, 2024

Added documentation for multiple battery monitoring #2167

Merged

nVitius mentioned this pull request Jul 1, 2024

feat(underglow): support command for displaying board statuses #2353

Open

		ZMK_EVENT_DECLARE(zmk_battery_state_changed);

		struct zmk_peripheral_battery_state_changed {

	if (CONFIG_ZMK_SPLIT_BLE_ROLE_CENTRAL)
	if (CONFIG_ZMK_SPLIT_ROLE_CENTRAL)

feat(battery): Split battery reporting over BLE GATT #1243

feat(battery): Split battery reporting over BLE GATT #1243

Conversation

Katona commented Apr 13, 2022

Katona commented Apr 15, 2022 • edited Loading

Nicell commented Apr 15, 2022

Katona commented Apr 15, 2022

tadfisher commented Apr 16, 2022

Katona commented Apr 16, 2022 • edited Loading

tadfisher commented Apr 17, 2022

Katona commented Apr 17, 2022

Katona commented Apr 19, 2022 • edited Loading

petejohanson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katona commented Jun 24, 2022

zhenchaopro commented Jun 29, 2022 • edited Loading

Katona commented Jun 30, 2022

zhenchaopro commented Jul 1, 2022

Katona commented Jul 1, 2022

petejohanson left a comment

Choose a reason for hiding this comment

kien242 commented Jul 20, 2022 • edited Loading

petejohanson commented Jul 20, 2022

Katona commented Jul 20, 2022

kien242 commented Jul 21, 2022 • edited Loading

Katona commented Jul 21, 2022

kien242 commented Jul 23, 2022

abonham commented Aug 5, 2022

WarpspeedSCP commented Aug 18, 2022 • edited Loading

Maksim-Isakau commented Nov 13, 2022

snprajwal commented Apr 6, 2023

Katona commented Apr 12, 2023

Babbie commented Sep 20, 2023 • edited Loading

Katona commented Sep 21, 2023

petejohanson commented Nov 10, 2023

Katona commented Nov 10, 2023

Urie96 commented Nov 17, 2023

Katona commented Nov 24, 2023

petejohanson commented Jan 3, 2024

tomatolog commented Jan 5, 2024

Katona commented Apr 15, 2022 •

edited

Loading

Katona commented Apr 16, 2022 •

edited

Loading

Katona commented Apr 19, 2022 •

edited

Loading

zhenchaopro commented Jun 29, 2022 •

edited

Loading

kien242 commented Jul 20, 2022 •

edited

Loading

kien242 commented Jul 21, 2022 •

edited

Loading

WarpspeedSCP commented Aug 18, 2022 •

edited

Loading

Babbie commented Sep 20, 2023 •

edited

Loading