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] config options for specific chips #3020

Open
MabezDev opened this issue Jan 23, 2025 · 7 comments
Open

[RFC] config options for specific chips #3020

MabezDev opened this issue Jan 23, 2025 · 7 comments
Labels
1.0-blocker package:esp-config Issues related to the esp-config package RFC Request for comment

Comments

@MabezDev
Copy link
Member

Following on from the discussion here: #3001 (comment)

We might want a better solution than just documenting which cfg's are supported on each chip (the current solution).

We one quite large constraint when building this:

  • we can't break builds when a config option is passed by the user, but the current target doesn't support that option. Doing so would break multi chip project setups.

With this constraint in mind, we still need to signal that the option isn't supported.

One idea I had:

(ab)use cargo cfg linting

Where a cfg isn't supported, we could choose not to emit this line:

println!("cargo:rustc-check-cfg=cfg({cfg_name})");
.

This would flag a warning in user code, that the cfg they're trying to use has no effect. Unfortunately I don't think we can control the error message here, so the message is quite vague - but maybe something we could document in an FAQ.

I would appreciate some more input/ideas here :).

I'm adding the 1.0 blocker label, though we might be able to add this later if we go with the above solution (iirc, introducing warnings is not a breaking change).

@MabezDev MabezDev added 1.0-blocker package:esp-config Issues related to the esp-config package RFC Request for comment labels Jan 23, 2025
@bugadani
Copy link
Contributor

Cargo doesn't print warnings that originate in an external crate, so I'm not sure if such a warning would be visible. And besides, I'd prefer not seeing warnings if my project happens to support two MCUs and I'm trying to set specific configurations for them. Even though a workaround exists, it's not entirely ideal that one is needed.

@MabezDev
Copy link
Member Author

I always forget that warnings from 3rd party crates aren't forwarded to the user.

My instinct currently is to just document the chip the config option supports, but given that it's the current behavior, I'll keep this open a bit and see if we can think of something better.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 23, 2025

My initial idea to support this was a bit different

e.g.

[env]
ESP_HAL_CONFIG_PLACE_ANON_IN_RAM = "true"

# don't do it on C2
ESP32C2_ESP_HAL_CONFIG_PLACE_ANON_IN_RAM = "false"

ESP32C6_ESP_HAL_CONFIG_FLIP_LINK = "true"
ESP32H2_ESP_HAL_CONFIG_FLIP_LINK = "true"

i.e. optionally prefix a key with a chip makes it override the default or the non-specific setting

Downsides

  • makes esp-config esp-specific (probably not a real problem)
  • what about using esp-config in a crate which is agnostic to the target (we don't have s.th. like that I guess - and it probably won't hurt)
  • can't apply an override to multiple chips (see the FLIP-LINK example)
  • even if a config is chip specific we should emit cargo:rustc-check-cfg=cfg({cfg_name}) (is this a problem?)
  • could we run into a similar situation which made us introduce the _CONFIG_ separator?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 29, 2025

Another thing we could but probably don't want to do is reading our own file (e.g. esp-config.toml) and populating the env-variables.

Could look something like this

[esp-hal]
place_anon_in_ram = true

[esp32s3.esp-hal]
psram-mode = "octal"

[esp-wifi]
mtu = 1524

Users could still override it via [env] and by setting env-variables

@bugadani
Copy link
Contributor

and populating the env-variables

What would be the mechanism to do this? I'm not sure if a crate's build script can see the whole project's config file, and we can't really set env variables for other crates (i.e. from the project's build.rs, we can't affect esp-hal - #2156 (comment)).

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 29, 2025

and populating the env-variables

What would be the mechanism to do this? I'm not sure if a crate's build script can see the whole project's config file, and we can't really set env variables for other crates (i.e. from the project's build.rs, we can't affect esp-hal - #2156 (comment)).

sure, we can't set env-vars for other crates but we can for the "to be compiled" crate which is enough - i.e. a crate would only pick it's own config keys

crate's build script can see the whole project's config file

Would be the same mechanism used by toml-cfg

@bugadani
Copy link
Contributor

I guess it could work. I wouldn't see any reason (except that we now build on env vars in the HIL tests) to keep using env vars (at least as a user), but I guess that's not necessarily a big negative, just a bit of complexity we'd keep around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0-blocker package:esp-config Issues related to the esp-config package RFC Request for comment
Projects
Status: Todo
Development

No branches or pull requests

3 participants