Rockbox wont idle powerdown if music is paused and sleep-timer is set

[edfardos]

We should probably fix this bug, or change the manual:

http://anythingbutipod.com/forum/showthread.php?p=644078#post644078

I posted fix to the source in the third post. 


thanks,
-edfardos

[saratoga]

1) Does this apply to the current source or just 3.13 ?

2) Can you post a patch of your changes rather then just those lines?

[edfardos]

Sorry, i was having trouble with the bugtracker, but here's a git formatted patch.  I did some extensive testing, with all possible combinations and permutations of idle/sleep timers, and play/pause audio status.  This patch was against the 3.13 tree, sorry, but I don't track the master personally.  I figured just a couple of lines would be easy to tweak.

thanks,
-edfardos

---
 firmware/powermgmt.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/firmware/powermgmt.c b/firmware/powermgmt.c
index b6d4b9f..7f4432d 100644
--- a/firmware/powermgmt.c
+++ b/firmware/powermgmt.c
@@ -963,8 +963,10 @@ void handle_auto_poweroff(void)
 #endif
         !usb_inserted() &&
         (audio_stat == 0 ||
-         (audio_stat == (AUDIO_STATUS_PLAY | AUDIO_STATUS_PAUSE) &&
-          !sleeptimer_active))) {
+//edfardos         (audio_stat == (AUDIO_STATUS_PLAY | AUDIO_STATUS_PAUSE) &&
+//edfardos          !sleeptimer_active))) {
+         audio_stat == (AUDIO_STATUS_PLAY | AUDIO_STATUS_PAUSE)
+         )) {

         if (TIME_AFTER(tick, last_event_tick + timeout)
 #if !(CONFIG_PLATFORM & PLATFORM_HOSTED)
--
1.7.9.5

[wodz]

Looking at the code your patch is incorrect. The line you changed says originally that if sleeptimer_active is true the control is passed to handle_sleep_timer() which in turn calls sys_poweroff() when sleeptimer expires. With your change this codepath is completely omitted. My understanding of the code is that if sleep timer is set it should prevail. So the actual question is if this makes sense and if this is what manual says.

[edfardos]

If the unit is paused, idle-powerdown should take effect.  There is no dependency on the sleep timer according to the manual.  I found the behavior objectionable, so I did some google searches, and others indicated this "bug" emerged in the 3.12 timeframe.   This begs the question if the sleep timer was designed to keep the unit awake for the prescribed period, or strictly to power down the device if it ever hits the time limit. 

..................................
8.6.2  Idle Poweroff

Rockbox can be configured to turn off power after the unit has been idle for a defined number of minutes. The player is idle when playback is stopped or paused. It is not idle while the USB or charger is connected , or while recording. Settings are either Off or 1 to 10 minutes in 1 minute steps. Then 15, 30, 45 or 60 minutes are available.

8.6.3  Sleep Timer

The Sleep Timer powers off your player after a given time, whether playing or not.

[saratoga]

I think it makes sense to have the idle power off still occur even if the sleep timer is set.  Logically at least I would expect power off conditions to be be OR'ed with one another, not have an unspecified one take precedence over the others. 

Besides, if you fall asleep with the music paused, no sense in wasting battery waiting for the sleep timer

[[Saint]]

Wow...Ok, so, apparently I'm the only one that thinks the current behaviour is deliberate?

To me, it makes absolutely no sense to idle shutdown if the user has specifically chosen to shutdown the device at a predetermined time in the future. This is a choice the user made consciously, and shouldn't be ignored. For what its worth, I also don't think it makes much sense at all to idle shutdown when the device is paused, either, but this may be semantics - to me, paused != idle.


[Saint]

[edfardos]

I think the important thing is that all desired behaviors are possible.   I don't personally ever use "stop", in fact, I modified the power/stop button so it does nothing but light up the display, and a long-hold shuts down the system.

User wants unattended playing music to end after an hour (sleep)
User wants a stopped device to power down after 10 mins (idle)
User wants a paused device to power down after 10 mins, but also wants unattended playing device to run for an hour (??)
User wants paused device to stay on for an hour (sleep)
User wants stopped device to stay on for an hour (idle)
User wants device to stay on until the battery is dead (neither)

Did I miss any?  Can we accommodate each scenario with idle, sleep, and reset-sleep-with-button?  I personally want unattended music to stop after an hour (I'm asleep), but I also want the unit to power down if it's not playing, because I paused it and threw it in my bag.  In all cases, I don't think anyone wants to pickup a device only to discover it's battery was flattened doing nothing.

This really is a cool project guys!  Lovin' this sansaclip w/ opensource rockbox,

-edfardos

[[Saint]]

Setting the sleep timer is a deliberate action (granted, pausing the device is often a deliberate action as well) and should be respected. I don't want my player to shut down because I got up to go to the bathroom and temporarily paused playback. Nor would I want to have to adjust the idle timeout each time to overcome this.

We probably can't make a case that fits all users, but we can try to make one that makes the most sense, and personally I think we already have it.


[Saint]

[Jason Arthur Taylor]

Hi all.  Saratoga was kind enough to have directed me here after I posted a bug about this issue to flyspray.  The url is http://www.rockbox.org/tracker/task/13058 in case anyone wants to read it there.  I guess I am supposed to discuss the issue here?  Certainly, we don't want to just change code arbitrarily.  Assuming that's the case, here goes.

As a preliminary matter, my calculations show that one should be able to easily quadruple battery life by using a shutdown of 2 minutes and a sleep of 30, if this feature were coded/allowed.  So this thread here isn’t trivial in nature.  It is huge.

Reading this thread I see that all but one person thinks this is a bug.  So, without getting into the details, the democratic thing to do might be to implement a patch, and to fix the problem that all but one person apparently thinks is a problem.

But perhaps we need to be sure.  If so, let me examine exactly what Saint and wodz said, and see if Saint is wrong or right, if that is instead the case.  Saint, hopefully you can explain your position in more detail in case you I mess up in my analysis.

Saint basically makes the case that if you want a computer to do something, it should do it.  The thing is that the normal use pattern of the sleep timer is to deal with unpredictability, not to do planned and definite things, like to cook eggs.

Saint wrote that,

"apparently I'm the only one that thinks the current behaviour is deliberate?

To me, it makes absolutely no sense to idle shutdown if the user has specifically chosen to shutdown the device at a predetermined time in the future. This is a choice the user made consciously, and shouldn't be ignored."

My first observation is that an assertion here Saint makes is actually incorrect. There is actually a popular option that resets the sleep timer upon any key-press, even if that keypress has nothing to do with the sleep timer.  So the timer isn’t normally invoked directly.

As an example, the typical use here is that someone is in bed with the intent of dosing off.  However, before that happens they might rewind a podcast or something, or adjust the volume briefly, or even use the player’s light.  The goal of the sleep timer here is to avoid having the player on all night long, no matter what, since you know you are going to sleep one way or the other.  And if you are awake and the player shuts down it isn’t a big deal.  You just can turn it on again.  So the auto-reset of the timer upon keypress is why it isn't true that the user has specifically chosen to shut down.  Not at all.  They selected the desire to have battery power in the morning.  That is what they wanted.  And to do it they used the auto-reset of the timer based on the last time they were clearly awake.  The method is to use the keypad as a sleeping/awake sensor, looking at any keypresses for anything.  I don't normally look at the clock, and set a time to have the unit turn off at all.  The sleep timer is constantly being reset all the time, especially since I want to use it during the day as well.  It should go on when I stop moving for a long time.  Or for instance if I take off the mp3 player because I am going swimming, or going to an event where I don't want an mp3 player, I want it to turn itself off. 

Secondly, even if we wanted a dictatorship, and I’m wrong in my intentionality argument above, if one really wants an mp3 player to go to sleep at a set time all they would have to do after the patch is in is to disable the inactivity shutdown timer, or set it to be longer than the sleep timer.  So, if you really want your player to go to sleep at a fixed time, you still will be able to, and you currently can, so there is no loss of a present feature by putting in the patch. So, even if I'm wrong, Saint you  will still have this feature you like with the patch. 

So even if I'm wrong, the patch should still go in.

That all said, let me comment about all other things Saint thus far has written.  He also wrote:

“I don't want my player to shut down because I got up to go to the bathroom and temporarily paused playback.”

The OP, edfardos, and many others have explained why the two different methods of shutting down are both simultaneously needed.  Moreover, the shutdown and startup process in flash-based mp3 players is actually super fast.  Long ago, this was not true.  So, unlike hdd mp3 players, use of shutdown and startup are really good ways to save battery life, especially with the auto-resume feature which Saint for some inexplicable reason is not using.  Un-pausing is only a little faster than turning on and auto-resuming, but one saves a lot of battery since nobody here has bothered to learn how to slow down the cpus when they aren’t being used.

The last thing Saint wrote that I haven’t addressed is this:

“I also don't think it makes much sense at all to idle shutdown when the device is paused “

IMO pause is used when there is a phone call or something.  But we all know a phone call can lead to a lot of other things.  At the end of the day, we need multiple methods of making sure the following DOES NOT OCCUR:  the player is on for no reason for a whole day or night.  We really need “sleep even though paused” capability because it is common for a player to be paused for an indeterminable amount of time.  All I know if I get a phone call when the player is on is that I'd better pause the mp3 player real fast, if it is running.  I have no idea when I will want it to resume, and I don't want to have to rush back to my residence because I forgot and left my mp3 player paused instead of in sleep shutdown because Saint doesn’t know about auto-resume.

Saint, please chime back in to explain if you still think we are all wrong.  Otherwise, let's get this bug patched to make our mp3 players last much longer between recharges.

The last thing I could comment about is the question by wodz about this being a bug as the OP says.  wodz says he has looked at the code and he thinks the code is designed to make sure the sleep timer prevails.   Well, that is in fact what the code does.  So, yes, if you just look at the code to determine intent, you end up with the current code being correct.  That is always the case, is it not, even if the code is wrong?  LOL.

And anyway, if the intent was to make sure the mp3 player was off via sleep within 30 minutes, and the inactivity timer shut the player off within 10 minutes, is not the desire that the player be off within 30 minutes not discharged if the player was already off 20 minutes early?  From a battery saving perspective, the answer is yes, and if we didn't care about saving power there would be neither sleep nor inactivity code at all.

I'd say the OP is instead correct, since if Saint were really correct we probably wouldn't even have these two separate times for inactivity and sleep in the first place.  The interface would be "either or" prior to being able to set these two timers; it would drop down to whichever method of automatic shutting down was preferred. 

We can also do some forensics, seeing who introduced the sleep code.  But that is too much for now.  Let's just look at the present code.  I think it says,

"/*
 * We shut off in the following cases:
 * 1) The unit is idle, not playing music
 * 2) The unit is playing music, but is paused
 * 3) The battery level has reached shutdown limit
 *
 * We do not shut off in the following cases:
 * 1) The USB is connected
 * 2) The charger is connected
 * 3) We are recording, or recording with pause
 * 4) The radio is playing
 */"

I didn't post this to flyspray but the 2nd #3 is no longer true since the sleep timer now also prevails there.  The coder who did the sleep feature apparently didn't do a complete job, and also broke the 1st #2, and didn't edit the comments either.  Why not? 

The best explanation is that the sleep timer coder didn't know he or she was breaking these features.  It is common for someone who wants a feature to focus just on their feature.  I'm guessing.  But if that guess is correct there didn't seem a need to edit these comments.  He was just trying to add a new feature.  I'd love to see the 2nd #3 and 1st #2 re-enabled. 

IMO, that the sleep coder seems to have patched the code in one particular and arbitrary way, unknowingly breaking two features, has little bearing on what we should do now that we realize he or she introduced two bugs.  Personally, I'm thinking the whole section could be rewritten, since this thread proves that the code isn't easy to understand. 

So, to sum up about the bug/feature debate, the current interface, and the manual, and the comments in the code itself all point towards this being two bugs and not some microsoft lawyer's vision of "hidden features". 

Cheers,
Jason Arthur Taylor jasontaylor.us

[monoid]

The thing is that the normal use pattern of the sleep timer is to deal with unpredictability, not to do planned and definite things, like to cook eggs.

I am not sure, this is right. At least not for me. If I set, switch off in 30 min. I meant it exactly so, there is no space for unpredictability.

