This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
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, 17:11 GMT+2
Last edited by Dominik Riebeling (bluebrother) - Thursday, 08 November 2012, 23:04 GMT+2
Opened by Kai Posadowsky (Riffer) - Saturday, 20 February 2010, 17:11 GMT+2
Last edited by Dominik Riebeling (bluebrother) - Thursday, 08 November 2012, 23:04 GMT+2
|
DetailsThis work is based on
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). 
sleep_timer_menu_with_wps_2_pause_24785.patch
(21.9 KiB)
apps/playlist.c | 4 apps/lang/english.lang | 58 ++++++++++- apps/gui/wps.c | 7 + apps/settings.h | 3 apps/menus/time_menu.c | 5 apps/menus/exported_menus.h | 23 ++-- apps/menus/main_menu.c | 43 -------- apps/settings_list.c | 12 ++ apps/SOURCES | 1 apps/root_menu.c | 6 + apps/main.c | 5 firmware/powermgmt.c | 13 +- manual/appendix/config_file_options.tex | 3 manual/main_menu/main.tex | 34 ++++-- uisimulator/common/stubs.c | 25 ++++ uisimulator/common/powermgmt-sim.c | 166 +++++++++++++++++++++++++++++++- 16 files changed, 327 insertions(+), 81 deletions(-) |
This task depends upon
Closed by Dominik Riebeling (bluebrother)
Thursday, 08 November 2012, 23:04 GMT+2
Reason for closing: Out of Date
Additional comments about closing: Closed on request of the reporter.
Thursday, 08 November 2012, 23:04 GMT+2
Reason for closing: Out of Date
Additional comments about closing: Closed on request of the reporter.
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.]
This is a complete patch as above with the fix for this issue.
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?
Can FS#10754 be removed now as it's happily redundant? (...wanders off to look...)
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.
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?
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?
Sounds good, I'm glad sleep timers have a new champion! It may just get accepted yet ;)
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?
My mistake was thinking if I built a better mousetrap they'd beat a path to my door.
Today evening, if nothing blocks me.
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.
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);
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?
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 :-))
* 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.
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
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"
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?
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
?
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.
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.
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.
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.
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.
- 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.
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.
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? ;)
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.
Updated manual removal of wake-up alarm warning from r28263.
Keep having to update this patch is getting boring.
Why is it not yet commited? Is there a procedure to go through or
just needs a developer who is able to commit it?
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.
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.