Rockbox

Tasklist

FS#8400 - Option to keep backlight off while changing volume etc.

Attached to Project: Rockbox
Opened by Alex Suykov (alexs) - Friday, 04 January 2008, 08:41 GMT
Task Type Patches
Category LCD
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

Some operations — like changing volume, switching to next song etc. — just don't require looking at the screen or even taking the DAP out of one's pocket. Nevertheless rockbox will turn power-expensive and sometimes annoying backlight on for every single keypress.

The patch adds an option to prevent any button but SELECT and/or POWER from turning backlight on, thus providing some king of manual backlight control.

The patch was tested on Sansa C200. It should work fine on other players as well, but the choise of buttons may not be the best one and may require tweaking.
This task depends upon

Comment by Michael Hahn (disorganizer) - Friday, 11 January 2008, 22:19 GMT
definitely this is exactly what i waited for :-) hope this compiles well for all targets.
can it be turned on and off in the menu?
Comment by Alex Suykov (alexs) - Saturday, 12 January 2008, 17:55 GMT
yes, General Settings/Display/LCD Settings/Which buttons turn backlight on.
Comment by Jonathan Gorin (crimsonking896) - Saturday, 12 January 2008, 22:47 GMT
With build r16061M for the ipod video target, the code patches without error but when compiling I receive a error

drivers/button.c:493:2: error: #error Can't guess BACKLIGHT_BTN_POWER for this targer
drivers/button.c: In function 'set_backlight_filter_buttons':
drivers/button.c:503: error: 'BACKLIGHT_BTN_POWER' undeclared (first use in this function)
drivers/button.c:503: error: (Each undeclared identifier is reported only once
drivers/button.c:503: error: for each function it appears in.)
make[1]: *** [/home/Jonathan/rockbox/build/firmware/drivers/button.o] Error 1
make: *** [build] Error 2

Thanks for this patch, I was looking for something like this for a while.
Comment by Alex Suykov (alexs) - Tuesday, 15 January 2008, 15:59 GMT
Yeah, seems like there's no POWER on iPod Video...
Ok, I thought a bit and here's version 2. Apply _instead_ of the first one.
Should work for iPod, please check it now.
Comment by Alexander Levin (fml2) - Tuesday, 15 January 2008, 16:09 GMT
I also like the idea. But while you're at it, why not make this setting an int with potentially many 'filters'? Now you have two but there could be more.
Comment by Jonathan Gorin (crimsonking896) - Tuesday, 15 January 2008, 17:01 GMT
I just tested the new patch, it compiles successfully and the menu shows up in the compiled build on rockbox, but even when setting it up to deactivate the back light for volume and the left and right buttons, they still trigger the back light. I compiled with the latest build from svn.
Comment by Alex Suykov (alexs) - Wednesday, 16 January 2008, 21:28 GMT
Strange, current svn build works as expected on Sansa, and I can't figure out what may prevent it from working on iPod. Try changing BACKLIGHT_BUTTON in firmware/target/ipod/button-target.h, say leave only BUTTON_SELECT there. Also, check config file for "backlight on buttons" parameter -- is it there?

> why not make this setting an int with potentially many 'filters'?
> Now you have two but there could be more.
For the same reason keymaps are hardcoded in rockbox. No need to. You can always change it in button-target.h and recompile rockbox leaving no useless code in it. BTW, I removed second filter option in v2 patch
Comment by Jonathan Gorin (crimsonking896) - Wednesday, 16 January 2008, 23:36 GMT
I edited the button-target.h (attached) to #define BACKLIGHT_BUTTON BUTTON_SELECT and checked to see if is in the config file and it is (attached) but it still triggers the back light with any action on the scroll wheel.
Comment by Michael Hahn (disorganizer) - Thursday, 24 January 2008, 21:19 GMT
i made a build with only this patch, and it doesnt work for the wheel on the sansa.
the forward/back keys etc do not trigger the backlight, but the scrollwheel still does trigger it.
as the mein purpose of this patch is to disable backlight on volume+-, i consider this is a bug?
(current svn as of a few minutes ago)
Comment by Josh Dionne (JTD121) - Sunday, 27 January 2008, 15:56 GMT
Anyone tried this on an iRiver H300 yet? Been getting some requests to add this in, but I wanted to know if I just had to add the patch, or download the two attachments that Mr. Gorin has so kindly provided as well? I have no qualms with changing the source before any patches, just wanted to know how this one is supposed to be applied.

We should have a small plugin that comes with this that will allow you to choose what button to use. And also have a way to detect the play it's on? Maybe different versions for different targets?
Comment by Jonathan Gorin (crimsonking896) - Sunday, 27 January 2008, 16:14 GMT
This patch seems to work on some targeted builds but not others, it compiles successfully with rockbox-backlight-keys-v2.diff and the option is clearly visible from rockbox's menu, but the function does not seem to work for the ipod 5.5gen player. I would say try and apply the rockbox-backlight-keys-v2.diff only and see if it affect the iriver H300 and if not you will have to take a look at the source to see if you can catch the problem (due to inexperience with C I am unable to locate the exact problem.) Hope that helps.
Comment by Michael Hahn (disorganizer) - Monday, 28 January 2008, 20:53 GMT
did any1 get this patch working on sansa?
Comment by Alex Suykov (alexs) - Tuesday, 29 January 2008, 11:16 GMT
Josh Dionne: To build it, apply v2 (or v3 now) patch only, that should be enough.
You can select those buttons by changing
#define BACKLIGHT_BUTTON
in button-target.h for your specific device. Also, any feedback will be appreciated.

Regarding other targets — sorry, I'm totally puzzled, simulator seems to work well and still I have no real
devices to test it on. Here's v3, which won't break simulator builds like v2 did ;-), and a small
patch to make it warn you of what's going on. Try playing with them, maybe you'll get some ideas...
Comment by Michael Hahn (disorganizer) - Wednesday, 30 January 2008, 00:30 GMT
just one thing i noticed:
would it be possible to have an option to only disable backlight for volume changes?
(imho options could be: all, not for volume, not for volume+trackskip, selected only)
the config-flags could be named all,novolume,novolumenskip,selected

the problem with having only menu and select enable the backlight is that you can easily go to a menu or other things without you noticing it (because the backlight doesnt turn on, but all keypresses are accepted).

another option would be to turn on the backlight as soon as another screen than the wps is displayed :-)
Comment by Josh Dionne (JTD121) - Friday, 01 February 2008, 16:23 GMT
Downloading fresh SVN and will try patching with the new v3.diff
Comment by Josh Dionne (JTD121) - Friday, 01 February 2008, 17:40 GMT
Got SVN r16195. Patched this (backlight_keys_v3) as well as multifont (1/22/08), BMP Resize (12/05/07), and the Rec_button patch. Will not compile, image attached is the error(s), and I cannot figure out what it's talking about. Looking at the source file, it looks fine. I know this has multiple patches on it, but I tried to build with each patch, and each time it's done this.
Comment by Alex Suykov (alexs) - Saturday, 02 February 2008, 18:09 GMT
Josh Dionne: strange, r16200 with backlight_keys_v3 and multifont patches had no problems compiling action.c for target 11 (H320/H340) with binutils-2.17 and gcc-4.0.3. Which version of compilter/binutils do you use?
Also, which FS# are two other patches?

me (disorganizer): well, there were two options in the first patch, but after using it a while I changed that. The idea was that BACKLIGHT_BUTTON should contain all buttons which open menu etc., so try tweaking it. Or perhaps take relevant portions from v1.

Turning BL on as screen changes sounds much more appealing to me though, I'll think of it.
Comment by Michael Hahn (disorganizer) - Saturday, 02 February 2008, 21:07 GMT
after thinking, propably it even is more efficient than watching the keys.
so if the wps is left, the backlight turns on. but if it still there after the keypress, it will stay off.

Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 10:12 GMT
i changed the keypresses for a test to not turn on the backlight for left/right/scrollup/scrolldown. this seems to be the best setting, as all other keys also open up a menu on the e200.
i also noticed that on left/right (aka track skipping) the caption backlight still works, which is nice :-)

but now the bad thing is:
this ONLY works in the simulator. when putting the patch on a real device it does not work (scrollwheel does turn on backlight).
the simulator showed no output with your debug commands, also the values for scrollwheel-actions match.

so i wonder:
why dont i get a debug output from your debug patch for the backlight on scrollwhell actions?
meaning: is there another place where the backlight is turned on when i use the scrollwheel?

btw:
the patch needs synching in the english.lang patch.
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 10:14 GMT
another idea: wouldnt it be better to modify your debug patch to capture the "backlight_on" function calls?
if the scrollwheel fires backlight_on but your original debug patch does not trigger, there must be some other function calling backlight_on when a scrollwheel action is detected, so your patch then needs to be expanded to also cover that function.
Comment by Alex Suykov (alexs) - Monday, 04 February 2008, 14:28 GMT
> but now the bad thing is: this ONLY works in the simulator.
Yeah, that's the main problem, as simulator is all I have.

> is there another place where the backlight is turned on when i use the scrollwheel?
there's buttonlight_* that can be triggered, and there's remote_backlight_* but e200 lacks
remote display, and seems like that's all.

> another idea: wouldnt it be better to modify your debug patch to capture the "backlight_on" function calls?
It's not that easy, but on simulator any call to backlight_on will result in subsequent call to sim_backlight, so effectively it's the same.
For the real device... here's a patch to make it more talkative, and second one to make logf() write on primary lcd. Perhaps it will give some clues.
Don't forget to enable logf support during build.
Also, feel free to add logf() to suspicious places, it should work almost anywhere.
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 22:17 GMT
i think i found something:
in file firmware\target\arm\sandisk\sansa-e200\button-e200.c there is a call to backlight_on() in line 197.
as far as i understand the code, this controlls the backlight behaviour for the scrollwheel.

i dont have my sansa handy right now so i cant compile and check, but i think this may be the cause for the "problem". could you check whether this could be the cause?
if not it will take some time until i find time and access to my e200 *g*
Comment by Michael Hahn (disorganizer) - Tuesday, 05 February 2008, 00:22 GMT
experimental patch to allow the e200 to not turn on the backlight on volume change or trackskip.
i commented the "backlight_on()" call in button-200.c to disable it.

i could not test it, but imho it should work (only for e200!).

at least it patches against 16213 without errors and compiles without errors :-)

if someone with an e200 is willing to test:
* try with "backlight on all keys" whether the scrollwheel still turns on the backlight. is there any difference when using the scrolllight?
* try with "backlight on selected" and check the scrollwheel now does not turn on the backlight.
Comment by Josh Dionne (JTD121) - Tuesday, 05 February 2008, 15:32 GMT
Alright, got it to work wonderfully on H300. Using r16213, and I am pretty sure there was another patch, but not quite sure which one as of now. So much to do during the day, and not enough time, ya know?
Comment by Michael Hahn (disorganizer) - Tuesday, 05 February 2008, 20:41 GMT
i tested my patch above, and it works too good.
even in the menu the scrollwheel is not recognised as movement, resulting in the backlight going off even if you move the scrollwheel. *sigh*
Comment by Michael Hahn (disorganizer) - Thursday, 14 February 2008, 21:38 GMT
hmm. so the next question is:
how can i let button-200.c know whether the active screen is the wps or not?
or said in another way:
as the scrollwheel turns on the backlight in button-200.c, how can i distinguish between "volume up/down"-moovements or "menu navigation" moovements of the scrollwheel?
or saind in yet another way:
if i disable the backlight_on() call as mentioned above, how can i ensure the backlight is turned on as soon as menu navigation is used instead of volume controll?
Comment by Alexander Levin (fml2) - Tuesday, 19 February 2008, 08:32 GMT
I think this is a general problem of how the backlight is turned on. It's done at the 'firmware' level, i.e. without knowing the app context (WPS/menu/...). To implement the feature right, one should IMHO maintain (in the app layer) a flag telling whether the backlight should be turned on if a key is pressed. And push the value of the flag (on every change) down to the button driver (or to the backlight thread). I don't know how easy or difficult it would be to implement such flag. It might be just a couple of places (e.g. menu function and WPS function) but it also could be all over Rockbox code. Someone with better code knowledge should tell. JdGordon?
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 21:16 GMT
during irc yesterday someone braught up the idea to have a variable in button?.c and a function to toggle this variable when the wps is displayed and toggle back when the wps is left into another screen via a function call (in button?.c).

