Rockbox

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.

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

   battery-optimized-cleaned.diff (7.3 KiB)

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.

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.

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.

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

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.

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

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.

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

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?

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

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.

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?

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.

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

I agree with you, that this is a bug. But it is not related to this patch.

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.

Is the COP kept 100% asleep if no threads are living on it or are interrupts still waking it all the time?

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.

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.

   battery-optimized-cleaned-upd... (9.3 KiB)

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

If I remember correctly, then it was like:
LCD: 12mA
ATA: 4mA
CPU: 6mA

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.

Can this ticket be closed?

Christian,
I think this can be closed. Except activating frequency scaling the ideas of this patch are now available in svn.