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

RFC: We need some sort of configuration system #1111

Closed
jessebraham opened this issue Jan 24, 2024 · 37 comments
Closed

RFC: We need some sort of configuration system #1111

jessebraham opened this issue Jan 24, 2024 · 37 comments
Assignees
Labels
RFC Request for comment
Milestone

Comments

@jessebraham
Copy link
Member

As this project continues to grow, we increasingly are feeling a need for user-configurable features. IMO we have reached a point where we have outgrown Cargo features.

I believe that we need some sort of configuration file and build system in order to continue adding hardware support and functionality. I believe that Cargo features should largely be reserved for functionality which requires additional dependencies, and that we should move towards configuring hardware via a configuration file.

I think conceptually this task is not overly difficult; I could cobble something together in a day or two I'm sure. However, I think we need to put some serious thought into what the configuration format will look like, which things should be configurable, and how users will interact with this configuration (ie. do we want to build a tool similar to idf.py menuconfig?).

I would like to open a discussion regarding potential paths forward on this work.

@jessebraham jessebraham added the RFC Request for comment label Jan 24, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 25, 2024

This is related to #42 I guess

I still think toml-cfg could be a nice option. We are also using it in esp-wifi already. Downside might be that it (at least I think so) only supports "flat" configs but I don't think we really need nested config structures (and maybe it's supported - just never tried). After unifying the HAL it would be even easier to use. Additionally, it was created by a well-known and trusted person.

It would be a nice fit to replace things like psram-2m, psram-4m ..., 'xtal-Xmhz` etc. features which really don't need to be Cargo-features.

As you already said: there are things which need and should be kept as features but there are a lot of other things which don't need to be features.

I'm not entirely sure what "build system" means since it could mean a few different things. One thing I could think of: Have a crate which provides functionality we want in the build.rs of binary crates and the build.rs of the binary crates will just look like this

fn main() {
    awesome_esp_hal_build_support_crate::main();
}

Not sure if we need or want something like that. Maybe it's not worth it at all

I would say at least in the beginning we won't really need something like menuconfig/guiconfig. We won't have much options in the beginning - editing a config file would probably be good enough. Later we might want to have something like that

@ProfFan
Copy link
Contributor

ProfFan commented Jan 29, 2024

I 2nd toml-cfg but it seems that it hasn't been updated for 2 years.

@jessebraham
Copy link
Member Author

I'm not sure I understand why we need toml-cfg at all? What does it do that we can't do with serde/toml already? I'd rather use maintained libraries.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 30, 2024

What we need is a way for the end user to configure things in our library crate. I don't see how serde/toml alone can do that.
The only way I can think of would be to require a build.rs in the user's binary crate which does "something somehow" - t.b.h. I don't really like that but we could provide the functionality via a dev-dependency crate which would make it a bit less awful.

But maybe I just don't see the obvious way - a POC might shed some light on this.

If we are concerned, we could just ask James Munns about the support status of toml-cfg - I assume there wasn't any update because it just works but maybe it's more or less abandoned - in that case we shouldn't use it and replace it in esp-wifi

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 31, 2024

Regardless of how we configure things in future .... here are the current features of esp-hal-common and an initial guess what will be a config option - certainly needs some discussion

Current Feature Feature/Config Comment
esp32 feature
esp32c2 feature
esp32c3 feature
esp32c6 feature
esp32h2 feature
esp32p4 feature
esp32s2 feature
esp32s3 feature
rt-riscv internal feature
rt-xtensa internal feature
flip-link feature
psram-2m config maybe we still need a general PSRAM feature?
psram-4m config
psram-8m config
opsram-2m config
opsram-4m config
opsram-8m config
opsram-16m config
psram-80mhz config
usb-otg needed at all?
direct-vectoring feature
interrupt-preemption feature
vectored feature
log feature
eh1 feature probably should get removed - eh1 should be default?
embedded-io feature probably should get removed?
ufmt feature
async feature probably should get removed and always enabled once we figured out the runtime interrupt binding
embassy feature
embassy-executor-interrupt feature
embassy-executor-thread feature
embassy-integrated-timers feature
embassy-time-systick feature
embassy-time-timg0 feature
riscv internal feature
xtensa internal feature
rv-init-data internal feature
rv-zero-rtc-bss internal feature
rv-init-rtc-data internal feature
debug feature
defmt feature

Edit by Scott: Removed the xtal features

@lu-zero
Copy link

lu-zero commented Feb 11, 2024

I 2nd toml-cfg but it seems that it hasn't been updated for 2 years.

I had a look and tried to clean it up, I second that it is very nice for the purpose.

@ProfFan
Copy link
Contributor

ProfFan commented Feb 16, 2024

So are we zeroing on toml-cfg? 😆 I really want to configure IRAM placements, it's such a pain currently and hurts performance 10x due to cache misses

@MabezDev
Copy link
Member

I think that toml-cfg solves part of the problem, one thing I'm missing is how to turn config options into cfg flags. For instance, in the case of placing things in ram we will need to do something like #[cfg_attr(somecfg, ram)] to tell the linker to put those sections in RAM. How do we get from toml_cfg to that?

@MabezDev
Copy link
Member

Another thing is how do we validate the config? For instance, if someone puts their PSRAM speed value to 400mhz, which isn't supported, do we just let it die at runtime? I'd personally like to see some bounds or value sets that we can set on some fields of the configuration struct.

@MabezDev
Copy link
Member

My final concern, in regards this time to specifically cfg-toml, is maintainability concerns. What happens if/when cfg toml starts limiting us and we need to adapt/modify it? It's currently not actively maintained, now maybe we can help out here and become a maintainer of it, or perhaps its feature complete and the author doesn't expect to add more changes.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 19, 2024

We can definitely go with our own solution - we probably just need to "borrow" this from cfg-toml: https://github.com/jamesmunns/toml-cfg/blob/71f45258c4f8c9a23e5ac00c5220dd8b2f0f780a/src/lib.rs#L208-L240

And then we should be able to locate the config in our esp-hal build.rs. Then we can generate Rust code and/or cfg flags.

The only thing I don't have a good idea for is validation. We could use a schema file but they are not fun to maintain

@MabezDev
Copy link
Member

We can definitely go with our own solution - we probably just need to "borrow" this from cfg-toml: https://github.com/jamesmunns/toml-cfg/blob/71f45258c4f8c9a23e5ac00c5220dd8b2f0f780a/src/lib.rs#L208-L240

And then we should be able to locate the config in our esp-hal build.rs. Then we can generate Rust code and/or cfg flags.

The only thing I don't have a good idea for is validation. We could use a schema file but they are not fun to maintain

I think there are some crates for validation we can take inspiration from. This is sorta what I have in my head:

#[config]
pub struct Config {
    #[cfg] // cfg converts bools to cfg flags, probably needs another name
    place_spi_in_ram: bool,
    #[values([40, 80])] // could also be a range?
    psram_speed_mhz: usize
}

This is just a rough design, but I think this would cover our use cases? Is there any other kind of configuration that this wouldn't work for?

Maybe a schema file would be better than "in-code" validation, but you're right writing schema files are no fun. Maybe there is a format/crate that exists already that would make this less tiresome though, which might make this path viable.

@lu-zero
Copy link

lu-zero commented Feb 19, 2024

Probably would be good to provide feedback on this pre-rfc.

toml-cfg as-is has some shortcomings since it breaks once --target-dir is declared.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 19, 2024

Maybe a schema file would be better than "in-code" validation, but you're right writing schema files are no fun. Maybe there is a format/crate that exists already that would make this less tiresome though, which might make this path viable.

Another option would be to take inspiration from Kconfig ( https://www.kernel.org/doc/html/next/kbuild/kconfig-language.html ) and just do the same in TOML (somehow). Since it's around for some time I guess they have everything we could ever need - even when we someday come up with a GUI

Or we just use their format. e.g. there is a parser https://crates.io/crates/nom-kconfig

@lu-zero
Copy link

lu-zero commented Feb 19, 2024

How complex would it be needed?

Conceptually it would be similar figment but at compile time, so the proc_macro might reuse the serde notation. Ideally the same syn code could be used to make a configure-like that parses the crates to get the information like cbindgen does.

But would be quite big and potentially overkill if something simpler might work.

@MabezDev
Copy link
Member

Another consideration would be whether it is possible to do some form of linker configuration. I.e #703? I would place this on the stretch goal side of things, but worth thinking about.

@jamesmunns
Copy link

Just copying in from chat:

  • I didn't realize y'all were actually using toml-cfg, neat!
  • I'll merge @lu-zero's PR (Update dependencies jamesmunns/toml-cfg#6) now and cut a release
  • If any of y'all would like to be a maintainer (and @MabezDev vouches for you), let me know and I'll give you permissions to GH and/or crates-io.

Glad to see it's been (somewhat) working for you!

@jessebraham
Copy link
Member Author

This conversation seems to have diverged heavily from what I originally envisioned, so I guess somebody else can take over this.

@MabezDev
Copy link
Member

We haven't settled on anything yet, we're still gathering requirements and poking around to see what's already out there. This discussion would benefit greatly from your vision and input, so please stick around!

@ProfFan
Copy link
Contributor

ProfFan commented Jun 17, 2024

Bumping this old thread: Could we just use cfg(foo) and let the user define those in build.rs?

@MabezDev
Copy link
Member

Bumping this old thread: Could we just use cfg(foo) and let the user define those in build.rs?

Whilst I like the simplicity, it doesn't do any validation; it also doesn't do any name collision detection which could make things very annoying to debug.

@Dominaezzz
Copy link
Collaborator

Could this be solved with a procedural macro (An enhanced version of #[entry])?
The idea here would basically be to generate a custom Esp32Reset or post_init in the user's application.

#[entry(watchdog, psram(80))]
fn main() {
    // stuff
}

I think all the settings described above can be configured in the macro. Stuff like place_spi_in_ram can remain a feature I think.

One of the big reasons I came to no_std was to run away from idf.py and deal with plain Cargo😅. I'd be strongly in favor of a Rust/Cargo native solution

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 16, 2024

There will definitely never ever be a esp_hal.py or something 😄 you will always be able to just do cargo run

What I currently have in mind is a plain config file (TOML) side-by-side to the manifest file. If there is no config, defaults will apply.
Similar to what we already have for esp-wifi.

While place_spi_in_ram is not a problem on its own, having such a feature for (almost) every driver is probably not too nice. (And I can think of a lot more optimizations and tweaks which should really be opt-in)

Additionally, there might be a TUI/GUI (which isn't mandatory to use) - something like this:

image

@Dominaezzz
Copy link
Collaborator

Oh okay that's great to hear.

I'm guessing there will be some way to merge these toml files across crates in the same way cargo additively merges features?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 16, 2024

In my current POC there is really only one config file in the end looking something like this

[esp-hal]
heap.size=37283
psram.enable=true
psram.size="4"
psram.type.type="octal"
[esp-wifi]
options.ble=true
options.buffer="2"

(Those things are totally made up for an example - not actually tested yet with the real crates)

We will still have things that remain features - the config is more meant for things where a feature is "overkill" (but the distinction is a bit blurry of cause)

@bugadani
Copy link
Contributor

Is this really a problem that we need to solve using some random external tool/exotic build system? What problem do we have where Rust's type system isn't good enough by itself?

@MabezDev MabezDev changed the title RFC: We need some sort of configuration/build system RFC: We need some sort of configuration system Aug 21, 2024
@MabezDev
Copy link
Member

Is this really a problem that we need to solve using some random external tool/exotic build system? What problem do we have where Rust's type system isn't good enough by itself?

I'm open to native solutions, but the only solution I'm aware of for something like setting heap size is using env variables which are not very ergonomic, and also doesn't offer any way to validate the options/value combinations (we probably could via some build scripts, but these are also not that pleasant).

@MabezDev
Copy link
Member

It looks like there is finally a fix for the [env] section not being updated by cargo: rust-lang/cargo#14058. It might be worth trying a PoC configuration using just cargo and env, and see what limitations we run into. If it works we can expand on it, if it's tricky to use or inflexible, we can continue with a more custom approach.

@MabezDev MabezDev removed the status:needs-attention This should be prioritized label Aug 29, 2024
@lu-zero
Copy link

lu-zero commented Aug 31, 2024

to recap, the idea is to have an xtask or such that updates a .cargo/config.toml [env] section?

Then the entries can follow usual pattern for env overrides as seen in system-deps.

@bugadani
Copy link
Contributor

This is by no means guaranteed, we haven't started designing anything around these parts yet. However, together with some of our additional plans, I think the most straightforward option is a project management tool that you can install from cargo, that you can then use to generate your project (clean or from a template) and manage its configuration. I think we can provide a menuconfig-like terminal app to manage the configuration that provides help on the options. The actual configuration format is undecided, there are subtleties around non-string configuration values.

@bugadani
Copy link
Contributor

bugadani commented Sep 2, 2024

Just a bad idea: it's possible to make #[ram] conditional over a constant, e.g. written like #[ram(CONSTANT)]. The implementation would need to duplicate the function as fn_name_flash and fn_name_ram, and the macro should generate a wrapper fn like this:

#[inline(always)] // (or maybe even place it into RAM?)
fn fn_name(...) {
    if CONST {
        fn_name_ram(...)
    } else {
        fn_name_flash(...)
    }
}

It's possibly a bad idea, but it's technically possible, so if we were to choose a system that uses generates constants instead of env vars, this wouldn't be a blocker.

@lu-zero
Copy link

lu-zero commented Sep 2, 2024

env vars can become cfg entries through build.rs

@MabezDev
Copy link
Member

Since opening this discussion esp-hal has been through several changes, so I took some time to reassess what we would actually need from a configuration system.

Compile time values

Less so for esp-hal, but definitely esp-wifi we have many tunable values that need to be known at compile time. Imo @bugadani's work on reworking the way the HAL is initialized can cover 99% of those cases, even something like heap initialization could be completed by taking a &'static mut [u8] in the global config.

Conditional compilation

Many of the original use cases for conditional compilation can be removed with a runtime config. We already did this for the xtal-* features, and I think we can do the same for the PSRAM features. There is one area where conditional compilation is required and that's placing drivers in RAM.

Conclusion

I think at this point, a full-blown configuration system with external tooling is not required. Not to say it never will be if our requirements change in the future, but for right now I believe we can achieve what we need to achieve with the tools cargo/rust give us natively through:

  • features for native conditional compilation where appropriate (we don't want to go back to thousands of features)
  • cargo [env] section for some forms of conditional compilation (RAM placement) and some compile time value acquisition where runtime configuration isn't possible

I'll be working on a small PoC to replace the place-spi-driver-in-ram feature of esp-hal, and some tunables in esp-wifi to see how this works out.

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 11, 2024

Sounds great - for mostly just booleans asking to place things in RAM this should definitely be good enough.

Only usability issue that comes to my mind would be when users have typos in env-var names (but if we have a common prefix, we can also check in build.rs that there is nothing we don't know)

When we go with our own solution for an esp-template replacement we can even offer most of these options there (so users at least can initially benefit from a TUI to configure this)

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 11, 2024

And for esp-wifi the current solution would probably also be good enough for now

@MabezDev
Copy link
Member

Here is the slightly more than PoC I came up with: #2156, which also resolves the esp-wifi configuration. I'm pretty happy with it, but feedback is welcome!

@MabezDev
Copy link
Member

Closed via #2156

@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment
Projects
Archived in project
Development

No branches or pull requests

10 participants