Skip to content

mcuboot.mk: refactor into a declarative-style makefile. #10494

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.

Sign up for GitHub

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

Closed
jcarrano wants to merge 6 commits into from
Closed

mcuboot.mk: refactor into a declarative-style makefile. #10494

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d2bc904
mcuboot.mk: remove unnecessary override, export and :=.
jcarrano Nov 27, 2018
1c7c365
mcuboot.mk: Do not define BINFILE.
jcarrano Nov 27, 2018
38b3cbd
mcuboot.mk: replace MCUBOOT_BIN by MCUBOOT_LOADER_BIN.
jcarrano Nov 28, 2018
d325b11
mcuboot.mk: Restructure targets to express dependencies.
jcarrano Nov 27, 2018
b439fd9
mcuboot.mk: support in-docker-container for mcuboot target.
jcarrano Nov 28, 2018
dfed9e9
makefiles/mcuboot.mk: always define rule to make key.pem.
jcarrano Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 34 additions & 31 deletions makefiles/mcuboot.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,52 @@
.PHONY: mcuboot mcuboot-create-key mcuboot-flash-bootloader mcuboot-flash

ifdef MCUBOOT_SLOT0_SIZE

IMGTOOL ?= $(RIOTTOOLS)/mcuboot/imgtool.py
override IMGTOOL := $(abspath $(IMGTOOL))
IMGTOOL_KEYFLAGS ?= keygen -t rsa-2048

@cladmi cladmi Dec 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Currently I do not find necessary to add the KEYFLAGS. If it was imgtool.py specific I would not put keygen in it.
And if supposed to be generic, here you do not include the -k so cannot be replaced by an other tool anyway.

Maybe keep this change for when there is a need.

@jcarrano jcarrano Dec 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can remove that commit.


BINFILE ?= $(BINDIR)/$(APPLICATION).bin
SIGN_BINFILE = $(BINDIR)/signed-$(APPLICATION).bin
MCUBOOT_KEYFILE ?= $(BINDIR)/key.pem
MCUBOOT_BIN ?= $(BINDIR)/mcuboot.bin
MCUBOOT_BIN_URL ?= http://download.riot-os.org/mynewt.mcuboot.bin
MCUBOOT_BIN_MD5 ?= 0c71a0589bd3709fc2d90f07a0035ce7

export IMAGE_HDR_SIZE ?= 512
MCUBOOT_ELF ?= $(ELFFILE:.elf=.mcuboot.elf)

@cladmi cladmi Dec 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not a mcuboot_elf specific thing it is only a normal RIOT firmware but linked for slot0.
In the bootloader PR it is $(BINDIR_APP)-slot0.elf.
So maybe something like ELFFILE_MCUBOOT_SLOT0 = $(ELFFILE:.elf=.mcuboot-slot0.elf)

@jcarrano jcarrano Dec 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"normal RIOT firmware": it also has space for the header. I saw the message "Re-linking for MCUBoot at..." and assumed it was a MCUBoot-specific thing.

MCUBOOT_BIN ?= $(MCUBOOT_ELF:.elf=.bin)

MCUBOOT_LOADER_BIN ?= $(BINDIR)/mcuboot.bin

@cladmi cladmi Dec 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefered MCUBOOT_BIN has it is the mcuboot binary file. For me it was completely clear. Here loader does not mean anything to me, at least bootloader would be clear.

However renaming the file to keep the full mynewt.mcuboot.bin name would prevent possible issues with the filename in bin.

MCUBOOT_LOADER_BIN_URL ?= http://download.riot-os.org/mynewt.mcuboot.bin
MCUBOOT_LOADER_BIN_MD5 ?= 0c71a0589bd3709fc2d90f07a0035ce7

$(MCUBOOT_KEYFILE) $(MCUBOOT_BIN): $(filter clean, $(MAKECMDGOALS))
IMAGE_HDR_SIZE ?= 512

$(MCUBOOT_KEYFILE) $(MCUBOOT_LOADER_BIN): $(filter clean, $(MAKECMDGOALS))

mcuboot-create-key: $(MCUBOOT_KEYFILE)

ifeq ($(BINDIR)/key.pem,$(MCUBOOT_KEYFILE))
$(MCUBOOT_KEYFILE):
$(BINDIR)/key.pem:
$(Q)mkdir -p $(BINDIR)
$(Q)$(IMGTOOL) keygen -k $@ -t rsa-2048
endif
$(Q)$(IMGTOOL) $(IMGTOOL_KEYFLAGS) -k $@

mcuboot: ROM_OFFSET=$$(($(MCUBOOT_SLOT0_SIZE) + $(IMAGE_HDR_SIZE)))
mcuboot: mcuboot-create-key link
@$(COLOR_ECHO)
$(MCUBOOT_ELF): ROM_OFFSET=$$(($(MCUBOOT_SLOT0_SIZE) + $(IMAGE_HDR_SIZE)))
$(MCUBOOT_ELF): $(BASELIBS)
@$(COLOR_ECHO) '$(COLOR_PURPLE)Re-linking for MCUBoot at $(MCUBOOT_SLOT0_SIZE)...$(COLOR_RESET)'
@$(COLOR_ECHO)
$(Q)$(_LINK) -o $(ELFFILE) && \
$(OBJCOPY) $(OFLAGS) -Obinary $(ELFFILE) $(BINFILE) && \
$(IMGTOOL) sign --key $(MCUBOOT_KEYFILE) --version $(IMAGE_VERSION) --align \
$(MCUBOOT_IMAGE_ALIGN) -H $(IMAGE_HDR_SIZE) $(BINFILE) $(SIGN_BINFILE)
@$(COLOR_ECHO)
@$(COLOR_ECHO) '$(COLOR_PURPLE)Signed with $(MCUBOOT_KEYFILE) for version $(IMAGE_VERSION)\
$(COLOR_RESET)'
@$(COLOR_ECHO)

$(MCUBOOT_BIN):
$(Q)$(DLCACHE) $(MCUBOOT_BIN_URL) $(MCUBOOT_BIN_MD5) $@

.PHONY: mcuboot-flash-bootloader mcuboot-flash

mcuboot-flash-bootloader: FLASHFILE = $(MCUBOOT_BIN)
$(Q)$(_LINK) -o $@

$(SIGN_BINFILE): $(MCUBOOT_BIN) $(MCUBOOT_KEYFILE)
@$(COLOR_ECHO) '$(COLOR_PURPLE)Signing with $(MCUBOOT_KEYFILE) for version $(IMAGE_VERSION)$(COLOR_RESET)'
$(Q)$(IMGTOOL) sign --key $(MCUBOOT_KEYFILE) --version $(IMAGE_VERSION) --align \
$(MCUBOOT_IMAGE_ALIGN) -H $(IMAGE_HDR_SIZE) $(MCUBOOT_BIN) $(SIGN_BINFILE)

ifeq ($(BUILD_IN_DOCKER),1)
mcuboot: ..in-docker-container

@cladmi cladmi Dec 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This currently only works in the tests/mcuboot application as it will not pass mcuboot to the docker make command.
A solution for this is to add DOCKER_MAKECMDGOALS_POSSIBLE += mcuboot.
However, IMAGE_VERSION must be set in the Makefile and not command line.
So could add a test for this to at least not try to compile.

I will find something for this.

else
mcuboot: $(SIGN_BINFILE)
endif # BUILD_IN_DOCKER

$(MCUBOOT_LOADER_BIN):
$(Q)$(DLCACHE) $(MCUBOOT_LOADER_BIN_URL) $(MCUBOOT_LOADER_BIN_MD5) $@

mcuboot-flash-bootloader: FLASHFILE = $(MCUBOOT_LOADER_BIN)
mcuboot-flash-bootloader: export FLASH_ADDR = 0x0
mcuboot-flash-bootloader: $(MCUBOOT_BIN) $(FLASHDEPS)
mcuboot-flash-bootloader: $(MCUBOOT_LOADER_BIN) $(FLASHDEPS)
$(flash-recipe)

mcuboot-flash: FLASHFILE = $(SIGN_BINFILE)
Expand Down