Rockbox

Tasklist

FS#2920 - H300 : First press on NAVI turns on backlight if it's off

Attached to Project: Rockbox
Opened by Nicolas Pennequin (nicolas_p) - Sunday, 15 January 2006, 22:32 GMT
Last edited by Nicolas Pennequin (nicolas_p) - Wednesday, 15 February 2006, 23:13 GMT
Task Type Patches
Category LCD
Status Closed
Assigned To No-one
Operating System
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

H300 : First press on NAVI turns on backlight if it's off

Simple patch to create a behaviour similar to the
original iRiver firmware : when the backlight is off, a
press on NAVI will only turn the LCD on.
NAVI is currently the only button with this behaviour.
Maybe i'll try to make it work exactly like the iRiver
firmware but i'm pleased with it as it is.

(use the -p1 switch to patch)
This task depends upon

View Dependency Graph

This task blocks these from closing
 FS#4909 - disable buttons if backlight is off 
Closed by  Björn Stenberg (zagor)
Friday, 24 March 2006, 16:17 GMT
Reason for closing:  Accepted
Comment by Anonymous Submitter - Monday, 16 January 2006, 13:16 GMT

could you possibly add a Config menu for this?
most users on MisticRiver said it would be better if this
is user selected...
Comment by Linus Nielsen Feltzing (linusnielsen) - Monday, 16 January 2006, 14:11 GMT

You need to return a value from button_read(). You should use

return 0;

instead of just

return;

Also, don't use TAB for indenting.
Comment by Nicolas Pennequin (nicolas_p) - Monday, 16 January 2006, 17:52 GMT

Linus, thanks for your comment. I've changed the tabs to
spaces and i replaced "return;" by "return -1;" ("return 0;"
didn't work as expected)

I also fixed a bug i found that blocked all NAVI presses
when the charger was plugged and the backlight when plugged
was set to on.
The previous patch was a bit messy and included a leftover
of the brightness patch... That's fixed too (sorry).

Next step is to add an option for this, but i still have to
figure out how...

There also is something i forgot in the summary : thanks to
linuxstb and Matze for helping me with this :-)
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 15 February 2006, 23:13 GMT

it seems adding a setting for this is a bit difficult, as my
patch deals with firmware and settings are on apps side.
I think i'll need dome help as i still have lots of things
to discover about rockbox :)
Comment by Paul van der Heu (paulheu) - Wednesday, 22 March 2006, 09:21 GMT
There appears to be a problem with the current CVS and this patch. It cause playback to pause for a senond or so when you press NAVI..
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 22 March 2006, 12:26 GMT
I think it's not really new and it seems to happen only on long presses. I don't really know what causes this problem but i'll try to have a look.
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 22 March 2006, 15:36 GMT
I improved the patch a lot, by finding a much smarter way to filter keypresses. Now it won't make the playback pause anymore.
I also added a setting (general setting > display > LCD settings), which allows to choose between 3 different behaviours :
- "None" : default rockbox behaviour, no keypresses are filtered
- "Only NAVI" : NAVI is the only button which will turn the backlight on, exactly like in the old version of the patch
- "All buttons" : This reproduces the iriver firmware behaviour, where all buttons are filtered if backlight is off

Now it's easy to add other buttons than NAVI, but IMHO NAVI is the best one for this.
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 22 March 2006, 22:37 GMT
Oops : fixed the compiler warning and made the setting be saved.
Comment by David Rothenberger (drothenberger) - Thursday, 23 March 2006, 05:15 GMT
I am really interested in this patch. I've always found it annoying that there is no way to enable the backlight without changing something. But, since I have an H140, this doesn't work for me.

Any chance this could be made to work with the H140? Any chance it could also work from the remote (e.g., when the Play/Pause button was pressed)?
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 23 March 2006, 10:10 GMT
Well with te recent changes i made, it's now quite easy to change the keys used, so what you suggest should be possible. Also it's only limited to the H300 because i thought it was the only target where something like this made sense. Maybe i should extend it to all targets with a backlight... The only problem is i'm afraid of bloating the code with too much possibilities.
Comment by Björn Stenberg (zagor) - Thursday, 23 March 2006, 10:21 GMT
I think this is a general issue for all players, and should be solved in a general manner with a single option that is available for all targets: "Ignore first button when backlight is off" (though preferrably a better and shorter name).

This is how most mobile phones work and is indeed valuable for devices with screens which are unreadable (or nearly) without backlight.

Configuring "this only happens for a specific key" is overkill IMHO and only adds code bloat and user confusion.
Comment by Mike Holden (mikeholden) - Thursday, 23 March 2006, 10:54 GMT
Not sure I see the need for this, as the record button on the h340 already turns on the backlight.
Comment by Paul van der Heu (paulheu) - Thursday, 23 March 2006, 11:48 GMT
> Configuring "this only happens for a specific key" is overkill IMHO and
> only adds code bloat and user confusion.

It's actually quite useful, Imagine you have your player in your pocket and you FF/REV/skip or whatever by pressing these keys 'blind'. There is no need for the backlight, yet at each keypress it turns on using up energy..
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 23 March 2006, 15:53 GMT
This is a simpler version of the patch, that will apply to all targets with a backlight. It should be OK for committing.
The setting now has only two possible values : "none" or "all buttons". I decided to keep it open to adding other possibilities, that's why the value isn't just stored as a boolean.
Also the setting defaults to "all buttons" on color LCD targets, and "none" on others.
Comment by Björn Stenberg (zagor) - Thursday, 23 March 2006, 16:08 GMT
Not to be a grumpy old fart, but please don't write this way. The code should do exactly the job you want to do, nothing more. Adding "possibilities for the future" in reality just means including cruft we don't use and making it less clean. It's much better to extend the code later, should we want it.

Also, we need to come up with a better name for the option, and better function/variable names. As it is now, it reads as if it controls whether backlight is turned on by buttons (at all) or not.
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 23 March 2006, 16:58 GMT
OK, i changed it so the setting now only uses a boolean.
Also i tryed to find better names for the variables/functions.
What do you think ?
Comment by David Rothenberger (drothenberger) - Thursday, 23 March 2006, 17:21 GMT
Does you new patch work with the remote?
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 23 March 2006, 20:16 GMT
No, it doesn't. If i implement it for the remote, though, it will work exactly like my latest does for the main unit : if the setting is "on", the first keypress will be filtered, whatever the button. I'm not sure this is what you want. Still, it might be a good idea to add it and like for the main unit, maybe extend it later with another patch.
Comment by Matthias Mohr (aka Massa) (mmohr) - Thursday, 23 March 2006, 20:33 GMT
I hope if you'll implement it for the remotes,
it will have a separate setting...

I like to have the feature for my main unit,
but I don't see any sense for it for my (NON-LCD)
remote ;)
Comment by David Rothenberger (drothenberger) - Thursday, 23 March 2006, 22:44 GMT
I have an LCD remote and this feature is most important to me for the remote. I use the remote in the car (with the main unit tucked away in the console) and often want to turn the backlight on without making other changes.

Can't the backlight for the remote be controlled separately from the main unit? It would make sense to me to only turn on the remote's backlight when a remote button is pressed, so this wouldn't get in anyone's way for non-LCD remotes.

Loading...