Skip to content
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

cortexm_common/ldscript: simplify generation of firmwares in section of the ROM #9351

Merged
merged 11 commits into from
Aug 12, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jun 14, 2018

Contribution description

This adds/modifies two build system configuration variables for cortexm_common/ldscript/cortexm.ld.

It is now possible to generate during a build, different elf files with different rom offset and firmware rom length by setting ROM_OFFSET and FW_ROM_LEN variables.

  • Modify ROM_OFFSET so it can be set as a target specific variable to be used during linking.
  • Add a new FW_ROM_LEN variable that can restrict the firmware length to use when linking instead of using the whole rom after ROM_OFFSET.
    • The previous solution was to change ROM_LEN to be ROM_LEN = ROM_OFFSET + FW_ROM_LEN which was not as straightforward to use.

Use cases

The goal is firmwares that do not use the whole ROM for the over the air update scenario with a ROM split in 3 parts "Boot Loader - Firmware 1 - Firmware 2".

Current state

In the test, I currently only set the supported board with a whitelist to iotlab-xx and samr21-xpro nodes. But it should be expandable to all boards using cortexm_common/ldscript/cortexm.ld as linking script.
I can do it depending on your feedback.

There are compilation tests for:

  • Generating firmware with 2 different offsets
  • Generating a firmware with a size of half the rom
  • Overflow detection when setting a too big FW_ROM_LEN

Issues/PRs references

Part of #9342
Depends on #9451

This is a modified version of things inspired from both following branches:

kYc0o https://github.com/kYc0o/RIOT/tree/ota_firmware_updates
kaspar030 https://github.com/kaspar030/RIOT/tree/ota_firmware_updates

I did not re-use the existing commits as it is a different implementation.
Tell me if you want to be mentioned in a commit.

@cladmi cladmi added 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 Area: OTA Area: Over-the-air updates labels Jun 14, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jun 14, 2018

I re-added the main if to only test if all symbols are defined when one is defined.
It should fix boards that do not use the common cortexm.ld.

I tried doing it in cortexm.ld with ASSERT but the memory section complains about undefined symbol before the ASSERT so the message is invalid length for memory region rom instead of the assert message.

@cladmi
Copy link
Contributor Author

cladmi commented Jun 14, 2018

Murdock broken build:

make: Entering directory '/tmp/dwq.0.9331483276232536/84bce6789fe2daca27b9d89f003b6097/tests/cortexm_common_ldscript'
make: *** No rule to make target '/tmp/dwq.0.9331483276232536/84bce6789fe2daca27b9d89f003b6097/build/auto_init.a', needed by '/tmp/dwq.0.9331483276232536/84bce6789fe2daca27b9d89f003b6097/build/tests_cortexm_common_ldscript_offset_0x1000.elf'.  Stop.
make: *** Waiting for unfinished jobs....

Is caused by #8913 as the rule defining BASELIBS is defined before it has its real value.

I did not see it before as I compile without parallel build in a clean environment.

@cladmi cladmi force-pushed the pr/make/cortexm_common/linkerscript branch from 640ff18 to 74e7cc2 Compare June 20, 2018 17:11
@cladmi
Copy link
Contributor Author

cladmi commented Jun 20, 2018

I rebased on commits that should handleBASELIBS definition.
It will do a separate PR for them next week.

I also added a debug test in Makefile.include to verify that BASELIBS is indeed stable in the file.

@smlng
Copy link
Member

smlng commented Jul 4, 2018

@cladmi this depends on #9451 right?

@smlng smlng added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 4, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 4, 2018

Yes, my bad I forgot to update it.

@cladmi cladmi added this to the Release 2018.10 milestone Jul 4, 2018
@cladmi cladmi force-pushed the pr/make/cortexm_common/linkerscript branch from 636d668 to 54c2b6b Compare July 5, 2018 11:41
@cladmi cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 5, 2018
@kYc0o
Copy link
Contributor

