esptool: Allow to pass the partition table CSV. by iosabi · Pull Request #16307 · RIOT-OS/RIOT

iosabi · 2021-04-11T00:46:13Z

Contribution description

The partition table of the device in the esp8266 and esp32 based boards
was set to a default table with one "factory" partition with exactly
the size of the compiled firmware. This is problematic if we want to
update the device on the field.

This patch allows to set the PARTITION_TABLE_CSV variable from the
Makefile to a .csv file with a custom partition table, for example this
could be set to a partition table with two ota entries, or with a single
factory entry but of a known fixed size.

As a side effect of the make cleanup in this patch we now support
passing -j to the make flash command so we can compile in parallel
and still run the flash commands only once at the end. Before this
patch the conversion from .elf to .elf.bin was happening before the
code was recompiled when running in parallel.

Testing procedure

make BOARD=esp8266-esp-12x Q= -C tests/lwip flash -j term

Verified that the compile commands at the end (after the link step) appear in the right order.

make BOARD=esp8266-esp-12x PARTITION_TABLE_CSV=mytable.csv -C tests/lwip flash -j term

Verified that the mytable.csv file is used instead.

Also:

make -C tests/od BOARD=esp32-wroom-32 all flash-only -j should work

For ci:

make -C tests/od BOARD=esp32-wroom-32 flash-only

Check that murdock receives the additional flash files (Green Murdock)

Issues/PRs references

Fixes #13492 due the usage of BUILD_FILES and TEST_EXTRA_FILES instead of FLASHDEPS for both files.

iosabi · 2021-04-11T00:47:11Z

@gschorcht FYI

fjmolinas

Seems sensible, but I dont have a BOARD to test, maybe @benpicco or @gschorcht could give it a run?

makefiles/tools/esptool.inc.mk

fjmolinas · 2021-04-20T09:08:09Z

@kaspar030 is the esp32-wroom32 still on the ci?

fjmolinas · 2021-04-20T09:12:49Z

Seems sensible, but I dont have a BOARD to test, maybe @benpicco or @gschorcht could give it a run?

I triggered the CI to run rests on that BOARD, it should tell us if its OK.

iosabi · 2021-04-24T13:54:56Z

I don't have a esp32-wrom board to test this, but here's what I did (PROG_DEV=/dev/null because I don't have such device):

git checkout esp_table^  # Checks out the parent commit
make -C tests/od/ BOARD=esp32-wroom-32 PROG_DEV=/dev/null RIOT_VERSION=esp32-test flash
mv tests/od/bin/esp32-wroom-32 tests/od/bin/esp32-wroom-32-head

Note that in the make command using -j causes this error: FileNotFoundError: [Errno 2] No such file or directory on the test_od.elf because of the missing dep this commit is also fixing, so no -j there.

Then, for this commit:

git checkout esp_table
make -C tests/od/ BOARD=esp32-wroom-32 PROG_DEV=/dev/null RIOT_VERSION=esp32-test flash -j
mv tests/od/bin/esp32-wroom-32 tests/od/bin/esp32-wroom-32-table

Then compare the two directories with meld, they are identical. I'm passing RIOT_VERSION= because otherwise the builds would be different due to the different version string.

Looking at the failure and the .murdock file, it seems that murdock runs flash-only which with this commit is also building the files (because of the dep) but before it wasn't, and the CI fails with make[1]: xtensa-esp32-elf-gcc: Command not found
I'll look into this a bit more.

iosabi · 2021-04-24T15:10:49Z

makefiles/tools/esptool.inc.mk

+# flashing only, but if they are added to FLASHDEPS instead a "flash-only"
+# target would cause a rebuild of $(ELFFILE) since these files depend on it and
+# ELFFILE depends on FORCE.
+BUILD_FILES += $(FLASHFILE).bin $(BINDIR)/partitions.bin


I changed these to use BUILD_FILES instead of FLASHDEPS even if they are only needed for flashing. The real problem here is that ELFFILE depends on FORCE so we can't have FLASHDEPS depend on the ELFFILE (FLASHFILE) and not trigger a rebuild. I think ELFFILE shouldn't depend on FORCE but instead have some build config dependency on a lazysponge output or similar... but that's for another bug.

I changed this back. Turns out this doesn't work either because the CI doesn't copy those files.

I don't see a way to set this up correctly because ELFFILE is FORCEd (not sure why). I filed #16385 to address that.

Meanwhile I implemented a somewhat hacky fix setting these as .PHONY when using flash-only. This should make the CI happy.

Turns out this doesn't work either because the CI doesn't copy those files.

you mean when running the tests? Try adding needed files to TEST_EXTRA_FILES.

@iosabi I took a look and I think adding to BUILD_FILES is the way to go. Simply add also a line with

TEST_EXTRA_FILES += $(FLASHFILE).bin $(BINDIR)/partitions.bin

Just below when you add to BUILD_FILES

Adding them to TEST_EXTRA_FILES works in the CI but the following doesn't work:

make -C tests/od BOARD=esp32-wroom-32 all flash-only -j

however, the following works:

make -C tests/od BOARD=esp32-wroom-32 flash -j

which I guess is good enough. I believe that flash -j works because flash: all dependency which flash-only doesn't have, and all (well link) eventually depends on the $(BUILD_FILES).

fjmolinas · 2021-04-26T06:36:49Z

@iosabi during a review process, please avoid push forcing unless asked, approved by the reviewer, it makes following your changes harder, especially if you rebase as well, you can take a look at our contributing guidelines for more add-fixup-commits-during-review.

fjmolinas

I took a closer look and I think your initial approach was the correct one @iosabi, just add TEST_EXTRA_FILES as suggested by @kaspar030.

Also I think these changes would fix #13492? no?

makefiles/tools/esptool.inc.mk

fjmolinas · 2021-04-26T07:28:37Z

makefiles/tools/esptool.inc.mk

+# it will cause it to be rebuilt even in "flash-only" mode because ELFFILE is
+# FORCEd. To avoid that we either add the dependency when calling make with
+# "all" or "flash", or we make it a PHONY target when only doing "flash-only".
+$(FLASHFILE).bin:


Suggested change

$(FLASHFILE).bin:

$(FLASHFILE).bin: $(FLASHFILE)

Or how about sticking to ELFFILE instead?

Suggested change

$(FLASHFILE).bin:

$(ELFFILE).bin: $(ELFFILE)

Strictly speaking, I think FLASHFILE should be $(ELFFILE).bin. I made that change in a second fixup commit now.

makefiles/tools/esptool.inc.mk

fjmolinas · 2021-04-26T07:33:33Z

I took a closer look and I think your initial approach was the correct one @iosabi, just add TEST_EXTRA_FILES as suggested by @kaspar030.

Also I think these changes would fix #13492? no?

I have an esp32-wroom-32 so I believe I can test this as well, I modified your test procedure to account for the corner cases. Thanks a lot for investigating this!

iosabi · 2021-04-26T11:41:36Z

I have an esp32-wroom-32 so I believe I can test this as well, I modified your test procedure to account for the corner cases. Thanks a lot for investigating this!

The last branch should do it. I added two fixup commits. Sorry to force push them earlier, you should be able to see the diff if you click on the force-pushed word in the automatic message from github if you want to see that.

fjmolinas · 2021-04-26T16:32:56Z

makefiles/tools/esptool.inc.mk

+# table setting PARTITION_TABLE_CSV.
+PARTITION_TABLE_CSV ?= $(BINDIR)/partitions.csv
+
+$(BINDIR)/partitions.csv: $(FLASHFILE)


Suggested change

$(BINDIR)/partitions.csv: $(FLASHFILE)

$(BINDIR)/partitions.csv: $(ELFFILE).bin

I believe this one is better to leave as FLASHFILE. If you are using the default partition table but want to use a different FLASHFILE you can do something like:

FLASHFILE = $(BINDIR)/myflashfile.bin $(BINDIR)/myflashfile.bin: $(ELFFILE).bin my-extra-static-data cat $^ > $@

The FFLAGS has 0x10000 $(FLASHFILE) so the default partition table should match that. If we were to implement OTA at some point then we wouldn't be using this default partition table and this point doesn't matter; but I think this should match what FFLAGS has and using FLASHFILE gives a tiny bit more flexibility.

I changed the body of the rule to use $< to avoid writing this twice.

fjmolinas · 2021-04-26T16:33:33Z

Looks good @iosabi can you address my final comment, you can squash afterwards!

The partition table of the device in the esp8266 and esp32 based boards was set to a default table with one "factory" partition with exactly the size of the compiled firmware. This is problematic if we want to update the device on the field. This patch allows to set the `PARTITION_TABLE_CSV` variable from the Makefile to a .csv file with a custom partition table, for example this could be set to a partition table with two ota entries, or with a single factory entry but of a known fixed size.

iosabi · 2021-04-26T22:51:32Z

Looks good @iosabi can you address my final comment, you can squash afterwards!

I added the last change to 6053f81 and then squashed and force-pushed.

fjmolinas · 2021-04-27T06:21:01Z

@iosabi can you post some kind of results for your custom partition table test?

iosabi · 2021-04-27T11:32:25Z

@iosabi can you post some kind of results for your custom partition table test?

Sure!

First we need a custom partition table like this:

$ cat mypartitions.csv 
# Name,   Type, SubType, Offset,   Size
nvs,      data, nvs,     0x9000,   0x6000
phy_init, data, phy,     0xf000,   0x1000
ota_0,    app,  ota_0,   0x010000, 0x70000
ota_1,    app,  ota_1,   0x080000, 0x70000
otadata,  data, ota,     0x0f0000, 0x2000

Then we can run the following command:

USEMODULE=esp_log_startup make -C tests/shell BOARD=esp8266-esp-12x PARTITION_TABLE_CSV=`pwd`/mypartitions.csv -j flash

I (66) boot: SPI Mode       : DOUT
I (69) boot: SPI Flash Size : 4MB
I (72) boot: Partition Table:
I (74) boot: ## Label            Usage          Type ST Offset   Length
I (81) boot:  0 nvs              WiFi data        01 02 00009000 00006000
I (87) boot:  1 phy_init         RF data          01 01 0000f000 00001000
I (93) boot:  2 ota_0            OTA app          00 10 00010000 00070000
I (100) boot:  3 ota_1            OTA app          00 11 00080000 00070000
I (106) boot:  4 otadata          OTA data         01 00 000f0000 00002000
I (113) boot: End of partition table
I (116) boot: No factory image, trying OTA 0
I (120) esp_image: segment 0: paddr=0x00010010 vaddr=0x40210010 size=0x3b950 (244048) map
I (250) esp_image: segment 1: paddr=0x0004b968 vaddr=0x3ffe8000 size=0x0112c (  4396) load
I (252) esp_image: segment 2: paddr=0x0004ca9c vaddr=0x3ffe912c size=0x0039c (   924) load
I (255) esp_image: segment 3: paddr=0x0004ce40 vaddr=0x40100000 size=0x08e30 ( 36400) load
I (280) boot: Loaded app from partition at offset 0x10000

Starting ESP8266 CPU with ID: 00199647

Note that the partition table is not the best, FWs are better if they start at 0x010000 and 0x110000 so you can use the same fw at either slot, but that's not working due to #16402 (larger partition tables work fine with a newer bootloader binary).

fjmolinas

ACK, CI is happy changes look good and @iosabi provided test output for the new functionality.

fjmolinas requested review from benpicco and gschorcht April 12, 2021 09:31

fjmolinas added Area: build system Area: Build system Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 12, 2021

fjmolinas reviewed Apr 20, 2021

View reviewed changes

makefiles/tools/esptool.inc.mk Show resolved Hide resolved

fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 20, 2021

iosabi commented Apr 24, 2021

View reviewed changes

iosabi requested a review from fjmolinas April 24, 2021 15:11

fjmolinas reviewed Apr 26, 2021

View reviewed changes

iosabi requested a review from fjmolinas April 26, 2021 14:47

fjmolinas reviewed Apr 26, 2021

View reviewed changes

fjmolinas approved these changes Apr 27, 2021

View reviewed changes

fjmolinas merged commit 31a6aa1 into RIOT-OS:master Apr 27, 2021

iosabi deleted the esp_table branch May 1, 2021 14:19

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

	$(BINDIR)/partitions.csv: $(FLASHFILE)
	$(BINDIR)/partitions.csv: $(ELFFILE).bin

Conversation

iosabi commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

iosabi commented Apr 11, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas commented Apr 20, 2021

Uh oh!

fjmolinas commented Apr 20, 2021

Uh oh!

iosabi commented Apr 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Apr 26, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Apr 26, 2021

Uh oh!

iosabi commented Apr 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Apr 26, 2021

Uh oh!

iosabi commented Apr 26, 2021

Uh oh!

fjmolinas commented Apr 27, 2021

Uh oh!

iosabi commented Apr 27, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iosabi commented Apr 11, 2021 •

edited

Loading