Skip to content

riotboot.mk: get variable as hex rather than dec#11201

Closed
danpetry wants to merge 1 commit intoRIOT-OS:masterfrom
danpetry:build/riotboot_slot_dectohex
Closed

riotboot.mk: get variable as hex rather than dec#11201
danpetry wants to merge 1 commit intoRIOT-OS:masterfrom
danpetry:build/riotboot_slot_dectohex

Conversation

@danpetry
Copy link
Copy Markdown
Contributor

@danpetry danpetry commented Mar 18, 2019 •
edited
Loading

Contribution description

Previously, summing two hex values using the shell in this location was resulting in a decimal value, meaning the slot 1 image was flashing in the wrong place when using jlink. (Jlink seems to interpret all flashing values as hex values, even when not preceded by "0x".) This provides a fix.

Suggest making the same change in other locations where shell echo is used to sum hex variables?

Testing procedure

  • tests/riotboot with nrf52dk

  • tests/riotboot with a board that uses openocd (e.g. samr21-xpro)

Issues/PRs references

Fixes #11126

Dependencies

#11200

@danpetry danpetry requested review from cladmi and jcarrano March 18, 2019 10:54
@danpetry danpetry mentioned this pull request Mar 18, 2019
Merged
4 tasks
riotboot.mk: get variable as hex rather than dec
c460d8b 
Previously, summing hex values using the shell was resulting in a
decimal value, meaning the image was flashing in the wrong place.
@danpetry danpetry force-pushed the build/riotboot_slot_dectohex branch from 39dc8da to c460d8b Compare March 18, 2019 16:06
@danpetry
Copy link
Copy Markdown
Contributor Author

danpetry commented Mar 18, 2019

rebased to resolve conflicts from #11181

@danpetry danpetry mentioned this pull request Mar 18, 2019
Merged
1 task
@danpetry danpetry added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 18, 2019
@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented Mar 19, 2019

Suggest making the same change in other locations where shell echo is used to sum hex variables?

Yes. If we precede all hex values by "0x" we ensure that no command can interpret that as decimal. Also, it makes 100% sense that summing hex gives back hex.

Running git grep '$((.*+.*))' gives:

boards/esp32-olimex-evb/Makefile.include:        $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
cpu/esp32/Makefile.dep:    $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
cpu/esp32/Makefile.dep:    $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
cpu/esp8266/Makefile.include:    $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
dist/tools/openocd/openocd.sh:        IMAGE_OFFSET=$(printf "0x%08x\n" "$((${IMAGE_OFFSET} + ${FLASH_ADDR}))")
makefiles/boot/riotboot.mk:export SLOT1_OFFSET ?= $(shell echo $$(($(SLOT0_OFFSET) + $(SLOT0_LEN))))
makefiles/boot/riotboot.mk:SLOT0_IMAGE_OFFSET := $$(($(SLOT0_OFFSET) + $(RIOTBOOT_HDR_LEN)))
makefiles/boot/riotboot.mk:SLOT1_IMAGE_OFFSET := $$(($(SLOT1_OFFSET) + $(RIOTBOOT_HDR_LEN)))
makefiles/boot/riotboot.mk:     $(Q)$(HEADER_TOOL) generate $< $(APP_VER) $$(($(ROM_START_ADDR)+$(OFFSET))) $(RIOTBOOT_HDR_LEN) - $
makefiles/boot/riotboot.mk:     $(Q)truncate -s $$(($(SLOT0_OFFSET) + $(SLOT0_LEN) + $(RIOTBOOT_HDR_LEN))) $@.tmp
makefiles/info-nproc.inc.mk:    NPROC := $(shell echo $$(($(NPROC) + 1)))
makefiles/mcuboot.mk:   $(Q)$(_LINK) $(LINKFLAGPREFIX)--defsym=offset="$$(($(MCUBOOT_SLOT0_SIZE) + $(IMAGE_HDR_SIZE)))" \
tests/cortexm_common_ldscript/Makefile: EXPECT_START_ADDR=$$(printf "0x%08x" $$(( $(ROM_START_ADDR) + $* ))); \
tests/cortexm_common_ldscript/Makefile:test-assert_overflow_too_big_for_rom: FW_ROM_LEN=$$(($(ROM_LEN_BYTES) + 1))

@danpetry
Copy link
Copy Markdown
Contributor Author

danpetry commented Mar 19, 2019

yep. Could put those changes in a separate PR so #11126 is unblocked soon?

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 19, 2019

Nice find about JLink interpreting hex values.

However, if the issue is JLink misinterpreting the value, jlink.sh should do the conversion.
It is not a global wide constraints but only a JLink one.

If for any reason another flasher needs a decimal value, then what would we do ?
The low level constraint should not constraint the abstraction but the low level should adapt to the abstraction.

cladmi
cladmi suggested changes Mar 19, 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.

This is not needed.

Your changes from #11200 already internally does the handling of hexadecimal values and works with #11126 without additional modifications. I will report over there directly.

@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented Mar 19, 2019

jlink.sh should do the conversion.

But it does not, and now how do we know we are not breaking anything if we "fix" jlink.sh?

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 20, 2019

I think this one can be closed as #11200 handles it properly.
Re-open if I am wrong.

@cladmi cladmi closed this Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcarrano jcarrano Awaiting requested review from jcarrano

1 more reviewer

@cladmi cladmi cladmi requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

State: waiting for other PR State: The PR requires another PR to be merged first

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@danpetry @jcarrano @cladmi