Rockbox

Tasklist

FS#5594 - Ipod scroll acceleration patch

Attached to Project: Rockbox
Opened by Thomas Paul Diffenbach (tpdiffenbach) - Saturday, 24 June 2006, 15:55 GMT
Last edited by Nicolas Pennequin (nicolas_p) - Thursday, 11 October 2007, 22:46 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System iPod 5G
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 5
Private No

Details

This patch adds ipod scroll acceleration, as explained here: http://forums.rockbox.org/index.php?topic=4973.0

In addition, it displays the scroll acceleration factor (0-3) and the clicks per second (96 clicks in 360 degrees) on the statusbar.

In addition, because I'm too lazy to remove it, it incorporates the list scrolling optimization, described here: http://forums.rockbox.org/index.php?topic=4998.0
This task depends upon

Closed by  Nicolas Pennequin (nicolas_p)
Thursday, 11 October 2007, 22:46 GMT
Reason for closing:  Out of Date
Additional comments about closing:  Superceded by  FS#7738 
Comment by Larry L. (lalittle) - Monday, 28 August 2006, 22:33 GMT
Since this no longer works in the newer builds (Aug. 06) is it going to be updated? I consider this a "mandatory" addition to scrolling with the iPod wheel since without it, it takes a frustratingly long time to scoll through longer lists.

Thanks,

Larry
Comment by Thomas Paul Diffenbach (tpdiffenbach) - Tuesday, 29 August 2006, 02:07 GMT
It will, give me a little more time.
Comment by Larry L. (lalittle) - Tuesday, 29 August 2006, 06:24 GMT
Thanks Thomas -- I'm relieved to see that you'll be persuing this (and thanks again for taking the time to make such a useful patch.)

Larry
Comment by Larry L. (lalittle) - Monday, 16 October 2006, 14:12 GMT
Thomas -- is this still something that you're working on? I'm just curious since we haven't heard from you in a few weeks now, and a lot of people have been asking about this patch.

Thanks,

Larry
Comment by Robert Keevil (obo) - Monday, 23 October 2006, 20:18 GMT
Sync to CVS

This has just been a copy/paste job, but seems to work.

Removed the list optimisations due to changes to the list code.
button-clickwheel.c is a bit of a mess (indentation wise), and no idea if the wheel position code still works...
Changed the default values, so it's not quite as twitchy.
Some whitespace clean-up.
Comment by Joe Termini (joetermini) - Thursday, 16 November 2006, 21:11 GMT
you need to resync
Comment by Jacob Matthews (tychver) - Monday, 27 November 2006, 08:11 GMT
I believe that this works with the CVS as of a few hours ago on the ipod 5th gen. Haven't had chance for extensive testing yet. Probably breaks all the rockbox conventions. Someone with better knowledge of C needs to take a look.
Comment by Linus Nielsen Feltzing (linusnielsen) - Tuesday, 28 November 2006, 09:32 GMT
The code doesn't look too shabby, except that you should remove the #include "button.h" instead of commenting it. One question: is it really necessary to have the thresholds configurable, or could we find a combination that works for all?
Comment by Jacob Matthews (tychver) - Tuesday, 28 November 2006, 21:36 GMT
Yeah I did think about that, I'd spent quite a long time getting it to compile cleanly and it as getting late, didn't want to go back through it all and edit it again. I'll bear it in mind next time though. The threshholds probably don't need to be configurable, I was thinking about going to a double scroll after one full rotation, a quad scroll two full rotations after that and possibly an eight scroll two full rotations after that. I'm not sure if more than a double and a quad is really necessary though. Over the next eight weeks I'll try and figure out how to port the optimised scroll routine and unroll the loop but I'm learning quite a lot of it as I go so I won't make any promises.
Comment by Jacob Matthews (tychver) - Tuesday, 28 November 2006, 21:45 GMT
Oh and I've discovered that whenever the statusbar is redrawn and no scrolling has yet been done the area of the screen reserved for the display (god knows why it exists) isn't drawn. So that's pretty much whenever the unit it turned on or after moving away from the WPS. It draws fine after scrolling though. I'll try adn fix that too.
Comment by Larry L. (lalittle) - Tuesday, 28 November 2006, 22:06 GMT
Regarding this:

"I was thinking about going to a double scroll after one full rotation, a quad scroll two full rotations after that and possibly an eight scroll two full rotations after that."

This wouldn't be a good approach. It's not the number of rotations that matter -- it's the SPEED of movement on the wheel that should determine the speed. If you're scrolling slowly through a list to look at the titles, you don't want it to suddenely go faster just because you completed more than one rotation -- you need to be able to maintain a slow speed if you desire. With slow movement on the wheel, you want the scroll speed to stay the same. It's when you actually rotate FASTER on the wheel that you want the scroll speed to increase.

PS. Thanks for working in this.
Comment by Jon (ace214) - Tuesday, 28 November 2006, 22:58 GMT
"One question: is it really necessary to have the thresholds configurable, or could we find a combination that works for all?" - Linus

I think that is kind of the ideal of Rockbox, more configuration. I think it would be kind of against the larger principle to have them not configurable. I personally do find they need to be configurable- everyone's used to different things.
Comment by Larry L. (lalittle) - Wednesday, 29 November 2006, 00:51 GMT
I tend to feel that if the speed was tweaked correctly, it really wouldn't need to be configurable. As far as what "correct" means, I'd say that the way the native apple firmware works is "correct." I've never had any desire to have the scroll speeds any different when using the native apple firmware.

I agree that Rockbox should offer lots of configuration, but there is a point of diminishing returns. At some point, more configuration options start to make the system overly complex, which has a negative effect.
Comment by Jon (ace214) - Wednesday, 29 November 2006, 03:02 GMT
That's what default settings are for. Figure out what is "correct," make it the default, and allow people the option to change it.
Comment by Jacob Matthews (tychver) - Wednesday, 29 November 2006, 08:18 GMT
lalittle: That's very true, I can imagine that being a pain. I often find that I scroll fast for about 1/4 a turn and suddenly I've gone up to the first acceleration level and miss where I was aiming and have to now go almost as far straight back up. The default values need tweaking a bit I guess.
Comment by Chris Banes (senab) - Friday, 22 December 2006, 15:41 GMT
Resync ;)
Comment by Chris (decayed.cell) - Monday, 01 January 2007, 14:07 GMT
Needs updating for 01/01/07 CVS :p
Comment by Chris Banes (senab) - Saturday, 06 January 2007, 16:30 GMT
Here's a version that doesn't display the statusbar calculations :)
Comment by Chris (decayed.cell) - Sunday, 07 January 2007, 02:25 GMT
Do I use p1 for this or something?
Comment by Chris (decayed.cell) - Sunday, 07 January 2007, 02:56 GMT
Hmm default values seem a bit out of wack... needs tweaking. Is this ready for CVS? :D
Comment by Chris (decayed.cell) - Tuesday, 16 January 2007, 00:07 GMT
Hmm with current SVN, getting these warnings while compiling

target/arm/ipod/button-clickwheel.c: In function 'ipod_4_g_button_read':
target/arm/ipod/button-clickwheel.c:184 passing argument 3 of 'queue_p' makes integer from pointer without a cast

ipod_scroll_wheel_gui.c:93: warning: return type defaults to 'int'
Comment by Chris (decayed.cell) - Friday, 19 January 2007, 06:44 GMT
Needs updating for SVN again.. damn I wish I knew how to
Comment by Chris Banes (senab) - Sunday, 04 February 2007, 20:54 GMT
Resync ;)

It shows a warning when compiling but it still works.
Comment by Jon (ace214) - Sunday, 11 March 2007, 19:57 GMT
don't really know what to do with this after the menu changes....
Comment by Chris Banes (senab) - Monday, 12 March 2007, 15:07 GMT
Neither have I to be honest, I haven't really had time to have a look at the new menu system though.
Comment by John Martin (CpuWhiz) - Saturday, 24 March 2007, 20:54 GMT
This is my first rockbox hack so be nice :D I updated the patch to work with the latest SVN and tested it on my iPod 30GB 5.5G. First off, I got it working with the new menu, and I must say the way the menus work now with the macros is very simple and a lot less code. I also made a few changes to the code. First off, I put everything inside the same #if statement that I found in about half the patch and added #else where the patch removed code. I removed a few silly patches where it just added/removed some whitespace. I removed the spaces between the statements and the ; (it was driving me crazy). I added a for #if statements for the simulator because I just happened to be using it to test the menu and it was giving me linker errors because it expected a function in the click wheel firmware code (which is not compiled for the simulator). I think that's it. If you don't like it, you can always change it. If anything makes no sense, it's because I was up till past 5:00am working on it, then I couldn't sleep, etc.
Comment by Chris (decayed.cell) - Monday, 26 March 2007, 10:45 GMT
Warning when compiling

