Skip to content

esp32: Provide a specific path to linker.lf. #16013

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.

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

Closed
wants to merge 1 commit into from

Conversation

karlp
Copy link
Contributor

@karlp karlp commented Oct 14, 2024

Providing a simple name relies on the right paths being searched at the right time. However, external boards, such as demonstrated in https://github.com/micropython/micropython-example-boards will fail to find the linker.lf file, and fail to build without providing a more explicit path. (Note that micropython-example-boards currently uses v1.21 of micropython, which predates the introduction of the linker.lf dependency, so it still works, this issue was uncovered while updating that repository to micropython v1.23)

Tested with builds for esp32 and builds and run tests for esp32-s3 with esp-idf 5.0.4, 5.1.2 and 5.2.2

Trade-offs and Alternatives

There may be other ways altogether of doing this?

Providing a simple name relies on the right paths being searched at the
right time.  However, external boards, such as demonstrated in
https://github.com/micropython/micropython-example-boards
will fail to find the linker.lf file, and fail to build without
providing a more explicit path.

Tested with builds for esp32 and builds and run tests for esp32-s3
with esp-idf 5.0.4, 5.1.2 and 5.2.2

Signed-off-by: Karl Palsson <karlp@tweak.au>
@karlp karlp force-pushed the karl-v123-esp32-linker branch from 6f9bca5 to 8c1d8d4 Compare October 14, 2024 20:38
@karlp
Copy link
Contributor Author

karlp commented Oct 14, 2024

NOTE: this was developed against v1.23 tag, but rebased to master to push. It was not further tested on master.

@dpgeorge
Copy link
Member

This looks OK to me, although I now wonder why other files in the main_esp32xx directories are found OK when building out-of-tree.

@projectgus
Copy link
Contributor

For in-tree builds, the IDF project's "main" component is set to ports/esp32/main_${IDF_TARGET} and this directory contains target-specific idf_component.yml, CMakeLists.txt and linker.lf files which are picked up by the common idf_component_register function.

For the out of tree build example in micropython-example-boards, the IDF project's main component is boards/CUSTOM_ESP32/main. This directory currently only contains idf_component.yml, CMakeLists.txt because (as you point out) there was no linker fragment at the time it was last updated.

Probably the linker fragment should be copied into the out-of-tree main component rather than hard-coded to be in-tree. This will allow out-of-tree builds to set custom linker fragments, which is probably useful sometimes.

How does that sound, @karlp?

(When discussing this with @dpgeorge we also noted that there are some other recent changes in the CMakeLists.txt file that aren't yet reflected in the out-of-tree copy. Will fix.)

@dpgeorge
Copy link
Member

Probably the linker fragment should be copied into the out-of-tree main component rather than hard-coded to be in-tree

I tested that and it works.

@karlp
Copy link
Contributor Author

karlp commented Oct 15, 2024

Perhaps, we could default to using the existing in tree linker fragments, but make it easy and clear how to override it with a local one? I'm not a uge fan of copying more into the "external boards" repo, as it's just yet more stuff that needs to be maintained and kept in sync. Especially when, at present, only esp32 classic needs a special linker fragment. the other esp32 parts are all just specifying an empty file to match the requirements. I'd like to keep it feasiible to "easily" add micropython as a submodule, rather than having forks of the main repo be the most tempting path.
I'll work up a new version for review.

@dpgeorge
Copy link
Member

Closing in favour of #16658.

@dpgeorge dpgeorge closed this Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants