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

Tracking issue to use native build strategy as default #10

Closed
4 of 7 tasks
N3xed opened this issue Sep 10, 2021 · 12 comments
Closed
4 of 7 tasks

Tracking issue to use native build strategy as default #10

N3xed opened this issue Sep 10, 2021 · 12 comments

Comments

@N3xed
Copy link
Collaborator

N3xed commented Sep 10, 2021

This issue tries to list everything that has to be done for us to switch to the native esp-idf build strategy.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Sep 14, 2021

Configuration of sdkconfig is much different for native, is this OK?

IMO, we should try to unify these.
By the way, I think there is a small issue here and the user-defined sdkconfig / sdkconfig.defaults is not really picked up. I'll contribute a small fix in the next days (working on making the native build working with ESP-IDF master ATM)

Currently no tools like cargo pio espidf exist for the native, should we add an equivalent tool before we set it as default?

IMO we do need at least the menuconfig tool, so that the user can visually configure the sdkconfig file (separately for debug vs release).

Second priority (but still very useful) would be support for the ESP-IDF monitor, as it is capable of properly decrypting panic backtraces even for RiscV chips.

@N3xed
Copy link
Collaborator Author

N3xed commented Sep 30, 2021

Small status update for this issue

  • I tried writing my own crate for downloading and unpacking tools here but I wasn't really satisfied with what I came up with and it also seems overkill right now.
  • I've also looked a little bit into idf-env, but it seems difficult to use this instead of directly using idf_tools.py. As it's a binary crate (depending on binary crates is currently unstable) we would need to publish idf-env as a library to crates.io. Additionally, it would mean more research into the code-base and many more dependencies for the end-user.
  • In the meantime (until I find a better solution), I will just move my install logic from the esp-idf-sys' native build script to a new espidf module in embuild and abuse idf-tools.py to install custom tools that we need (with a custom tools.json). I'm moving this logic to embuild, so that we have a more or less general solution to detecting installed esp-idf tools and the esp-idf itself. This allows us then create a tool similar to cargo-pio.

@ivmarkov
Copy link
Collaborator

  • In the meantime (until I find a better solution), I will just move my install logic from the esp-idf-sys' native build script to a new espidf module in embuild and abuse idf-tools.py to install custom tools that we need (with a custom tools.json). I'm moving this logic to embuild, so that we have a more or less general solution to detecting installed esp-idf tools and the esp-idf itself. This allows us then create a tool similar to cargo-pio.

I agree with that plan.
Note that in the long term we still might have to reconsider depending on a library incarnation of idf-env, because - if I understood correctly - the plan of Espressif is to gradually retire idf-tools.py & other installer logic which is currently in Python/scripts in favor of a Rust-based solution.

@N3xed
Copy link
Collaborator Author

N3xed commented Nov 7, 2021

Configuration of sdkconfig and sdkconfig.defaults

We have to find a consensus as to what the way to configure these is. I believe my way (setting ESP_IDF_SDKCONFIG and ESP_IDF_SDKCONFIG_DEFAULTS) makes the most sense and follows the way they are configured for the esp-idf itself (minus the ESP_IDF_ prefix). I'm not sure though how applicable this approach is to the pio build.

Please tell me what you think?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 7, 2021

Configuration of sdkconfig and sdkconfig.defaults

We have to find a consensus as to what the way to configure these is. I believe my way (setting ESP_IDF_SDKCONFIG and ESP_IDF_SDKCONFIG_DEFAULTS) makes the most sense and follows the way they are configured for the esp-idf itself (minus the ESP_IDF_ prefix). I'm not sure though how applicable this approach is to the pio build.

Please tell me what you think?

How would you copy and track the custom partition table when there is one? The PlatformIO mechanism is generic for these reasons precisely - as there might be multiple things you might want to shuffle into the native PlatformIO project prior to building.

@N3xed
Copy link
Collaborator Author

N3xed commented Nov 7, 2021

How would you copy and track the custom partition table when there is one? The PlatformIO mechanism is generic for these reasons precisely - as there might be multiple things you might want to shuffle into the native PlatformIO project prior to building.

That is indeed a good question.

If it's just the partition table, which as far as I know, is only used at the end of the build for size checking and then to flash its binary version. And since we have a dummy project which doesn't actually use anything from the esp-idf, the compiled size is almost zero, so the size check should really always pass.
So if what is said is the case, a custom partition table should be irrelevant for esp-idf-sys (and the compilation of the esp-idf).

But besides that, are there other things other than the sdkconfig and partition table that the esp-idf needs and that are critical for the compilation of the esp-idf?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 8, 2021

Well, if the partition table CSV file is really only used (in our case) for size check comparisons, then the partition table problem is solved.

The PIO builder does use the generic glob-based copying mechanism internally to patch the ESP-IDF itself (the story is a bit complicated), and I would actually like to preserve the glob functionality, because the PIO builder can also be used to drive a cargo-first build for non-esp-idf-sys and non-ESP-IDF projects too, where generic glob-based copying might be useful.

But we can always layer your set of variables on top of my PIO glob patterns and this way achieve parity between the two builders. One annoying complication is that regarding sdkconfig, PIO actually uses two separate files:
sdkconfig.release and sdkconfig.debug - depending on what type of build you are performing. But I think I can hide this somehow behind the scenes.

As for other files that we might have to contribute to the ESP-IDF build... not sure. I couldn't find others, but I could've missed something.

@N3xed
Copy link
Collaborator Author

N3xed commented Nov 8, 2021

The PIO builder does use the generic glob-based copying mechanism internally to patch the ESP-IDF itself (the story is a bit complicated), and I would actually like to preserve the glob functionality, because the PIO builder can also be used to drive a cargo-first build for non-esp-idf-sys and non-ESP-IDF projects too, where generic glob-based copying might be useful.

I agree we shouldn't remove the glob functionality. Maybe in the future, the esp-idf needs some other custom files why by that mechanism can easily be added to its build. (So lets add this mechanism to the native build too)

But we can always layer your set of variables on top of my PIO glob patterns and this way achieve parity between the two builders.

Does the esp-idf pio platform not support giving the sdkconfig and sdkconfig.defaults as paths? Because if it does we could do this instead of copying like I'm doing with the native build.

If it doesn't support this:

Because the sdkconfig is such a common file that really everyone that does something non-trivial needs to change, my proposal would be to special-case these files. Meaning, instead of using the glob mechanism for copying these, we can instead add some logic that just looks at the ESP_IDF_SDKCONFIG and ESP_IDF_SDKCONFIG_DEFAULTS variables and specially copies these files into the pio project.

I think for the sdkconfig having a special way to set it makes more sense than using glob to copy it. This way we can also just set the paths to these files if the build system supports it.

One annoying complication is that regarding sdkconfig, PIO actually uses two separate files: sdkconfig.release and sdkconfig.debug - depending on what type of build you are performing. But I think I can hide this somehow behind the scenes.

Wouldn't this feature also be nice to have in the native build?

But also the whole build type opens another can of worms that we should really address before setting the native build as default. Currently, we have two independent settings that determine the optimization level:

  • The cargo profile

    It is used by the cmake crate to determine which CMAKE_BUILD_TYPE it should set. CMake then uses this to add the appropriate compiler flags. (Note: That this also affects the bootloader build, which isn't ideal)

  • CONFIG_COMPILER_OPTIMIZATION configured in the sdkconfig

    This is separate from CMake's mechanism and depending and this optimization flags will also be added in addition to the above.

So, my idea would be to use the cargo profile settings to determine the CONFIG_COMPILER_OPTIMIZATION or use any configured in the user-supplied sdkconfig. And use the cargo profile name to determine which sdkconfig.<profile> to use, like pio. Finally, we should prevent the cmake crate from setting a CMAKE_BUILD_TYPE to stop interfering with the bootloader optimization configuration in the sdkconfig and as it is already configured in the sdkconfig.

@N3xed
Copy link
Collaborator Author

N3xed commented Nov 12, 2021

@ivmarkov Any comments on this?

If not, I'll whip up an implementation of this.

@ivmarkov
Copy link
Collaborator

The PIO builder does use the generic glob-based copying mechanism internally to patch the ESP-IDF itself (the story is a bit complicated), and I would actually like to preserve the glob functionality, because the PIO builder can also be used to drive a cargo-first build for non-esp-idf-sys and non-ESP-IDF projects too, where generic glob-based copying might be useful.

I agree we shouldn't remove the glob functionality. Maybe in the future, the esp-idf needs some other custom files why by that mechanism can easily be added to its build. (So lets add this mechanism to the native build too)

+1

But we can always layer your set of variables on top of my PIO glob patterns and this way achieve parity between the two builders.

Does the esp-idf pio platform not support giving the sdkconfig and sdkconfig.defaults as paths? Because if it does we could do this instead of copying like I'm doing with the native build.

Not that I'm aware of. Maybe I'm missing something, but unlikely. FYI, the black magic PlatformIO does on top of ESP-IDF is here, in their "espressif32" "platform".

If it doesn't support this:

Because the sdkconfig is such a common file that really everyone that does something non-trivial needs to change, my proposal would be to special-case these files. Meaning, instead of using the glob mechanism for copying these, we can instead add some logic that just looks at the ESP_IDF_SDKCONFIG and ESP_IDF_SDKCONFIG_DEFAULTS variables and specially copies these files into the pio project.

I'm fine with that. In fact, you might re-use the glob mechanism to copy the files as well, but actually up to you.

I think for the sdkconfig having a special way to set it makes more sense than using glob to copy it. This way we can also just set the paths to these files if the build system supports it.

One annoying complication is that regarding sdkconfig, PIO actually uses two separate files: sdkconfig.release and sdkconfig.debug - depending on what type of build you are performing. But I think I can hide this somehow behind the scenes.

Wouldn't this feature also be nice to have in the native build?

My only concern is that it is not very "ESP-IDFy". And, I don't see a similar mechanism in PIO for sdkconfig.defaults.
I'm actually OK to stick with plain sdkconfig as well, but then we have to touch cargo pio espidf menuconfig as well - and - if you feel sdkconfig.debug|release has benefits in the native world as well, I'm fine with that.

But also the whole build type opens another can of worms that we should really address before setting the native build as default. Currently, we have two independent settings that determine the optimization level:

  • The cargo profile
    It is used by the cmake crate to determine which CMAKE_BUILD_TYPE it should set. CMake then uses this to add the appropriate compiler flags. (Note: That this also affects the bootloader build, which isn't ideal)
  • CONFIG_COMPILER_OPTIMIZATION configured in the sdkconfig
    This is separate from CMake's mechanism and depending and this optimization flags will also be added in addition to the above.

So, my idea would be to use the cargo profile settings to determine the CONFIG_COMPILER_OPTIMIZATION or use any configured in the user-supplied sdkconfig. And use the cargo profile name to determine which sdkconfig.<profile> to use, like pio. Finally, we should prevent the cmake crate from setting a CMAKE_BUILD_TYPE to stop interfering with the bootloader optimization configuration in the sdkconfig and as it is already configured in the sdkconfig.

I'm OK with that too.

@ivmarkov
Copy link
Collaborator

@ivmarkov Any comments on this?

If not, I'll whip up an implementation of this.

I don't have strong feelings/disagreements with any of your statements from above, so by all means - go ahead.

@N3xed
Copy link
Collaborator Author

N3xed commented Apr 28, 2022

573bcc6 made native the default.

@N3xed N3xed closed this as completed Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants