This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#6800 - Sansa e200 backlight fade in/out
Attached to Project:
Rockbox
Opened by Ivan Z (zivan56) - Sunday, 11 March 2007, 23:12 GMT+2
Last edited by Steve Bavin (pondlife) - Wednesday, 26 November 2008, 09:26 GMT+2
Opened by Ivan Z (zivan56) - Sunday, 11 March 2007, 23:12 GMT+2
Last edited by Steve Bavin (pondlife) - Wednesday, 26 November 2008, 09:26 GMT+2
|
DetailsThis 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 
e200_lcd.patch
(2 KiB)
backlight-e200.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- backlight-target.h | 1 + 2 files changed, 43 insertions(+), 4 deletions(-) |
This task depends upon
Closed by Steve Bavin (pondlife)
Wednesday, 26 November 2008, 09:26 GMT+2
Reason for closing: Accepted
Additional comments about closing: Thanks!
Wednesday, 26 November 2008, 09:26 GMT+2
Reason for closing: Accepted
Additional comments about closing: Thanks!
Any idea what might be the reason?
does this need to be configurable? I dont think so.
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).
it doesnt work though :p
I'm hoping another pair of eyes can figure it out.
currently it only dims once :(
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.
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?
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?
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.
Although I have commit access, I'd like the opinions of other devs before this goes into SVN.
+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
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.
- 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!
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.
Works again for my Sansa e260
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
I added some comments and copyright notes. Old code is still working when removing the #define.
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.
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.
Then use hardware when possible/desired on individual targets?
H300 would be my named target, but why not just do all targets (or all that can set the backlight brightness).
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?
-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).
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.
still contains beast, although LambdaCalculus37 reported MikeS to fade "a little more nicely"
still need h300 testers please.
implement his suggestion about queue_wait_w_tmo
- 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.
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).
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.
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
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).
Edit: yes, I looked at the code and couldn't work out why the fade in seemed faster ;)
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.
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;
}
I was already curious if setting the brightness is enough for _backlight_init().
Thanks for your testing and help.
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.
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.
Having corrected this, it works fine on H300.
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.