esp32: improve toolchain management #38540

sylvioalves · 2021-09-14T15:41:08Z

No description provided.

github-actions · 2021-09-14T17:58:44Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_espressif	zephyrproject-rtos/hal_espressif@`0d30edf`	zephyrproject-rtos/hal_espressif@`6cb3748`	zephyrproject-rtos/hal_espressif@0d30edf0..6cb37487

Note: This message is automatically posted and updated by the Manifest GitHub Action.

sylvioalves · 2021-09-14T18:00:01Z

Rebased to fix conflict. PR ready for review.

cfriedt · 2021-09-18T13:08:10Z

@sylvioalves - I'm ok with this going in as there are no code changes and it is mainly toolchain-related. Have you run twister successfully with some representative hardware in the loop? I would like to know with some certainty that this change will not introduce any regressions not covered by CI.

cfriedt

This is fine with me, as long as @sylvioalves can run twister to ensure we have test coverage on areas not covered by CI.

sylvioalves · 2021-09-18T13:14:00Z

@sylvioalves - I'm ok with this going in as there are no code changes and it is mainly toolchain-related. Have you run twister successfully with some representative hardware in the loop? I would like to know with some certainty that this change will not introduce any regressions not covered by CI.

@cfriedt As far as I know, there is not CI for Espressif devices regarding hardware tests (yet). Zephyr's CI also does not run twister for variant espressif.

cfriedt · 2021-09-18T13:21:38Z

@cfriedt As far as I know, there is not CI for Espressif devices regarding hardware tests (yet). Zephyr's CI also does not run twister for variant espressif.

@sylvioalves Have you been able to test this out on hardware with fairly decent coverage? No compiler / linker bugs or things of that sort?

tejlmand

to avoid this being merged.
Will post comments soon.

tejlmand

small proposal that will make it possible to drop: a625c27

soc/riscv/esp32c3/Kconfig.defconfig

cmake/toolchain/espressif/target.cmake

tejlmand · 2021-09-20T19:40:28Z

boards/riscv/esp32c3_devkitm/doc/index.rst

@@ -51,21 +51,19 @@ On Linux and macOS:
 .. code-block:: console

   export ZEPHYR_TOOLCHAIN_VARIANT="espressif"
-   export ESPRESSIF_TOOLCHAIN_PATH="${HOME}/.espressif/tools/riscv32-esp-elf/1.24.0.123_64eb9ff-8.4.0/riscv32-esp-elf"
+   export ESPRESSIF_TOOLCHAIN_PATH="${HOME}/.espressif/tools/zephyr"


just curious.

It seems the espressif toolchain is now expected to be installed under a different folder per default.
(/.espressif/tools/zephyr/riscv32-esp-elf/... vs. /.espressif/tools/riscv32-esp-elf/...)
Is that part of the espressif installer itself, or is there some missing doc update on toolchain installation ?

Also what about users already having the espressif toolchain installed ?
How will they be aware that they must now move their current
.espressif/tools/riscv32-esp-elf into .espressif/tools/zephyr/riscv32-esp-elf instead.

Also, is 1.24.0.123_64eb9ff-8.4.0 no longer part of the toolchain installation path ?

@tejlmand I have explained that already in this PR. Can you check previous comments? The main reason is having the same export PATH for Espressif toolchain to all SoCs, not depending on folder names and versioning as it is right now.

So, as you can see in the document update in this PR, no more 1.24.0.123_64eb9ff-8.4.0 or toolchain name explicit in folder path.

west espressif install is the download agent responsible to download and extract the toolchain in proper place (or user can manually download it as well).

Also what about users already having the espressif toolchain installed ?

When building, it will give them cmake error with a message to re-configure toolchain path.
Is there any recommendation on how to inform users about this change besides changing the ESP32 documentation?

This is the output when running west espressif install. It also informs the user how to export path into the environment.

sylvio@pc:~/projects/espressif/zephyr/zephyr$ west espressif install === downloading ESP-IDF tools.. Installing tools: xtensa-esp32-elf, xtensa-esp32s2-elf, riscv32-esp-elf, openocd-esp32 Skipping xtensa-esp32-elf@esp-2020r3-8.4.0 (already installed) Skipping xtensa-esp32s2-elf@esp-2020r3-8.4.0 (already installed) Skipping riscv32-esp-elf@1.24.0.123_64eb9ff-8.4.0 (already installed) Skipping openocd-esp32@v0.10.0-esp32-20200709 (already installed) === downloading ESP-IDF tools completed The toolchain has been downloaded to /home/sylvio/.espressif/tools/zephyr Export or set the following variables into the environment: export ESPRESSIF_TOOLCHAIN_PATH="/home/sylvio/.espressif/tools/zephyr" export ZEPHYR_TOOLCHAIN_VARIANT="espressif"

@tejlmand I have explained that already in this PR.

If you refer to this comment #38540 (comment), then you only describe why, but my question was about who / how that path was ensured.
The installer or a guide ?

Now that question you answer with the west espressif install sentence.

Also what about users already having the espressif toolchain installed ?

When building, it will give them cmake error with a message to re-configure toolchain path.

No, you give them an error message requesting users to re-install something that is already installed.
I think we should ensure that if users have a setup with the old style then that will still be supported.

That will allow users to keep working and update the toolchain when a new revision is released and they update anyway.

By the way, your west espressif install fails on my machine.

west espressif install === downloading ESP-IDF tools.. /usr/bin/env: ‘python’: No such file or directory FATAL ERROR: command exited with status 127: ./tools/idf_tools.py --tools-json=tools/zephyr_tools.json install

because I only have python3 and no python link.
Why not use the python that was already invoked with west ?

then we can remove support for the old style at a later release, similar to what is done for other things we deprecate.

sylvioalves · 2021-09-20T19:59:27Z

cmake/toolchain/espressif/generic.cmake

+if(NOT CROSS_COMPILE_TARGET)
+  message(FATAL_ERROR
+    "Unable to find toolchain in ${TOOLCHAIN_HOME} "
+    "Run `west espressif install` to download it. Then, export to path accordingly.")


@tejlmand This is current error if toolchain not found by either missing its installation or having previous installation version.

yes, I see that.
But this just ask user to re-install, but a user already having the toolchain installed should not need to re-install it.

cfriedt · 2021-09-21T22:19:30Z

Looks like this will likely be going into a dot release.

sylvioalves · 2021-09-21T22:27:04Z

Looks like this will likely be going into a dot release.

@cfriedt I am working on it. Hope there is still room for another round.

sylvioalves · 2021-09-22T00:52:01Z

@tejlmand

I think we should ensure that if users have a setup with the old style then that will still be supported.

This update allows using already configured environment as you requested.

But this just ask user to re-install, but a user already having the toolchain installed should not need to re-install it.

I kept this message and it will only be displayed when no toolchain is exported into path.

because I only have python3 and no python link.
Why not use the python that was already invoked with west ?

I also added into this PR the necessary changes to use ${PYTHON_EXECUTABLE} as part of the scripts. There were a few places where python was hardcoded.

@cfriedt However, I had to update west.yml in order to get things in place in hal side. Now, #38543 needs to be merged before this MR. Once that gets merged, we can merge this. Do you think it can be accomplished before 2.7.0?

cfriedt · 2021-09-22T01:05:24Z

The release team says they would prefer it to go into a dot release.

sylvioalves · 2021-09-22T01:13:33Z

The release team says they would prefer it to go into a dot release.

Ok, so we wait. Thanks.

tejlmand

Thanks for the update, I think I spotted some flaws.

cmake/toolchain/espressif/generic.cmake

tejlmand

lgtm

cfriedt · 2021-09-22T13:42:46Z

Added the DNM label just until we are out of the stabilization period

In order to retrieve TOOLCHAIN_NAME from Kconfig, generic.cmake points to any toolchain as a boilerplate implementation. Now, target.cmake file handles proper toolchain selection. Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>

Add toolchain path updates into documentation. Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>

This configures esptool, partition generator and external bootloader build to use python tool defined in Zephyr's context environment. This fixes error regarding missing /usr/bin/python when python2.7 is not installed in users's machine. Partition table offset is also updated as part of this change. Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>

sylvioalves · 2021-09-28T11:54:47Z

@cfriedt Only rebased to fix merge conflict.

sylvioalves requested review from andyross, dcpleung, kgugala, nashif, pgielda and tejlmand as code owners September 14, 2021 15:41

github-actions bot added area: Boards area: Build System area: Documentation labels Sep 14, 2021

sylvioalves force-pushed the feature/improve_toolchain_path branch from e975f8b to 608b933 Compare September 14, 2021 16:06

sylvioalves self-assigned this Sep 14, 2021

dcpleung approved these changes Sep 14, 2021

View reviewed changes

sylvioalves force-pushed the feature/improve_toolchain_path branch from 608b933 to e861c60 Compare September 14, 2021 17:57

github-actions bot added the area: Modules label Sep 14, 2021

sylvioalves requested a review from glaubermaroto September 14, 2021 17:58

github-actions bot added manifest west manifest-hal_espressif labels Sep 14, 2021

sylvioalves force-pushed the feature/improve_toolchain_path branch from e861c60 to 5ff9b24 Compare September 14, 2021 17:59

sylvioalves requested a review from cfriedt September 14, 2021 18:00

glaubermaroto approved these changes Sep 14, 2021

View reviewed changes

This was referenced Sep 16, 2021

espressif: esp32 & esp32s2: drivers: clock_control #38543

Merged

openocd: esp32: add debugging support #38557

Merged

cfriedt approved these changes Sep 18, 2021

View reviewed changes

cfriedt added this to the v2.7.0 milestone Sep 18, 2021

tejlmand requested changes Sep 20, 2021

View reviewed changes

soc/riscv/esp32c3/Kconfig.defconfig Outdated Show resolved Hide resolved

cmake/toolchain/espressif/target.cmake Outdated Show resolved Hide resolved

sylvioalves force-pushed the feature/improve_toolchain_path branch 2 times, most recently from c7502a4 to 8422b57 Compare September 20, 2021 16:14

sylvioalves requested a review from tejlmand September 20, 2021 16:15

tejlmand reviewed Sep 20, 2021

View reviewed changes

sylvioalves commented Sep 20, 2021

View reviewed changes

sylvioalves force-pushed the feature/improve_toolchain_path branch 3 times, most recently from 14fdb5d to 8cc668a Compare September 22, 2021 00:35

tejlmand requested changes Sep 22, 2021

View reviewed changes

cmake/toolchain/espressif/generic.cmake Outdated Show resolved Hide resolved

cmake/toolchain/espressif/generic.cmake Outdated Show resolved Hide resolved

cmake/toolchain/espressif/generic.cmake Outdated Show resolved Hide resolved

sylvioalves force-pushed the feature/improve_toolchain_path branch from 8cc668a to d88a130 Compare September 22, 2021 10:28

sylvioalves requested a review from tejlmand September 22, 2021 10:30

tejlmand approved these changes Sep 22, 2021

View reviewed changes

cfriedt added the DNM This PR should not be merged (Do Not Merge) label Sep 22, 2021

sylvioalves added 3 commits September 28, 2021 08:53

toolchain: esp32: define target toolchain

a20ef7e

In order to retrieve TOOLCHAIN_NAME from Kconfig, generic.cmake points to any toolchain as a boilerplate implementation. Now, target.cmake file handles proper toolchain selection. Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>

docs: esp32: update environment setup content

de6bfaa

Add toolchain path updates into documentation. Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>

sylvioalves force-pushed the feature/improve_toolchain_path branch from d88a130 to 0c4917b Compare September 28, 2021 11:53

github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Sep 28, 2021

cfriedt removed this from the v2.7.0 milestone Sep 28, 2021

cfriedt merged commit fd3696a into zephyrproject-rtos:main Sep 29, 2021

sylvioalves mentioned this pull request Sep 29, 2021

Esp32 compilation error after enabling CONFIG_NEWLIB_LIBC #38978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp32: improve toolchain management #38540

esp32: improve toolchain management #38540

sylvioalves commented Sep 14, 2021

github-actions bot commented Sep 14, 2021 •

edited

Loading

sylvioalves commented Sep 14, 2021

cfriedt commented Sep 18, 2021 •

edited

Loading

cfriedt left a comment

sylvioalves commented Sep 18, 2021

cfriedt commented Sep 18, 2021 •

edited

Loading

tejlmand left a comment

tejlmand left a comment

tejlmand Sep 20, 2021 •

edited

Loading

sylvioalves Sep 20, 2021 •

edited

Loading

sylvioalves Sep 20, 2021

tejlmand Sep 21, 2021

tejlmand Sep 21, 2021

sylvioalves Sep 20, 2021

tejlmand Sep 21, 2021

cfriedt commented Sep 21, 2021

sylvioalves commented Sep 21, 2021

sylvioalves commented Sep 22, 2021

cfriedt commented Sep 22, 2021

sylvioalves commented Sep 22, 2021

tejlmand left a comment

tejlmand left a comment

cfriedt commented Sep 22, 2021

sylvioalves commented Sep 28, 2021

esp32: improve toolchain management #38540

esp32: improve toolchain management #38540

Conversation

sylvioalves commented Sep 14, 2021

github-actions bot commented Sep 14, 2021 • edited Loading

sylvioalves commented Sep 14, 2021

cfriedt commented Sep 18, 2021 • edited Loading

cfriedt left a comment

Choose a reason for hiding this comment

sylvioalves commented Sep 18, 2021

cfriedt commented Sep 18, 2021 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

sylvioalves Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

sylvioalves Sep 20, 2021

Choose a reason for hiding this comment

tejlmand Sep 21, 2021

Choose a reason for hiding this comment

tejlmand Sep 21, 2021

Choose a reason for hiding this comment

sylvioalves Sep 20, 2021

Choose a reason for hiding this comment

tejlmand Sep 21, 2021

Choose a reason for hiding this comment

cfriedt commented Sep 21, 2021

sylvioalves commented Sep 21, 2021

sylvioalves commented Sep 22, 2021

cfriedt commented Sep 22, 2021

sylvioalves commented Sep 22, 2021

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

cfriedt commented Sep 22, 2021

sylvioalves commented Sep 28, 2021

github-actions bot commented Sep 14, 2021 •

edited

Loading

cfriedt commented Sep 18, 2021 •

edited

Loading

cfriedt commented Sep 18, 2021 •

edited

Loading

tejlmand Sep 20, 2021 •

edited

Loading

sylvioalves Sep 20, 2021 •

edited

Loading