riotboot: add RIOTBOOT_BUILD make var

[fjmolinas]

Contribution description

This PR attempts to fix the first part of #12003

• Introduced by bb71e97
  • the file use when flashing is not the bootloader firmware but an extended bootloader with a slot0 bootloader. So riotboot/flash-bootloader would overwrite slot0 and slot1 header.

With the pseudomodule we can specify that the bootloader application is using to bootloader itself, and normal applications aren't, they are just using riotboot modules or functionality.

This allows defining FLASHFILE as a regular ELFFILE for the bootloader and keeping the current behavior where we wan't it, i.e. flashing an application + boot-loader using riotboot.

All boards providing riotboot now provide riotboot_bootloader.

Testing procedure

• Verify this fixes the issue:

BOARD=iotlab-m3 make --no-print-directory -C bootloaders/riotboot/ info-debug-variable-FLASHFILE
/home/francisco/workspace/RIOT/bootloaders/riotboot/bin/iotlab-m3/riotboot.elf

• Previous behavior is kept

BOARD=iotlab-m3 make --no-print-directory -C tests/riotboot/ info-debug-variable-FLASHFILE
/home/francisco/workspace/RIOT/tests/riotboot/bin/iotlab-m3/tests_riotboot-slot0-extended.bin

Issues/PRs references

Addresses #12003
Related to #12302

[kaspar030]

Why not simply ifneq $(riotboot, $(APPLICATION))... ?

[fjmolinas]

I had posted this as an alternative in #12003, I discussed it with @cladmi and we though that you shouldn't look at the APPLICATION variable to change build behavior. Maybe @cladmi has another argument I don't recall?

[kaspar030]

I don't see why not? Do we expect people calling their application "riotboot"?

Anyhow, an alternative to having to add the feature everywhere would be to set "BUILDING_RIOTBOOT=1" in bootloader/riotboot/Makefile and check for that.

[kaspar030]

Or re-use "-DRIOTBOOT" as in "ifneq (,$(filter -DRIOTBOOT,$(CFLAGS)))...".

[kaspar030]

having to add the feature everywhere

I cannot imagine @cladmi would accept use of FEATURES for that, anyways. 😉

[fjmolinas]

I cannot imagine @cladmi would accept use of FEATURES for that, anyways. wink

That can be correct, he may not like any of the solutions I had proposed.

[kaspar030]

Anyhow, before, we introduced "FLASHFILE = $(RIOTBOOT_EXTENDED_BIN) ". This is wrong in the bootloader case, as riotboot itself behaves like a normal application. So we need to get the information that we're building the bootloader into that file.
Using a feature might seem like a straightforward idea, but that forces us to make changes to completely unrelated files (board / cpu / *). Those might even be external, and they grow.
So that's a bad idea in any case.

Adding a simple "BUILDING_RIOTBOOT=1" to the riotboot makefile can do the same, without the need to add a line everywhere else.

Is makefiles/boot/riotboot.inc.mk" actually supposed to be included when building riotboot itself? Maybe we can skip including it altogether in that case.

[fjmolinas]

Is makefiles/boot/riotboot.inc.mk" actually supposed to be included when building riotboot itself? Maybe we can skip including it altogether in that case.

I don't think it does, the target riotboot/flash-bootloader does though. But we would still have to change the trigger for including makefiles/boot/riotboot.inc.mk since right now it is still FEATURES_USED += riotboot. It would come down to the same kind of solutions but done elsewhere. But in this case we would avoid including uneeded targets so your approach sounds better to me.

[fjmolinas]

Adding a simple "BUILDING_RIOTBOOT=1" to the riotboot makefile can do the same, without the need to add a line everywhere else.

Ok, this does sound more simple. I still don't like calling it RIOTBOOT, but I know you don't agree with this as stated in #11940. So I'll change it to your suggestion.

[fjmolinas]

@kaspar030 changed do implement your approach and I'm checking APPLICATION, I know I'm changing my mind here but I disliked the idea of adding another variable to specify we are building the bootloader.

@cladmi I know this isn't great, but i wasn't a fan of checking CFLAGS or creating yet another variable to tell we are building the bootloader.

[cladmi]

If you think APPLICATION is a variable than can be used to configure any different behavior of the build, please document this in makefiles/vars.inc.mk for consistency.

But as CFLAGS (not even considering evaluating CFLAGS can slow down the build), it goes for me against the dependency inversion principle, but as I will not maintain it is your choice

My issue with doing something with "it's the bootloader", is that then the behavior is not triggered by enabling a feature, or a module, but enabled even when not needed except when we disable it after.
I prefer only add things than blacklist for configuration.

If only the feature riotboot is not enough to differentiate between a bootloader and an application, anymore, it needs another name. Similar to saying "I am a bootloader", but from the application side to not blacklist.
I would change that application that wants to be build for riotboot, should say USEMODULE += riotboot_slot or something (except that this one clashes with an existing module).

[fjmolinas]

@cladmi I pushed a different approach which I think makes sense respecting your thoughs, if agreed I will also update the documentation. The idea is that now riotboot is treated as module as anyother so it doesn't trigger any special build handling or recipes.

Application that want more than that, i.e. compile for riotboot should do FEATURES_REQUIRED+=riotboot instead of USE_MODULE+=riotboot%.

@kaspar030 what do you think about this?

PS: I have to change in the commit that riotboot is in caps right now.

[fjmolinas]

@cladmi I pushed a different approach which I think makes sense respecting your thoughs, if agreed I will also update the documentation. The idea is that now riotboot is treated as module as anyother so it doesn't trigger any special build handling or recipes.

Application that want more than that, i.e. compile for riotboot should do FEATURES_REQUIRED+=riotboot instead of USE_MODULE+=riotboot%.

@kaspar030 what do you think about this?

PS: I have to change in the commit that riotboot is in caps right now.

This actually doesn't work since without FEATURES_REQUIRED we can't filter out CPU's /BOARD's that don't support this... I will rollback on the approach or think of a new one.

[fjmolinas]

@kaspar030 adapted to one of your first suggeestion, can you take a look?

[kaspar030]

@kaspar030 adapted to one of your first suggeestion, can you take a look?

IMO this is straight forward.

[fjmolinas]

IMO this is straight forward.

Is that an ACK?

[fjmolinas]

ping @kaspar030 !

[kaspar030]

Is that an ACK?

Yes, ACK. Tests still compile & run as expected.