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

Export arg parse functions for native modules. #14335

Merged
merged 2 commits into from
May 24, 2024

Conversation

BrianPugh
Copy link
Contributor

@BrianPugh BrianPugh commented Apr 19, 2024

Exports the very useful mp_arg_parse_all_kw_array for native modules.

I imagine we would also want to export mp_arg_parse_all, but I wanted to check in first to see if this is the correct path.

EDIT: Building a list of symbols that I wish I had:

  • mp_load_method_maybe - I'm trying to check if an object has an attribute without raising an exception.
  • mp_obj_str_binary_op - I'm trying to use mp_obj_is_str_or_bytes and this is missing.

Copy link

github-actions bot commented Apr 20, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +104 +0.013% standard[incl +32(data)]
      stm32:   +12 +0.003% PYBV10
     mimxrt:    +8 +0.002% TEENSY40
        rp2:    +8 +0.002% RPI_PICO
       samd:   +12 +0.005% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@dpgeorge
Copy link
Member

Luckly the native .mpy sub version has been increased for the upcoming 1.23.0 release, which means we can make changes like this before the release (because this changes the native ABI).

mp_obj_str_binary_op

You should be able to access this already via mp_type_str.slots[2].

mp_load_method_maybe

For this, you could call hasattr(...) (after loading hasattr from globals), or call mp_load_method(...) and wrap it in an nlr block, but both of those approaches are pretty inefficient. So probably worth exposing mp_load_method_maybe.

py/nativeglue.h Outdated
@@ -83,6 +83,7 @@ typedef enum {
MP_F_NATIVE_YIELD_FROM,
MP_F_SETJMP,
MP_F_NUMBER_OF,
MP_F_ARG_PARSE_ALL_KW_ARRAY,
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed.

py/nativeglue.c Outdated
@@ -348,6 +348,7 @@ const mp_fun_table_t mp_fun_table = {
&mp_stream_readinto_obj,
&mp_stream_unbuffered_readline_obj,
&mp_stream_write_obj,
&mp_arg_parse_all_kw_array,
Copy link
Member

Choose a reason for hiding this comment

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

Please move this up, say between mp_get_stream_raise and mp_binary_get_size. And then increase MP_FUN_TABLE_MP_TYPE_TYPE_OFFSET by 1 (in tools/mpy_ld.py).

@dpgeorge dpgeorge added this to the release-1.23.0 milestone Apr 20, 2024
@BrianPugh
Copy link
Contributor Author

Thanks for the review @dpgeorge ! I believe I have addressed your comments.

I'll try and get a MVP of my native module working this weekend. Would we like to add all the exported symbols I need to this PR? Or would we like to tackle them one by one?

I think was having issues trying to use nlr stuff in my native module on the unix port, possibly due to MICROPY_NLR_SETJMP? I didn't really investigate. I'll got ahead and go with mp_load_method_maybe for now.

@dpgeorge
Copy link
Member

Would we like to add all the exported symbols I need to this PR?

If there are only a handful of them, then, yes, please add them here in this PR.

@dpgeorge
Copy link
Member

I'll try and get a MVP of my native module working this weekend.

I see you merged BrianPugh/tamp#128. Did that require any more additions to the native exports?

@BrianPugh
Copy link
Contributor Author

I ended up with a few workarounds by executing the things I needed on the python-side of things. These are the concepts I ran up against (some already discussed in this PR) when writing native modules in C:

  1. Keyword arguments parsing for classes (and functions)? I got around this by defining my public api in python, and passing in all the arguments as positional arguments to a private C-defined class. Another (minor) issue is that the canonically defined static const mp_arg_t allowed_args[] cannot be static, since the value of new MP_QSTR_ aren't known at compile time. The resulting mymod.config.h looks like:
#define MP_QSTR__C (mp_native_qstr_table[1])
#define MP_QSTR__D (mp_native_qstr_table[2])
#define MP_QSTR_flush (mp_native_qstr_table[3])

I think it just has to be that way.

  1. Checking if an attribute exists. I ended up just doing this on the python side. We discussed possibly exporting mp_load_method_maybe. mp_load_method is a light wrapper around mp_load_method_maybe, and we could maybe remove the export by #defineing it, but it might not be worth it.
  2. Exception creation. I tried creating a custom exception on the C side of things, but I couldn't figure it out and/or it was unsuccessful at linking. I ended up just defining it on the python side, and then raising it in the C side via:
nlr_raise(mp_obj_new_exception(mp_load_global(MP_QSTR_ExcessBitsError)));

Is there an easy macro for creating custom exceptions in C?
4. File opening/interactions. I ended up opening the file on the python side, and then manipulating it on the C-side via:

mp_obj_t write_method = mp_load_attr(self->f, MP_QSTR_write);
// other stuff
mp_call_function_n_kw(write_method, 1, 0, &bytearray_obj);

however, the file could have been totally private to the C side, so I would have liked to been able to open/write/read/close from C. What are the main file interaction functions? mp_vfs_open? mp_reader_new_file?
5. The following bytearray macros would have been nice:

#define mp_obj_new_bytearray(size) mp_obj_new_bytearray_by_ref(size, m_malloc(size))
#define mp_type_bytearray (*(mp_obj_type_t *)(mp_load_global(MP_QSTR_bytearray)))  // Or explicitly export it

Conceptual hurdles I had to overcome: using m_malloc and how the GC is aware of the memory if the pointer is stored in the struct allocated by mp_obj_malloc, so it would be good to add an example/documentation of that in the natmod examples.


I can create more examples in examples/natmod/, and then export all the symbols once I receive feedback on the above 5 points.

@BrianPugh
Copy link
Contributor Author

@dpgeorge any additional thoughts on the above?

@dpgeorge
Copy link
Member

I ended up with a few workarounds by executing the things I needed on the python-side of things.

That's a good approach, to do as much as possible on the Python side. The C side should be for things that need to be fast, or things/libraries that are already written in C.

Another (minor) issue is that the canonically defined static const mp_arg_t allowed_args[] cannot be static, since the value of new MP_QSTR_ aren't known at compile time

Yes. There's really not any way around this. If you want to create an array of allowed arguments, you'd need to populate the qstr values at runtime (probably only once in the init function).

Checking if an attribute exists. I ended up just doing this on the python side. We discussed possibly exporting mp_load_method_maybe

IMO it makes sense to expose mp_load_method_maybe (as well as keeping the existing mp_load_method). This is a very common function and benefits from being fast, ie being a direct lookup in the native table.

Exception creation. I tried creating a custom exception on the C side of things, but I couldn't figure it out and/or it was unsuccessful at linking

There's MP_DEFINE_EXCEPTION() but that won't work in native modules because it requires embedding a qstr value in the static data.

We would need to either expose all the needed exception functions (like mp_obj_exception_make_new, mp_obj_exception_print), or create and expose a new function to populate a type struct with the right fields for an exception, eg called mp_obj_new_exception_type(mp_obj_full_type_t *type, qstr name).

File opening/interactions.

If you want to do this on the C side you can, just implement the relevant Python calls in C. Eg mp_load_global(MP_QSTR_open), then call open with the filename using mp_call_function_n_kw(...), etc.

I don't think it's necessary to expose any functions like mp_vfs_open() to native modules. Instead, it would be a good idea to make some helper inline functions to open, close, read, write files.

The following bytearray macros would have been nice:

We can definitely add the mp_type_bytearray macro. The other one you suggest is not compatible with the existing signature of the mp_obj_new_bytearray() function.

Conceptual hurdles I had to overcome: using m_malloc and how the GC is aware of the memory if the pointer is stored in the struct allocated by mp_obj_malloc, so it would be good to add an example/documentation of that in the natmod examples.

Yes, it would be good to add an example of this.


Summary: for this PR I would suggest adding the following:

  • expose mp_arg_parse_all to native modules
  • expose mp_load_method_maybe to native modules
  • add a mp_type_bytearray macro
  • add any examples you want to

I'll see if I can add something to help create exception types.

@dpgeorge
Copy link
Member

I'll see if I can add something to help create exception types.

See #14497 for that.

@dpgeorge
Copy link
Member

@BrianPugh would you like me to pick this PR up and make the above changes that I suggested?

@BrianPugh
Copy link
Contributor Author

Yes please! Sorry I haven't gotten to this, I've been super busy moving for the past 2 weeks.

Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (a196468) to head (e253495).

Current head e253495 differs from pull request most recent head df41913

Please upload reports for the commit df41913 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #14335   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20864    20864           
  Misses        340      340           

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

BrianPugh and others added 2 commits May 24, 2024 13:50
Also define `mp_type_bytearray`.  These all help to write native modules.

Signed-off-by: Brian Pugh <bnp117@gmail.com>
Signed-off-by: Damien George <damien@micropython.org>
Python code is no longer needed to implement keyword arguments in
`btree.open()`, it can now be done in C.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge merged commit df41913 into micropython:master May 24, 2024
61 of 62 checks passed
@dpgeorge
Copy link
Member

I made the following changes to this PR:

  • expose mp_arg_parse_all to native modules
  • expose mp_load_method_maybe to native modules
  • add a mp_type_bytearray macro
  • modified the examples/natmod/btree example to use mp_arg_parse_all

And it's now merged.

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

Successfully merging this pull request may close these issues.

None yet

2 participants