Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface → Themes
  • Assigned To
    learman
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by learman - 2008-01-27
Last edited by kugel. - 2009-04-14

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

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.

Closed by  kugel.
2009-04-14 22:14
Reason for closing:  Fixed
Additional comments about closing:  

Caption backlight broken by this is fixed as of r20708.

petur commented on 2008-01-28 13:18

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?

Still, would be be nice if someone could test it in case of e200 :) Maybe I do when I got some time.

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

petur commented on 2008-02-03 13:25

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?

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.

jac0b commented on 2008-02-04 14:37

Has this been tested on the gigabeat?

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?

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.

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

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

jac0b commented on 2008-02-14 23:44

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.

jac0b commented on 2008-02-15 00:18

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.

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.

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.

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.

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.

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

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.

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

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

MikeS commented on 2008-12-31 22:39

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.

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.

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.

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.

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.

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.

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?

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

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.

That’s interesting. How did you manage to disable lcd updates? Does that mean you actually lcd_enable()? Let’s see your patch.

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.

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.

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.

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?

I’m not sure, but I think scrolling engine is already disabled without backlight (or rather, when the lcd is disabled)?

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.

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)

Should be fixed with this one.

Fix a bug with track changing.

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.

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.

Right. But the Clip should have it too I think. I’ll correct the 5G one.

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.

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.

Sync. A good deal less changes are needed now.

Hopefully last update.

Still could need testing on remotes.

Is there remotes that are readable without backlight?

petur commented on 2009-03-23 06:28

yes, iriver remotes… I’ll try to test tonight

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.

Probably the last update before committing. Needs a test on a remote target.

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

I’ll fix that when I come home.

sadur commented on 2009-04-14 18:04

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.

The fast switching sublines thing should really be fixed. I cannot reproduce it on my sansa.

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

Available keyboard shortcuts

Tasklist

Task Details

Task Editing