Rockbox

Tasklist

FS#8523 - Disable WPS updating when the backlight is off.

Attached to Project: Rockbox
Opened by Magnus Holmgren (learman) - Sunday, 27 January 2008, 22:01 GMT
Last edited by Thomas Martitz (kugel.) - Tuesday, 14 April 2009, 22:14 GMT
Task Type Patches
Category Themes
Status Closed
Assigned To Magnus Holmgren (learman)
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This is a proof-of-concept patch to stop the WPS from updating the screen if the backlight is off. For now, it is only enabled for the Sansa e200, as I have one and the e200 screen cannot usually be read with the backlight off.

The reason for doing it is that it can increase the runtime a little, depending on the WPS used, though we are probably not talking about very big differences.

Should this be applied at all? If so, how? I.e., should it only be enabled for targets where it makes sense, or should an option be added for all targets?

The patch also includes some minor code policing changes.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Tuesday, 14 April 2009, 22:14 GMT
Reason for closing:  Fixed
Additional comments about closing:  Caption backlight broken by this is fixed as of r20708.
Comment by Peter D'Hoye (petur) - Monday, 28 January 2008, 13:18 GMT
1) I have seen this discussion before and it was agreed that runtime would not be influenced a lot unless the codec was doing (a lot of) boosting
2) several targets have a display that is perfectly readable without backlight
3) why mix functional code with code policing in one patch?
Comment by Thomas Martitz (kugel.) - Monday, 28 January 2008, 16:30 GMT
Still, would be be nice if someone could test it in case of e200 :) Maybe I do when I got some time.
Comment by Magnus Holmgren (learman) - Monday, 28 January 2008, 20:35 GMT
1) Maybe that discussion was made when WPS:s were a bit simpler? With album art, lots of bitmaps and conditionals on sometimes big screens, things could be a bit different.

Also, battery benchmarks collected as a part of the PortalPlayer power optimization tests suggests there is a (small but noticeable) difference on some targets. As for boosting, when playing Vorbis files (q 4.5), with some sound processing, I get 40-50 % boost ratios on my e200.

2) I'm well aware of that, which is why the patch only does it for the e200 for now.

3) Yeah, I probably should remove it. But it stared me in the face when I was editing that function... :)
Comment by Peter D'Hoye (petur) - Sunday, 03 February 2008, 13:25 GMT
Unless I overlooked, I don't see a display refresh forced when backlight is switched on. Or is the normal refresh done fast enough to be ready by the time the backlight is on again?
Comment by Thomas Martitz (kugel.) - Sunday, 03 February 2008, 15:40 GMT
In  FS#8379  Frank Gevaerts said that showing the wps or the main menu makes a 15% difference in runtime. This makes me think, that updating the screen permanently or not makes indeed a difference.
Comment by Jacob Brooks (jac0b) - Monday, 04 February 2008, 14:37 GMT
Has this been tested on the gigabeat?
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 21:44 GMT
fyi:
doesnt patch together with  FS#8385  (viewport-wps).
needs manually modification for viewports, but works then.

now regarding the general idea:
i understood that this patch stops updating the wps, so graphics are not drawn any more.
so far so good, but is the lcd also completely turned off? or is this handled by any other power saving patch?
Comment by Thomas Martitz (kugel.) - Monday, 04 February 2008, 21:47 GMT
The LCD is turned of either way. This patch just changes the rendering of the wps to NOT happen when the LCD is off, which saves CPU time and thus some energy.
Comment by Magnus Holmgren (learman) - Wednesday, 06 February 2008, 14:19 GMT
Normal refresh is 5 times per second. This was fast enough when I tested it on an e200 (I couldn't see any "stale" graphics or text).
Comment by Magnus Holmgren (learman) - Thursday, 07 February 2008, 20:09 GMT
Better version of the patch. Don't stop scrolling when not updating the WPS. Removed the code policing. Still only enabled for the e200 (easy to fix if you know basic C :).
Comment by Jacob Brooks (jac0b) - Thursday, 14 February 2008, 23:44 GMT
I think this will work for the Gigabeat only. I did 2 tests and it looks like it didn't update the screen until I changed the volume or did something to make the backlight come on.
Comment by Jacob Brooks (jac0b) - Friday, 15 February 2008, 00:18 GMT
I think this will work for the Gigabeat only. I did 2 tests and it looks like it didn't update the screen until I changed the volume or did something to make the backlight come on.
Comment by Jacob Brooks (jac0b) - Monday, 07 April 2008, 23:03 GMT
Resync
Comment by Thomas Martitz (kugel.) - Sunday, 18 May 2008, 01:37 GMT
This patch should work for both, Gigabeat F and Sansa e200.

I simply added

/* Define this if the screen is unreadable without backlight */
#define NEED_BACKLIGHT

into their config files.

I also put it into every other config-target, just commented out of course. I hope I didn't forget a target (I tripple checked it).

Feel free to add your player if you know that the screen is unreadable without backlight.

PS: I rather like that patch. It doesn't change much actually, but it potentially increases battery runtime.
Comment by Bob (viperman3) - Wednesday, 21 May 2008, 17:53 GMT
The patch doesn't compile for iriver H320 and X5 - errors in gwps-common.c. I tried it for the e200 and it does compile.
Comment by Thomas Martitz (kugel.) - Sunday, 25 May 2008, 19:56 GMT
Hmm, I don't have the compiler for H3xx/X5 installed, so I can't really test-build. The sims for both compiled fine though.

It would be helpful if you could post the make error to www.pastebin.ca.
Comment by Bob (viperman3) - Tuesday, 03 June 2008, 21:02 GMT
Just tried this patch on build r17647 with option to compile for sim. There same problem persists for the X5. This is the last snippit of the error when the build fails:

gui/gwps-common.c: In function `gui_wps_refresh':
gui/gwps-common.c:2030: error: too few arguments to function `is_remote_backlight_on'
make[1]: *** [/home/bpanesar/rockbox-r17647/rockbox-r17647/build/apps/gui/gwps-common.o] Error 1
make: *** [build] Error 2


patch 8523 is the only one I applied to the svn build mentioned so as to identify if there was another patch that was causing a problem.

Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 13:23 GMT
"If you have alternating text in your WPS, once you reactivate the LCD it cycles really fast through all the updates that were not "printed on screen" while the backlight was off."

Can anyone look into that issue?
Comment by Magnus Holmgren (learman) - Tuesday, 24 June 2008, 18:59 GMT
Don't see how that could happen. There's simply no way for the updates to be queued up. At most the old LCD contents could be visible briefly before the up-to-date information is rendered.
Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 19:06 GMT
Well, no.

The longer the backlight was off, the longer the alternating lines cycle through (at a fast rate). So I expect they are indeed "queued".
Comment by Magnus Holmgren (learman) - Wednesday, 25 June 2008, 17:03 GMT
Ah, so it is subline specific (I don't use sublines myself). When updating is disabled, the subline_expire_time isn't incremented, hence the fast cycling when it is incremented again. Maybe the reset_subline check at line 1741 in gwps-common.c can be removed (and always based it on current_tick)?
Comment by Michael Sevakis (MikeS) - Wednesday, 31 December 2008, 22:39 GMT
Updates can be received (when lcd is reenabled) through lcd_set_enable_hook. The hook could broadcast a message. One hook is available at a time. The LCD drivers should simply add lcd_call_enable_hook in their lcd_enable(true) case.
Comment by MichaelGiacomelli (saratoga) - Thursday, 01 January 2009, 00:17 GMT
Quick and dirty attempt at a fix for the broken alternating lines problem. I'd appreciate any feedback, I don't typically work with the core so I could be doing things in an unacceptable way.
Comment by Delyan Kratunov (archivator) - Tuesday, 06 January 2009, 15:32 GMT
I compiled the patch on a iPod 5.5G (had to remove the LCD_ENABLE ifdefs) and my first informal impression was that the battery lasted longer. I am yet to run proper benchmarks but I feel that the results would be satisfyingly impressive.

I noticed, though, that scrolling text (the one that does not fit the space allocated) still updates regularly. I have no idea where to begin. If you give me a hint, I can try and complete that part.
Comment by Thomas Martitz (kugel.) - Tuesday, 06 January 2009, 17:12 GMT
Does the iPod not have LCD_ENABLE?
Also, you noticed that it scrolls, does this mean you can read the display without backlight?

This defined is used since it indicates that the backlight is unreadable without backlight *indirectly*. Sad if the ipod doesn't have the possiblity to turn the lcd off, would safe a little battery life too (even without this patch).


Re: scrolling text: The scrolling engine is separate. So, scrolling must be explicitly disabled. Seems it's not done yet.
Comment by Delyan Kratunov (archivator) - Tuesday, 06 January 2009, 18:16 GMT
If the firmware/export/config* headers are the only place for such defines, then no, the iPod 5.5g does not support turning off the LCD. Now that I think about it.. I believe the OF does exactly that in deep sleep.. I guess no one has figured out how the turn off the screen yet, I'm almost certain the capability is there..

As for the screen itself, I can read it with a desk lamp straight above it. It's hard on my eyes but quite readable.
Comment by Thomas Martitz (kugel.) - Tuesday, 06 January 2009, 18:28 GMT
So, it should be implemented. I'd be really surprised if there wasn't such capabilities. For the time being, HAVE_LCD_ENABLE could be defined, along with a stub, not sure if that's good though.
Comment by Delyan Kratunov (archivator) - Wednesday, 14 January 2009, 00:40 GMT
Just wanted to note that I have tried an alternate approach and it worked as well as this one. I disabled screen updating when the light's off right in the driver code and gained a 57 min. battery life gain. Of course, this wouldn't be as much in non-wps screens but I find the gain sufficient.

Some developers expressed concerns that showing an outdated screen is worse than wasting some battery. Do you believe it is a good idea to blank the screen before stopping updates (thus, simulating turning it off)? Or, perhaps, we could show a "screen saver" with info updated every once in a while (say, every minute a giant image of the battery status gets drawn on a black backgrond; the image could be chosen to be in such colors that it's easily viewed without backlight).

Also, what would be the easiest way to figure out BCM's turn off command?
Comment by Thomas Martitz (kugel.) - Wednesday, 14 January 2009, 00:43 GMT
Which target were you talking about?

This patch is surely mainly targeted to those player, where the screen isn't readable at all without backlight. Those (with the exception iPod Video apparently) already disable display updating without backlight.
This patch aims to disable wps updating, by the means that the code for keeping the wps upto date, doesn't run anymore (or returns very early respectively).
Comment by Delyan Kratunov (archivator) - Wednesday, 14 January 2009, 00:59 GMT
I should have clarified. It's 3 o'clock in the morning here, excuse my absent-mindedness.

I only have an iPod Video and that's why my patch gives almost equivalent performance to this one - the actual lcd updates draw significant amounts of energy. Naturally, both patches combined should improve things significantly but I am yet to do benchmarks. I am considering alternatives to the frozen out-of-date screen effect but that's out of the scope of this patch.

I must stop spamming with irrelevant things in this patch's comments, though. Tomorrow I'll post a separate FS entry.
Comment by Thomas Martitz (kugel.) - Wednesday, 14 January 2009, 01:26 GMT
That's interesting. How did you manage to disable lcd updates? Does that mean you actually lcd_enable()? Let's see your patch.
Comment by Delyan Kratunov (archivator) - Wednesday, 14 January 2009, 09:47 GMT
I did it in the simplest of ways - by including backlight.h in the driver and simply doing an if(is_backlight_on) return; ..

I only wanted to check battery life, so the implementation wasn't important. I'll do a proper implementation (LCD_ENABLE and all) and submit that some time this week.
Comment by Johannes Linke (Jaykay) - Tuesday, 03 February 2009, 10:34 GMT
i forgot to write this a few weeks ago... this patch increases battery runtime about 1:15... if anyone want to see the benches, i wrote them some weeks ago in the -dev mailing list, or ask me.
Comment by Thomas Martitz (kugel.) - Thursday, 26 February 2009, 23:17 GMT
I think I'm going to sync this to the latest HAVE_TRANSFLECTIVE and ipod changes and commit soon. Over 1h more runtime shouldn't be allowed to rot.
Comment by Johannes Linke (Jaykay) - Friday, 27 February 2009, 06:33 GMT
> Re: scrolling text: The scrolling engine is separate. So, scrolling must be explicitly disabled. Seems it's not done yet.

@kugel: how about that? is this going to be disabled too?
Comment by Thomas Martitz (kugel.) - Friday, 27 February 2009, 14:35 GMT
I'm not sure, but I think scrolling engine is already disabled without backlight (or rather, when the lcd is disabled)?
Comment by Thomas Martitz (kugel.) - Friday, 27 February 2009, 18:10 GMT
Here's a patch which also implements this for the ipod video.

I was right, the scrolling engine is already (partly) disabled, if lcd_enabled() is false. I adapted it for the ipod too now.
edit: updated a slightly more correct patch.
Comment by Dave Woyciesjes (woyciesjes) - Friday, 27 February 2009, 20:35 GMT
Tested on iPod Video 5.5G. at about 1:50pm -
Not working right for me. Chose song, let it play to next, screen blanked.. Looked at WPS, had updated. Then, skipped to next track, while screen was active, did not update. Skipped two more, still no update. Went to main menu, then tried to go to Now Playing. Did not switch to WPS, but the controls acted as if I were on the WPS. Let the screen black out, during the song, hit a button to bring it back, and there it was, the WPS screen.
Basically, seems like you achieved the opposite of what you wanted. WPS updates while the screen is off, and not if it's on.

One more hitch. Came back from the meeting (at 3:10), and the iPod wouldn't wake up from when Rockbox put the display to sleep for the hour. I had to give it the 2finger to bring it back. Not sure if it's related to the Apple firmware bug (mentioned this morning on the dev mailing list)
Comment by Thomas Martitz (kugel.) - Saturday, 28 February 2009, 15:10 GMT
Should be fixed with this one.
Comment by Thomas Martitz (kugel.) - Saturday, 28 February 2009, 19:24 GMT
Fix a bug with track changing.
Comment by Thomas Martitz (kugel.) - Tuesday, 03 March 2009, 00:51 GMT
OK, this uses the lcd_enable hook to instantly update the wps when the backlight is turned on again.

With this, I'd like to get some opinions on wether this is committable or not.
Comment by Boris Gjenero (dreamlayers) - Friday, 13 March 2009, 06:54 GMT
lcd_set_enable_hook and lcd_call_enable_hook are in firmware/drivers/lcd-16bit.c.
The Sansa Clip has a 1 bit LCD, so linking fails because those two are undefined.
Other targets with HAVE_LCD_ENABLE or HAVE_LCD_SLEEP have LCD_DEPTH == 16.
Comment by Thomas Martitz (kugel.) - Friday, 13 March 2009, 12:12 GMT
Right. But the Clip should have it too I think. I'll correct the 5G one.
Comment by Thomas Martitz (kugel.) - Friday, 13 March 2009, 13:16 GMT
So, I'm uploading a seperate patch to activate lcd_enabled for HAVE_LCD_SLEEP. It also unifies it, so that target independent code can do lcd_active for both.
This needs to be done in order for this patch to be not that hacky, and will be committed anyway.
Comment by Thomas Martitz (kugel.) - Tuesday, 17 March 2009, 05:09 GMT
I committed the above patch (yes, it was a nightmare, but needed to be done), and some wps rework. Both will make this patch much cleaner.
Comment by Thomas Martitz (kugel.) - Tuesday, 17 March 2009, 14:51 GMT
Sync. A good deal less changes are needed now.
Comment by Thomas Martitz (kugel.) - Monday, 23 March 2009, 00:16 GMT
Hopefully last update.

Still could need testing on remotes.

Is there remotes that are readable without backlight?
Comment by Peter D'Hoye (petur) - Monday, 23 March 2009, 06:28 GMT
yes, iriver remotes... I'll try to test tonight
Comment by Thomas Martitz (kugel.) - Monday, 23 March 2009, 15:28 GMT
Ok, this one only disables updates for the main unit, not for remotes. I've got told that all supported remotes are readable without backlight.
Comment by Thomas Martitz (kugel.) - Wednesday, 08 April 2009, 18:23 GMT
Probably the last update before committing. Needs a test on a remote target.
Comment by Boris Gjenero (dreamlayers) - Tuesday, 14 April 2009, 16:26 GMT
On my 5G 30GB iPod, this patch breaks caption backlight. Sadur found the problem and reported it at: http://www.rockbox.org/tracker/task/10130#comment29735

(I thought it made more sense to re-open this than to submit a new task because this way I can immediately reach those who worked on this patch. If I did the wrong thing, sorry.)
Comment by Thomas Martitz (kugel.) - Tuesday, 14 April 2009, 16:40 GMT
I'll fix that when I come home.
Comment by SadurnĂ­ (sadur) - Tuesday, 14 April 2009, 18:04 GMT
Thank you Boris and Thomas.

It makes sense to me because I noticed that after playing some songs with the LCD off, when I touched the click-wheel and the LCD and backlight comes back, the wps started to switch the lines with scroll very fast until I turned the Hold switch on and off. This happened to me twice in the last tests.
It seems the same as http://www.rockbox.org/tracker/task/8523#comment24473 that I thought it was fixed.

Thanks again.

Sadur.
Comment by Thomas Martitz (kugel.) - Tuesday, 14 April 2009, 19:29 GMT
The fast switching sublines thing should really be fixed. I cannot reproduce it on my sansa.
Comment by Boris Gjenero (dreamlayers) - Tuesday, 14 April 2009, 19:48 GMT
Sadur, which device and which WPS are you using?

Here's a patch for caption backlight. It simply moves the caption backlight code, so it still runs when the LCD isn't active. (I started on this before I noticed kugel's reply above. It works well, so I might as well post it.)

Loading...