Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category LCD
  • Assigned To No-one
  • Operating System Sansa e200
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes 3
  • Private
Attached to Project: Rockbox
Opened by zivan56 - 2007-03-11
Last edited by pondlife - 2008-11-26

FS#6800 - Sansa e200 backlight fade in/out

This adds a fade in/out feature similar to the one used on the iPod instead of just cutting power to the LCD.
The udelay used to leave the LCD in a low power state for a ~2 secs is not ideal, and probably needs to be removed or made non-blocking if possible. It would be useful to have the player in this state for ~5 sec if it didn’t adversely affect the response time of the player.

Ivan

Closed by  pondlife
2008-11-26 08:26
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Thanks!

It seems the udelay interferes with audio, just get rid of it…

this significantly slows down my sansa when playing audio

e200_lcd2.patch does not affect my e250 in any way in terms of performance (it was quite sluggish before). The first patch does slow the system down when in the low power state.

Synced to SVN and fixed a bug where it would revert to default brightness when brightness was set to below 3. Also reduced code size.

Synced to SVN.

jeton commented on 2007-06-14 16:25

Chrisjs, who maintains a cutom build for Sansa e200 series, reported a problem with compiling this patch.
Any idea what might be the reason?

I am aware of the cause of it not working - It was out of sync and I didn’t look through the code carefully enough, and mistakingly skipped a few hunks

Sync’d to SVN

This is a different way of donig the same thing. atm its not configurable and doesnt disable the lcd when its fading out (dont know why, uncommenting the 2 // comment lines will cause the sansa to crash

final version. this seems to work fine, even if the lcd sleep option is set to always.
does this need to be configurable? I dont think so.

MikeS commented on 2007-07-22 20:44

Big problem here: you cannot call the i2c driver from a tick task. It calls mutex functions and yields. Interrupts can only use queue_post and call code specifically intended for that context.

I also saw some traffic on IRC about this and there seemed to be confusion about lcd_sleep/enable? lcd_enable(false) only shuts down visible display operations, not the chip itself. lcd_sleep_does the full move to standby. lcd_enable(true) of course brings it back from any level of power down. Those functions yield and aren’t really multithread safe (yet).

here is another try which uses the backlight thread to do it all properly and thread-safely….
it doesnt work though :p
I’m hoping another pair of eyes can figure it out.
currently it only dims once :(

Anonymous Submitter commented on 2007-09-08 14:17

Sync please, I think recent SVN changes due to the c200 port broke this patch.

Anonymous Submitter commented on 2007-09-15 16:13

Synced it myself.

this patch no longer compiles against the current source

Pretty hard to sync, I’ll have a look another day, when I’ve got time.

Anyway, I did talk to the devs, we need to follow jdgordons approach (for thread-safety, proper coding etc), and we need to make it configurable in order to have a chance to get this committed. I hope someone will help us to do so.

Can’t someone resync please?

Resynced against r16053M-080111. Works fine on my e260 so far.
What exactly do we need to get this committed? Maybe the fade should be done in _backlight_on() and _backlight_off() instead of introducing __backlight_dim()? That way it would be compatible to the HAVE_BACKLIGHT_PWM_FADING stuff and backlight.c could be left unchanged. At least, further resyncs would be easier then.
Any suggestions?

We need to do exactly what JdGorden began. Thread-safety. Your suggestion doesn’t sound too bad. However, PWM fading isn’t proper. The way it acutally works, by settings the backlight brighness down in a intervall is much better, since it’s more accurate. I also think it looks better.

Thanx for your work on resyncing this patch Martin. Works fine on my e280

Kugel, of course no PWM on the Sansa. I wasn’t aware, that jdgordon did something completely different. Simply picked the last patch.

Anyway, I now managed to get jdgordon’s approach running, too. It does the fading in the backlight_thread() itself instead of a ticktask, which was considered to be bad. It works quite well, however, I don’t like it very much. While fading, the backlight_thread() feeds up itself with tons of BACKLIGHT_FADE_* events. Was it supposed to work this way, or should we rather use a timer to feed the queue?

Thanks a lot for your work, Martin.

I did a little rework, trying to gain the best of the 2 different solutions above.

Fading is now controlled by the new variable backlight_fade_state. It is set to 1 or 2 in _backlight_on() or _backlight_off(), indicating that either a fade-up or fade-down is desired. If so, backlight_tick() starts sending BACKLIGHT_FADE events to the backlight_thread() at a rate of HZ/25. Depending on the value of backlight_fade_state, the backlight_thread() then calls either backlight_fade_up() or backlight_fade_down() of the target. When fading is fished, backlight_fade_state is set back to 0, and backlight_tick() stops sending fade events.

Also fixed a bug when brightness was set to 1.

It’s been tested quite intensely. If further work is needed to get it committed, someone may tell me, please.

Synchronised against 17138.

Fixed a bug where the screen sometimes turned white in mpegplayer.

The patch works well, how about committing (generally, not necessarily before 3.0)?

Synced against r18656. Tested on my Sansa c240.

Although I have commit access, I’d like the opinions of other devs before this goes into SVN.

New version; forgot to make the patch for firmware/export/config_c200.h before.

Admin
fg commented on 2008-09-28 15:26

Some style observations:

+static int backlight_fade_state = 0; /* 0=not fading, 1=fading up, 2=fading down */

Why not an enum?

Also, backlight_fade_up() and backlight_fade_down() have two underscores. Why?

Sorry, no comments about “real” issues, as I’m not very familiar with this part of the code

I plan to extend the patch to enable lcd fading to more (if not all) targets.

After c200, I added premiliary support for Gigabeat S. I implemented fading down only for the gigabeat, since fading up is done in hardware automatically.

NOTE: It’s known-to-not-work-well. I got a report that fading is working, but the backlight brightness setting is ignored. Possibly more glitches.

Please test and help improving.

Here we go. Full support for Gigabeat S.

- disabled HW fading for now on gigabeat s, until we figure out how to use it correctly.
- don’t define HAVE_LCD_ENABLE (not implemented)

Many thanks to Robert Menes for testing!

Small update. Use enum as proposed by Frank + some optimizations to the e200 code.

I see two problems after attaching the latest patch, both on my sansa c200 and my e200:
1) when choosing ‘Backlight’ in the LCD settings menu, then selecting OFF, waiting until backlight is fully off and switching to ON, the backlight comes on but the screen is blank (full white on the e200, full black on the c200)
2) when Backlight OFF is selected, pressing the select button makes the backlight pulsate, repeatedly coming up at full brightness and fading to black.
Tested with default settings (REC held during boot) on SVN r18763.

Seems the Sansa (c200/e200) part of the patch has been broken as of r18992 (not necessarily that version, but that’s when I noticed). I got a syntax error when making, and reverting then trying to repatch gives a hunk error ONLY for the Sansa file(s). Here’s hoping it’s fixed.

jayme commented on 2008-11-09 22:32

Resync to rev. 19053

Works again for my Sansa e260

I already have an updated version which fixes bertriks issues in the pipeline. I want a gigabeat s tester first though.

Here you go.

Factored out much code. Most of it is in an extra file now. I reduced the changes to the backlight driver to a minimum, and other can targets can be more easily adopted. The code is much more readable as well.

Binsize
e200:
without patch:
Binary size: 576512
Actual size: 575240
RAM usage: 1423392

with patch:
Binary size: 576512
Actual size: 575784
RAM usage: 1423936

beast:
without patch:
Binary size: 522292
Actual size: 522284
RAM usage: 1632276

with patch:
Binary size: 522784
Actual size: 522776
RAM usage: 1632788

Current patch doesn’t allow Gigabeast builds to compile. A little detective work is in order to determine why.

forgot svn add the files

(Hopefully) fix beast.

(Hopefully) make beast fading up smoother.

Final version. I don’t intend to change anything, except adapting another target (which one?).

I added some comments and copyright notes. Old code is still working when removing the #define.

MikeS commented on 2008-11-17 00:57

Perhaps this is sort of out of place here. This uses the hardware fade feature of the MC13783 for the Gigabeat S. It has its own limitations but is very smooth regardless of initial brightness level and keeps the CPU relatively uninvolved.

So you propose to use hardware fading on the beast instead? I remember you said it’s hard to cope with, but if you got it to work I think it’s fine.
What do you mean with keeps the CPU uninvolved? The backlight thread is running regardless, I just made use of it.

Is the hardware fading you achieved smoother than the “software” fading we implemented? I assume you’ve compared both.

And finally, I’d like to get it committed, regardless of the fading beast is going to use. I think the fading we implemented here works nicely on e200/c200 and it can easily adopted to future ports which don’t offer some kind of hardware fading.

MikeS commented on 2008-11-17 01:51

There’s no fading between brightness levels on the beast but only for on/off and if you interrupt the fade early by pushing a button, it simply goes back to “on”.

The hardware fading is perfectly smooth no matter what brightness you start at. Changing brightness levels on the S can glitch because of the current level changes used to get more than the 16 levels only PWM can give.

For the ones that must use the thread, why post countless messages from the ISR instead of using queue_wait_w_tmo if in a fading state and otherwise using with queue_wait when the state is NOT_FADING? The brightness change can happen in SYS_TIMEOUT.

As an aside, is there any chance that a universal method could be used for software fade, so other targets (e.g. H300) could gain this feature? Might also make for simpler code….

Then use hardware when possible/desired on individual targets?

pondlife, that’s exactly how I’ve designed it. Other targets are relatively easy to adapt. Just name a target, and I’ll look into it.

hehe, I should have looked at the code, I guess. Maybe one day once i’ve finished my day job :).

H300 would be my named target, but why not just do all targets (or all that can set the backlight brightness).

Because I have no idea which target has hardware fading (I asked and it seems pretty much have such) or which don’t have backlight at all. And because I cannot test on targets I don’t own.

pondlife, ping me in irc (or message me in other ways) when you’re available for testing.

MikeS, I’m not sure what you mean. The backlight thread runs, and at every tick it checks for the fading state. It only does quere_post if it’s supposed to fade (”if (fading_state) && […]”), i.e. when the backlight timeout is reached. Is that wrong?

-Add h300 (hopefully).
-Fix MIN_BRIGHTNESS_SETTING for gigabeat s. It should be > 0, since 0 is no backlight you shouldn’t be able to set the backlight off by changing the brightness (other targets do that correctly).

fix warning by making all backlight_brightness signed

MikeS commented on 2008-11-19 01:06

BOO!

Thomas, I was talking about trigging the looping and letting the backlight thread run out the cycles without further posting of messages to it.

if (state != NOT_FADING)

queue_wait_w_tmo(&q, &ev, HZ/xxx);

else

queue_wait(&q, &ev);

if (ev.id == SYS_TIMEOUT)

 <do fading step>

On Gigabeast, the HW fading should suffice and I think I’ll finish that up and commit for it. Nothing prevents it being changed of course. The only reason I didn’t do it before was that I couldn’t seem to get it to work the first time I tried it. I don’t know what I did wrong, but no matter now.

MikeS commented on 2008-11-19 02:05

Update hardware patch for Gigabeat S

add settings for fading with reusing many of the present settings stuff from pwm fading.

still contains beast, although LambdaCalculus37 reported MikeS to fade “a little more nicely”

still need h300 testers please.

Works fine for me on H300; my personal preference would be for a slightly faster fade, but no big problem.

Works fine on my Gigabeast. The fade can be a little faster on the beast as well, but it’s no biggie.

fix red for h300 and adjust the delay between the brightness changes to be more dependent on the available brightness levels (see backlight_thread_fading.h for the calculation).

MikeS commented on 2008-11-19 23:25

The beast hardware fade time is fixed at 1 second so there are tradoffs for the smoother fade. There beast fading needs the settings work from here since those will be similar.

remove gigabeat since MikeS patch is preferable.
implement his suggestion about queue_wait_w_tmo

On H300, if you disable the Fade In, but leave Fade Out enabled, it fades out, then only comes back at a very low brightness setting. I have brightness set to 15, FWIW.

Try this one. With this version, I cannot reproduce it on my sansa.

Same problem happens with v10.

- quite major rework. e.g. backlight_brightness is now handled in backlight.c, which makes it possible to leave most backlight drivers completely untouched
- some code optimization
- preliminary support for iaudio x5

one issue: when trying to change the backlight timeout to always off, it doesn’t instantly fade out when selecting that option if fading is enabled (is instantly switching out when fading is disabled). I assume that’s related to MikeS’s propose which I implemented.

oh, and fix the h300 problem reported by pondlife.

1) Applying this patch breaks the Cowon D2 build. The old ‘backlight_brightness’ declaration in firmware/target/arm/tcc780x/cowond2/backlight-cowond2.c needs to be removed.

2) Fading out works nicely on the D2 with your patch, but fading in does not work (the backlight goes to brightness level 1 and stays there).

x5 is confirmed to work, without the issue I mentioned.

Ha, nice surprise :)

Try this one. backlight_brightness is now handled in backlight (set in backlight_set_brightness).

Looking at the backlight driver, is it similar to h300 and x5? all pcf50606_write use, but the d2’s is a bit different.

-preliminary support for cowon d2.

Thanks, that version works nicely (although the fade in is so fast that it’s only really noticeable when the brightness is set near to the maximum).

One more comment, I noticed in a few places the patch does something like:

#if defined(HAVE_BACKLIGHT_PWM_FADING)

  int backlight_fade_in;  /* backlight fade in timing: 0..3 */
  int backlight_fade_out; /* backlight fade in timing: 0..7 */

#endif
#ifdef USE_BACKLIGHT_THREAD_FADING

  bool backlight_fade_in;
  bool backlight_fade_out;

#endif

Wouldn’t this be neater as:

#if defined(HAVE_BACKLIGHT_PWM_FADING)
/* do stuff */
#elif defined(USE_BACKLIGHT_THREAD_FADING)
/* do other stuff */
#endif

It would be neatest if you could use ints in OFFON_SETTING :) But yea, you’re right.

So you find it too fast? It of course differs from target to target. But I heard voices that it’s too slow (and it’s perfectly for me on my e200).

Ah you only talked about the fade it. Yes, it looks fast, but it’s technically as long as the fade out. Probably has something to do with human eyes and/or brain and how they receipt luminance changes.

I’ll take back that comment - if the fade in were any longer it’d probably just be annoying. The fade in effect is very subtle when using the lower half of the brightness range (eg. 1-7 of a possible 14), but I guess that’s by design. I think it’s fine as it is.

Edit: yes, I looked at the code and couldn’t work out why the fade in seemed faster ;)

Yep. In the end, a longer fade in annoys more than a longer fade out (if they ever annoy that is ;) )

This version uses the old method of queue_post’ing BACKLIGHT_FADE again (not MikeS’ one with SYS_TIMEOUT).

That is working much better for me. With MikeS’ I experienced major quirks especially with backlight timeout (no instant fade down when selecting the option for example.
The fading also looks a bit smoother to me, but that might be my tired eyes.

I left the code I implemented for MikeS’ proposal in it, covered by #if defined() && 0 and so on, for your review.

So far there’re no open bug reports.

A small bug on the D2 (unrelated to the above): the D2’s screen is completely black without backlight, so the backlight needs to be enabled by default. This no longer happens since you removed the _backlight_on() call from _backlight_set_brightness(), so the screen goes black for about half a second during bootup.

To fix it replace _backlight_init() with the following:

int _backlight_init(void)
{

  _backlight_set_brightness(DEFAULT_BRIGHTNESS_SETTING);
  
  /* set backlight on by default, since the screen is unreadable without it */
  _backlight_on();
  
  return true;

}

Here you go. I also made the code police you mentioned before.
I was already curious if setting the brightness is enough for _backlight_init().

Thanks for your testing and help.

Hi kugel,

H300 build under Cygwin - I get a warning with 13b:

… CC firmware/backlight-thread-fading.c
/home/Steve/rockbox/firmware/backlight-thread-fading.c: In function `_backlight_fade_up’:
/home/Steve/rockbox/firmware/backlight-thread-fading.c:55: warning: control reaches end of non-void function

Running on H300 - “Fade Out only” now works ok, but “Fade In only” (i.e. in=on, out=off) doesn’t fade in at all - the display seems to glitch bright white before appearing.

first commit candidate. It’s using queue_wait_w_tmo, but it needs a queue_post when (and only once when) the backlight is set to off to keep that one glitch free.

The only thing I could imagine to be done: move the functions which are in defined backlight.c to backlight-thread-fading.c to keep that code a bit separated. But that would mean exporting more functions AND variables through backlight-thread-fading.h.

You may comment on that one. Apart from that, it’s working nicely for me and my e200.

Please test on all affected targets (e200, c200, x5, h300 and d2). Thank you.

There is a small disadvantage with the queue_wait_w_tmo approach. The fade rate is not as accurate as when using the ticktask, because it depends on the execution speed of the _backlight_fade_* functions. At high CPU load the fading might become slower or irregular. It starts getting visible on my e200 when playing ogg with eq enabled (all bands). Not quite much, but maybe more in other situations.

Hi Thomas - I don’t see a patch since v13d. That one doesn’t compile for H300 as backlight.c has a semi-colon missing from line 484.

Having corrected this, it works fine on H300.

Here you go, sorry about that.

Martin, that’s probably the reason I find not using queue_wait_w_tmo slightly smoother as well. I’ll look into it. This version is using queue_wait_w_tmo.

BTW: The e200 is generally to slow to run with all bands anyway.

Thanks - that works fine on H300.

Project Manager

Works fine on X5 as well.

v14 is working fine on D2 too.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing