Skip to content

Implement a module system for external C modules. #3871

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

Closed
wants to merge 3 commits into from

Conversation

aykevl
Copy link
Contributor

@aykevl aykevl commented Jun 14, 2018

This system makes it a lot easier to include external libraries as native modules in MicroPython. Simply set MODULE_DIR to the directory containing the modules. It was originally inspired by this solution by @glennrub.

A module contains a module.mk file, which adds required C files to SRC_MOD. For example, for a module called foobar, this could be the module.mk file:

SRC_MOD += $(MODULE_DIR)/foobar/foobar.c

Compared with #1627, this is a much simpler solution to implement, though maybe harder for a user. It would make a system like the NodeMCU custom build service much easier to implement, however.

EDIT: updated to new system with MODULES_DIR make param.

@aykevl aykevl force-pushed the modules branch 2 times, most recently from 73f1b12 to 7813846 Compare June 14, 2018 14:42
@aykevl aykevl changed the title all: Implement a module system for external C modules. Implement a module system for external C modules. Jun 14, 2018
@stinos
Copy link
Contributor

stinos commented Jun 14, 2018

Pretty neat but perhaps make the modules directory configurable?

@aykevl
Copy link
Contributor Author

aykevl commented Jun 14, 2018

You mean like FROZEN_MPY_DIR? That sounds like a good idea.

@stinos
Copy link
Contributor

stinos commented Jun 15, 2018

Yeah something like that. So modules kan be kept out of the source dir. You also wouldn't need the .gitignore file and modules.md could go in the root dir or so.

