Rockbox

In button-clickwheel.c (firmware/target/arm/ipod) you can see that "backlight_on()" is called in the ISR of the button handling. It is called each time an interrupt was generated from the scrollwheel sensors. So, it kindof overrides the "keypress" setting. I cannot play with the code right now as I am not on my dev PC, but I am pretty sure that this is the root cause. Furthermore I don't think that the wanted functionality can be achieved easily: The 5Gs scrollwheel action is not transmitted the same way as the button presses (button presses are read via polling from button.c, scrollwheel action is transmitted via event). But it is possible to reduce the probability of this minor malfunction: "backlight_on()" should not be called on each _touch_ of the wheel, but if an scroll-event is generated. Doing so the user must at least move the finger over the wheel (4 of 96 sensors)...

Yes, this is definitely not correct. I stumbled upon this too when doing the scrollwheel driver for the fuze. I ended up calling backlight_on() too (but only when the scrollevent is posted), however it didn't really feel right.

A fix would be to take the "- returning BUTTON_SCROLL_BACK/BUTTON_SCROLL_FWD out of scrollwheel driver (which was missing before)" of  FS#8668  and handle backlight like the other buttons do.

Kugel, implementing BUTTON_SCROLL/BACK/BUTTON_SCROLL_FWD in the scrollwheel driver not a good idea. Reason is that the scrollwheel drivers (iPod and e200) use accelerated wheel scrolling and send additional information in the event. So, either the event handling should be totally removed and another way should be found to transfer all the needed data. Or we just leave it that way - the failure is not too obtrusive. Once I had implemented BUTTON_SCROLL/BACK/BUTTON_SCROLL_FW in the iPod scrollwheel driver in addition to the event solution (r17470). I had to remove it after a very short while because afterwards scrolling was sent via two ways -- some debug menus (I remember the audio buffer debug screen) went nuts.
This is why  FS#8668  does not implemement such handling, too. It does only use kind of a new trigger BUTTON_SCROLL which is not used for scrolling but only for signalling a user interaction for the gui boost.

My approach: Just moving "backlight_on()" to the section where scroll events are sent.

Just to be clear, a proper implementation of this would allow someone to adjust volume through scrolling without having to turn on the backlight (and now that it's a feature, the lcd screen as well), yes? I definitely didn't give this function a second thought until the lcd sleep stuff came up... I almost always adjust volume through my pocket, so there's never a real point in being able to see the screen when I'm doing that. I could probably save a decent amount of battery with this working the way I believe it usually works, since my screen would almost never be on.

If I'm totally wrong about what this option does, definitely clue me in. Thanks.

Short update to my post from above:
1st approach: Just moving "backlight_on()" to the section where scroll events are sent.
2nd approach: Using the implementation from  FS#8668  and removing "backlight_on()" from the scrollwheel drivers. This cleans up the drivers as it uses the target independent way of backlight handling.

Glenn, as a quick test you may use  FS#8668  (what you already do as far as I understood) and additionally comment out "backlight_on()" in firmware/target/arm/ipod/button-clickwheel.c This equals approach 2.

It's probably worth saying that (if I'm remembering correctly), the ipod scrollwheel driver was implemented before the "first keypress enables backlight only" feature. and making a touch of the wheel turn on the backlight was intentional - this is a useful way to turn on the backlight without performing any action. Also, IIUC the only ipods that are truely invisible when the backlight is off is the 5g with that option enabled.

But I also agree that power-conscious users would like to be able to use the wheel to change volume without turning on the backlight/lcd on. (I'm not a power-conscious user...)

Maybe the available options could be:

1) Never ignoring keypresses, but a touch of the wheel will turn on the backlight (current behaviour with the "first keypress..." set to off)
2) Touching or scrolling the wheel doesn't turn on backlight, and first keypress is ignored.

If I'm understanding the discussion here correctly, the intention is to remove option 1) ?

Hmm, the question is whether we want to wakeup backlight on scrollwheel activity or not.

a) We do not want it. Then we just need to remove "backlight_on()" from button-clickwheel.c In this case the implementation in button.c should wake up the backlight on all other button presses (except scroll wheel). The scroll wheel is used for scrolling or volume change (depends on context, e.g. WPS or menu tree). So, we also will not wake up the backlight, if scrolling in the database or menu.
b) We do want it. Then we either keep it as it is (touching wakes up backlight) or we change the code to only wake up the backlight if a scroll-activity is detected.
b1) Keep it as it is.
b2) Just move "backlight_on()" into the code section where the scroll event is sent.
b3) Remove "backlight_on()" from button-clickwheel.c and use the BUTTON_SCROLL-signalling from  FS#8668  instead. This will use the platform code in button.c to enable backlight.

I personally do not like option a) because of the context dependent function of the scrollwheel -- even if I am one of the power-conscious users ;-)

Buschel, I know that GUI boost implements BUTTON_SCROLL, rather than SCROLL_FWD and _BACK, to not issue doubled scrolling events.
But I still think that the backlight handling should not be separated from the rest of the buttons, but rather be the same. That's why I'm still for b3).

Btw, the same applies for the buttonlight.

Kugel, ok. Then I just misunderstood your posting :-)

My favorite is b3), too. It removes the ugly backlight-calls from the driver and uses the target independent code for backlight handling. If we agree to this solution I will provide a patch here.

Just to clarify, while I am for scrollwheel activity not turning on the backlight in the situation I mentioned, that thought was definitely meant to be coupled with the buttonpress enabling backlight option. If I'm actually sitting around using my player actively, touching the scrollwheel to enable the backlight is perfectly natural (and works the same as the OF. The idea in my head was definitely to make scrollwheel activity configurable to be (or not be) a backlight activity. If this is deemed as too much work for one target, then I know I personally probably won't be using the option in the first place.

In short (hopefully)...
1) the feature is fixed, and the option to consider scrollwheel activity as a buttonpress is part of the configuration: I'd use that. Reflecting on it after seeing everyone else's comments makes me think it's probably an unnecessary feature, though.
2) The feature is fixed, but it is now always set so that scrollwheel activity does not turn on the backlight: I would not use that. It's definitely a feature I appreciate when I'm actually looking at my player. If this was the current thinking on this issue, I'd definitely go with one of the B options instead.

In the end, it's sort of a semantics issue: it was just my thinking that the option is "first keypress enables backlight only", while scrolling isn't really a "press" per se. Like I said though, this is really the only situation in which I imagined it would not turn on the backlight. Hope that's clear, and if this feature gets the ax instead (for whatever reason) I won't really mind. Just thinking about saving more battery life.

Well, the "first keypress enables backlight only" doesn't only apply to the scrollwheel, but also to the center button. Sure one could argue this feature doesn't fit so well to the touchwheel ipods (or even scrollwheel in general), but there's also other buttons besides the wheel, where it always fits.

Yep, I guess this feature was planned for non-scrollwheel targets. With button-only targets it is reasonable to wake up the backlight/lcd with the first hit to be able to see what is going on before hitting any button then. For scrollwheel this is hard to implement because of the two-way communication (events from driver + button.c). Another additional option would be to remove the setting if HAVE_SCROLLWHEEL is defined, this would also reduce the binsize a bit -- so, b3) + remove setting.

Why would that be an option? That mean I can't switch on the light on my e200 anymore without making an action?

It becomes complicated ;-)
As far as I understood the e200 also calls "backlight_on()" in the wheel driver, but not on touching (like iPod) but on detected scrolling. Does this work that good that there is no misdetection of scrolling when pressing a button? If so, I understand your reject of my thoughts. Otherwise (like with the current iPod wheel driver) the backlight is switched on anyway as soon as pressing a button (except the center button).

The e200 has some more buttons that are not (on the) wheel than the ipod.
I don't turn the light on using the wheel, I mostly use the center button. And this option is one of the first ones that I enable after installation.

On the e200, you can press all buttons without touching the wheel. But you also can manage to touch the wheel just slightly, before pressing anything (which would equal the the ipods situation). But that doesn't turn the backlight on, as of now,

Hmm, again the iPod is in a special situation here... Now your use case is clear. So, forget about my last guess. Just b3) then :-)

I'm not entirely sure where the problem with b3) is.

What would change?
-The bug reported here would be fixed, which is good.
-You cannot turn on the light just by very slightly touching the wheel anymore. BUT you only need to touch it a little bit more (so that it generates a scroll). I can't imagine that this would be much more touching. That isn't so bad, is it?
-Bring consistency between targets.

There is no problem with b3). It will be better than the current solution because of the reasons discussed above and summed up in your last post.

Btw, this is quite near to chatting ;-)

Which isn't bad afterall, is it? :)

I enjoyed it! :)

Wow, lots of discussion :)
I may be a bit late, but anyways, here are my thoughts:

I want a way to turn on the backlight without changing anything else. Wheel touching is great for this. If it didn't turn on the backlight, what other options are there? I guess just the hold switch, and that's not very convenient. (On my Archos V2 Recorder I hold F1, but all iPod buttons do things when held.)

I don't like "First Keypress Enables Backlight Only" because then I need to know backlight state. For example, if I want to go to the next track without looking, do I press once and maybe only turn on the backlight or press twice and maybe skip an additional track? If the purpose of the option is turning on the backlight without causing other effects, then wheel touching makes it unnecessary.

That's one more voice for the b)-options. As dreamlayers wrote this bug-report b1) is out. b2) is out because it is more or less the ugly version of b3). I will post a patch tomorrow (or tonight if I cannot find sleep;).

I'm only in favour of b3 if the current behaviour is retained when "First Keypress Enables Backlight Only" is set to "No". (For reasons why, see my previous post.)

It may be best to just remove the "First Keypress Enables Backlight Only" option on click wheel iPods. What is the purpose of that option if touching the wheel can turn on the backlight without any other effects?

The only useful implementation may be making the wheel not turn on the backlight when the option is set to "Yes". I'm not sure if this is a good idea because it is different from other targets and it might be annoying when using the wheel to scroll. I'm attaching a patch nevertheless, to see what others think of this idea.

b3) would make you need to touch the wheel slightly more in order to get the backlight on (including emitting a scroll event) if it's turned off. It would not emit the scroll event if it's turned on.
And no, there's not only the hold switch, but also the center button which can be used to turn the wheel on (with, or without emitting an action).

I don't quite understand your point. Why did you fill a bug report in the first place, if you expect (or want) the current behavior?

He might have filed it because I noticed it was broken. He pretty much mirrors what I was trying to say, the only reason I was interested in the option was to be able to change volume without activating the screen when the option was enabled. If that doesn't seem like a good idea, then I don't see how necessary the option is, either.

Your wish has absolutely nothing to do with this option nor with this bug.

Yeah, I submitted the bug report because Glenn reported the problem at http://www.rockbox.org/tracker/task/9890#comment28460 and I confirmed that the same behaviour exists without  FS#9890 . I never actually needed "First Keypress Enables Backlight Only" behaviour; I just thought this was a problem which should be documented here and fixed.

In retrospect, I guess I shouldn't have submitted this bug report. Perhaps I wasted others time with something unimportant. I'm sorry.

Well, to me, this is certainly a bug. What Glenn demands is something entirely different, anyway. (edit) He's searching for something like http://www.rockbox.org/tracker/task/8400 or its related tasks.

No, it's not really a wish. I just noticed the functionality was broken. The volume changing thing is pure opinion, and does have nothing to do with this bug. I'm not demanding anything, I was just confused as to what the proper behavior was supposed to be. Sorry if the way I discussed it sounded like I was making demands, I thought I was being more clear that it didn't matter to me either way. I just wanted to help out by pointing out the bug. Sorry Thomas.

Also, thank you for the link to the other task... if it's not deemed appropriate for SVN then I'll forego it. It's not important enough for me to work with just to save a bit of battery life. I appreciate it, though :)

I would agree that the behaviour when the "First keypress..." option is enabled is a bug - so thank you Boris for documenting it.

As for the solution, I guess I 'm in favour of simply removing this option on the ipods where this happens.

Just to be able to test it I have attached the option b3) as patch. I must say after short testing that I dislike the "correct" behaviour because with the patch it is not possible anymore to wake up the backlight/LCD via a short touch of the wheel. Maybe it is really just better to keep the code as it is.

For what it's worth, I agree with that... having used the patch, the inability to wakeup again with a scrollwheel press was actually something I missed (so I guess in the end I wouldn't be really interested in FS#8400 anyway, probably).

So, why not just disable this option on affected targets? There's some agreement that the current behaviour is a bug, so it will probably seem that way to others who encounter it in the future.

Well, I'm not opposed to that. I can imagine that the current (buggy) behavior is wanted on the iPods.

Boris, agreed. Then I would propose to use a new "HAVE_"-define to encapsulate all the relevant code. Currently the "First keypress..."-setting is valid for all targets with HAVE_BACKLIGHT defined.