Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Notice: A non well formed numeric value encountered in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 96 Deprecated: Function create_function() is deprecated in /sites/rockbox.org/flyspray/includes/class.flyspray.php on line 104 Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /sites/rockbox.org/flyspray/adodb/adodb.inc.php on line 845 Deprecated: Function create_function() is deprecated in /sites/rockbox.org/flyspray/includes/class.user.php on line 111 FS#6421 : Make "disk power off" always be on on supported targets, remove the option.

Rockbox

Tasklist

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

Attached to Project: Rockbox
Opened by Nils Wallménius (nls) - Friday, 08 December 2006, 11:42 GMT
Last edited by Nils Wallménius (nls) - Friday, 08 December 2006, 14:15 GMT
Task Type Patches
Category Settings
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

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.
   ata_poweroff_always_enable.di... (6.1 KiB)
This task depends upon

Closed by  Nils Wallménius (nls)
Tuesday, 23 January 2007, 15:46 GMT
Reason for closing:  Accepted
Additional comments about closing:  comitted
Comment by Nils Wallménius (nls) - Monday, 18 December 2006, 20:14 GMT
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 :-)
Comment by Nils Wallménius (nls) - Monday, 18 December 2006, 21:54 GMT
Synced with cvs 061218
   ata_poweroff_always_enable2.d... (6.1 KiB)
Comment by Nils Wallménius (nls) - Wednesday, 27 December 2006, 19:32 GMT
synced with CVS 061227
(application/octet-stream)    ata_poweroff_always_enable3.d... (6.1 KiB)
Comment by Nils Wallménius (nls) - Friday, 29 December 2006, 11:27 GMT
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 :-)
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 16:33 GMT
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.
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 19:41 GMT
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.
Comment by Nils Wallménius (nls) - Friday, 19 January 2007, 19:53 GMT
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.
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 19:57 GMT
I will test that next
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:10 GMT
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.

Comment by Nils Wallménius (nls) - Friday, 19 January 2007, 20:15 GMT
Which exact error message do you see when the reading fails?
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:18 GMT
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.
Comment by Nils Wallménius (nls) - Friday, 19 January 2007, 20:23 GMT
That is definatley bad, does the same thing happend without the patch and the option enabled?
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:25 GMT
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.
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:33 GMT
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.)
Comment by Nils Wallménius (nls) - Friday, 19 January 2007, 20:34 GMT
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 ;-)
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:45 GMT
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)) {
Comment by Nils Wallménius (nls) - Friday, 19 January 2007, 20:54 GMT
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).
Comment by Robert Cotey (coteyr) - Friday, 19 January 2007, 20:55 GMT
then no need to be a pest
Comment by Barry Wardell (barrywardell) - Saturday, 20 January 2007, 01:28 GMT
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).
Comment by Robert Cotey (coteyr) - Saturday, 20 January 2007, 01:31 GMT
giving it a try now
Comment by Robert Cotey (coteyr) - Saturday, 20 January 2007, 01:54 GMT
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
Comment by Nils Wallménius (nls) - Saturday, 20 January 2007, 02:10 GMT
Synced to SVN 070120 and removed a useless check, so its a tiny bit cleaner :-)
   ata_poweroff_always_enable4.d... (5.1 KiB)
Comment by Robert Cotey (coteyr) - Saturday, 20 January 2007, 09:42 GMT
a little over 7 hours. Still it's an improvment.

No crashes or read errors.
   battery_bench.txt (53.5 KiB)
Comment by Barry Wardell (barrywardell) - Saturday, 20 January 2007, 12:20 GMT
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)?
Comment by Nils Wallménius (nls) - Tuesday, 23 January 2007, 15:27 GMT
Ok, synced with current SVN, and confirmed working on x5, so I guess this should go in.
   ata_poweroff_always_enable5.d... (5.2 KiB)

Loading...