lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
237 stars 156 forks source link

gen_mpy.py small issues and potential fixes #279

Open chunis opened 1 year ago

chunis commented 1 year ago

Thanks for the great work. I'm using the gen_mpy.py script and it works amazing! Still I think I found a few small issues, and figured out some workarounds but I'm not sure they're the right solution, as below.

1. bool type support seems don't work

Given below head file:

#include <stdbool.h>

bool revert_bool(bool b);

The produced code shows:

/*
 * Function NOT generated:
 * Missing conversion from _Bool
 * _Bool revert_bool(_Bool b)
 */

For workaround, I added the map to the dictionaries as:

--- a/gen_mpy.py
+++ b/gen_mpy.py
@@ -485,6 +485,7 @@ mp_to_lv = {
     'const uint8_t *'           : 'mp_to_ptr',
     'const void *'              : 'mp_to_ptr',
     'bool'                      : 'mp_obj_is_true',
+    '_Bool'                     : 'mp_obj_is_true',
     'char *'                    : '(char*)convert_from_str',
     'char **'                   : 'mp_write_ptr_C_Pointer',
     'const char *'              : 'convert_from_str',
@@ -524,6 +525,7 @@ lv_to_mp = {
     'const uint8_t *'           : 'ptr_to_mp',
     'const void *'              : 'ptr_to_mp',
     'bool'                      : 'convert_to_bool',
+    '_Bool'                     : 'convert_to_bool',
     'char *'                    : 'convert_to_str',
     'char **'                   : 'mp_read_ptr_C_Pointer',
     'const char *'              : 'convert_to_str',
@@ -563,6 +565,7 @@ lv_mp_type = {
     'const uint8_t *'           : 'void*',
     'const void *'              : 'void*',
     'bool'                      : 'bool',
+    '_Bool'                     : 'bool',
     'char *'                    : 'char*',
     'char **'                   : 'char**',
     'const char *'              : 'char*',

2. uint32_t overflow

I've a function for setting frequency has a argument with type uint32_t. When I pass it with 2400000000, it fails with "OverflowError: overflow converting long int to machine word". The value is 0x8f0d1800, I guess it is converted to int32_t first by (uint32_t)mp_obj_get_int, and causes this overflow. It's too late to convert it back to uint32_t later.

My solution:

--- a/gen_mpy.py
+++ b/gen_mpy.py
@@ -493,7 +493,7 @@ mp_to_lv = {
     '%s_obj_t *'% module_prefix : 'mp_to_lv',
     'uint8_t'                   : '(uint8_t)mp_obj_get_int',
     'uint16_t'                  : '(uint16_t)mp_obj_get_int',
-    'uint32_t'                  : '(uint32_t)mp_obj_get_int',
+    'uint32_t'                  : '(uint32_t)mp_obj_get_ull',
     'uint64_t'                  : '(uint64_t)mp_obj_get_ull',
     'unsigned'                  : '(unsigned)mp_obj_get_int',
     'unsigned int'              : '(unsigned int)mp_obj_get_int',

3. typedef issue

Given below code:

typedef struct _info {
    int age;
    int sum;
} my_info;

typedef my_info info_x;
typedef my_info info_y;

void check_info(info_x *info);

//void do_nothing(my_info *info);

The produced code won't contain the definition of my_info, unless un-comment the do_nothing( ) line. My understanding is: unless my_info is used in function declaration, it will be ignored, even it's been used as the source of typedef. This doesn't feel right to me, but I don't know how to fix it besides of the extra do_nothing() declaration or define struct info_x and struct info_y directly.

4. v1.20 compile issue

I also checked the v1.20 branch, but the script produced code compiled giving below error:

error: incompatible types when initializing type 'const mp_obj_type_t *' {aka 'const struct _mp_obj_type_t *'} using type 'mp_obj_full_type_t' {aka 'const struct _mp_obj_full_type_t'}
amirgon commented 1 year ago

Hi @chunis thank you for these suggestions!

I'm glad to hear that you find gen_mpy.py useful.
On which libraries do you use it?

+    '_Bool'                     : 'mp_obj_is_true',
-    'uint32_t'                  : '(uint32_t)mp_obj_get_int',
+    'uint32_t'                  : '(uint32_t)mp_obj_get_ull',

Both your suggestions about _Bool and mp_obj_get_ull sound good to me!

The produced code won't contain the definition of my_info, unless un-comment the do_nothing( ) line. My understanding is: unless my_info is used in function declaration, it will be ignored, even it's been used as the source of typedef.

That's correct.
Micropython binding for a struct is only generated if the struct is "used" - that means it's sent or received as a function argument or a return value.
The rational is that struct binding is expensive in terms of program memory. Sometimes structs are "private" and have lots of members but aren't really needed in the API so it's a waste to bind them.

Could you explain the use case for a struct that appears in the API but is never "used" in any function?
You can think of a function that receives void* and expect the user to cast, but that would usually not be a clean API design.

I also checked the v1.20 branch, but the script produced code compiled giving below error:

Until now I only tested v1.20 with LVGL API and it works there (you can see the unix tests are passing), but of course there could be issues with other headers.

Could you show a short reproducible example of an H file that causes this?

chunis commented 1 year ago

Hi, @amirgon Thanks for your quick response, and glad my fixes are useful.

The library I work on is the SX128x driver: https://github.com/Lora-net/SWSD001/tree/master/lora_basics_modem/lora_basics_modem/smtc_modem_core/radio_drivers/sx128x_driver.

The problem for the 3rd item is as below code shows:

typedef struct sx128x_pkt_status_gfsk_flrc_ble_s
{
    int8_t                     rssi;    //!< FSK/FLRC/BLE packet RSSI (dBm)
    sx128x_pkt_status_errors_t errors;  //!< FSK/FLRC/BLE packet error bit mask
    sx128x_pkt_status_t        status;  //!< FSK/FLRC/BLE packet status bit mask
    sx128x_pkt_status_sync_t   sync;    //!< FSK/FLRC/BLE packet sync bit mask
} sx128x_pkt_status_gfsk_flrc_ble_t;

typedef sx128x_pkt_status_gfsk_flrc_ble_t sx128x_pkt_status_gfsk_t;
typedef sx128x_pkt_status_gfsk_flrc_ble_t sx128x_pkt_status_flrc_t;
typedef sx128x_pkt_status_gfsk_flrc_ble_t sx128x_pkt_status_ble_t;

sx128x_status_t sx128x_get_gfsk_pkt_status( const void* context, sx128x_pkt_status_gfsk_t* pkt_status );
sx128x_status_t sx128x_get_flrc_pkt_status( const void* context, sx128x_pkt_status_flrc_t* pkt_status );
sx128x_status_t sx128x_get_ble_pkt_status( const void* context, sx128x_pkt_status_ble_t* pkt_status );

The API doesn't use the struct sx128x_pkt_status_gfsk_flrc_ble_t directly, but this struct is used to typedef another 3 types which all be used in the API. But the original struct is skipped in the produced code causing the derived 3 types all be undefined.

I understand that unused struct should be ignored; but looks like the typedef is not handled correctly for this case.

For the v1.20 compile issue:

Could you show a short reproducible example of an H file that causes this?

This seems happen with the enum type. I can reproduce it with below files (all files put under folder micropython/lora/sx128x):

sx128x_status_t sx128x_wakeup( const void* context );


- file `sx128x.c`:
```c
#include "sx128x.h"

sx128x_status_t sx128x_wakeup( const void* context )
{   
    return SX128X_STATUS_OK;
} 

SRC_USERMOD += $(EXAMPLE_MOD_DIR)/sx128x.c SRC_USERMOD += $(EXAMPLE_MOD_DIR)/sx128x_wrapper.c

CFLAGS_USERMOD += -I$(EXAMPLE_MOD_DIR) CEXAMPLE_MOD_DIR := $(USERMOD_DIR)


Produce the C code with command:
```shell
./gen_mpy-v1.20.py -M sx1280 sx128x.h > sx128x_wrapper.c

Then go to folder micropython/ports/unix, run:

make USER_C_MODULES=../../lora

will produce error as:

CC ../../lorb/sx128x/sx128x_wrapper.c
../../lorb/sx128x/sx128x_wrapper.c:1065:20: error: incompatible types when initializing type ‘const mp_obj_type_t *’ {aka ‘const struct _mp_obj_type_t *’} using type ‘mp_obj_full_type_t’ {aka ‘struct _mp_obj_full_type_t’}
 1065 |     .mp_obj_type = mp_lv_SX128X_STATUS_type_base,
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
amirgon commented 1 year ago

I understand that unused struct should be ignored; but looks like the typedef is not handled correctly for this case.

Right. If a struct is used indirectly through typedef, it should be generated. I've opened a new issue to track this: https://github.com/lvgl/lv_binding_micropython/issues/281

This seems happen with the enum type. I can reproduce it with below files (all files put under folder micropython/lora/sx128x):

Thank you for the example. I will look into it.

chunis commented 1 year ago

@amirgon I checked the latest script (merged from branch v1.20), and the enum type compatibility issue disappeared, and the compile process goes all right. Thanks.