-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
C++ Compatibility For Minimal Necessary Header Files #15353
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
C++ Compatibility For Minimal Necessary Header Files #15353
Conversation
Adds C++ compatibility to the minimal set of files needed for a C++ application to integrate MicroPython. Signed-off-by: Rafeeq Muhammad <rafeeqmuhammad@google.com>
Adds C++ compatibility to shared/runtime/pyexec.h. Signed-off-by: Rafeeq Muhammad <rafeeqmuhammad@google.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15353 +/- ##
=======================================
Coverage 98.42% 98.42%
=======================================
Files 161 161
Lines 21249 21249
=======================================
Hits 20915 20915
Misses 334 334 ☔ View full report in Codecov by Sentry. |
Code size report:
|
The idea is sound, but this implementation isn't ideal. For instance: imo external code shouldn't include py/stackctrl.h directly, but py/runtime.h instead. On the other hand here's the list of headers I've been using for embedding the windows and unix ports:
and some of these are lacking here. So I think either the situation should remain as-is, or we'd have to settle on which files form the toplevel api and then modify those. Or maybe even provide one common header which is C++ compatible, but that has the (slight) disadvantage that it's going to include more than needed for most users. |
I'm unclear how to give this PR a Zephyr tag. Also I'd like to mention that this PR doesn't attempt to implement C++ compatibility for all ports, just Zephyr. |
The changes here are not zephyr specific, they apply to everything (all ports), so they need to work with everything. In general this should be usable by anyone embedding MicroPython in a C++ project. |
I've been embedding C++ code into MicroPython builds since 0cf12dd, this being the core of our (probably not wise in retrospect) software strategy at Pimoroni. I've found that introducing C++ code into MicroPython also brings a lot of C++ baggage and footguns- while these may be manageable for a seasoned C++ developer, wider support could open the gates for more naive user (such as me!) to grapple with the problems that I did and still do. If C++ is "officially" endorsed by this change and works across ports, I think documentation should also be updated to reflect these pitfalls. Chiefly there are two issues that have plagued me:
Note: I also use a C++ module which redirects C++ malloc/free to MicroPython's GC_HEAP and allows me to track allocations. It's not ideal but it's a little more debuggable than MicroPython's "eh just throw C some space, it'll be fine" approach. You can see it in its full horror here: https://github.com/pimoroni/pimoroni-pico/blob/main/micropython/modules/cppmem/cppmem.cpp I guess embedding MicroPython into something else is rather an about-face of my memory management problems, but I figure my painfully earned two cents would be better said than unsaid here. |
Didn't know what that option was so found out it's for rp2 only, to be fair it says |
Summary
This Pull Request adds C++ functionality to the minimal set of header files needed to integrate MicroPython into a Zephyr project using C++. Without these changes all users with C++ applications must manually add C++ compatibility themselves.
Testing
Testing was done on an out-of-tree custom ARM board running Zephyr. With these changes, the code was built successfully and MicroPython ran correctly.
Trade-offs and Alternatives