`icemulti' improvements

[rlutz]

This branch contains several improvements to the icemulti tool. Most notably, the option -P has been added which specifies the image to be used on power-on/reset by filename. This allows to specify an additional, fifth image for power-on/reset which is different from the four images available via the warmboot mechanism.

According to this change, the image used on power-on/reset is now always packed first in the output bitstream, even if it is not the first "regular" image. This is for the sake of consistency: If an arbitrary fifth image was packed first, but an image included as a regular image wasn't, icemulti would sometimes create a bitstream with the power-on/reset image first and sometimes a bitstream with the first "regular" image first. This is something the user wouldn't expect and, therefore, a bad thing.

Further improvements:

• In analogy to the -p/-P option, a -d/-D option has been added which allows specifying the "default" image to be used for the omitted positions if less than four images have been specified.
• Images are now only included once in the output even if they are referenced multiple times.
• Specifying a power-on/reboot image with coldboot enabled now merely causes a warning. The power-on/reset image is ignored in this mode, but the resulting output bitstream is valid, so there's no reason why the user shouldn't be allowed to specify it.
• Added proper error checking
• Added test suite
• Added option --help

Also, a top-level target check has been added which runs the test suite on all programs built by make all. The icebram tests require additional tools, so they are skipped if the required tools aren't installed.

[rlutz]

Please note that, for some reason, GitHub doesn't preserve the order of the commits when displaying a pull request. You can view them in the correct order on the branch page or using a command like git log or gitk.

[cliffordwolf]

This PR touches more than 160 files. Please split it up in smaller pull requests that reasonably can be reviewed and discussed.

[rlutz]

There are two reasons why this pull requests touches many files:

• It adds a test suite for icemulti which consists of 75 files.
• It contains commit 69a15c1 which removes whitespace in 87 files.

I included commit 69a15c1 at the beginning of the pull request because it completes commit b3d35cc, so it makes sense if two are close to each other in the commit history. I could move it to a separate pull request, but this would create merge conflicts when merging the branches.

As a workaround, I created pull request 91 which contains only that commit. Based on that commit, this pull request doesn't contain whitespace changes.

Removing the test suite from the pull request would be possible but would defeat the purpose of the test suite, which is making sure the subsequent changes don't break anything in the code. If you want to see the changes excluding the test suite, just look at the file icemulti.cc; all other non-whitespace changes are related to tests.

[rlutz]

Since commit 69a15c1 appears to be controversial, I removed it from this pull request.

[ArcaneNibble]

Please do not use err.h. These functions are not available when building for Windows using MinGW. In fact, according to this man page, "these functions are nonstandard BSD extensions."

[martinda]

@rlutz, in commit d742375, I do not understand what you mean by "re-use" images (even after reading the code). Can you explain more please?

[martinda]

@rlutz, in commit 738e3ae, what is the advantage of making coldboot a global flag?

[martinda]

@rlutz, in commit 0dda53c, what is the benefit to making write_header a global function?

[martinda]

@rlutz, in commit 5c15c93, what is the benefit to populating headers early?

[martinda]

@rlutz, I am confused... commit 5878b1d seems to continue the work of commit 5c15c93. I feel like these commits are the train of thoughts you've gone through since you started working on icemulti.

[martinda]

@cliffordwolf, @rqou: Hi. I just want to introduce myself briefly. I am co-mentoring Roland for this work which is part of GSoC 2017. For full transparency, and as you have surely guessed, I am not a natural C/C++ coder, and I am not an icestorm user. The first time I heard about icestorm was when I volunteered to co-mentor Roland.

@rlutz, I have completed my reading of the commits.

IMO a better way to present these changes would have been to create multiple pull-requests, gradually over time, rather than submit all of them all at once.

My view at this time is that there are a lot of commits that simply aim to make the code suit your preferences, but as it is my personality to try to make code as perfect as I can (proper variable names, complete help, use of getopt, error checking, best practices, etc.), I tend to like the changes. However, because this is not my project, I am only voicing a non-authoritative opinion.

Regarding the functionality addition of a power-on reset image, I hope that it is useful to users.

Regarding the tests, I think they are always a great addition to any project.

I leave it up to the core maintainers to decide what to do with the PR.

[cliffordwolf]

@martinda I think at least most of this PR is stuff that would be useful to users in one way or another, but I'm not going to review a big single blop like that. If @rlutz splits it up into reasonably small pieces that can be reviewed and (if necessary) discussed individually then I'll be happy to look into those smaller PRs.

[rlutz]

in commit d742375, I do not understand what you mean by "re-use" images (even after reading the code). Can you explain more please?

When the same file name is listed multiple times on the command line, icemulti used to include the image multiple times in the output. This is unnecessary since multiple headers can point to the same image, which already is the case with the power-on/reset image (which isn't included twice even though at least two headers point to it) and unspecified images (whose headers just point to the power-on/reset image, too).

Since there's no point in including images multiple times, it results in larger output bitstreams, and even more importantly, there's no useful concept of when to include an image multiple times and when not if treating all five possible images equally, I changed the code in a way that allowed to detect identical file names and include the images only once.

[rlutz]

in commit 738e3ae, what is the advantage of making coldboot a global flag?
in commit 0dda53c, what is the benefit to making write_header a global function?

These are preparations for removing the class Header in commit 9bcf6db, which in turn is a preparation for the final commit in the arc, d742375 ("icemulti: Re-use images").

in commit 5c15c93, what is the benefit to populating headers early?

In order to implement re-using images in a clean way, I needed to separate the concept of an image slot from the concept of an actual image, so multiple image slots could point to the same image. The old code

for (int i=0; i<image_count; i++)
    header_images[i + 1] = &*images[i];

assumed a 1:1 connection between the members of images and the members of header_images. This isn't true any more as of commit d742375. Moving the population of header_images to the main image processing loop allowed me to have a new slot (header_images member) point to an older image (images member).

I am confused... commit 5878b1d seems to continue the work of commit 5c15c93. I feel like these commits are the train of thoughts you've gone through since you started working on icemulti.

Actually, the commits in this line of thought are:
5c15c93 – move the code to where I need it
fa32bae – use a new variable header_count which can be different from image_count
d742375 – only create a new image if it's different from all existing images
5878b1d – move image loading to a separate function

[rlutz]

IMO a better way to present these changes would have been to create multiple pull-requests, gradually over time, rather than submit all of them all at once.

The problem is that most changes build on top of each other, and GitHub won't let me create a pull request for anything that isn't based on current upstream code. In principle, the individual commits already are pull requests on their own: I took great care to keep each commit as small as possible while having it cover exactly one idea and leaving the project in a valid state. The test suite passes after every commit. In theory, Clifford could look at each commit in turn, pick those which he wants, and drop the others.

I think the problem here is the broken GitHub interface for pull requests. Linus Torvalds wrote about it:

Git comes with a nice pull-request generation module, but github instead decided to replace it with their own totally inferior version. As a result, I consider github useless for these kinds of things. It's fine for hosting, but the pull requests and the online commit editing, are just pure garbage.

I've told github people about my concerns, they didn't think they mattered, so I gave up. Feel free to make a bugreport to github.

Since Clifford still wants to use it and doesn't want to look at the individual commits, and since I can't split the commit history into multiple pull requests because GitHub won't let me, I only see one solution: continue to look at the commits a few at a time, discuss them, and proceed once the commits are merged. It's a slow method, but I don't know what else to do. If you have another idea, please let me know.

[rlutz]

My view at this time is that there are a lot of commits that simply aim to make the code suit your preferences, but as it is my personality to try to make code as perfect as I can (proper variable names, complete help, use of getopt, error checking, best practices, etc.), I tend to like the changes.

I wouldn't call documentation, argument checking and error checking a matter of preferences…

[martinda]

@rlutz, could you merge the 5c, fa, d7, 58 commits together (we do not require to see the details of the train of thoughts), and organize the rest to form 3 or 4 pull-requests? Maybe you could push them to different branches that branch off one another (since they build on top of one another)? I am only trying to help... and I don't know github that well that much.