-
Notifications
You must be signed in to change notification settings - Fork 136
Implement new API for reading eFuse values #847
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
Conversation
@SergioGasquez pointed out that there may be a few more opportunities to use the field constants rather than EDIT: This is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, way more grok-able than the random bit shifts :D.
I have a couple of initial comments to address. I'm also aware that we also have some efuse field generation in esp-hal's xtask, is the plan to retire that and keep field generation here and periodically copy the (presumably the .rs) files over to esp-hal?
I don't know what the plan is, meant to bring this up in the original comment. Probably it's not a bad idea to even have a separate Open to suggestions here, was just going with the path of least resistance to get something working. |
Probably I should update the CI workflow to ensure the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for working on this, it's definitely way better now!
Regarding this, probably our options are:
Probably 1 or 2 are the preferable choices, don't really care much which we go with. I also think this can be decided on at a later date and does not need to block this PR. I think I have completed everything I planned to here for the time being, so unless there are additional review comments to address then it should be okay to merge whenever. |
I would also lean towards 1 or 2, maybe I like having a standalone package a bit more, it shouldn't be too difficult to maintain and release. |
1 or 2 seems best here, probably 1 initially and we can push it into it's own crate after. |
This turned out to be pretty large, so apologies for that. Plenty of the code can be disregarded in review though (i.e. the generated eFuse constants).
This PR introduces a new
xtask
package to the workspace. While I've been resisting adding this for some time, it's now become a bit of a necessity. This is used to generate the eFuse field definitions from the YAML files defined in theesptool
repository.Additionally, all code relating to reading of eFuse values has been completely rewritten. There are a few hacky bits here and there, mostly relating to the ESP32, but overall I think things are in much better shape. Perhaps we can iterate on this further and improve those areas, however for the time being I'm not too worried about it.
I have tested on all supported devices to confirm that things are working as expected; as far as I can tell, we're in good shape. There is one caveat, on the ESP32-S2 for whatever reason we are not reading the correct BLOCK2 version number; this does not really make sense to me, and honestly I'm getting kind of tired of thinking about eFuses at this point, so happy to just open an issue for this and worry about it later.
Closes #827