riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used #11690

kaspar030 · 2019-06-13T10:25:46Z

Contribution description

Previously, even an application that had "FEATURES_REQUIRED += riotboot"
set would still flash the non-riotboot binary on "make flash".
This is usually not what the user wants.

This commit set's the FLASHFILE variable to the combined "riotboot
bootloader + slot0 + empty slot1" binary, if the riotboot feature is used.
This has the effect that "make all", "make flash" and "make flash-only" will compile and/or flash a working riotboot setup.

This was previously done for tests/riotboot within that application's Makefile. This PR moves the definition into makefiles/boot/riotboot.inc.mk, guarded by whether the riotboot feature is used.

Testing procedure

confirm that non-riotboot flashing still works (e.g., by flashing hello-world)
run "make flash" in tests/riotboot. It should flash the combined binary

Issues/PRs references

kaspar030 · 2019-06-13T11:48:54Z

fixed some issues @cladmi pointed out IRL

cladmi · 2019-06-13T11:59:57Z

"riotboot: use riotboot if feature is used" The PR title and commit message should maybe be more precise and say "use the riotboot extended firmware" or any other descriptive name.

This was not done at the beginning as FLASHFILE was not supported for all boards and required hacks. Doing it globally before it was supported could have led to overwriting both ELFFILE, HEXFILE, BINFILE and cause recursive definitions issues

Now it should be safe to do.

kaspar030 · 2019-06-13T12:07:30Z

"riotboot: use riotboot if feature is used" The PR title and commit message should maybe be more precise

agreed. Changed to what the commit actually does.

kaspar030 · 2019-06-13T14:13:04Z

@cladmi could you take another look?

cladmi · 2019-06-17T12:04:36Z

This is currently changing multiple things in the same PR without doing them completely and giving testing for it.

This changed without justifying the firmware flashed for tests/riotboot.
Even if it is a needed change it is not explained why.

The documentation still recommends to use riotboot/flash-combined-slot0 for flashing instead of using flash directly.

The documentation update for the previous APP_VER api change is not complete as many examples still say to use APP_VER=$(date +%s).

kaspar030 · 2019-06-17T14:00:25Z

This changed without justifying the firmware flashed for tests/riotboot.
Even if it is a needed change it is not explained why.

added a paragraph to the commit description

The documentation still recommends to use riotboot/flash-combined-slot0 for flashing instead of using flash directly.

... which is not wrong. Anyhow, changed it to say "make flash".

The documentation update for the previous APP_VER api change is not complete as many examples still say to use APP_VER=$(date +%s).

Which is also not wrong. I piggybacked the fix for actually wrong information, I can kick it out if you insist. I do not want to have more unrelated fixes in here.

kaspar030 · 2019-06-18T11:48:17Z

@cladmi are your comments sufficiently addressed?

cladmi · 2019-06-18T12:15:33Z

makefiles/boot/riotboot.mk

+# make applications that use the riotboot feature default to actually using it
+# Target 'all' will generate the combined file directly.
+# It also makes 'flash' and 'flash-only' work without specific command.
+ifneq (,$(filter riotboot, $(FEATURES_USED)))


This guard is duplicating the one from the file. So is not necessary.

fjmolinas

Tested it on samr21-xpro, it works as the PR documentation states. As I can see the required documentation has been updated but not in tests/riotboot for consistency I think it should be just as bootloaders/riotboot/README

My only concern is that now there is no way of flashing just the application without riotboot, right?

kaspar030 · 2019-06-20T13:37:47Z

My only concern is that now there is no way of flashing just the application without riotboot, right?

That might indeed be an issue. I'll try to come up with something.

kaspar030 · 2019-07-01T09:27:44Z

My only concern is that now there is no way of flashing just the application without riotboot, right?

Do you mean e.g., only flash slot0 or slot1? That's still possible using make riotboot/flash-slotX.
Or do you mean flashing a non-riotboot version?

fjmolinas · 2019-07-01T09:30:48Z

Do you mean e.g., only flash slot0 or slot1? That's still possible using make riotboot/flash-slotX.
Or do you mean flashing a non-riotboot version?

Non-riotboot version.

kaspar030 · 2019-07-01T09:47:58Z

Non-riotboot version.

Ok, this would require disabling usage of an available and optional or required feature. It would probably need a new variable like "FEATURES_DISABLED", which will make people scream "YOU CANNOT DISABLE WHAT THE HARDWARE ALWAYS PROVIDES!".

Unless we had a strong use-case that is now not supported anymore, I suggest we postpone this.

fjmolinas · 2019-07-01T14:25:46Z

Ok, this would require disabling usage of an available and optional or required feature. It would probably need a new variable like "FEATURES_DISABLED", which will make people scream "YOU CANNOT DISABLE WHAT THE HARDWARE ALWAYS PROVIDES!".

True.

Unless we had a strong use-case that is now not supported anymore, I suggest we postpone this.

Ok, I agree. @cladmi do you still have issues with this?

fjmolinas · 2019-07-03T08:54:50Z

I'm OK with the changes, but since @cladmi requested most of them I would like for him to ACK that his concerns are addressed.

kaspar030 · 2019-07-04T12:46:34Z

@cladmi ping

kaspar030 · 2019-07-05T09:11:54Z

I realized that "bootloaders/riotboot/README.md" also had outdated testing instructions. I've replaced them with a pointer to "tests/riotboot/README.md".

I think this is good to go, could someone take a look?

fjmolinas · 2019-07-08T12:25:09Z

@cladmi have your issues been addressed?

fjmolinas

I have a comment on your last change.

bootloaders/riotboot/README.md

kaspar030 · 2019-07-08T14:14:03Z

Two more boards got riotboot support, but are too small to fit the test in half the flash. Pushed a commit adding them to the blacklist.

fjmolinas

Changes look good to me, I tested that changes didn't affect normal applications and now any application using FEATURES_REQUIRED+=riotboot will by default use slot0-extended when using flash and flash-only.

... if the riotboot feature is used. Previously, even an application that had "FEATURES_REQUIRED += riotboot" set would still flash the non-riotboot binary on "make flash". This is usualy not what the user wants. This commit set's the FLASHFILE variable to the combined "riotboot bootloader + slot0 + empty slot1" binary. This has the effect that make all, flash and flash-only will compile and/or flash a working riotboot setup. tests/riotboot and tests/riotboot_flashwrite now default to flashing the riotboot-extended binary. tests/riotboot was previously configured to use the riotboot-combined binary. This has been changed in order to not behave differently than how usual riotboot applications do.

FLASHFILE is now set to RIOTBOOT_EXTENDED_BIN, changing the meaning of make all`, `flash`, `flash-only`. This commits updates the documentation accordingly.

Adding saml1[01]-xpro, nucleo-f302r8|f334r8 to BOARDS_INSUFFICIENT_MEMORY. The riotboot build fails because it only offers half the flash size (per slot), compared to before where the default build would not use riotboot at all, thus have double the flash (but being useless).

cladmi · 2019-07-08T15:37:30Z

Comments were addressed.

fjmolinas · 2019-07-08T16:02:58Z

GO!

kaspar030 added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jun 13, 2019

kaspar030 requested a review from cladmi June 13, 2019 10:25

kaspar030 changed the title ~~riotboot: use riotboot if feature is used~~ riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used Jun 13, 2019

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from 1e31f40 to e956ef8 Compare June 13, 2019 12:06

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from 91ea9cb to 9a7ac3f Compare June 17, 2019 14:00

kaspar030 mentioned this pull request Jun 17, 2019

tests/riotboot: add automatic test #11707

Merged

cladmi reviewed Jun 18, 2019

View reviewed changes

fjmolinas reviewed Jun 19, 2019

View reviewed changes

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from e90b896 to 924b52f Compare July 5, 2019 09:10

fjmolinas mentioned this pull request Jul 8, 2019

tests/riotboot_flashwrite: add python test script #11808

Closed

kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 8, 2019

DEPENDENCY MERGE: pr/always_flash_riotboot_if_required (RIOT-OS#11690)

669f902

kaspar030 mentioned this pull request Jul 8, 2019

sys/suit: initial support for SUIT firmware updates #11818

Merged

fjmolinas reviewed Jul 8, 2019

View reviewed changes

bootloaders/riotboot/README.md Outdated Show resolved Hide resolved

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from 924b52f to bd32bee Compare July 8, 2019 13:59

fjmolinas approved these changes Jul 8, 2019

View reviewed changes

kaspar030 added 3 commits July 8, 2019 16:33

riotboot: update documentation to match automatic use

8624d3d

FLASHFILE is now set to RIOTBOOT_EXTENDED_BIN, changing the meaning of make all`, `flash`, `flash-only`. This commits updates the documentation accordingly.

riotboot: fix APP_VER default documentation

b3cc3ab

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from 7166099 to b35646a Compare July 8, 2019 14:33

kaspar030 force-pushed the pr/always_flash_riotboot_if_required branch from b35646a to 8370acb Compare July 8, 2019 14:56

fjmolinas merged commit ecdccdd into RIOT-OS:master Jul 8, 2019

kb2ma added this to the Release 2019.10 milestone Sep 15, 2019

kaspar030 deleted the pr/always_flash_riotboot_if_required branch October 10, 2019 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used #11690

riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used #11690

kaspar030 commented Jun 13, 2019

kaspar030 commented Jun 13, 2019

cladmi commented Jun 13, 2019

kaspar030 commented Jun 13, 2019

kaspar030 commented Jun 13, 2019

cladmi commented Jun 17, 2019

kaspar030 commented Jun 17, 2019

kaspar030 commented Jun 18, 2019

cladmi Jun 18, 2019

kaspar030 Jun 18, 2019

fjmolinas left a comment

kaspar030 commented Jun 20, 2019

kaspar030 commented Jul 1, 2019

fjmolinas commented Jul 1, 2019

kaspar030 commented Jul 1, 2019

fjmolinas commented Jul 1, 2019

fjmolinas commented Jul 3, 2019

kaspar030 commented Jul 4, 2019

kaspar030 commented Jul 5, 2019

fjmolinas commented Jul 8, 2019

fjmolinas left a comment

kaspar030 commented Jul 8, 2019

fjmolinas left a comment

cladmi commented Jul 8, 2019

fjmolinas commented Jul 8, 2019

riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used #11690

riotboot: set FLASHFILE to RIOTBOOT_EXTENDED_BIN if riotboot feature is used #11690

Conversation

kaspar030 commented Jun 13, 2019

Contribution description

Testing procedure

Issues/PRs references

kaspar030 commented Jun 13, 2019

cladmi commented Jun 13, 2019

kaspar030 commented Jun 13, 2019

kaspar030 commented Jun 13, 2019

cladmi commented Jun 17, 2019

kaspar030 commented Jun 17, 2019

kaspar030 commented Jun 18, 2019

cladmi Jun 18, 2019

Choose a reason for hiding this comment

kaspar030 Jun 18, 2019

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

kaspar030 commented Jun 20, 2019

kaspar030 commented Jul 1, 2019

fjmolinas commented Jul 1, 2019

kaspar030 commented Jul 1, 2019

fjmolinas commented Jul 1, 2019

fjmolinas commented Jul 3, 2019

kaspar030 commented Jul 4, 2019

kaspar030 commented Jul 5, 2019

fjmolinas commented Jul 8, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

kaspar030 commented Jul 8, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

cladmi commented Jul 8, 2019

fjmolinas commented Jul 8, 2019