Rockbox

Tasklist

FS#9305 - Context sensitive backlight on key press

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Wednesday, 20 August 2008, 20:14 GMT
Task Type Patches
Category Applications
Status Unconfirmed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

This 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.
This task depends upon

Comment by John Gu (einmus) - Saturday, 23 August 2008, 07:13 GMT
This function is great! The backlight drains too much power!

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?
Comment by Martin Ritter (MartinR) - Wednesday, 03 September 2008, 12:09 GMT
Thanks for doing a new approach on this issue. Yet it doesn't work with the scrollwheel of the sansa e200, because there is another backlight_on call in clickwheel_int in button-e200.c. Same applies to ipods.
Comment by Martin Ritter (MartinR) - Wednesday, 17 September 2008, 08:07 GMT
This is a first functional patch. The backlight will not turn on on volume change and on pause/play.
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...

Comment by John Gu (einmus) - Friday, 19 September 2008, 08:07 GMT
Tested it, worked as proposed. But it conficts with backlight-fade patch  FS#6800 . You can build succesfully with both patches, but backlight-fade function was disabled.
Comment by Martin Ritter (MartinR) - Friday, 19 September 2008, 09:07 GMT
I'm using  FS#6800  in parallel with this patch without problems. Can you please double check? Did you get any errors while patching?
Comment by John Gu (einmus) - Friday, 19 September 2008, 12:43 GMT
Tested again with a clean download of src. Yes,it worked with both patches (actually plus anti-aliased font patch). Maybe I've messed up sth previously. Right now I'm very happy with this very patch! Thank you MartinR!
Comment by Alexander Levin (fml2) - Friday, 19 September 2008, 19:11 GMT
Martin, thank you for continuing the work! I have two more thoughts on this:

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.
Comment by Martin Ritter (MartinR) - Monday, 22 September 2008, 08:47 GMT
Alexander, actually I was just too lazy to fill in all those identifiers. But by now, I would also tend to drop the field, because there are just too few actions for which it would make sense. Actually, I can't think of any other context than the WPS, for which this patch would make any sense. So it should be enough to define the callback only in CONTEXT_WPS and set it to NULL otherwise. This way we would get rid of this current_context variable. What do you think?
OK about the option. I'm just wondering how to name it... Would we have separate options for each action then?
Comment by Alexander Levin (fml2) - Monday, 22 September 2008, 18:07 GMT
Martin, I think you're right in that this "smartness" only makes sense in the WPS. So we can drop the additional field since the decision is made by the logic and not data driven. If we'll need an additional field we can always introduce it. The general frame will remain the same. I don't quite understand what you mean by "define the callback in CONTEXT_WPS" and how we can get rid of the static variable. BTW: why do we need it? Isn't the context known at any time anyway? How is the context tracked then when the action is determined?

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).
Comment by Martin Ritter (MartinR) - Tuesday, 23 September 2008, 07:26 GMT
AFAIK the context is only known in get_action_worker(). That's why I introduced current_context to make it available in the callback. To get rid of it, I was thinking of doing something like this in get_action_worker():

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.
Comment by Alexander Levin (fml2) - Tuesday, 23 September 2008, 16:40 GMT
Ah, yes, now that I've looked into action.c I see what you mean. The context is passed to get_action as a parameter. I thought it was tracked somehow and thus was already known. So your solution with a new static variable is perfectly OK.

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! :-)
Comment by Martin Ritter (MartinR) - Wednesday, 24 September 2008, 08:30 GMT
I understand your point in concentrating the oracle logic in the callback. But it's a matter of definition somehow. If we define that the callback is intended exclusively for CONTEXT_WPS, then evaluating the context would not be part of the actual logic. Let's name it backlight_on_keypress_oracle_wps ;).
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.
Comment by Alexander Levin (fml2) - Wednesday, 24 September 2008, 17:08 GMT
Martin, if you understand all pros and cons but still think the way without the static variable is better then do it that way! I wouldn't rename the function to ..._wps thus keeping the option to make other contexts 'smart' as well. But I'd place a comment telling that you made it so deliberately and not as a mistake.

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.
Comment by Martin Ritter (MartinR) - Monday, 29 September 2008, 08:53 GMT
New patch as discussed.
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.
Comment by Martin Ritter (MartinR) - Monday, 29 September 2008, 08:55 GMT
Sorry, omitted the patch.
Comment by Alexander Levin (fml2) - Monday, 29 September 2008, 18:58 GMT
Hello Martin! Thank you for the work! I have just one question: why did you put the function 'backlight_on_by_button' into button.h? IIUC, it should not be exposed.
Comment by Martin Ritter (MartinR) - Tuesday, 30 September 2008, 07:41 GMT
Alexander, backlight_on_by_button() needs to be exposed for the scroll wheel handlers. See button-e200.c, button-1g-3g.c, button-mini1g.c... Probably I should have used #ifdef HAVE_SCROLLWHEEL here.
Comment by Alexander Levin (fml2) - Tuesday, 30 September 2008, 20:55 GMT
Ah, ok, that exposes the function within the same layer (firmware). And the other function is exposed to the higher layer (apps). But since we can't tell that this should be only exposed here and that there we just put them both into the header file. Case closed! :-)
Comment by Martin Ritter (MartinR) - Tuesday, 13 January 2009, 08:02 GMT
Resync
Comment by Martin Ritter (MartinR) - Tuesday, 10 March 2009, 09:17 GMT
Resync
Comment by Martin Ritter (MartinR) - Tuesday, 08 December 2009, 08:19 GMT
Resync
Comment by Martin Ritter (MartinR) - Thursday, 24 June 2010, 07:41 GMT
Resync
Comment by Ted Twain (ted209) - Sunday, 27 June 2010, 09:25 GMT
I think this is a great idea. I just compiled it for my Sansa Clip+. It shows up in the menu, but the screen comes on, on volume change.

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+.
Comment by Martin Ritter (MartinR) - Monday, 28 June 2010, 09:35 GMT
Hi Ted, I think I found it. At least, it works in the sim now.
The patch didn't respect the ALLOW_SOFTLOCK bit of the clip+.
Please try the attached patch.
Comment by Ted Twain (ted209) - Monday, 28 June 2010, 19:41 GMT
Works perfectly on the Sansa Clip+ now - thankyou very much!
Comment by Alexander Levin (fml2) - Tuesday, 29 June 2010, 16:24 GMT
Start a poll to commit it?
Comment by Martin Ritter (MartinR) - Wednesday, 22 September 2010, 07:39 GMT
Resync. Still not sure about the wheeled ipods.
Comment by Ted Twain (ted209) - Tuesday, 14 December 2010, 17:55 GMT
I don't know if it's a Sansa Clip+ only issue, but this patch doesn't work on the radio. Any chance of incorporating this functionality?
Comment by Alexander Levin (fml2) - Tuesday, 14 December 2010, 21:05 GMT
Hm... It seems that the patch only checks for the CONTEXT_WPS. I'm not sure whether this covers the FM screen as well. I suspect it does not.
Comment by Martin Ritter (MartinR) - Thursday, 16 December 2010, 08:47 GMT
Hi Ted, please try this one. It now checks for CONTEXT_FM as well. Works in the sim.
Comment by Ted Twain (ted209) - Saturday, 19 March 2011, 23:30 GMT
Just compiled a version with the latest patch for the Clip+, and it works with radio too!
Thanks very much for updating this - I only just noticed the reply!
Comment by Johannes Rauh (johnbtracker) - Monday, 18 April 2011, 17:50 GMT
Hi Martin,

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
Comment by Martin Ritter (MartinR) - Wednesday, 20 April 2011, 09:15 GMT
Hi 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?
Comment by Johannes Rauh (johnbtracker) - Wednesday, 20 April 2011, 09:54 GMT
Martin, works just fine now! Thanks a lot.
Johannes
Comment by Arno Cronje (arnoc) - Monday, 02 May 2011, 10:50 GMT
Excellent patch, I can confirm it's working on my Fuze V2. Thanks! :D
Comment by Martin Sägmüller (dfkt) - Saturday, 12 November 2011, 20:18 GMT
This patch seems to be out of sync as of now (action.c).
Comment by Martin Sägmüller (dfkt) - Saturday, 12 November 2011, 20:38 GMT
Resynced to r30969.
Comment by Darek (kepinskidar) - Sunday, 25 March 2012, 15:11 GMT
Can someone post working version?
Comment by Martin Ritter (MartinR) - Monday, 26 March 2012, 11:20 GMT
Resynced.
Comment by Jonathan Gordon (jdgordon) - Monday, 26 March 2012, 11:29 GMT
consider posting this to gerrit if you want developer attention
Comment by Alexander Levin (fml2) - Monday, 26 March 2012, 19:07 GMT
I think we should decide whether this feature is wanted or not. If yes then we should commit it (no need to go to gerrit for that); if not then I see no point in transferring it to gerrit. Maybe post a message to the developers' mail list?
Comment by Cástor Muñoz (prof_wolfff) - Monday, 26 March 2012, 22:46 GMT
This is a very useful patch for my, it works well on Classic and saves a lot of power if you are used to changing the volume frequently.
Comment by Ted Twain (ted209) - Monday, 26 March 2012, 22:59 GMT
I agree - I used this on my clip+ and now my fuze, I change volume all the time as I travel and it's great to know it's not sapping the power with this feature.
Comment by Johannes Rauh (johnbtracker) - Tuesday, 27 March 2012, 11:31 GMT
I also use this patch successfully on my Ipod Video, Sandisk Fuzes and Clip+es. And I find it very useful.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 27 March 2012, 11:43 GMT
after actually looking at the patch I don't like th eimplementation. Really the various button drivers should not be responsible for poking the backlight, this should be similar to how keyclicks are now handled (though the callback is much simpler).

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()
Comment by Martin Ritter (MartinR) - Wednesday, 28 March 2012, 07:09 GMT
I too think that the backlight handling should move from driver to app level. But that should go to a separate patch as it is a fundamental change to the concept. This has to be discussed and tested separately and should not be mixed with implementing a new feature, I think.
Volunteers, be aware of the 'First Buttonpress Enables Backlight' setting. ;)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 28 March 2012, 07:41 GMT
I agree, but I'm not inclined to push this patch without changing the backlight handling first.
Comment by Vladyslav Shtabovenko (vlad-sht) - Saturday, 23 June 2012, 18:28 GMT
First of all, I'd like to thank Alexander Levin and Martin Ritter for this wonderful patch.
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!

Comment by ts (kragaslynx) - Friday, 23 August 2013, 19:09 GMT
Resynced as of git commit fbc4ef7 on master
Comment by Juan Gonzalez (Nephiel) - Wednesday, 28 August 2013, 16:43 GMT
This feature is really useful. However, as it is now, it's possible for the backlight to turn off while actively using the device.

Consider this example: You're on the WPS. Let's say you're skimming over the current playlist, skipping tracks one by one by pressing "Next" until you find one you like. You want to see the title of each track, so you need the backlight. However, the "Next" button is on the ignore list, so when the backlight timeout is up, it will turn off on you - even if you just pressed the button a second before.

The same happens on the FM screen while scanning for a station or choosing a preset. I find that a bit annoying.

IMHO it's more intuitive to have every button press (even the ignored ones) restart the backlight timeout when the screen is on. I'm attaching a patch that does this (apply it on top of the current patch).
Tested on my own Clip Zip
Comment by Vladyslav Shtabovenko (vlad-sht) - Sunday, 15 September 2013, 13:44 GMT
I totally agree with Juan Gonzalez. Great idea!

However, for some strange reason I can't download any patch in this thread. The browser just opens an empty tab.
Could someone please put Juan's patch to PasteBin or something? I really would like to try that out on my Clip+
Comment by Juan Gonzalez (Nephiel) - Sunday, 15 September 2013, 14:59 GMT
The tab is not empty - if you scroll all the way down, the code is there.
No one seems to know why, but lately Flyspray prints out a lot of empty lines before any attached file.
Comment by Vladyslav Shtabovenko (vlad-sht) - Sunday, 15 September 2013, 17:21 GMT
Ahhh! Now I see it too. Thanks!

Your patch works great! I merged all three patches into one (against v3.13, current stable), just in case
someone would like to use it in a custom build.
Comment by MichaelGiacomelli (saratoga) - Monday, 23 September 2013, 17:52 GMT
Rather than check that the action is not one of a large number of things, would it be more efficient to just check that its whatever condition you're looking for?

Theres some white space changes which shouldn't be included, and I guess the debugf code is probably not needed either.

I'm not familiar with the rest of that code, so someone else would have to review it.

Loading...