-
Notifications
You must be signed in to change notification settings - Fork 19.4k
Add and use esp32_hwdef.py to allow creation of hwdef.h from hwdef.dat #30847
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
Could you try your hand at the esp32s3m5stampfly? That one I own and can flight test. |
Yes indeed. Bare in mind I was really only looking to move very minor things into here, so don't expect everything to move over! I'm hoping the builds are repeatable in esp32-land. I don't suppose you know if they are? That would make the process of moving everything into the hwdef.dat files much cleaner/faster... |
defb168
to
b51ef00
Compare
I've pushed a branch moves a single define over. Seems to work:
A bunch of stuff isn't working ( ... something changed, however:
... probably need to make sure that board was built with the s3 compiler. Hmmm. |
b51ef00
to
954b145
Compare
I am reasonably sure the whole Concretely I wouldn't suggest putting in the effort to factor it out. |
954b145
to
8a57ceb
Compare
I'd be very happy to rebase on top of a patch in master that removes it :-) ... in the meantime, it seriously irked me I was making a third copy of that method :-) |
@tpwrules any testing as yet? |
@@ -137,6 +137,8 @@ def add_hwdefs_from_hwdef_dir(self, hwdef_dir): | |||
board.toolchain = 'arm-linux-gnueabihf' | |||
elif "ChibiOS" in hwdef_dir: | |||
board.toolchain = 'arm-none-eabi' | |||
elif "ESP32" in hwdef_dir: | |||
board.toolchain = 'xtensa-esp32-elf' |
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.
What does this do? It is wrong for esp32s3
. But the build works for it anyway.
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.
So this is a default value for which toolchain to use for the things in that hwdef dir. The vast majority of our Linux boards use arm-linux-gnueabihf
, so you can see that default in there. However, several boards override with TOOLCHAIN
entry in their hwdefs - navigator64 is an example of that. Moving the toolchain requirement out of the waf
stuff and into the hwdefs is definitely something to look forward to - we'll probably want something close to the ChibiOS hwdef's MCU
line or something as an abstraction instead of specifying TOOLCHAIN in the hwdef... but we'll see.
This entry isn't used for compilation, only for working out which nm
to run, for example.
Thanks for the patience. This does indeed build and fly on the Stamp Fly. I just tried a clean tree, did not try to change options or break anything. I will come back and attack |
8a57ceb
to
1241a02
Compare
@tpwrules are you happy enough with this to approve it? It's a foot in the door, at least. |
just be aware that 9 months ago, as part of this mega-esp32- pr that never got merged I implemented a full version of this, mixed-in with the rest of the PR: #28514 . If I was doing it, I'd be pulling as many of the relevant bits from that PR as I could and/or perhaps just rebasing it and reducing the number of things it touches ... but i'm not, so this probably a step in the right direction for master. |
I'm just an interested watcher from this point, dont wait for me to respond or anything. |
I did look at pulling that stuff in. Unfortunately a lot has happened since then, notably factoring I can't commit to grabbing stuff in from the mega-PR - this is already some pretty epic yak-shaving as I'm trying to fix clang analysis warnings by removing ownptr, and the build system is making that problematic, this |
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.
Seems reasonable enough. I disagree with the toolchain selection for esp32s3
in one case but haven't dug in to see what it breaks if anything; maybe the nm
is compatible.
I have also not had a chance to look through the mega PR branch though I do agree it would be nice to take lessons from.
Tested on a VM with esp32buzz.dat. The single line moved has effect (I added
#error
s to the code to make sure).Modeled on similar patches made for the Linux HAL.