-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add small RIOT bootloader and multislot management #7457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@kaspar030 I rebased and adapted your branch in this PR. I decided to dissociate the bootloader and multislot stuff from the OTA module. I'm working on that and I think I can submit it soon. If you can review this soon it would be great, as I'm going on vacations soon. |
How does it sound? :) |
A beautiful melody ;) |
kaspar030
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial round...
| include $(RIOTMAKE)/tools/serial.inc.mk | ||
|
|
||
| # export ROM partitions | ||
| export ROM_SIZE = 0x80000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why export these variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're used in several places not only in Makefiles but also in flashing scripts and in subsequent linking operations, but I can modify this to export only those really needed. Does it harm?
boards/samr21-xpro/Makefile.include
Outdated
| export ROM_SIZE = 0x40000 | ||
| export SLOT0_ADDR = 0x0 | ||
| export SLOT0_SIZE = 0x4000 | ||
| CFLAGS +=-DSLOT0_SIZE=$(SLOT0_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing these defines the second time. Maybe find a common place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe we can add them to the exported variables in the main makefile intended for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're already there, right? (in sys/firmware/Makefile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I didn't remember that. So then I guess there they're ok?
bootloader/main.c
Outdated
| @@ -0,0 +1,59 @@ | |||
| /* | |||
| * Copyright (C) 2017 Kaspar Schleiser | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add my email address to the copyright
| @@ -0,0 +1,12 @@ | |||
| APPLICATION = bootloader | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely happy with adding another top-level folder for the bootloader...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I also don't think that adding it to examples is the good place... For me it looks good here because it states an essential part of RIOT, though it's optional...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll ask on the mailing list
dist/tools/firmware/genmeta.c
Outdated
| #include "firmware.h" | ||
| #include "common.h" | ||
|
|
||
| const char genmeta_usage[] = "genmeta <BINFILE> <VERSION> <APPID> <START-ADDR> <seckey> <outfile>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"firmware genmeta ..." (I knew where to look. ;) )
makefiles/multislot.mk
Outdated
| bootloader-clean: | ||
| @env -i PATH=$(PATH) BOARD=$(BOARD) make -C $(RIOTBASE)/bootloader clean | ||
|
|
||
| multislot-clean: firmware-tools-clean bootloader-clean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe rename to clean-multislot (so it is consistent with "flash-multislot")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed...
tests/bootloader_test/README.md
Outdated
| @@ -0,0 +1,31 @@ | |||
| Hello Bootloader! | |||
| ============ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more equal signs (match string above)
tests/bootloader_test/Makefile
Outdated
| APP_ID = 0xBB8 | ||
| APP_VERSION = 0x1 | ||
|
|
||
| BOARD_WHITELIST := samr21-xpro iotlab-m3 fox iotlab-a8-m3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about FEATURE_MULTISLOT, which is set to true when the corresponding variables are set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds good to me, but to "whom" does this feature belong? board? cpu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the CPU?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think this feature can also belong to the board, since some development boards come with a pre-flashed bootloader, which I think we don't aim to overwrite (until we have a nice solution to flash all the boards using this bootloader through serial or USB), so maybe is better to put it as a board feature.
|
|
||
| .PHONY: flash-multislot verify firmware-tools-clean | ||
|
|
||
| ifneq (, $(filter $(BOARD),iotlab-m3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a board specific define needed here? need to get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because I'm relying on the FFLAGS to avoid modifying more the flashing process. I didn't find a place or another way to only do a flash-bin without affecting the FFLAGS for other platforms (it happened to me that without this the normal flashing failed since it tried to always do flash-bin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, it can't be here. If we add board-specific stuff in this makefile, we'll end up with hundreds of exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, it was an initial way to avoid the error. We maybe need to decide which kind of files we prefer to flash in a board. We can start to put binary by default in boards on which the bootloader can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I think is incorrect to send a binary file as a HEXFILE variable, so that can also be fixed.
a74999c to
cfa70e7
Compare
|
Addressed comments. |
390c385 to
ea014d8
Compare
ea014d8 to
3aa7a50
Compare
|
Same as #7396. |
Another approach tackling the need of a bootloader. It's based on @kaspar030 work and #7396, without all the cryptographic tools embedded in the bootloader.
In this approach, the security checks will be relayed to the OTA update module, which will come in a subsequent PR.
The instructions to use the RIOT bootloader are the same as in the previous PR a bit more simplified:
make combined: Creates Ready-To-Send images for both slots, including metadata and signatures.make verify: Verifies if the image's signature is valid, using the provided public key.make flash-multislot: Builds, links, signs and flashes an image including the bootloader and the application on the first slot. Flashing the bootlaoder with the application in slot 2 is not supported.make multislot-clean: Deletes all the files created by any of the previous targetsA small difference on the created files:
slot, with its corresponding metadata ready to be sent over the air.
You can find these files in the binary folder of your application.
The signature checks are not done by the bootloader, but the build system generates valid and signed images with the provided keys (or creates new keys if none are provided).