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

Create an xtask subcommand to generate eFuse fields #1258

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Mar 8, 2024

Rather than keeping the (now outdated) CSV files in this repository, we can instead just generate them periodically using the latest versions from ESP-IDF. There's no need to parse these files in the build script during every build.

Since our CSV files were out of date, I ended up pretty much completely rewriting the parser. I think it's easier to understand and is a bit more flexible now, so probably a win anyway.

There is still room for improvements here, we're still skipping a few fields, but I'm not sure how relevant they really are in practice.

Did some small refactoring to accommodate this, nothing noteworthy though.

@jessebraham jessebraham added the skip-changelog No changelog modification needed label Mar 8, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 11, 2024

I think it's a good thing to not regenerate the efuse fields during the build - they are not expected to change.

The idea is to use the CSV from esp-idf directly when generating, right? I agree there is little use in copying them

I remember the old parser had some problems with the MAC address for P4 which needed some modification of the original CSV but seems your changes fixed it as far as I can see - generally it's better to have a parser which doesn't need random modifications to esp-idf's CSVs to work 😄

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for doing this!

@jessebraham
Copy link
Member Author

I remember the old parser had some problems with the MAC address for P4 which needed some modification of the original CSV but seems your changes fixed it as far as I can see - generally it's better to have a parser which doesn't need random modifications to esp-idf's CSVs to work 😄

Yes this was also a problem for the ESP32 when trying to generate from the latest eFuse tables, which is what ultimately led me to rewriting the parser. This is handled by a second processing iteration over the fields once they've been parsed.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jessebraham jessebraham added this pull request to the merge queue Mar 11, 2024
Merged via the queue into esp-rs:main with commit 513a063 Mar 11, 2024
18 checks passed
@jessebraham jessebraham deleted the feature/efuse-fields branch March 11, 2024 14:45
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.

4 participants