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

Unify sdkconfig configuration options #38

Merged
merged 9 commits into from
Dec 2, 2021
Merged

Unify sdkconfig configuration options #38

merged 9 commits into from
Dec 2, 2021

Conversation

N3xed
Copy link
Collaborator

@N3xed N3xed commented Nov 23, 2021

This mostly implements the plan outlined here.

Native and pio build now match with the sdkconfig configuration. Though I've implemented in addition to specifying the profile in the file name (like pio required before), that one can now also specify only the mcu or both. Also, the .<profile> requirement for pio has been lifted.

Selecting the sdkconfig now has the following precendence:

  1. <path>.<profile>.<chip>
  2. <path>.<chip>
  3. <path>.<profile>
  4. <path>

For the native build, the optimization configurations now don't conflict anymore. They are set using a generated sdkconfig.defaults file from the cargo opt-level and debug settings.

One quirk remains for the native build though:

When the path to the sdkconfig is specified, it is always updated to the configurations used in the latest build. So, if you leave some things unspecified in the sdkconfig it will be set after the build, which makes sdkconfig.defaults redundant after the first build. Because we copy the files in the pio build this does not happen there (i.e. the sdkconfig used for the build and generated after the build are separate).
This seems to be intended behavior for normal esp-idf C projects, but doesn't make sense for me. Please tell me what you think about this.

@ivmarkov
Copy link
Collaborator

One quirk remains for the native build though:

When the path to the sdkconfig is specified, it is always updated to the configurations used in the latest build. So, if you leave some things unspecified in the sdkconfig it will be set after the build, which makes sdkconfig.defaults redundant after the first build. Because we copy the files in the pio build this does not happen there (i.e. the sdkconfig used for the build and generated after the build are separate). This seems to be intended behavior for normal esp-idf C projects, but doesn't make sense for me. Please tell me what you think about this.

I don't like it either, but I also do not understand: how come the sdkconfig is touched/changed during the build? Do you mean that happens when I also provide an sdkconfig.defaults file?

@igrr
Copy link

igrr commented Nov 25, 2021

That's right, values from sdkconfig.defaults will be used if sdkconfig file doesn't exist yet, or if some value is not yet found in sdkconfig (docs).

sdkconfig file will be updated by the build process:

  • if it doesn't exist yet
  • if menuconfig is invoked and the user changes some configuration options
  • if IDF is updated and it adds or removes configuration options

There are two ways to treat sdkconfig file:

  • as part of the project, adding it to git. In this case you can use menuconfig to modify the settings in sdkconfig file.
  • as a build product, adding it to .gitignore. In this case, you can maintain sdkconfig.defaults file in the project directory. Menuconfig contains a feature to save the minimal sdkconfig file (with just the values which are different from the defaults), it can be used to create sdkconfig.defaults. Or you can create sdkconfig.defaults manually, if you know the option names you wish to modify. If sdkconfig is treated as a build product, you can also place it into the build directory by setting SDKCONFIG CMake variable to build/sdkconfig, either via command line (idf.py -DSDKCONFIG=build/sdkconfig ...) or in the project CMakeLists.txt file (set(SDKCONFIG ${CMAKE_BINARY_DIR}/sdkconfig)).

Depending on their preference, we see customers using either of these options.

Normally the `cmake` crate sets this variable depending on the current cargo profile,
but we don't want this as it interferes with esp-idf's own configuration.

The esp-idf is configured with kconfig and in that system there already exist options
for configuring optimization settings (using sdkconfig). Additionally, while compiling
the esp-idf, the bootloader is compiled too and setting `CMAKE_BUILD_PROFILE` would
also interfere with bootloader optimization options which are seperate from the application's.
Native build now more closely mirrors the pio build. The sdkconfig can now be specific
per build profile (debug/release) by adding the suffix `.<profile>`.

Additionally, one can also now specify the chip or both the chip and profile. Selecting
the sdkconfig has the following precendence:
1. `<path>.<profile>.<chip>`
2. `<path>.<chip>`
3. `<path>.<profile>`
4. `<path>`
Generates a sdkconfig.defaults file that contains the rust
optimization level approximately translated to the kconfig options.
This implementation has the same capabilities as the native now.
@N3xed
Copy link
Collaborator Author

N3xed commented Dec 1, 2021

sdkconfig file will be updated by the build process:

  • if it doesn't exist yet
  • if menuconfig is invoked and the user changes some configuration options
  • if IDF is updated and it adds or removes configuration options

As far as I've seen it, the sdkconfig will always be updated if not all options are specified in it. So for example, if you comment an option out, the file will be updated with the option set based on the defaults.

But this makes forwarding the compiler optimization options redundant after the first build, because then the sdkconfig is already fully specified and for example COMPILER_OPTIMIZATION_* will not be updated based on defaults anymore. The problem with this is, that we don't know if the user specified it or if it was changed by the build, that's why directly changing the sdkconfig file will not help.

I see three options on how to proceed here:

  1. In the native build, use the ESP_IDF_SDKCONFIG file as the last sdkconfig.defaults file. This is functionally equivalent to what the pio build does. The given sdkconfig file will not be changed by the build anymore. The generated (fully specified)
    sdkconfig will be in the OUT_DIR of esp-idf-sys (though we could introduce a further env variable so that the user can change
    this location). The last sdkconfig.defaults file overwrites any options of defaults files that come before, so this does not
    change the meaning of the sdkconfig.
    Also, menuconfig has to be taken into account, because we generally don't want fully specified sdkconfigs; so it should by default save the minimal sdkconfig (= sdkconfig.defaults).

  2. Only use ESP_IDF_SDKCONFIG_DEFAULTS for specifying the configurations. Change the pio build so that it has the same behavior as the native build: copy the generated sdkconfig from the out_dir to the file specified in ESP_IDF_SDKCONFIG.

  3. Scrap the distinction between sdkconfig.defaults and sdkconfig entirely and merge them. Menuconfig should also by default generate minimal sdkconfigs.

Option 1 matches the pio build, option 2 matches the behavior of an esp-idf C project, and option 3 goes a step further than the pio build, because there the sdkconfig is really no different than it being the last sdkconfig.defaults file.

We could sidestep the menuconfig issue, if we force the compiler optimization options to be dependant on the rust optimization options and so not separately configurable by the user. But we would need to do the same with possible future rust generated sdkconfig values.

I think option 1 makes the most sense for us.

@igrr
Copy link

igrr commented Dec 1, 2021

As far as I've seen it, the sdkconfig will always be updated if not all options are specified in it. So for example, if you comment an option out, the file will be updated with the option set based on the defaults.

You are right, I didn't consider the "sdkconfig was edited manually" situation. This will also cause sdkconfig to be updated.

Am I right that if we eliminate the issue of setting compiler optimization via sdkconfig (and replace it, for instance, with standard CMake -DCMAKE_BUILD_TYPE=Release type of setting) then we no longer need to handle sdkconfig in esp-idf-sys in any special way? Is there anything other than the build optimization level that we need to control sdkconfig for?

@N3xed
Copy link
Collaborator Author

N3xed commented Dec 1, 2021

Am I right that if we eliminate the issue of setting compiler optimization via sdkconfig (and replace it, for instance, with standard CMake -DCMAKE_BUILD_TYPE=Release type of setting) then we no longer need to handle sdkconfig in esp-idf-sys in any special way?

Yes. Maybe in the future there are other configuration options that we want to set the cargo way instead of sdkconfig, but I don't see any others now.

But we still have to choose either the way it is for the pio build or the native esp-idf way.

Prevents the sdkconfig from being updated by the build,
which maches the pio build behavior.
@N3xed
Copy link
Collaborator Author

N3xed commented Dec 1, 2021

Okay, I've implemented option 1 and with this, the sdkconfig configuration should now be the same between the two build strategies.

This PR should be ready for merge. Please tell me if you have any comments or suggestions.
@ivmarkov Do you want to review the code?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Dec 2, 2021

Okay, I've implemented option 1 and with this, the sdkconfig configuration should now be the same between the two build strategies.

This PR should be ready for merge. Please tell me if you have any comments or suggestions. @ivmarkov Do you want to review the code?

Go ahead, merge and publish! :)

Mention that `ESP_IDF_SDKCONFIG_DEFAULTS` and `ESP_IDF_SDKCONFIG`
is also supported by the pio build.
@N3xed N3xed merged commit 5b8dd42 into esp-rs:master Dec 2, 2021
@N3xed N3xed deleted the dev branch December 2, 2021 11:56
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

Successfully merging this pull request may close these issues.

None yet

3 participants