-
Notifications
You must be signed in to change notification settings - Fork 13
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
Refactor xtask to bind and compile for every target. #15
Refactor xtask to bind and compile for every target. #15
Conversation
I think this should be ready for a review. I still don't know how to manage the different bindings per chip, if we use different headers. Or we generate everything in the same bindgen file, and we use cfg / features to specify which signature per chip of they differ. |
In esp-wifi we have bindings per chip and use a feature + cfg to select the right binding. Probably we can do the same here |
- Add support for esp32s2 - Move headers file back to esp-mbedtls-sys - Update bindgen - Update esp-hal, esp-wifi
Sorry, this PR got quite lengthy.
TODO
Every library compiles successfully, but on Xtensa targets, I'm getting errors when trying to run an example.
|
Maybe we need to pass https://gcc.gnu.org/onlinedocs/gcc/Xtensa-Options.html#index-mlongcalls to the compiler on Xtensa |
I've done a minimal testing on I don't know how rigorous we want to be on testing. Edit: I just realized how large the binaries generated are, compared to origin, I think this should be looked at. It's over twice the size. |
Using clang, I was able to reduce binary sizes a bit, but not to the same level as upstream: This PR @90568c3:
With CLANG:
Origin:
I can't seem to figure out what else to do to reduce the binary size. I think they should still be optimized and reduced. |
Besides the lib size looks quite good to me. There is a warning about
I have no idea what causes the increased size. I thought it's just he The final binaries are smaller now but that might be mainly because of optimizations in esp-wifi itself. There is not much differences in how Clang is called (comparing the previous shell file and the output of the CMake crate). (But there are some differences - maybe that is the clue) |
On my side, I would get a
I do think it's mainly because of the improvements in Since I've looked a bit at the binaries, I hope this gives us some clue:
nmOrigin:
This PR:
objdumpOrigin:
This PR:
|
I guess the Maybe it's worth to try changing the original compile script to pass the exact same arguments to CMake and see if it produces the same libraries. I guess it will. |
Using the original Bash build script ( I really feel it's something in the configuration and now cmake is called, but I don't know what. I've tried changing the @bugadani Maybe you can help with this. |
I can throw in random ideas but I'm very unlikely to be much help here. I can see that the latest rev uses gcc and its toolchain files, while the main branch uses clang's toolchain file so I assume it also uses clang, but that's about all the difference I see. Are the archives actually meaningfully different or does one compiler just throw in a bunch of random zero-sized stuff and different metadata? What you pasted here is essentially identical |
I've dumped the bytes of the generated Using the same cmake toolchain file origin:
this PR:
|
It feels like you're not passing some flags esp-idf passes, e.g. |
- Fixes an issue caused by `cmake-rs` auto-generating C flags which caused a larger binary size than expected. - Remove ESP32-S2 from justfile since it won't compile for async
76d7219
to
e9cec52
Compare
The issue ended up being caused by I haven't successfully got clang working to xtensa targets, hence whey they are still so large, but the last commit is still a considerable improvement. |
This fixes an issue where docs would be generated on a single line instead of being properly normalized and formatted
Sorry, this is a kind of a big PR. I'm not sure what's left to be done. I was hoping to get the compilation with clang to produce the same binaries as origin, which are the smallest, too, but I don't think it really affects that much the total binary size. Especially with I think eventually the compilation could be done in CI, when it detects a changes in the header files, so that users don't have to push their own compiled libs, to mitigate Supply chain attacks. |
@@ -1,5 +1,5 @@ | |||
[target.xtensa-esp32-none-elf] | |||
runner = "espflash flash --monitor" | |||
runner = "espflash flash --monitor --baud 921600" |
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.
Would be good to not have a hardcoded baud-rate here (and in the other places)
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.
Looks very good to me - Thanks for all the effort!
Maybe it would be worth to mention how to generate the bindings / compile the code in the README.md
Especially using +stable
to make it not complain about the missing --target
might not be obvious
I've refactored the xtask process to be able to compile mbedtls without needing a bash script, to try and improve user experience and compatibility. Defining a config per chip also allows a finer grain control over which peripherals we provide an alternative for.
Using different configs will generate different bindings, which might be an issue.
What's left to be done:- binding fails when the config file is namedmbedtls_config.h
saying it cannot findtime.h
.- Adding binding and compilation target for Xtensa chips.Usage
.cargo/config.toml
To generate bindings:
To compile libs: