FS#12338 - Option to restart running sleep timer on keypress

Attached to Project: Rockbox
Opened by Nick Peskett (nickp) - Tuesday, 18 October 2011, 03:39 GMT
Last edited by Nick Peskett (nickp) - Monday, 26 December 2011, 09:34 GMT
Task Type Patches
Category Settings
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.9
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


If set and a sleep timer is running, the sleep timer is restarted with the value of the "sleeptimer duration" setting each time a key is pressed.
This task depends upon

Closed by  Nick Peskett (nickp)
Monday, 26 December 2011, 09:34 GMT
Reason for closing:  Accepted
Additional comments about closing:  r31437
Comment by Nick Peskett (nickp) - Tuesday, 18 October 2011, 03:49 GMT
This patch relies on >= r30777.
Comment by Martin Sägmüller (dfkt) - Tuesday, 18 October 2011, 08:31 GMT
Wow, thank you for that patch, Nick. Being aware of each and any key press looks like the most universal approach to reset the sleep timer.
Comment by Nick Peskett (nickp) - Tuesday, 18 October 2011, 08:49 GMT
I should add, as this patch currently piggy-backs on the poweroff_timer code (idle poweroff?), volume up/ down and scrolling up/ down lists seem to be ignored, but not in/ out of options or anything else I tried.

I think this was probably done to stop the interface becoming sluggish, and if so, seemed a good idea.
Comment by Nick Peskett (nickp) - Wednesday, 26 October 2011, 08:39 GMT
I was wrong about volume and menu up/down being exceptions, I was testing with a Sansa e200 and it turns out the scroll wheel was activating the back light, but not resetting the idle state. It was only after testing on a Clip+ I realised all buttons should restart the timer.

Here's an update with a fix to the e200 scroll wheel code.

It now also includes a manual entry.
Comment by Nick Peskett (nickp) - Thursday, 27 October 2011, 04:26 GMT
I've split out the e200 fix and reported it as a bug;

Updated revision, purely implementing the restarting the sleep timer on keypress.

If you've already applied this patch, there have been no functional changes since the first version.
Comment by Nick Peskett (nickp) - Thursday, 08 December 2011, 09:53 GMT
Here's a version which makes it the default behaviour (no setting to enable/ disable it).
As discussed on IRC;
Comment by Alexander Levin (fml2) - Wednesday, 14 December 2011, 16:00 GMT
I think the code in the firmware layer should not access any app level data. In this patch, however, just that happens since "global_setting" lives in the app layer. A more correct (and less pragmatic) solution would be to have a (static) var at the firmware level which would be set every time the global_setting changes. The code in powermgmt.c would only use that static var.
Comment by Nick Peskett (nickp) - Thursday, 15 December 2011, 11:06 GMT
Thanks for the advice Alexander.

Here's an updated couple of patches; the first one applies the restart by default, the second as an option.
Comment by Alexander Levin (fml2) - Thursday, 15 December 2011, 13:33 GMT
Hello Nick.

You did exactly what I proposed, thank you! I have some notes:

1. It would be good if the new variable (sleeptimer_duration) would be commented, specifically, in what units it is (I think, it's seconds).
2. In "set_sleep_timer", the variable can be set unconditionally, i.e. the assignment can be pukked out of if-then-else.

As to the setting... I can't comment on that but has anything changed since the last discussion? I.e. do we need it?
Comment by Nick Peskett (nickp) - Thursday, 15 December 2011, 13:51 GMT
Good idea about the commenting, I'm a bit tied up but will add that soon.

Not sure what you mean about 2, sleeptimer_duration is just keeping track of what the sleep timer was last instantiated with so it can be reinstated on keypress, or am I missing the point?

I plan to put the question re a setting to the dev list soon. Rasher wasn't keen on it being a default, there may be others;

I think if another sleep timer setting is added, there ought to be a dedicated sub-menu under settings, rather than squatting in time&date (not sure what its got to do with t&d anyway).

As it is, I think there's a case for a dedicated sub-menu, if only to stop polluting the root settings menu with the couple of existing sleep timer options for non-RTC targets.
Comment by Alexander Levin (fml2) - Thursday, 15 December 2011, 17:27 GMT
Re 1: I mean that you do not need to write "sleeptimer_duration = seconds;" and "sleeptimer_duration = 0;" in the branches, but can just write "sleeptimer_duration = seconds;" parallel to the if.
Comment by Nick Peskett (nickp) - Friday, 16 December 2011, 03:42 GMT
Seems obvious now, silly me! Thanks for your patience.
Comment by Nick Peskett (nickp) - Monday, 26 December 2011, 09:25 GMT
Moved to the new General Settings > Startup/Shutdown menu, prior to committing.