Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Battery/Charging
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by dreamlayers - 2012-01-20
Last edited by dreamlayers - 2012-01-21

FS#12555 - Displayed battery level is not smoothed

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.

Closed by  dreamlayers
2012-01-21 23:51
Reason for closing:  Fixed
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

109084d

MikeS commented on 2012-01-20 02:04

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.

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.

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).

MikeS commented on 2012-01-20 14:24

So, what is more “general”? battery_(something generic)_voltage?

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.

http://git.rockbox.org/?p=rockbox.git;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).

Okay, that’s nicely documented (thanks!), and it’s reasonable to rename battery_adc_voltage() for consistency.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing