Rockbox

Hi.

I don’t know if this is related to that I noticed yesterday. I’m not sure from when it’s happening (must be since last week), but if the LCD sleep is set to Always or to a number of seconds that leads to the LCD being OFF when the song changes, although the “Caption Backlight” is ON, the LCD won’t wake up. If I set the LCD sleep to Never the caption backlight is working as expected.

Anyway, thanks for your great job.

Sadur.

Sadur, that problem is entirely unrelated to this task. The problem described here can only happen when changing LCD settings. (I mean when you’re actually in those settings menus and scrolling or selecting settings.) Based on tests on my 5G 30GB iPod, the “Caption Backlight” problem is due to  FS#8523  (”Disable WPS updating when the backlight is off.”), which was committed in r20666.

I’m strongly in favor of solution 2.

This would be solution 2. It fixes some weirdness when switching from off to on and vice versa (as you said).

   backlight.c.vc.diff (1.7 KiB)

I see you removed the work around.

I actually noticed weirdness on my e200 too (entirely white screen), when I use the PWM code path for sleeping (which is posting to the queue, instead of calling lcd_sleep directly in backlight_lcd_start_countdown()). This doesn’t happen in SVN, but certainly shows that there’s something wrong too.

Putting disable_irq() into lcd_sleep seems to help, but I’m still wondering why this. The lcd_sleep of the e200 has a HZ/50 sleep, which means a) that it yields and b) that it runs for more than a tick. That might be related.

r20793 introduced backlight_lcd_sleep_countdown(true) into the “timeout < 0” part of backlight_update_state(). That code is executed at the start of the fade when selecting “Off” in the current backlight timeout setting menu. So, if “LCD Sleep (After backlight off)” is set to “Always”, the LCD will go to sleep at the start of the fade then, effectively bringing back  FS#10129  in that case. The simple solution is attached. Is it good enough? It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can’t think of any nice ways to do that at the moment.

   lcd_sleep_after_pwm_fade_when... (0.5 KiB)

Well, it should be called when fading is done. I don’t know where this exactly is. That’s the reason it’s at another place for software fading too.

I find it confusing that _backlight_on/_off() is used to start fading for PWM, but not for software fading (there, backlight_setup_fade_up/_down() does it) so I’m sorry that I overlooked that.

It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can’t think of any nice ways to do that at the moment.
Didn’t I do that?

Edit: I guess it’s done when BACKLIGHT_FADE_FINISH is posted, so it should probably be inserted into the switch cases for BACKLIGHT_FADE_FINISH, or not?

I find it confusing that _backlight_on/_off() is used to start fading for PWM, but not for software fading (there, backlight_setup_fade_up/_down() does it) so I’m sorry that I overlooked that.

Yes, that is confusing. PWM fading is a layer in between the platform-independent backlight code which is used on all targets which have a backlight and the target-specific backlight code. That layer offers the same _backlight_on() / _backlight_off() interface that target-specific code offers on targets without fading. It uses different target-specific functions however.

It might be nice to remove backlight_lcd_sleep_countdown(true) calls from PWM fading _backlight_off() code, but I can’t think of any nice ways to do that at the moment.
>Didn’t I do that?

Oh yeah, you removed the call from _backlight_off(). Thanks for pointing that out. It would be a problem if lcd_sleep_after_pwm_fade_when_backlight_always_off.patch was committed.
The backlight_lcd_sleep_countdown(true) in _backlight_off() was used when Rockbox is compiled with PWM fading support and backlight fade-out is disabled.
The backlight_lcd_sleep_countdown(true) in backlight_isr() is used when PWM fading support is compiled in, backlight fade-out is enabled, and the fade-out completes.
The backlight_lcd_sleep_countdown(true) in backlight_switch() is used if timer_register() fails or something else steals the timer interrupt used by PWM fading before the fade-out completes.
The latter two are not in _backlight_off() but they get executed due to a chain of events which start with a call to _backlight_off().

The simplest solution is to add the backlight_lcd_sleep_countdown(true) which was in _backlight_off() before and commit lcd_sleep_after_pwm_fade_when_backlight_always_off.patch. But, that’s inconsistent; only that one _backlight_off() calls backlight_lcd_sleep_countdown(true). Maybe the functions which start PWM fading could simply be renamed, maybe to backlight_pwm_fade_on() and backlight_pwm_fade_off().

Can’t there be a way to determine PWM backlight being turned off regardless of the setting? What’s the deal with BACKLIGHT_FADE_FINISH?

Oh, and _backlight_off() does not return before fading is finished? If yes, how is the current code not working? If not, how would your proposed changes help at all?

Can’t there be a way to determine PWM backlight being turned off regardless of the setting? What’s the deal with BACKLIGHT_FADE_FINISH?

In my previous message, I explained the different ways that the backlight can be turned off. There currently isn’t any one common point in backlight.c where all alternatives end. BACKLIGHT_FADE_FINISH is only used when fading is enabled and the fade finishes. BACKLIGHT_FADE_FINISH could be used as a common point, though it would be a bit of an inefficiency and one needs to be careful with it.

Oh, and _backlight_off() does not return before fading is finished? If yes, how is the current code not working? If not, how would your proposed changes help at all?

When fading is disabled, _backlight_off() calls target-specific code to turn off the backlight.
When fading is enabled _backlight_off() merely sets up PWM fading and returns.
PWM fading _backlight_off() shouldn’t behave differently from normal target-specific _backlight_off(). So yeah, I think PWM fading _backlight_off() and _backlight_on() need to be renamed to clarify this.

The problems in r20809 are:
1) When turning the backlight off via the currently used backlight timeout setting menu, with LCD fade-out enabled, the LCD goes to sleep at the start of the fade.
2) When no fading support is present or PWM fading is supported but turned off, the LCD won’t go to sleep when after the backlight times out.

lcd_sleep_after_pwm_fade_when_backlight_always_off.patch would fix 1). To fix 2) with PWM fading, backlight_lcd_sleep_countdown(true) could be added to the branch of PWM fading _backlight_off() which is executed when PWM fading is disabled. When there is no fading support, backlight_lcd_sleep_countdown(true) ought to be in do_backlight_off().

I’d definitely welcome a simplification here.

The simplest solution is to add the backlight_lcd_sleep_countdown(true) which was in _backlight_off() before and commit
» lcd_sleep_after_pwm_fade_when_backlight_always_off.patch. But, that’s inconsistent; only that one _backlight_off() calls backlight_lcd_sleep_countdown(true).
» Maybe the functions which start PWM fading could simply be renamed, maybe to backlight_pwm_fade_on() and backlight_pwm_fade_off().

I did that now. Hopefully it works again.

Thanks! The code seems correct. On my 5G 30GB iPod, LCD sleep after backlight off works both at the settings menu and normally, both with fading enabled and disabled.

Ah cool. So we can close this finally? :)

What about the brightness setting functions? Currently, functions which work directly with the hardware are being called outside of the backlight thread. I don’t see any weirdness, but I think they should be called from within the backlight thread. Here’s a patch.

   set_brightness_via_bl_thread.... (2.7 KiB)

Seems nice. Noticed no difference. Go for it :)