Skip to content
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

Use of {runtime.platform.path} makes referencing other cores often impossible #2616

Closed
3 tasks done
egnor opened this issue May 28, 2024 · 7 comments · Fixed by #2652
Closed
3 tasks done

Use of {runtime.platform.path} makes referencing other cores often impossible #2616

egnor opened this issue May 28, 2024 · 7 comments · Fixed by #2652
Assignees
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@egnor
Copy link

egnor commented May 28, 2024

Describe the problem

(I found this issue discussed various places, but no specific open bug for the problem? I would be delighted to get a pointer if I missed something.)

The ability to reference other cores from a platform seems to be an important design feature, and it is the recommended way to create custom boards.txt entries without having to stuff every board ever made into the platform vendor's boards.txt file.

HOWEVER, in practice, for many platforms (including very popular ones like rp2040 and esp32), this referencing doesn't work. The reason is that those platform definitions reference {runtime.platform.path} heavily. For example, the arduino-esp32 platform.txt file does this sort of thing

...
tools.esp32-arduino-libs.path={runtime.platform.path}/tools/esp32-arduino-libs
...
compiler.sdk.path={tools.esp32-arduino-libs.path}/{build.mcu}
...
compiler.cpreprocessor.flags="@{compiler.sdk.path}/flags/defines" "-I{build.source.path}" -iprefix "{compiler.sdk.path}/include/" "@{compiler.sdk.path}/flags/includes" "-I{compiler.sdk.path}/{build.memory_type}/include"
...

Many of the uses are deep in long, complex operations one wouldn't want to cut-and-paste:

recipe.hooks.prebuild.4.pattern=bash -c "[ -f "{build.source.path}"/bootloader.bin ] && cp -f "{build.source.path}"/bootloader.bin "{build.path}"/{build.project_name}.bootloader.bin || ( [ -f "{build.variant.path}"/{build.custom_bootloader}.bin ] && cp "{build.variant.path}"/{build.custom_bootloader}.bin "{build.path}"/{build.project_name}.bootloader.bin || cp -f "{runtime.platform.path}"/tools/sdk/{build.mcu}/bin/bootloader_{build.boot}_{build.boot_freq}.bin "{build.path}"/{build.project_name}.bootloader.bin )"

One might say this is a bug in arduino-esp32 (and arduino-pico, and...), and they should use {build.core.path} instead. And maybe so-- but if they do, their platform.txt becomes incompatible with older versions of Arduino, e.g. before #1012 was fixed.

Speaking of #1012, the discussion there points to use of symlinks as a workaround, but that's clearly messy, difficult, fragile, and hard to automate. There's also discussion of this issue in esp8266/Arduino#5769 (ESP8266) and espressif/arduino-esp32#4691 (ESP32). (From those discussions, it would be great if {build.core.platform.path} was available to avoid needing to use {build.core.path}/../.. if trying to work with this.)

Until this is solved, external core reference and all the reuse it was supposed to enable is basically broken.

To reproduce

Attempt to reference another core from a simple boards-only platform definition, as recommended in various places

Expected behavior

Core reference works, code builds and uploads

Arduino CLI version

arduino-cli Version: 0.35.3 Commit: 95cfd65 Date: 2024-02-19T13:24:24Z

Operating system

N/A

Operating system version

All

Additional context

No response

Issue checklist

  • I searched for previous reports in the issue tracker
  • I verified the problem still occurs when using the nightly build
  • My report contains all necessary details
@egnor egnor added the type: imperfection Perceived defect in any part of project label May 28, 2024
@egnor
Copy link
Author

egnor commented May 28, 2024

Proposal:

  • if [board].use_core_platform=true (a new property), set {runtime.platform.path} to the CORE platform path (otherwise set it to the BOARD platform path as current)
  • ALWAYS set {runtime.board.platform.path} to the BOARD platform path
  • ALWAYS set {runtime.core.platform.path} to the CORE platform path
  • (maybe add {runtime.tool.platform.path} and {runtime.variant.platform.path} for completeness?)

Also fix this inconsistency:

When using the Arduino IDE, if the board uses a core reference, the platform.txt of the core platform is not used at all in defining the recipes for erase and bootloader actions. When using Arduino development software other than the Arduino IDE, the handling of properties from the core platform's platform.txt is done as usual.

This means

  • no changes to default behavior (except for fixing the weird IDE/CLI discrepancy)
  • no changes needed to existing platforms
  • doesn't break existing external-core-using boards if they work
  • enables new external-core-using boards to work with all platforms (I think)

@cmaglie
Copy link
Member

cmaglie commented Jun 17, 2024

@egnor thank you very much for the valuable bug report.

Can you provide a POC core that references an esp32/pico core (to demonstrate the issue)?
I want to work on a fix, and your proposal looks like a good solution, I need something to test on and I can make up a test, but if you already have a good example it would be much better.

@cmaglie cmaglie changed the title Use of {runtime.platform.path} makes referencing other cores often impossible Use of {runtime.platform.path} makes referencing other cores often impossible Jun 17, 2024
@egnor
Copy link
Author

egnor commented Jun 17, 2024

(will do, stay tuned!)

@egnor
Copy link
Author

egnor commented Jun 18, 2024

OK here's a minimal example: https://github.com/egnor/repro_arduino_cli_2616

  1. Install arduino-cli v1.0.0
  2. git clone https://github.com/egnor/repro_arduino_cli_2616
  3. cd repro_arduino_cli_2616/test_sketch
  4. arduino-cli compile

There's a sketch.yaml file there that defines the platform URLs, including the base Espressif platform and also that repo itself (on github). When I try, I get:

Warning: Board egnor:esp32:derived_esp32 doesn't define a 'build.board' preference. Auto-set to: ESP32_DERIVED_ESP32
cp: cannot stat '/home/egnor/source/repro_arduino_cli_2616/arduino_data/internal/egnor_esp32_1.0.1_37f3eb13a64228ba/tools/partitions/{build.partitions}.csv': No such file or directory

Used platform Version Path
egnor:esp32   1.0.1   /home/egnor/source/repro_arduino_cli_2616/arduino_data/internal/egnor_esp32_1.0.1_37f3eb13a64228ba
esp32:esp32   3.0.1   /home/egnor/source/repro_arduino_cli_2616/arduino_data/internal/esp32_esp32_3.0.1_feaea63fd96e67bc
Error during build: exit status 1

(I'm ignoring the build.board warning, the auto-setting is fine.)

The cp doesn't work because of this line (and ones around it) in the base esp32 platform.txt, which uses {runtime.platform.path} as described in the initial bug report:
https://github.com/espressif/arduino-esp32/blob/99750cd37e4bef0875d36cff7e319f327d8532a2/platform.txt#L113

(I think there might also be an issue with {build.partitions}, which should be inherited from the base esp32 boards.txt, but maybe that's just an output thing. In any case the directory is wrong.)

I have found by experience if I fudge my way through this particular issue, there are many others, as {runtime.platform.path} is used extensively in the base platform.txt definitions.

@per1234 per1234 added the topic: code Related to content of the project itself label Jun 19, 2024
@cmaglie
Copy link
Member

cmaglie commented Jun 25, 2024

Thanks @egnor
I've made a first try in #2652, but the compile still fails. Other variables are probably missing.
I've slightly changed your boards.txt by adding the following lines:

derived_esp32.build.partitions=default_8MB
derived_esp32.build.mcu=esp32s3
derived_esp32.build.use_core_platform_for_runtime_platform_path=true

I'm sure that many other definitions are missing. I'm also concerned this system could be fragile since a change in the esp32 platform.txt (variable names, or how they are used...) could lead to a breaking change in the platforms that use esp32 as a reference.

@egnor
Copy link
Author

egnor commented Jun 26, 2024

This is exciting @cmaglie !!

You're right, while the platform is referenced, nothing about the board definition is. I have updated my repo (https://github.com/egnor/repro_arduino_cli_2616) accordingly:

  • made derived_esp32 a full board definition, taking all the default options from the esp32 board
  • added an update_package.py that bumps the version, rebuilds the zipfile, edits the index file, and edits the sketch project file to all be consistent, for quicker iteration

You can see if that works? I'm going to try to pull down your PR branch and see if it's easy for me to build.

As far as fragility based on the platform definition -- a certain amount of that is inevitable with configuration extension like this BUT I agree that it feels like a lot of duplication. (Less, at least, than forking the entire platform!) I would propose some mechanism for "inheriting" one board definition from another, which I think is outside the scope of this particular piece of work. (It should be simpler than core reuse, since boards don't have associated assets, it's just a matter of copying all the key values and then overriding.)

@egnor
Copy link
Author

egnor commented Jun 26, 2024

I also had to make sure to reference the variant externally:

derived_esp32.build.variant=esp32:esp32

having done so 🎉 it works 🎉 using your PR branch!

% ../../arduino-cli/arduino-cli compile    
Downloading platform egnor:[email protected] (https://github.com/egnor/repro_arduino_cli_2616/raw/main/package_egnor_repro2616_index.json)...
Downloading index: package_index.tar.bz2 downloaded                             
Downloading index: package_egnor_repro2616_index.json downloaded                
egnor:[email protected] downloaded                                                    
Installing platform egnor:[email protected]...
Sketch uses 275041 bytes (20%) of program storage space. Maximum is 1310720 bytes.
Global variables use 17008 bytes (5%) of dynamic memory, leaving 310672 bytes for local variables. Maximum is 327680 bytes.

Used platform Version Path
egnor:esp32   1.0.4   /home/egnor/source/repro_arduino_cli_2616/arduino_data/internal/egnor_esp32_1.0.4_708e4ca792f7a01c
esp32:esp32   3.0.1   /home/egnor/source/repro_arduino_cli_2616/arduino_data/internal/esp32_esp32_3.0.1_feaea63fd96e67bc

or at least the compile succeeded, I haven't tried running it on a board, but I think this is most of the way for sure!

I am excited to see this PR land, it will be a great step forward. (And then we can get board-to-board inheritance... and maybe some way to reference local index and/or package files without hitting a public URL... and standardize on some way to add compiler defines... then we're golden!!)

@cmaglie cmaglie linked a pull request Jul 26, 2024 that will close this issue
6 tasks
@cmaglie cmaglie added this to the Arduino CLI v1.0.4 milestone Jul 26, 2024
@cmaglie cmaglie added the conclusion: resolved Issue was resolved label Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants