This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#9305 - Context sensitive backlight on key press
Attached to Project:
Rockbox
Opened by Alexander Levin (fml2) - Wednesday, 20 August 2008, 22:14 GMT+2
Opened by Alexander Levin (fml2) - Wednesday, 20 August 2008, 22:14 GMT+2
|
DetailsThis is an attempt to achieve the goal once discussed in the forums and/or in irc. It should allow to not turn on the backlight on certain key presses. IIRC there is already a patch doing this. But a problem there was that the backlight is handled at the firmware level (as opposed to the app level) and hence it was not possible to take the context into account (whether we're in the WPS or in a menu or...).
This tries to solve this problem. The approach is to provide a callback to the button code (firmware level) that would tell if the backlight should be turned on. The callback itself has access to the app level information (context). The solution is not finished yet. TODOs: 1. Implement the backlight oracle (see the TODO mark in action.c) 2. Define the backlight properties for the key maps. It's not done in this patch so it produces many warnings (unassigned struct member). I nevetherless put up this patch to discuss the approach. Comments are welcome. 
bl_on_keypress.patch
(5.7 KiB)
apps/action.c | 10 ++++++++++ apps/action.h | 1 + firmware/export/button.h | 16 ++++++++++++++++ firmware/drivers/button.c | 28 +++++++++++++++++++++++++--- uisimulator/sdl/button.c | 25 ++++++++++++++++++++++++- 5 files changed, 76 insertions(+), 4 deletions(-) |
This task depends upon
Like I said in FS#8400, the short REC button of Sansa e200 is rarely used. We can invest that as the "backlight switch" as an alternative approach. Can anyone tell how I should do that, which file should I deal?
It works also in conjunction with the 'First Buttonpress Enables Backlight' option.
Scroll wheel support is only implemented for e200 yet, but should be possible for other targets as well.
The warnings about missing initializers are still there.
Though this is working quite well, I feel that doing an app level callback out of a ticktask or the scroll wheel interrupt handler is not very nice. I could think of moving the callback into the backlight thread, if there wouldn't be this 'First Buttonpress Enables Backlight' option...
FS#6800. You can build succesfully with both patches, but backlight-fade function was disabled.FS#6800in parallel with this patch without problems. Can you please double check? Did you get any errors while patching?1. I've introduced the additional field in the action structure to be able to very fine tune which action should turn the BL on and which not. In your patch, you do it by comparison. So the field can be dropped. Or is your solution also a temporary one?
2. I think there should be an option to enable this feature. Sometimes I use volume up/down just to turn on the BL (one step in volume change is not audible to me). It would be pity if this trick were disabled.
OK about the option. I'm just wondering how to name it... Would we have separate options for each action then?
As for the option, I'd call it 'Backlight on volume change'. I'd have just one option for all the keys which will be eventually covered by the "smartness", possibly with many values (true/false for now).
if (context == CONTEXT_WPS)
set_backlight_on_keypress_oracle(backlight_on_keypress_oracle);
else
set_backlight_on_keypress_oracle(NULL);
So, the callback will only be active in CONTEXT_WPS and we don't need a global variable which tracks the current context. If you agree, I would make up a new patch with this change, drop the additional field and define the new option.
I see one drawback in the solution above (your code snippet): while it eliminates the need in the static var, it places part of the 'oracle' logics into get_action_worker. I'd rather have all the oracle logic in the callback. You decide what's better! :-)
But there is a more important reason why I would prefer my suggestion. The callback is invoked either from the button ticktask or the scroll wheel interrupt handler. Both are rather time critical. So we really should avoid executing the callback unnecessarily.
If no one disagrees, I'll make a new patch as I get time.
You'll write it like below, right?
if ((context == CONTEXT_WPS) && <new option is activated>)
set_backlight_on_keypress_oracle(backlight_on_keypress_oracle);
else
set_backlight_on_keypress_oracle(NULL);
Then you don't need to check for CONTEXT_WPS in the oracle function.
And another question (just for the case): is it safe to call 'do_button_check' (as a parameter to backlight_on_action) twice with the same '&i'? Won't the effect of the first call (setting i) affect the second call? Wouldn't it be safer to use a different variable, say j, initially set to 0?
Otherwise I have no further concerns.
1. New option 'Backlight on volume change'. Defaults to 'Yes', you have to switch it off to make the patch work.
2. German translation for this option
3. Removed the static var. It can be easily reintroduced as it was not *that* bad after all.
4. Untested scrollwheel support for Ipods. Can't be tested in the sim, need testers.
This may be because the clip+ doesn't have a separate backlight (it has an OLED display) - does anyone have any ideas? I'd love to get this working on the clip+.
The patch didn't respect the ALLOW_SOFTLOCK bit of the clip+.
Please try the attached patch.
Thanks very much for updating this - I only just noticed the reply!
just for the record: I compiled the latest version of the patch and it works fine on my iPod Video 80GB.
On my Sansa Fuze V2 it works only for play/pause, but not for volume change (turning of the wheel). Any chance to get this added, too?
Cheers
Johannes
thanks for the report on the iPod Video. This patch now includes also support for the Fuze V2 scroll wheel. Would you please try it?
Johannes
1) get rid of backlight_on() in all the button drivers
2) after button_get() in action.c check the action returned is not WPS_VOL* or whatever, if it isnt call backlight_on()
Volunteers, be aware of the 'First Buttonpress Enables Backlight' setting. ;)
While browsing through my playlists and checking songs, I certainly don't want the backlight
to go on and off all the time. Unfortunately, the original patch prevents backlight only for
play and the volume buttons. So, I created a small modification of the original patch
that adds next and prev to the ignore list. Adding 6 more lines to an if condition is really
no big deal, which is why all credits should go to the original patch authors.
I've been testing this patch on my Clip+ for more than 3 weeks and didn't encounter any problems.
In my LCD settings, I set "Backlight" to 5s, "First Buttonpress Enables
Backlight" to Off and "Backlight on Volume Change" to No. The battery life is simply amazing now!