Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface
  • Assigned To No-one
  • Operating System iPod 5G
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes 5
  • Private
Attached to Project: Rockbox
Opened by tpdiffenbach - 2006-06-24
Last edited by nicolas_p - 2007-10-11

FS#5594 - Ipod scroll acceleration patch

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

Closed by  nicolas_p
2007-10-11 22:46
Reason for closing:  Out of Date
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Superceded by  FS#7738 

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

It will, give me a little more time.

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

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

obo commented on 2006-10-23 20:18

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.

you need to resync

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.

Project Manager

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?

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.

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.

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.

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

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.

That’s what default settings are for. Figure out what is “correct,” make it the default, and allow people the option to change it.

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.

senab commented on 2006-12-22 15:41

Resync ;)

Needs updating for 01/01/07 CVS :p

senab commented on 2007-01-06 16:30

Here’s a version that doesn’t display the statusbar calculations :)

Do I use p1 for this or something?

Hmm default values seem a bit out of wack… needs tweaking. Is this ready for CVS? :D

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’

Needs updating for SVN again.. damn I wish I knew how to

senab commented on 2007-02-04 20:54

Resync ;)

It shows a warning when compiling but it still works.

don’t really know what to do with this after the menu changes….

senab commented on 2007-03-12 15:07

Neither have I to be honest, I haven’t really had time to have a look at the new menu system though.

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.

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

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.

synced ;) (needs -p1 now if anyone wants to fix it)

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

I don’t even get to there lol

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

I’ll try sync it

chris, removing the } character on line 729 in settings.h makes the patch apply cleanly, good luck with syncing

senab commented on 2007-05-02 09:35

It was just a }; put in the wring place, meaning the icon weren’t being included in the user_settings struct.

Thought so :D

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.

Oh, and that forum post (refeenced in teh above comment) also goes into some of how the scroll accel works, for anyone interested.

dip commented on 2007-07-02 18:58

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.

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.

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.

Ok, I lied.. Here is a proper diff without all that overhead:

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.

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.

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.

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.

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.

Btw, with this latest patch if you use custom_list_position you still need to apply scroll_use_custom_list afterwards.

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.

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

Whoops, you guys probably want a patch that will actually add the files you don’t have.

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.

I could do that, but before I do I’d like to request some feedback on what the default speed should be.

I am working on another solution which uses the same interfacing as the Sansa.  FS#7738 .

Thanks,
Andree

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing