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

Autodetect relevant batteries instead of relying on hardcoded filenames. #448

Merged
merged 8 commits into from
Feb 15, 2025

Conversation

peterdk
Copy link
Contributor

@peterdk peterdk commented Feb 13, 2025

Battery on my Dell XPS 9345 Snapdragon X didn't show up in Resources. Turns out the naming of the battery in /sys/class/power_supply/ is not BATx but a whole different qcom-battmgr-bat. So it seems it's not very wise to rely on a hardcoded detection of ^BAT.

I did some research and figured out a quite simple way to detect which of the power_supply/* items are indeed system batteries and not your Mouse/Keyboard ones.

enumerate /sys/class/power_supply/*    -> conditions: type = "Battery" and scope != "Device"

Or to get a quick debug output:

for a in /sys/class/power_supply/*; do echo "$a: type: $(cat $a/type) scope: $(cat $a/scope 2>/dev/null)"; done

This was tested by a another user with a Dell laptop with a BAT0 entry. I tested it with my Snapdragon laptop. And will try to test it on another laptop as well. But it seems quite solid. Ofcourse, some extra testing would be really nice to see if we regress.

I wanted to add unittests for the logic, but there is no way to mock the fs part, and not sure if we want to use a temp filesystem structure. From what I see in the project the sysfs reading logic is not really tested, so I omitted them.

@peterdk
Copy link
Contributor Author

peterdk commented Feb 13, 2025

Debug bash output on Dell XPS 9345:

/sys/class/power_supply/hidpp_battery_0: type: Battery scope: Device
/sys/class/power_supply/hidpp_battery_1: type: Battery scope: Device
/sys/class/power_supply/qcom-battmgr-ac: type: Mains scope: 
/sys/class/power_supply/qcom-battmgr-bat: type: Battery scope: 
/sys/class/power_supply/qcom-battmgr-usb: type: USB scope: 
/sys/class/power_supply/qcom-battmgr-wls: type: Wireless scope: 
/sys/class/power_supply/ucsi-source-psy-pmic_glink.ucsi.01: type: USB scope: System
/sys/class/power_supply/ucsi-source-psy-pmic_glink.ucsi.02: type: USB scope: System

On A x86 Dell laptop:

/sys/class/power_supply/AC: type: Mains scope: 
/sys/class/power_supply/BAT0: type: Battery scope: 
/sys/class/power_supply/hidpp_battery_4: type: Battery scope: Device
/sys/class/power_supply/ucsi-source-psy-USBC000:001: type: USB scope: System
/sys/class/power_supply/ucsi-source-psy-USBC000:002: type: USB scope: System
/sys/class/power_supply/ucsi-source-psy-USBC000:003: type: USB scope: System

HP Elitebook 840 G4 x86 laptop:

/sys/class/power_supply/AC: type: Mains scope: 
/sys/class/power_supply/BAT0: type: Battery scope: 

@peterdk
Copy link
Contributor Author

peterdk commented Feb 13, 2025

Dell XPS 9345 with this change:
Screenshot From 2025-02-13 18-52-44

@peterdk
Copy link
Contributor Author

peterdk commented Feb 13, 2025

Flatpak run fails, but seems a internal error not this PR. I can't rerun I guess

@nokyan
Copy link
Owner

nokyan commented Feb 13, 2025

Thank you for the effort! I'll check it out later today or tomorrow!

Flatpak run fails, but seems a internal error not this PR. I can't rerun I guess

Yeah, likely to be a Flatpak action thing (again). I'll look into getting it fixed soon, I'll tell you when I'm done so you can merge main again.

I wanted to add unittests for the logic, but there is no way to mock the fs part, and not sure if we want to use a temp filesystem structure. From what I see in the project the sysfs reading logic is not really tested, so I omitted them.

That's okay, no worries.

@nokyan
Copy link
Owner

nokyan commented Feb 15, 2025

Hi, I've merged a fix for the CI pipeline. :)

@peterdk
Copy link
Contributor Author

peterdk commented Feb 15, 2025

Ah nice, I guess I merge in main and push again?

@nokyan
Copy link
Owner

nokyan commented Feb 15, 2025

Ah nice, I guess I merge in main and push again?

Correct

@peterdk
Copy link
Contributor Author

peterdk commented Feb 15, 2025

Hmm, I rebased, maybe I should have normal merged? Sorry, figuring out a bit how this works with PRs and Github.
Not sure why the diff also shows the .github workflow file as change. Let me know if I need to change something here.

@nokyan
Copy link
Owner

nokyan commented Feb 15, 2025

Hmm, I rebased, maybe I should have normal merged? Sorry, figuring out a bit how this works with PRs and Github. Not sure why the diff also shows the .github workflow file as change. Let me know if I need to change something here.

The way you did it is correct, you've basically taken your changes and "put them on top" of the newest commit of main instead of the one that you started your branch with, hence my commits are now in there as well. :)

@nokyan nokyan merged commit c330856 into nokyan:main Feb 15, 2025
1 check 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.

2 participants