this variable could then be use to toggle whether the backlight should be lit on every buttonpress and scrollwheel moovement or not.

as im no programmer im sadly not in the condition to implement this.
Comment by Jacob Brooks (jac0b) - Monday, 07 April 2008, 23:02 GMT
Resync
Comment by Jacob Brooks (jac0b) - Wednesday, 21 May 2008, 00:13 GMT
Resync
Comment by Jacob Brooks (jac0b) - Wednesday, 11 June 2008, 23:10 GMT
Resync
Comment by Jacob Brooks (jac0b) - Thursday, 12 June 2008, 00:08 GMT
Disregard the above patch I made a mistake on it. Use this one.
Comment by Shiloh Hawley (gree665) - Tuesday, 17 June 2008, 05:27 GMT
Hello I love this patch, only have one problem with it: When changing volume, skipping tracks, etc the screen does not light up (this part works properly), but the button lights do light up. I would prefer the button lights to stay off when the screen stays off, and only be triggered by SELECT, MENU, or POWER, just as the screen light is.
Comment by Shiloh Hawley (gree665) - Tuesday, 17 June 2008, 19:01 GMT
Sorry for the double post, I forgot to mention I am using a Gigabeat F.
Comment by John Gu (einmus) - Friday, 15 August 2008, 16:01 GMT
I too love this patch! However,as Michael Hahn has said menu navigation and volume control behaviors cannot be separated treated by this patch.

I have an alternative idea: The short REC button is rarely used except for the recording screen to start record (you can also press PLAY to achieve this action). So why don't we invest this REC button to be the backlight switch?
Comment by Jacob Brooks (jac0b) - Sunday, 30 November 2008, 14:44 GMT
Resync
Comment by Jacob Brooks (jac0b) - Monday, 12 January 2009, 20:38 GMT
Resync
Comment by Jacob Brooks (jac0b) - Wednesday, 11 March 2009, 23:43 GMT
Added the setting options for the menu for the gigabeat s-series
Comment by Shiloh Hawley (gree665) - Sunday, 23 August 2009, 19:58 GMT
As mentioned earlier, I want the button lights (Gigabeat F) not to go on in those keypress cases when the backlight does not go on. If you are using the backlight patch with a Gigabeat F, I suggest you use the following patch as well to fix this issue. Actually this is just duplicated functionality, from what I can tell, I may submit this as a separate patch as well.
Comment by Shiloh Hawley (gree665) - Sunday, 23 August 2009, 21:17 GMT
As mentioned earlier, I want the button lights (Gigabeat F) not to go on in those keypress cases when the backlight does not go on. If you are using the backlight patch with a Gigabeat F, I suggest you use the following patch as well to fix this issue. Actually this is just duplicated functionality, from what I can tell, I may submit this as a separate patch as well.
Comment by Shiloh Hawley (gree665) - Tuesday, 08 September 2009, 04:36 GMT
Sorry for the double post there, I shouldnt have used the 'back' button.

Your patch gets rid of the 'First Buttonpress Enables Backlight' option in the settings menu. So depending on the settings that are remembered from the previous build installed (saved in the config.cfg file), the behavior will vary when using this patch, and the only way to change the behavior is to manually edit the config file.

Consider the default setting where the 'First Buttonpress Enables Backlight' is off, and you use this patch and set 'Backlight On Buttons' to 'POWER, SELECT, and MENU'. Suppose you are on the WPS with the backlight off, and you want to see the name of the song that is playing. You have no way to do this, pressing POWER turns on the backlight, but it also stops the playback, for example.

I prefer the behavior when 'First Buttonpress Enables Backlight' is on, this way with your patch, you can skip tracks or change the volume without enabling the backlight, but if you want to view your WPS screen you can use POWER, SELECT, or MENU to turn on the backlight without doing anything else.

So I propose you either (1) force the 'First Buttonpress Enables Backlight' option to be on whenever 'Backlight On Buttons' is set to 'POWER, SELECT, and MENU' or (2) leave the old 'First Buttonpress Enables Backlight' option in the settings.

Here is a patch with the 'First Buttonpress Enables Backlight' option restored (sugestion 2 above), for anyone interested.

Loading...