Rockbox

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 :-)

Synced with cvs 061218

synced with CVS 061227

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 :-)

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.

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.

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.

I will test that next

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.

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

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.

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

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.

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.)

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 ;-)

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)) {

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).

then no need to be a pest

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).

giving it a try now

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

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

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

No crashes or read errors.

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)?

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