Rockbox

documentation (how to install, how to use, programmers guide) in the rockout rar file.

Did you forget the patch?

Some quick comments:

1) rockbox style guidelines. They're in docs/CONTRIBUTING. No //, no tabs, 80 characters per line, etc. Take a look.

2) Its really hard to read that because the spacing is broken. If you could go through, fix the tabluation, and then set your text editor to save tabs as 4 spaces it would be much easier to figure out your code.

3) You have an enormous case statement where almost every case is identical. Unless I'm missing something, of those 300 odd cases, you really only need BUTTON_REPEAT, BUTTON_RELEASE and a null one for neither, and then some pointer arithmetic or even if statements in each case to set or zero the flags. You could cut down the size of that patch by perhaps a thousand lines if you got rid of all the redundant ones.

Hi selectohh,

First, thanks for contributing this plugin! I'd love to see this included in Rockbox. To put my (and saratoga's) comments into context, please remember that the people who criticize your work are the ones interested in it. Figuring that out when I started contributing to Rockbox took me a while. :)

Here are some more comments:

Thanks for including documentation with your patch; that was helpful.

I see nothing in principle that would prevent at least basic support for other players that have enough RAM (RaaA, all AMSv2 players?), except the per-player configuration of the key mapping and the key-repeat timeout.

The patch is missing the required changes to SOURCES or to makefiles (if any).

You are licensing the code under a noncommercial CC license. Are you dual-licensing the code alternatively to the GPL, which you also mention? If not (and the noncommercial restriction is in addition to the GPL), the code cannot be included in Rockbox.

I agree with saratoga that the coding needs to be improved. There is lots of repetition, loops that check for each loop index separately in their body, nondescriptive variable names, hardcoded constants sprinkled throughout the code, etc. Also, the memory management looks rather ad-hoc; consider using buflib.

Some more general remarks:

If you'd like developers to be constructive and productive with your work, you need to optimize for their time investment. Here are some tips:
* Follow saratoga's advice regarding the coding style. Also make sure the patch does not contain DOS line endings (CRLF).
* Convert the developer and user documentation to plain text, and upload them and the patch as separate attachments, rather than asking them to use tools like Rar and Word.
* Consider splitting the code into multiple source files.
* Remove profanity from the code and documentation.

eh ok thank you for all that. my first c program and my first attempt at a plugin so that is my defense. will see about following as many of these things as possible

sideral is 100% correct. The advice given here is not designed to put you off contributing, but just to ensure the consistency of the Rockbox code.
Please don't think that you should be on the defensive.
Also, I have to say that I haven't yet tried this patch but it sounds extremely interesting! So thanks for providing it and I hope that you continue to contribute.

here is the new patch. i hope this fixes:
a) 80 chars per line
b) unix newlines \n instead of \r\n
c) tabs are all unified and i'm think if i fixed the tabs→spaces problem, i think i did but i can't "see" them
d) no more // comments (only /* */)
e) no more swearing in code/docs
f) docs are now .txt and not word format
g) patch should have SOURCES file diff included (i cut out a chunk of this diff file, i had it on autorock, hopefully it still works + doesn't have formatting from resaving)
h) files are now outside of a rar file.. well.. the code will be. how do you suggest i upload the minimum required samples folders

not fixed:
a) the superlong buttondown case.
b) support for other players that have enough RAM (RaaA, all AMSv2 players?)
c) splitting code into separate files
d) the non-commercial licence. why isn't that CC licence acceptable? is rockbox considered "commercial use"?

i will probably be back to fix b) and d) of the "not fixed".

thanks for your suggestions, please let me know if there's others or if the above fixes didn't work. i suppose it was discouraging to hear all the criticisms but thankfully a good number of them are formatting. although i'm sure if/when you look at the code you will be quite unhappy with my sloppy programming :)

   rockoutv1_1.diff (156.9 KiB)
   rockout v1 user guide.txt (7.2 KiB)
   rockout v1 developers guide.t... (6.7 KiB)

why isn't that CC licence acceptable?

Your plugin will link against Rockbox, which is GPL, and so must also be GPL. You can dual license of course, but since CC code cannot be used with GPL projects like rockbox, it would be fairly pointless.

Hi selectohh,

Thanks, the latest patch is much easier to read! Please don't be shy with your code. I'd say that for a first C program, this plugin is quite an achievement!

I wouldn't waste time porting this to other players just yet.

Instead, I suggest you focus next on removing the code's redundancy. At first glance, it looks like almost all the case differentiations (switch and chained if statements) could be replaced by a single code instance with a lookup table and a little pointer or index arithmetic.

For example, instead of differentiating among stolen_audio_buffer, stolen_audio_buffer1, stolen_audio_buffer2, and so on, you can have a *stolen_audio_buffer[] array indexed with the buffer number. Another example that springs to mind are raw_file{,1,2,3,4,5} variables. Once you know how to factor out common code, you can tackle the huge switch(button_get) statement.

A minor comment: The patch to SUBDIRS is not needed.

Regarding the license: The GPL is incompatible with a noncommercial license because the GPL does not restrict commercial applications, but explicitly disallows restricting the rights granted by the GPL in derived works. That's why it's often said the GPL is “viral”: GPL'd works can only be combined with works under other licenses that put less restrictions on licensing than the GPL itself, and doing so effectively GPLs the resulting derived work.

hi sideral, thanks! will do. thanks for being specific about what could be changed it makes it easier for me to understand how to go about it. not sure how much time i have right now but worst case in a month i should have time

Please update your Flyspray profile to include your real name. We have a real name policy and people are not going to search after your real name. If not this task is likely to get rejected because of no real name soon.