-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
RFC #2: Mechanism for allowing dynamic native modules to call port-specific functionality. #5711
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
base: master
Are you sure you want to change the base?
Conversation
I added the symbols from libc that are already linked into MP to the mp_port_fun_table. Additional symbols can be added when needed, but these ones come at very little cost (4 bytes of flash per symbol, total of 80-odd symbols). |
I've looked over this PR and I think it's a good solution to the problem of a port exporting a custom list of symbols for the dynamic loader. And I think it can be simplified a bit further: there's no need to have the new
There do not need to be any changes to And this scheme would generalise to adding arbitrary numbers of tables, with table0 being the existing Then it's up to each port to provide this extra table and specify what is in it. This should be rather fixed so that .mpy files don't need rebuilding all the time. It should also be versioned, eg with a hash of all the strings/signatures of the included functions. |
Thanks for reviewing, I'll address the individual points over the coming days.
I'm a bit confused by your comment about versioning. The way I envisioned it is:
With the rules above the index of a given function never changes and "versioning" is pretty automatic 'cause either the required functions are there or the import is aborted. The only issue is if at some point the deprecated entries use up too much space and a reset of the table is desired. |
Yes that would work OK for the specific case of the esp32 port exposing the IDF. But consider also:
|
Hmmm, what happens if my custom esp32 build changes the mp_fun_table? (I suspect stuff would crash, why would the mp_port_fun_table be different?)
I can see three options:
A nice thing about the last two options is that they could allow a subtable of common ARM entry points and another subtable of architecture-specific entry points |
cae1455
to
a844d08
Compare
@dpgeorge this PR is ready from my point of view. I believe I've addressed all comments, except for the versioning. I reworked the commits into two: one for the port-independent part and one for the esp32. I believe this is more useful than a historical bang-by-bang split because it shows what the next port needs to copy/adapt. I did pull in #5715 because I need the espidf-v4 build to use the correct GCC version, otherwise the WRT versioning, I added code to raise an exception at import time if there is an mp_port_fun_table reference in a module and the table is not supported and also at run-time if a slot that is not available is referenced. This doesn't guard against running on a firmware that has a table with completely different functions, however. If you think we should implement the latter then please outline what you'd like done. E.g. do we add a port_table_vers into the mpy header? Need to bump the mpy version then (yuck). Or use the top 16 bits of the index into the table to designate the table version. Or ... To be honest, I would postpone all that until it actually becomes relevant, i.e., some other port actually implements the mp_port_fun_table and there actually is potential for different table content. |
@dpgeorge nudge ;-) I know you're swamped, do you perhaps have an ETA? |
…ctions in native modules
a844d08
to
bbab4b5
Compare
rebased on master |
bbab4b5
to
cbc939c
Compare
cbc939c
to
5bde2b9
Compare
It looks like very clever and useful code. Any reason not to commit this? |
@dpgeorge IMHO this is an important PR to get in because it allows a lot of functionality to be built without modifying MP itself. That reduces the pressure to get more and more stuff added to MP. You mentioned elsewhere that you'd like to get a release out soon, I can see two positions:
I can see arguments for both sides. |
Any possibility in getting this added in? It would be nice to have program that I can distribute with out needing a modded MP install. (I needed WPA2 EAP support for my ESP32 which MP doesn't expose.) |
Hello, I'm also interested in this feature to enable pcnt support : esp32-counter |
Six months have passed. |
Now that 1.13 is out, it may be the right time to bring these functionality to micropython firmware ? |
According to Damien's plan this is not slated for v1.14. I use it all the time with my own builds and the one thing that makes me a little uncomfortable is the versioning aspect. Maybe we can come up with a solution in the meantime? The problem is that a mpy file gets compiled against a specific function table and then when it's run on a target there is no guarantee that the function table is the same. In principle, the function table should only ever get additions so the existing check to make sure nothing indexes past the end of the table is sufficient. I believe this covers releases. But locally, while experimenting around, I have added entries, then removed them to replace them with others, etc. The only flexible solution I can come up with is to double the size of the function table such that each 4-byte entry can have an associated 32-bit CRC that covers all entries up to and including it (the CRC would be over the concatenated function names). An mpy file could then include the index of the highest entry it uses and the associated CRC. Then it's an easy comparison to make sure that all entries referenced by the mpy file are unchanged. In the end, I'm not sure all this is really worth it. The function table is already pretty large, so doubling it isn't a minor detail. In addition, this only guards against explicit changes in the table. It doesn't guard against functional changes. For example, a module intended to work on esp32 with esp-idf v4 may seem compatible with a target running esp-idf v3 but the functions it calls may behave quite differently. Maybe it's best to postpone adding further checks until there's a bit of an accumulation of issues so we know what the real-world failure modes are? |
I guess its time to start building custom firmware again to add support for WPA2 EAP again. |
I'm having second thoughts about this PR... Or maybe it's third thoughts by now... Every time I try to write a dynamically loaded module I bump into the issue that the functions I need are not all exported. So I go and add to the table of exported functions. But that's really a PITA and kind'a defeats the whole purpose. For example, using an esp32, right now I'm writing a function that registers an GPIO interrupt handler that is almost identical to the one in machine.Pin except that it passes the current time (time.ticks_us) to the soft-IRQ handler. The IRQ handler needs to call mp_sched_schedule and mp_hal_wake_main_task_from_isr, neither of which is included in the list of exported functions as of this PR. I'd like to propose a different implementation of dynamic loading, which is to include a load map of the micropython firmware in the on-board filesystem (i.e. something derived from the application.map), include the names of the called functions in the native module, and do the linking at load time using the file. I did a quick test and including all external text symbols that don't start with __ results in just under 100KB. Thoughts? |
I've had the exact same experience for the past 5 years: just when you think you have all functions, again you find one which you can use, so in the end the only thing which really works well is having the complete publicly visible API available one way or another. That also saves a bunch of time spent on discussion and X commits to 'add Y to |
good idea! |
Maybe add a build script to allow generating map files that can be read and used to map items not in mp_fun_table? |
I'd love to see a full listing, and I think a separate map file on the spi filesystem is a good compromise. I have been using #5711 on a number of devices quite happily, but needed |
Yes that could work, on the surface it seems like a scalable solution which could work across all the different kinds of devices/systems. A related but simpler solution would be to just do the linking on the host when the .mpy file is created: the load map would be on the host (not the device) and the .mpy file would be generated for that particular load map. The downside of this approach is that .mpy files are no longer very portable So the options seem to be:
|
Is a feature like this still being worked on? Or is the only alternative still to make custom firmware binaries to add very simple functions that make calls the parts of the IDF that are not exposed. (For example adding WPA2 EAP support to wifi by simply calling the IDF functions) |
@wgaylord I'm not seeing any forward movement on any larger esp32 PRs, so I don't expect this to get resolved anytime soon. |
So, there is any chanche that this pull request will be merged? |
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
This PR builds on #5682 (and is similar to #5653). Additions:
mp_port_fun_table
is populated automatically from the ESP-IDF documentation (RFC: Mechanism for allowing dynamic native modules to call port-specific functionality. #5682 left that out).#include
esp-idf header files can use the same definitions as those used for the firmware in the first place.mp_port_fun_table
are dynamically linked using fix-ups, no explicit double-indirection via themp_port_table
necessary.This PR is obviously a lot longer than #5682 but that is primarily due to the reworking of the Makefile and the generation of the
mp_port_fun_table
contents (both "conveniently" left out of #5682 ;-) ). IMHO themp_port_fun_table
is not usable without those two features. The dynamic linking fixup changes are pretty small after that...Items left to do:
add stdlib functions to(update: done in 30eca7d)mp_port_fun_table
: for example, I neededstrlen
is a simple native module I wrotewrite/update user documentation(done)fix(done)mp_port_fun_table
contents for esp-idf v3 (there are a couple of functions in the table that don't exist)fix error handling in the dynamic linker: if an entry in the(done)mp_port_fun_table
is 0 or the index is out of bounds the import process should be aborted