It's great that this contains docs, and actually starts a whole new "Development" chapter. Writing great docs is hard though, some suggestions from my side.

Thanks for the suggestions!
I can't take credit for any of the existing docs, they were all done by @aykevl in the original PR and I haven't really updated any of them for the changes I've added.

I certainly intend to, as alluded to by the todo in my description... it's just taking me some time to get to it. Your comments will definitely help with the cleanup.

Regarding the python header generator, yes it and the docs definitely warrant extended detail as there is no code within the micropython codebase as reference for it's use.

I'm also thinking the header generator might need extending also to allow for early & late boot hooks as well as shutdown hooks in main for modules that have init requirements.

I've worked around this so far in the omv module in the example link by enabling and using import init hooks on the module, but this doesn't work so well when one thing (framebuffer) needs memory initialised before its use by a number of related modules. This also doesn't really allow for cleanup before a soft-reboot.

I'm happy to see you working on it! I have been really busy with some other stuff lately so my MicroPython work has been kind of stalled... Feel free to do whatever you want with the code (as long as I get part of the credit somewhere). I'd like to see the functionality included in MP whatever the exact form :)

@andrewleech : Thanks for continued work on this!

This is a rebased and extended version of PR #3871 by @aykevl

I can't take credit for any of the existing docs, they were all done by @aykevl in the original PR and I haven't really updated any of them for the changes I've added.

I see, I kinda thought that initial USER_C_MODULES support was already merged, and this PR extends on it. I see that it wasn't. Given that @aykevl seems to "pass the baton" on this matter to you, can you perhaps add "the final comment" at #3871 that this PR is intended to supersede it fully?

Yep sure, I've added a note as such there.

I've not pushed getting this one merged as I feel there's a bit more that could be done cleaner, of note I'd prefer my other module declaration PR first then I could get rid of the build script and json file in this pr.

I'm also thinking the header generator might need extending also to allow for early & late boot hooks as well as shutdown hooks in main for modules that have init requirements.

So, my, hopefully expected, suggestion would be to not get too carried away with it, and wait for first parts of it merged before adding more features. Actually, as I mentioned, I thought that at least parts of #3871 were merged.

And I'm glad that you try to factor out independent parts of it, because that's, as you explained, happens in #4289. Doing that way is also a suggestion a could make.

So, those are my suggestions, which I try to follow (and tried to follow when I was a comaintainer in this repo), and I'm not sure they apply to this repo (any more). (If the situation doesn't improve and you'll want to discuss it, you know which projects to submit tickets to ;-)).

I also understand this comes from your needs as a vendor of a particular system. That's also a top-level goal of my comments - to provide feedback which could allow this to be a feature which enables new levels of cooperation/reuse in the MicroPython ecosystem.

So, let me get to that point in my next comment.

See an example of its usage at https://gitlab.com/alelec/mp_omv

So, I think that's a great example, it shows that this featureset allows to address one of the big problems in the current MicroPython ecosystem - there're many great forks, which offer some focused functionality (machine vision, bindings for a bunch of C modules, etc.). But those forks are usually also hardware-specific, and fairly speaking, they're just vendor forks, where their owners care just about their specific product at hand and nothing else.

So, this PR lays the grounds how to address that, allowing interested parties to extract the functionality, and make it reusable across various forks. Again, taking OMV makes a great example. Let's however look at the technical details of how it's done now.

In this case the omv folder is the extra c module.

Ok, so that repository is not a repository for external MicroPython module. Let's face it - it's a typical vendor monorepo set up for adhoc product needs.

It must include at minimum a file micropython.mk declaring the make requirements to to build your module.

So, going inside "omv" folder, there's no micropython.mk.

And that's the current issue - this PR offers a great promise, and shows a great example where it can be useful, but so far, it doesn't try to set up "best practices" how to do that.

And while there can be updates to implementation of this PR, while merge process is also not clear, I still would like to raise the discussion of these best practices.

As I already wrote a bunch, let me get to specific proposals.

The intended goal: There's a wealth of independently-maintained C modules for MicroPython. An end user (!) should be able to integrate any of them easily to build their own binary. By "end user" I mean a person who can build a project from source, that should be a baseline requirement of a user for an open source project anyway.

So, let's think (if there's interest) how to move in that direction. My idea:

1. Set up a git repository which show the best practices how an external C module should be organized/structured. I'd say it should be something more featureful than add_ints from the docs example in this PR (thanks for updating in btw!), and definitely less complex than OMV. As a random thought, there was a question some time ago about linked list support. It totally makes no sense to implement it in C, as it can trivially done in Python. But as an example, why not? Turning Python into Scheme as a weekend hack isn't the worst thing which can be done.
2. Let's consider ways how such modules (and we'll have one example) can be integrated into local build easily. As usual, it can start lean and mean, and can be elaborated gradually.

So, those are my idea. To clarify on their status, let those be somewhere inbetween "suggestion" and "something I'd like to do". I find the current situation with the mainline maintenance frustrating, and round-robin into my other projects, which I pushed back trying to work on another streak to "finish MicroPython". Actually, I'm pulling myself by hair to write down all this - to not regret later that I didn't take a chance to keep up the momentum of work on great features. So, that's it - if you agree that the above makes sense, feel free to draft it. If you hands are full, count me in (some time later apparently). And if you don't agree that's right/needed way for this PR to evolve, well, let's either discuss, or just let know.

Thanks!

So I made a fork of micropython so that I could apply a previous version of this (or some similar) patch, and have done some work with it. Some of the result you can see here: https://github.com/sdfgeoff/slate-mech/tree/master/software/onboard/python_c_modules and the makefile to co-ordinate the build flags is here: https://github.com/sdfgeoff/slate-mech/blob/master/software/onboard/Makefile
Of the three modules written there, one of them is definitely hardware bound (the connection module uses the ESP32's internal radio), one of them is partially hardware bound (the chassis module uses a UART, but this could be abstracted somehow), and one of them is not hardware bound at all (kinematics is purely mathematics). I am not sure the best way to handle these three different things.

I haven't looked at the docs about develing for micropython recently, but last I checked there wasn't any documentation on the internals (eg how to bind your own function), so I had to learn it by looking at existing modules. While I am not particularly fluent with the micropython binding code, I am happy to write some documentation on what I do know - But that will be easiest to do once a PR is merged and already has some place in the docs.

My proposals regarding "best practice":

1. Have the git directory structure contain the micropython.mk file in the root directory. This allows direct cloning of the repository into the path specified as the USER_C_MODULES.
2. Frequently there is some library you are integrating, and the binding code to micropython. In the "connection" module, I separated these two things, and suffixed the binding code with "_py". I found this useful as it makes it easier to find the binding code. It may also be able to write a generator that attempts to create this binding code from a supplied .h file - but this is certainly a project for "later"

Thanks for the detailed suggestions, I agree with most all that's said and will follow up in more detail when I have some time.

While in general I agree with the principle of getting initial versions in and improving from there, when it comes to the methods of declaring and defining module interfaces though I feel it's worth trying to get it right first time because it will be much harder to change once people are using it.

For now though, my omv fork is intended as a best practice working example on the omv_module branch, for some reason though that repo keeps reverting the default bench to master: https://gitlab.com/alelec/omv/tree/omv_module

While in general I agree with the principle of getting initial versions in and improving from there, when it comes to the methods of declaring and defining module interfaces though I feel it's worth trying to get it right first time because it will be much harder to change once people are using it.

That's definitely true, especially with a kind of maintainer as @dpgeorge who thinks that any goof done once ago should stay their forever (my formulation, yeah ;-) ). At the same time, "getting it right" approach doesn't really help either, as @sdfgeoff's example shows. Once it's there as a PR, it's already in the wild.

And there're more risks. The MicroPython API is not stable, and surely will change.

And all that is ok. There should be implicit "social contract" among people who make/use this stuff - it may (will!) require further updates down the road, and everyone should be ready for that.

But yeah, it makes sense to "design for breakage". For example, please consider adding some versioning scheme to micropython.mk - at least as a declaration, but validating is even better (can be done later). And actually, ideas above clearly lead to the need of some metadata, so maybe makes sense to go there right away (hint: I don't think it should be in JSON format ;-) ).

Another issue with "do it right first" is chicken and egg problem. I'm not interested to dig in make files/scripts, I want to play with modules. But once some easily integratable modules are available, it's already too late - I just want to squash this stuff up and pull into my fork ;-).

https://gitlab.com/alelec/omv/tree/omv_module

D'oh, that answers. So yeah, it's pretty complex an example, needs something easier to not scare away people.

Yes versioning the interface is always a good idea.
I did think at one point out would be better to not have a makefile fragment at all, instead declaring all the needed bits of info ( c files, header paths, etc ) in a more user friendly metadata file, perhaps that is a better way to go in general... but it is less flexible.

And yeah, I know, I hate json too. My goto config format is yaml with my structured_config module however pyyaml can be hard to install for some platforms. I gave up trying to port any useful yaml parser to micropython.
Toml is another good format, but again does have external dependencies.
Json's one main advantage is being built-in.

That being said there's probably no real reason to not just make a metadata file in python format itself?

micropython-lib goes a long way with "foo = bar" metadata files ;-). But I'd start with "foo: bar" syntax indeed, which is YAML, even if with adhoc parser being used.

Yeah true, it doesn't always need a serious syntax, basic key value pairs go a long way.

For functionality such as this PR, the basic author, description and attribution details like in micropython-lib are useful, then a list of C files (probably with wildcard support) and include paths, then a catch-all cflags field would probably be enough for most users.
Declaring module objects could be via defines with my other PR or also in the meta. A method of listing startup init / de-init functions can come later.

Perhaps a makefile fragment could be the optional extra for module that need anything else on top of that.

IMHO, that bikesheds to the other side of the hill. Does makefile fragment gets stuff built? Quite, as OMV example shows. Let it be.

Now there's idea to add "build system version". A natural place would be the same micropython.mk, but there's a suspicion that this very file is also a part of "current version". If way to spec source files changes, do we want to carry a dummy micropython.mk with just a version inside forever? Maybe no, that's where an idea to put a version inside another file comes. And then perhaps let that file be a container of any other additional metainformation too.

YMMV

Ping @andrewleech. If there're not enough resources to deal with windows stuff now, I'd suggest to just unbreak the mainline build and deal with windows support later.

Question:
Does this feature support external libraries that take advantage of Micropython gc?

On LittlevGL Micropython bindings we attempt to replace LittlevGL memory allocation functions with Micropython's gc-aware allocation functions when Micropython bindings are enabled. This requires marking all gc roots on LittlevGL and registering them as Micropython gc root pointers.
Now it's done per port although a better approach would be to do it globally for all ports when LittlevGL module is used.

Was there any thinking done on how to register an external library root pointers with gc?
I didn't read the whole thread and related issues so sorry if I missed it somewhere.

@amirgon

Was there any thinking done on how to register an external library root pointers with gc?

No, I don't think that was considered within the scope of this PR so far. A case of needing to register static GC roots doesn't arise too often, but I may imagine it's needed for a more complex case like LittlevGL.

Well, I guess handling that could be achieved with some additional custom preprocessor, but would be nice to start with a less intrusive solution.

On baremetal ports, where we fully control linking, this could be achieved by by defining a special section where to put extra root pointers, wrap that in a macro, and offer for modules to use.

The problem is with OSed ports, like Unix. However, there's a claim that everything should just work automagically with Linux/ELF: https://stackoverflow.com/questions/11840651/how-can-i-implement-a-dynamic-dispatch-table-in-c/11844418#11844418 .

@dpgeorge, @andrewleech, what do you think about such approach?

@amirgon Your questions about "registering gc root pointers" is beyond my understanding of the micropython gc... I've used 338635c to customise the start and end of gc to leave a unmanaged ram space for a library to use, and I've also used openmv as a module which uses gc functions instead of malloc/realloc/free https://gitlab.com/alelec/omv/blob/omv_module/xalloc.c but I don't know of any alternate usage methods.

On an related not, I love the look of littlevgl ! We're starting to use micropython at work and there's talk of projects wanting lcd gui's, at first glance this looks like a pretty nice fit. I may be able to find some time to try out configuring it as a cmodule with this pr (though not likely too soon), but if you're wanting to have a go at it I'm more than happy to support any way I can!

@stinos / @pfalcon yeah I'll try to bring in your suggestions and/or help fix windows support. I've recently moved house though and still setting things up there which has seriously cut into the amount of dev time I've had at home lately!

@stinos thanks for the vs project patch, that fixed the module registration portion of this changeset nicely (along with some cleanups in my python script).

The actual core functionality of compiling external C modules into micropython is not satisfied in the windows port still as I think it would require quite a bit more work.

In the normal make usage of this PR, you would have a new top level project with micropython as a git submodule. Then there's one or more extra folders of C code and a matching makefile fragment which will be included in the overall make command to build the folders into micropython.

For windows/visual studio support, you would need a vcxproj file in the cmodule library folder(s) matching the makefile fragment(s), then a visual studio solution in the top level folder including the micropython.vcxproj along with the various cmodule vcxproj files into one project.

