Rockbox

Hi Boris, I am again impressed! I wish I would have more time to test these changes. This was always one of my favorite tasks to save power. Did you already make any runtime measurements?

I have applied the patch and the screen is switched off after timeout. But it doesn’t wake up again (playback is running fine and volume control is working).

I must say that this patch works perfectly for me (iPod Video 5.5G 80GB).
I’ve configured it to sleep always when the backlight turns off and my impression is that It responds very fast.
Once again, thank you for your great work!

I don’t quite understand HAVE_LCD_SHUTDOWN thing.

lcd_enable is meant to turn off the lcd (and maybe clear it too), including cutting the power off.
lcd_sleep is, in contrast, meant to additionally turn the lcd controller off to save further energy.

lcd_enable(false) is always called if the backlight is turning off, while lcd_sleep has a (configurable) delay, since powering the lcd_controller on may noticeably delay the backlight on sequence.

So, as far as I understood, your lcd_shutdown as lcd_enable is completely right, without defining HAVE_LCD_SHUTDOWN.

Oops, I was still using the hard-coded location of the BCM firmware in flash. The patch finds the vmcs part of the flash via code based on a part of FS#6721, but I forgot to actually use what it found. This would cause wakeup to fail if the actual offset or size differs from the hard-coded value. Sorry! I’m attaching an updated patch. Hopefully that fixes the problem Andree had.

HAVE_LCD_SHUTDOWN causes lcd_shutdown() to be called in shutdown_hw() in firmware/powermgmt.c (when Rockbox is shutting down for power-off). Currently, power_off() in firmware/target/arm/ipod/power-ipod.c just clears the LCD and I made HAVE_LCD_SHUTDOWN disable that. I thought putting the LCD to sleep is better than clearing it, and implementing lcd_shutdown() is better than doing it in power_off(). However, this is not necessary for LCD sleeping functionality.

Yes, I’ve seen how lcd_enable is often called in backlight code of some targets to switch the LCD with the backlight. I didn’t feel that is appropriate for the 5G iPod because then the user couldn’t make the LCD stay on without the backlight, and the LCD can be usable without the backlight. I thought about separating the shutdown code into lcd_enable(false) which sleeps the LCD and lcd_sleep() which shuts down the BCM, but I saw no benefit in that.

   bcm_lcd_sleep-v0.2.patch (14.2 KiB)

Hi Boris, I will test your new patch soon. Nevertheless I have started a battery bench with the first patch applied. The bad thing is: the iPod crashed after a few hours (I do not know why, maybe caused by other changes). But the battery bench is VERY interesting in terms of power consumption. Please see the attached pdf. One graph shows a bench from 95-60% with my standard test suite and the other one with this patch applied. The steepness is MUCH slower – should be equal to savings of ~5mA. The runtime of my 5.5G 30GB should be quite over 18h with this patch

   bench.pdf (18.5 KiB)

Andree, thanks for the battery bench results! You’re right, that does look very good.

I wonder about that crash. I’m tempted to say it can’t be due to the patch because lcd-video.c is not really doing anything when the LCD is off. The only changes elsewhere are lcd_enabled() related and only scroll_engine.c calls that. Oh, and there are also timing changes due to lcd_tick() not running and the LCD update functions returning immediately.

BTW This means the WPS continues to be updated when the LCD is off. For that I shall take a look at  FS#8523 .

I’ve looked at GPO32_VAL & 0×8000. I think that might be an alternate power supply for BCM sleep mode. It uses less power than when GPO32_VAL & 0×4000 is set, but less than when both are cleared. It might be possible to wake the BCM from that mode without having to reload the firmware, but I haven’t been able to do that yet, and the current wakeup speed is probably fast enough.

Boris, your second patch works fine on my 5.5G 30GB. Nice work!
Btw 1, I am also pretty sure that the crash happened due to another change – I will recharge my iPod now and perform a bench again.
Btw 2, the WPS updating shouldn’t be a problem if the screen buffer update is not done. The main CPU load while WPS activity comes from update_rect(). If this function is returning immediately the CPU load should be minor.

Sorry, for GPO32_VAL I mean:
0×4000 set, 0×8000 clear: BCM running, most power used. This is the lcd_enable(true) state.
0×8000 set, 0×4000 clear: less power, BCM not usable, I guess it’s sleeping.
both cleared: uses the least power. This is the lcd_enable(false) state.
It may be necessary to set one before clearing the other to preserve some BCM state.

BTW Is there some way to fix errors in comments (without having to post another comment)?

Just saying. IMO you should not introduce special cases with the ipod. Virtually any other target uses the existing lcd_enable and lcd_sleep framework where possible.

If you think the LCD is very usable without backlight (and should be usable), then implementing lcd_enable isn’t appropriate at all. In this case, you should combine everything possible into lcd_sleep, which is going to kick in 10sec (the default setting) after backlight off anyway.

I, however, think, that considering the LCD of the 5G as usable without backlight may have some points, but not enough to to waste power and do special casing (I haven’t seen a 5G lcd without backlight though, yet). In this case, I’d just turn the LCD and backlight off with lcd_enable(false), and the controller with lcd_sleep. Also, I claim, that, even if you think the lcd is readable, it won’t be enough for everyday use and particularly not for the majority of users.

So, for the sake of consistency, minimize the power consumption, without introducing special cases.

PS: This is all my point of view, others may have different opinions.

I decided to define HAVE_LCD_ENABLE because that allows the LCD do be woken via lcd_enable(true) and it allows other code to see if the LCD is on via lcd_enabled(). Yes, I see how it’s a bit inappropriate because lcd_enable(false) is never called outside of lcd-video.c, but I think it’s better than the alternative. I think all targets which have lcd_sleep also have lcd_enable.

The LCD is transreflective. Here’s a closeup and explanation: http://t17.net/transflectiveTFT/ . The reflected light is too dim when in a dim setting, but when outside on a bright day I’m sometimes unable to tell if the backlight is on or off.

What is the point of the two stage lcd_enable(false), lcd_sleep() sequence? I guess that’s because waking up from a sleep state takes a longer time?
Why is lcd_enable called from platform-specific backlight code? If it is meant to be used when the backlight is being turned on and off, why isn’t it always called from firmware/backlight.c?

“What is the point of the two stage lcd_enable(false), lcd_sleep() sequence? I guess that’s because waking up from a sleep state takes a longer time?” Pretty much, yes. On my e200, the wake up from sleep (i.e. powering the controller) takes noticeably longer.

“Why is lcd_enable called from platform-specific backlight code? If it is meant to be used when the backlight is being turned on and off, why isn’t it always called from firmware/backlight.c?” It’s called in the target’s backlight driver, like backlight-e200-c200.c. The targets implement _backlight_off() (which is called by backlight.c), which then calls lcd_enable(false) and start the lcd_sleep countdown. That’s at least how the targets I’m a bit familiar do it.

this is my first post to RockBox, and i just want to say thank you to everyone developing the code. i have been using RB for a year+, and the improvements are very impressive. you’re all doing a fantastic job, and i appreciate it very much. i have no coding skills, so i cant help directly, but i am very willing to try new code and post results if anyone asks…

im using an ipod 5.5 30gb, with a 580mAh battery. my music (approx 1700 files) is a 50/50 mix between 220kbs mpc and 320kbs mp3 files.
using svn v19996, no wps, and this patch v0.2 in addition to  FS#9730 , the ipod is stable and runs without problems. i experienced a runtime of 17.54.33h (didnt think to keep the bench.txt, sorry) on random play using the entire playlist. the previous average was about 12h. a very impressive difference!

i’ll run some benchmarks using the standard method (repeat one album), and post them soon.

I AM IMPRESSED! My 5.5G 30GB ran for 19:30h! This was tested with my standard setup (mpc album repeat, -56dB, no WPS, no accessory supply, hold on). The software uses  FS#9890  (v0.2),  FS#9708  (v0.3),  FS#8668  (24MHz normal + 100MHz boosted). To have a comparison I have also attached an older bench with  FS#9708  +  FS#8668 . The .pdf is a graphical representation of both bench results.

With  FS#9890 : battery_bench_r19983_udma1_bcmoff.txt, total runtime: 19:30h (90-10: 17:15h) → The battery was not even fully loaded, the max reachable runtime is a bit more than measured here.
Without  FS#9890 : battery_bench_r19589_udma1.txt, total runtime: 14:44h (90-10: 12:42h)

In my opinion this patch definately must find its way to svn

   battery_bench_r19983_udma1_bc... (77.5 KiB)
   battery_bench_r19589_udma1.txt (58.6 KiB)
   battery_bench_r19983_udma1_bc... (13.6 KiB)

minor concern:
my ipod shut off without warning. im using SVN r20007 and this patch only. 5.5G 30GB, 580mAh battery, .mpc album, -30vol, no WPS, accessory power off, hold switch on.
below is a segment of battery_bench.txt, its very odd (can attach full file if requested):

Battery bench run for Apple iPod Video version r20007M-090215

Battery type: 600 mAh Buffer Entries: 1000

Time:,  Seconds:,  Level:,  Time Left:,  Voltage[mV]:, C:, S:, U:

00:01:19, 00079, 097%, 16:37, 4162, -, -, -
00:02:19, 00139, 097%, 16:37, 4157, -, -, -
00:03:19, 00199, 097%, 16:37, 4160, -, -, -
00:04:19, 00259, 097%, 16:37, 4162, -, -, -
00:05:19, 00319, 097%, 16:37, 4163, -, -, -
00:06:19, 00379, 097%, 16:37, 4163, -, -, -
00:07:19, 00439, 097%, 16:37, 4162, -, -, -
00:08:19, 00499, 097%, 16:37, 4162, -, -, -
00:09:19, 00559, 097%, 16:37, 4161, -, -, -
00:10:19, 00619, 097%, 16:37, 4161, -, -, -
00:11:20, 00680, 097%, 16:37, 4160, -, -, -
<snip>
…quite uneventful until… </snip>
06:24:43, 23083, 064%, 10:58, 3895, -, -, -
06:25:43, 23143, 064%, 10:58, 3894, -, -, -
06:26:43, 23203, 064%, 10:58, 3894, -, -, -
06:27:44, 23264, 064%, 10:58, 3894, -, -, -
06:28:44, 23324, 064%, 10:58, 3894, -, -, -
06:29:44, 23384, 064%, 10:58, 3894, -, -, -
06:30:44, 23444, 063%, 10:48, 3893, -, -, -
06:31:07, 23467, 048%, 08:44, 3822, -, -, -
06:31:34, 23494, 030%, 05:39, 3752, -, -, -
06:32:01, 23521, 013%, 02:44, 3684, -, -, -
06:32:46, 23566, 005%, 00:56, 3574, -, -, -
–Battery bench ended, reason: power off–

when i turned it back on, the battery was at 59%, and resumed fine three songs back from when it shut off.
i couldn’t reproduce it, so im not certain if it was the patch, the SVN, the song, or the ipod itself. could have just been a random incident, ipods are strange creatures…

It’s nice to see how this patch can improve battery life. Thanks for performing the battery benches.

That last battery bench certainly is odd. The iPod went from 63% to 5% in two minutes. Even constant disk activity shouldn’t do that. If I take the battery capacity and percentages literally, that makes it seem like a huge current was flowing, much more than the highest current the iPod can use, and almost like a short circuit. I hope this isn’t what happened. It ought to trip overcurrent protection, and it might cause damage. I think it’s more probable that it’s just a “knee” in the discharge curve, (for example see: http://images.google.ca/images?q=lithium%20polymer%20discharge%20curve ) but I don’t know why it would start at 3.8V like that.

Does anyone else have comments on what kugel said? I still feel like I’m doing things the right way. The special case is that the 5G iPod has a transreflective LCD, and I think Rockbox should allow the user to take advantage of that.

Any other ideas or comments on what still needs to be done with this patch?

We had some short discussion on irc yesterday. Of course the behaviour should be as follows:
1. The LCD and BCM should be shut down via timer (e.g. 5s after backlight has been switched off), if the user activates this timer. → power saving
2. The LCD and BCM should not be shut down via timer, if the user deactivates this feature. → Make use of transflective display e.g. during daylight.
But such implementation does not need to introduce lcd_enable()-functionality. It is just needed to switch off/on _both_ the BCM and the LCD within lcd_sleep(). Such implementation should also fit into the existing framework without any change in the target-independent code.

If the LCD can persist an image with the BCM shut down, would there be any gains regularly switching off the BCM (for example, for a WPS that updates once per second, the BCM could be initialised, instructed to send the update to the LCD, and then shut down again, leaving the LCD switched on, for possible gains while still using transflective display)?

Regarding removing HAVE_LCD_ENABLE:
* It removes lcd_enabled(), which stops the scrolling code from running when the LCD isn’t enabled.
* It removes the LCD enable hook, which is used by mpegplayer to redraw the YUV data when the LCD is enabled
Should I really ignore these things and remove HAVE_LCD_ENABLE anyways?

Andree, regarding what you’re saying about settings which the user can change: Do you mean that I should implement other LCD timeout settings and not use HAVE_LCD_SLEEP_SETTING functionality? HAVE_LCD_SLEEP_SETTING seems like such a good fit, except that I now understand that it is used differently on other platforms. (There lcd_enable(false) shuts down the LCD and lcd_sleep() shuts down the LCD controller after that, producing no visible change at the time but making the next lcd_enable(true) take more time.)

I hope I can use HAVE_LCD_SLEEP functionality. Creating separate timeout code seems redundant, and lcd_sleep() may require a thread if it is too slow to be called from lcd_tick().

I agree that not changing target-independent code is a very good idea. I’m planning to remove the firmware/backlight.c changes.

Yes, the LCD can persist with the BCM shut down, and that state uses less power. However, the first LCD update command after the BCM has been reinitialized will cause the LCD to be reinitialized, which causes the LCD to blink, apparently uncleanly shutting down and then turning back on. For now I am not planning to work on figuring out a way around this because I am busy with other things.

Hi Boris, you should keep HAVE_LCD_SLEEP_SETTING functionality as this is the best possible fit. But instead of implementing lcd_enable(), you just leave it – from my understanding this will leave the transflective LCD enabled with backlight off. As a second step you implement lcd_sleep() butninlcude both the LCD on/off and the BCM on/off into this function. This way the code behind HAVE_LCD_SLEEP_SETTING will shut off the BCM/LCD after timeout when backlight went off.
Regarding stopping the scrolling code and such. Imho only checking for lcd_enabled() is not sufficient for the transflective LCD which use HAVE_LCD_SLEEP_SETTING. rockbox should also check for lcd_sleeping() as this is the major state for such LCDs. Or you define HAVE_LCD_ENABLE and leave “void lcd_enable(bool)” unimplemented but only implement “bool lcd_enabled(void)”. Is this possible without sideeffects?

Hi Andree, it’s good to know that I can keep using HAVE_LCD_SLEEP_SETTING.

Note that lcd_sleep() is defined in firmware/export/lcd.h as “lcd_sleep(void);”, so I can’t really put both on and off code in there. Other platforms with lcd_sleep also have lcd_enable and the LCD is always turned on with just lcd_enable(true). Without lcd_enable(true) I guess I have to use a platform-specific function to wake the LCD. Where do I put the prototype for that; can I use backlight-target.h or should I create lcd-video.h?

Also note that there is no lcd_sleeping() function anywhere in Rockbox. (I don’t see why there should be separate lcd_enabled() and lcd_sleeping() functions because I think other code just needs to know whether or not the LCD is displaying stuff. I don’t see why other code should care if the LCD is in a light or deep sleep.) Yes, defining HAVE_LCD_ENABLE but not implementing lcd_enable should work because lcd_enable is only called from backlight-*.c for specific targets.

Hmm, sounds like the whole lcd_sleep/lcd_enable stuff was designed for usecases of non-transflective LCDs only… Now I understand the issues you are concerned about. Maybe it is ok to just use HAVE_TRANSFLECTIVE_LCD (already defined in the config-file) to change the behaviour of lcd_enable/_sleep in the platform code. That is not the most elegant way of course…

Another idea:
A) Backlight switches off: lcd_enable(false) is called, the code does nothing in this case to keep the transflective LCD active. After a timeout lcd_sleep(void) is called and both LCD/BCM are switched off.
B) LCD/BCM to be switched on again: lcd_enable(true) is called and enabled both LCD/BCM again.
C) lcd_enabled(void) is true, if LCD/BCM is active and false if LCD/BCM is switched off (after lcd_sleep() was called)
This is a minor mixture between lcd_sleep, lcd_enable and lcd_enabled. Is this possible?

“Yes, defining HAVE_LCD_ENABLE but not implementing lcd_enable should work because lcd_enable is only called from backlight-*.c for specific targets.” That could work, but lcd_enable would be missleading (imo), instead you can do implement something like lcd_awake, and call this in _backlight_on

There’s no lcd_sleep(true) or lcd_awake(), since no target needed it. Basically, the _backlight_on() implementation is supposed to take care of that. That is, because you may not sleep until some timeout, but awake always instantly.

What Buschel proposes sounds very good, but I think we shouldn’t create confusion to source reader with functions, that do special things on the ipod, but are “generic” on the other targets.
Re: C) There’s no lcd_enabled without defining HAVE_LCD_ENABLE I think (but I may be wrong in this case).

edit (missed some comments):
>* It removes lcd_enabled(), which stops the scrolling code from running when the LCD isn’t enabled.
>*It removes the LCD enable hook, which is used by mpegplayer to redraw the YUV data when the LCD is enabled

In this cases, I think the current code should be fixed to work with LCD_SLEEP only too. But not in this patch. If you can’t get it to work using the existing framework, the patch should be committed anyway, and this framework fixed up later.

kugel, the A/B/C usecases of course need HAVE_LCD_ENABLE, HAVE_LCD_SLEEP and HAVE_LCD_SLEEP_SETTING. The target specific functions just behave different for iPod (or transflective LCDs). When implementing the functionality this way there is no need to change target independent code (if I understood correctly). Of course there is the need to document the behaviour…

If the target independent code doesn’t handle transflective LCDs, then it needs to be fixed, imo. Using LCD_ENABLE because some parts of Rockbox don’t check for LCD_SLEEP doesn’t sound right to me.

i have tried another benchmark, using v3.1 from source and this patch only. (5.5G 30GB, 580mAh battery, mp3 album 250VBR, -30vol, no WPS, accessory power off, hold switch on.)
total time of 17:23h. no sudden power failures this time. i would attribute my previous problem to either the SVN or the ipod itself.

i say the patch is good. it passes my strenuous testing strategy of headbanging and/or rocking out ;)

i have tried another benchmark, using v3.1 from source and this patch only. (5.5G 30GB, 580mAh battery, mp3 album 250VBR, -30vol, no WPS, accessory power off, hold switch on.)
total time of 17:23h. no sudden power failures this time. i would attribute my previous problem to either the SVN or the ipod itself.

i say the patch is good. it passes my strenuous testing strategy of headbanging and/or rocking out ;)

I was just timing components of what is now lcd_enable(true). The whole thing takes .5 s, with BCM init taking .2 s and the first LCD update command taking .3 s (probably because the BCM initializes the LCD).
The first three loops with yield() in bcm_init() take 4-5, 2-3 and 2 microseconds. Should I remove those yields? Is there some general guideline for when to yield() and when to just wait?

Should I move all the GPIO stuff which doesn’t change into lcd_init_device()? They can be probably be removed because those things are already set, but IMHO having those things in Rockbox is a good thing.

I’ll remove HAVE_LCD_ENABLE and implement lcd_awake(). I think Rockbox only needs one “is LCD displaying stuff?” function and one “power up whatever is necessary so the LCD starts displaying” function, but I understand the concerns regarding lcd_enable() and I understand that can be dealt with later, after this patch is committed.

Are there any other things I need to know before I release a new version of this patch?

“Should I move all the GPIO stuff which doesn’t change into lcd_init_device()? They can be probably be removed because those things are already set, but IMHO having those things in Rockbox is a good thing.” - Most definitely ;)

I cannot tell much about the yield thing, sorry.

Great job so far anyway!

Here’s the new version. The main changes are:
* HAVE_LCD_ENABLE and related functions removed. Code from lcd_enable(bool) is now split into lcd_sleep() and lcd_wake().
* Patch now makes no changes to backlight.c or other platform-independent code. LCD is now woken via lcd_wake() from backlight-nano_video.c.
* GPO_32 and GPIO inits moved to lcd_init_device().

Another question: The LCD update code has a timeout and retries the command if it takes too long. None of the new stuff in this patch has timeout checking, and I haven’t seen any lockups. Should I add timeout checking and recovery code?

   bcm_lcd_sleep-v0.3.patch (15.3 KiB)

Thanks for the updated patch. I’ve just installed it and works perfectly.
I will try to change the automatic shutdown setting to “Never” and see how long it takes to drown the battery with the LCD off.

Boris, this is one major step for the battery runtime. Thank you very much for your effort!
Submitted with r20076.

I’ll keep this patch open for any further comments or optimizations.

Is it possible to modify this patch to work on ipod Photo/Color, too ??

Thanks Andree!

Jakub, other iPods have different hardware so very little of this patch can be re-used. On 5G and 5.5G iPods the LCD is connected to the BCM2722 DSP which is used for video acceleration, and Rockbox interacts with that chip. Other iPods don’t have that chip, and Rockbox interacts directly with the LCD controller. It should be easy to implement LCD sleep on Color/Photo iPods with the HD66789R LCD controller. That chip is documented and used on other Rockbox targets. I don’t know about the other unknown LCD controller however. BTW Apparently all 1G Nanos use the HD66789R.

Hi Boris, I’d like to start off by saying that your work is amazing, and hopefully you’ll continue making great progress on ipod video hardware.

This patch is great, and I’m glad to see that it is going to save som serious battery life. I’ve found one or two quirks (which may have been mentioned or discovered already… 1) The function does not play nice with “first buttonpress enables backlight only”. Not sure if I need to expand on that, you’ll see if you enable it.
2) Setting the sleep function to “always” along with a fadeout causes the screen to go blink white on inaction, and then fade to off. My backlight fadeout is set to 2000ms.
3) When shutting down my player, the ipod goes through the shutdown screen and then blinks white before shutting off (same as #2). I set the sleep function to 5s, but this still happens for me if I set it back to the default 10s.

Besides these things I haven’t had any real problems. Hoping your other projects will also be added to SVN very soon!

1) Did “first buttonpress enables backlight only” work before? I renamed .rockbox, extracted the USB test build (r20033), enabled that option, started playing music, and waited for the backlight to finish fading. When I pressed the next track button, I went to the next track.
2) Yeah, I know that the LCD sleep timer starts at the start of the fade, and I also think that it should start after the fade is complete.. To change that I would have to modify target-independent code in firmware/backlight.c, and it was decided that this patch shouldn’t modify target-independent code. For now there’s the simple workaround of setting the LCD sleep time to 5 s instead of always. I’ll submit another patch for this soon.
3) The flash at shutdown was there before. This patch just shuts down the LCD sooner so the screen is white for a longer time when shutting down. Rockbox does not turn off the backlight when shutting down, and the backlight only goes off when power is cut to the whole system. I guess it exists to show that the LCD was properly shut down and that power was actually turned off. I also think this should be changed on stable targets.

I found a problem: if a version of Rockbox without this patch is loaded via ROLO, the LCD display is corrupted. I’m pretty sure this is because in the old version lcd_setup_rect does not write all the necessary values to 0xE0000-0xE001F in the BCM and so the LCD update command fails. As noted earlier, in this patch I changed to a simpler LCD update command which puts the LCD data at 0xE0000 in the BCM, without any parameters. Should I go back to the old function? (Just writing the necessary values before ROLO executes the code it loaded would be ugly.)

Hm, you’re probably right on 1… I only ended up enabling it after updating to use this functionality. My hope was that I could use this to adjust volume without having to turn the display back on (honestly, not sure if that was how it worked in the first place). Hopefully someone will look into that…

As for 3, yes, the flash was previously present but much more apparent now. Not a big deal, just wanted to make sure it wasn’t anything bad. Keep up the great work, sir.

