-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
fix(esp_ulp): Add support for multiple ULP program embedding without name collision (IDFGH-14152) #14954
base: master
Are you sure you want to change the base?
Conversation
👋 Hello X-Ryl669, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
a0b2fc8
to
5aba452
Compare
components/ulp/esp32ulp_mapgen.py
Outdated
|
||
args = parser.parse_args() | ||
|
||
with open(args.outputfile + '.h', 'w') as f_h, open(args.outputfile + '.ld', 'w') as f_ld: | ||
gen_ld_h_from_sym(args.symfile, f_ld, f_h, int(args.base_addr, 0)) | ||
with open(args.outputfile + '.h', 'w') as f_h: |
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.
Should the file name be .hpp
for C++ case, making it clear that the file shall not be usable from C code?
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.
Can be. It probably make more sense, I'll change that.
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.
Tried to do that but it's a PITA since changing the extension here means also making the CMake code conditional to the prefix type (make all dependency tracking & artifacts as variable, ...). I don't think it matters anyway, the only way to get a C++ header is to specify a prefix with a namespace declaration, and in that case, if the .h
header is included in a C source file, the compiler will just choke on the "namespace" part so it's pretty obvious.
@X-Ryl669 Thank you for the PR. We can definitely accept the changes related to customization the prefix ( |
I've reworked the PR to fit the review. I've reverted the linker script removal and instead changed its format so if the declared symbols already exists, it errors out with a meaningful error. This should prevent silent building and runtime crash on colliding name for multiple ULP programs. I've also removed the |
Description
Currently, the ULP's auto variable name embedding script has multiple issues:
This PR fixes it:
nm
cross compilation tool toreadelf
tool for extracting symbols. This allows keeping the symbol's scope (global or local), the symbol type (object or function or ...) and the symbol size (for arrays).ulp_lp_core_lp_ana_peri_intr_handler
ulp_
prefix, but now, you can specify it)ULPmain::
and it'll mangle the name and work withoutextern "C"
): perfect to move the shorter variables in their own namespace and avoid collisions without dumb long namesuint32_t myVariable[32]
is correctly exported so can be used like a native variable in HP core, no more hacky((uint32_t*)myVariable)[0]
like specified in the documentation.From a user perspective, nothing changes (the useless symbol will be removed, but they couldn't be used anyway previously).
However, she can now replace the line:
to this:
And build collision-free and working multiple ULP binary.
Related
Fix #14945 and other issues too related to misusage of ULP symbols in HP core.
Testing
I've tested on my system with ESP32C6. I haven't tested with other CPU types.
However, the tools used in this PR are common cross compilation tools, it shouldn't depend on the CPU architecture.
Checklist
Before submitting a Pull Request, please ensure the following: