Rockbox

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

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?

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.

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.

Warning: Undefined array key "useheading" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 1099 Warning: Undefined array key "target" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 557 Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 557

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.

PS excuse the poor punctuation - something odd with the keyboard in this development VM.

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.

Ha, no problem. The codebase is unfamiliar, so I wouldn't be at all surprised (or offended) to be told my approach was incorrect.

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.

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?

Nah, I didn't understand the point of the patch. I'll look again now that it's clear to me.

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!

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.

(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!

Ok, I guess I need to intercept the code executed when play is pressed? Back to the code (and #rockbox). Thanks!

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.

Functionally identical, just cleaning up my build a bit and applying against the latest trunk.

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.

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?

If you're planning work on the sleep timer could you also consider  FS#11042 ?

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.

Warning: Undefined array key "useheading" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 1099 Warning: Undefined array key "target" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 557 Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 557

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

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

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?

Nick, do you want to take over this?
You're the sleep timer expert ;)

Hi Thomas,

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