cpu/esp*: code deduplication and cleanup #12955

gschorcht · 2019-12-16T08:58:29Z

Contribution description

After PR #11108 has been merged, it became possible to deduplicate a lot of the ESP32 and ESP8266 code by moving it to cpu/esp_common. This PR realizes these deduplications and some cleanup that are tracked in issue #10658.

Testing procedure

examples/gnrc_networking can be used as test that includes most parts of the changes. This should be tested with any ESP8266 and any ESP32 board.

USEMODULE='esp_wifi' CFLAGS='-DESP_WIFI_SSID=\"ssid\" -DESP_WIFI_PASS=\"pass\"' \
make BOARD=esp... -C examples/gnrc_networking flash term

Furthermore: changed peripherals should to be tested.

flash: make BOARD=esp... -C tests/pkg_spiffs flash test
i2c: make BOARD=esp... -C tests/periph_i2c flash term
hwrng: make BOARD=esp... -C tests/periph_hwrng flash test
spi: make BOARD=esp... -C tests/periph_spi flash term
uart: make BOARD=esp... -C tests/periph_uart flash term (only possible on ESP32)

Issues/PRs references

Tracked in issue #10658
Fixes issue #11571
Contains the changes of PRs #12967, #13004
Depends on #13035

gschorcht · 2019-12-16T09:02:54Z

@MrKevinWeiss This is the result of a complete deduplication of ESP code. Do you think it would be better to split it into different PRs? The advantage of one PR is that it has to be tested only once while it has to be tested different times in case of multiple PRs.

cpu/esp32/Makefile.include

cpu/esp8266/Makefile.include

cpu/esp32/Makefile.include

cpu/esp8266/Makefile.include

cpu/esp_common/Makefile.include

cpu/esp_common/esp-wifi/esp_wifi_netdev.c

cpu/esp_common/include/irq_arch_common.h

aabadie · 2019-12-16T09:33:22Z

+1,566 −10,312

Nice!

yegorich · 2019-12-16T09:33:25Z

cpu/esp_common/include/syscalls_common.h

+/** Time since boot in ms (32bit version) */
+uint32_t system_get_time_ms (void);
+
+/** memset version that the compiler should not be allowed to optimize this */


Drop "this"?

This system functions are used in vendor code and platform implementation as well. I had to declare them anywhere. So I though to place all system function prototypes in syscalls.h which is included in a lot of files. This was the same before the deduplication.

I mean to rephrase the comment like shown below i.e. without "this" at the end:

"memset version that the compiler should not be allowed to optimize"

gschorcht · 2019-12-16T09:35:20Z

@yegorich Thanks for starting the review. Most of the typos just come from former code. I hope that codespell will also help to identify them.

I will them fix, but first we should decide whether it would be better to split the PR.

gschorcht · 2019-12-16T09:37:22Z

+1,566 −10,312

Nice!

Yeah, the complete reimplementation in PR #11108 based on new ESP8266 SDK made it possible. But it was a long way.

MrKevinWeiss · 2019-12-16T09:47:47Z

@MrKevinWeiss This is the result of a complete deduplication of ESP code. Do you think it would be better to split it into different PRs? The advantage of one PR is that it has to be tested only once while it has to be tested different times in case of multiple PRs.

I guess it is possible to keep it in 1 PR, this means of course it will take longer and be harder to get in but as long as the commits are nice it should be fine.
Again I can look at the code and run through testing but it probably won't that attention until 15.01.2020 to 30.01.2020.

gschorcht · 2019-12-16T10:02:46Z

Again I can look at the code and run through testing but it probably won't that attention until 15.01.2020 to 30.01.2020.

@MrKevinWeiss That's perfectly fine. I'm not planning any major changes to these parts of the code in the next few weeks. So it should be easy to keep this PR consistent with the current master in the next few weeks.

gschorcht · 2019-12-16T10:12:59Z

No changes, I just rebased it onto current master to resolve the conflicts after the merge of PRs #12948 and #12950.

gschorcht · 2020-02-21T08:14:58Z

@benpicco Rebased and resolved the remaining conflict as separate commit e869fbd.

gschorcht · 2020-02-21T10:18:57Z

I had to remove and add again the CI: ready for build label to retrigger the CI build. The last compilation failed with errors in tests/lwip* for all boards that are not related to this PR. These compilation errors were caused by a connectivity problem to git.savannah.nongnu.org.

benpicco · 2020-02-21T10:21:37Z

These compilation errors were caused by a connectivity problem to git.savannah.nongnu.org.

All PRs are currently failing because of that. Yet I can reach git.savannah.nongnu.org in my browser without problems.
Could it be that git.savannah.nongnu.org is doing some rate-limiting? @kaspar030

gschorcht · 2020-02-21T10:23:03Z

Could it be that git.savannah.nongnu.org is doing some rate-limiting? @kaspar030

That's very likely.

gschorcht · 2020-02-21T10:32:21Z

Yet I can reach git.savannah.nongnu.org in my browser without problems.

Me too. [UPDATE] And, I can compile tests/lwip* locally without any problems. Maybe they just adjusted their firewall rules.

Perhaps it would be better to create a mirror repository on GitHub and use it for RIOT compilations. Since we always use a particular commit of lwIP, it would be sufficient to update it only from time to time.

benpicco · 2020-02-21T16:14:41Z

The Micropython failure on samr21-xpro is to blame on #13306, but run_test/tests/pkg_littlefs2 is failing too.
Does it also fail on master? The we can ignore the test (and fix it in a separate PR).

Failure of suit_update on nrf52dk is unrelated too.

gschorcht · 2020-02-21T23:45:00Z

but run_test/tests/pkg_littlefs2 is failing too. Does it also fail on master?

Hm, that's the kind of errors I really like 😟 Instead of telling me something like on Murdock

main(): This is RIOT! (Version: buildtest)
.
littlefs_tests.tests_littlefs_format (tests/pkg_littlefs2/main.c 159) exp 0 was -19
.
littlefs_tests.tests_littlefs_mount_umount (tests/pkg_littlefs2/main.c 173) exp 0 was -22
...
run 8 failures 8

my esp32-wroom-32 board simply does nothing and is reset after a watchdog timeout. However, it works without any problems with my esp32-wrover-kit board. It works also for my esp32-wroom-32 board if debugging options are enabled. And it works when I disable the locking mechanism for the newlib.

So it seems to be a problem of any race condition and the locking mechanism. At the moment, I don't know how to figure it out.

tests/pkg_littlefs2 is very new (from yesterday). It works with the current master because the locking mechanism for the newlib is not really used.

During the write access to the SPI flash, the IROM cache is not available and only code from the IRAM can be executed. Therefore, the code of file system implementations which access the SPI flash must reside in IRAM.

gschorcht · 2020-02-22T00:58:30Z

Fortunately, we have had similar problems with other file system implementations. The code of file system implementations must be placed in IRAM (Instruction RAM). This is done by adapting the linker script.

Background: ESPs have two different sections from which code can be executed, the IRAM (Instruction RAM) and the IROM (Instruction ROM). While the code in IRAM is loaded from the SPI flash during the boot, the code in IROM is read directly from the SPI flash via a cache. It is ten times slower than the IRAM.

Unfortunately, this cache must be disabled during a write access to the SPI flash. This means that the cache and therefore the IROM is not available during the SPI Flash write access. Code that is to be executed during a write access to the SPI flash must therefore be located in the IRAM. Therefore file system implementations which use the remaining SPI Flash as MTD drive must be placed in IRAM.

In contrast to other platforms, ESPs always use the remaining SPI flash as an MTD drive. Therefore, the file system tests do not use RAM-based MTDs, but perform real write accesses.

gschorcht · 2020-02-22T01:03:16Z

The only remaining error in last compilation with commit 5b4389c was examples/micropython/samr21-xpro:gnu which is not releated to this PR.

In RIOT-OS#12955 optimization was switche to O2 because with the '-Os' option, the ESP32 hangs sporadically in 'tests/bench*' if interrupts where disabled too early by benchmark tests. Since hasn't been reproduced since and in RIOT-OS#13196 O2 was causing un-explained hardfaults, since the formentioned issue could not be reproduced we switch back to Os.

In RIOT-OS#12955 optimization was switched to O2 because with the '-Os' option, the ESP32 hangs sporadically in 'tests/bench*' if interrupts where disabled too early by benchmark tests. Since it hasn't been reproduced since and in RIOT-OS#13196 O2 was causing un-explained hardfaults, since the aforementioned issue could not be reproduced we switch back to Os by removing O2, as Os will be used by default.

gschorcht force-pushed the cpu/esp/deduplicate_and_cleanup branch from f2830ba to 077c21e Compare December 16, 2019 09:09

gschorcht mentioned this pull request Dec 16, 2019

cpu/esp*: Tracking refactoring of ESP MCU implementations #10658

Open

31 tasks