8000 cpu/esp32: enforce MAXTHREADS is at least 3 by maribu · Pull Request #18210 · RIOT-OS/RIOT · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cpu/esp32: enforce MAXTHREADS is at least 3 #18210

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 6, 2022
Merged

cpu/esp32: enforce MAXTHREADS is at least 3 #18210

merged 3 commits into from
Jul 6, 2022

Conversation

maribu
Copy link
Member
@maribu maribu commented Jun 15, 2022

Contribution description

ESP32 has an esp_timer thread in addition to the main and the idle thread. So applications that work fine with 2 threads on other platforms will break on ESP32. This adds a static_assert() to enforce MAXTHREADS is never less than 3 on ESP32 and fixes the offenders.

Testing procedure

Green Murdock and tests/pkg_fff should now pass on ESP32 boards.

Issues/PRs references

#17076 (comment)

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: tests Area: tests and testing framework labels Jun 15, 2022
@github-actions github-actions bot added the Area: cpu Area: CPU/MCU ports label Jun 15, 2022
@jenswet jenswet mentioned this pull request Jun 15, 2022
Closed
@maribu
Copy link
Member Author
maribu commented Jun 15, 2022

Test now passes:

$ make BOARD=esp32-mh-et-live-minikit BUILD_IN_DOCKER=1 flash test -C tests/pkg_fff
[...]
Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2022.07-devel-773-gad07a-cpu/esp32)
Testing fff
FFF test completed: SUCCESS
All tests successful

gschorcht
gschorcht reviewed Jun 15, 2022
View reviewed changes
tests/periph_gpio_ll/Makefile Outdated
@@ -44,7 +44,14 @@ CFLAGS += -DPIN_IN_0=$(PIN_IN_0)
CFLAGS += -DPIN_IN_1=$(PIN_IN_1)
CFLAGS += -DPIN_OUT_0=$(PIN_OUT_0)
CFLAGS += -DPIN_OUT_1=$(PIN_OUT_1)
# We only need 1 thread (+ the Idle thread on some platforms) and we really want
# this app working on all boards.
CFLAGS += -DMAXTHREADS=2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CFLAGS += -DMAXTHREADS=2

MAXTHREADS is set in the following ifeq ... else ... endif in any case, so this line should be removed here.

@maribu maribu force-pushed the cpu/esp32 branch from ad07a58 to b3cd15f Compare June 15, 2022 10:12
@gschorcht
Copy link
Contributor
gschorcht commented Jun 16, 2022 •
edited
Loading

Here again the hashes problem. It's really strange that it occurs in different PRs for exactly one but different app that is nt related to the changes in the PR and exactly one but different board:

PR #17040: tests_mtd_mapper for esp32-ethernet-kit-v1_1 (worker comsys)
PR #17844: tests_xtimer_periodic_wakeup for esp32-heltec-lora32-v2 (worker comsys)
PR #18178: tests_touch_dev for esp32-heltec-lora32-v2 (worker comsys)
PR #18207: tests/periph_i2c for esp32-heltec-lora32-v2 (worker comsys)
PR #18209: tests/xtimer_remove for esp32-heltec-lora32-v2 (worker comsys)
PR #18210: tests_pkg_fff for esp32-heltec-lora32-v2 (worker breeze)

The problem occurs mostly for esp32-heltec-lora32-v2 and mostly on worker comsys.

At the moment many compilations fail because of this problem. I have already tried to figure out what's in the additional debug information - without success.

@maribu
Copy link
Member Author
maribu commented Jun 16, 2022

Note that I can reproduce the issue locally with BUILD_IN_DOCKER=1, so that is unrelated to the worker they are executed on. With elf_diff I can see that no code has changed. A diff of the output of objdump -D of the two files looks like this:

--- a
+++ b
@@ -1,5 +1,5 @@
 
-a.elf:     file format elf32-xtensa-le
+b.elf:     file format elf32-xtensa-le
 
 
 Disassembly of section .rtc.text:
@@ -31826,145 +31826,137 @@
 00000000 <.debug_info>:
        0:	00000004000024fe 	{ excw; excw; excw }
        8:	040000               	extui	a0, a0, 0, 1
-       b:	97e001               	l32r	a0, fffe5f8c <_rtc_bss_rtc_end+0xaffe5f64>
+       b:	9ecb01               	l32r	a0, fffe7b38 <_rtc_bss_rtc_end+0xaffe7b10>
        e:	0c0000               	excw
-      11:	0056b5               	call12	57c <XtExcFrameSize+0x50c>
-      14:	b3d000               	movgez	a13, a0, a0
+      11:	005c33               	excw
+      14:	bacf00               	ceil.s	a12, f15, 0
[...]

And it is really long. With objdump -d the diff is this:

--- a
+++ b
@@ -1,5 +1,5 @@
 
-a.elf:     file format elf32-xtensa-le
+b.elf:     file format elf32-xtensa-le
 
 
 Disassembly of section .rtc.text:

So a white-space only change. The issue really is that the .debug section differs.

@maribu
Copy link
Member Author
maribu commented Jun 16, 2022 •
edited
Loading

#18216 should fix the issue *fingers crossed*

@chrysn
Copy link
Member
chrysn commented Jun 30, 2022

CI reports:

Building application "tests_pkg_fff" for "esp32-heltec-lora32-v2" with MCU "esp32".

[...]
startup.c: In function 'system_init':
startup.c:302:5: error: static assertion failed: "ESP32 requires at least 3 threads, esp_timer, idle, and main"
     static_assert(MAXTHREADS >= 3,
     ^~~~~~~~~~~~~

If I understand this correctly, where this is tested (esp32/startup.c) has different conditions than when it is set (filter arch_esp32,$(FEATURES_USED)) in the fff pkg -- is there a better criterion for the Makefiles?

This was referenced Jun 30, 2022
Merged
Merged
@maribu
Copy link
Member Author
maribu commented Jul 4, 2022

Interesting, I cannot reproduce (after a rebase) the issue Murdock found. Let me push the rebase and give it another round

@maribu
Copy link
Member Author
maribu commented Jul 6, 2022

Aha, it fails withs TEST_KCONFIG=1 because FEATURES_USED will be empty then.

maribu added 3 commits July 6, 2022 09:00
@maribu
tests/pkg_fff: Account for extra thread needed for ESP32
91afd31
@maribu
tests/periph_gpio_ll: Account for extra thread needed for ESP32
f59e910
@maribu
cpu/esp32: make sure MAXTHREADS is at least 3
026854d 
ESP32 has an esp_timer thread in addition to the main and the idle
thread. So applications that work fine with 2 threads on other
platforms will break on ESP32.
@maribu
Copy link
Member Author
maribu commented Jul 6, 2022

Hooray! All green now :)

chrysn
chrysn approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member
@chrysn chrysn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

@chrysn
Copy link
Member
chrysn commented Jul 6, 2022

As this is supposed to fix tests that currently fail, you could enable tests for merging. (But there might still be other unrelated failures -- I don''t get the good overview from mobile).

@maribu maribu 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 and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 6, 2022
@maribu
Copy link
Member Author
maribu commented Jul 6, 2022

Wow, all tests succeeded :) Citing https://ci.riot-os.org/RIOT-OS/RIOT/18210/026854d788a4038e28549f577170dfc1952a01be/output/run_test/tests/pkg_fff/esp32-wroom-32:gnu.txt for the most relevant one:

-- running on worker pi-67e8efb7 thread 1, build number 11193.
-- executing tests for tests/pkg_fff on esp32-wroom-32 (compiled with gnu toolchain):
- executing hook run_test_pre
- resetting USB power...
- waiting for tty to re-attach...
- hook run_test_pre finished
make: Entering directory '/tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff'
esptool.py --chip esp32 --port /dev/ttyUSB0 --baud 460800 --before default_reset write_flash -z --flash_mode dout --flash_freq 40m    --flash_size detect 0x1000 /tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff/bin/esp32-wroom-32/esp_bootloader/bootloader.bin 0x8000 /tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff/bin/esp32-wroom-32/partitions.bin 0x10000 /tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff/bin/esp32-wroom-32/tests_pkg_fff.elf.bin
esptool.py v2.6
Serial port /dev/ttyUSB0
Connecting........_
Chip is ESP32D0WDQ6 (revision 1)
Features: WiFi, BT, Dual Core, 240MHz, VRef calibration in efuse, Coding Scheme None
MAC: 30:ae:a4:f4:7a:e0
Uploading stub...
Running stub...
Stub running...
Changing baud rate to 460800
Changed.
Configuring flash size...
Auto-detected Flash size: 4MB
Compressed 15936 bytes to 11160...

Writing at 0x00001000... (100 %)
Wrote 15936 bytes (11160 compressed) at 0x00001000 in 0.3 seconds (effective 417.8 kbit/s)...
Hash of data verified.
Compressed 3072 bytes to 85...

Writing at 0x00008000... (100 %)
Wrote 3072 bytes (85 compressed) at 0x00008000 in 0.0 seconds (effective 3807.3 kbit/s)...
Hash of data verified.
Compressed 101344 bytes to 42502...

Writing at 0x00010000... (33 %)
Writing at 0x00014000... (66 %)
Writing at 0x00018000... (100 %)
Wrote 101344 bytes (42502 compressed) at 0x00010000 in 1.3 seconds (effective 618.9 kbit/s)...
Hash of data verified.

Leaving...
Hard resetting via RTS pin...
make: Nothing to be done for 'termdeps'.
make: Leaving directory '/tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff'
make: Entering directory '/tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff'
r
socat - open:/dev/ttyUSB0,b115200,echo=0,raw,cs8,parenb=0,cstopb=0 
32 kHz XTAL not found, switching to internal 150 kHz oscillator


Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: buildtest)
Testing fff
FFF test completed: SUCCESS
{ "All tests successful

make: Leaving directory '/tmp/dwq.0.356379192510457/f78098f9135f81be87c0eee9eec79ad9/tests/pkg_fff'
-- saving test result to cache: OK

@maribu maribu merged commit 29d2193 into RIOT-OS:master Jul 6, 2022
@maribu maribu deleted the cpu/esp32 branch July 6, 2022 15:57
@maribu
Copy link
Member Author
maribu commented Jul 6, 2022

Thx everyone :)

@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht left review comments

@chrysn chrysn chrysn approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
3 participants
@maribu @gschorcht @chrysn
0