Skip to content

Refactor getBattery function for voltage reading #1915

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.

Sign up for GitHub

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

Jump to bottom
Merged
pr3y merged 1 commit into from
Dec 11, 2025
Merged

Refactor getBattery function for voltage reading #1915

pr3y merged 1 commit into from
Dec 11, 2025

Conversation

@jader242
Copy link
Contributor

@jader242 jader242 commented Dec 10, 2025

Refactor getBattery function to improve voltage calculation and ensure percent is clamped between 0 and 100.

Proposed Changes

Changed battery calculation math/logic

Types of Changes

added voltage divider compensation per the m5unified library, both upper and lower ends bound

Testing

I have not tested it yet, but I will in a few hours

Linked Issues

None yet

User-Facing Change

Should drastically increase battery percentage accuracy, as it stands now Bruce is very innacurate often being 30-40% off what the power function of M5Unified returns

Further Comments

None atm

@jader242
Refactor getBattery function for voltage reading
83f4b27 
Refactor getBattery function to improve voltage calculation and ensure percent is clamped between 0 and 100.
@bmorcelli bmorcelli changed the base branch from main to dev December 11, 2025 00:26
@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

I also removed a couple unnecessary/redundant lines such as setting GPIO10 to input as analogReadMilliVolts() already does this, also changed the voltage to be stored as a 16 bit unsigned integer as 32 bit is very very overkill lol

@bmorcelli
Copy link
Member

bmorcelli commented Dec 11, 2025

Making the math in the float realm is a good idea as It removes the risk of lose some values in the float/int conversions mid calculation

But I didn't understand why you multiplied the voltage by 2... It will give you always 100% in the end...

    uint16_t adcReading = 3600; // lets say it read 3600mV, around 35% of the battery 
     float actualVoltage = 3600 * 2.0f; // 7200
     const float MIN_VOLTAGE = 3300.0f; 
     const float MAX_VOLTAGE = 4150.0f; 
     float percent = ((7200 - 3300) / (4150 - 3300)) * 100.0f; // (3900/850)*100 = 454%
     if (percent < 0) percent = 0; 
     if (percent > 100) percent = 100; 
     return (int)percent;

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

Because GPIO10 reads the voltage after a voltage divider, so you must account for that by multiplying it by two. Idk if you've ever used any other firmwares [edit to add: on the cardputer], but Bruce reports wildly differently than others including one I made that uses the function built into m5unified. for example, a week after using my cardputer everyday, Bruce was still reporting 85%, yet (for example) Evil-M5 was reporting around 40%. So to get to the bottom of it, I wrote a basic firmware that printed the battery percent via M5Cardputer.Power() and it was pretty much spot on to Evil-M5. Here is the section of the Power_Class that corresponds to the cardputer, notice the 2x ADC ratio

case board_t::board_M5CardputerADV:
_batAdcCh = ADC1_GPIO10_CHANNEL;
_batAdcUnit = 1;
_pmic = pmic_t::pmic_adc;
_adc_ratio = 2.0f;
break;

https://github.com/m5stack/M5Unified/blob/master/src/utility/Power_Class.cpp

I'm pretty sure yours is underflowing tbh, since at full charge pin 10 will read about 2.1V after the divider. So after all that math you do you end up with a negative battery percent every time

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

Just checked out of curiosity and evil-cardputer also accounts for the voltage divider

uint32_t mv = esp_adc_cal_raw_to_voltage(raw, adc_chars) * 2; // facteur 2 car diviseur interne

Edit: just saw on the sticker on the back of the cardputer it shows 1/2VBAT:G10 coming out of the voltage divider. Would've been much much easier to look there to confirm that instead of combing through crap tons of code like I did 😅

@pr3y pr3y merged commit 8f7d2d8 into BruceDevices:dev Dec 11, 2025
@bmorcelli
Copy link
Member

bmorcelli commented Dec 11, 2025

well well.. now it doesn't show anything when unplugged :(

better wrong then missing

@Huzzla101
Copy link

Huzzla101 commented Dec 11, 2025 via email

@bmorcelli
Copy link
Member

bmorcelli commented Dec 11, 2025

I fixed it... Now I need to fix a problem of other PR

@Huzzla101
Copy link

Huzzla101 commented Dec 11, 2025 via email

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025

What was the fix if you don't mind me asking?

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

@Huzzla101 why would I need to do it for every device? Do you not see that this one file is specific to the cardputer? Lololol

Sure the logic could probably benefit from being upgraded, but the biggest issue here (which is fixed now) was that Bruce wasn't accounting for the voltage divider so even at 100% the value was underflowing

@bmorcelli
Copy link
Member

bmorcelli commented Dec 11, 2025

All other devices that use ADC in fact use voltage divider..

The problem was in an implicit conversion.. now it is working and showing the same as Evil

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

Wait I'm confused now, it looks like you removed the whole section of code in question if I'm not mistaken. So where is it accounting for the voltage divider (2x factor for the cardputer, but it may be different for other devices idk)? Is it in another file I'm not seeing?

Edit: is it the ini file maybe?

@bmorcelli
Copy link
Member

bmorcelli commented Dec 11, 2025

I removed the redundant code from all envs that use the same kind of logic, moved the code to other file and set the flags on these environment config files

https://github.com/BruceDevices/firmware/blob/dev/src%2Fcore%2Futils.cpp#L32-L57

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

Gotcha, I ended up finding it right before you made that comment 😅 out of curiosity (and to learn) what was the issue with the code I submitted? I have a feeling it was with removing the line that sets pin 10 to input, as after researching it I was pretty sure analogReadMilliVolts() already did that, but I could've been wrong I guess. Doing a quick comparison of the two, that's the only change that stands out to me. Am I correct?

Edit: or I suppose it could've been an issue with using uint16_t instead of uint32_t?

@Huzzla101
Copy link

Huzzla101 commented Dec 11, 2025 via email

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025

@Huzzla101 idk if you meant it, but your original comment came off very condescending/passive aggressive/dickish, like:
"So how then will you fix this Mr Jader242... =) As all respected coders
finish what they start--"

Yea, I wasn't going to just leave it not working... Why would I go through all of the work of figuring out what the issue was if I was just going to leave it in an unworking state? I was taking a nap after working an 8 hour day and being a full time student, so I apologize I didn't see piratas notification the second he made the comment saying something was wrong with my code. Unfortunately, I do not have all the time in the world as you say. If you indeed do, count your blessings my friend

@jader242
Copy link
Contributor Author

jader242 commented Dec 11, 2025 •
edited
Loading

@bmorcelli what was the charge state of your battery when you tested it and it was blank unless plugged in? I have a feeling your battery was relatively low, and the math I did was slightly off (at least lol) and likely calculated it was below minimum voltage and returned 0. Which would explain why it was blank, because when getBattery() is called from drawStatusBar() in display.cpp it only calls drawBatteryStatus() if getBattery() returns a value greater than 0

edit: wait I now see that you said the problem was with an implicit conversion, my bad, for some reason I didn't realize that was you telling me the issue with the original code. I thought that part of your reply was to something else. Thanks though! I hope to continue learning and contributing :)

@Huzzla101
Copy link

Huzzla101 commented Dec 12, 2025 via email

@jader242
Copy link
Contributor Author

jader242 commented Dec 12, 2025

@Huzzla101 well in that case no hard feelings. to be fair, this was my very first github pull request, and it's also some of the first c++ i've ever written. so you have the right to be skeptical of me. i'm really just here to learn. the cardputer is a fascinating device that i'm absolutely in love with, so i thought it would be a good way to learn to code. and i just happened to notice that bruce/launcher reported a battery percentage way different than other firmwares, and after bringing it up in the discord a couple times with no response, i figured why not fix it myself ya know? i hope to contribute more in the future, but as i'm still a beginner (i've only known that esp32s exist for about a month lol) i won't rush anything :)

@bmorcelli
Copy link
Member

bmorcelli commented Dec 12, 2025

@Huzzla101 well in that case no hard feelings. to be fair, this was my very first github pull request, and it's also some of the first c++ i've ever written. so you have the right to be skeptical of me. i'm really just here to learn. the cardputer is a fascinating device that i'm absolutely in love with, so i thought it would be a good way to learn to code. and i just happened to notice that bruce/launcher reported a battery percentage way different than other firmwares, and after bringing it up in the discord a couple times with no response, i figured why not fix it myself ya know? i hope to contribute more in the future, but as i'm still a beginner (i've only known that esp32s exist for about a month lol) i won't rush anything :)

I'm sorry for being rude before, I wasn't on a good day..

Bruce and Launcher are all about learning, you are welcome to ask whatever you need and contribute whenever possible..

Your suggestion triggered a modification on Launcher too.. so the Beta version is already fixed and working..

About the code you made, I have no idea why it didn't work.. because the changes I made were about converting to the right kind of variable in the calculation.. kinda misterious

@Huzzla101
Copy link

Huzzla101 commented Dec 12, 2025 via email

@Huzzla101
Copy link

Huzzla101 commented Dec 12, 2025 via email

@pr3y pr3y mentioned this pull request Dec 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jader242 @bmorcelli @Huzzla101 @pr3y