Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9155 - Simplified battery bench plugin

Attached to Project: Rockbox
Opened by Bertrik Sikken (bertrik) - Saturday, 05 July 2008, 13:13 GMT+2
Last edited by Bertrik Sikken (bertrik) - Friday, 15 August 2008, 00:46 GMT+2
Task Type Patches
Category Battery/Charging
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Version 3.0
Due Date Undecided
Percent Complete 100%
Private No

Details

Attached patch contains the current work-in-progress for a simplified version of the battery bench plugin. The main difference with the old one is that the battery measurements themselves are now done purely on a time basis (once a minute) and do not depend on disk activity or disk spindown times. Writing the measurements to disk is still as unobtrusive as possible (written when the disk is active anyway).

Other changes:
* Now uses the ata idle callback mechanism to flush measurements to disk, instead of polling for disk activity with ata_disk_is_active(). Infact, this patch removes ata_disk_is_active from the plugin API and adds the ata idle register/unregister functions.
* Skipped measurements are no longer kept track of, because they are nearly impossible. The only problem occurs when the measurement buffer gets full after > 16 hours of no disk activity, which is rather unusual. The current implementation chooses to save the data in that case (and spin up the disk).
* Measurements/disk activity is no longer logged, because it makes no sense with time-based logging.

Things to consider:
* The measurement thread simply sleeps for 60 seconds between measurements, resulting in a measurement interval that is actually slightly more than 60 seconds.
* Only tested on my sansa e200 (which is a flash target), so it has not been tested on a DAP with a hard disk yet. I tested the buffer-full scenario by modifying the source for a small measurement buffer (500 bytes) and short measurement interval (10s).
   battbench_r17943.patch (19.8 KiB) 
 apps/plugins/battery_bench.c |  297 ++++++++++++++++++-------------------------
 apps/plugin.c                |    9 -
 apps/plugin.h                |   21 +--
 firmware/events.c            |   14 --
 4 files changed, 147 insertions(+), 194 deletions(-)

This task depends upon

Closed by  Bertrik Sikken (bertrik)
Friday, 15 August 2008, 00:46 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed as svn r18281
Comment by Jonathan Gordon (jdgordon) - Saturday, 05 July 2008, 14:04 GMT+2
much better :)

one thing to remember is that on the flash targets, the ata callback is called every 3s or so (iirc)....
Comment by Andree Buschmann (Buschel) - Wednesday, 09 July 2008, 21:24 GMT+2
Worked fine on my iPod 5.5G. I will perform the next run with full charged battery and switched off accessory supply to check whether the expected results come up.
   battery_bench.txt (51.3 KiB)
Comment by Bertrik Sikken (bertrik) - Thursday, 10 July 2008, 12:24 GMT+2
jdgordon, as far as I interpreted the code the ata idle callbacks are called 3 secs after the last ata activity, not every 3 secs.

Buschel, thanks for testing so far. Did you notice anything strange? Can you verify that it does no unnecessary disk spinups and that logging is complete (measurement cache should be flushed on shutdown)?
Comment by Andree Buschmann (Buschel) - Friday, 11 July 2008, 08:16 GMT+2
Bertrik, here is the second bench. This time the runtime result is quite near to what I've expected. So, there seems to be no additional disk spin-up or anything else that might impact battery life. Nevertheless I am confused that the bench again stops at 3.5V or 3-4%. Normally my benchs show voltages down to 3.3V (0%) -- which equals up to 10-20min additional runtime.

E.g. http://www.rockbox.org/twiki/bin/viewfile/Main/IpodRuntime?rev=1;filename=battery_bench_080505_mpc_24_nolineout.txt.

Maybe the flush at shutdown is not performed?
   battery_bench_acc_off.txt (57.4 KiB)
Comment by Bertrik Sikken (bertrik) - Sunday, 13 July 2008, 13:29 GMT+2
Buschel, in your nolineout bench around 14:20:25 the voltage dips from 3.559 to 3.395. This dip is about the same voltage (3.557) and at about the same time (14:24:08) as the last voltage logged with the simplified battery bench. I think that somehow the player powered down because of this dip during the battery bench with my patch. There is an explicit flush in the simplified battery bench that is called when the plugin is stopped during the power-down procedure, so I think that the flush was performed correctly. I could add a (temporary) line to the text file ("End of battery bench" or something) on shutdown to verify that.

I don't know what your player's shutdown voltage is exactly, or what caused the dip. BTW, there seem to be some bugs in the battery runtime calculation code. At a voltage of 3.431, the estimated runtime is 0 minutes, but at 3.430 it becomes 2:20 again.

Attached is the patch updated to current SVN. It's a bit smaller because I stripped most of the non-essential white-space changes, but it's otherwise the same.
   battbench_r18023.patch (14.7 KiB) 
 apps/plugins/battery_bench.c |  261 +++++++++++++++++--------------------------
 apps/plugin.c                |    3 
 apps/plugin.h                |    8 -
 firmware/events.c            |    2 
 4 files changed, 114 insertions(+), 160 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 21 July 2008, 04:13 GMT+2
Just a small thing: AFAIK you either bump the plugin api version (not only the min version) or you add new functions to the end of the api (then you'll only need to bump the min version). Now, you've put new functions to into the middle of the api, but only bumped the min version.

See comments in plugin.h before and at the end of the plugin api.
Comment by Thomas Martitz (kugel.) - Monday, 21 July 2008, 04:20 GMT+2
Ah, since you even removed a function, I think you actually have to bump the whole version.

But, I possibly confused the two versions. Now, that I've read the comment again, I think you've done it correctly.
Anyway, you didn't take the opportunity to sort in any new function which are "waiting" at the end of the function table :D

Meh, I'm too tired right now...
Comment by Tomasz Wasilczyk (tomkiewicz) - Monday, 21 July 2008, 14:07 GMT+2
there is related task:
http://www.rockbox.org/tracker/task/8070
maybe some ideas (ie. more frequent flushing when battery is under 10%) would be helpful
Comment by Bertrik Sikken (bertrik) - Monday, 21 July 2008, 20:08 GMT+2
Thomas, you're right about me having to move the waiting functions, I'll do that when this finally gets committed.

Tomasz, I added task 8070 as a related task. I'd like to keep the amount of special cases (like more frequent flushing) to an absolute minimum. I don't want to fix something if it isn't clear that it is a problem in the first place. Besides, more frequent flushing will make the battery bench less true to the normal situation (because of the extra disk spinups).

Attached is a new version of the patch. It is updated to svn r18813 and fixes the problem that it would not compile in the simulator.
   battbench_r18113.patch (14 KiB) 
 apps/plugins/battery_bench.c |  261 +++++++++++++++++--------------------------
 apps/plugin.c                |    5 
 apps/plugin.h                |    6 
 firmware/events.c            |    2 
 4 files changed, 116 insertions(+), 158 deletions(-)

Comment by Bertrik Sikken (bertrik) - Sunday, 27 July 2008, 15:03 GMT+2
Attached is a new version of the battery bench, updated to svn r18127. Changes:
* on exit, the battery bench now logs a final line with the reason for exiting (normal plugin exit or power off). This way you can detect if a battery bench terminated abnormally.
* fixed compilation issue for ifp7xx.
* minor bugfix in buffer overflow handling.
   battbench_r18127.patch (14.8 KiB) 
 apps/plugins/battery_bench.c |  275 ++++++++++++++++++-------------------------
 apps/plugin.c                |    5 
 apps/plugin.h                |    6 
 firmware/events.c            |    2 
 4 files changed, 128 insertions(+), 160 deletions(-)

Comment by Bertrik Sikken (bertrik) - Sunday, 27 July 2008, 18:55 GMT+2
Stupid bug on my part when combining an assignment and an if statement. Fixed in attached patch.
   battbench_r18131.patch (15.2 KiB) 
 apps/plugins/battery_bench.c |  281 ++++++++++++++++++-------------------------
 apps/plugin.c                |    5 
 apps/plugin.h                |    6 
 firmware/events.c            |    2 
 4 files changed, 130 insertions(+), 164 deletions(-)

Loading...