Rockbox

Tasklist

FS#7036 - Sansa: Increase battery runtime

Attached to Project: Rockbox
Opened by Toni (ahellmann) - Sunday, 15 April 2007, 20:24 GMT
Task Type Patches
Category Battery/Charging
Status Closed
Assigned To No-one
Operating System Sansa e200
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Increase battery runtime on the sansa e200 by:
1. enabling frequency scaling
2. shutdown of the lcd controller (if backlight off)
3. shutdown of the ata controller (if no diskaccess in progress)
According to the current measurements the current is reduced from 65mA (current svn) to 45mA (patched) with mp3 128kB and medium volume. That means some more hours of runtime per battery cycle.
In addition this patch includes the updated anti audio glitch patch.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Saturday, 19 May 2007, 08:48 GMT
Reason for closing:  Accepted
Additional comments about closing:  cpu freq scaling wasnt accepted, but will be eventually..
Comment by Paul Louden (Llorean) - Sunday, 15 April 2007, 21:56 GMT
Seriously, do not post patches that attempt to solve more than one problem. Make one patch per objective. There's no reason to re-post the anti-glitch fixes.

For patches to be included, they should have individual purposes.
Comment by Toni (ahellmann) - Monday, 16 April 2007, 17:56 GMT
Well, this update removes the anti audio glitch part.
But be aware, that you need the above full patch for better audio quality (no channel swapping, etc).
The previous posted anti audio glitch patch http://www.rockbox.org/tracker/task/6908 doesn't sync anymore with current svn.
So for ease better use the cumulative patch above. :)
Comment by MichaelGiacomelli (saratoga) - Tuesday, 17 April 2007, 03:14 GMT
Toni:

This is a bit unclear to us on IRC. Does this patch depend on #6908? If so, perhaps you could separate these into multiple .patch files that could be independently tested? It seems to me the main complaint with 6908 (this may be incorrect), is that disabling those cache flushes had other side effects unrelated to what it was fixing. I don't see any obvious relationship between cache flushing and disabling the ATA controller and enabling frequency scaling. Perhaps these two issues could be put in seperate tasks so that they can be independently tested, and so that they are not bottlenecked with the cache flushing issue is examined.
Comment by Toni (ahellmann) - Tuesday, 17 April 2007, 05:51 GMT
The frequency scaling makes the memory lock issue more obvious. Probably timing related the lockings appear more often. I also don't think, that the DEV_EN manipulations have any effect on the audio. I have not experienced (and not seen from others) any problems with #6908, but maybe I am wrong.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 17 April 2007, 08:27 GMT
fwiw, I'm listening to a track which kept swapping left/right channels this morning and it sounds perfect now :)

having a look at the debug audio screen, the boost ratio is 50% (which should be fine, but its constantly boost-unboosting it) so I wonder if there is any real saving from enabling cpu scaling because of all the time spent switching?

and has been said, please split fs#6908 and update that patch separately.
Comment by Toni (ahellmann) - Tuesday, 17 April 2007, 16:04 GMT
That is good news. As I pointed out, #6908 is only a workaround. If I understand it correctly, then you halfed the calls to lcd_update/lcd_update_rect?

50% boost means half of the time at 30MHz the half 75MHz, which leads to 50% of the time 45MHz corresponding to 11mA current saved.
So this is already a noticable reduction (5.5mA from 65mA => about 1hour more play time).
Comment by Toni (ahellmann) - Tuesday, 17 April 2007, 17:16 GMT
jdGordon

I checked your commits together with the 'battery-optimized-cleaned.diff' patch, which only reduces power consumption:
- still get those ticks and channel swapping (due to frequency scaling enabled)
- after a certain time the lcd display fades out (don't know exactly, wether this happened to the patch before)

I also checked your commits together with the 'battery-optimized.diff' patch, which reduces power consumption AND removes audio glitches:
- no issues found

Conclusion: The audio glitches are very obvious and the display is affected too, if frequency scaling is enabled. Currently, if you want good sound AND better battery runtime: 'battery-optimized.diff' patch is best. I will update patch #6908 for a not completely impossible inclusion in svn.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 17 April 2007, 22:09 GMT
"50% boost means half of the time at 30MHz the half 75MHz" yes, but on say iriver, if its at 50% it means that it sits at boosted for a while, then unboosts for a while.. here its constantly switching which probably is bad.

which commits are you talking about? the button change? that shouldnt affect anything.
Comment by Toni (ahellmann) - Wednesday, 18 April 2007, 05:49 GMT
jdgordon

It's perfectly ok, that the sansa switches permanently from boosted to unboosted. The boost itself should take around ~10ms, so if it switches at an interval of 2/sec, it is only 'wasted' 10ms from 500ms.
Ah, I thought you talked about your commits yesterday morning. I thought those did reduce the channel swapping.
Comment by Daniel Ankers (dan_a) - Thursday, 19 April 2007, 07:32 GMT
Toni,
Nice work!
I can't see any problem with these. If nobody beats me to it, I hope to get them committed this weekend (after testing them on my Sansa.)
Dan
Comment by Barry Wardell (barrywardell) - Saturday, 21 April 2007, 15:48 GMT
Looks good. I think maybe we could add #define's for (1<<14) and (1<<26) to pp5020.h, or maybe pp5024.h. This would make it a little clearer what's happening.

Also, why do you only do lcd_enable(true) for non-bootloader builds?
Comment by Toni (ahellmann) - Saturday, 21 April 2007, 17:14 GMT
I completely agree with the #defines. This should be done for all DEV_EN bits (not only those in the patch).

lcd_enable() is only set for non-bootloader builds, because otherwise you get the bootloader text, which is not wanted me thinks.

If you apply the 'cleaned' patch be aware of the additional audio problems (without applying #6908).
This is why I am still for 'battery_optimized.diff'.
Comment by Paul Louden (Llorean) - Saturday, 21 April 2007, 17:20 GMT
Bootloader text is avoided by not calling lcd_update(), you still need to be able to show the bootloader text if it encounters any of the errors.
Comment by Toni (ahellmann) - Saturday, 21 April 2007, 17:42 GMT
I didn't see that. So I have to expand the patch in that respect. Currently the 'battery_optimized.diff' either always or never enables the lcd dma. How about adding the lcd_enable(true) in the bootloader when needed?
Comment by Toni (ahellmann) - Saturday, 21 April 2007, 19:35 GMT
I had a look into the current svn bootloader again. Still I don't see any special code, which handles error messages differently from standard text. It seems that the error messages are totally ignored.
Comment by Paul Louden (Llorean) - Saturday, 21 April 2007, 19:40 GMT
Look at the bootloaders for other targets.

They don't call lcd_update to draw the text unless they encounter certain errors, or you hold down a button (usually the Right button).
Comment by Toni (ahellmann) - Saturday, 21 April 2007, 21:06 GMT
I agree with you, that this is a bug. But it is not related to this patch.
Comment by Barry Wardell (barrywardell) - Saturday, 21 April 2007, 21:33 GMT
In the SVN bootloader, the error() function is called when there is a fatal error. That function does the lcd_update(). Otherwise, lcd_update() is done by printf() if verbose mode is enabled by holding any button down. There isn't any bug that I know of in the SVN bootloader that is related to this.
Comment by Michael Sevakis (MikeS) - Saturday, 21 April 2007, 23:03 GMT
Is the COP kept 100% asleep if no threads are living on it or are interrupts still waking it all the time?
Comment by Paul Louden (Llorean) - Sunday, 22 April 2007, 02:25 GMT
I didn't say there was a bug. I was pointing out that the lcd bootloader text is wanted in specific circumstances, in response to his saying it's not wanted.
Comment by Toni (ahellmann) - Sunday, 22 April 2007, 09:46 GMT
Sorry, I had installed an elder bootloader here, which indeed did not show any error text.
The current svn bootloader is perfectly fine in that respect.

Now I have updated both patches, to work well with the bootloader and the application.
- removed the suspicious lcd_enable(true) from lcd_init (device is enabled already earlier)
- always indicate backlight on for the bootloader

For flawless audio playback also the updated anti-audio-glitch-resynced-update have to be applied.

MikeS: There is no special power saving treatment for the cpu/cop besides the frequency scaling.
Comment by Barry Wardell (barrywardell) - Monday, 23 April 2007, 12:25 GMT
I tried this out and it seems like it's good to go for SVN. Out of curiousity, have you done current measurements to see how much difference each of the 3 changes makes (LCD, ATA, CPU frequency)?
Comment by Toni (ahellmann) - Monday, 23 April 2007, 18:15 GMT
If I remember correctly, then it was like:
LCD: 12mA
ATA: 4mA
CPU: 6mA
Comment by Barry Wardell (barrywardell) - Monday, 23 April 2007, 23:27 GMT
I've committed the LCD and ATA changes, but left out the CPU frequency scaling because of the problems it causes with audio playback. Thanks for the great work Toni.
Comment by Christian Gmeiner (ChristianGmeiner) - Friday, 18 May 2007, 08:36 GMT
Can this ticket be closed?
Comment by Toni (ahellmann) - Saturday, 19 May 2007, 07:45 GMT
Christian,
I think this can be closed. Except activating frequency scaling the ideas of this patch are now available in svn.

Loading...