FS#10849 - Sleep timer options: persistent duration and start on boot.

Attached to Project: Rockbox
Opened by Nick Peskett (nickp) - Sunday, 13 December 2009, 09:18 GMT
Last edited by Thomas Martitz (kugel.) - Tuesday, 18 October 2011, 19:08 GMT
Task Type Patches
Category Settings
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


[EDIT: More accurate description of latest patch posted in comments section, requested by nickp]

This patch adds two sleep timer options and a sub-menu to hold them (Settings > General Settings > System > Sleep Timer).

The two options are;

Sleep Timer Duration: Allows the value to be set permanently (as 'sleeptimer duration' in the config file). The value will be used until reset to a new value.

Start Sleep Timer On Boot: Whether you would like a sleep timer to automatically be started when the device boots up (stored in the config file as 'sleeptimer on startup').

These options are particularly useful for users who have a dedicated DAP by their bedside, or a specific config file used when sleeping. When the later is set, powering the device up will play for the users specified duration, then switch the device off.

The existing (System > (Date & Time) > Sleep Timer) option has been modified to be a switch showing "Start Sleep Timer (duration)" when inactive, or "Stop Sleep Timer (remaining)" when active. Duration and remaining being the current setting and remaining minutes left on the timer, respectively.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Tuesday, 18 October 2011, 19:08 GMT
Reason for closing:  Accepted
Additional comments about closing:  r30777. Thank you for this.
Comment by Kai Posadowsky (Riffer) - Tuesday, 16 February 2010, 23:12 GMT
Just an update to the above patch for revision 24705 - hope I did right, its my first try to add something like that.
Comment by Kai Posadowsky (Riffer) - Tuesday, 16 February 2010, 23:40 GMT
As I see now the file apps/menus/sleeptimer_menu.c is missing.
I made this patch by "svn diff >sleep_timer_menu.patch".
Which option brings svn diff to include the a new file?
Comment by Kai Posadowsky (Riffer) - Tuesday, 16 February 2010, 23:46 GMT
Ok - found out - had to add the file to the svn itself - here is the patch, finally.
Comment by Nick Peskett (nickp) - Monday, 10 May 2010, 13:31 GMT
Synced to r25926.
Comment by Nick Peskett (nickp) - Tuesday, 18 May 2010, 13:29 GMT
Synced to r26142.
Comment by Nick Peskett (nickp) - Friday, 06 August 2010, 13:18 GMT
Synced to r27736.
Comment by Nick Peskett (nickp) - Thursday, 26 August 2010, 15:25 GMT
If you find this patch interesting, you should also check out;
Comment by alex wallis (alexwallis646) - Friday, 27 August 2010, 13:11 GMT
just curious, but if fs#11042 is a more sophisticated version of this patch, why not close this task? or are there differences between this task and fs#11042
Comment by Nick Peskett (nickp) - Saturday, 28 August 2010, 00:59 GMT
While  FS#11042  has more features, at this point I'm not convinced it is more sophisticated.

This is mainly due to to the unsolved issue (which you have also commented on) regarding cancelling of timers when pause is pressed in wps, and the timer not being reinstated unless sleeptimer_on_startup is set.

Depending on this getting resolved, I'll either request this is closed or import the uisimulator improvements into this patch.
Comment by Nick Peskett (nickp) - Thursday, 02 September 2010, 23:15 GMT
Sleep Timer menu moved to /Settings/General Settings/System/Sleep Timer.
Comment by Nick Peskett (nickp) - Saturday, 16 October 2010, 22:52 GMT
Synced to r28291.
Updated manual removal of wake-up alarm warning from r28263.
Comment by Nick Peskett (nickp) - Monday, 29 November 2010, 05:07 GMT
Synced to r28701.
Comment by Nick Peskett (nickp) - Saturday, 29 January 2011, 06:36 GMT
Synced to r29159.
Comment by Nick Peskett (nickp) - Thursday, 03 February 2011, 07:13 GMT
Synced to r29198.
Comment by Nick Peskett (nickp) - Monday, 28 February 2011, 08:18 GMT
Synced to 3.8 (r29442).
Comment by Nick Peskett (nickp) - Tuesday, 01 March 2011, 02:14 GMT
Synced to r29474.
Got rid of redundant uisimulator/common/stubs.c code, it seems something very similar is now in trunk.
Comment by Martin Sägmüller (dfkt) - Wednesday, 27 April 2011, 11:47 GMT
Could someone please re-sync this patch? With r29788 it appears to not compile anymore.
Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 12:18 GMT
Hi dfkt,

It's working for me after an svn up to r29788.

Could you try the following in your rockbox checked out root and attach /tmp/reject.txt?

wget -O /tmp/sleeptimer_submenu_29474.patch
patch -p0 -i /tmp/sleeptimer_submenu_29474.patch --global-reject-file=/tmp/reject.txt
svn info >> /tmp/reject.txt

Maybe another patch is conflicting with it?

Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 12:33 GMT
Ah sorry, you said compile not apply patch.

I just tried a compile for the Clip+ and it seems to run fine on the device - what errors are you getting?
Comment by Martin Sägmüller (dfkt) - Wednesday, 27 April 2011, 13:00 GMT
Hello Nick,

Thanks for your fast reply. I tried the patch on a clean source, no other patches applied. Applying the patch normally (patch -p0 < sleeptimer_submenu_29474.patch) gives no error. Trying your trouble shooting above gives no reject.txt. However, when actually compiling a clean source with the patch, it spits out hundreds of errors.

Here's the last few lines of the terminal buffer, after trying make: - there's hundreds more errors like these.

Applying any other patches to the same clean source gives no issues.

I'm trying to build it for a Clip+, on Ubuntu 10, by the way.

Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 13:11 GMT
Thanks Martin.

Are you sure it isn't  FS#11042  you're applying? I only ask because menu_sleeptimer_pause_mode only exists there, not in this patch (which just powers off the device when the timer completes). That patch is pretty neglected nowadays.

If not I'll have a go downloading and installing a Ubuntu 10 over the weekend, I'm still on an ancient 8.04 server here...
Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 13:25 GMT
After applying the patch, hopefully "sed -n 93,94p apps/menus/sleeptimer_menu.c" at the command line should return;

&menu_toggle_sleeptimer, &menu_sleeptimer_duration, &menu_sleeptimer_on_startup);
Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 13:41 GMT
I just noticed, you're getting errors way beyond the end of the file;

/home/dfkt/rockbox-clean/apps/menus/sleeptimer_menu.c:649: error: redefinition of ‘menu_toggle_sleeptimer’

apps/menus/sleeptimer_menu.c should only have 94 lines - I think there's something odd going on there...

Try "wc -l apps/menus/sleeptimer_menu.c"

Comment by Martin Sägmüller (dfkt) - Wednesday, 27 April 2011, 15:25 GMT
Nick, we're getting closer... :)

I re-downloaded the patch from the site, to make sure it's not  FS#11042  I wrongly renamed or similar. It was the correct patch I used.

The "sed" command indeed does return the lines you specified. The "wc" command returns "848 apps/menus/sleeptimer_menu.c". I checked that file and it indeed contains 848 lines, several slightly different copies one after each other.

I did a "svn revert -R ." after that, and applied the patch again, normally. This is what sleeptimer_menu.c looks like after applying the patch: - one too many copies.
Comment by Martin Sägmüller (dfkt) - Wednesday, 27 April 2011, 18:25 GMT
Might this be causing issues with the patch, on some computers?

--- apps/menus/sleeptimer_menu.c (revision 0)
+++ apps/menus/sleeptimer_menu.c (revision 0)
@@ -0,0 +1,94 @@
Comment by Nick Peskett (nickp) - Wednesday, 27 April 2011, 22:14 GMT
This patch adds sleeptimer_menu.c, so it shouldn't exist in a pristine checkout of the rockbox trunk (that's why it's revision 0 - not yet checked in).

I think the root of the problem is the revert doesn't seem to be getting rid of the file/ maybe other stuff before reapplying it.

A quick hack might be to just "rm apps/menus/sleeptimer_menu.c" before you start.

I personally always keep a pristine checkout in one directory (rockbox_orig), then if I want to apply any patches do a "svn up rockbox_orig/ && cp -a rockbox_orig rockbox && cd rockbox" before I start, then "cd .. && rm -rf rockbox" after I've finished.

Could you face a fresh checkout of rockbox? I know it's quite a hit to take but I think your current one might have become a bit corrupted.
Comment by Martin Sägmüller (dfkt) - Sunday, 01 May 2011, 00:10 GMT
Nick, I finally founds some time to try your suggestions. I checked out a fresh version in a new folder, and the sleep timer patch indeed applied and compiled without issues.

After successfully compiling a build with the sleep timer patch, I did a "svn revert -R ." on the new source. Lo and behold, sleeptimer_menu.c didn't get removed by that, and reapplying the patch to the presumably "clean" source led to sleeptimer_menu.c becoming filled with duplicates. I didn't have that behavior with any other patch yet, but at least now I know that "svn revert -R ." doesn't fully clean up the source with this one patch. I wonder if anyone else ever had those issues, or why I'm having them...

Thanks for your help, Nick - highly appreciated. I'm happy I can use your sleep timer again in my builds.
Comment by Martin Sägmüller (dfkt) - Sunday, 01 May 2011, 00:12 GMT
I forgot to mention, "svn diff" didn't show any clues as well, that sleeptimer_menu.c was still present after reverting all changes to svn.
Comment by Nick Peskett (nickp) - Sunday, 01 May 2011, 02:10 GMT
Thanks for persevering and keeping me updated Martin.

It all makes sense now, even though you wrote "svn revert -R .", for some reason I was thinking you were doing a patch -R.

Due to sleeptimer_menu.c not being in the repository the code was checked out from (the patch creates it from scratch), it's necessary for me to do a "svn add apps/menus/sleeptimer_menu.c" before svn diff to create the patch. Otherwise (as you found) svn diff doesn't even mention the file.

This means when you checkout from trunk, svn knows nothing about sleeptimer_menu.c, so doesn't bother reverting it. The next time you apply the patch, it would apply the change to line 0 of the sleeptimer_menu.c, leaving the previous change at the end.

I think even patch -R would leave an empty file rather than deleting it, not ideal.

I just checked svn help revert, there doesn't even seem to be an option to delete unknown files... Oh well.
Comment by Martin Sägmüller (dfkt) - Saturday, 07 May 2011, 20:32 GMT
I forgot to mention, "svn diff" didn't show any clues as well, that sleeptimer_menu.c was still present after reverting all changes to svn.
Comment by Martin Sägmüller (dfkt) - Saturday, 07 May 2011, 20:34 GMT
Ooops, browser cache in my virtual machine messed up and resent the old post. Please delete. Sorry!
Comment by Miguel S (Hemingway) - Saturday, 02 July 2011, 22:05 GMT
Works great with rev.29474. My favorite patch; no idea, why this is not in standard Rockbox branch.
any chance to get this updated to 3.9 or current cvs rev.?
Comment by Nick Peskett (nickp) - Sunday, 03 July 2011, 00:06 GMT
Hi Hemingway,

I just tried it here, apart from a few offsets, the current patch seems to work with v3_9;

svn switch svn://
wget -O /tmp/sleeptimer_submenu_29474.patch
patch -p0 -i /tmp/sleeptimer_submenu_29474.patch

I tend to only update the patch when changes to the code base break the patch. Are you getting any failures? If so, could you post them?


Comment by Miguel S (Hemingway) - Sunday, 03 July 2011, 07:49 GMT
Hi Nick,

switched to v3_9, patched, recompiled and installed it on my Sansa Clip+:
works like a charm.

Comment by Nick Peskett (nickp) - Monday, 08 August 2011, 05:16 GMT
Synced to r30267.
Comment by Nick Peskett (nickp) - Tuesday, 09 August 2011, 10:12 GMT
Introduces menu option re-organisation discussed on IRC;
Comment by sideral (sideral) - Tuesday, 09 August 2011, 13:28 GMT
This looks fine to me. I like the new menu/settings structure. I'd like to see this committed.

I'll test this for a few days and then report back. Unless something surprising surfaces, it's time to brings this up on the developer's mailing list.
Comment by Nick Peskett (nickp) - Tuesday, 09 August 2011, 13:31 GMT
Thanks sideral, appreciated.
Comment by Martin Sägmüller (dfkt) - Tuesday, 09 August 2011, 15:12 GMT
Thanks for the improvements, nickp. I will try it ASAP in my next build.

Regarding the one nifty feature  FS#11042  has over this one (which obviously caused some issues) - may I ask if it would be possible/easier to add an option for "reset timer on *any* key press" instead of having it only linked with the pause function? Creative players stock firmwares have this feature, and it seems rather comfortable to me.
Comment by Nick Peskett (nickp) - Tuesday, 09 August 2011, 22:32 GMT
It's a feature I've be meaning to implement for ages, dfkt, but I'm not happy with how FS #11042 just intercepts a wps pause and cancels the current timer (only restarting if sleeptimer_on_startup is set).

I think the most elegant implementation might be to tap into the code which wakes the display on key press, refreshing the sleep timer if it's active. This might also be good to be optional as a setting too.

At this stage though, sideral and I think that the best chance to get this accepted is to introduce as few changes as possible initially, presenting new features as further patches.
Comment by sideral (sideral) - Saturday, 13 August 2011, 22:45 GMT
Some minor changes after reviewing this patch:

* Lang additions have to be at the end of english.lang
* Removed sleeptimer_menu.c to minimize diff; options live in settings_menu.c for the time being
* Moved sleep-timer settings next to idle-poweroff settings
* Some whitespace and formatting changes
* The formats using LANG_SLEEP_TIMER_START and LANG_SLEEP_TIMER_STOP needed to be made a part of the corresponding translation strings to cater for right-to-left languages.

Still missing:
[EDIT: nothing right now.]
Comment by Nick Peskett (nickp) - Saturday, 13 August 2011, 23:04 GMT
Thanks sideral.

Re LANG_SLEEP_TIMER_(START|STOP), do you think we could get away with (stripped for brevity)?

if lang_is_rtl()
"%s (%d:%02d)"
"(%02d:%d) %s"
Comment by Nick Peskett (nickp) - Saturday, 13 August 2011, 23:05 GMT
Ha ha, beat me to it!
Comment by Nick Peskett (nickp) - Wednesday, 17 August 2011, 10:26 GMT
sideral: I couldn't resist implementing Thomas Jarosch's suggested changes (thanks Thomas). I did this in the full knowledge that rockbox-dev opinion may sway in a different direction and the work would have to be done again ;). The idea was a good one though and curiosity got the better of me.

I think it turned out well, the patch is even smaller now and I think this actually adds functionality.
Comment by Nick Peskett (nickp) - Thursday, 25 August 2011, 23:05 GMT
After further discussions on IRC earlier and the rockbox-dev list, here's a new version.

All Sleep Timer entries are now all in System (> Time & Date)[if real time clock].

Existing sleep timer entry now also sets the persistent sleep timer value.

"Start sleep timer on boot" option added after "Alarm Wake up Screen" in same menu.
Comment by Nick Peskett (nickp) - Thursday, 22 September 2011, 04:51 GMT
sideral: Are you tied up with something else at the moment? Would you prefer I found somebody else to test and commit this?
Comment by Thomas Martitz (kugel.) - Friday, 07 October 2011, 19:38 GMT
I'm sorry you're waiting. I'll try to get this in.
Comment by Thomas Arendsen Hein (ThomasAH) - Saturday, 08 October 2011, 10:41 GMT
Thank you, kugel, that would be great!
Comment by Thomas Martitz (kugel.) - Sunday, 09 October 2011, 01:24 GMT
This is the patch series I would like to commit. It implements the proposal from the mailing list.

The third patch is based on sleeptimer_submenu_30355.patch, however I added that the sleep timer menu item transforms into "cancel running timer (XX min)" and cancels the current timer on selection (took me longer than I anticipated :\ )

Perhaps I split the last one for commit, but that's a minor topic.
Comment by Nick Peskett (nickp) - Sunday, 09 October 2011, 02:28 GMT
Looks good, thanks for picking this up kugel.
Comment by Nick Peskett (nickp) - Tuesday, 18 October 2011, 03:46 GMT
kugel: Thanks for submitting this, I really appreciate it.
dfkt: I just added another patch which adds a "restart running sleep timer on keypress" option:  FS#12338 . Sorry it took a while, I was holding off until the persistent sleep timer duration value was available.