Some posts I made about these issues:
1) (”first buttonpress enables backlight only”): FS#9942 , http://forums.rockbox.org/index.php?topic=20707.0 (Seeking comments on what would be the correct behaviour.)
3) (flash at shutdown): http://forums.rockbox.org/index.php?topic=20708.0

Both (1) + (3) are not an effect of this patch. I have added some comments to FS#9942 and the posted forum links.

Here is a fix for issue (2). The behaviour is due to target-independent PWM fading code in firmware/backlight.c. I removed backlight_lcd_sleep_countdown(true) from the end of _backlight_off() and put it in several places where the backlight is finally turned off. I also made lcd_sleep_timer SHAREDDATA_ATTR because it is shared by the COP (backlight_tick()) and CPU (other code).

I also noticed a very brief white flash when turning on the LCD without backlight fade-in. It seemed like only part of the screen was white, and I remembered the 47 Hz vsync signal I saw at http://www.electricstuff.co.uk/ipodlcd.html , so I put sleep(HZ/50) at the end of lcd_awake().

   lcd_sleep_after_pwm_fadeout.p... (1.5 KiB)
   wait_for_lcd_refresh_in_lcd_a... (0.5 KiB)

Twice I’ve seen an old image persist after awaking. The next action which triggers an LCD update caused the image to update.

I think I understand why. LCD update calls are ignored while lcd_awake() is waiting for its own LCD update to complete. Such LCD update calls need to trigger another update. I’ll upload a patch soon.

While testing a solution to the above I saw other problems:
* If backlight timeout is 1 s, “Sleep (After Backlight Off)” is set to “Always” and I turn on the LCD by quickly scrolling a list with long filenames, backlight turn-on is delayed, and it’s possible to end up in a loop with the LCD going on and off repeatedly until scrolling stops. After that the LCD may be turned on normally; I haven’t seen lockups. I seems that scrolling-related CPU activity is starving the backlight thread, and the 1 s timeout expires before there is a chance to reset it. Such scrolling can also starve playback and cause pauses, even when LCD sleep isn’t involved.
* When starting up, code assumes that the LCD is on. This can be a problem when loading Rockbox via ROLO with a very short backlight timeout.

Unrelated: The OF can turn off PCF5060X_D3REGC1, which pcf50605.c says is the LCD voltage supply. When I shut it down in lcd_sleep(), FireWire input current decreased by 0.1 mA. However, buttons didn’t work and caption backlight didn’t turn on the LCD (even though the regulator was enabled in lcd_awake()). IMHO, because of these problems and the very small current savings, further investigation of this is a very low priority.

Here’s a fix finally. I spent too much time thinking about various alternatives. Comments are welcome.

* To prevent an old image from persisting, the first update is done via lcd_update(). Then, lcd_awake() waits for it to finish via wakeup_wait. As a side-effect, this gets rid of the slight white flash when there is no backlight fade-in, so wait_for_lcd_refresh_in_lcd_awake.patch is unnecessary. It also causes the first update to have a timeout. I like this, but I kept wondering if I should I do a simpler, smaller solution.
* I removed several yield() calls. If the CPU is busy, they can take a long time. The longest such polling loop takes 40 microseconds. I think it’s better to poll for 40 µs than to yield and maybe end up waiting 0.1 s. This makes it harder to get into the LCD-on LCD-off loop described above. To totally fix this and the playback interruptions caused by fast list scrolling, some changes need to be made elsewhere.
* In lcd_init_device(), I check GPO32_VAL & 0×4000 (BCM power). If it is on, I assume the BCM is initialized. If it is off, I call lcd_awake(). It might be simpler to just make rolo.c ensure the backlight is on when executing loaded code, but I don’t know if I should make changes there.
* The time the BCM needs to stay off and the wait between power on and start of bootstrap have both been decreased to 50 ms. This should speed up LCD wakeup.

While testing I examined another problem: When switching on the backlight via a backlight timeout menu, brightness is sometimes at the minimum, without any change to the Rockbox setting. The next time the backlight turns on in the normal way, brightness is correct. This can happen without LCD sleep, but it seems to happen more with LCD sleep. I think this is due to the multiple concurrent backlight on issue which I mentioned earlier. In _backlight_hw_enable there is a sleep which can cause problems. Changing backlight_update_state() to backlight_on() in the set_timeout functions (like in an earlier version of this patch) seems to fix the problem. It would also allow the lcdstate_lock mutex to be removed. So, what do I do with the backlight.c issues? Should I start another separate patch?

I tested to make sure lcd_update() calls during LCD initialization don’t cause problems. The LCD was initialized properly even when I made the timeout too short so more LCD update commands were executed during initialization.

   bcm_lcd_wakeup_improved.patch (7.4 KiB)

You may also think about implementing lcd_enable_hook. This is currently only for HAVE_LCD_ENABLE, but does absolutely fit to LCD_SLEEP only targets too.

See also discussion and latest patch here: http://www.rockbox.org/tracker/task/8523

The original version of this patch defined HAVE_LCD_ENABLE and implemented the LCD enable hook and lcd_enabled(). However, more experienced Rockbox developers said that HAVE_LCD_ENABLE was not appropriate (see discussion above). Right now I don’t know what to do about this.

I could make the LCD enable hook conditional on HAVE_LCD_ENABLE or HAVE_LCD_SLEEP, but that misleads because the name implies it is an LCD enable thing. Creating separate identical code for an LCD awake hook doesn’t seem like a good idea either.
I see that  FS#8523  implements lcd_sleeping(). I don’t think there’s a need for separate lcd_enabled() and lcd_sleeping() functions, because I think platform-independent code only cares if the LCD is currently displaying an image.
It might be best to rename the LCD enable hook and lcd_enabled() so they’re not specific to “enable” or “sleep”.

Before I can do anything about this, I need to know what is acceptable.

I’d accept it. lcd_sleeping is more of a work-around anyway. It’s essentially the same as lcd_enable.

In theory, I wouldn’t mind naming it lcd_enable too for ipods, but on the other hand it will probably get confusing with the HAVE_LCD_ENABLE define (confusing for new developers). And even if the ipod had lcd_enabled(), it doesn’t have lcd_enable().

It’s a bit tricky, I agree. The 5G is basically messing up a system which worked well for years ;) What I did at  FS#8523  was going the way with least impact on unrelated code.

I guess the best would be to get rid of lcd_enabled(), and implement something that can be used independently. Other code doesn’t care which state the display is in. However, cannot really change lcd_enable() since that works differently from lcd_sleep() (i.e. what you proposed ;) )

Oh yeah, you’re the person who first complained about my abuse of lcd_enable(). I wish I remembered names more.

In  FS#8523  disable-wps-update-v5.patch, I see that you’ve already made the LCD enable hook code compile if HAVE_LCD_SLEEP is defined. You’ve also added lcd_call_enable_hook() to other targets. If you want to proceed this way, you just need to add lcd_call_enable_hook() near the end of lcd_awake(). A comment makes the location obvious. Note that mpegplayer also uses the LCD enable hook. I’ll resync my last patch here if your patch is committed first.

Alternatively, how about using “active” as a common term? For example: HAVE_LCD_DEACTIVATION, lcd_active(), lcd_call_activate_hook()

I think I added it at the proper location in  FS#8523 .

Anyway, I think I’ll make this little rework independently of the  FS#8523 . But it’s not going in until the freeze is over. I’ll try to attach a patch here or over there tomorrow.

Oh, and I think we just can do HAVE_LCD_ENABLE || HAVE_LCD_SLEEP. That makes it a bit more obvious that both lead to unreadable displays, but nevertheless slightly differ.

I think lcd_call_enable_hook() belongs inside the if. It is that way on other targets. If lcd_awake() doesn’t awaken the LCD, there’s no need to call the hook.

Ok, it was painful, but I committed the rework. I hope it’s fine and you can use it.

Kugel, a belated thanks for the rework. It works great!

Here’s a resynched bcm_lcd_wakeup_improved.patch. I’m planning to commit it soon.

   bcm_lcd_wakeup_improved-r2068... (7.3 KiB)

I forgot some #ifdefs in bcm_lcd_wakeup_improved, which led to a bootloader compile error when I committed it in r20695. I’m sorry about that. I fixed the problem in r20696.

I will close this task soon. The remaining platform-independent bits here should go into their own separate task.