esp-config tui #3442

bjoernQ · 2025-04-30T06:52:27Z

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 developer guidelines

Extra:

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

Pull Request Details 📖

Description

This supercedes esp-rs/esp-generate#149

TL;DR this adds a TUI to edit config options

I guess in this case skip-changelog is fine

Testing

Testing:

generate a project via esp-generate, patch all esp-* dependencies, e.g. esp-config = { git = "https://github.com/esp-rs/esp-hal/" } etc.
install the binary: e.g. inside the esp-config directory cargo install --features=tui --path .
inside your generated project: esp-config

bugadani · 2025-04-30T07:33:16Z

For me this part is the weakest link in this approach:

install the binary: e.g. inside the esp-config directory cargo install --features=tui --path .

esp-config is now 3-in-1: a build script library, a set of library-facing macros, and a tui. esp-hal projects' Cargo.lock files will include all sorts of non-embedded nonsense and people will ask why.

bjoernQ · 2025-04-30T08:29:45Z

The three "things" even don't share much code

MabezDev · 2025-04-30T12:14:17Z

esp-config/src/bin/esp-config/main.rs

+}
+
+// This partially mirrors [esp_config::ConfigOption]
+// but not exactly - `&'static str` is not great for deserializing


Why not just make it String in esp-config?

It makes it more annoying to use as is - we could ofc create constructors to compensate for that - my thinking was just that I don't want to do those changes w/o at least discussing them before

MabezDev · 2025-04-30T12:14:30Z

esp-config/src/bin/esp-config/main.rs

+}
+
+// This partially mirrors [esp_config::Stability]
+// but not exactly.


also &'static str

MabezDev · 2025-04-30T12:15:18Z

esp-config/src/bin/esp-config/main.rs

+
+// This partially mirrors [esp_config::Validator]
+// but not exactly.
+// We especially can't and don't want to handle the `Custom` validator.


Why can't we? What am I missing here :D?

Box<dyn Fn(&Value) -> Result<(), Error>> isn't good for serialization/deserialization - and for having it clone,copy,eq etc.

