makefiles/boot/riotboot: clean bootloader when cleaning application #16197

fjmolinas · 2021-03-16T10:31:22Z

Contribution description

When using riotboot conveniently the extended binary is used. But if clean is also used then the bootloader binary is not cleaned, this can lead to unexpected behavior where a previous bootloader (eg: different start addresses) is used.

Testing procedure

make -C tests/riotboot flash
make -C tests/riotboot clean flash -j3 you should see that that the bootloader and application are compiled while in master only the application is:

# application
"make" -C /home/francisco/workspace/RIOT2/boards/stm32f429i-disc1
"make" -C /home/francisco/workspace/RIOT2/core
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/stmclk
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/vectors
"make" -C /home/francisco/workspace/RIOT2/drivers
"make" -C /home/francisco/workspace/RIOT2/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT2/sys
"make" -C /home/francisco/workspace/RIOT2/sys/auto_init
"make" -C /home/francisco/workspace/RIOT2/sys/checksum
"make" -C /home/francisco/workspace/RIOT2/sys/isrpipe
"make" -C /home/francisco/workspace/RIOT2/sys/malloc_thread_safe
"make" -C /home/francisco/workspace/RIOT2/sys/newlib_syscalls_default
"make" -C /home/francisco/workspace/RIOT2/sys/pm_layered
"make" -C /home/francisco/workspace/RIOT2/sys/riotboot
"make" -C /home/francisco/workspace/RIOT2/sys/shell
"make" -C /home/francisco/workspace/RIOT2/sys/shell/commands
"make" -C /home/francisco/workspace/RIOT2/sys/stdio_uart
"make" -C /home/francisco/workspace/RIOT2/sys/test_utils/interactive_sync
"make" -C /home/francisco/workspace/RIOT2/sys/tsrb
# bootloader
"make" -C /home/francisco/workspace/RIOT2/boards/stm32f429i-disc1
"make" -C /home/francisco/workspace/RIOT2/core
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/stmclk
"make" -C /home/francisco/workspace/RIOT2/cpu/stm32/vectors
"make" -C /home/francisco/workspace/RIOT2/drivers
"make" -C /home/francisco/workspace/RIOT2/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT2/sys
"make" -C /home/francisco/workspace/RIOT2/sys/checksum
"make" -C /home/francisco/workspace/RIOT2/sys/malloc_thread_safe
"make" -C /home/francisco/workspace/RIOT2/sys/newlib_syscalls_default
"make" -C /home/francisco/workspace/RIOT2/sys/riotboot
"make" -C /home/francisco/workspace/RIOT2/sys/stdio_null

What is actually called can be seen with this patch easily:

diff --git a/makefiles/boot/riotboot.mk b/makefiles/boot/riotboot.mk
index 164b6f7062..440f669b79 100644
--- a/makefiles/boot/riotboot.mk
+++ b/makefiles/boot/riotboot.mk
@@ -79,7 +79,7 @@ RIOTBOOT_CLEAN = $(if $(filter clean, $(MAKECMDGOALS)),riotboot/bootloader/clean
 # riotboot bootloader compile target
 riotboot/flash-bootloader: riotboot/bootloader/flash
 riotboot/bootloader/%: $(BUILDDEPS) | $$(filter-out $$@,$$(RIOTBOOT_CLEAN))
-       $(Q)/usr/bin/env -i \
+       /usr/bin/env -i \
                QUIET=$(QUIET) PATH="$(PATH)"\
                EXTERNAL_BOARD_DIRS="$(EXTERNAL_BOARD_DIRS)" BOARD=$(BOARD)\
                DEBUG_ADAPTER_ID=$(DEBUG_ADAPTER_ID) \

makefiles/boot/riotboot.mk

kaspar030 · 2021-03-16T10:59:49Z

makefiles/boot/riotboot.mk

 # riotboot bootloader compile target
 riotboot/flash-bootloader: riotboot/bootloader/flash
-riotboot/bootloader/%: $(BUILDDEPS)
+riotboot/bootloader/%: $(BUILDDEPS) | $$(filter-out $$@,$$(RIOTBOOT_CLEAN))


or just add | $(CLEAN) here?

ah, no, it must be riotboot/bootloader/clean

maybe riotboot/bootloader/clean can be added to "clean" as prerequisite?

That does the trick as well.

fjmolinas · 2021-03-16T14:06:43Z

Split out the secondexpansion part as it is now unrelated.

fjmolinas · 2021-03-17T07:35:06Z

Split out the secondexpansion part as it is now unrelated.

Hmm this also introduces a circular dependency:

make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/vectors/STM32F429xx.c <- clean dependency dropped.
make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/include/irqs/f4/irqs.h <- clean dependency dropped.

fjmolinas · 2021-03-19T14:17:10Z

Split out the secondexpansion part as it is now unrelated.

Hmm this also introduces a circular dependency:
make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/vectors/STM32F429xx.c <- clean dependency dropped.
make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/include/irqs/f4/irqs.h <- clean dependency dropped.

Only related to stm32 and the fact the build the vectors.

fjmolinas · 2021-03-19T14:21:42Z

Split out the secondexpansion part as it is now unrelated.

Hmm this also introduces a circular dependency:
make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/vectors/STM32F429xx.c <- clean dependency dropped.
make: Circular /home/francisco/workspace/RIOT2/cpu/stm32/include/irqs/f4/irqs.h <- clean dependency dropped.
Only related to stm32 and the fact the build the vectors.

I think I like the other version better, by adding it ti clean I'll get circular dependencies every time something extra is added to BUILDDEPS, make removes this on its own but it leads to these ugly messages. Do you have another suggestion otherwise @kaspar030? Maybe the latest change is not what you had in mind?

fjmolinas · 2021-04-08T06:15:44Z

ping @kaspar030!

fjmolinas · 2021-06-07T07:39:24Z

ping @kaspar030!

Figured out a solution with secondexpansion.

kaspar030 · 2021-06-08T09:37:07Z

I think there is an issue:

riot/tests/riotboot on  pr/16197                                                                                                         [16/5152]
❯ BOARD=nrf52840dk make clean all -j                                      
Building application "tests_riotboot" for "nrf52840dk" with MCU "nrf52".
                                                                         
compiling /home/kaspar/src/riot/dist/tools/riotboot_gen_hdr/bin/genhdr... 
make: Nothing to be done for 'all'.                                      
"make" -C /home/kaspar/src/riot/boards/nrf52840dk
"make" -C /home/kaspar/src/riot/core                   
"make" -C /home/kaspar/src/riot/cpu/nrf52
"make" -C /home/kaspar/src/riot/drivers              
"make" -C /home/kaspar/src/riot/sys
"make" -C /home/kaspar/src/riot/boards/common/nrf52xxxdk
"make" -C /home/kaspar/src/riot/cpu/cortexm_common    
"make" -C /home/kaspar/src/riot/sys/auto_init              
"make" -C /home/kaspar/src/riot/drivers/periph_common
"make" -C /home/kaspar/src/riot/cpu/nrf52/periph
"make" -C /home/kaspar/src/riot/sys/checksum
"make" -C /home/kaspar/src/riot/cpu/nrf52/vectors
"make" -C /home/kaspar/src/riot/sys/isrpipe
"make" -C /home/kaspar/src/riot/sys/malloc_thread_safe
"make" -C /home/kaspar/src/riot/sys/newlib_syscalls_default
"make" -C /home/kaspar/src/riot/cpu/nrf5x_common
"make" -C /home/kaspar/src/riot/sys/riotboot
"make" -C /home/kaspar/src/riot/sys/shell
"make" -C /home/kaspar/src/riot/sys/shell/commands
"make" -C /home/kaspar/src/riot/sys/stdio_uart
"make" -C /home/kaspar/src/riot/sys/test_utils/interactive_sync
"make" -C /home/kaspar/src/riot/cpu/cortexm_common/periph
"make" -C /home/kaspar/src/riot/sys/tsrb
"make" -C /home/kaspar/src/riot/cpu/nrf5x_common/periph
"make" -C /home/kaspar/src/riot/boards/nrf52840dk
"make" -C /home/kaspar/src/riot/boards/common/nrf52xxxdk
creating /home/kaspar/src/riot/tests/riotboot/bin/nrf52840dk/tests_riotboot-slot0.1623144927.riot.bin...
   text    data     bss     dec     hex filename
  12772     128    2396   15296    3bc0 `
"make" -C /home/kaspar/src/riot/core 
"make" -C /home/kaspar/src/riot/cpu/nrf52
"make" -C /home/kaspar/src/riot/cpu/cortexm_common
"make" -C /home/kaspar/src/riot/cpu/cortexm_common/periph
"make" -C /home/kaspar/src/riot/cpu/nrf52/periph
"make" -C /home/kaspar/src/riot/cpu/nrf52/vectors
"make" -C /home/kaspar/src/riot/cpu/nrf5x_common
"make" -C /home/kaspar/src/riot/cpu/nrf5x_common/periph
"make" -C /home/kaspar/src/riot/drivers
"make" -C /home/kaspar/src/riot/drivers/periph_common
"make" -C /home/kaspar/src/riot/sys
"make" -C /home/kaspar/src/riot/sys/checksum
"make" -C /home/kaspar/src/riot/sys/malloc_thread_safe
"make" -C /home/kaspar/src/riot/sys/newlib_syscalls_default
"make" -C /home/kaspar/src/riot/sys/riotboot
"make" -C /home/kaspar/src/riot/sys/stdio_null
riot/tests/riotboot on  pr/16197 took 5s 
❯

see compilation going on (I think it is riotboot) after /home/kaspar/src/riot/tests/riotboot/bin/nrf52840dk/tests_riotboot.elf has been linked.

kaspar030 · 2021-06-15T09:07:08Z

see compilation going on (I think it is riotboot) after /home/kaspar/src/riot/tests/riotboot/bin/nrf52840dk/tests_riotboot.elf has been linked.

This is actually alright, thanks @fjmolinas. The elf gets built, then the bootloader, then they get combined into the final .bin.

kaspar030

ACK.

fjmolinas added Area: build system Area: Build system Area: OTA Area: Over-the-air updates labels Mar 16, 2021

fjmolinas requested a review from kaspar030 March 16, 2021 10:31

kaspar030 reviewed Mar 16, 2021

View reviewed changes

makefiles/boot/riotboot.mk Outdated Show resolved Hide resolved

kaspar030 reviewed Mar 16, 2021

View reviewed changes

fjmolinas force-pushed the pr_riotboot_submake_clean branch from cd6be45 to 744b0fb Compare March 16, 2021 14:03

fjmolinas mentioned this pull request Mar 16, 2021

Makefile: enable SECONDEXPANSION globally for modules and applications #16198

Merged

makefiles/boot/riotboot: clean bootloader when cleaning application

edeacbf

fjmolinas force-pushed the pr_riotboot_submake_clean branch from 744b0fb to edeacbf Compare June 7, 2021 07:39

github-actions bot removed the Area: OTA Area: Over-the-air updates label Jun 7, 2021

kaspar030 self-assigned this Jun 8, 2021

kaspar030 approved these changes Jun 15, 2021

View reviewed changes

fjmolinas merged commit 6756563 into RIOT-OS:master Jun 15, 2021

fjmolinas deleted the pr_riotboot_submake_clean branch June 15, 2021 09:14

MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makefiles/boot/riotboot: clean bootloader when cleaning application #16197

makefiles/boot/riotboot: clean bootloader when cleaning application #16197

fjmolinas commented Mar 16, 2021 •

edited

Loading

kaspar030 Mar 16, 2021

kaspar030 Mar 16, 2021

kaspar030 Mar 16, 2021

fjmolinas Mar 16, 2021

fjmolinas commented Mar 16, 2021

fjmolinas commented Mar 17, 2021

fjmolinas commented Mar 19, 2021

fjmolinas commented Mar 19, 2021

fjmolinas commented Apr 8, 2021

fjmolinas commented Jun 7, 2021

kaspar030 commented Jun 8, 2021 •

edited

Loading

kaspar030 commented Jun 15, 2021

kaspar030 left a comment

makefiles/boot/riotboot: clean bootloader when cleaning application #16197

makefiles/boot/riotboot: clean bootloader when cleaning application #16197

Conversation

fjmolinas commented Mar 16, 2021 • edited Loading

Contribution description

Testing procedure

kaspar030 Mar 16, 2021

Choose a reason for hiding this comment

kaspar030 Mar 16, 2021

Choose a reason for hiding this comment

kaspar030 Mar 16, 2021

Choose a reason for hiding this comment

fjmolinas Mar 16, 2021

Choose a reason for hiding this comment

fjmolinas commented Mar 16, 2021

fjmolinas commented Mar 17, 2021

fjmolinas commented Mar 19, 2021

fjmolinas commented Mar 19, 2021

fjmolinas commented Apr 8, 2021

fjmolinas commented Jun 7, 2021

kaspar030 commented Jun 8, 2021 • edited Loading

kaspar030 commented Jun 15, 2021

kaspar030 left a comment

Choose a reason for hiding this comment

fjmolinas commented Mar 16, 2021 •

edited

Loading

kaspar030 commented Jun 8, 2021 •

edited

Loading