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

Added documentation for multiple battery monitoring #2167

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

dennisorlando
Copy link
Contributor

addressed by #2166

docs/docs/config/battery.md Outdated Show resolved Hide resolved
docs/docs/config/battery.md Outdated Show resolved Hide resolved
docs/docs/features/battery.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this page should be modified. Duplication aside, these additional configuration settings are split-specific and probably shouldn't be enabled by default given how limited host-side support currently is.

I also don't think it is a good idea to link this specific application as part of ZMK's documentation. Perhaps if it was a well-known program with a lot of activity, but this appears to almost be a one-off by a single developer for testing on Windows. Linking to an application in the docs implies that it is ZMK-sanctioned even if that isn't explicitly stated; this puts pressure on not only Maksim-Isakau but also ZMK collaborators for continued support/development.

...A roundup of peripheral battery-level reporting applications might be a good fit for some kind of a community spotlight type post on the ZMK blog, though...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually the case, those options are disabled by default.
I wanted to modify this page in order to mention that it is actually possible to monitor both battery levels; if not, then people (just like I did) will suppose that it's just not possible, which will make them not even try to find a solution. If you google the issue, you don't find anything -> only reddit post that comes up from googling "zmk split battery monitoring"

Hmmm I agree that the program shoudln't be linked; I think a blog post on ZMK would be cool

Copy link
Contributor

Choose a reason for hiding this comment

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

The absence of ways to actually display the multiple battery level information was my main hang-up on expanding this documentation. I also am not a big fan of directly linking to 3rd party projects in the docs, but I'd like to hear others' opinions on it.

That being said, the current status of the PR seems like a strict improvement over the status quo.

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 think that solutions will start to pop up once the docs mention the issue; there are some scripts out there which display both battery levels, they are just not that easy to find.
I myself will probably start working on a GNOME extension in the near future

Copy link

Choose a reason for hiding this comment

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

On the topic of displaying battery levels, I recently opened a PR that addresses this in the same way that MoErgo did for their Glove80:
#2353

It adds support for displaying battery level via RGB underglow on the keyboard.

@dennisorlando dennisorlando force-pushed the main branch 3 times, most recently from 281e57d to dc627c5 Compare February 16, 2024 10:59
docs/docs/config/battery.md Outdated Show resolved Hide resolved
docs/docs/features/battery.md Outdated Show resolved Hide resolved
Co-authored-by: Cem Aksoylar <[email protected]>

Added documentation for peripheral battery monitoring
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

LGTM but I'll wait for @lesshonor 's input.

@caksoylar caksoylar merged commit 604af2e into zmkfirmware:main Feb 18, 2024
7 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.

4 participants