It seems to me, you use the sleep timer for tasks that it was not intended for. And because of that you require different behaviour of it to meet your pattern of use...

Maybe that the solution would be to implement a special switch-off timer that would meet your requirements, which make sence in some cases, but do not in another cases.

Sleep timer should behave like expected by most of people, e.g. as "dull" sleep timer which switches off after preset time without any other conditions.

Switch off timer might fulfill more complex tasks....

I understand your point that it is possible to setup switch-off timer to behave like sleep timer, but it is technical poin of view. One should look at things from user perspective. Most of users do not read manuals and many have problems even with simple setups, not speaking of more complex ones. So simply, sleep timer should be sleep timer. And if there is another timer, than OK. Less advanced user would skip it, more advanced would use it.

I speak about interface. Internaly, both timers might be represented by single computer code, if possible.
 

[Jason Arthur Taylor]

Thanks so much for taking the time to chime in, monoid.  With 100+ posts, I’m sure everyone respects your opinion very much, and that you've now likely prevented any change to this sacred code.  However, I am not smart enough to really understand all of your perspective, on many levels.  May I ask you some questions?  If so, here goes.

Firstly, is it true that you agree that, as I said previously,

“if one really wants an mp3 player to go to sleep at a set time all they would have to do after the patch is in is to disable the inactivity shutdown timer, or set it to be longer than the sleep timer.  So, if you really want your player to go to sleep at a fixed time, you still will be able to, and you currently can, so there is no loss of a present feature by putting in the patch. So, even if I'm wrong, … you  will still have this feature you like with the patch.”

?    Also, do you agree that if the mp3 player shutsdown because someone enabled the inactivity timer and programmed that to go faster than the sleep timer, that it saves battery life? 

The code already has two shutdown timers.  To anyone who has used a laptop computer, which is most people using rockbox IMO, it is actually far less complicated to have both running independently, than the current situation in which one timer mysteriously, against the documentation, turns the other off, and without any explanation or warning.  Consider a laptop.  Does setting to shutdown in 120 minutes disable all other shutdowns, such as a hibernate setting at 60 minutes of inactivity?  Or the screen saver?  Or the hdd spindown timer?  No.  This is not how laptops are programmed.  But this is how you apparently think they should be programmed.  In a laptop, each timer runs independently.  What you see as simpler, I see as strange, more complex, wrong, and disrespectful to most users who would welcome more battery life. 

Is it correct that you think the average users here:

(1) don’t care much about battery life,
(2) are smart enough to mod the firmware of their device,
(3) are also smart enough to figure out how to turn on the sleep timer,
(4) but, mysteriously, are not smart enough to figure out how to disable the idle poweroff timer,
(5) and do not want the idle power off to work in conjunction with the sleep timer,
(6) and are not able to read the manual, which is currently wrong,
(7) and I, not you, have a special use situation, even though most people also shared my view.

Is 1-3 correct?  Are you sure 4-7 are also correct?  If so, why?  What exactly is your use of the sleep timer that it makes battery life unimportant? Perhaps if we understood that, we’d be able to comprehend what you are saying better.

[monoid]

“if one really wants an mp3 player to go to sleep at a set time all they would have to do after the patch is in is to disable the inactivity shutdown timer, or set it to be longer than the sleep timer.  So, if you really want your player to go to sleep at a fixed time, you still will be able to, and you currently can, so there is no loss of a present feature by putting in the patch. So, even if I'm wrong, … you  will still have this feature you like with the patch.”

No, I do not agree fully with this statement.

While it is logically and technically correct, it omits the fact, that the average user would not disable inactivity shutdown timer. And many users might be confused, many users might set the things incorrectly and than complain that there is a bug. Keep in mind that hardly any user reads the manual. Many users are not able to understand the manual. And that it is quite common that there are mistakes in manuals or manuals are outdated.

Because of that, user interface should be as self explanatory, as possible and as simple as possible.

Beleive me or not, it is not easy task to decide, what features and settings to include in SW and what not. It is not the more, the better.

And yes, I think 1 to 7 is generally true. But I would rewrite 4 and 6:
(4) do not know that there is the idle poweroff timer set on and influencing sleep timer
(5) many of them do not read the manual at all, and if by chance do, quite big part of them does not understand it (even if it is by chance correct)

Keep in mind, that ability to read does not mean ability to comprehend. And to comprehend interaction of two settings at two different places is beyond ability of quite high percentage of users. From my experience, I would guess 20%, maybe more. And manual reads about 10 to 20% users, so you migth have probably more than one half of confused users of this feature. (Keep in mind that part of RB users is not native English speaker, some hardly speak English, and even quite big part of native speakers has severe problems to understand more complex technical texts.)

That is why I suggested to add one more switch-off timer which would do what you suggest. It would be disabled on default. The idea behind it is, that user would know, that it is different from sleep timer and so there would be some chance that he would look in the manual what it is for.

I understand, that you desire that feature and OK, why not. But it does not seem to me be good idea to modify current behaviour of sleep timer. Instead I would suggest to add one more timer to do what you desire. :-)

But, I am not developer of the RockBox and thus I have no influence on what is going to be implemented and what not. I just shared my opinion on the topic.

[[Saint]]

I spent a very long time just now painfully rebutting all the points you made, and, just now, I highlighted all that text and deleted it.

Because...y'know what? It's just not worth it for me to invest my time in disingenuous claims and some laughable attempt to paint me as a dictator because I have an opposing view from you and a higher position in this forum. That doesn't make me a dictator. It makes me just as you are, a person with an opinion.


The reality here is that unless I see any indication otherwise, I am going to choose to believe that your claims about battery life being increased exponentially are invalid.

Why?

Simply put, I have no reason to believe that anyone else, let alone the majority, is using the sleep timer in the same way you are.


[Saint]

[gevaerts]

For the record, rockbox is *not* a democracy. Looking at it as a dictatorship with the committers in power is probably a fairly good way of looking at it. And yes, that is wanted.