(Fun fact: we don't use that validator at all currently IIRC - and while it's a wildcard for "everything" it is not nice to use it.
I'd personally like to see it go away and instead add another validator whenever we see a need for it)

Alternatively we could probably have a Custom validator using either a Rhai expression as we use it in other places or use the evalexpr crate which is a bit more lightweight

If it's not used, let's not solve the problem we don't have

bugadani · 2025-05-05T13:49:03Z

esp-config/src/bin/esp-config/tui.rs

+use ratatui::{prelude::*, style::palette::tailwind, widgets::*};
+use tui_textarea::{CursorMove, TextArea};
+
+const TODO_HEADER_BG: Color = tailwind::BLUE.c950;


Considering esp-rs/esp-generate#172 maybe we shouldn't hard-code colors and emojis here, either. Also, why is this called TODO_HEADER_BG, I couldn't figure it out even in esp-generate :)

oh yes - thanks for the reminder - I wanted to add a TODO there but finally forgot to add it 😄

TODO_HEADER_BG

That's a good question 🤔

bugadani · 2025-05-05T16:00:35Z

esp-config/src/generate/mod.rs

+    /// An integer config option
+    ///
+    /// Unstable and active by default.
+    pub fn integer(name: &str, description: &str, default_value: i128) -> Self {


I would like to see the ConfigOption changes in a separate PR. I'm also wondering if we could get rid of the different constructors and decide based on the type of the value instead (with Into<Value> for example)

Sure - can split it up if we agree it's a direction we want to go in general

I'm also wondering if we could get rid of the different constructors and decide based on the type of the value instead (with Into for example)

Sounds like a good idea

I think turning the big structs into builders is, well, not that valuable but I'm also not against it and you clearly think it's a good change so I'll not argue against it (except to say that I like named fields and the string constructor being 3 stirngs is slightly-but-not-really harder to understand). But this is a huge PR and that part is a bit of a logical unit, so if we can split it out to shrink this one, that is worth it to me.

bjoernQ · 2025-05-06T06:29:16Z

~~Back to draft for splitting out the config-struct changes~~

bugadani · 2025-05-08T11:47:31Z

esp-config/src/bin/esp-config/main.rs

+    let mut errors_to_show = None;
+
+    loop {
+        let repository = tui::Repository::new(configs.clone());


I don't understand why this is in a loop

the idea is that a user could change something which is checked in the build.rs (e.g. esp-wifi does it) and the build check below will fail - in that case we show the TUI again with the error message from the build - the user can fix the problem or revert all changes in that case

I think if any such issue could occur, is an issue with the options and their validations.

The users' projects can fail to compile for a multitude of reasons, for example the build could require a feature flag to be set. The TUI will not know how to build the project properly, so a check like this can turn the tool completely unuseable.

bugadani · 2025-05-08T11:48:52Z

esp-config/src/bin/esp-config/main.rs

+fn check_build_after_changes(path: &PathBuf) -> Option<String> {
+    println!("Check configuration...");
+
+    let status = std::process::Command::new("cargo")


We probably shouldn't build the project, or we should make it optional. It can be very slow, or have special needs.

making it optional sounds good - then we should also make the ensure_fresh_build optional - the reason we do it is to detect invalid settings which are only checked in build.rs (esp-config initially wasn't built with tooling in mind)

we could also allow the user to pass features needed for building (while a vanilla esp-generate generated project won't require that) - I guess with a lot of customizations sooner or later a user will get to the point that they need to manage the config manually - on the other hand I assume most users will be fine with what esp-generate can do for them

t.b.h.I wouldn't be sad to remove the "build-my-project" steps from the tool :)

Currently it builds before launching the TUI since it's the easiest way to guarantee the config-json is up to date .... the original plan was to check the "freshness" of the config-json and bail if needed instead - I guess that would be fine, too

For the build after applying the changes (and the cursed loop): users can modify the config in a way that makes the project fail to build (e.g.

esp-hal/esp-wifi/src/lib.rs

Lines 236 to 249 in 29060ae

// Validate the configuration at compile time

#[allow(clippy::assertions_on_constants)]

const _: () = {

// We explicitely use `core` assert here because this evaluation happens at

// compile time and won't bloat the binary

core::assert!(

CONFIG.rx_ba_win < CONFIG.dynamic_rx_buf_num,

"WiFi configuration check: rx_ba_win should not be larger than dynamic_rx_buf_num!"

);

core::assert!(

CONFIG.rx_ba_win < (CONFIG.static_rx_buf_num * 2),

"WiFi configuration check: rx_ba_win should not be larger than double of the static_rx_buf_num!"

);

};

)

Without the check users will need to manually fix/revert the config change because the UI won't launch (because the config-json is outdated - I guess in the example above this isn't much of a problem since build.rs will write out the config-json before we fail the build in esp-wifi's lib.rs ???).

Probably that is

not too common

we can expect someone who is able to write Rust code to get the project back to a buildable state

So, this might be acceptable.

We could also go the extra mile and make the" build-my-project" steps opt-out - most users will benefit from the additional convenience while those who really need to can avoid it

But this tool doesn't know how to build my project 🤔

But this tool doesn't know how to build my project

Not yours but the projects of most people just using a vanilla esp-generate generated one - so if you could pass --no-build (subject to bikeshed) you could use it?

bjoernQ · 2025-05-20T09:34:48Z

Back to draft, again - since we agreed we want an overhaul of esp-config to make tooling easier

bjoernQ added the skip-changelog No changelog modification needed label Apr 30, 2025

bjoernQ changed the title ~~Esp config tui~~ esp-config tui Apr 30, 2025

bjoernQ marked this pull request as ready for review April 30, 2025 07:33

MabezDev reviewed Apr 30, 2025

View reviewed changes

bjoernQ mentioned this pull request Apr 30, 2025

Have a esp-config binary esp-rs/esp-generate#149

Closed

bjoernQ force-pushed the esp-config-tui branch from 910e485 to a05a5d8 Compare May 5, 2025 13:40

bugadani reviewed May 5, 2025

View reviewed changes

bjoernQ force-pushed the esp-config-tui branch from a05a5d8 to 25f742f Compare May 5, 2025 15:53

bugadani reviewed May 5, 2025

View reviewed changes

bjoernQ marked this pull request as draft May 6, 2025 06:28

bjoernQ mentioned this pull request May 6, 2025

Make esp-config structs de-serialization friendly #3455

Merged

6 tasks

Add esp-config TUI

4e8ca9a

bjoernQ force-pushed the esp-config-tui branch from 25f742f to 4e8ca9a Compare May 8, 2025 09:29

bjoernQ marked this pull request as ready for review May 8, 2025 09:55

bugadani reviewed May 8, 2025

View reviewed changes

bjoernQ marked this pull request as draft May 20, 2025 09:33

	// Validate the configuration at compile time
	#[allow(clippy::assertions_on_constants)]
	const _: () = {
	// We explicitely use `core` assert here because this evaluation happens at
	// compile time and won't bloat the binary
	core::assert!(
	CONFIG.rx_ba_win < CONFIG.dynamic_rx_buf_num,
	"WiFi configuration check: rx_ba_win should not be larger than dynamic_rx_buf_num!"
	);
	core::assert!(
	CONFIG.rx_ba_win < (CONFIG.static_rx_buf_num * 2),
	"WiFi configuration check: rx_ba_win should not be larger than double of the static_rx_buf_num!"
	);
	};

esp-config tui #3442

Are you sure you want to change the base?

esp-config tui #3442

Conversation

bjoernQ commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

Uh oh!

bugadani commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjoernQ commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugadani May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugadani May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ commented May 20, 2025

Uh oh!

Uh oh!

bjoernQ commented Apr 30, 2025 •

edited

Loading

bugadani commented Apr 30, 2025 •

edited

Loading

bugadani May 5, 2025 •

edited

Loading

bjoernQ commented May 6, 2025 •

edited

Loading

bjoernQ May 8, 2025 •

edited

Loading

bugadani May 8, 2025 •

edited

Loading