Rockbox

Tasklist

FS#10754 - Amend Sleep Timeout to Pause Timeout

Attached to Project: Rockbox
Opened by Sean Inglis (seani) - Monday, 02 November 2009, 15:12 GMT
Task Type Patches
Category Music playback
Status Assigned
Assigned To Thomas Jarosch (thomasjfox)
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

Amend the sleep timeout behaviour to pause when the timeout matures.

This allows the idle timeout to power off the player if it remains untouched, or the sleep / pause timeout to recycle if the user unpauses before idle timeout kicks in.
This task depends upon

Comment by Sean Inglis (seani) - Monday, 02 November 2009, 21:34 GMT
Ok, this time with the patch.

This has been tested on a Sansa C240 and Gigabeat F40 targets and works as Id expect. The sleep timeout restarts for the chosen period if the user resumes playback and this process continues until the player powers off.

Best thought of as listening-to-a-long-audiobook-or-mix-and-might-fall-asleep mode.


Standard sleep timer behaviour is changed in two ways:

1) The time for an actual power-off is changed from sleep-timeout to (sleep-timeout + idle-timeout)

2) If no idle timeout is set, the player will never actually sleep, but will stay on pause
Comment by Thomas Martitz (kugel.) - Monday, 02 November 2009, 21:44 GMT
I'm not sure why you change behavior in this way. I also would like to know what it achieves, can't you get the same result using idle timeout only?
Comment by Sean Inglis (seani) - Monday, 02 November 2009, 22:15 GMT
Idle timeout turns off the player when its in a paused or stopped condition.

Sleep timeout turns off the player unconditionally with no opportunity for the user to listen for longer if required, and when the player is started up again, the sleep timeout isnt persisted and needs to be re-enabled.

An example of use case I have in mind is a longish audiobook. Set the sleep timeout for 10 minutes and you get the track in 10 minute chunks. If you get distracted, fall asleep, whatever, the sleep timeout pauses and then the idle timeout matures and turns it off. If, on the other hand, you want another 10 minutes worth, you just press play and get another chunk.

This is what prompted me - I was nodding off and having to search backward and forwards through the book / podcast to find the last thing I could remember.

The change seemed fairly non-intrusive to me from a code and a behaviour point of view.
Comment by Thomas Martitz (kugel.) - Monday, 02 November 2009, 23:07 GMT
And you can't do this with the idle timeout only? EDIT: Ok ,I get that part now, but still:

The other changes you've made seem unrelated (and bad) to me.
Comment by Sean Inglis (seani) - Monday, 02 November 2009, 23:19 GMT
No, as I say, idle timeout only works when the player is *already* paused or stopped (idle).

Sleep timeout turns off the player immediately when the sleep timeout matures. Try it and youll see what I mean.


Im interested in "bad" as a description of the changes - how so. This is the first change Ive attempted so Im keen to understand whats bad about them?


As far as the rationale for the functionality is concerned, originally I was just canvassing for opinion to see if anyone other than me would use it in this post:

http://forums.rockbox.org/index.php?topic=23063.0


I wasnt actually making a feature request initially, but it was moved to feature requests where a couple of people (bakseetdrivr and AlexP) seemed to think it might have legs, so I gave it a spin.


Comment by Sean Inglis (seani) - Monday, 02 November 2009, 23:20 GMT
PS excuse the poor punctuation - something odd with the keyboard in this development VM.
Comment by Thomas Martitz (kugel.) - Monday, 02 November 2009, 23:55 GMT
Sorry for being rude. I thought the object of the patch was to refresh (start again from the selected value) the sleep timer on an action. Under that impression, the other changes seem unecessary to me.
Comment by Sean Inglis (seani) - Monday, 02 November 2009, 23:59 GMT
Ha, no problem. The codebase is unfamiliar, so I wouldn't be at all surprised (or offended) to be told my approach was incorrect.
Comment by Sean Inglis (seani) - Tuesday, 03 November 2009, 00:04 GMT
But can I confirm; do some of the changes *still* seem unnecessary?

If this is the case could you give me a pointer which changes you mean - they all seemed necessary to me. I wanted to pause on sleep-timeout, and restart the timeout if play was resumed.
Comment by Sean Inglis (seani) - Tuesday, 03 November 2009, 00:23 GMT
Actually looking back at it, I think I can see what you mean. I was cautious about reusing some of the existing vars being concerned about side-effects but I should revisit that.

Let me have another go at it and see if I can address your concerns. Do I update this thread with other patch candidates?
Comment by Thomas Martitz (kugel.) - Tuesday, 03 November 2009, 00:30 GMT
Nah, I didn't understand the point of the patch. I'll look again now that it's clear to me.
Comment by Steve Bavin (pondlife) - Tuesday, 03 November 2009, 07:39 GMT
Would it be possible/reasonable to reset the sleep timer on button press (i.e. including unpause)? I'd think that playback.c shouldn't actually be involved in this change.

Also, rename "sleep timer" to "pause timer" - and make a note (at least) that the manual needs updating too.

Other than that, I think this is a good idea!
Comment by Sean Inglis (seani) - Tuesday, 03 November 2009, 10:13 GMT
pondlife, I had a good think about it, but there were a couple reasons I didn't go down that route:

1) Resetting the timer on any button press seems like a significant change to the behaviour of the current sleeptimer, which determinedly counts down in the face of any UI activity. As I couldn't find any feature request to change this behaviour I thought it best left alone. As it is I think anyone currently relying on the sleeptimer wouldn't be inconvenienced (or even notice) the change in behaviour.

2) The way I look at it is that it's still a sleeptimer, just with the option of a quick reset within the idle-timeout period. Pause-timer was a convenient term to refer to the change, but it's still just a refinement to the sleeptimer to me. I'd imagine it's still a good idea to update the manual with some mention of this particular use-case? (no idea how that's done at the moment).

3) Is the reference to playback.c really a polite way of saying "you shouldn't be making changes in playback.c" :-) I sucked my teeth a bit about this as well.
Comment by Steve Bavin (pondlife) - Tuesday, 03 November 2009, 10:19 GMT
(1) Fine - this was mainly inspired by (3)
(2) OK
(3) playback.c ought not to need to be aware of such things, if at all possible.

Sorry for the terse response - work calls!
Comment by Sean Inglis (seani) - Tuesday, 03 November 2009, 10:40 GMT
Ok, I guess I need to intercept the code executed when play is pressed? Back to the code (and #rockbox). Thanks!
Comment by Sean Inglis (seani) - Tuesday, 03 November 2009, 12:58 GMT
Ok, second bite at it:

1) Removed the reset-sleep-timer changes from playback.c and shifted them to wps.c

2) Small change to functions in powermgmt.c

3) Added a couple of lines in the manual describing the change.
Comment by Sean Inglis (seani) - Friday, 06 November 2009, 18:05 GMT
Functionally identical, just cleaning up my build a bit and applying against the latest trunk.
Comment by alex wallis (alexwallis646) - Friday, 13 November 2009, 23:48 GMT
I have wanted this feature for several years now, in fact I am sure I put a forum suggestion, or mailing list post suggesting exactly this feature. as there is nothing more annoying than being wide awake and not being able to reset the sleep timer, and then having to go through putting your player back on etc. and in fact I like this way better than the way the original sleep timer worked.
Comment by Thomas Jarosch (thomasjfox) - Thursday, 03 March 2011, 01:20 GMT
I took a look at the latest version of the patch. Some comments:

- We should still execute sys_poweroff() if the idle timeout is disabled.
Otherwise we break the existing sleep timer.

Reading the first comment again, this change was intentional?

- The rest of the sleep timer in apps/gui/wps.c won't trigger in all cases,
f.e. the "multimedia buttons" used by Android/mameo.

playback.c wasn't a bad place after all. Either we tap into that
or add an event queue to the "power" thread and send an
event to it. Other options? kugel?
Comment by Nick Peskett (nickp) - Tuesday, 08 March 2011, 23:13 GMT
If you're planning work on the sleep timer could you also consider  FS#11042 ?
Comment by Thomas Jarosch (thomasjfox) - Wednesday, 09 March 2011, 06:17 GMT
Hi nickp, what's with your own version of this in FS #10849?

It's a bit confusing that there are two active patches
floating around doing almost the same thing.
Comment by Nick Peskett (nickp) - Wednesday, 09 March 2011, 07:02 GMT
Hi Thomas,

My version doesn't have the option to only activate pause when the timer comes to fruition, it just powers the device off like the code in current trunk. In  FS#11042  the behaviour is optional.

The reason I keep  FS#10849  active is there's a bit of a flaw in  FS#11042 , in that the timer gets cancelled when the audio is paused, and only re-instated when the audio is resumed if sleeptimer_on_startup is set (http://www.rockbox.org/tracker/task/11042#comment36858).

I'd be more than happy for  FS#10849  to be closed if a neat solution could be found to this problem.

Cheers

Nick



Comment by Thomas Jarosch (thomasjfox) - Thursday, 10 March 2011, 19:32 GMT
Hello Nick,

thanks for the explanation. I'm currently still thinking if the modification of apps/gui/wps.c
is the right place for this code as it won't work for the "multimedia" keys (I guess).
Dunno about SBS (status bar) themes, too.

What about taking care of FS #10754 first and then look into FS #10849/11042?
If I understood currectly, the latter ones already contain the same feature as
this task, so it shouldn't hurt to split the functionality.

Cheers,
Thomas
Comment by Nick Peskett (nickp) - Thursday, 10 March 2011, 23:12 GMT
I agree wps.c isn't the best place for the code, the multimedia keys point is a good one.

I was only posting a link to the other sleep timer patches in case you'd missed them, it makes sense to sort this one out first.

Are you intending to tackle the issue of manual pause cancelling a current timer, or do you plan for the timer to do nothing if it reaches zero and pause is already active?
Comment by Thomas Jarosch (thomasjfox) - Tuesday, 03 January 2012, 22:38 GMT
Nick, do you want to take over this?
You're the sleep timer expert ;)
Comment by Nick Peskett (nickp) - Wednesday, 04 January 2012, 09:23 GMT
Hi Thomas,

Sorry, I'm a bit sick of the sleep timer now.

Loading...