FS#11288 - Sansa Fuze v2 scrollwheel lockup after 20 minutes - fix

Attached to Project: Rockbox
Opened by Chris Savery (csavery) - Tuesday, 18 May 2010, 02:09 GMT
Last edited by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 15:42 GMT
Task Type Patches
Category Drivers
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


Here's a patch to test for the scroll wheel lockup bug.
It appears to be working fine for me but I'm not completely sure about wheel acceleration and any subtle behaviours. It does work for me after about 30 minutes.
Tested against r26214.
Please feel free to test and let me know if something is wrong.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Thursday, 20 May 2010, 15:42 GMT
Reason for closing:  Fixed
Additional comments about closing:  r26175, also see comments
Comment by Chris Savery (csavery) - Tuesday, 18 May 2010, 02:11 GMT
Oops. Incorrect patch info but I can't edit...
Comment by Rafaël Carré (funman) - Tuesday, 18 May 2010, 07:34 GMT
I can't test it now but I have remarks:

The loop interval is changed from 3ms (+/- 1ms) to 10ms (+/- 10ms), doesn't it change the wheel behaviour too much ?

Also when giving the intervals you assume HZ=100.
While it's unlikely to change, it's better if you do something like (30*HZ) / 1000 for 30ms
Comment by Chris Savery (csavery) - Tuesday, 18 May 2010, 10:00 GMT
Thanks for fixing the patch info.
I didn't notice any difference in the feel of the scrolling but I did notice that wheel acceleration was ineffective.
I think this is due to the low resolution of the HZ timer which is made worse by the inversion into a velocity value.
I was modifying the define values as suggested when it occurred to me of a better solution that keeps the high res kernel timer.
I'm just testing this method now and will post shortly if it works.
It should maintain the res needed for useful acceleration, and would be a much simpler change.

btw that's a mistake up above, actual test build was r26114. I seem to be all butter fingers today.
Comment by Chris Savery (csavery) - Tuesday, 18 May 2010, 11:10 GMT
Ok. Here's a new patch.
Wheel feel and acceleration both work the same and it didn't lock up after 30 minutes.
This one makes a very simple change using the current hi-res timer.
Normally we compare the time difference "v" with WHEEL_LOOP_INTERVAL but now if it is less than zero I assume a wrap has occurred and I re-sync the last_wheel_post time before returning. This throws away the first wheel click if after about 23 minutes but I think we can live with that as the following clicks are back in sync and get responded to.
Let me know if any problems.
Comment by Thomas Martitz (kugel.) - Wednesday, 19 May 2010, 17:38 GMT
I think it's not the optimal solution but it should be good enough, so it could be committed if you resync it.
Comment by Chris Savery (csavery) - Thursday, 20 May 2010, 09:25 GMT
It looks like in current svn the WHEEL_LOOP_INTERVAL test has been removed.
This makes this patch no longer relevant.
Comment by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 10:08 GMT
Wasn't the problem that "long time = TIMER2_VALUE + current_tick*TIMER_TICK; /* to timer unit */" overflows? I think that problem still applies.
Comment by Chris Savery (csavery) - Thursday, 20 May 2010, 13:27 GMT
Yes, but the overflow only affects one "click" because the last_time is sync'd. It was the test and return that caused the lockup since no matter how many "clicks" followed the overflow they would all cause a return, until the overflow value once again became positive. Without the test condition and return no lockup occurs any more.

I don't think the test condition for WHEEL_LOOP_INTERVAL really did much because in my reading of the code it seems that hardware polling for the wheel occurred at 200Hz. This meant that the minimum interval between possible clicks was 5ms. Since the test condition detected and removed clicks below 3ms I think it would have made very little difference to how the wheel ultimately behaved. Removing the test condition is likely just as good as my fix which prevented it from locking on negative values (overflows).

I sync'd my code to r26185 today and didn't notice any change in wheel use but I didn't try a 30 minute test because this code is now reverted to r26059, before the test condition was inserted, and that code worked ok.
Comment by Thomas Martitz (kugel.) - Thursday, 20 May 2010, 14:04 GMT
OK, I'll close this one then, you explained that we don't need to worry about the overflow. Is that OK?

On the fuzev2, there's no polling. Each wheel change (4 per physical click) generates an interrupt, i.e. the function is run. The fuzev1 wheel is read by polling.
Comment by Chris Savery (csavery) - Thursday, 20 May 2010, 14:46 GMT
Ok with me. I believe this to be fine now that the code was changed back.
I probably got mixed up in the code regarding polling. It's still very new to me.