Skip to content

expose more types from obj.h to native modules #5643

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

Conversation

jeremyherbert
Copy link
Contributor

@jeremyherbert jeremyherbert commented Feb 14, 2020

Basically just a copy-paste of all of the types in obj.h to allow access in native modules.

Related to #5609

@jeremyherbert jeremyherbert force-pushed the add_type_to_natmod branch 2 times, most recently from 03a65e1 to eee30c1 Compare February 14, 2020 03:50
@jeremyherbert
Copy link
Contributor Author

The only travis error is due to a slight increase in code size (needed for the unsupported type indicator).

@dpgeorge
Copy link
Member

I'm not sure this is the right approach, to just add everything. Each addition to the table adds a word-sized-pointer to the firmware. Ok, that's not too much for some ports, but for ports that are really tight on flash (eg 256k) it's best to keep this list at a minimum. Ie minimum entries in the table for maximum capabilities.

Some are probably never going to be needed. I would rather add them only if they are shown to be useful.

One option could be to provide "full" and "reduced" table options, selected at build time of the main firmware. Large ports could enable the full table if needed. Then when a .mpy file is built it would set a flag if it needed the full table or not, and the import would fail if the full table was not available. But I'd still rather not go down this path, to keep .mpy file importing compatible across all ports.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Feb 14, 2020
@jeremyherbert
Copy link
Contributor Author

That’s fair. For the CBOR module I am developing I probably only need 1/3 of these, but some more functions need to be exposed too.

What do you think is the best way to handle types which may or may not be present in some other ports? For example, a high performance FFT library would probably need a complex number type to be available, but as-is not all ports have it.

@dpgeorge
Copy link
Member

For the CBOR module I am developing I probably only need 1/3 of these, but some more functions need to be exposed too.

I think the best thing to do here is build the CBOR module, add those that are needed, then I can review them alongside their use in CBOR (if it's open source...). I really want to be careful adding things here because the native table is a very public ABI and will basically be set in stone once something is added.

What do you think is the best way to handle types which may or may not be present in some other ports?

You can see in py/nativeglue.c how set/slice/float are handled when they are unsupported on a given platform.

For example, a high performance FFT library would probably need a complex number type to be available, but as-is not all ports have it.

In this case the C code would do the low-level transform and some Python code would do the "parsing" of the inputs, eg complex numbers. For an FFT I'd guess that array.array('d') would be used, and that doesn't require knowledge of the complex type at all at the C level.

@dpgeorge
Copy link
Member

With native modules the aim is not to write everything in C (and allow anything to be written in C). But rather to use C only when needed, and in a simple way. Complicated handling of Python objects/types should be done in Python.

@jeremyherbert
Copy link
Contributor Author

Ok, will proceed based on this. CBOR module will be open source (MIT), it’s just not finished yet. Ideally I’d like to have it in the interpreter itself (not as a native module) because to me it feels like it should be there alongside ujson, but I can see the argument against this in terms of flash usage.

I will update this PR soon.

@jeremyherbert jeremyherbert changed the title expose all types from obj.h to native modules expose more types from obj.h to native modules Feb 14, 2020
@jeremyherbert jeremyherbert force-pushed the add_type_to_natmod branch 4 times, most recently from fc9ec45 to 71a6944 Compare February 14, 2020 11:13
@jeremyherbert jeremyherbert mentioned this pull request Feb 15, 2020
@v923z
Copy link
Contributor

v923z commented Feb 19, 2020

For example, a high performance FFT library would probably need a complex number type to be available, but as-is not all ports have it.

@jeremyherbert This is actually not true: you can calculate the real and imaginary parts of the transform without any reference to complexes. https://github.com/v923z/micropython-ulab/blob/master/code/fft.c

@jeremyherbert
Copy link
Contributor Author

@v923z yep, I meant for returning the processed data (like np.complex128)

@v923z
Copy link
Contributor

v923z commented Feb 19, 2020

@v923z yep, I meant for returning the processed data (like np.complex128)

@jeremyherbert But this has nothing to do with the performance, it is a question about how you represent the results. If you really need it, I could return a list of complexes at the end of the calculation, but then what do you do with it?

@jeremyherbert
Copy link
Contributor Author

Yes, returning the results is what I am referring to. If there is a complex type and you are returning a complex number, wouldn’t it make more sense to the end user to have a complex number come out? That’s what numpy does.

But none of this is related to performance, it’s more ease of use and doing what the user expects. The “high performance” I was referring to is in the calculation of the FFT itself, not arranging the buffers to return the data in a specific data type.

@stinos
Copy link
Contributor

stinos commented Feb 19, 2020

One option could be to provide "full" and "reduced" table options, selected at build time of the main firmware ... But I'd still rather not go down this path, to keep .mpy file importing compatible across all ports.

On one hand I get this, on the other hand: native modules are a really nice way to extend MicroPython but not having the complete API available means that you're back to using USER_C_MODULES or similar from the moment you need anything which isn't in the table. And as raised in #5654 both aren't interchangeable due to the way the module's globals dict is populated, so that means people have to make a choice between the 2 at one point and going back and forth between the 2 ways isn't super trivial. Not sure how big of a problem that is, so I think waiting it out for a bit and see what people are asking for is a viable strategy.

@jeremyherbert
Copy link
Contributor Author

Yes, this is a tricky problem.

I’m interested to see how this goes: #5618

@dpgeorge
Copy link
Member

And as raised in #5654 both aren't interchangeable due to the way the module's globals dict is populated, so that means people have to make a choice between the 2 at one point and going back and forth between the 2 ways isn't super trivial.

Yes there's a difference (RAM vs ROM) for the globals dict. But I made a few of the extmod modules compile as either static or dynamic using the MICROPY_ENABLE_DYNRUNTIME option. So it is possible to maintain both versions together. And, if the need is there, we could possible make it so the dynamic module also has a static globals dict (might be worth the effort).

native modules are a really nice way to extend MicroPython but not having the complete API available means that you're back to using USER_C_MODULES or similar

So far my main use of native modules has been on bare metal stm32 where the complete API of the hardware is just the peripheral registers, which are available to dynamic native modules (you can even include the CMSIS header files to get all the register definitions).

But on a target like esp32, or a PC system, the API is going to be functions. And to make dynamic native modules truly useful there maybe it is worth exposing all functions. On esp32 a count of symbols in application.map shows there are 8167 of them, so that's a 32k table of pointers in the firmware.

An alternative way of doing it is to link against known offsets: for a given port (say esp32) the build would produce a file listing the address of all functions, and mpy_ld.py would read this file in when it builds the mpy output file. That mpy would then only work on that exact firmware.

@tve
Copy link
Contributor

tve commented Feb 20, 2020

My vote would be to keep the current mp_fun_table, dynruntime and nativeglue approach focused on interfacing to portable MP functions; and to use something along the lines of #5653 to link against platform-specific functions on the platforms that need it.

@jeremyherbert
Copy link
Contributor Author

Perhaps it is best to take this discussion to one of the open issues? #5654 looks good.

I don't plan to change this PR to add any of the extra functionality, I just wanted to expose some core types (which are portable anyway). So it would probably be better located somewhere else for archival purposes.

@dpgeorge
Copy link
Member

I don't plan to change this PR to add any of the extra functionality, I just wanted to expose some core types (which are portable anyway).

Yes, +1 on that. The only issue is that it requires bumping the .mpy file version, which is not a real problem, I just wanted it to stay the same for at least one version :)

@jeremyherbert
Copy link
Contributor Author

yeah no biggie, this is a breaking change after all so slow and steady is probably the right way to go

@dpgeorge
Copy link
Member

this is a breaking change after all

Adding #define's to dynruntime.h is not a breaking change, so they can be added without question.

@jeremyherbert
Copy link
Contributor Author

yes, but changing mp_fun_table to include the extra types is a problem, right?

@dpgeorge
Copy link
Member

I merged a small part of this PR: the two macros to create a new dict, and store to a dict. See 148d122

Then in a follow up commit 0e556f2 I added the types (except float, which isn't always available) using a mechanism that doesn't require adding entries to the native glue table.

@dpgeorge dpgeorge closed this Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants