Rockbox

Tasklist

FS#11042 - Dedicated sleep timer menu with persistent duration and "on power up" option - refined

Attached to Project: Rockbox
Opened by Kai Posadowsky (Riffer) - Saturday, 20 February 2010, 16:11 GMT
Last edited by Dominik Riebeling (bluebrother) - Thursday, 08 November 2012, 22:04 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

Details

This work is based on  FS#10849  from Nick Peskett and been inspired by FS#10754 from Sean Inglis.

This patch adds persistent duration values and a dedicated menu (under System) to control and hold the items.
It starts the sleeptimer when you start playing and stops it when you stop playing - if you want to have the player to shutdown you can easily get this by setting shutdown time in the standard settings menu!

The sleep timer menu has three items;

Start/ Stop Timer - This option starts (or stops if currently active) the sleep timer for this session. If a duration hasn't been set, this will be half an hour.

Timer Duration - The duration of sleep timers. The value set here is persistent, with its value being stored under "sleeptimer duration" in config.cfg.

Start Timer On Power Up? - Whether a sleep timer should be initiated when the device <del>starts up</del> plays - this regards to your setting. (If you have set the starting screen to wps it starts then too!). As described above the sleep timer also starts/stops when you press play/stop while playing . Stored in config.cfg under "sleeptimer on startup".

In addition the uisimulator has been largely extended by a small version of the original device power thread to have a much more realistic simulation when testing sleeptimer functions. Some additional debug messages have been added too.

The sys_poweroff function now really terminates the simulation (by sending SDL_QUIT to SDL).
This task depends upon

Closed by  Dominik Riebeling (bluebrother)
Thursday, 08 November 2012, 22:04 GMT
Reason for closing:  Out of Date
Additional comments about closing:  Closed on request of the reporter.
Comment by Kai Posadowsky (Riffer) - Saturday, 20 February 2010, 16:14 GMT
Sorry, forget to include apps/menus/sleeptimer_menu.c
Comment by Hayden Pearce (St.) - Sunday, 21 February 2010, 06:35 GMT
Just what I've been looking for, thanks!

I saw seani's "swap sleep timer with pause timer" thread, which in turn lead me here...
Good work, will apply to current SVN and test.


[St.]
Comment by Kai Posadowsky (Riffer) - Monday, 22 February 2010, 20:32 GMT
A line of code was missing. The sleeptimer did not really stop when pause is pressed while playing screen.
This is a complete patch as above with the fix for this issue.

Comment by Jonathan Gordon (jdgordon) - Monday, 22 February 2010, 21:47 GMT
remove tabs, // comments, commented out code that isnt being used etc.

the root_menu.c change is wrong, the timer should be started on boot regardless of which screen is the start screen.

Would these options make sense in the time/date submenu instead of system?
Comment by Sean Inglis (seani) - Monday, 22 February 2010, 21:50 GMT
Great stuff!

Can FS#10754 be removed now as it's happily redundant? (...wanders off to look...)
Comment by Jonathan Gordon (jdgordon) - Monday, 22 February 2010, 21:55 GMT
firmware/powermgmt.c is also wrong. that doesnt handle stopping the radio or recorder. audio_stop() should be used instead of audio_pause() anyway
Comment by Sean Inglis (seani) - Monday, 22 February 2010, 22:11 GMT
Not certain about radio / rec, but the idea of audio_pause() is that the idle timer turns it off anyway. That way you just hit play to continue within the idle period instead of a full powerup.
Comment by Kai Posadowsky (Riffer) - Monday, 22 February 2010, 23:58 GMT
What seani writes is right - the plan ist to just let do the idle timer do the job of powering off the dap.

The change in root_menu.c starts the sleeptimer only if one is using the wps as start screen.
That was my intention because I only use my dap as an audio book player when going to bed
(and while driving my car, but thats another thing).

The only job I have to do at night is to power up the dap and let the timer(s) do the rest. When sleep timer
runs out the idle timer is responsible to power off the dap (in my case a minute). When I am still awake
while the sleep timer runs out and pauses the player I just press play/pause and listen on. I know now
that some people expect the dap to turn off after the sleep timer but in the explained situation this
works best for me. And I understand that other people have different needs.

After talk in IRC I see that there should be some changes because of the radio. As jdgordon suggested
at IRC I will implement it like ("on and pause", "on and poweroff", "off") for the "enable at boot" setting.

I have no radio dap - could someone tell me wether the sleep timer or the idle timer shuts down when in radio mode?
It comes to my mind there is some code for it in the powermgmnt, but I would like to be sure.

What about moving the menu from "system" to "settings/system" - where "idle power off" menu is as well?
Its my feeling that they belong together as similar things do.
Comment by Nick Peskett (nickp) - Tuesday, 23 February 2010, 00:08 GMT
Riffer: When seani and I first talked about adding a dedicated sleep timer menu, the idea was to have the behaviour when the timer expired be configurable (power off or pause);

http://www.rockbox.org/tracker/task/10779?project=1&type=4&pagenum=18#comment33643

How do you feel about that? I prefer mine to power off now I've moved to a device with a very quick start-up time (Sansa e200v2).

While we're on the subject, could activation on startup/ activation on pressing play also be configurable?.

jdgorgan: I originally tried adding these options to the time/date submenu, but the clock display at the top was using too much screen space. In addition, with the if statement around whether there was a realtime clock or not, all the options would have had to be in two places.

Would a submenu from the regular settings be better?
Comment by Sean Inglis (seani) - Tuesday, 23 February 2010, 00:14 GMT
I'd certainly support a choice, but if only one makes the cut, I'd vote for pause; the C200 accompanies it's startup routine with a blinding blue LED flash that I wouldn't miss.
Comment by Kai Posadowsky (Riffer) - Tuesday, 23 February 2010, 07:08 GMT
nickp: This why I will make it configurable ("on and pause", "on and poweroff", "off") - Certainly there are different speed of power up. My methusalem Jukebox Recorder takes a significant time to power up, thats why I personally prefer "pause".
So for you and seani: "on and poweroff" will do the job for you, right?

Regarding the pause option for start/activation on play or powerup I had the same idea - could be an option, too?
Comment by Kai Posadowsky (Riffer) - Tuesday, 23 February 2010, 07:10 GMT
seani: missunderstood you. the option "on and pause" will do better - no more blinding flashes for your eyes...
Comment by Nick Peskett (nickp) - Tuesday, 23 February 2010, 07:17 GMT
Yes, sorry about that, your reply only appeared for me once I'd posted - we must have been typing around the same time.

Sounds good, I'm glad sleep timers have a new champion! It may just get accepted yet ;)
Comment by Kai Posadowsky (Riffer) - Tuesday, 23 February 2010, 13:55 GMT
After the discuss in irc yesterday evening I do not really feel like a champ but like a drongo ;-)
Its my first try to contribute at the source code level - only a very small part I can do for all this good years with rockbox!

Did you ever tried to let your sleep timer patch get accepted?
Comment by Nick Peskett (nickp) - Tuesday, 23 February 2010, 14:10 GMT
No, I went on IRC and let them know it existed, but didn't push it.

My mistake was thinking if I built a better mousetrap they'd beat a path to my door.
Comment by alex wallis (alexwallis646) - Tuesday, 13 July 2010, 18:33 GMT
Hi, could this please be fixed to work with the latest revision? As it appears to be out of date.
Comment by Kai Posadowsky (Riffer) - Thursday, 22 July 2010, 12:02 GMT
My failure. Every time I refill the adp of my mother I compile a new version and should have attached the actual version here, too.
Today evening, if nothing blocks me.
Comment by Kai Posadowsky (Riffer) - Thursday, 22 July 2010, 21:59 GMT
Another update (had a short test with Sansa V2 and SDL app, all works as expected)... wish I had time to implement what is long discussed above.
Comment by alex wallis (alexwallis646) - Sunday, 01 August 2010, 15:27 GMT
Hi, there is one slight problem with this patch, that is the first option under the sleepttimer menu isn't voiced.
please could this be corrected?
You land on the option as soon as you go into the sleeptimer menu. I am looking forward to testing this, and seeing the other functionality discussed above implemented when time allows.
Comment by Sean Inglis (seani) - Sunday, 01 August 2010, 15:46 GMT
To follow up, I've added this in a local build by adding this function to sleeptimer_menu.c

static int sleeptimer_speak_action(int selected_item, void * data)
{
(void)selected_item;
(void)data;

if (get_sleep_timer())
talk_id(LANG_SLEEP_TIMER_STOP, true);
else
talk_id(LANG_SLEEP_TIMER_START, true);

return 0;
}


And changing the menu entry definition to:

MENUITEM_FUNCTION_DYNTEXT(
menu_toggle_sleeptimer,
0,
toggle_sleeptimer,
NULL,
get_current_status,
sleeptimer_speak_action, NULL, NULL, Icon_NOICON);


Comment by alex wallis (alexwallis646) - Tuesday, 10 August 2010, 17:08 GMT
I have this patch running on a build, but I can't see an option called shutdown time in the settings menu.
Why have it under settings though anyway, perhaps all the sleeptimer options should be grouped together under system including shutdown time.

Also, have you considered adding in an off option as Sean Inglis does in his patch for removing the duration setting from the config file?
Comment by Sean Inglis (seani) - Sunday, 15 August 2010, 22:09 GMT
This amended patch should address the comment style, commented code, radio and recorder issues highlighted by JDG above.

It also addresses the issue with the initial start and stop time menu option not being voiced.

Finally, I've removed the final power_off() statement in favour of allowing the player to power off after the idle timeout matures. This allows the player to be instantly restarted for another bite of the timer without discarding the buffer contents or spinning the disc up again on startup (where applicable).

Although I'd advocate using pause as being the more flexible of the two options, if someone feels strongly enough to change it back, I won't be undoing that change again (at least not on here :-))
Comment by Frank Gevaerts (fg) - Sunday, 22 August 2010, 17:08 GMT
Some technical comments after skimming through the patch

* apps/lang/english.lang says "If you want to add a new string, add it to the end of this file!"
* You end up having sleep_timer_set() and set_sleep_timer(), one for minutes and one for seconds. I know they have different scope, but maybe the naming could be better, e.g. set_sleep_timer_minutes()?
* "extern void powermgmt_init(void);" in apps/main.c. If you need it for the sim, the declaration should be added to the proper header file. I also don't see why it's "extern"
* I'd really prefer the uisimulator extensions as a separate patch. They're useful on their own, and I don't really like commits (and therefore patches) that change more than one thing.

I have no real opinion on the desirability of the feature itself.
Comment by Sean Inglis (seani) - Tuesday, 24 August 2010, 23:13 GMT
@fg

Shifted the LANG entries to the end of the file

Corrected for extraneous extern in main.c

sleep_timer naming resolved


I haven't made any changes to the sim extensions as having to test on target is a pain; they do seem twinned with the main patch

Comment by Nick Peskett (nickp) - Wednesday, 25 August 2010, 23:34 GMT
seani: I think you're forgetting to do "svn add apps/menus/sleeptimer_menu.c" before your svn diff. Your apps/SOURCES continue to mention it, but it's missing.

I was thinking of adding a menu option to choose the behaviour when the timer finishes, is everyone OK with that? Maybe "Timer Behaviour: Power Off/ Pause"
Comment by Sean Inglis (seani) - Wednesday, 25 August 2010, 23:44 GMT
nickp: quite right, that's my svn ignorance showing, sorry.
Comment by Nick Peskett (nickp) - Wednesday, 25 August 2010, 23:56 GMT
I was also thinking of moving the sleep timer menu from "/System/Sleep Timer" to "/Settings/General Settings/System/Sleep Timer".

I was always thought "/System/Time & Date" wasn't particularly intuitive and after reading the following comment, now that the values are persistent maybe they belong in the "/Settings" hierarchy.

http://www.rockbox.org/irc/log-20100823#21:43:19

Any arguments for leaving it where it is or moving it somewhere else?
Comment by Sean Inglis (seani) - Thursday, 26 August 2010, 00:09 GMT
For my money, Sleep Timer and Idle Timer are a natural pairing so appearing on the same menu would make sense. I don't think it sitting under System is natural, but it's hard to tell as you become habituated to these little quirks.

I wonder if the Pause / Power Off behaviour should be folded into the Start / Stop timer option, rather than living separately?

So:

* Stop Timer
* Start TImer (Pause)
* Start Timer (Power)

or

* Stop Timer
* Start Pause TImer
* Start Power TImer

?
Comment by Nick Peskett (nickp) - Thursday, 26 August 2010, 00:20 GMT
I think most people will have a preference for one or the other behaviour, so having to choose which type they want each time they start a timer would just be an added hassle.

Besides, if the behaviour isn't persistent, we wouldn't know which type to initiate if "sleeptimer on startup" was set.

Also the menu option "Start TImer (x:xx)" is currently used to quickly show how long the timer will be.
Comment by Sean Inglis (seani) - Thursday, 26 August 2010, 00:53 GMT
I don't understand where the hassle comes into it. You'd persist the last choice you made to cover the case where the sleep timer restarts on power up. If you need to start the timer again, you *have* to visit that menu. All you'd see is an additional option. And you can still show the remaining time in both prompts.

In any case I don't see the need to distinguish and have a separate setting - what's gained by it?

At most, you save as much energy as is consumed by the player sitting idle for as long as the idle timer is set. I can't see why it's so important to have it power off immediately as opposed to pause for a few minutes and then power off - what use case demands it?

But I don't care enough to force the issue - I'll just remove the power_off in my own build.


Comment by Nick Peskett (nickp) - Thursday, 26 August 2010, 01:14 GMT
As far as a use case goes, the power button is easier to reach over and press than pause on a Sansa E200 ;)

I don't understand why you'd go to the trouble of removing power_off in your build though, why not just set Timer Behaviour to Pause? I could even make that the default, I only put power first as that's the current behaviour in Rockbox.

I feel two start timer options are more confusing than a single start timer and a behaviour option, but could be persuaded.
Comment by Sean Inglis (seani) - Thursday, 26 August 2010, 01:27 GMT
Hmm, I suppose pause is a bit close to the wheel. It's 50-50 on a C240.

I meant "in the event that only power off makes it in", I'll just amend my build - and it's one comment, I can handle it :-)

My thinking is: how does needing more config. settings effect the patches chance of acceptance? If adding more config options slows acceptance, I'd happily fall on my sword and just make the simple change myself. Having one less full patch to synch to trunk when I build is worth it.

If an option gets added, it wouldn't matter to me where it was personally - it'll get set and then almost never change.
Comment by alex wallis (alexwallis646) - Thursday, 26 August 2010, 13:04 GMT
Personally I would prefer to have pause if only one optione makes it in, but in the event its both, I would rather have the timer pause playback as default, and then if the player is unpaused have the timer restart for whatever duration has been set. But if its a case of selecting from two options I think pause should be the default behaviour.

I think enfolding the power off or pause options into the start timer setting unless the wording is correct could be confusing, but I agree if two options make it in, whatever option has been selected should persist unless changed.
Comment by Nick Peskett (nickp) - Thursday, 26 August 2010, 15:00 GMT
Here's a version with the following changes;

- Sleep Timer menu moved to /Settings/General Settings/System/Sleep Timer
- New option "Timer Pause Mode: Yes/ No"
- Manual updated

I made the pause mode (behaviour) a boolean as I thought this made things easier on if statements in the code.

I've kept pause resetting the timer in WPS as I thought this was handy for both modes.
Comment by alex wallis (alexwallis646) - Thursday, 26 August 2010, 20:26 GMT
does the timer still get cleared completely, if the player is manually paused before the timer runs out?

What happens is, if you pause the player before the timer runs out, the timer would actually be stopped, and you would have to restart it from the menu, when of course it starts again from scratch.

I feel that what should actually happen is that if a person pauses the player before the timer runs out, the timer should also be paused, and once playback is restarted the timer should then continue. As I think pausing playback indicates that the user of the dap intends to resume listening shortly.
I am not sure what should happen if a person chooses to stop a file completely, what do you think?

