Add argument to makefile to simplify Continuous Integration flow

[torerams]

I am using the neorv32 as a git submodule in a project where I compile both bootloader and application into VHDL modules in a CI flow (using a Jenkins server).
I have the source code if my application in my own repository and use the "make install" and "make bootloader" to compile and make local VHDL files in my repository.
But the 2 VHDL files (neorv32_bootloader_image.vhd and neorv32_application_image.vhd) are copied to the /rtl/core folder in the neorv32 repository messing up source control (and the CI flow) for this repository.

There are several workarounds for this (and I am using several), but a clean way to deal with this is to have an optional makefile argument to support CI flow better.

This PR adds a FLOW argument to the sw/common makefile (common.mk). Making it easy to use CI flow for your own projects.

To compile as before just use the same commands as before:

make install
make bootloader

To make bootloader and application in a CI flow:

make install FLOW=CI
make bootloader FLOW=CI

These commands will not change any source files in the neorv32 repository.

[umarcor]

While I understand the motivation and I agree that it can be improved, I am unsure about the specific solution/implementation in this PR. The fact that building/compiling the software and updating it implies modification of VHDL sources is undesirable. That is modificating the state of the repo unnecessarily. That is a problem for CI, but also for local usage. When testing multiple boards with different software, the software changes need to be handled in temporary commits, in order to avoid conflicts. That requires good knowledge of git and it can be cumbersome for less experienced users.

I think a reasonable solution would be not to check neorv32_application_image.vhd and neorv32_bootloader_image.vhd into the SCM. Since those are auto-generated, there is no need to track them. Moreover, forcing the users to compile and install one bootloader and one software application makes it explicit that we are dealing with a soft CPU and software cannot be ignored.

Alternatively, *_image.vhd sources should read an HEX file from a path given through a generic of type string. That would make the VHDL sources agnostic to the software. #35 is where we tracking this approach.

The third option would be to modify the software makefiles as done in this PR. Currently, $(APP_IMG) implies "build img and install it". IMHO, it'd be desirable to split that into two targets: one for building and one for installing (which depends on the former). Then, users willing to ignore the last part would be able to use a different makefile target, without introducing an additional variable. So, for instance:

image: $(APP_ASM) $(APP_IMG)
install: image install-$(APP_IMG)

# Generate NEORV32 executable VHDL boot image
$(APP_IMG): main.bin $(IMAGE_GEN)
	@set -e
	@$(IMAGE_GEN) -app_img $< $@ $(shell basename $(CURDIR))

install-$(APP_IMG): $(APP_IMG)
	@set -e
	@echo "Installing application image to $(NEORV32_RTL_PATH)/$(APP_IMG)"
	@cp $(APP_IMG) $(NEORV32_RTL_PATH)/.

[torerams]

The 3rd solution from @umarcor seems like a better solution then mine, and would work nicely in my CI setup.
No negative side effects for current users as far as I can see.

[umarcor]

@torerams feel free to update this PR with that snippet!

[stnolting]

@torerams
I also understand the problem (I have the same troubles) but as @umarcor already indicates this might be too specific.

@umarcor

I think a reasonable solution would be not to check neorv32_application_image.vhd and neorv32_bootloader_image.vhd into the SCM. Since those are auto-generated, there is no need to track them. Moreover, forcing the users to compile and install one bootloader and one software application makes it explicit that we are dealing with a soft CPU and software cannot be ignored.

I know this is not pretty, but I like the concept that everything is already there so a synthesis will work out of the box.

Alternatively, *_image.vhd sources should read an HEX file from a path given through a generic of type string. That would make the VHDL sources agnostic to the software. #35 is where we tracking this approach.

😅 👍

The third option would be to modify the software makefiles as done in this PR. Currently, $(APP_IMG) implies "build img and install it". IMHO, it'd be desirable to split that into two targets: one for building and one for installing (which depends on the former). Then, users willing to ignore the last part would be able to use a different makefile target, without introducing an additional variable. So, for instance:

I like this, too!
However, if I understand this correctly, this will only effect the "application image" and not the bootloader image. Hence, I would suggest to add/change some makefile targets:

• image: create local-only VHDL init image (application / IMEM) -> neorv32_application_image.vhd
• bl_image: create local-only VHDL init image (bootlader / BOOTROM) -> neorv32_bootloader_image.vhd
• install: maybe check if neorv32_application_image.vhd and/or neorv32_bootloader_image.vhd are already available in the local folder and copy them to rtl/core when available; print some message if none is available, but do not call the according generate target if they are not generated yet

The default all target would do image and install. The bootloader target would do bl_image and install.

[stnolting]

I have modified the makefile to support some new targets:

• image: create local application VHDL memory initialization file
• image_bl: create local bootloader VHDL memory initialization file
• install: copy all VHDL memory initialization files to rtl/core (if available)

I have also updated the default targets accordingly. If the modifications work as expected I can update this PR if you like.

-> common.mk.txt (changed file ending due to upload constraints)

[torerams]

I like the new split into image and install but I am not that exited about the install target only copying old files.

I see a lot of down-side of this approach as this will break the current flow for all/most users and scripts. I also expect there will be some people spending hours debugging before realizing that the install target copies old files, and are not using the source files they have just updated. I don't see much (if any) up-side with this approach.

I really like @umarcor solution where:

• target image build new VHDL image file in local folder
• target install build new VHDL image file in local folder and copies VHDL image file to /rtl/core

With this solution everything is working as before when using the install target, but users can (not must) use the image target to build a local VHDL image file only.

[umarcor]

I guess what @torerams is suggesting comes down to adding the dependencies to the install target. I believe it was just an oversight by @stnolting since he was probably more focused on the app/bootloader stuff (which I missed in the snippet).

[stnolting]

I guess what @torerams is suggesting comes down to adding the dependencies to the install target. I believe it was just an oversight by @stnolting since he was probably more focused on the app/bootloader stuff (which I missed in the snippet).

Yes and no 😄
I wanted to de-couple image generation and image installation. I did not think about that this would crash the current function of the install target as @torerams has noted.

So yeah, maybe the easiest way would be to have a new image target that creates the local application image and use install to copy that (and use image as dependency).

Same with the bootloader: image_bl and install_bl ( keep current bootloader target as an alias for install_bl)

I think this would be more straightforward while also being backwards compatible.
What do you think?

[torerams]

I have updated the PR with the solution we have discussed.
Seems to be working fine when I am testing, but my makefile skills are limited (code-by-example), so feel free to check/comment.

[stnolting]

@torerams
Great! I just fixed the all target to be backwards compatible. Furthermore, all bootloader-ralted target require additional gcc flags and symbols for linking and crt0.S (I know this is a dirty workaround, but it is the best I could come up with).

I think this can be merged, if I did not broke anything, right?

[torerams]

Thank @stnolting for fixing up the PR. Everything seem to be working fine with my testing, and the PR is ready for merging.