-
Notifications
You must be signed in to change notification settings - Fork 2k
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
make/testbed-support: fix error with FLASHFILE not defined #12394
Conversation
Maybe @cladmi should have a look here. |
e6ad671
to
7549b1c
Compare
It would be great if @cladmi could have a second look. This is a bug fix, on useful feature (for iot-lab), I think it should be backported once merged. |
@@ -55,7 +55,7 @@ IOTLAB_USER ?= $(shell cut -f1 -d: $(IOTLAB_AUTH)) | |||
IOTLAB_EXP_ID ?= | |||
|
|||
# File to use for flashing | |||
IOTLAB_FLASHFILE ?= $(ELFFILE) | |||
FLASHFILE ?= $(ELFFILE) |
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 change required for the fix to work ?
The commit says "useless" without explaining anything or justifying why it is useless.
Not all board have a "protected" programmer configuration that would not set FLASHFILE
so it would quite easily use another file. The file used to flash in RIOT can be whichever bin/hex/elf, but on IoT-AB it must be the ELFFILE
)except for `wsn430.
I feel that changing this assumes IoT-LAB can flash with whichever file produced. If it is now the case, please clarify this.
git grep 'FLASHFILE ?='
boards/cc2538dk/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/chronos/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/common/blxxxpill/Makefile.include: FLASHFILE ?= $(BINFILE)
boards/common/msb-430/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/common/msba2/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/common/nrf52/Makefile.include: FLASHFILE ?= $(HEXFILE)
boards/common/remote/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/common/wsn430/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/f4vi1/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/lobaro-lorabox/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/mbed_lpc1768/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/native/Makefile.include:FLASHFILE ?= $(ELFFILE)
boards/nz32-sc151/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/opencm904/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/openmote-b/Makefile.include: FLASHFILE ?= $(HEXFILE)
boards/openmote-cc2538/Makefile.include: FLASHFILE ?= $(BINFILE)
boards/pic32-clicker/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/pic32-wifire/Makefile.include: FLASHFILE ?= $(HEXFILE)
boards/seeeduino_arch-pro/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/spark-core/Makefile.include:FLASHFILE ?= $(BINFILE)
boards/teensy31/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/telosb/Makefile.include:FLASHFILE ?= $(HEXFILE)
boards/z1/Makefile.include:FLASHFILE ?= $(HEXFILE)
cpu/esp32/Makefile.include:FLASHFILE ?= $(ELFFILE)
cpu/esp8266/Makefile.include:FLASHFILE ?= $(ELFFILE)
dist/testbed-support/makefile.iotlab.single.inc.mk:IOTLAB_FLASHFILE ?= $(ELFFILE)
makefiles/tools/avrdude.inc.mk:FLASHFILE ?= $(HEXFILE)
makefiles/tools/bossa.inc.mk:FLASHFILE ?= $(BINFILE)
makefiles/tools/dfu.inc.mk:FLASHFILE ?= $(BINFILE)
makefiles/tools/edbg.inc.mk:FLASHFILE ?= $(BINFILE)
makefiles/tools/jlink.inc.mk:FLASHFILE ?= $(BINFILE)
makefiles/tools/openocd.inc.mk:FLASHFILE ?= $(ELFFILE)
makefiles/tools/pic32prog.inc.mk:FLASHFILE ?= $(HEXFILE)
makefiles/tools/pyocd.inc.mk:FLASHFILE ?= $(HEXFILE)
makefiles/tools/uniflash.inc.mk:FLASHFILE ?= $(ELFFILE)
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.
IoT-LAB only supports ELF files now (except for WSN4330) but soon it will also support bin files.
How about using FLASHFILE ?= ELFFILE
as default, early (eg before boards Makefile.include are parsed) for all boards except WSN430 ?
If we agree on that, I'll reword the commit message.
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 adapted the code because I noticed that it was broken with firefly (iot-lab requires ELFFILE but the build system was already setting BINFILE).
I was able to replicate the Issue and test it is fixes by the PR: Master:
This PR:
|
7549b1c
to
a506a6d
Compare
I noticed this issue also applies to |
I tested on the following BOARDS:
master: only b-l072z-lrwan1 b-l475e-iot01a succeed
pr: only samr30-xpro, rnf5x-mdk and firefly failed
The following boards
|
#12395 adds support for the 52840-mdk. I can add a commit there for the 52832-mdk (I thought it was already supported...). samr30-xpro are broken on Saclay... firefly I don't know, maybe some problems on the Lille site. |
Done. And thanks for testing by the way ! |
@aabadie I think this is good, please squash. |
This way FLASHFILE is either already defined by the board and, if not, the default one for iotlab can be used.
a506a6d
to
ffc4c84
Compare
Done! |
@cladmi I think your comments where addressed, are you ok with 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.
Contribution description
This PR is fixing a bug introduced by #12317 when using IOTLAB_NODE. For all nrf boards (and maybe others), FLASHFILE was set based on the programmer used (jlink, openocd, pyocd). Since iotlab is set as default programmer, FLASHFILE is no longer set and this results in an error raised here:
RIOT/Makefile.include
Line 458 in d749e2d
This PR is now setting the default FLASHFILE when IOTLAB_NODE is used to the value corresponding to the board (HEXFILE for wsn430, ELFFILE for others).
Note that this PR is also removing the IOTLAB_FLASHFILE variable, since it becomes useless after the proposed fix.
CC'ing @cladmi to also get his opinion on the fix and his possible ideas for improvement.
Testing procedure
=> on master, you get the following error message:
With this PR, it works like a charm for all boards.
Issues/PRs references
Bug introduced by #12317