Skip to content

RFC: Mechanism for allowing dynamic native modules to call port-specific functionality. #5682

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
wants to merge 2 commits into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Feb 21, 2020

This is an alternative to #5653 by @tve

The main benefit of this approach is that it's extremely simple and requires no modifications to the linker/mpy machinery. Maintaining the table is obvious and straightforward and a logical extension of the existing mp_fun_table.

Main downside is that it calls to port-specific functions involve an indirection but I'm not sure this is too serious.

I'm not too fond of "portnativeglue.{c,h}" but mostly this is a PoC for discussion.

Includes a modification to natmod/features0 to demonstrate using it on ESP32.

This allows them to be used by dynamic native modules.
For example, to allow access to underlying HAL/RTOS functionality (e.g. ESP32 IDF).
@tve
Copy link
Contributor

tve commented Feb 21, 2020

Nice!
How is this different from the first version in my PR? Other than omitting the indirect jump table, which I created for two reasons: (1) so one could write esp_clk_cpu_freq() without having to explicitly go through a mp_ports_fun_table and more importantly, so (2) one wouldn't have to import all the .h files and redefine all the ESP-IDF functions in your portnativeglue.h (you conveniently picked a sample function that doesn't have any special argument types or returns types, which many (most?) esp-idf functions have).

I'm currently adding a generator to produce an espidf module so the functions can be called directly from python. Arguably that can work with any of the three approaches.

As far as I can tell:

  • your approach has the least machinery and forces all functions to be defined with full signature in portnativeglue and requires all call to have the form mp_port_fun_table.<function>.
  • my initial approach has a bit more machinery so a function name list is sufficient in the equivalent of portnativeglue and the calls can be made straight using the esp-idf function name.
  • my second approach has the dynamic linking stuff which fixes all the call sites cleanly and doesn't require type definitions.

I find the last approach the cleanest overall, but I don't have a strong preference, I just want to be able to move on and write device drivers in python...

@miketeachman
Copy link
Contributor

miketeachman commented Feb 21, 2020

I'm currently adding a generator to produce an espidf module so the functions can be called directly from python. Arguably that can work with any of the three approaches.

@tve If I understand your ambitions, it's worth looking at the littlevgl micropython port. @amirgon created a way to bind ESP-IDF API functions to MicroPython. The binding generator is written in Python and is called from the MicroPython build. The result is a new espidf module in MicroPython. Very cool.
Check out:
https://blog.littlevgl.com/2019-08-05/micropython-pure-display-driver#auto-generated-c-api

refer to the section that reads:

"Preprocess espidf.h by the C preprocessor. This applies all nested “includes” and produces a single preprocessed file with the entire API we want to generate.

Run gen_mpy.py on the preprocessed file. This is where the magic happens, and C functions and structs are transformed into a Micropython module. The output of this step is mp_espidf.c, which is the implementation of the ESP-IDF micropython module."

I just want to be able to move on and write device drivers in python...

that's exactly the purpose of his work.

@tve
Copy link
Contributor

tve commented Feb 21, 2020

@miketeachman thanks for the pointer, very interesting! Also very complex! I have to take a closer look at what gets generated. It's not clear to me yet what the desired end-point is. I'd like to have more flexibility writing drivers outside the MP firmware and also about how much code is in python vs. C.
One issue with the notion of an espidf python module is that the moment you load the module you get all the code. If we're talking 10 functions then that's a don't-care. But when that becomes stubs for hundreds of functions I'm not sure I like that anymore.

@amirgon
Copy link
Contributor

amirgon commented Feb 21, 2020

One issue with the notion of an espidf python module is that the moment you load the module you get all the code. If we're talking 10 functions then that's a don't-care. But when that becomes stubs for hundreds of functions I'm not sure I like that anymore.

@tve This is configurable when building Micropython.
You don't have to pull in the entire esp-idf as a python module. You can take only part of it, up to granularity of a single H file.
Another option is to write some C functions that wrap only the functionality you are interested in, and convert that into Micropython module with the script.

@tve
Copy link
Contributor

tve commented Feb 21, 2020

Maintaining the table is obvious and straightforward and a logical extension of the existing mp_fun_table.

I'm not so sure I fully agree here. You put a single function into your demo table that conveniently does not use any esp-idf type so you didn't need to hunt down any include file. By the time you include functions for a few devices you'll (a) have to change the makefile for the dynamic module to include all the esp-idf include tree, (b) add esp-idf include files into portnativeglue.h, (c) ifdef around any esp-idf v3/v4 incompatibilities in portnativeglue.h, (d) live with the fact that compiling your native module #includes a lot of esp-idf 'cause of portnativeglue.h. Each time you want to add a function to mp_port_tab you also have to hunt down its signature definition in the esp-idf include tree, you can't just go from the manual.

The above issues aren't deal breakers, but IMHO they do erode the "simpler" claim and they're the reason I decided to try the jump table, which makes the glue non-typed.

@tve
Copy link
Contributor

tve commented Feb 21, 2020

@armirgon: you are correct, I'll have to try your tool and see how it ends up working in practice.

@tve
Copy link
Contributor

tve commented Feb 21, 2020

@amirgon well, after over an hour of hacking around to get mpy_gen.py working on a cut down espidf.h I finally realized that the code generated only works for a built-in module, not for a dynamically loaded module...
I did an example where I only include driver/pcnt.h and I must say that the amount of generated code is rather mind-boggling! (I also got all of gpio in there, probably because pcnt includes that...)

@tve
Copy link
Contributor

tve commented Feb 21, 2020

@amirgon you write:

This is configurable when building Micropython.
You don't have to pull in the entire esp-idf as a python module. You can take only part of it, up to granularity of a single H file.
Another option is to write some C functions that wrap only the functionality you are interested in, and convert that into Micropython module with the script.

I'm not sure I buy this, at least given the current script. I did a test just including "driver/pcnt.h" after removing a #include <driver/gpio.h> from that file. The result was ~1900 lines of generated C code and the global table below (to give an idea of the scope of stuff generated). No matter how I measure this it's "a lot of stuff" IMHO.

STATIC const mp_rom_map_elem_t pcnt_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_pcnt) },

    { MP_ROM_QSTR(MP_QSTR_esp_get_idf_version), MP_ROM_PTR(&mp_esp_get_idf_version_obj) },
    { MP_ROM_QSTR(MP_QSTR_get_ccount), MP_ROM_PTR(&mp_get_ccount_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_err_to_name), MP_ROM_PTR(&mp_esp_err_to_name_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_err_to_name_r), MP_ROM_PTR(&mp_esp_err_to_name_r_obj) },
    { MP_ROM_QSTR(MP_QSTR__esp_error_check_failed), MP_ROM_PTR(&mp__esp_error_check_failed_obj) },
    { MP_ROM_QSTR(MP_QSTR__esp_error_check_failed_without_abort), MP_ROM_PTR(&mp__esp_error_check_fa
iled_without_abort_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_mark_shared), MP_ROM_PTR(&mp_esp_intr_mark_shared_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_reserve), MP_ROM_PTR(&mp_esp_intr_reserve_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_alloc), MP_ROM_PTR(&mp_esp_intr_alloc_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_alloc_intrstatus), MP_ROM_PTR(&mp_esp_intr_alloc_intrstatus_obj)
},
    { MP_ROM_QSTR(MP_QSTR_esp_intr_free), MP_ROM_PTR(&mp_esp_intr_free_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_get_cpu), MP_ROM_PTR(&mp_esp_intr_get_cpu_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_get_intno), MP_ROM_PTR(&mp_esp_intr_get_intno_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_disable), MP_ROM_PTR(&mp_esp_intr_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_enable), MP_ROM_PTR(&mp_esp_intr_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_set_in_iram), MP_ROM_PTR(&mp_esp_intr_set_in_iram_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_noniram_disable), MP_ROM_PTR(&mp_esp_intr_noniram_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_intr_noniram_enable), MP_ROM_PTR(&mp_esp_intr_noniram_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_unit_config), MP_ROM_PTR(&mp_pcnt_unit_config_obj) },
    { MP_ROM_QSTR(MP_QSTR_get_counter_value), MP_ROM_PTR(&mp_pcnt_get_counter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_counter_pause), MP_ROM_PTR(&mp_pcnt_counter_pause_obj) },
    { MP_ROM_QSTR(MP_QSTR_counter_resume), MP_ROM_PTR(&mp_pcnt_counter_resume_obj) },
    { MP_ROM_QSTR(MP_QSTR_counter_clear), MP_ROM_PTR(&mp_pcnt_counter_clear_obj) },
    { MP_ROM_QSTR(MP_QSTR_intr_enable), MP_ROM_PTR(&mp_pcnt_intr_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_intr_disable), MP_ROM_PTR(&mp_pcnt_intr_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_event_enable), MP_ROM_PTR(&mp_pcnt_event_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_event_disable), MP_ROM_PTR(&mp_pcnt_event_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_set_event_value), MP_ROM_PTR(&mp_pcnt_set_event_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_get_event_value), MP_ROM_PTR(&mp_pcnt_get_event_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_isr_register), MP_ROM_PTR(&mp_pcnt_isr_register_obj) },
    { MP_ROM_QSTR(MP_QSTR_set_pin), MP_ROM_PTR(&mp_pcnt_set_pin_obj) },
    { MP_ROM_QSTR(MP_QSTR_filter_enable), MP_ROM_PTR(&mp_pcnt_filter_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_filter_disable), MP_ROM_PTR(&mp_pcnt_filter_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_set_filter_value), MP_ROM_PTR(&mp_pcnt_set_filter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_get_filter_value), MP_ROM_PTR(&mp_pcnt_get_filter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_set_mode), MP_ROM_PTR(&mp_pcnt_set_mode_obj) },
    { MP_ROM_QSTR(MP_QSTR_isr_handler_add), MP_ROM_PTR(&mp_pcnt_isr_handler_add_obj) },
    { MP_ROM_QSTR(MP_QSTR_isr_service_install), MP_ROM_PTR(&mp_pcnt_isr_service_install_obj) },
    { MP_ROM_QSTR(MP_QSTR_isr_service_uninstall), MP_ROM_PTR(&mp_pcnt_isr_service_uninstall_obj) },
    { MP_ROM_QSTR(MP_QSTR_isr_handler_remove), MP_ROM_PTR(&mp_pcnt_isr_handler_remove_obj) },
    { MP_ROM_QSTR(MP_QSTR_task_delay_ms), MP_ROM_PTR(&mp_task_delay_ms_obj) },
    { MP_ROM_QSTR(MP_QSTR_get_ccount), MP_ROM_PTR(&mp_get_ccount_obj) },
    { MP_ROM_QSTR(MP_QSTR_task_delay_ms), MP_ROM_PTR(&mp_task_delay_ms_obj) },

    { MP_ROM_QSTR(MP_QSTR_MODE), MP_ROM_PTR(&mp_PCNT_MODE_type) },
    { MP_ROM_QSTR(MP_QSTR_COUNT), MP_ROM_PTR(&mp_PCNT_COUNT_type) },
    { MP_ROM_QSTR(MP_QSTR_UNIT), MP_ROM_PTR(&mp_PCNT_UNIT_type) },
    { MP_ROM_QSTR(MP_QSTR_CHANNEL), MP_ROM_PTR(&mp_PCNT_CHANNEL_type) },
    { MP_ROM_QSTR(MP_QSTR_EVT), MP_ROM_PTR(&mp_PCNT_EVT_type) },

    { MP_ROM_QSTR(MP_QSTR_config_t), MP_ROM_PTR(&mp_pcnt_config_t_type) },
    { MP_ROM_QSTR(MP_QSTR_C_Pointer), MP_ROM_PTR(&mp_C_Pointer_type) },
};

@amirgon
Copy link
Contributor

amirgon commented Feb 21, 2020

@tve

I finally realized that the code generated only works for a built-in module, not for a dynamically loaded module...

Yes, I was trying to optimize RAM on expense of Flash. On microcontrollers such as ESP32 there is plenty of Flash that can be mapped to the MCU's address space, so you can place there all program and constants. In such case, the modules generated by mpy_gen.py consume no RAM at all, while dynamically loaded modules wastes RAM, AFAIK.

I did an example where I only include driver/pcnt.h and I must say that the amount of generated code is rather mind-boggling! (I also got all of gpio in there, probably because pcnt includes that...)

The trick is to prevent nested includes being parsed.
The H file you are interested in usually includes some other H files you are not interested in. To work around this you can define the include guards conditionally, such that when the script is parsing the H file to generate the module, nested includes will not be considered. That's a hack, but it works well.

As an example, here is an espidf.h that includes driver/pcnt.h and excludes all it's nested includes by defining their include guards:

/**
 * This file defines the Micorpython API to ESP-IDF
 * It is used as input to gen_mpy.py to create a micropython module
 **/ 
#if __has_include("esp_idf_version.h")
#   include "esp_idf_version.h"
#endif

// Disable some macros and includes that make pycparser choke

#ifdef PYCPARSER
#define __attribute__(x)

// FreeRTOS portmaco is excluded, but we still need TickType_t

#include <stdint.h>
typedef uint32_t TickType_t;

// Micropython specific types

typedef void *mp_obj_t;

// Prevent pcnt nested includes 

#define __ESP_INTR_H__
#define INC_FREERTOS_H
#define SEMAPHORE_H
#define __XTENSA_API_H__
#define _ESP32_SOC_H_
#define _SOC_PCNT_REG_H_
#define _SOC_PCNT_STRUCT_H_
#define _SOC_GPIO_SIG_MAP_H_
#define _DRIVER_GPIO_H_
#define __ESP_INTR_ALLOC_H__

// pcnt.h needs intr_handle_t which is defined in the exluded nested includes.
// Instead, define it here:

typedef void *intr_handle_t;

#else // PYCPARSER

#endif //PYCPARSER

// The following includes are the source of the esp-idf micropython module.
// All included files are API we want to include in the module

#include "driver/pcnt.h"

Looking at the resulting mp_espif.c, the global table now contains only pcnt functions, structs and enums:

/*
 * espidf module definitions
 */

STATIC const mp_rom_map_elem_t espidf_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_espidf) },
    
    { MP_ROM_QSTR(MP_QSTR_esp_err_to_name), MP_ROM_PTR(&mp_esp_err_to_name_obj) },
    { MP_ROM_QSTR(MP_QSTR_esp_err_to_name_r), MP_ROM_PTR(&mp_esp_err_to_name_r_obj) },
    { MP_ROM_QSTR(MP_QSTR__esp_error_check_failed), MP_ROM_PTR(&mp__esp_error_check_failed_obj) },
    { MP_ROM_QSTR(MP_QSTR__esp_error_check_failed_without_abort), MP_ROM_PTR(&mp__esp_error_check_failed_without_abort_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_unit_config), MP_ROM_PTR(&mp_pcnt_unit_config_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_get_counter_value), MP_ROM_PTR(&mp_pcnt_get_counter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_counter_pause), MP_ROM_PTR(&mp_pcnt_counter_pause_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_counter_resume), MP_ROM_PTR(&mp_pcnt_counter_resume_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_counter_clear), MP_ROM_PTR(&mp_pcnt_counter_clear_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_intr_enable), MP_ROM_PTR(&mp_pcnt_intr_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_intr_disable), MP_ROM_PTR(&mp_pcnt_intr_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_event_enable), MP_ROM_PTR(&mp_pcnt_event_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_event_disable), MP_ROM_PTR(&mp_pcnt_event_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_set_event_value), MP_ROM_PTR(&mp_pcnt_set_event_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_get_event_value), MP_ROM_PTR(&mp_pcnt_get_event_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_isr_register), MP_ROM_PTR(&mp_pcnt_isr_register_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_set_pin), MP_ROM_PTR(&mp_pcnt_set_pin_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_filter_enable), MP_ROM_PTR(&mp_pcnt_filter_enable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_filter_disable), MP_ROM_PTR(&mp_pcnt_filter_disable_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_set_filter_value), MP_ROM_PTR(&mp_pcnt_set_filter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_get_filter_value), MP_ROM_PTR(&mp_pcnt_get_filter_value_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_set_mode), MP_ROM_PTR(&mp_pcnt_set_mode_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_isr_handler_add), MP_ROM_PTR(&mp_pcnt_isr_handler_add_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_isr_service_install), MP_ROM_PTR(&mp_pcnt_isr_service_install_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_isr_service_uninstall), MP_ROM_PTR(&mp_pcnt_isr_service_uninstall_obj) },
    { MP_ROM_QSTR(MP_QSTR_pcnt_isr_handler_remove), MP_ROM_PTR(&mp_pcnt_isr_handler_remove_obj) },
    
    { MP_ROM_QSTR(MP_QSTR_PCNT_MODE), MP_ROM_PTR(&mp_PCNT_MODE_type) },
    { MP_ROM_QSTR(MP_QSTR_PCNT_COUNT), MP_ROM_PTR(&mp_PCNT_COUNT_type) },
    { MP_ROM_QSTR(MP_QSTR_PCNT_UNIT), MP_ROM_PTR(&mp_PCNT_UNIT_type) },
    { MP_ROM_QSTR(MP_QSTR_PCNT_CHANNEL), MP_ROM_PTR(&mp_PCNT_CHANNEL_type) },
    { MP_ROM_QSTR(MP_QSTR_PCNT_EVT), MP_ROM_PTR(&mp_PCNT_EVT_type) },
    
    { MP_ROM_QSTR(MP_QSTR_pcnt_config_t), MP_ROM_PTR(&mp_pcnt_config_t_type) },
    { MP_ROM_QSTR(MP_QSTR_C_Pointer), MP_ROM_PTR(&mp_C_Pointer_type) },  
};

STATIC MP_DEFINE_CONST_DICT (
    mp_module_espidf_globals,
    espidf_globals_table
);

const mp_obj_module_t mp_module_espidf = {
    .base = { &mp_type_module },
    .globals = (mp_obj_dict_t*)&mp_module_espidf_globals
};

If this is not concise enough for you (or if you don't like using the include guard trick), another option (as I mentioned earlier) is to create some wrapper functions only around the functionality you are interested in and process these wrapper functions by the the script.

@tve
Copy link
Contributor

tve commented Feb 21, 2020

Thanks for the tip, I'll try that and see whether I can hack the codegen to support dyn loaded modules. I understand what you were trying to do, but here we're trying to enable external drivers...
BTW, would you be open to separating gen_mpy out into its own repo?

@amirgon
Copy link
Contributor

amirgon commented Feb 21, 2020

@tve

BTW, would you be open to separating gen_mpy out into its own repo?

gen_mpy could come from a git sub module within lv_binding_micropython, so yes I'm open to that.

But so far there was no demand and the only project that uses it is lvgl, so let's first see if some other project needs it.

but here we're trying to enable external drivers...

Theoretically you could load an external mpy to flash on runtime and execute it from there. I think it's important especially for large modules where we don't want to waste a lot of RAM.
I've already brought this up here: #5083 (comment).

@tve
Copy link
Contributor

tve commented Feb 22, 2020

A speed bump I just ran into is that I called strlen() in a native module and it looks like dynruntime.mk doesn't statically link that in... Does that mean that dynruntime.mk should be changed to perform that linking or should the stdlib entry points go into mp_port_fun_table? If the latter, what does that mean about the 3 approaches (how would those calls be changed to mp_port_fun_table.strlen())?

@tve
Copy link
Contributor

tve commented Feb 22, 2020

@amirgon FYI, I tried to add support for generating dynamic modules to gen_mpy but gave up for now... too many things I don't understand.

@amirgon
Copy link
Contributor

amirgon commented Feb 22, 2020

I tried to add support for generating dynamic modules to gen_mpy but gave up for now...

@tve - There is something I don't understand.
Is your goal to dynamically load esp-idf itself, or only the drivers that use it?
Isn't the entire esp-idf too big to fit in RAM as a dynamically loaded module?

If you only want to dynamically load the drivers, and you assume that esp-idf itself is statically linked, what is then your motivation to dynamically load the python wrappers for esp-idf? Why not have them statically linked together with esp-idf?

@tve
Copy link
Contributor

tve commented Feb 23, 2020

I tried to add support for generating dynamic modules to gen_mpy but gave up for now...

@tve - There is something I don't understand.
Is your goal to dynamically load esp-idf itself, or only the drivers that use it?
Isn't the entire esp-idf too big to fit in RAM as a dynamically loaded module?

If you only want to dynamically load the drivers, and you assume that esp-idf itself is statically linked, what is then your motivation to dynamically load the python wrappers for esp-idf? Why not have them statically linked together with esp-idf?

Thanks for asking! :-) I wish there was a place to have these discussions other than hijacking someone else's PR and that the master who decides everything in the end would chime in, but here we are...

My goal is to free up writing drivers and other add-ons to the esp32 port (and other ports) outside of the MP repo. As far as I can tell, a majority of PRs for the esp32 only have to be built into the firmware image because of the linking issue. Take my machine.Counter PR #5216 as an example: it only has to be in the firmware 'cause of linking against esp-idf. But it's stuck in a "I'm not sure how many other people want this" state. I don't need to make it part of MP, I just need to link against esp-idf. A number of other PRs are stuck in a similar state, e.g. "I know it's not general/portable/to-your-liking but it works for me" or "I did the best I can" or "no-one has time to really review and merge this", etc.

So what I'm hoping we can get done is to add a linking method, such that when someone comes around and produces a PR to access functions X, Y, and Z then that just adds a couple of words of flash and is a no-brainer to merge. Ideally everything else they need to do stays outside of MP.

Now comes the question whether this "everything else" benefits from minimal stubs to make the esp-idf functions callable from python directly. You have more experience than I do, but it seems worthwhile 'cause a lot of stuff can be written in python as far as I can tell so far. I wrote a poor-man's stub generator, you wrote the cadillac version. Either way, if my app has 2-3 drivers that need access to a handful of ESP-IDF functions each, then either stub generator can be used to produce the necessary stubs whether they end up in RAM or Flash. I don't think it makes much of a difference (I assume the app itself is much larger than 20-odd stubs).

I gather that you like the idea of generating stubs for "all of esp-idf" once and for all, and bake those into the MP firmware. That probably would be nice. The concerns I have are: (1) what does that actually look like size-wise, (2) how do you even define and construct "all of esp-idf" (may be best to parse the docs and collect the set of functions described there?), (3) who signs up to maintain that (which now includes gen-mpy.py), and (4) how do you get that merged? Note that "concerns" != "opposition"!!!

WRT Flash vs. RAM my opinion would be to work on allowing dynamically loaded modules to be executed from flash. Damien has indicated that he's open to that. I'm not sure what exactly would be involved but it doesn't seem that difficult.

Overall, I'm not trying to push my solution. I'm not even sure I know yet what my solution would be! All I know is that I would much like a common solution that allows all of us to publish device drivers and other cool libraries that use ESP-IDF in a way that lets others upip install them without having to build their own version of the firmware. I can easily maintain my own fork of MP, but that's akin to taking my marbles and playing alone in my corner...

@stinos
Copy link
Contributor

stinos commented Feb 23, 2020

So what I'm hoping we can get done is to add a linking method, such that when someone comes around and produces a PR to access functions X, Y, and Z then that just adds a couple of words of flash and is a no-brainer to merge. Ideally everything else they need to do stays outside of MP

Sorry to hijack this thread, but I keep on wondering: ideally the part of having to explicitly allow access to X/Y/Z gets skipped as well, no? Which should be possible somehow because after all X/Y/Z are already linked into the frimware so the only problem is getting their address out at runtime and passing that to the module. I don't have a CS degree but what I'm describing here is essentially what dynamic loading with ELF format does, I think. And I've done that (loading .so files as modules) on the unix port and it all just works. So can someone enlighten me: why is it not a viable solution of attempting like that instead of having a static table? Is it just because of the code size required to implement or is there more? Too hard/too much work perhaps?

@amirgon
Copy link
Contributor

amirgon commented Feb 23, 2020

why is it not a viable solution of attempting like that instead of having a static table?

Perhaps the missing FFI support for ESP32?

@tve
Copy link
Contributor

tve commented Feb 23, 2020

why is it not a viable solution of attempting like that instead of having a static table?

Fundamentally somewhere somehow a symbol name to address lookup needs to be made so the dynamically loaded module can be fixed-up with the correct addresses. There are 3 options that are relevant here:

  1. The symbol->address table is on your laptop, when you generate the mpy for a dynamic module it is fixed up before copying it to the MP system. Pro: uses no space on the MP system. Con: a fixed-up module can only work with a specific firmware version as any firmware change may shift addresses; also, if you have many boards in the field you need to keep track of the table for each one.
  2. The symbol->address table is on the MP system in flash, when you load a dyn module is gets fixed-up (similar to the way it's currently done). Pro: the correct table is on the correct MP system so an mpy can be loaded onto any board. Con: space, the table has the text of each symbol and its address.
  3. There is a symbol->numeric_index table on your laptop and a numeric_index->address table on the MP system in flash. When you generate an mpy the symbol names are translated to an index, and when the mpy is dynamically loaded the resident table is used to convert the indexes to addresses. The numeric indexes are kept constant across firmware builds/versions. This is what the mp_fun_table and mp_port_fun_table do. Pros: minimal table on the board, portability of mpy across boards. Con: need to manage what's in that table.

The difference between this PR and my PR is whether the dynamic module gets fixed-up the way most dynamic linkers work by patching the code, or whether the code in the dynamic module does the look-up explicitly. So both PRs assume option 3.

Something worth exploring: what if we put substantially all external esp-idf symbols into the mp_port_fun_table? Or all symbols occurring in the docs?

@stinos
Copy link
Contributor

stinos commented Feb 24, 2020

Thanks for the explanation. So looks like main reason for not having full-blown loading (2) is code size right? Would there be a way to use (2) but at build-time have a way of telling 'don't create complete table but only put these symbols in it'? Makes code size less of a problem for those who want it. But in essence it's the same problem as getting all functions for (3) just the other way around: (3) needs a custom way to tell 'I want X/Y/Z' and (2) needs a custom way to tell 'I do not want X/Y/Z'. But I guess (2) makes it easier to just have everything work by default if code size doesn't matter.

@tve
Copy link
Contributor

tve commented Feb 24, 2020

@stinos the issue with (2) is that the table is in the firmware and most users (I assume) install the released firmware so there's no opportunity for them to say "I do not want X/Y/Z".

@stinos
Copy link
Contributor

stinos commented Feb 24, 2020

Unless it's not built int the firmware but as a bunch of separate files which get loaded on demand. Which might actually reduce code size. But has some management overhead. And might not be possible on all ports, not sure.

@tve
Copy link
Contributor

tve commented Feb 29, 2020

I tried to generate an mp_port_fun_table for all the ESP-IDF functions that are described in the docs. Getting the list of function names isn't that difficult, it turns out. However... drumroll... if I create a table that references all of them then the code for all the functions gets linked into the executable and that causes xtensa-esp32-elf-ld: region 'iram0_0_seg' overflowed by 4984 bytes. I first thought "huh, why is mp_port_fun_table going into iram???", but it's not that, it's all the ESP-IDF stuff that the table references. :-(

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2020

Part of the discussion here is about the distinction between a static espidf module that exposes all IDF functions to Python, versus the ability to dynamically load and link at run-time against any IDF C function/symbol. They both have their pros/cons. Eg an espidf module means you can just write in Python, but if there is a lot of call to make to the IDF then converting objects to/from C (done by the stubs that wrap the IDF) will make it very inefficient. There are also things you can't do from Python, like install an IRQ handler (which needs a pointer to a C function with the right signature).

In the end, even if an espidf module is added to access the IDF from Python, I think exposing IDF symbols to dynamic native modules is still going to be wanted/needed.

@jimmo
Copy link
Member Author

jimmo commented Mar 3, 2020

Closing in favour #5711, in particular with @dpgeorge's suggestion to have the port_fun_table alongside, rather than nested in, the mp_fun_table.

The goal of this PR was to avoid modifying mpy_ld.py at all, but if we're going to do that, then that approach makes more sense.

@jimmo jimmo closed this Mar 3, 2020
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.

6 participants