Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Settings
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Nils Wallménius - 2006-12-08
Last edited by Nils Wallménius - 2006-12-08

FS#6421 - Make "disk power off" always be on on supported targets, remove the option.

This patch makes rockbox always power down the disk when it is spun down on supported targets and removes the “disk poweroff” option in the “general settings → system → disk” menu as it is now uneccesary. I do not know of a way to check if the disk is actually powered down… Tested on H320 works well. Also since the settings block changed, your settings will be reset.

Edit: size comparison shows that it makes rockbox.iriver 172 bytes smaller for H300.

Closed by  Nils Wallménius
2007-01-23 15:46
Reason for closing:  Accepted
Additional comments about closing:  

comitted

Nils Wallménius commented on 2006-12-18 20:14

Tested and working on an iriver h100, I am about to make some battery runtime tests with my h300 to see if the disk is actually powering down, will get back with results. I you have a player which supports disk poweroff and hasn't been tested yet please do and if you can do the neccesary measuring to tell if the disk is actually powering down all the better :-)

Nils Wallménius commented on 2006-12-18 21:54

Synced with cvs 061218

Nils Wallménius commented on 2006-12-27 19:32

synced with CVS 061227

Nils Wallménius commented on 2006-12-29 11:27

Ok, battery testing on my h320 gave one hour longer battery life with the patch than without and the option set to "no". That means it works as intended :-)

Robert Cotey commented on 2007-01-19 16:33

The patch introduces or exposes a bug. When the harddrisk is powered off it takes two Hard drive actions to get it to work again. The first fails(and activiates the drive) the second succeededs.

Things to and from the hard drive should be cached to avoid this issue. I am not sure what problems wold arise of you cached disk commands.

to Reproduce go to a directory containing a txt file. Wait for the hard drive to power off. Try opeining the text file. This will faill but the drive will spool. Try reading it again and It will work.

Tried on H10 20G

The directory cache setting is turned on.

Robert Cotey commented on 2007-01-19 19:41

Turning directory cache settings off seem to fix the lockups caused by the "bug" but it still takes two access of a file to get the file to work.

On a good note I tested this patch and got an extra 3 hours out of my device. This put battery consumption for the H10 at ~ 8 hours with no non-default settings other then this patch.

I am sure the crashes didn't help as when they happen the disk continued to spin until I noticed it and reset it. (Sometimes al long as 30 -45 mins.) I will continue to do more precise testing.

Nils Wallménius commented on 2007-01-19 19:53

Thank you for testing!
I have been trying to reproduce the bug that the first read fails, but it works flawlessly on my H300.
My suspision is that Rockbox tries to read from the disk too soon, as the power on, then spin up takes longer to finish.
One thing You could try is increasing the value of READ_TIMEOUT on line 64 in ata.c

(it looks like this)
#define READ_TIMEOUT 5*HZ

try increasing the number (seconds) before *HZ to maybe 7 or 8 and see if it works ok.

Robert Cotey commented on 2007-01-19 19:57

I will test that next

Robert Cotey commented on 2007-01-19 20:10

your assumptions seem correct If i listen carfully enough I can hear the powerup for the harddrive taking longer then it does to display the error on the screen. the READ_TIMEOUT 8*HZ still produced the error. I am going to increse the value more and see what happens.

Nils Wallménius commented on 2007-01-19 20:15

Which exact error message do you see when the reading fails?

Robert Cotey commented on 2007-01-19 20:18

I see errors pertaining to hard drive access.

Sometimes "Can't open viewer" type errors.

Sometimes .rockbox directory missing

sometimes it just crashes (most notably when changeing songs)

Others it just freezes.

It does not take long to test the REAT_TIMEOUT setting. I can repoduce it every time.

Nils Wallménius commented on 2007-01-19 20:23

That is definatley bad, does the same thing happend without the patch and the option enabled?

Robert Cotey commented on 2007-01-19 20:25

I just tried the READ_TIMEOUT 40*HZ //if it is supposed to work in 5 then it should definatly in 40

but it doesn't

I am now trying without the patch and with the option on.

Robert Cotey commented on 2007-01-19 20:33

Looks like it does do it without the patch. There for it simply exposed an already existing problem. Other then this little hiccup (which is not caused by this patch) This patch seems to work well.

I will definaly be running this patch (as soon as I figure out a way around this power down issue.)

Nils Wallménius commented on 2007-01-19 20:34

Could you test increasing the timeout on line 104, it seems very long but it might be worth a shot, I will try to catch someone who is more familiar with the ata driver and see if they have any idea, but friday night migt not be the best time ;-)

Robert Cotey commented on 2007-01-19 20:45

Made the following changes but it had no effect. I am going to poke around a-little. I should also open a new bug report as this does not seem to be a part of your patch that is causeing the problem.

Index: firmware/drivers/ata.c

— firmware/drivers/ata.c (revision 12071)
+++ firmware/drivers/ata.c (working copy)
@@ -100,7 +100,7 @@
static int wait_for_bsy(void) ICODE_ATTR;
static int wait_for_bsy(void)
{
- long timeout = current_tick + HZ*30;
+ long timeout = current_tick + HZ*60;

   while (TIME_BEFORE(current_tick, timeout) && (ATA_STATUS & STATUS_BSY)) {
       last_disk_activity = current_tick;
       yield();

@@ -120,7 +120,7 @@

   if (!wait_for_bsy())
       return 0;

- timeout = current_tick + HZ*10;
+ timeout = current_tick + HZ*20;

   while (TIME_BEFORE(current_tick, timeout) &&
          !(ATA_ALT_STATUS & STATUS_RDY)) {
Nils Wallménius commented on 2007-01-19 20:54

The main H10 developer is aware of the problem and had some ideas, he might be able to fix it.
Feel free to open a bug report on this issue (although it's known it ensures that it isn't forgotten).

Robert Cotey commented on 2007-01-19 20:55

then no need to be a pest

Barry Wardell commented on 2007-01-20 01:28

I've just committed a fix for Robert's problem. Provided it works as it should (my tests show it does), I think disk poweroff should now always be enabled on the H10 (ie. I'm in favor of this patch).

Robert Cotey commented on 2007-01-20 01:31

giving it a try now

Robert Cotey commented on 2007-01-20 01:54

works perficetly,applied the patch to the updated SVN and it seems to work perfectly. I am going to run a test overnight and see that the battery life is now. Hopefully we will reach more then 8 hours (hopeing that the time will be longer with the fact that it shouldn't crash due to file access problems)

I'll let you know how it turns out.

Thanks

Nils Wallménius commented on 2007-01-20 02:10

Synced to SVN 070120 and removed a useless check, so its a tiny bit cleaner :-)

Robert Cotey commented on 2007-01-20 09:42

a little over 7 hours. Still it's an improvment.

No crashes or read errors.

Barry Wardell commented on 2007-01-20 12:20

So what's keeping this patch from being committed at this stage? It is reported as working on H10, H100 and H300. Is it just that it still needs to be tested on the remaining affected targets (X5, Gigabeat, Player and Recorder as a far as i can tell)?

Nils Wallménius commented on 2007-01-23 15:27

Ok, synced with current SVN, and confirmed working on x5, so I guess this should go in.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing