cpu/esp32: fixes compile problems by gschorcht · Pull Request #10362 · RIOT-OS/RIOT

gschorcht · 2018-11-10T13:25:15Z

Contribution description

This PR fixes the following compilation problems:

bypassing the module management system by directly accesing posix headers without depending on the posix module as described in PR sys/posix: make posix module provide only headers. #10357
compile error when using a WiFi module such as esp_now due to lack of dependency on the pthread module
different other minor dependency inconsistencies when WiFi modules like esp_now are used

Testing procedure

Try to compile a test application with a combination of spiffs and esp_now modules, e.g.:

USEMODULE="spiffs esp_now" make BOARD=esp32-wroom-32 -C tests/shell

The compilation should be successful.

Issues/PRs references

See also PR #10357.

cladmi · 2018-11-12T11:56:44Z

cpu/esp32/Makefile.include

    LINKFLAGS += $(BINDIR)/esp_idf_esp32.a
    LINKFLAGS += $(BINDIR)/esp_idf_nvs_flash.a
    LINKFLAGS += $(BINDIR)/esp_idf_spi_flash.a
+    LINKFLAGS += $(BINDIR)/pthread.a


Putting the libraries into the linker command should already be done by

RIOT/makefiles/modules.inc.mk

Line 7 in 98f42ee

export BASELIBS += $(REALMODULES:%=$(BINDIR)/%.a)

However I see from the other parts that you duplicate dependency handling in both in Makefile.include and Makefile.dep. This comes because dependencies are resolved after board and cpu Makefile.include and I am currently trying to address it #9913

@cladmi

Putting the libraries into the linker command should already be done by

Unfortunatly, it is a bit more tricky and I was not able to figure out another solution when I had to add the vendor libraries:

LINKFLAGS += -lcore -lrtc -lnet80211 -lpp -lsmartconfig -lcoexist LINKFLAGS += -lwps -lwpa -lwpa2 -lespnow -lmesh -lphy -lstdc++

References in these libraries could only be resolved only by grouping them with module archives

esp_idf.a esp_idf_esp32.a esp_idf_nvs_flash.a esp_idf_spi_flash.a riot_freertos.a

even though they were already added by RIOT module makefiles. But they were in another --start-group ... --end-group and I could not figure out how to add further libraries to this list. Furthermore, these module archives reference symbols from other RIOT modules. So I had also to group

xtimer.a core.a pthread.a

with the libraries to be able to resolve all symbols.

With your hint to the BASELIBS variable, I could solve the problem as following:

BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libcore.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/librtc.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libnet80211.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libpp.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libsmartconfig.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libcoexist.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libwps.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libwpa.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libwpa2.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libespnow.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libmesh.a BASELIBS += $(ESP32_SDK_DIR)/components/esp32/lib/libphy.a

Would this be the right way?

@cladmi

However I see from the other parts that you duplicate dependency handling in both in Makefile.include and Makefile.dep.

Yes, the reason was that the dependencies are included too late.

@gschorcht I did not know that being in the same --start-group --end-group was important

For switching your -l to putting archives in BASELIBS I am not sure it is better to list all archives. I do not know the difference for the toolchain between the two.
Would it work and be sensible to put your -l into BASELIBS directly ? it would put it in the same group even if the variable name does not describe this

RIOT/Makefile.include

Line 433 in 0319a56

_LINK = $(if $(CPPMIX),$(LINKXX),$(LINK)) $(UNDEF) $(LINKFLAGPREFIX)--start-group $(BASELIBS) -lm $(LINKFLAGPREFIX)--end-group $(LINKFLAGS) $(LINKFLAGPREFIX)-Map=$(BINDIR)/$(APPLICATION).map

Regarding missing symbols there could also be issues with the current linking process as explained first in #5757 currently followed in this PR #8711

With this diff to your PR it also compiles for your test command (not tested!)

diff --git a/cpu/esp32/Makefile.include b/cpu/esp32/Makefile.include index 12ff0b1af..89e3d3814 100644 --- a/cpu/esp32/Makefile.include +++ b/cpu/esp32/Makefile.include @@ -121,21 +121,8 @@ LINKFLAGS += -L$(ESP32_SDK_DIR)/components/esp32/lib LINKFLAGS += -Wl,--start-group ifneq (,$(filter esp_wifi_any,$(USEMODULE))) - LINKFLAGS += $(BINDIR)/cpu.a - LINKFLAGS += $(BINDIR)/esp_idf.a - LINKFLAGS += $(BINDIR)/esp_idf_esp32.a - LINKFLAGS += $(BINDIR)/esp_idf_nvs_flash.a - LINKFLAGS += $(BINDIR)/esp_idf_spi_flash.a - LINKFLAGS += $(BINDIR)/pthread.a - LINKFLAGS += $(BINDIR)/riot_freertos.a - LINKFLAGS += $(BINDIR)/xtimer.a - LINKFLAGS += -lcore -lrtc -lnet80211 -lpp -lsmartconfig -lcoexist - LINKFLAGS += -lwps -lwpa -lwpa2 -lespnow -lmesh -lphy -lstdc++ -endif - -ifneq (,$(filter pthread,$(USEMODULE))) - LINKFLAGS += $(BINDIR)/core.a - LINKFLAGS += $(BINDIR)/pthread.a + BASELIBS += -lcore -lrtc -lnet80211 -lpp -lsmartconfig -lcoexist + BASELIBS += -lwps -lwpa -lwpa2 -lespnow -lmesh -lphy -lstdc++ endif LINKFLAGS += -lhal -lg -lc -lg

When putting -lhal -lg -lc -lg into BASELIBS it failed to compiled but did not investigate further.

@cladmi Thanks, based on your hints I was able to further reduce the special stuff.

Cool :) Could you split this part out as it can be merged alone.

The handling of INCLUDES path and conflicts is a different topic where there are other things to consider. I am also still not sure of what to do there.

@cladmi Do you mean as a separate pull request or just a separate commit?

I would say separate PRs as I could merge the BASELIBS changes alone earlier.

I wanted to have the split merged before this one but I forgot to put the label >< My bad.

jcarrano · 2018-11-19T15:02:51Z

I'm trying to test this, but I cannot run your command neither with this PR nor with master:

$ BOARD=esp32-wroom-32 make DOCKER="sudo docker" BUILD_IN_DOCKER=1 USEMODULE="spiffs esp_now"  -C tests/shell
make: Entering directory '/home/jcarrano/source/masterRIOT/tests/shell'
ESP32_SDK_DIR should be defined as /path/to/esp-idf directory
ESP32_SDK_DIR is set by default to /opt/esp/esp-idf
make: /home/jcarrano/source/masterRIOT/cpu/esp32/Makefile.dep:15: pipe: Too many open files
make: /home/jcarrano/source/masterRIOT/cpu/esp32/Makefile.dep:15: pipe: Too many open files
/home/jcarrano/source/masterRIOT/boards/common/esp32/Makefile.dep:1: *** Too many open files.  Stop.
make: Leaving directory '/home/jcarrano/source/masterRIOT/tests/shell'

I think it may have something to do with the recursive dependency resolution. Using USEMODULE="spiffs" or USEMODULE="esp_now" does not fail but the combination does.

Is this a bug?

cladmi · 2018-11-19T15:10:49Z

@jcarrano You are putting USEMODULE= on the command line side instead of setting it through the environment variables, I understand how it could mess up with our Makefile.dep processing.

jcarrano · 2018-11-19T15:13:03Z

It seems to work:

$ BOARD=esp32-wroom-32 USEMODULE="spiffs esp_now" make DOCKER="sudo docker" BUILD_IN_DOCKER=1 -C tests/shell
make: Entering directory '/home/jcarrano/source/masterRIOT/tests/shell'
ESP32_SDK_DIR should be defined as /path/to/esp-idf directory
ESP32_SDK_DIR is set by default to /opt/esp/esp-idf
make[1]: Nothing to be done for 'Makefile.include'.
Launching build container using image "riot/riotbuild:latest".
sudo docker run --rm -t -u "$(id -u)" \
    -v '/home/jcarrano/source/masterRIOT:/data/riotbuild/riotbase' \
    -v '/home/jcarrano/source/masterRIOT/cpu:/data/riotbuild/riotcpu' \
    -v '/home/jcarrano/source/masterRIOT/boards:/data/riotbuild/riotboard' \
    -v '/home/jcarrano/source/masterRIOT/makefiles:/data/riotbuild/riotmake' \
    -v '/home/jcarrano/source/masterRIOT:/data/riotbuild/riotproject' \
    -v /etc/localtime:/etc/localtime:ro \
    -e 'RIOTBASE=/data/riotbuild/riotbase' \
    -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' \
    -e 'RIOTCPU=/data/riotbuild/riotcpu' \
    -e 'RIOTBOARD=/data/riotbuild/riotboard' \
    -e 'RIOTMAKE=/data/riotbuild/riotmake' \
    -e 'RIOTPROJECT=/data/riotbuild/riotproject' \
    -v /home/jcarrano/.gitcache:/data/riotbuild/gitcache -e GIT_CACHE_DIR=/data/riotbuild/gitcache -v /home/jcarrano/source/vanillaRIOT/.git:/home/jcarrano/source/vanillaRIOT/.git \
    -e 'BOARD=esp32-wroom-32' \
    -w '/data/riotbuild/riotproject/tests/shell/' \
    'riot/riotbuild:latest' make  
