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

gen_mpy.py small issues and potential fixes #279

Open
chunis opened this issue Jun 13, 2023 · 4 comments
Open

gen_mpy.py small issues and potential fixes #279

chunis opened this issue Jun 13, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@chunis
Copy link

chunis commented Jun 13, 2023

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
Copy link
Collaborator

amirgon commented Jun 13, 2023

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
Copy link
Author

chunis commented Jun 14, 2023

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):

  • file sx128x.h:
typedef enum sx128x_status_e
{
    SX128X_STATUS_OK = 0,
    SX128X_STATUS_UNSUPPORTED_FEATURE,
    SX128X_STATUS_UNKNOWN_VALUE,
    SX128X_STATUS_ERROR,
} sx128x_status_t;

sx128x_status_t sx128x_wakeup( const void* context );
  • file sx128x.c:
#include "sx128x.h"

sx128x_status_t sx128x_wakeup( const void* context )
{   
    return SX128X_STATUS_OK;
} 
  • file micropython.mk:
EXAMPLE_MOD_DIR := $(USERMOD_DIR)

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:

./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
Copy link
Collaborator

amirgon commented Jun 14, 2023

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: #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
Copy link
Author

chunis commented Jun 20, 2023

@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.

@PGNetHun PGNetHun added the bug Something isn't working label Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants