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

Enable more features in the embed port #11430

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

cwalther
Copy link
Contributor

@cwalther cwalther commented May 5, 2023

In preparation for using the embed port in a project, I am trying to enable more features in it than the minimalistic examples/embedding demonstrates. I am making good progress, however for some features I was unable to achieve my goal completely out-of-tree and required patches to the MicroPython source tree itself. This PR is my attempt to get these patches upstream.

I am not done yet with enabling more features (next on the list is a filesystem), hence the draft status, but I still wanted to publish my work in progress for public scrutiny, in case people more familiar with MicroPython internals than me have better ideas on how to achieve my goals. Accordingly, in the short term, I plan to update this branch by force push occasionally, to incorporate new insights or to rebase on current master. Once I consider it done, I will remove the draft status.

@github-actions
Copy link

github-actions bot commented May 5, 2023

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@cwalther cwalther force-pushed the embed branch 3 times, most recently from 9751e6c to 0eb84d3 Compare May 5, 2023 23:37
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (f2d7ce0) to head (7016512).
Report is 2 commits behind head on master.

❗ Current head 7016512 differs from pull request most recent head ca93048. Consider uploading reports for the commit ca93048 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11430   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cwalther
Copy link
Contributor Author

cwalther commented May 6, 2023

Okay, CI has completed successfully (including the new Check examples / embedding-full job), so no more force pushes for a bit.

@jimmo
Copy link
Member

jimmo commented May 9, 2023

@cwalther Thank you this will be really helpful! Thanks for the really clean PR and separate commits.

Only one minor comment (maybe more for @dpgeorge) -- I was hoping that as we added more embedding examples they would be much more narrow one-example-per-specific-feature.

(This is something that annoys me in other SDKs, when I want to do only X but the nearest example does X+Y+Z and I have to figure out which bits are relevant to me).

That said, I don't think there's anything particularly wrong with having a "kitchen sink" example either.

@cwalther
Copy link
Contributor Author

cwalther commented May 9, 2023

I have no particular opinion regarding single-feature vs. kitchen-sink examples, although at some point the number of examples may become unwieldy or a maintenance burden. For now, as an improvement over the current state, I considered it good enough to have a “minimal” and a “maximal” example and leave it to users to interpolate between them according to their use case. But more could be added later.

I was also hoping that making additions one by one in separate commits would make it easier for a user to keep them apart – if it occurs to them to check the Git history. But of course this property will fall apart over time as other changes are made.

By the way, if you’re interested in my application, which is getting MicroPython on Playdate, I talked about it here a few days ago: https://youtu.be/uArei1EjJnM?t=1872.

@mattytrentini
Copy link
Sponsor Contributor

Very cool @cwalther; my Playdate will be arriving soon and I have always thought it'd be a great MicroPython device! However I had envisioned MicroPython running on bare metal, rather than within the Playdate environment - embedding it is an interesting idea! I'll be following closely... 😄

@jimmo
Copy link
Member

jimmo commented May 26, 2023

I'm keen to try and find some time to progress this soon. Sorry about the delay @cwalther

So it doesn't get lost, made a comment on a discussion thread that is relevant to this PR: https://github.com/orgs/micropython/discussions/11616#discussioncomment-6007798

@cwalther
Copy link
Contributor Author

I have rebased the current work on master, which required a few adjustments (mostly for the umodulemodule renaming). Will push the commits one by one so CI can check them all. Work continues on adding filesystems (which was interrupted by running into #11781).

@cwalther
Copy link
Contributor Author

I’m not sure what’s up with the intermittently failing unix port / coverage_32bit / thread_exc1.py, it seems to have absolutely nothing to do with my changes.

@mattytrentini
Copy link
Sponsor Contributor

@cwalther I've created a thread in the developer channel in the Playdate discord to discuss the possibility of MicroPython on Playdate; thought you might be interested 😄

@cwalther
Copy link
Contributor Author

I've created a thread in the developer channel in the Playdate discord to discuss the possibility of MicroPython on Playdate; thought you might be interested 😄

Thanks for the notice, I’ll take a look! I haven’t been on the Playdate Discord so far (only the forum, and haven’t looked at that either for a while).

By the way, I am still working on this as time permits, currently fixing some bugs in the Posix VFS.

@cwalther
Copy link
Contributor Author

Rebased on master with minor adjustments, in particular for #13653.

Next up on my to-do list is still filesystems (with #12137 merged, there is one obstacle out of the way).

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

@NemesisXB
Copy link

NemesisXB commented Mar 14, 2024

Hi @cwalther,
I tried building the embedding full example the building the embedded port fails with the following:

make -f micropython_embed.mk 
Use make V=1 or set BUILD_VERBOSE in your environment to increase build verbosity.
Including User C Module from modules/c_hello
mkdir -p build-embed/genhdr
GEN build-embed/genhdr/mpversion.h
GEN build-embed/genhdr/qstr.i.last
GEN build-embed/genhdr/moduledefs.split
GEN build-embed/genhdr/moduledefs.collected
Module registrations updated
GEN build-embed/genhdr/moduledefs.h
GEN build-embed/genhdr/qstr.split
GEN build-embed/genhdr/qstrdefs.collected.h
QSTR updated
GEN build-embed/genhdr/qstrdefs.generated.h
GEN build-embed/genhdr/root_pointers.split
GEN build-embed/genhdr/root_pointers.collected
Root pointer registrations updated
GEN build-embed/genhdr/root_pointers.h
-e Error: micropython-lib submodule is not initialized. Run 'make submodules'
make: *** [../../py/mkrules.mk:215: build-embed/frozen_content.c] Error 1

Running make submodules returns:

make: *** No rule to make target 'submodules'.  Stop.

What could be the issue?
EDIT: It was because I did not run make -C mpy-cross. Maybe the embedding-full readme should be updated with this step?

@cwalther
Copy link
Contributor Author

I suspect the make submodules should in this case be make -f micropython_embed.mk submodules, because that’s the makefile that builds MicroPython. Makefile builds the embedding application. I guess I should add that to the readme. Probably it wasn’t there because the original embedding example needs no submodules.

I guess make -C mpy-cross was not mentioned because it’s the same for every port. But good point, I can add it to both readmes while I’m at it.

@cwalther
Copy link
Contributor Author

Here come the filesystems: littlefs and POSIX.

Littlefs complicates things a bit, for two reasons:

  • Unlike all previous .c files in lib/ (crypto-algorithms/*.c for modhashlib, re1.5/*.c for modre, uzlib/*.c for moddeflate and modbinascii), which are #included by other .c files and must not be individually compiled in the application build, the littlefs sources are located in lib/ and do need to be compiled into the application. I have solved this by placing these files in a separate subdirectory libsrc/ in the micropython_embed distribution, so that the instructions for the application build system can still be “compile all .c files except those in lib/”. This would be simpler if included .c files and compiled .c files were already separated into different directories in the source tree. Or if there were no included .c files at all, but those files were named something other than .c, e.g. .h.
  • The application build system needs to add an additional include path, CFLAGS += -I$(EMBED_DIR)/lib/littlefs. This is because lfs2.c and other files inside the littlefs implementation refer to headers as "lfs2.h" (whereas vfs_lfs.c uses "lib/littlefs/lfs2.h", which is already covered by the previous -I$(EMBED_DIR)). I can’t think of any way to make this simpler without editing the library sources.

I am also hitting the limitations of the kitchen-sink example now: There are two mutually exclusive implementations of sys.stdin/out/err (MICROPY_PY_SYS_STDFILES): One in shared/runtime/sys_stdio_mphal.c, brought in by adding that file to the build, and one in extmod/vfs_posix_file.c, brought in by enabling MICROPY_VFS_POSIX. These can’t both be demonstrated by the same example at the same time. For now I enable one and then two commits later disable it again and enable the other, but in the final state that should probably be two separate examples.

Also rebased over the STATIC removal and updated readme as suggested by @NemesisXB.

Next steps: I think I now have the main features I need for my Playdate project, so I will now try this in an actual out-of-tree build in a Playdate project and see how it holds up.

When MicroPython is used as a submodule and built from the containing
project, e.g. for the embed port, `make submodules` fails because it goes
looking for the sub-sub-module paths in the outer repository instead of in
the micropython repository. Fix this by invoking git inside the micropython
submodule.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
It compiles and runs in this state, but a lot of functionality is still
missing, to be extended over the following commits.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
The implementation of mp_hal_stdout_tx_strn_cooked() does not belong into
the library, but into the embedding application. Some applications may not
want Python output to go straight to their stdout, or may not even have
printf() available.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
To include extmod and its dependencies in the output, include the word
"extmod" in make variable EMBED_EXTRA when building the port.

Ideally this would be deduced automatically from the set of modules enabled
in mpconfigport.h (in a more fine-grained way too), but I can't think of a
simple way of achieving that.

Change in mphalport.h: fixes compiler error "type name requires a specifier
or qualifier" in machine_i2c.h, included from machine_i2c.c.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
Use FROZEN_MANIFEST as documented in docs/reference/manifest.rst.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
This works out of the box in the embed port.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
Signed-off-by: Christian Walther <cwalther@gmx.ch>
Signed-off-by: Christian Walther <cwalther@gmx.ch>
Optionally adds gmtime, localtime, mktime, time, time_ns to the time
module, implemented using mp_hal_time_ns(). This could also be used by
other ports.

I'm unsure where to put modtime_mphal.h, it could also be in extmod. The
important thing is that for MICROPY_PY_TIME_INCLUDEFILE to work it must be
at the same path in both the port build (original source tree) and the
application build (micropython_embed distribution), therefore not in
ports/embed/port.

It is named .h, mismatching the corresponding ports/*/modtime.c, because it
must not be compiled separately, which naming it .c would make harder for
users of the embed port - they would need to explicitly exclude it, whereas
this way they can continue to just compile all the .c files found in the
micropython_embed distribution except those in lib.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
Signed-off-by: Christian Walther <cwalther@gmx.ch>
Signed-off-by: Christian Walther <cwalther@gmx.ch>
To make use of it, as demonstrated in the embedding-full example:
- Include the word(s) "littlefs1" and/or "littlefs2" in EMBED_EXTRA in
  micropython_embed.mk.
- Enable the desired MICROPY_*VFS* features in mpconfigport.h.
- Add include path "$(EMBED_DIR)/lib/littlefs" to your application build
  system.

For demonstration, a block device implementation containing a small
read-only littlefs2 image is added to the embedding-full example. The block
device could have been written in C and interface to some external,
possibly writable storage medium, but was written in Python and frozen for
simplicity.

What makes this a bit awkward is the fact that the littlefs sources are
located in lib/ and do need to be compiled into the application, unlike all
previous .c files from lib/ that are #included by other C files and must
not be compiled separately. Therefore, introduce a new directory 'libsrc'
in the embed build output that contains these files and can be added to the
application build system as usual, while 'lib' can remain excluded. The
headers need to stay in 'lib' because vfs_lfs.c refers to them under that
path. I am also not entirely happy about having to add an additional
include path to the application build, but see no way around that as long
as vfs_lfs.c refers to "lib/littlefs/lfs2.h" while lfs2.c refers to
"lfs2.h".

Signed-off-by: Christian Walther <cwalther@gmx.ch>
In the embedding-full example, that requires disabling mphal-backed stdio
again as it conflicts with the implementation brought in by
vfs_posix_file.c.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
cwalther added a commit to cwalther/pew-playdate that referenced this pull request Apr 21, 2024
Running the embedding-full example (minus POSIX VFS) from micropython/micropython#11430 and a rudimentary REPL.
@cwalther
Copy link
Contributor Author

Rebased on #14344 which fixes an issue I ran into with the Playdate project.

Things are looking pretty good on the Playdate front, the embedding-full example code from here (minus POSIX VFS) as well as a rudimentary REPL are running. Still lots of glue to write to hook things up to the Playdate environment.

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.

None yet

6 participants