Also, its not a huge problem, as it doesn't affect me that much, but the timer is started as soon as you select the start timer option, rather than when audio playback is started. I can live with it as I only lose a few seconds, but thought I would mention it.
Comment by Nick Peskett (nickp) - Thursday, 26 August 2010, 22:25 GMT
The timer gets cleared when paused and only restarted (at the full duration) if sleeptimer_on_startup is true. This was the behaviour before I made the changes and seemed to still be the case when I tested it.

I like this resetting behaviour as pressing pause is a sign of being fully awake. I suppose someone could implement some sort of variable to keep track of whether a temporary timer was in place before pause was pressed. Or we could only clear the timer if sleeptimer_on_startup is true...

>the timer is started as soon as you select the start timer option, rather than when audio playback is started

Why don't you start the audio first then? ;)
Comment by alex wallis (alexwallis646) - Friday, 27 August 2010, 13:01 GMT
the timer isn't just reset when the player is paused by the user, it is actually completely stopped, meaning that you have to restart it from the menus.

Now, if it were to simply be reset to lets say 45 minutes if that's what it was set to when first started I could understand that, but when you pause audio, the timer is completely stopped.

There are several reasons I can think of why someone might want to pause the audio while the sleeptimer is running, for example they might have to get up to use the bathroom.
I think it would make much more sense to either reset the sleeptimer to the value it was originally set to, or pause it and then resume it when the audio is unpaused.
Comment by Nick Peskett (nickp) - Saturday, 16 October 2010, 22:53 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:37 GMT
Synced to r29159.
Comment by Nick Peskett (nickp) - Thursday, 03 February 2011, 07:14 GMT
Synced to r29198.
Keep having to update this patch is getting boring.
Comment by Martin Sägmüller (dfkt) - Thursday, 03 February 2011, 10:48 GMT
I agree. Please commit it to SVN, it's very useful.
Comment by Thomas Arendsen Hein (ThomasAH) - Wednesday, 09 February 2011, 17:25 GMT
Finally having it in SVN would be great!
Comment by Kai Posadowsky (Riffer) - Wednesday, 09 February 2011, 19:26 GMT
Certainly I agree to commit this patch to SVN, too!
Comment by Richard Fröhning (omnirox) - Monday, 28 March 2011, 13:51 GMT
Me too - I really like it!
Why is it not yet commited? Is there a procedure to go through or
just needs a developer who is able to commit it?
Comment by Torne Wuff (torne) - Monday, 28 March 2011, 14:47 GMT
It was mentioned earlier that the simulator power management changes should be split into a seperate task since it's not really related; until that happens at least it's unlikely anyone is going to be very interested in looking at it.
Comment by dave t (sockbox) - Friday, 13 May 2011, 06:12 GMT
i would love to have this committed.

a couple suggestions...

put the sleep time in the new location under system settings in the main menu.
change the toggle option to an on off sub menu so that you can set it as a quickscreen function and toggle it from there.
Comment by sideral (sideral) - Tuesday, 09 August 2011, 20:14 GMT
The patch in  FS#10849  (on which the present work is based) has evolved into a quasi-committable state IMHO. I intend to shepherd it into the main trunk soonish.

I don't see a reason why the extensions proposed here cannot be accepted in follow-up commits. But I suggest to follow the formerly stated advice and split the patch up by feature.

sockbox writes:
> change the toggle option to an on off sub menu so that you can set
> it as a quickscreen function and toggle it from there.

I'm not sure that's a good idea. The quickscreen can access only persistent configuration settings, and in my view starting the timer is a function, not a setting.

As an alternative, someone on IRC proposed to add the start-sleep-timer function as a hotkey option.
Comment by Kai Posadowsky (Riffer) - Thursday, 08 November 2012, 21:28 GMT
please close this task - all what we wanted is nearly there
Comment by Kai Posadowsky (Riffer) - Thursday, 08 November 2012, 21:45 GMT
If someone is interested in "pause after sleeptimer" - have a look here: http://www.rockbox.org/tracker/task/12779

Loading...