Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category LCD
  • Assigned To No-one
  • Operating System
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Nicolas Pennequin - 2006-01-15
Last edited by Nicolas Pennequin - 2006-02-15

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

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)

The task blocks this from closing
ID Project Summary Priority Severity Assigned To Progress
4909 Rockbox  FS#4909 - disable buttons if backlight is off  Very Low Low
100%
Closed by  Björn Stenberg
2006-03-24 16:17
Reason for closing:  Accepted
Anonymous Submitter commented on 2006-01-16 13:16

could you possibly add a Config menu for this?
most users on MisticRiver said it would be better if this
is user selected…

Admin
Linus Nielsen Feltzing commented on 2006-01-16 14:11

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.

Nicolas Pennequin commented on 2006-01-16 17:52

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

Nicolas Pennequin commented on 2006-02-15 23:13

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

Paul van der Heu commented on 2006-03-22 09:21

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

Nicolas Pennequin commented on 2006-03-22 12:26

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.

Nicolas Pennequin commented on 2006-03-22 15:36

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.

Nicolas Pennequin commented on 2006-03-22 22:37

Oops : fixed the compiler warning and made the setting be saved.

David Rothenberger commented on 2006-03-23 05:15

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

Nicolas Pennequin commented on 2006-03-23 10:10

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.

Admin
Björn Stenberg commented on 2006-03-23 10:21

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.

Mike Holden commented on 2006-03-23 10:54

Not sure I see the need for this, as the record button on the h340 already turns on the backlight.

Paul van der Heu commented on 2006-03-23 11:48
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..

Nicolas Pennequin commented on 2006-03-23 15:53

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.

Admin
Björn Stenberg commented on 2006-03-23 16:08

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.

Nicolas Pennequin commented on 2006-03-23 16:58

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 ?

David Rothenberger commented on 2006-03-23 17:21

Does you new patch work with the remote?

Nicolas Pennequin commented on 2006-03-23 20:16

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.

Matthias Mohr (aka Massa) commented on 2006-03-23 20:33

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

David Rothenberger commented on 2006-03-23 22:44

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

Available keyboard shortcuts

Tasklist

Task Details

Task Editing