ESP32_SDK_DIR should be defined as /path/to/esp-idf directory
ESP32_SDK_DIR is set by default to /opt/esp/esp-idf
Building application "tests_shell" for "esp32-wroom-32" with MCU "esp32".

"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/div
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/sys/log
"make" -C /data/riotbuild/riotbase/sys/luid
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/ps
"make" -C /data/riotbuild/riotbase/sys/random
"make" -C /data/riotbuild/riotbase/sys/random/tinymt32
"make" -C /data/riotbuild/riotbase/sys/shell
"make" -C /data/riotbuild/riotbase/sys/shell/commands
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/sys/tsrb
"make" -C /data/riotbuild/riotbase/sys/xtimer
"make" -C /data/riotbuild/riotboard/esp32-wroom-32
"make" -C /data/riotbuild/riotboard/common/esp32
"make" -C /data/riotbuild/riotcpu/esp32
"make" -C /data/riotbuild/riotcpu/esp32/freertos
"make" -C /data/riotbuild/riotcpu/esp32/periph
"make" -C /data/riotbuild/riotcpu/esp32/vendor
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/driver
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/esp32
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/heap
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/soc
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/spi_flash
"make" -C /data/riotbuild/riotcpu/esp32/vendor/esp-idf/wpa_supplicant
"make" -C /data/riotbuild/riotcpu/esp32/vendor/xtensa
   text	   data	    bss	    dec	    hex	filename
  71765	   4904	   6352	  83021	  1444d	/data/riotbuild/riotproject/tests/shell/bin/esp32-wroom-32/tests_shell.elf
make: Leaving directory '/home/jcarrano/source/masterRIOT/tests/shell'

gschorcht · 2018-11-20T00:03:33Z

@cladmi Sorry, I have lost the focus on it and forgot to split the PR. Now, I have it done and @jcarrano got it also working. Can we merge it in near future? Issue #10432 seems to be related to it.

TimoRoth · 2018-11-20T12:31:46Z

I can confirm that cherry-picking this PR on top of 2018.10 fixes the build issue from #10432 for me.

jcarrano

Tested, it also seems to fix #10432 .

gschorcht · 2018-11-21T13:22:46Z

@jcarrano Thanks 😃

cladmi · 2018-11-23T13:48:30Z

@gschorcht please tell me when you have a PR split to put the libraries in BASELIBS, or if you want me to do it.

gschorcht · 2018-11-23T17:12:57Z

@cladmi Done, see PR #10460

cpu/esp32: fixes compile problems

16f0bf4

gschorcht mentioned this pull request Nov 10, 2018

sys/posix: make posix module provide only headers. #10357

Merged

gschorcht requested a review from jcarrano November 10, 2018 14:01

cladmi reviewed Nov 12, 2018

View reviewed changes

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Nov 19, 2018

jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 19, 2018

gschorcht mentioned this pull request Nov 19, 2018

ESP32 toolchain linked to in doc.txt is broken #10432

Closed

cpu/esp32: fixes compile problems

2a7c33b

gschorcht force-pushed the esp32_compile_fix branch 2 times, most recently from 8d52ae1 to 2a7c33b Compare November 19, 2018 23:59

jcarrano approved these changes Nov 21, 2018

View reviewed changes

jcarrano merged commit 68d68d1 into RIOT-OS:master Nov 21, 2018

gschorcht deleted the esp32_compile_fix branch November 23, 2018 17:13

cladmi mentioned this pull request Nov 26, 2018

cpu/esp32: fixes LINKFLAGS in Makefile.include #10460

Merged

aabadie added this to the Release 2019.01 milestone Dec 2, 2018

Conversation

gschorcht commented Nov 10, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

gschorcht Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jcarrano commented Nov 19, 2018

Uh oh!

cladmi commented Nov 19, 2018

Uh oh!

jcarrano commented Nov 19, 2018

Uh oh!

gschorcht commented Nov 20, 2018

Uh oh!

TimoRoth commented Nov 20, 2018

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Nov 21, 2018

Uh oh!

cladmi commented Nov 23, 2018

Uh oh!

gschorcht commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gschorcht commented Nov 10, 2018 •

edited

Loading

gschorcht Nov 12, 2018 •

edited

Loading

gschorcht commented Nov 23, 2018 •

edited

Loading