py/mkrules.mk Outdated
@@ -105,7 +105,7 @@ endif
ifneq ($(FROZEN_MPY_DIR),)
# to build the MicroPython cross compiler
$(TOP)/mpy-cross/mpy-cross: $(TOP)/py/*.[ch] $(TOP)/mpy-cross/*.[ch] $(TOP)/ports/windows/fmode.c
$(Q)$(MAKE) -C $(TOP)/mpy-cross
$(Q)$(MAKE) -C $(TOP)/mpy-cross MODULE_DIR=
Copy link
Contributor Author

@aykevl aykevl Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is required to get mpy-cross to work. The cross compiler won't need these custom modules anyway.
EDIT: probably not as necessary now as it was before, due to other changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still necessary, in case your modules use features that are not enabled (or different) in the cross compiler.

@aykevl aykevl force-pushed the modules branch 2 times, most recently from 57bdf3c to f10fe9c Compare June 16, 2018 23:54
@aykevl
Copy link
Contributor Author

aykevl commented Jun 16, 2018

I've updated the PR with the option of setting MODULES_DIR, just like FROZEN_MPY_DIR.

@aykevl aykevl force-pushed the modules branch 6 times, most recently from b9fa52c to a7e2885 Compare June 17, 2018 00:36
@aykevl
Copy link
Contributor Author

aykevl commented Jul 3, 2018

@dpgeorge ?

py/mkrules.mk Outdated
@@ -105,7 +105,7 @@ endif
ifneq ($(FROZEN_MPY_DIR),)
# to build the MicroPython cross compiler
$(TOP)/mpy-cross/mpy-cross: $(TOP)/py/*.[ch] $(TOP)/mpy-cross/*.[ch] $(TOP)/ports/windows/fmode.c
$(Q)$(MAKE) -C $(TOP)/mpy-cross
$(Q)$(MAKE) -C $(TOP)/mpy-cross MODULE_DIR=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still necessary, in case your modules use features that are not enabled (or different) in the cross compiler.

def update_modules(path):
modules = []
for module in sorted(os.listdir(path)):
if not os.path.isfile('%s/%s/module.mk' % (path, module)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this puts a restriction on the directory structure, in that your module name must match the directory it is in. It's probably worth documenting this fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this puts a restriction on the directory structure, in that your module name must match the directory it is in. It's probably worth documenting this fact.

Good point, I've added it to the docs.

# Print header file for all external modules.
print('// Automatically generated by genmodules.py.\n')
for module in modules:
print('extern const struct _mp_obj_module_t %s_module;' % module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also puts a restriction: the module struct in your .c file must be <name>_module. That's worth documenting.

Also, to avoid possible clash with other names, it's probably worth making this name more descriptive, like <name>_user_cmodule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the new commit.

py/mpconfig.h Outdated
// Enable extra modules. This will be set by py/py.mk, don't set it in a
// config header file.
#ifndef MICROPY_HAS_EXTRA_MODULES
#define MICROPY_HAS_EXTRA_MODULES (0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this config option, you might be able to get away with just using:

#ifndef MICROPY_EXTRA_BUILTIN_MODULES
#define MICROPY_EXTRA_BUILTIN_MODULES
#endif

like it's done with MICROPY_PORT_BUILTIN_MODULES

Copy link
Contributor Author

@aykevl aykevl Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly that isn't directly possible due to a chicken-and-egg problem at the top of objmodule.c so I've left it in place. It would be nice to get rid of it, however.
A way to do that is by always generating genhdr/modules.h (even if empty). I'm not sure what the better option is.

py/py.mk Outdated
@@ -103,6 +103,13 @@ $(BUILD)/$(BTREE_DIR)/%.o: CFLAGS += -Wno-old-style-definition -Wno-sign-compare
$(BUILD)/extmod/modbtree.o: CFLAGS += $(BTREE_DEFS)
endif

# External modules written in C.
ifneq ($(MODULE_DIR),)
CFLAGS_MOD += -DMICROPY_HAS_EXTRA_MODULES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line won't be necessary if you get rid of MICROPY_HAS_EXTRA_MODULES (as discussed above).

@dpgeorge
Copy link
Member

dpgeorge commented Jul 4, 2018

Thanks for the contribution, it does look pretty neat. I didn't really come across the need for this so far, but I can see how it would be useful for users to easily build in their own C modules.

Compared with #1627, this is a much simpler solution to implement,

Well, I hope that we can eventually get to adding dynamic native code support, so this wouldn't be a replacement for that feature.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 4, 2018

I'd rather keep the root directory as clean as possible, so don't know if adding MODULES.md there is the way to go. It's a bit long to add to the root README.md, maybe it can go in tools (and referenced from the main README.md so it can be found)? Or add a simple example of extending the unix port with custom modules in a new dir in examples/? Or even put it in the docs?

I think the feature itself should be given a more unique name, rather than the generic name of "modules". I think "C" should be in the name to make it explicit that it's allowing additional C modules. And they are modules added by the user, so maybe USER_C_MODULES?

@aykevl
Copy link
Contributor Author

aykevl commented Jul 15, 2018

I have updated this PR in several ways:

  • Rename MODULE_DIR to USER_C_MODULES, as suggested.
  • Move documentation from MODULES.markdown to MP docs. They're not (yet) referenced from the base README, but I think they're easy to discover now when reading the online docs at https://docs.micropython.org/.
  • Rename module identifier from _module to _user_cmodule as suggested by @dpgeorge.
  • Rename module.mk to micropython.mk, because I think that's a better / more descriptive name.

The biggest (non-name change) is moving the documentation to the actual MicroPython docs, in the section titled "The MicroPython language". The documentation should now also be a bit more helpful.

I've pushed this as a separate commit for easier diff-viewing, but feel free to squash as needed (or I can do that as well).

@aykevl
Copy link
Contributor Author

aykevl commented Jul 15, 2018

I didn't really come across the need for this so far, but I can see how it would be useful for users to easily build in their own C modules.

I've needed it from time to time, mostly because the nrf targets are very tiny so won't include less commonly used modules like neopixel in the standard library (unlike the ESP8266). Also, I'd like to write a LED animation library at some point that can be used directly from MicroPython similar to how NumPy is used for fast number crunching by performing calculations on a whole array at a time. I've tried doing it in Python directly but it's just way too slow. Previously I made a fork of MicroPython for this but that one quickly got out of date.

Well, I hope that we can eventually get to adding dynamic native code support, so this wouldn't be a replacement for that feature.

Yeah, that would be nice. I think both have their uses. Dynamically loadable modules may be easier to use while this PR is more resource-efficient and easier to implement.

Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating. It looks good, just a few more minor things.


$ make axtls
$ make USER_C_MODULES=path-to-modules-folder
$ make deploy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need an explicit make axtls because that should be done automatically. And with make deploy it's wise to keep the extra make options there, like make USER_C_MODULES=path-to-modules-folder deploy. This is in case the make runs (part of) the build again before deploying, you don't want it to use different CFLAGS for certain parts of the build.

Instead of typing this long make command over and over again (and sometimes forgetting), it's easier to create a local makefile or GNUmakefile with your custom settings and then include the MicroPython Makefile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if you don't have an explicit make axtls then these instructions become generic for any port.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need an explicit make axtls because that should be done automatically

It's only done automatically in few (one?) ports. (E.g. not done in unix port).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the ESP8266-specific parts. The only goal of it was showing how the make flag works (for those not that familiar with make), not as a guide for how to deploy.

It may be useful to write something about make flags and GNUmakefile files in a compilation guide, but I think that's outside of the scope of this PR.

It's only done automatically in few (one?) ports. (E.g. not done in unix port).

Not in the ESP8266 port either, while I seem to remember it worked before. It would be nice if this were fixed, though. I have so many times run make just to discover I had to run make axtls first. Again, that's outside the scope of this PR.


To build such a module, compile MicroPython (see `getting started
<https://github.com/micropython/micropython/wiki/Getting-Started>`_) with an
extra ``make`` flag named ``USER_C_MODULES`` to the directory containing all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... named USER_C_MODULES set to the ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

py/py.mk Outdated
CFLAGS_MOD += -DMICROPY_HAS_EXTRA_MODULES
include $(USER_C_MODULES)/*/micropython.mk
SRC_QSTR += $(BUILD)/genhdr/modules.h
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling it cmodules.h instead of modules.h?

And then to eliminate the need for MICROPY_HAS_EXTRA_MODULES you could do this:

CFLAGS_MOD += -DMICROPY_CMODULES_INCLUDE_H="genhdr/cmodules.h"

Then at the top of objmodule.c use:

#ifdef MICROPY_CMODULES_INCLUDE_H
#include MICROPY_CMODULES_INCLUDE_H
#endif

And I wouldn't add this MICROPY_CMODULES_INCLUDE_H option to mpconfig.h because it's not really settable by the user, it's handled by this Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I've applied it.

@pfalcon
Copy link
Contributor

pfalcon commented Jul 16, 2018

The biggest (non-name change) is moving the documentation to the actual MicroPython docs, in the section titled "The MicroPython language".

What this stuff has to do with "MicroPython language"? "MicroPython language" is the language implemented by MicroPython (i.e. Python). C-level hacks have very little to do with that. Taking CPython docs (https://docs.python.org/3/index.html) as an example, "The MicroPython language" roughly corresponds to "Language Reference" https://docs.python.org/3/reference/index.html . Where "quick way to add C modules" stuff would belong is "Extending and Embedding
(tutorial for C/C++ programmers)" section of CPython. Such section doesn't exist in MicroPython manual. (And stuff belonging there should be stuck in random places in the docs instead.)

@aykevl
Copy link
Contributor Author

aykevl commented Jul 16, 2018

@pfalcon what would your suggestion be, then? Create a new section, or something else?
I know it's maybe not the right place. I first considered the "differences from CPython" section but that seemed even more out of place. Remember that it also has other docs that aren't really "the MicroPython language", like a section about modules (upip, frozen packages, etc.).

@aykevl
Copy link
Contributor Author

aykevl commented Jul 16, 2018

Thank you for the review.
Applied all suggestions, again with not-so-great individual commit messages (they should be squashed at the end).

EDIT: fixed a bug and renamed gen-modules.py to gen-cmodules.py (consistent with the other renames).
Also, I've written an actual module (very incomplete) over here: https://github.com/aykevl/mpy-pixels

.. code::

# Add all C files to SRC_MOD.
SRC_MOD += $(MODULE_DIR)/example/example.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked a bit better with USER_C_MODULES instead of MODULE_DIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my bad. I need to fix the docs there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@glennrub
Copy link
Contributor

I'm dry-running the proposal on my perlin noise c-module (https://github.com/glennrub/unoise/blob/adopt_to_ayke_cmodule_proposal/micropython.mk).

The genhdr/cmodules.h looks as expected:

// Automatically generated by genmodules.py.
  
extern const struct _mp_obj_module_t unoise_user_cmodule;

#define MICROPY_EXTRA_BUILTIN_MODULES \
    { MP_ROM_QSTR(MP_QSTR_unoise), MP_ROM_PTR(&unoise_user_cmodule) }, \

However, it does not look like the SRC_MOD files get's embedded into the build automatically as i would expect. Have i missed something essential that should have enabled the file into the build?

@dhylands
Copy link
Contributor

I don't see any references to MICROPY_EXTRA_BUILTIN_MODULES in the code. In particular, this is where all of the modules come together:

STATIC const mp_rom_map_elem_t mp_builtin_module_table[] = {

I've always added to the MICROPY_PORT_BUILTIN_MODULES #define

@aykevl
Copy link
Contributor Author

aykevl commented Jul 16, 2018

@dhylands are you sure you're on the right branch/commit? It certainly is in the diff (see the diff in GitHub for example).

@aykevl
Copy link
Contributor Author

aykevl commented Jul 16, 2018

@glennrub I've found the issue. Most ports add SRC_MOD to SRC_C in the Makefile, but not the nrf port. The fix is probably removing SRC_MOD from SRC_QSTR and adding it instead to SRC_C directly.

@glennrub
Copy link
Contributor

Thanks, @aykevl ! I tested out the proposed fix, adding SRC_MOD to SRC_C. The framework work as expected and i get my module loaded.

@aykevl
Copy link
Contributor Author

aykevl commented Jul 18, 2018

Rebased to master and squashed the first commit.

While testing the nrf port, I found a bug: the object files are put in a seemingly random directory (USER_C_MODULES relative to the build directory, in my case ports/). Because of this, make clean doesn't work as expected.

I'm thinking about the best way to solve this, probably by putting the object files in build/cmodules or something like that.

@pfalcon
Copy link
Contributor

pfalcon commented Jul 18, 2018

@pfalcon what would your suggestion be, then? Create a new section, or something else?

Generic suggestion would be either do it right, introduce "Developing and building MicroPython" top-level section to host all docs related to "C" things, or do it in the simplest manner, not adding kind of info the docs aren't ready to take in yet, and keep it in README or similar.

Remember that it also has other docs that aren't really "the MicroPython language", like a section about modules (upip, frozen packages, etc.).

All these chapters share the same high-level topic: they about "the MicroPython language" or infrastructure directly related to the language (variant of Python). For example, people who would receive a board with preprogrammed MicroPython or downloaded a firmware and flashed to their board, ideally would be interested in reading all docs in "the MicroPython language". However, they don't have to be interested, and likely will be confused, to read about "C language" and related stuff. That's why CPython has a strict separation in the docs between Python and C world, and it only makes sense to follow that separation.

aykevl added 2 commits July 20, 2018 01:03
This system makes it a lot easier to include external libraries as
native modules in MicroPython. Simply pass USER_C_MODULES (like
FROZEN_MPY_DIR) as a make parameter.
@aykevl
Copy link
Contributor Author

aykevl commented Jul 19, 2018

I have moved the documentation to a new section. I didn't add more information than the absolute minimum, because I don't think that belongs in this PR. (Also, rebased + squashed several commits).

Note: I have added it to wipy and pyboard as well, but I haven't tested this system on them.

@aykevl
Copy link
Contributor Author

aykevl commented Jul 20, 2018

I've fixed the incorrect build location of the C modules by symlinking the module directory in the build directory (see 4cd27d0). Not very nice, but I can't think of a better way (suggestions welcome!).

@andrewleech
Copy link
Contributor

I've been using this PR for the last couple of days with great success, I strongly support this being accepted.

I do have a couple of requested additions beforehand however, described in my matching branch: aykevl/micropython@modules...andrewleech:modules

Did you have a particular use case for linking the C files into the build folder?
This seems a bit strange and unnecessary in my usage and it also breaks debugging on windows (windows can't traverse the *nix links).

Without the linking, you need to be careful when you define USER_C_MODULES as it's used relative to the port folder you're running the Makefile from (micropython/ports/stm32 folder), but for me it works following the basic method shown in https://github.com/andrewleech/micropython/wiki/Customised-Micropython-projects

Also when using this mechanism to include C files that already have the micropython module definitions in place I don't want the definitions to be auto-generated by the python script, so my update checks for defs in files and using them first. If there are none, it still generates one as before.

@stinos
Copy link
Contributor

stinos commented Sep 11, 2018

windows can't traverse the *nix links

Didn't look into this in detail, but windows does support symbolic links so wouldn't the solution be to not create *nix links but windows links instead?

@andrewleech
Copy link
Contributor

Yes technically it's possible to do windows or *nix links as appropriate (with more makefile ifs and buts) but my point was I'm pretty sure it's complely unneccesary to link the files at all.

Nothing else I've seen in the codebase symlinks during build to copy c files around, and this works fine for me without doing so, as such it's much cleaner to not need to do it at all.

fwiw I'm actually running make through the official bash on windows, so as far as the Makefile is concerned it's actually running on linux, but then the debugging is in VSCode and windows gdb. This works great for me, it's really seamless. Until symlinks get into the c code path...

@aykevl
Copy link
Contributor Author

aykevl commented Oct 1, 2018

There is a reason these files are symlinked, and the reason is that otherwise the object files end up in the wrong place (not in the build directory). I hit this problem when I built something for an ARM chip, and then built the same module for an ESP (or the other way around) and the linker complained about linking object files for different architectures.

You may consider copying the files instead of linking if it is difficult on Windows. Or maybe you can come up with a different solution.

@andrewleech
Copy link
Contributor

@aykevl Ah I see, I'd noticed a similar issue at work yesterday where files from a different port which I was building into an external module had their object files in the wrong place, now I see that all cmodule files are doing that (just even further away again.

I fixed my problem yesterday by getting rid of the relative entries in the path with a bit of $(abspath pth) and the same trick appears to work here according to what I just tested:
e037374

Actually, this trick probably wont work on a native windows build where you've got a "C:" or similar in the absolute path that can't just be joined on the end of the build folder.

@andrewleech
Copy link
Contributor

I've got another solution for keeping the object files inside the build folder:
c4ec4c3

I was about to go back to your symlink solution, however that also breaks if the build folder is outside the micropython tree. It also still breaks confuses debugging as the debug symbols point to the symlinks rather than the real c files.

My new approach requires adding the USER_C_MODULES dir to the vpath when compiling which keeps all the c and object paths really clean.
The couple of extra changes in py.mk provide a convenience function to get the module dirname if you don't want it hardcoded in the makefile and strips off the absolute path from c files in case they're declared as such (or found with wildcard's etc).

@andrewleech
Copy link
Contributor

My cmodules PR #4195 includes and extends upon this one, hopefully as a replacement that meets all the original needs of this one.

@dpgeorge
Copy link
Member

dpgeorge commented Mar 8, 2019

Superseded by #4195, this PR here merged as part of that in 2e51607

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

Successfully merging this pull request may close these issues.

7 participants