Skip to content
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

RFC #2: Mechanism for allowing dynamic native modules to call port-specific functionality. #5711

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tve
Copy link
Contributor

@tve tve commented Mar 1, 2020

This PR builds on #5682 (and is similar to #5653). Additions:

  • the 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).
  • the ESP32 Makefile is split in half so native modules that #include esp-idf header files can use the same definitions as those used for the firmware in the first place.
  • the functions in mp_port_fun_table are dynamically linked using fix-ups, no explicit double-indirection via the mp_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 the mp_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 mp_port_fun_table: for example, I needed strlen is a simple native module I wrote (update: done in 30eca7d)
  • write/update user documentation (done)
  • fix mp_port_fun_table contents for esp-idf v3 (there are a couple of functions in the table that don't exist) (done)
  • fix error handling in the dynamic linker: if an entry in the mp_port_fun_table is 0 or the index is out of bounds the import process should be aborted (done)

@tve
Copy link
Contributor Author

tve commented Mar 2, 2020

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).

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2020

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 port_fun_table included in mp_fun_table as a subtable. The new port_fun_table can simply live alongside mp_fun_table as a second table. This means there are only 2 basic changes that need to be made to enable such port-specific symbols:

  1. make mpy_ld.py know about the second table, and what its entries are (already done in this PR)
  2. make mp_native_relocate() know about the second table and how to relocate entries to it (again, already done in this PR)

There do not need to be any changes to nativeglue.[ch].

And this scheme would generalise to adding arbitrary numbers of tables, with table0 being the existing mp_fun_table. But for now probable best to stick to just one extra port-specific table (like done in this PR).

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.

ports/esp32/Makefile Outdated Show resolved Hide resolved
ports/esp32/portnativeglue.c Outdated Show resolved Hide resolved
py/mkenv.mk Outdated Show resolved Hide resolved
tools/mpy_ld.py Outdated Show resolved Hide resolved
tools/mpy_ld.py Outdated Show resolved Hide resolved
@tve
Copy link
Contributor Author

tve commented Mar 3, 2020

Thanks for reviewing, I'll address the individual points over the coming days.

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.

I'm a bit confused by your comment about versioning. The way I envisioned it is:

  • the table starts with a set of initial entries
  • new entries are always added to the end
  • deprecated entries are nulled out and the linker is made to abort the import (it should also abort the import if an index past the end of the table is requested, I haven't figured out how to do the abort yet)

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.

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2020

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.

Yes that would work OK for the specific case of the esp32 port exposing the IDF. But consider also:

  • a custom esp32 build which uses a different table (would be nice for the import of a .mpy to at least raise an exception in such a case, rather than just crash)
  • IDFv3 vs IDFv4 (not sure if this is an issue, maybe a single .mpy can be loaded on both IDF3/4)
  • architectures like ARM Cortex-M which have many ports, and each port would provide a different table; eg stm32's will be different to samd's but both those ports would import the same .mpy without error

@tve
Copy link
Contributor Author

tve commented Mar 3, 2020

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.

Yes that would work OK for the specific case of the esp32 port exposing the IDF. But consider also:

  • a custom esp32 build which uses a different table (would be nice for the import of a .mpy to at least raise an exception in such a case, rather than just crash)

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?)

  • IDFv3 vs IDFv4 (not sure if this is an issue, maybe a single .mpy can be loaded on both IDF3/4)
  • architectures like ARM Cortex-M which have many ports, and each port would provide a different table; eg stm32's will be different to samd's but both those ports would import the same .mpy without error

I can see three options:

  • use some MPY versioning (I haven't looked at this at all so don't know whether something already exists, pointers welcome)
  • steal a few more indexes from mp_fun_table, e.g. 120-126 instead of just 126
  • use ranges in the mp_port_fun_table, e.g. stm32 could use 0..8191, samd could use 8192..16k, etc. (I think the loss in varint encoding efficiency can be tolerated)

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

@tve tve force-pushed the port-fun-table-link branch 4 times, most recently from cae1455 to a844d08 Compare April 10, 2020 01:31
@tve
Copy link
Contributor Author

tve commented Apr 10, 2020

@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 -Wno-builtin-declaration-mismatch doesn't work.

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.

@tve
Copy link
Contributor Author

tve commented Apr 17, 2020

@dpgeorge nudge ;-) I know you're swamped, do you perhaps have an ETA?

@tve
Copy link
Contributor Author

tve commented Apr 17, 2020

rebased on master

@carterw
Copy link

carterw commented Apr 27, 2020

It looks like very clever and useful code. Any reason not to commit this?

@tve
Copy link
Contributor Author

tve commented May 8, 2020

@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.
(Just as an example, I'm planning to pull adjtime/settime out of the "fix time" PR 'cause they are only implemented on esp32 and they can be implemented as a dynamically loadable module with this PR).

You mentioned elsewhere that you'd like to get a release out soon, I can see two positions:

  1. this is a nice capability for a new release enabling lots of innovation
  2. this is an important new feature, would be good to get it in right after a release so it matures for a while until the next release

I can see arguments for both sides.

@wgaylord
Copy link

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.)

@pidou46
Copy link

pidou46 commented Jul 29, 2020

Hello,

I'm also interested in this feature to enable pcnt support : esp32-counter

@jcw
Copy link

jcw commented Sep 8, 2020

Six months have passed.

@pidou46
Copy link

pidou46 commented Sep 11, 2020

Now that 1.13 is out, it may be the right time to bring these functionality to micropython firmware ?
By the way, it is one of the last issue left in the V2.0 roadmap, so hopefully it should happen ?

@tve
Copy link
Contributor Author

tve commented Sep 15, 2020

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?

@wgaylord
Copy link

I guess its time to start building custom firmware again to add support for WPA2 EAP again.

@tve
Copy link
Contributor Author

tve commented Oct 5, 2020

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?

@stinos
Copy link
Contributor

stinos commented Oct 6, 2020

I bump into the issue that the functions I need are not all exported

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 mp_fun_table'. The separate map file seems the most logical as it allow things to remain micro: map file itself can be optional, and the content as well by e.g. allowing the process creating it to filter functions based on regexes or on directories so one can say 'only functions from py/'.

@tve
Copy link
Contributor Author

tve commented Oct 6, 2020

allowing the process creating it to filter functions based on regexes or on directories so one can say 'only functions from py/'.

good idea!

@wgaylord
Copy link

wgaylord commented Oct 6, 2020

Maybe add a build script to allow generating map files that can be read and used to map items not in mp_fun_table?

@itdaniher
Copy link

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 esp_wifi_set_ps which is, of course, not in this mp_fun_table.

@dpgeorge
Copy link
Member

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.

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:

  1. Known symbols/exported entities built in to the firmware image (with the choice of whether the symbol names be included in the firmware, of if the host needs to know a symbol<->index mapping).
  2. Symbols and their address stored on the device's filesystem (potentially compressed).
  3. Symbols and their address stored on the host.

@wgaylord
Copy link

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)

@tve
Copy link
Contributor Author

tve commented Dec 23, 2020

@wgaylord I'm not seeing any forward movement on any larger esp32 PRs, so I don't expect this to get resolved anytime soon.

@shakin89
Copy link

So, there is any chanche that this pull request will be merged?
It could be very useful to build some iot like wind sensor for example.

@itdaniher
Copy link

Many applications opened up by this! Heartened by the further activity in micropython, but unsure if @tve is still interested. /cc @pidou46

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants