Skip to content

riotboot,mcuboot: Use canned recipe for flashing.#11110

Merged
cladmi merged 2 commits intoRIOT-OS:masterfrom
jcarrano:flash_canned_recipe_conversion
Mar 6, 2019
Merged

riotboot,mcuboot: Use canned recipe for flashing.#11110
cladmi merged 2 commits intoRIOT-OS:masterfrom
jcarrano:flash_canned_recipe_conversion

Conversation

@jcarrano
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano commented Mar 5, 2019

Contribution description

#10548 introduced a canned recipe for flashing. The aim was to reduce code duplication, and also to be able to perform more than one command. This is important for boards that need more complex procedure, for example to put them in a special mode or disabling the watchdog (currently done in "preflash")

By having the flashing recipe concentrated in a single place maintenance is simplified. Eventually the flash recipe will be defined with ?= to allow overriding, and then the benefits of this change will become clearer.

Testing procedure

Build and flash tests/riotboot. I could not test mcuboot because I do not have a supported board.

Flashing continue to work fine.

Issues/PRs references

See: #10548.

jcarrano added 2 commits March 5, 2019 14:39
@jcarrano
makefiles/riotboot: use canned recipe for flashing.
748bc6d 
A canned recipe had previously been defined to perform the flashing
procedure. The canned recipe is preferred to calling $(FLASHER) $(FFLAGS)
as there might be additional steps involved in flashing (this is handled by
preflash currently but with the canned recipe we will be able to fix it.)
@jcarrano
makefiles/mcuboot: use canned recipe for flashing.
420b2c9 
The canned recipe is preferred to using $(FLASHER) $(FFLAGS) as it
allows to specify additional action actions (like what preflash is
currently doing.)
@jcarrano jcarrano added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 5, 2019
@jcarrano jcarrano requested a review from cladmi March 5, 2019 15:51
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 6, 2019

Flashing test/riotboot is not enough, it does not uses the flash recipes defined in makefiles/boot/riotboot.mk. The different flash targets should be tested.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 6, 2019

For mcuboot, the firmware keeps working as in master when doing mcuboot-flash.

For a pure command reviews, the mcuboot-flash targets that calls both flash commands correctly reports the two flash commands.

make mcuboot-flash
...
/home/harter/work/git/RIOT/dist/tools/jlink/jlink.sh flash /home/harter/work/git/RIOT/tests/mcuboot/bin/nrf52dk/mcuboot.bin
...
/home/harter/work/git/RIOT/dist/tools/jlink/jlink.sh flash /home/harter/work/git/RIOT/tests/mcuboot/bin/nrf52dk/signed-tests_mcuboot.bin

In master it was the same with the environment on the command line:

FLASH_ADDR=0x0 /home/harter/work/git/RIOT/dist/tools/jlink/jlink.sh flash /home/harter/work/git/RIOT/tests/mcuboot/bin/nrf52dk/mcuboot.bin
...
FLASH_ADDR=0x8000 /home/harter/work/git/RIOT/dist/tools/jlink/jlink.sh flash /home/harter/work/git/RIOT/tests/mcuboot/bin/nrf52dk/signed-tests_mcuboot.bin

By replacing the FLASHER by env we can get the FLASH_ADDR value:

make mcuboot-flash FLASHER=env FFLAGS=" | grep ^FLASH_ADDR"
...
env | grep ^FLASH_ADDR
FLASH_ADDR=0x0
env | grep ^FLASH_ADDR
FLASH_ADDR=0x8000

In master it was the same.

make mcuboot-flash FLASHER=env FFLAGS=" | grep ^FLASH_ADDR"
...
FLASH_ADDR=0x0 env | grep ^FLASH_ADDR
FLASH_ADDR=0x0
FLASH_ADDR=0x8000 env | grep ^FLASH_ADDR
FLASH_ADDR=0x8000

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 6, 2019

For riotboot I tested the whole functionality with iotlab-m3.
I also checked that the output uses the correct file. Here there are no changes with env variables so no need for a dedicated test.

This test procedure successfully executes:

# Erase what was there
BOARD=iotlab-m3 make -C examples/gnrc_networking flash
# Boot slot 0 with correct version
APP_VER=20 BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-combined-slot0

# Erase what was there
BOARD=iotlab-m3 make -C examples/gnrc_networking flash

# Flash only bootloader, then slot 1, it  boot with slot1 (not slot 0)
BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-bootloader
APP_VER=11 BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-slot1

# Flash slot 0 with a newer version, it uses it
APP_VER=12 BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-slot0

# Then slot1 again to be sure with a new version, it uses it
APP_VER=13 BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-slot1

# Use the extended version with a lower `APP_VER`. It correctly boots slot0
APP_VER=1 BOARD=iotlab-m3 make -C tests/riotboot riotboot/flash-extended-slot0

cladmi
cladmi approved these changes Mar 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, tested and the changes are good. The handling of the export is also the good way of doing it.

@cladmi cladmi added 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 Mar 6, 2019
@cladmi cladmi merged commit 411e98e into RIOT-OS:master Mar 6, 2019
@cladmi cladmi mentioned this pull request Mar 6, 2019
Merged
@danpetry danpetry added this to the Release 2019.04 milestone Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cladmi cladmi cladmi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.04

Development

Successfully merging this pull request may close these issues.

3 participants

@jcarrano @cladmi @danpetry