This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#8523 - Disable WPS updating when the backlight is off.
Attached to Project:
Rockbox
Opened by Magnus Holmgren (learman) - Sunday, 27 January 2008, 23:01 GMT+2
Last edited by Thomas Martitz (kugel.) - Wednesday, 15 April 2009, 00:14 GMT+2
Opened by Magnus Holmgren (learman) - Sunday, 27 January 2008, 23:01 GMT+2
Last edited by Thomas Martitz (kugel.) - Wednesday, 15 April 2009, 00:14 GMT+2
|
DetailsThis 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. 
wps.patch
(5.2 KiB)
../apps/gui/gwps-common.c | 124 +++++++++++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 50 deletions(-) |
This task depends upon
Closed by Thomas Martitz (kugel.)
Wednesday, 15 April 2009, 00:14 GMT+2
Reason for closing: Fixed
Additional comments about closing: Caption backlight broken by this is fixed as of r20708.
Wednesday, 15 April 2009, 00:14 GMT+2
Reason for closing: Fixed
Additional comments about closing: Caption backlight broken by this is fixed as of r20708.
2) several targets have a display that is perfectly readable without backlight
3) why mix functional code with code policing in one patch?
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... :)
FS#8379Frank 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.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?
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.
It would be helpful if you could post the make error to www.pastebin.ca.
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.
Can anyone look into that issue?
The longer the backlight was off, the longer the alternating lines cycle through (at a fast rate). So I expect they are indeed "queued".
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.
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.
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.
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?
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).
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.
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.
@kugel: how about that? is this going to be disabled too?
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.
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)
With this, I'd like to get some opinions on wether this is committable or not.
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.
This needs to be done in order for this patch to be not that hacky, and will be committed anyway.
Still could need testing on remotes.
Is there remotes that are readable without backlight?
(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.)
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.
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.)