Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Applications
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by fml2 - 2008-08-20
Last edited by speachy - 2020-06-15

FS#9305 - Context sensitive backlight on key press

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.

Closed by  speachy
2020-06-15 15:56
Reason for closing:  Fixed

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?

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.

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…

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.

I'm using  FS#6800  in parallel with this patch without problems. Can you please double check? Did you get any errors while patching?

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!

fml2 commented on 2008-09-19 19:11

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.

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?

fml2 commented on 2008-09-22 18:07

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).

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.

fml2 commented on 2008-09-23 16:40

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! :-)

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.

fml2 commented on 2008-09-24 17:08

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.

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.

Sorry, omitted the patch.

fml2 commented on 2008-09-29 18:58

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.

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.

fml2 commented on 2008-09-30 20:55

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! :-)

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+.

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.

Works perfectly on the Sansa Clip+ now - thankyou very much!

fml2 commented on 2010-06-29 16:24

Start a poll to commit it?

Resync. Still not sure about the wheeled ipods.

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?

fml2 commented on 2010-12-14 21:05

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.

Hi Ted, please try this one. It now checks for CONTEXT_FM as well. Works in the sim.

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!

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

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?

Martin, works just fine now! Thanks a lot.
Johannes

arnoc commented on 2011-05-02 10:50

Excellent patch, I can confirm it's working on my Fuze V2. Thanks! :D

dfkt commented on 2011-11-12 20:18

This patch seems to be out of sync as of now (action.c).

dfkt commented on 2011-11-12 20:38

Resynced to r30969.

Can someone post working version?

Resynced.

consider posting this to gerrit if you want developer attention

fml2 commented on 2012-03-26 19:07

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?

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.

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.

I also use this patch successfully on my Ipod Video, Sandisk Fuzes and Clip+es. And I find it very useful.

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()

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. ;)

I agree, but I'm not inclined to push this patch without changing the backlight handling first.

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!

Resynced as of git commit fbc4ef7 on master

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

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+

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.

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.

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.

This was implemented in dc87e9e9f3c383b63c3cb3713886a6c93b6a79d1 back in 2016-11-22.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing