esp-config

[MabezDev]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

So this PoC ended up going much better than I'd originally anticipated. I implemented the PoC defined in the RFC quite quickly, before stumbling across rust-lang/rust#124941 which will be stable next release. Using this it was possible to completely replace toml_cfg in esp-wifi.

Usage

For HAL authors

Pass an array of tuples with the key, the default value and a description of the config option

generate_config(
    "esp_hal",
    &[(
        "place-spi-driver-in-ram",
        Value::Bool(false),
        "Places the SPI driver in RAM for better performance",
    )],
);

For end users

As part of the generation process, a md table of the config options can be included in the lib level docs:

From here, users are able to override the default values by setting the environment variable, this can be done in a number of ways but most common would be to add an entry to cargo's [env] section.

Caveats

const from_str_radix is not stable till 1.82

Which would be quite the MSRV bump. One option to avoid this would be to roll our own const int parser, something like

let mut bytes = s.as_bytes();
let mut val = 0;
while let [byte, rest @ ..] = bytes {
    assert!(b'0' <= *byte && *byte <= b'9', "invalid digit");
    val = val * 10 + (*byte - b'0') as usize;
    bytes = rest;
}

would work.

edit: the above work around has been implemented.

cargo env changes are not detected

This is really annoying and requires a clean build to detect new changes in the environment. There is hope however, rust-lang/cargo#14058 should fix this. Unfortunately the author of the PR has run out of steam, hopefully someone else picks it up, but it may be worth one of us taking a look if we go with this approach.

What's left?

I'm opening this now to get some feedback before spending too much time on it if it's not the right path, but IMO this works quite well. I'd like to add:

• Tests
• Resolve the TODOs

How to test

Playing around with it locally, and building and looking at the docs also.

[Dominaezzz]

Is it possible to set these from a library crate?

esp_hal -> my library (set an env for the application) -> an application using my library

[MabezDev]

Is it possible to set these from a library crate?

By these, you mean could a 3rd party library set esp-hal configs? Maybe, but also more than likely no. It would depend on the build order I think - I think we should discourage doing anything like this.

Did you have a use case in mind for doing so?

[Dominaezzz]

Is it possible to set these from a library crate?

By these, you mean could a 3rd party library set esp-hal configs? Maybe, but also more than likely no. It would depend on the build order I think - I think we should discourage doing anything like this.

Did you have a use case in mind for doing so?

I'm not sure which features are going to be moved over to environment variables so it's hard to give a concrete use case.

Take the psram feature for example, I'd like to be able to set this in a library in case I want to statically allocate something in the library.

If I'm calling the spi driver from an interrupt in my library (because I have a nice queuing implementation), I'd like to be able to enforce that the driver methods live in ram, so I know the handler is IRAM-safe.

[liebman]

If I'm calling the spi driver from an interrupt in my library (because I have a nice queuing implementation), I'd like to be able to enforce that the driver methods live in ram, so I know the handler is IRAM-safe.

What if two libs have competing asks of the config, which would win? It may be better just to document what's needed....

[bugadani]

Generally, the dependency is built first, so I don't think you can define env vars from the dependent crate and hope that a prior step will pick it up.

[Dominaezzz]

If I'm calling the spi driver from an interrupt in my library (because I have a nice queuing implementation), I'd like to be able to enforce that the driver methods live in ram, so I know the handler is IRAM-safe.

What if two libs have competing asks of the config, which would win? It may be better just to document what's needed....

Yeah that seems like the sane option

[bugadani]

I guess we'll have to bump MSRV to 1.79 because of inline const but I think it's well worth it.

[bjoernQ]

Generally, I like the simplicity of this approach! Initially I was obsessed by the idea of having a menuconfig like TUI and all rules encoded in a machine-readable format but - t.b.h. I don't think we need that

[MabezDev]

Generally, I like the simplicity of this approach! Initially I was obsessed by the idea of having a menuconfig like TUI and all rules encoded in a machine-readable format but - t.b.h. I don't think we need that

I still like and value the work you did with rconfig, a TUI is still a good idea, but not required for the MVP at the very least. In the future we could make things more user-friendly by offering a tui that sets the value in .cargo/config.toml directly.

This approach still doesn't yet address validation either, but I think we could extend what we have here to put some limits on values etc.

[Dominaezzz]

Any guidance on when to use a feature vs when to use a config value? Particularly for the feature-like/boolean things. (Numbers and enums are a no-brainer, use config)

[bugadani]

In my head this boils down to dependencies. If something needs to enable a dependency, use a cargo feature. Otherwise, prefer a config variable.

[MabezDev]

Yes exactly that, use cargo for what it's good at: dep management. Use the config for anything else. Most crates blur the lines because it's easy to add a quick feature and get some make-shift conditional compilation going which works well enough for most use cases.

[bugadani]

@Dirbaio do you think a configuration solution like this could be useful for embassy, instead of the infinite amount of embassy-time-driver features, for example?

[Dominaezzz]

fwiw, cargo features were also intended for conditional compilation, and have better support for auto-complete and documentation in IDEs.

With regards to dependencies, after #2070 landed, dependencies will now be hard included, does this mean there's no longer a use case for features in this repo?

[bugadani]

With regards to dependencies, after #2070 landed, dependencies will now be hard included

There are still exclusive choices, mainly defmt vs log, where we need to keep the cargo feature. And don't let me forget the actual MCU selection, either.

[MabezDev]

I added a version of the workaround, it seems to help quite a bit. I don't think it will help in workspaces, and obviously setting env via any other method still won't work, but I think this is the common path.

[bugadani]

What's this entry? :D