stinos / micropython-wrap

API for interop between C/C++ and MicroPython
MIT License
124 stars 23 forks source link

Support for keyword arguments in functions #11

Closed JustinOng closed 2 years ago

JustinOng commented 2 years ago

Hello,

Does this wrapper support keyword arguments in functions as described here?

The closest I've found is optional variables, but that dosen't provide the same level of verbosity when passing arguments.

Thanks!

stinos commented 2 years ago

That is not supported, main reason being that I don't know a way of generating the wrapper automatically, i.e. if a C function is int func(int a, int b) I wouldn't know how to prgrammatically get a and b strings in the call to the registration (Def<funcName>(func)) to generate the MicroPython boilerplate code you show.

It could probably be made to work if the registration would take the keyword names (something like Def<funcName>(func, "a", "b")) but I haven't looked into that. Though I admit I've wanted it a couple of times..

stinos commented 2 years ago

@JustinOng had a look at this and it turned out to be relatively easy, see #12.

JustinOng commented 2 years ago

Thanks for getting back! Does look good, I'll attempt to integrate it with my module sometime next week.

stinos commented 2 years ago

I'll attempt to integrate it with my module

With regards to that: now that MicroPython's user module support matured, in the same PR as mentioned above I've simplified what needs to be done to link micropython-wrap into existing code so that a couple of macros are now sufficient, instead of manually having to write the module definition.

JustinOng commented 2 years ago

Once again, thank you for adding the feature! It works as expected (at least on the unix port), I'll be playing around with this wrapper on the ESP32 and testing further now that it has support for keyword arguments.

stinos commented 2 years ago

If you come across something not listed in the README please let me know, seems like there can be some peculiarities with ESP32's binary layout and perhaps other things.

JustinOng commented 2 years ago

Got it to build against micropython e4d90be and ESP-IDF v4.4.2-378-g9269a536ac (From the V4.4 Docker image).

Couple of notes:

I have to note that I didn't exactly follow your integration guide but instead stuck to the original mpy build guide for building with modules, tweaking the cmake file as necessary.

Noting the presence of configuration flags to select c++17/turn on exceptions, I have to ask if the use of inline above is meant to be ungated.

Defining UPYWRAP_USE_EXCEPTIONS=0 UPYWRAP_THROW_ERROR_CODE=0 turns off exceptions except here https://github.com/stinos/micropython-wrap/blob/5e628e490b30f789f2e37d106ae2aac6dbeed0e1/classwrapper.h#L375, is this also intended?

stinos commented 2 years ago

The use of inline variables means that the IDF has to be configured for gnu++17

Shouldn't that be c++17 instead? Anyway: my mistake, replaced it with an inline function doing the same but simpler.

and RTTI have to be enabled

Do you happen to know why RTTI has to be enabled? unix/windows ports build ok here with no RTTI.

Defining UPYWRAP_USE_EXCEPTIONS=0 UPYWRAP_THROW_ERROR_CODE=0 turns off exceptions except here

Right, fixed; UPYWRAP_THROW_ERROR_CODE will use UPYWRAP_USE_EXCEPTIONS's value, and exception is replaced with a MicroPython one as should have been the case already.

JustinOng commented 2 years ago

Shouldn't that be c++17 instead? Anyway: my mistake, replaced it with an inline function doing the same but simpler.

The warning mentioned using either c++17 or gnu++17, but c++17 results in a compilation error discussed here https://github.com/espressif/esp-idf/issues/5897#issuecomment-697257163. From that discussion I guessed that the ESP-IDF team might be testing against gnu++17 which is why I went there.

Do you happen to know why RTTI has to be enabled? unix/windows ports build ok here with no RTTI.

Building throws this error:

/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h: In static member function 'static void* upywrap::ClassWrapper<T>::AsPyObj(upywrap::ClassWrapper<T>::native_obj_t)':
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h:318:30: error: cannot use 'typeid' with -fno-rtti
       o->typeId = &typeid( T );
                              ^
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h: In static member function 'static upywrap::ClassWrapper<T>* upywrap::ClassWrapper<T>::AsNativeObjCheckedImpl(mp_obj_t)':
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h:355:26: error: cannot use 'typeid' with -fno-rtti
             || typeid( T ) != *native->typeId
                          ^
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h: In static member function 'static void upywrap::ClassWrapper<T>::CheckTypeIsRegistered()':
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h:647:72: error: cannot use 'typeid' with -fno-rtti
         RaiseTypeException( (std::string( "Native type " ) + typeid( T ).name() + " has not been registered").data() );
                                                                        ^
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h: In static member function 'static void upywrap::ClassToPyObj<std::function<_Res(_ArgTypes ...)> >::InitWrapper()':
/workspaces/myproject/firmware/modules/myproject/lib/micropython-wrap/classwrapper.h:1109:45: error: cannot use 'typeid' with -fno-rtti
       static wrapper_t reg( typeid( funct_t ).name(), wrapper_t::ConstructorOptions::RegisterInStaticPyObjectStore );

I'm afraid I'm not familiar with these features of C++, though I can share my ESP32 build environment if necessary.

Can confirm that UPYWRAP_USE_EXCEPTIONS=0 works correctly now.

stinos commented 2 years ago

Building throws this error:

Hmm, interesting, sort of. I didn't notice after disabling RTTI for msvc because there 'no RTTI' means literally what it says, so typeid still works for static compile-time type information i.e. as used here, which isn't really the same as run-time type information (for dynamic_cast etc). And I didn't notice with gcc simply because I put -fno-rtti in the wrong place :(

Anyway: thanks for finding; not all of those are really necessary so I made them conditional. If you can't/don't want to enable RTTI, build with 'UPYWRAP_FULLTYPECHECK=0`.

I can share my ESP32 build environment if necessary

Is that different from the official MicroPython instructions?

JustinOng commented 2 years ago

Is that different from the official MicroPython instructions?

Nope, its just the ESP32 environment. I believe most people working with the ESP32 install it directly onto their host, and I find it much easier to just pull a Docker image and work inside.

stinos commented 2 years ago

Tested this for a while myself, found no issues, so now in main branch.