FS#12555 - Displayed battery level is not smoothed

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 01:49 GMT
Last edited by Boris Gjenero (dreamlayers) - Saturday, 21 January 2012, 23:51 GMT
Task Type Bugs
Category Battery/Charging
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


I recently found that displayed battery level can change very rapidly after the hard drive spins down. This is very easy to see on my 5G iPod with the original 30GB hard drive, running 007f61f. It happens because battery_read_info() calls _battery_voltage(), which reads battery voltage from the hardware. I changed it to call battery_voltage(), which returns smoothed voltage, and that restored the familiar old behaviour. I'm attaching the patch.

I think some smoothing is needed. Maybe the old behaviour was doing too much smoothing, but total lack of smoothing is worse.

This only affects targets which measure battery voltage. The changed code also builds on other targets, because they provide a battery_voltage() function which just returns -1.
This task depends upon

Closed by  Boris Gjenero (dreamlayers)
Saturday, 21 January 2012, 23:51 GMT
Reason for closing:  Fixed
Additional comments about closing:  109084d
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 02:04 GMT
battery_read_info was intended to read unfiltered voltage. It wasn't supposed to be used for monitoring decisions.

ETA: And battery_adc_voltage was renamed regardless of the fact that the "adc" was an important hint as to what it did.
Comment by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 05:10 GMT
Ok, I see the change I proposed is wrong: the debug screen ought to be getting current ADC values via battery_read_info(). I hope this is better.
Comment by Thomas Martitz (kugel.) - Friday, 20 January 2012, 13:27 GMT
Looks good to me.

Micheal, I don't see how having adc in name is an important hint. It's exposing an unintersting implementation detail in my book (fwiw, some hosted targets that can report it don't even use adc for that).
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 14:24 GMT
So, what is more "general"? battery_(something generic)_voltage?
Comment by Boris Gjenero (dreamlayers) - Saturday, 21 January 2012, 19:33 GMT
I committed the v2 patch in 109084d.

The function names here aren't very helpful. _battery_voltage() makes me think it's internal or more low level than battery_voltage(), but it doesn't explain the difference and battery_adc_voltage() seems more descriptive. If the value is processed a lot, for example, being smoothed by lower level code, then the name hints that the value needs to be treated differently (instead of being returned via battery_adc_voltage() and smoothed again). Also battery_read_info() isn't descriptive. Why should one expect that it returns values that aren't smoothed? However, I don't think these things are worth worrying about now.
Comment by Thomas Martitz (kugel.) - Saturday, 21 January 2012, 19:45 GMT;a=blob;f=firmware/export/powermgmt.h;h=14d0078c3160f5458ae45e834d66788b6a9c6908;hb=HEAD#l131 has a short description of battery_{voltage,level,time} and their _battery_* counterparts. It also mentions that _battery_* is unfiltered and implemented by the target (so yes, they are indeed internal and more low level but the difference is actually explained). So I don't see what's wrong with them.

battery_adc_voltage doesn't exist anymore, it has been renamed to _battery_voltage because a) adc is an implementation details which is not necessarily true, and b) _battery_time and _level where added and it should be consistent (battery_adc_time and battery_adc_level would have been absolutely wrong).
Comment by Boris Gjenero (dreamlayers) - Saturday, 21 January 2012, 23:50 GMT
Okay, that's nicely documented (thanks!), and it's reasonable to rename battery_adc_voltage() for consistency.