Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Settings
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.9
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by nickp - 2011-10-18
Last edited by nickp - 2011-12-26

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

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.

Closed by  nickp
2011-12-26 09:34
Reason for closing:  Accepted
Additional comments about closing:  

r31437

nickp commented on 2011-10-18 03:49

This patch relies on >= r30777.

dfkt commented on 2011-10-18 08:31

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.

nickp commented on 2011-10-18 08:49

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.

nickp commented on 2011-10-26 08:39

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.

nickp commented on 2011-10-27 04:26

I’ve split out the e200 fix and reported it as a bug;

http://www.rockbox.org/tracker/task/12351

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.

nickp commented on 2011-12-08 09:53

Here’s a version which makes it the default behaviour (no setting to enable/ disable it).
As discussed on IRC;
http://www.rockbox.org/irc/log-20111208#10:12:43

fml2 commented on 2011-12-14 16:00

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.

nickp commented on 2011-12-15 11:06

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.

fml2 commented on 2011-12-15 13:33

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?

nickp commented on 2011-12-15 13:51

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;

http://www.rockbox.org/irc/log-20111208#10:40:52

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.

fml2 commented on 2011-12-15 17:27

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.

nickp commented on 2011-12-16 03:42

Seems obvious now, silly me! Thanks for your patience.

nickp commented on 2011-12-26 09:25

Moved to the new General Settings > Startup/Shutdown menu, prior to committing.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing