Rockbox

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

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.

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

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

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

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.

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.

   backlight_navi_v3.patch (6.8 KiB)

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

   backlight_h300.patch (7.1 KiB)

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

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.

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.

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

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

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.

   backlight.patch (6.5 KiB)

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.

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 ?

   backlight_v2.patch (6.2 KiB)

Does you new patch work with the remote?

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.

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

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.