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

Remove flip-link feature, replace by flip-link option #3001

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Jan 20, 2025

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:

Pull Request Details 📖

Description

  • remove flip-link feature, replaced by ESP_HAL_CONFIG_FLIP_LINK config
  • remove quad-psram and octal-psram features - replaced by psram feature plus ESP_HAL_CONFIG_PSRAM_MODE (quad/octal)
  • introduce a Validator::Enumeration validator in esp-config (we could use it in esp-hal-embassy, too - but probably in another PR)

skip-changelog because of non-user-facing changes in some packages

closes #2997

Testing

CI, manual testing

@bjoernQ bjoernQ changed the title Flip link option Remove flip-link feature, replace by flip-link option Jan 21, 2025
@bjoernQ bjoernQ added the skip-changelog No changelog modification needed label Jan 21, 2025
@bjoernQ bjoernQ marked this pull request as ready for review January 21, 2025 12:35
esp-hal/CHANGELOG.md Outdated Show resolved Hide resolved
Value::Bool(false),
None
),
#[cfg(any(feature = "esp32c6", feature = "esp32h2"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, I didn't think about doing this! Could you also add a cfg for spi-address-workaround as it's esp32 only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will make projects that target multiple chips very hard to write. If my project targets an S3 and a C6, I won't be able to configure flip-link for the C6, because the S3 build will fail with an unknown config error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True so far but we really should have a way to have chip specific configs anyway. e.g. for esp-wifi the ideal (in terms of throughput) settings are quite different for different chips.)

A lame workaround would be to have scripts which set the correct env-vars and invoke the build (at least I remember we said something like that when we introduced esp-config and realized we don't support chip specific settings - but maybe it's just in my head and no one ever said that)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for my case I may be able to set env variables in my xtask, so maybe if someone has enough experience to build a project, they may be able to work around this. It's still a bit unfortunate, maybe we could do something with the config format instead, e.g. parse the value as json and pick out device specifics (ESP_XY = "{esp32: 0, esp32c2: 1}"), or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that or I was even thinking of s.th. easier like ESP32C6_OVERRIDE_ESP_HAL_CONFIG_FLIP_LINK="true" ... (not really thought that through t.b.h. - stopped thinking about it when I realized I should not include it in this PR)

I guess it's something we should have a solution for sooner or later. Given enabling "flip-link" today for ESP32-S3 already results in a failed build and given that esp-generate doesn't support multi-target projects, yet - I guess this is fine for now (but I agree we can do better)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if rust-lang/cargo#10273 was supported this would have been a really nice solution.

We can certainly add a chip field to esp-config (and I think even in a future compatible way), where all configs supported by all chips by default, unless the config specifies otherwise.

In the interesting question is what do we do if we encounter a env variable which isn't supported by the current build, i.e the scenario @bugadani presented? We can't error out, as that puts us in the same situation here 🤔.

esp-hal/ld/sections/rwdata.x Show resolved Hide resolved
esp-riscv-rt/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flip-link should be a config setting, not a feature
4 participants