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

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

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

Attached to Project: Rockbox
Opened by Nick Peskett (nickp) - Tuesday, 18 October 2011, 05:39 GMT+2
Last edited by Nick Peskett (nickp) - Monday, 26 December 2011, 10:34 GMT+2

Task Type	Patches
Category	Settings
Status	Closed
Assigned To	No-one
Player Type	All players
Severity	Low
Priority	Normal
Reported Version	Release 3.9
Due in Version	Undecided
Due Date	Undecided
Percent Complete
Private	No

Details

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.

keypress_resets_sleep_timer_30780.patch (4.4 KiB)

 apps/lang/english.lang                  |   14 ++++++++++++++
 apps/settings.h                         |    1 +
 apps/menus/main_menu.c                  |    4 +++-
 apps/menus/time_menu.c                  |    4 +++-
 apps/settings_list.c                    |    2 ++
 firmware/powermgmt.c                    |    3 +++
 manual/appendix/config_file_options.tex |    1 +
 7 files changed, 27 insertions(+), 2 deletions(-)

This task depends upon

Closed by Nick Peskett (nickp)
Monday, 26 December 2011, 10:34 GMT+2
Reason for closing: Accepted
Additional comments about closing: r31437

Comments (13)
Related Tasks (0/0)

Comment by Nick Peskett (nickp) - Tuesday, 18 October 2011, 05:49 GMT+2

This patch relies on >= r30777.

Comment by Martin Sägmüller (dfkt) - Tuesday, 18 October 2011, 10:31 GMT+2

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, 10:49 GMT+2

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, 10:39 GMT+2

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.

keypress_resets_sleep_timer_30835.patch (5.7 KiB)

 apps/lang/english.lang                          |   14 ++++++++++++++
 apps/settings.h                                 |    1 +
 apps/menus/main_menu.c                          |    4 +++-
 apps/menus/time_menu.c                          |    4 +++-
 apps/settings_list.c                            |    2 ++
 firmware/target/arm/as3525/scrollwheel-as3525.c |    2 ++
 firmware/powermgmt.c                            |    3 +++
 manual/appendix/config_file_options.tex         |    1 +
 manual/configure_rockbox/sleep_timer.tex        |    3 +++
 9 files changed, 32 insertions(+), 2 deletions(-)

Comment by Nick Peskett (nickp) - Thursday, 27 October 2011, 06:26 GMT+2

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.

keypress_resets_sleep_timer_30842.patch (5 KiB)

 apps/lang/english.lang                   |   14 ++++++++++++++
 apps/settings.h                          |    1 +
 apps/menus/main_menu.c                   |    4 +++-
 apps/menus/time_menu.c                   |    4 +++-
 apps/settings_list.c                     |    2 ++
 firmware/powermgmt.c                     |    3 +++
 manual/appendix/config_file_options.tex  |    1 +
 manual/configure_rockbox/sleep_timer.tex |    3 +++
 8 files changed, 30 insertions(+), 2 deletions(-)

Comment by Nick Peskett (nickp) - Thursday, 08 December 2011, 10:53 GMT+2

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

keypress_resets_sleep_timer_nooption_31177.patch (0.6 KiB)

 firmware/powermgmt.c |    3 +++
 1 file changed, 3 insertions(+)

Comment by Alexander Levin (fml2) - Wednesday, 14 December 2011, 17:00 GMT+2

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, 12:06 GMT+2

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.

keypress_resets_sleep_timer_nosetting_31266.patch (0.9 KiB)

 firmware/powermgmt.c |    5 +++++
 1 file changed, 5 insertions(+)

keypress_resets_sleep_timer_31266.patch (6.4 KiB)

 apps/lang/english.lang                   |   14 ++++++++++++++
 apps/settings.c                          |    2 ++
 apps/settings.h                          |    1 +
 apps/menus/main_menu.c                   |    4 +++-
 apps/menus/time_menu.c                   |    4 +++-
 apps/settings_list.c                     |    2 ++
 firmware/export/powermgmt.h              |    1 +
 firmware/powermgmt.c                     |   11 +++++++++++
 manual/appendix/config_file_options.tex  |    1 +
 manual/configure_rockbox/sleep_timer.tex |    3 +++
 10 files changed, 41 insertions(+), 2 deletions(-)

Comment by Alexander Levin (fml2) - Thursday, 15 December 2011, 14:33 GMT+2

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, 14:51 GMT+2

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.

Comment by Alexander Levin (fml2) - Thursday, 15 December 2011, 18:27 GMT+2

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, 04:42 GMT+2

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

keypress_resets_sleep_timer_nosetting_31320.patch (0.8 KiB)

 firmware/powermgmt.c |    5 +++++
 1 file changed, 5 insertions(+)

keypress_resets_sleep_timer_31320.patch (6.4 KiB)

 apps/settings.c                          |    2 ++
 apps/lang/english.lang                   |   14 ++++++++++++++
 apps/settings.h                          |    1 +
 apps/menus/main_menu.c                   |    4 +++-
 apps/menus/time_menu.c                   |    4 +++-
 apps/settings_list.c                     |    2 ++
 firmware/export/powermgmt.h              |    1 +
 firmware/powermgmt.c                     |   12 ++++++++++++
 manual/appendix/config_file_options.tex  |    1 +
 manual/configure_rockbox/sleep_timer.tex |    3 +++
 10 files changed, 42 insertions(+), 2 deletions(-)

Comment by Nick Peskett (nickp) - Monday, 26 December 2011, 10:25 GMT+2

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

keypress_resets_sleep_timer_31435.patch (5.9 KiB)

 apps/settings.c                                       |    2 ++
 apps/lang/english.lang                                |   14 ++++++++++++++
 apps/settings.h                                       |    1 +
 apps/menus/settings_menu.c                            |    5 ++++-
 apps/settings_list.c                                  |    2 ++
 firmware/export/powermgmt.h                           |    1 +
 firmware/powermgmt.c                                  |   12 ++++++++++++
 manual/appendix/config_file_options.tex               |    1 +
 manual/configure_rockbox/startup_shutdown_options.tex |    3 +++
 9 files changed, 40 insertions(+), 1 deletion(-)

	Tasks related to this task (0)

Duplicate tasks of this task (0)

Rockbox

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

Details

Loading...