-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix: Refactored the battery notification script with added feature and fix for inconsistent notifications #3832
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
base: dev
Are you sure you want to change the base?
Conversation
…ations, slightly altered parsing logic which should fix inconsistent notification issues, omarchy-battery-remaining script is now implemented inside this script rendering it obsolete unless it is used by some other scripts
|
Hi! 👋 I'm also seeing a lot of ENOENT noise in the systemd journal because of race conditions when he try tracking these short-lived processes. 🔍 Click to see the trace log evidence (ENOENT storm)trace > omarchy-battery.log Proposal: We can achieve the exact same logic with near-zero overhead (<1ms execution) by reading directly from the kernel interface (/sys/class/power_supply/) using pure Bash built-ins, instead of shelling out to upower. |
Hello there, yes i have been thinking on reading directly from kernel but i choose not to stray too far from the original script just to whip up something fast and test while my battery was discharging to find out why exactly it was working inconsistently(which i couldn't exactly tell why), but i guess you are right it can be a lot cleaner and free of overhead, i might look into it later but you are free to contribute upon this pr i have sent you a collaboration request to my fork. |
|
Hi @brutalsam96 — thanks for the invite! On second thought, instead of bundling the refactor here, I think it's safer to just fix the fullscreen notification issue (#3066) first while maintaining the current logic. Actually the problem is not the bash script alone ( notify-send -u critical) this flag is already there but cannot force the visual layer. It can only request "critical" status; the config file decides where "critical" things get drawn. Without the config change, the script fix is useless for fullscreen scenarios. Im testing out if this could be the final fix for the issue you mentioned, maybe i will make a separate pr for this one, but i would like to collaborate for a refactor later as well, in orther to use sysfs if upower is not prefered istead for this use case. But im not sure is gonna be accepted (i already tried it locally and works fine on my thinkpad) |
|
@scale03 no problem, i have found that we have status and capacity at the /sys/class/power_supply, this is much easier than parsing full info from upower, but now need to dive deeper and if all drivers provide same status and capacity files also the path /sys/class/power_supply/BAT0, need to also find out if BAT0 is always the name for the battery and maybe handle multiple battery sources. |
|
@brutalsam96 Nice, i just created a pr to fix the silent death while on full screen. #3838 completely agree on dropping the upower parsing for direct sysfs reads could be a huge win in the log run. Since BAT0 isn't universal (and some machines have multiple batteries like BAT0 + BAT1), we probably shouldn't hardcode the path. I suggest we iterate through /sys/class/power_supply/* and check the type file to confirm it's a "Battery" before reading status and capacity. That way we support multi-battery setups and different driver naming conventions out of the box. |
|
@scale03 Then again this shouldn't do much with overhead and context switch problems, i will research more regarding this matter, until then i will push updated commit that utilizes DisplayDevice instead of BAT. |
|
I'm not saying that this is a problem, I'm just saying that for this monitor battery script and maybe something else too the logic is simple enough that could be refactored in bash only. I notice also that systemd is producing every 30 second A LOT of enoet while monitoring the battery monitor script's childs but is just noise not a problem, and the fact that runs every 30 sec to check the battery life could make sense to optimize it (is good for the battery itself) but probably upower is still necessary somehow so maybe is better open a discussion to discuss about this instead of commenting this pr. Cheers |
ryanrhughes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some feedback for a few small changes. I've only validated this on my laptop but to others' points; do not know what this might do in a situation where you have a UPS or something on a desktop. However, in those cases, perhaps you'd also want the same notice.
…ining to use more concise logic to extract remaining percentage digits and simplified most of the script as suggested
|
Added requested changes. |
Summary
Refactored the omarchy-battery-monitor script to improve reliability of battery notifications.
Changes
Rewrote battery level fetching and parsing logic, removing dependency on omarchy-battery-remaining call that may have caused inconsistent notifications mentioned in issue #3066
Updated notification flow to use two levels:
A higher-threshold warning.
A lower-threshold critical alert.
Outcome
Should ensure battery notifications trigger consistently and notify twice now.
P.S If omarchy-battery-remaining script is not used elsewhere it should be obselete and safe to remove.