@andrewleech: Ok, thanks for updating this.

Note that "During make, makemoduledefs.py parses the current builds c files for" is not a compliant commit title.

So, appveyor build is now fixed, but now travis has some issue.

Coverage build weirdly fails with:

{u'message': u"Couldn't find a repository matching this job.", u'error': True}
The command "(cd ports/unix && coveralls --root ../.. --build-root . --gcov $(which gcov) --gcov-options '-o build-coverage/' --include py --include extmod)" exited with 1.

@andrewleech, can you please re-push (e.g. with a fix to that commit title) to retrigger build and see if it was random glitch.

There's also:

$ tools/check_code_size.sh
Old size: 69820 new size: 69824
Validation failure: Core code size increased
The command "tools/check_code_size.sh" exited with 1.

But that's ok.

@pfalcon Thanks, I lost the top line of the commit msg during a rebase and squash.

I'm a bit confused about the code size change however, there really shouldn't be any change... it should all be in macros and defines that amount to the same code generated.
I guess the array module I've defined via this as example is now at the bottom of the array rather than the top, whether that's changed some optimisation somewhere?

I'm a bit confused about the code size change however, there really shouldn't be any change...

I wouldn't worry too much about those 4 bytes in this case, that check is there for people who don't pay attention to such matters at all.

And now it's clear there's something with coveralls testing for all recent PRs, @dpgeorge, can you please look into this? Did it start after you refactored .travis.yml?

And well, @andrewleech, now it's time to ping @dpgeorge to get this merged (because little seems to be merged by its own ;-) ).

Thanks all involved for the work getting this to a good state. I trust the logic and structure behind it is sound and would be happy to merge without detailed review on my side. All I would do is fix a few typos during the merge.

Re travis and coveralls: the failures started a few days ago, maybe due to github deprecating its old status API, not quite sure. I already resync'd with coveralls.io and the last push to master seemed to work correctly.

For windows/visual studio support, you would need a vcxproj file in the cmodule library folder(s) matching the makefile fragment(s), then a visual studio solution in the top level folder including the micropython.vcxproj along with the various cmodule vcxproj files into one project.

That could work but I'm not sure that's going to be the most convenient. Solution file isn't really required anyway, only as project container in VS, and if you add the module project to it you'd have to exclude it from the build anyway because building micropython.vcxproj is going to build the module already.

Now, the makefile fragment is just a list of source files and compiler and linker flags. So theoretically there are 2 ways to use go with this for msbuild: look for micropython.proj (or some similar name) and treat it as an msbuild file, and import it in the build. Alternative: require GNU make to be available and use that to parse micropython.mk. Which is nice because modules could be cross-platform (i.e. not requiring both micropython.mk and micropython.proj) but does mean people need to be able to properly install make on Windows (from msys2 or from cygwin or whatever works these days) and know how to use Makefiles and it's a bit more advanced to get working. Ideas?

To make progress with this (ie merge it), how about we postpone support for use with Windows? When someone who uses Windows finds they need/want this feature then it can be implemented. As long as the changes made here don't break the normal Windows build (which it looks like they don't) I'm happy with it.

To make progress with this (ie merge it), how about we postpone support for use with Windows?

Sure. I really want to implement this but at the moment I'm swamped with all kinds of work so it's not going to happen anytime soon.

@andrewleech looking at merging this, I think it'd be cleaner to squash all commits into one. This is because docs files were moved/renamed since the first commit by @aykevl, and one of the support scripts (tools/gencmodule.py I think) was added and then removed during the course of the 4 commits here. The PR adds a single feature so it makes sense to have it squashed.

But then author of the whole squashed commit will be @aykevl , not yourself. I suggest either 1) to squash all into one and add a comment in the commit about your involvement; 2) squash your 3 commits into one, separate to @aykevl's commit, so there would be 2 commits in total left. Maybe option 2 is the best to give you both credit.

@dpgeorge I'm happy to squash them down more, though the "objmodule" commit in the middle is separate functionality to the cmodules stuff, so I think it would be worth keeping separate (probably before the cmodule stuff).

I'll try to squash the others down to two cleaner commits that don't write then move now-nonexistant files docs etc.

I'm fine with squashing (even if the commit isn't mine anymore), as long as something says I contributed to it.

Ok, this was merged in cf22f47, 2e51607 and 89ff506

Thanks all!