target/arm/ipod/button-clickwheel.c: In function ‘ipod_4g_button_read’:
target/arm/ipod/button-clickwheel.c:202: warning: passing argument 3 of ‘queue_post’ makes integer from pointer without a cast

Testing on 30GB 5.5G :p
Comment by Chris (decayed.cell) - Monday, 26 March 2007, 11:58 GMT
Brilliant. It works. A touch too sensitive using default values I think (might be my CPU Frequency patch), but it seems to work quite well.
Comment by Gary Light (evilg123) - Tuesday, 24 April 2007, 22:52 GMT
synced ;) (needs -p1 now if anyone wants to fix it)
Comment by Gary Light (evilg123) - Tuesday, 24 April 2007, 23:57 GMT
sorry, spoke too soon. Patches fine now but throws this error:

filetypes.c: In function ‘read_viewer_theme_file’:
filetypes.c:152: error: ‘struct user_settings’ has no member named ‘viewers_icon_file’
make[1]: *** [/home/g-force/Rockbox/enviro/rockbox-bleeding1/test/apps/filetypes.o] Error 1
make: *** [build] Error 2

Comment by Chris (decayed.cell) - Tuesday, 01 May 2007, 13:32 GMT
I don't even get to there lol

settings.h:729: error: syntax error before ‘}’ token

I'll try sync it
Comment by Gary Light (evilg123) - Wednesday, 02 May 2007, 00:41 GMT
chris, removing the } character on line 729 in settings.h makes the patch apply cleanly, good luck with syncing
Comment by Chris Banes (senab) - Wednesday, 02 May 2007, 09:35 GMT
It was just a }; put in the wring place, meaning the icon weren't being included in the user_settings struct.
Comment by Chris (decayed.cell) - Wednesday, 02 May 2007, 09:46 GMT
Thought so :D
Comment by Thomas Paul Diffenbach (tpdiffenbach) - Friday, 04 May 2007, 06:31 GMT
Hi, original patch author here. First, big thanks to Chris Banes and John Martin for maintaining this patch in my absence. and big thanks to Chris and to "EvilG123" for including it in builds -- I can't build my own right now, so I only have my patch running on my ipod thanks to them. (But John Martin, I /like/ a space before the statement terminating semicolon! :) That's how I distinguish the lines I wrote from everyone elses'. Of course, I remove it in the final official patch to avoid general ire.)

Second, the damned thing is still too choppy, but there's a simple fix that (I think!) makes it less choppy, which I've documented here: http://forums.rockbox.org/index.php?topic=10287.0

Anyone who can (unlike me) actually /do/ a build that proves the change compiles is welcome to post an updated version here. Anyone who actually wants to add menu and settings code to make in user configurable, that would be absolutely brilliant.
Comment by Thomas Paul Diffenbach (tpdiffenbach) - Friday, 04 May 2007, 06:33 GMT
Oh, and that forum post (refeenced in teh above comment) also goes into some of how the scroll accel works, for anyone interested.
Comment by Dieter (dip) - Monday, 02 July 2007, 18:58 GMT
Unfortunately this patch is out of sync.
In my opinion this is one of the most important patches for the iPod, so could anybody with sufficient knowledge please try to sync it. I think a lot of rockbox users who don't have this knowledge (like me) would really appreciate it.
Comment by Travis Tooke (tdtooke) - Monday, 16 July 2007, 04:02 GMT
Quick and dirty sync as they say. button-clickwheel.c needs a bit of a cleanup, but since it wasn't involved with any of the recent code changes I just left it alone for the time being.
Comment by Travis Tooke (tdtooke) - Monday, 16 July 2007, 04:43 GMT
I just noticed something, I forgot to remove things like somefile.c.orig so this will be a little bloated..sorry I'd fix it but to be honest I'm tired and crazy from fooling with this and reading the recent commits page so I'm going to bed.
Comment by Travis Tooke (tdtooke) - Monday, 16 July 2007, 05:48 GMT
Ok, I lied.. Here is a proper diff without all that overhead:
Comment by Travis Tooke (tdtooke) - Monday, 16 July 2007, 06:20 GMT
Last note for the day, really am going to bed this time. If you're using custom_list_position you'll get one hunk failing on apps/gui/list.c. You'll be able to easily see what goes where from the reject. It doesn't break this at all, I just messed around too much with the white space. Oops.
Comment by Travis Tooke (tdtooke) - Tuesday, 17 July 2007, 00:40 GMT
If you use this patch with custom_list_position in ipod_scroll_wheel_gui.c in gui_synclist_handle_accel replace int nb_lines = lists->gui_list[screen].display->nb_lines; with [code]bool use_custom_list=lists->gui_list[screen].display->screen_type==SCREEN_MAIN &&global_settings.listlines;
int nb_lines;
if (use_custom_list) {
nb_lines = global_settings.listlines;
} else {
nb_lines = lists->gui_list[screen].display->nb_lines;
}[/code]From what I can tell by having to add that this patch never worked properly with a custom list. Now it does.
Comment by Travis Tooke (tdtooke) - Tuesday, 17 July 2007, 00:42 GMT
minus out the '[code]' and '[/code]'. I was under the impression those tags would work here..oops. As soon as I can I'll get an updated patch up with that and many cleanup type things.
Comment by Travis Tooke (tdtooke) - Tuesday, 17 July 2007, 02:37 GMT
cleanup
Comment by Travis Tooke (tdtooke) - Tuesday, 17 July 2007, 02:39 GMT
If you use the custom_list_position patch you should apply this after you apply scroll_nostatusbar. I would've liked to have included this change in the original, but if you don't use custom_list_position this causes problems. Really this sort of change belongs in custom_list_position, but having that dependent on a target-specific patch would probably be a no-no.
Comment by Travis Tooke (tdtooke) - Tuesday, 17 July 2007, 07:36 GMT
I'm genuinely embarrassed now looking back at how cluttered I'm making this page.. This is my last update, I mean it this time ;). Slight cleanup and optimization, now should compile with 0 warnings.
Comment by Travis Tooke (tdtooke) - Wednesday, 18 July 2007, 08:27 GMT
Btw, with this latest patch if you use custom_list_position you still need to apply scroll_use_custom_list afterwards.
Comment by Travis Tooke (tdtooke) - Wednesday, 18 July 2007, 10:55 GMT
Final version of my never ending revisions. This patch does not require the helper patch scroll_use_custom_list if you use a custom list and works fine if you don't as well.
Comment by Travis Tooke (tdtooke) - Friday, 20 July 2007, 17:54 GMT
*Real* Final version of my never ending revisions...for now. You really want to upgrade to this one. I noticed in my pursuit of making this fully up with the latest massive landscape changes in list.c that I took out all use of jump->lines in gui_synclist_handle_accel in ipod_scroll_wheel_gui.c which is really unfortunate because that is pretty much the entire point of that method. You might not have noticed any real difference in performance unless you were in a really, really, long list like trying to scroll through the track listing if you're using database. But unfortunately if you were on such a long list and that's kinda the whole point of this patch, you would notice it. My apologies to all on that pretty big oversight, I just got so caught up in what had changed that I didn't pay enough attention to what should've stayed the same. So...if there are any problems at all please let me know, it's becoming a personal grudge match now, me and this patch :)
Comment by Travis Tooke (tdtooke) - Friday, 20 July 2007, 18:20 GMT
Whoops, you guys probably want a patch that will actually add the files you don't have.
Comment by Travis Tooke (tdtooke) - Tuesday, 24 July 2007, 04:22 GMT
resync
Comment by Andree Buschmann (Buschel) - Friday, 07 September 2007, 10:21 GMT
Works fine for me (5.5G 30GB). Patch could be applied very easily without any problem against current trunk (#14627).
Small flaw: The default speeds are too high in my opinion.
Comment by Travis Tooke (tdtooke) - Friday, 14 September 2007, 06:48 GMT
I could do that, but before I do I'd like to request some feedback on what the default speed should be.
Comment by Andree Buschmann (Buschel) - Friday, 14 September 2007, 18:34 GMT
I am working on another solution which uses the same interfacing as the Sansa.  FS#7738 .

Thanks,
Andree

Loading...