kYc0o commented Jul 5, 2018

I'd like to ACK this PR but I don't have the hardware to test.

Tests are needed for:

I'd say any of the tests on these boards should validate this PR.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 5, 2018

I will add them to the list of tested boards in tests/cortem_common_ldscript. It does not verify that the code is functionnal, but that at least, the test works as expected.

@MrKevinWeiss
Copy link
Contributor

I tested on the bluepill (using swd flasher), tests/leds make LED0 flash. I also ran the tests/cortem_common_ldscript and got

Test compilation with offset 0x2000: [OK]
Test compilation with half ROM length: [OK]
Test ROM overflow detection: [OK]
Test ROM overflow detection: [OK]

@kYc0o Let me know if you would like anything more extensive.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 6, 2018

I tested on the bluepill (using swd flasher)

Actually the goal is to keep the bootloader, this PR modifies a bit the way the offset is managed to move the code outside the space allowed for the bootloader, but I can imagine that if you flash with SWD you've already remove the bootloader. Did you change the ROM_OFFSET variable to do so?

@MrKevinWeiss
Copy link
Contributor

Did you change the ROM_OFFSET variable to do so?

Nope, I will reflash the bootloader (sometimes a pain) then attempt to vary the ROM_OFFSET using the tests/cortem_common_ldscript... After the RIOT meeting.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 6, 2018

then attempt to vary the ROM_OFFSET

The goal is not to modify it, but just test if RIOT flashed with the normal settings still works.

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jul 6, 2018

Actually the goal is to keep the bootloader

Hmmm seems like my hardware isn't happy (I cannot use the USB bootloader to program the bluepill). I will try some other ones Monday. I was able to flash with the st-link from the nucleo board or the UART setting it BOOT0 pin to 1.

@MrKevinWeiss
Copy link
Contributor

So here are the things I found... Currently the bluepill, which uses the stm32f103c8 works with swd and RIOT. Using the bootloader to flash is very outdated and full of problems. The bootloader firmware generic_boot20_pc13.bin changed the bootmode pin to B2 (which means the boot1 jumper must be connected to 1). The dfu-util also seems to show 3 instances instead of 1, requiring a serial number to specify the bootloader.

dfu-util: Invalid DFU suffix signature
dfu-util: A valid DFU suffix will be required in a future dfu-util release!!!
dfu-util: More than one DFU capable USB device found! Try `--list' and specify the serial number or disconnect all but one device

Here are the instances with the most recent bootloader

Found DFU: [1eaf:0003] ver=0201, devnum=110, cfg=1, intf=0, alt=2, name="STM32duino bootloader v1.0  Upload to Flash 0x8002000", serial="LLM 003"
Found DFU: [1eaf:0003] ver=0201, devnum=110, cfg=1, intf=0, alt=1, name="STM32duino bootloader v1.0  Upload to Flash 0x8005000", serial="LLM 003"
Found DFU: [1eaf:0003] ver=0201, devnum=110, cfg=1, intf=0, alt=0, name="STM32duino bootloader v1.0  ERROR. Upload to RAM not supported.", serial="LLM 003"

The third thing is it appears to not produce a .bin file which the bootloader requires.

Finally the last problems (though it is more likely my problem) requires it to be run with sudo or the udev rules updated.

Note that I used the dfu-util 0.8 as this is the most recent version for Ubuntu 16.04 LTS

@cladmi
Copy link
Contributor Author

cladmi commented Jul 9, 2018

The third thing is it appears to not produce a .bin file which the bootloader requires.

If you do make BOARD=bluepill PROGRAMMER=dfu-util flash it should create the binfile and use it for flashing.

ifeq ($(PROGRAMMER),dfu-util)
export ROM_OFFSET ?= 0x2000 # Skip the space needed by the embedded bootloader
export FLASHER = dfu-util
export DEBUGGER = # no debugger
export RESET = # dfu-util has no support for resetting the device
HEXFILE = $(BINFILE)
export FFLAGS = -d 1d50:6017 -s 0x08002000:leave -D "$(HEXFILE)"

@kYc0o
Copy link
Contributor

kYc0o commented Jul 10, 2018

I had a arduino-mkr1000 on my drawer, so I could test. Everything works and the bootloader is kept. I'd say then it's safe to merge.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 10, 2018

However, discovered a bug specific of that target (sam0). It works just by chance but the Makefiles need to be adapted. Can you take a look @cladmi?

@kYc0o
Copy link
Contributor

kYc0o commented Jul 10, 2018

Basically, the --defsym=_rom_offset=0x2000 variable is correctly set, however there's no logic to subtract that amount from the total size of the ROM. This can lead to no error while compiling large images which wouldn't fit the available ROM.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 17, 2018

@bergzand I will add a README with the expected output.
In the main.c I put the /* The important rules are in the Makefile */ comment but maybe it is not clear enough.

@bergzand
Copy link
Member

@bergzand I will add a README with the expected output.

Thanks, I'm afraid that mainly the negative tests might confuse people

@cladmi
Copy link
Contributor Author

cladmi commented Aug 7, 2018

I added a compile-test sub target to mention it in the documentation and a README.md.

@bergzand
Copy link
Member

ACK!

@cladmi Thank you for adding the readme. Please squash

@cladmi cladmi force-pushed the pr/make/cortexm_common/linkerscript branch from 47e9355 to 599bcee Compare August 11, 2018 05:09
@cladmi
Copy link
Contributor Author

cladmi commented Aug 11, 2018

Thank you for adding the readme.

You were right to request having one in the first place :) I just tend to forget if it is not minor changes.

I am re-running the test in our docker image for verification (but pushed before testing of course…)

@cladmi
Copy link
Contributor Author

cladmi commented Aug 11, 2018

Compilation and compile tests works on docker image and arm-none-eabi-gcc 8.1.0.

@bergzand
Copy link
Member

@cladmi Your commits show up as unverified in the github webinterface. Are you sure that those are your commits? Should we trust these changes? 😆

@cladmi
Copy link
Contributor Author

cladmi commented Aug 11, 2018

Oh it's because I am from my laptop and did not commit with my fu-berlin address. Will rebase.

@cladmi
Copy link
Contributor Author

cladmi commented Aug 11, 2018

Oh just misconfiguration of the commit gpg key ID I think, will rebase and see.

Trigger an overflow by 1 byte to detect in ROM_LEN is indeed used.
It will help testing if it is taken into account and for defining for outside
after.
Test that _rom_offfset is removed from the available _rom_length.
The variables should all always be defined.
Define _rom_offset with a conditional evaluated at execution time to allow
setting it in compilation rules and generate in the same make instance different
elf files with different configurations.
Compile two elf files with different offset and verify the linked file offset.
I only enabled samr21-xpro and iotlab nodes for the moment.
Allow defining a specific rom length to use for linking the firmware,
_fw_rom_length, instead of the default configuration to use the whole rom from
_rom_offset to the end.

 * Add cortexm_common/Makefile.include FW_ROM_SIZE configuration
 * Add an assertion that _fw_rom_length still respects _rom_length
Compile an elf file with a length equals to half the rom length.
Verify that specifying a too big _fw_rom_length for the rom is detected and
prevent compilation.
Explain the test and the output you should get.
@cladmi cladmi force-pushed the pr/make/cortexm_common/linkerscript branch from 599bcee to 6abcf2e Compare August 11, 2018 09:34
@kYc0o
Copy link
Contributor

kYc0o commented Aug 13, 2018

Congrats! One step forward for easy OTA updates 😃

@cladmi cladmi deleted the pr/make/cortexm_common/linkerscript branch August 13, 2018 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: OTA Area: Over-the-air updates CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants