[v6.0] Callback conventions

[amirgon]

As discussed on #557, #785, #316 and #935, callbacks must have a well defined structure that would allow an automatic script (for [micro]python bindings) find the related user_data of each callback.
A user_data is needed when more than one callback instance can be present for a function callback, and we need to correlate the right callback object of a higher level language with the C callback functions that was just fired.

A convention that we agreed upon was:

• The callback will receive as the first parameter the pointer to the structure that contains the user_data
• The user_data will be named according to the callback name (<callback>_user_data)

I checked and this convention solves some cases, but not all cases.

Here is a list of callbacks which currently do not follow this convention:

void lv_indev_feedback_t(struct _lv_indev_t *, uint8_t)
int32_t lv_anim_path_t(const struct _lv_anim_t *)
void lv_anim_fp_t(void *, int32_t)
void lv_anim_cb_t(void *)
void lv_group_style_mod_func_t(struct _lv_group_t *, lv_style_t *)
void lv_group_focus_cb_t(struct _lv_group_t *)
lv_res_t lv_img_decoder_info_f_t(const void *src, lv_img_header_t *header)
const uint8_t *lv_img_decoder_open_f_t(const void *src, const lv_style_t *style)
lv_res_t lv_img_decoder_read_line_f_t(lv_coord_t x, lv_coord_t y, lv_coord_t len, uint8_t *buf)
void lv_img_decoder_close_f_t(void)
lv_res_t lv_tileview_action_t(lv_obj_t *, lv_coord_t, lv_coord_t)
void lv_spinbox_value_changed_cb_t(lv_obj_t *spinbox, int32_t new_value)
void mem_blend_cb(lv_color_t *dest, const lv_color_t *src, uint32_t length, lv_opa_t opa)
void mem_fill_cb(lv_color_t *dest_buf, const lv_area_t *dest_area, const lv_area_t *fill_area, lv_color_t color)
void task(void *)
const uint8_t *get_bitmap(const struct _lv_font_struct *, uint32_t)
int16_t get_width(const struct _lv_font_struct *, uint32_t)
void cb(lv_obj_t *) // used on lv_obj_animate
void f(lv_log_level_t, const char *, uint32_t, const char *)

I would like to ask your opinion:

• Which functions do you think are important for [micro]python bindings? My opinion is that we should support as much as possible, including images, animations etc.
• Do we need user_data for each of these functions? In some cases (like logging) a single global callback is enough so we don't need a user_data per callback (or we can manage it globally, internally in the binding code)
• Where do you think we can add the missing user_data on each of these callbacks? Can we use the existing convention (and pass a struct as first parameter) or do we need some new convention (like passing the user_data itself to the callback)?

[embeddedt]

I think lv_spinbox_value_changed_cb_t and lv_tileview_action_t should be updated to use the event system.

[embeddedt]

(cc @kisvegabor)

[canardos]

I'm a little late to this party, but I've read up on the prior discussions referenced above.

Apologies in advance for being a dissenting voice, but it feels like there is the potential for a lot of complexity, overhead, and constraints to be added on the LVGL side in order to support automated code generation for bindings and python/micropython in general.

When I see suggestions, such as adding macros to reference count micropython objects, requiring fixed struct member order and naming, adding different user_data depending on the callback, it gives me pause. All of these changes restrict the code, and make future development more difficult and constrained. Some will also increase the resource load.

I just wanted to raise this because while those that are developing the bindings are understandably vocal in their requirements and @kisvegabor has been extremely accommodating in making changes, there are likely a lot of current C users of LVGL that don't follow these issues and are thus silent in these discussions.

There are both less and more resource intensives GUI libraries out there, but LGVL sits in a somewhat unique position, running on MCUs with very limited resources, and is very cleanly structured and documented. We need to remember that the gradual creep of too much complexity and overhead has the potential to push it out of that niche.

Apologies again for the intrusion. This is not the place for an extended discussion, nor would I like to start one. Just wish to remind everyone to remain mindful of these possibilities.

[embeddedt]

@canardos As a C developer myself, I can definitely understand where you're coming from.

adding macros to reference count Micropython objects

This was actually for Python itself, not Micropython. Micropython doesn't need that complexity.

Requiring fixed struct member order

Can you point me to that suggestion? I don't see why that would be necessary for a high-level language binding.

[canardos]

This was actually for Python itself, not Micropython

Got it, but I think the point remains. Regarding the struct ordering, I don't recall exactly where I saw it mentioned. There have been a lot of discussions and it's possible I'm misremembering function/param naming/param order, but as mentioned, I don't really want to get into a discussion around individual changes, just suggest that we remain mindful of striking the right balance between flexibility and added complexity/overhead.

I'll bow out of this discussion here. Keep up the good work all :)

[amirgon]

@canardos I just want to mention that LittlevGL version v5.3 already supports micropython. It required very small modifications to LittlevGL sources in order to add this support, and all these modifications were backward compatible and invisible to the user.

I would like to make the discticntion between Python and MicroPython requirements, I think it's important.

There are both less and more resource intensives GUI libraries out there, but LGVL sits in a somewhat unique position, running on MCUs with very limited resources, and is very cleanly structured and documented. We need to remember that the gradual creep of too much complexity and overhead has the potential to push it out of that niche.

I agree with your point, and still I think that micropython fits LittlevGL well. It is targeted to the same audience (MCUs with very limited resources) as LVGL.

You are right that there was a lot of discussion about adding conventions and changing the API prototypes, but most of them were related to the Python binding, not micropython. Examples are reference counting and special naming conventions to identify who allocates memory (caller or callee), all of which are not needed for micropython.

The only exception, and really the only big feature that is planned for micropython on LVGL v6.0 is the Generic Callbacks support. It would add a lot of flexibility for micropython and we'll make sure that the cost on C is minimal - this is why I opened this issue in the first place!

I would like to identify which callbacks are important for micropython. Those which are not, will not change. For those which are important, I hope the change will be minimal, and will not add penalty in resources or performance. Regarding the C style, if anything, it will make it more clear as we would need to use a clear and consistent convention for callback definitions.

In general, I think user_data is instrumental for callbacks in general, not only in the micropython bindings context. For example, if you ever want to use LVGL with C++ in a proper way and pass over the object pointer to callbacks, user_data is as useful.

[kisvegabor]

@amirgon Thank you for this list. It's extremely useful. I fixed some the issue but some need to be discussed.

void lv_indev_feedback_t(struct _lv_indev_t *, uint8_t)

I moved feedback_cb to the driver, similarly to read_cb.

void mem_blend_cb(lv_color_t *dest, const lv_color_t *src, uint32_t length, lv_opa_t opa)
void mem_fill_cb(lv_color_t *dest_buf, const lv_area_t *dest_area, const lv_area_t *fill_area, lv_color_t color)

I updated them.

int32_t lv_anim_path_t(const struct _lv_anim_t *)

I added user data to lv_anim_t.

void lv_group_style_mod_func_t(struct _lv_group_t *, lv_style_t *)
void lv_group_focus_cb_t(struct _lv_group_t *)

Groups already have user data. What was the problem here?

const uint8_t *get_bitmap(const struct _lv_font_struct *, uint32_t)
int16_t get_width(const struct _lv_font_struct *, uint32_t)

No user data in the fonts right now. The fonts are being reworked and will take care of adding a user data.

void lv_anim_fp_t(void *, int32_t)

It's a generic prototype compatible with the majority of lv_xxx_set_yyy() functions. Should be fine this way.

void lv_anim_cb_t(void *)
void cb(lv_obj_t *) // used on lv_obj_animate

void * means the "thing" being animated. However, it would more versatile to pass lv_anim_t. I updated this way.

lv_res_t lv_img_decoder_info_f_t(const void *src, lv_img_header_t *header)
const uint8_t *lv_img_decoder_open_f_t(const void *src, const lv_style_t *style)
lv_res_t lv_img_decoder_read_line_f_t(lv_coord_t x, lv_coord_t y, lv_coord_t len, uint8_t *buf)
void lv_img_decoder_close_f_t(void)

User data might be used in case of open, read_line and close. I'm sure it's required.

lv_res_t lv_tileview_action_t(lv_obj_t *, lv_coord_t, lv_coord_t)
void lv_spinbox_value_changed_cb_t(lv_obj_t *spinbox, int32_t new_value)

These are obsolete. I deleted them.

void task(void *)

void * is a pointer defined by the user in lv_task_create. It's like a user data. It's a common practice to have void * user_data parameter in task create functions so I think it's fine this way.

void f(lv_log_level_t, const char *, uint32_t, const char *)

A global user data should be fine as discussed earlier.

[kisvegabor]

@canardos I agree with your concerns. Care must be taken to keep the library readable and easy to contribute. Please join the conversation in #935 about the Python binding. It might introduce some inconveniences so any ideas are welcome!

[amirgon]

@kisvegabor

void mem_blend_cb(lv_color_t *dest, const lv_color_t *src, uint32_t length, lv_opa_t opa)
void mem_fill_cb(lv_color_t *dest_buf, const lv_area_t *dest_area, const lv_area_t *fill_area, lv_color_t color)

I updated them.

I can't find mem_blend_user_data and mem_fill_user_data. Did you add them?

int32_t lv_anim_path_t(const struct _lv_anim_t *)

I added user data to lv_anim_t.

What about typedef void (*lv_anim_exec_cb_t)(void *, int32_t); ?

void lv_group_style_mod_func_t(struct _lv_group_t *, lv_style_t *)
void lv_group_focus_cb_t(struct _lv_group_t *)

Groups already have user data. What was the problem here?

These callbacks are defined like this:

typedef struct _lv_group_t
{
    ...
    lv_group_style_mod_func_t style_mod;
    lv_group_style_mod_func_t style_mod_edit; 

Can we name them style_mod_cb and style_mod_edit_cb, as other callbacks?

void task(void *)

void * is a pointer defined by the user in lv_task_create. It's like a user data. It's a common practice to have void * user_data parameter in task create functions so I think it's fine this way.

That's fine, but if we want to support this, we need to define a new convention that would identify these cases.
In which cases can the script assume that a void * passed to a callback function is the user_data?

---

Addition problem:

lv_event_cb_t is used like this:

void lv_obj_set_event_cb(lv_obj_t * obj, lv_event_cb_t cb);

The user_data name is derived from the parameter name cb and not the type lv_event_cb_t.
This is what we were doing until now. For example, when the script looks for the user_data for lv_event_cb_t event_cb; member, it look for event_user_data based on event_cb member name. We can't really use the type name to find the corresponding user_data because same type may appear more than once on the same struct (with different user_data for each), for example, style_mod and style_mod_edit.
The problem in this case is that the parameter name is cb and there is no user_data called cb_user_data...

So I suggest changing the callback function parameter name on functions that receive function pointers, to be consistent with the corresponding user_data name.
The parameter is not named correctly on:

• lv_obj_set_event_cb
• lv_obj_set_signal_cb
• lv_obj_set_design_cb
• lv_group_set_style_mod_cb
• lv_group_set_style_mod_edit_cb

Another option is to change the convention to allow the script find the user_data according to the type name in certain cases, but I think it's less favorable because it's better to keep the callback conventions as simple as possible and not allow multiple "options" that could cause confusion.

[kisvegabor]

@amirgon
All seems doable. I'll fix them later today.

[kisvegabor]

I hope I fixed all the mentioned things. See 514e2ba

[amirgon]

hope I fixed all the mentioned things. See 514e2ba

@kisvegabor Going over the changes, I can see many of these issues are resolved. Thanks!
Remaining open issues are:

• img_decoder callbacks
• lv_anim_exec_cb_t
• get_bitmap/get_width - waiting for reworked fonts
• Task callback convention.
  How can the script know that void * passed to a callback is meant to be used as the user_data itself?
  • We can either pass a pointer to lv_task_t and add user_data to lv_task_t
  • Or we can leave it as is with void* parameter, but extend the user_data convention definition: Name the parameter user_data and define a new convention that a whenever a callback receives void *user_data parameter, it will use it as the user_data.
    This would require naming the parameter user_data in such cases. For example,

    void (*task)(void *user_data);

[kisvegabor]

img_decoder callbacks

I still need to examine it.

lv_anim_exec_cb_t

It should be compatible with LittlevGL API functions so we can't add user data here.
Shall we rename this function to avoid confusion? E.g. exec_cb -> animator_func.

We can either pass a pointer to lv_task_t and add user_data to lv_task_t

I like this approach. With this, the pointer to the created task (lv_task_t * task = lv_task_craete(...)) doesn't have to be stored because it's available in the task itself. I update dev-6.0 with this.

[amirgon]

@kisvegabor

lv_anim_exec_cb_t

It should be compatible with LittlevGL API functions so we can't add user data here.
Shall we rename this function to avoid confusion? E.g. exec_cb -> animator_func.

I'm not so sure how we can support this on Micropython. Could you explain how it is meant to be used?

We can either pass a pointer to lv_task_t and add user_data to lv_task_t

I like this approach. With this, the pointer to the created task (lv_task_t * task = lv_task_craete(...)) doesn't have to be stored because it's available in the task itself. I update dev-6.0 with this.

A few things were not clear to me, I've added comments on 5243d23.

[kisvegabor]

animations
In C it looks like this:

lv_anim_t a;

a.var = obj;
a.start = 10;
a.end = 100;
a.exec_cb = (lv_anim_exec_cb_t)lv_obj_set_y;
a.path_cb = lv_anim_path_linear;
a.ready_cb = NULL;
a.act_time = 0;
a.time = 200;
a.playback = 0;
a.playback_pause = 0;
a.repeat = 0;
a.repeat_pause = 0;
a.user_data = NULL;

lv_anim_create(&a);

However, I don't how it can be used in MicroPython. Shall we improve something here?

task

A few things were not clear to me, I've added comments on 5243d23.

I replayed in the commit.

[amirgon]

However, I don't how it can be used in MicroPython. Shall we improve something here?

@kisvegabor Is there a strict naming convention for those functions that are assigned to exec_cb? Something like lv_obj_set_<var>?

If there is, we can support this in Micropython by providing access to the addresses of each of these functions from Micropython.
I provide two ways for assigning a callback in Micropython:

• One is by providing a python callable object such as a python function. In such case we need a user_data to store the pointer to the micropython callable object.
• The other is by directly providing the address of a C function. This is useful when the callback is already implemented in C and we have access to the callback function pointer in Micropython, for example in case of a display driver that is implemented in C.

What we are missing in this case is a list of pointers to functions that can be assigned to exec_cb.
If we could identify all the relevant functions that could be assigned to exec_cb, we could write something like this in Micropython:

a.exec_cb = lv.obj_set.y

Where obj_set is a class that contains the constant values of the pointers to each of these C functions.

[kisvegabor]

Not only lv_xxx_set_yyy() functions can be used as callback but any custom, user-defined function.

So the two cases:

• Use built-in functions directly: lv_anim_exec_cb_t should have 2 parameters (pointer to something to animate + value). This way it's easy to create animations to change some basic properties.
• Use custom functions: the first parameter of lv_anim_exec_cb_t should have a pointer to lv_anim_t to get the user data.

I don't see a good solution right now. Do you have any ideas?

[amirgon]

I don't see a good solution right now. Do you have any ideas?

@kisvegabor Yes, I think this is solvable.

• Use built-in functions directly: lv_anim_exec_cb_t should have 2 parameters (pointer to something to animate + value). This way it's easy to create animations to change some basic properties.

For the built in functions, their names should obey some naming convention.
For example lv_<obj>_set_<var>(void *, int). Is this the case already?
Could you list all the built-in functions that can be used with exec_cb?
Once the script can identify them, we can let the user assign them to exec_cb in the python script.
In the same way, we would also need a list of built-in functions that can be assigned to path_cb.

Another option, instead of a naming convention, is using an enum in the same manner we solved the Symbols issue. (an enum that would list all functions that could be used with exec_cb and path_cb). The disadvantage of an enum is that it would need to be maintained when built-in functions are added/removed, so I like the naming-convention option better.

• Use custom functions: the first parameter of lv_anim_exec_cb_t should have a pointer to lv_anim_t to get the user data.

We can simply add another inline function like this:

typedef void (*lv_anim_custom_exec_cb_t)(lv_anim_t *, int32_t);

inline void lv_anim_set_custom_exec_cb(lv_anim_t *a, lv_anim_custom_exec_cb_t exec_cb)
{
  a->var = NULL;
  a->exec_cb = (lv_anim_exec_cb_t)exec_cb;
}

In this case, the script will identify exec_user_data as the relevant user_data on lv_anim_t struct, and will update a->exec_user_data before calling lv_anim_set_custom_exec_cb.
(Question: What should be assigned to a->var in this case? I assumed that NULL)

What do you think?

---

Another problem

https://github.com/littlevgl/lvgl/blob/9f216a55be65ec05c477ac55f73a6efad50f7680/src/lv_core/lv_obj.h#L624-L625

lv_obj_animate does not obey the callback convention: It receives a callback ready_cb, and it looks for the user_data on the first function parameter - which is obj.
However, ready_cb is defined on lv_anim_t and not on lv_obj_t so the binding code cannot find it.

[kisvegabor]

Based on your idea with the inline callback we can add functions for all parameters of lv_anim_t. Probably it would worth to group the related ones. For example:

lv_anim_init(&a);  //Set default values

lv_anim_set_var(&a, var);
lv_anim_set_values(&a, start_val, end_val);
lv_anim_set_time(&a, duration, delay);

lv_anim_set_exec_cb(&a, exec_cb);
lv_anim_set_path_cb(&a, path_cb);
lv_anim_set_ready_cb(&a, ready_cb);

lv_anim_set_playback(&a, enable, wait_time);
lv_anim_set_repeat(&a, enable, wait_time);

lv_anim_set_user_data(&a, user_data;

Would it make things easier?

---

lv_obj_animate
I see. I can imagine 2 solutions:

1. Remove ready_cb. I don't like it because ready_cb might be important to synchronize the animations or delete something when it is faded out.
2. Remove the whole lv_obj_animate. I think it's quite limited and can not be used very well. Actually, the currently supported FLOAT and GROW animation can be easily created without lv_obj_animation as well. The above proposed lv_anim_... functions also help here.

[embeddedt]

We can add functions for all parameters of lv_anim_t.

Would they all be static inline functions in a header file, or actual functions in the source files?

Remove ready_cb. I don't like it because ready_cb might be important to synchronize the animations or delete something when it is faded out.

I agree. ready_cb greatly simplifies things when it comes to chaining animations together. We should definitely keep it.

[amirgon]

Would it make things easier?

@kisvegabor It makes sense, although for the binding I only need lv_anim_set_exec_cb, because of the mismatch we have today between lv_anim_t * (where the user_data is defined), and void * var provided to lv_anim_exec_cb_t callback.
That's why I defined above typedef void (*lv_anim_custom_exec_cb_t)(lv_anim_t *, int32_t); which receives lv_anim_t * instead of void *.

To solve the issue we need to handle the exec_cb issue, and make a decision about naming convention for the built-in modifier functions as I explained above.

lv_obj_animate
I see. I can imagine 2 solutions:

Any of these solutions is good for me.

Would they all be static inline functions in a header file, or actual functions in the source files?

@embeddedt they can be static inline functions. The script parses them the same way as any other function. (The problem is only with constructs that are consumed by the preprocessor, like defines and comments)

I agree. ready_cb greatly simplifies things when it comes to chaining animations together. We should definitely keep it.

@embeddedt Do you agree to @kisvegabor's second solution, to remove the whole lv_obj_animate?

[embeddedt]

It's fine to remove lv_obj_animate.

[kisvegabor]

In summary, I will add these lv_anim_... functions and remove lv_obj_animate().

@amirgon

To solve the issue we need to handle the exec_cb issue, and make a decision about naming convention for the built-in modifier functions as I explained above.

Using the user_data the built-in functions, (like lv.obj_set.y) can be used similarly as a custom function, right?

[amirgon]

Using the user_data the built-in functions, (like lv.obj_set.y) can be used similarly as a custom function, right?

@kisvegabor No. If we only have user_data, we can only use custom functions, but not assign a function directly. That's because for function assignment we need to expose the C function pointer in micropython instead of performing an actual call.

That's the distinction I made above between lv.obj.set_y and lv.obj_set.y. The first is a function you can call, the second is a function pointer you can assign.
If we want to support assigning built-in functions in python (like in C), we would need a naming convention for the built-in modifier functions, so we could identify them and expose their function pointers.

If we don't provide a built-in function support, the user would always have to define a custom function to delegate the call. That's why I suggested that we have a naming convention for built-in functions - to support assigning them directly to exec_cb (and path_cb) instead of delegating them.

Example of doing the same thing with/without delegation:

• Using a built in function:

a.exec_cb = lv.obj_set.y

• Using a custom function:

def my_cb(obj, new_y):
    obj.set_y(new_y)

a.exec_cb = my_cb

Another option is to provide a way to extract a C function pointer from every micropython wrapper function. It may be possible, but I'm not sure yet (it may require more memory, for example). This approach is an overkill because we really need to be able to assign only a very small number of functions. Most functions we only call, not assign.

[kisvegabor]

Got it, thank you.

That's the distinction I made above between lv.obj.set_y and lv.obj_set.y. The first is a function you can call, the second is a function pointer you can assign.

In general the "lv_" + <obj> + "_set_" <something> convention can work. For simplicity and clearance what if we make it mandatory to use custom functions for animations? Would it be too inconvenient?

[amirgon]

So there are no direct assignments for struct fields by the user, only via functions, right?

@kisvegabor actually on lv_font_t we've got get_glyph_dsc and get_glyph_bitmap, on disp drv we have flush_cb and probably there are other cases where the user needs to update the callback directly on the struct.
And that's fine, the binding supports this as long as conventions are followed, and now that's the case in most cases.
Even if we add a setter function for each of the cases the convention is not followed, I think we should keep the possibility to update the callback directly on the struct for future flexibility.

So - in the rare cases where the convention is not followed, for example in case of exec_cb field of lv_anim_t struct, do you think we can find a way to mark it? (either on the struct itself or on the callback prototype, not only on lv_anim_set_exec_cb as you suggested)

[kisvegabor]

actually on lv_font_t we've got get_glyph_dsc and get_glyph_bitmap, on disp drv we have flush_cb and probably there are other cases where the user needs to update the callback directly on the struct.

True. However in these cases, e.g. lv_anim_t a.exec_cb = my_func, it's the user responsibility to set an exec_cb function with the correct type.
So marking the parameter name in lv_anim_set_exec_cb and lv_event_send_func should work, right? Here really only prototype should be marked because there are valid use cases of lv_anim_exec_cb_t and lv_event_cb_t. In contrast theme style mod is invalid in all cases so here we can mark the names in the struct directly.

So in summary

1. Mark the parameter name in the API if the callback is valid generally, but that API function is tricky somehow (lv_event_send_func)
2. Mark the variable name in the struct if the callback is in a "wrong place", e.g. theme style_mod_cb.

What do you think?

[amirgon]

@kisvegabor

True. However in these cases, e.g. lv_anim_t a.exec_cb = my_func, it's the user responsibility to set an exec_cb function with the correct type.

But the binding needs to know if it's ok or not that callback convention is not followed.
When the callback is a struct field (such as exec_cb or flush_cb) the binding script identifies this as a callback and checks if the callback conventions are followed.
In this case, it recognizes that flush_cb follows the convention, but exec_cb doesn't.
Now we need to tell it that it's ok that exec_cb doesn't follow the convention, but flush_cb should still follow it.

What about adding the naming convention in the callback prototype itself? Something like:

typedef void (*lv_anim_exec_cb_t)(void *bad_cb, lv_anim_value_t);

(you can change bad_cb to any other naming convention, that marks that it's not an error that this callback cannot be supported by the binding)

The advantage of adding the convention in the callback prototype is that the same prototype is used in both cases - when registering a callback with a function call and when just assigning a function pointer on a struct.

[kisvegabor]

I see. In light of that, it should work:

• anim_exec_cb: use bad_cb in the typedef because it's an "invalid" type
• lv_event_send_func: mark the paramter name because this function is an exception, lv_event_cb_t is still valid
• theme-group: mark the theme->group_style_mod_bad_cb becasue in this context the style mod functions are invalid.

Do you agree?

[amirgon]

@kisvegabor I wish there was a simpler "single" way to represent this, instead of 3 different ways depending on the situation.
But if there isn't, this is probably good enough.

[kisvegabor]

@amirgon
I also think that these solutions are good enough as they are not really visible for the user.

I added the updates here bceb46b

How is it working?

[amirgon]

@kisvegabor I haven't looked at it until now, but what about lv_fs callbacks? I'm not sure they follow the callback conventions, and I don't remember if we discussed this.

[kisvegabor]

I'm not sure they follow the callback conventions, and I don't remember if we discussed this.

They are not following the convention now but it seems it's easy to fix it. I'll do it soon.

How about the previous updates? Are they working?

[amirgon]

How about the previous updates? Are they working?

@kisvegabor I didn't have the chance to check it yet. It requires some changes in the binding script and I'm working on something else right now.

I'll check that later and let you know.

[kisvegabor]

@amirgon
I update the fs callbacks: 760359f

[amirgon]

I update the fs callbacks: 760359f

@kisvegabor We would also need user_data on lv_fs_drv_t for the callback convention.

[kisvegabor]

Done: 51878e6

[amirgon]

Done: 51878e6

Thanks!

[kisvegabor]

Me too!
I've still added _cb suffix to the callbacks: abae15a#diff-8be2b3447383f1eac6acdcba2b919c01

[amirgon]

@kisvegabor There is still some problems with marking exclusions (as you added in bceb46b)

• lv_task_create receives lv_task_cb_t task_cb, which does not follow the convention but is not marked with xcb
• lv_event_send_func receives lv_event_cb_t event_cb does not follow the convention but is not marked with xcb

The others seems ok.

[kisvegabor]

I fixed them here d84e977

[amirgon]

@kisvegabor
I've discovered a new problem, related to lvgl/lv_binding_micropython#25.

https://github.com/littlevgl/lvgl/blob/ca1f21ad04ed3da251369a199dd6ec619c35c881/src/lv_objx/lv_list.c#L177
https://github.com/littlevgl/lvgl/blob/ca1f21ad04ed3da251369a199dd6ec619c35c881/src/lv_objx/lv_list.c#L195

lv_list_add receives event_cb and registers it on liste list.

The problem is that Micropython binding assumes that every event registration also sets user_data to the Micropython callable object.
(Actually this is not entirely accurate. The user_data points to a dictionary of callable objects, since a single lvgl object/struct can have multiple callbacks)

In this case, an event is registered, but the associated user_data is not set. When the callback is called, Micropython binding cannot find the callable object in user_data.

The problem is not limited to lv_list.
I searched for calls to lv_obj_set_event_cb and found this:

./src/lv_objx/lv_list.c:    lv_obj_set_event_cb(liste, event_cb);
./src/lv_objx/lv_tabview.c:        lv_obj_set_event_cb(ext->btns, tab_btnm_event_cb);
./src/lv_objx/lv_tileview.c:        lv_obj_set_event_cb(ext->page.scrl, tileview_scrl_event_cb);
./src/lv_objx/lv_win.c:    lv_obj_set_event_cb(btn, event_cb);
./src/lv_objx/lv_kb.c:        lv_obj_set_event_cb(new_kb, lv_kb_def_event_cb);
./src/lv_objx/lv_mbox.c:        lv_obj_set_event_cb(new_mbox, lv_mbox_default_event_cb);
./src/lv_objx/lv_page.c:        lv_obj_set_event_cb(ext->scrl, scrl_def_event_cb); /*Propagate some event to the background

---

A possible solution is to set user_data of the internal object to the user_data of the external object.
It solves the problem because the binding will first set the callable object on the user_data of the external object, and the callback itself will receive the user_data of the internal object, so we just need to make sure it's the same user_data.

For example, in the lv_list_add case:

    lv_obj_set_event_cb(liste, event_cb);
    lv_obj_set_user_data(liste, list->user_data);

Is this a valid solution? Can we apply this to all the cases above?

[amirgon]

A possible solution is to set user_data of the internal object to the user_data of the external object.

Thinking about this again... there is a problem with my suggestion:
If one "external" object contains multiple "internal" objects, my suggestion would cause all the internal objects to have the same user_data. This means their micropython callbacks would overrun each other and only the last callback registered will be invoked for all internal objects.

So we probably need to think about some other solution to this problem.

[kisvegabor]

The problem is not limited to lv_list.

Does really every element of the list is problematic? Most of them seems valid to me. If lv_list_add is a special case we can figure out a workaround for it.

[amirgon]

Does really every element of the list is problematic? Most of them seems valid to me. If lv_list_add is a special case we can figure out a workaround for it.

@kisvegabor You are right, it's problematic only when the callback is delegated from the user.
If the callback is internal to lvgl, it's not a problem.

So going over the list I think that we have a problem at:

• lv_list_add
• lv_win_add_btn

In both these cases, the callback is given as a parameter, and then passed on to lv_obj_set_event_cb.

But I noticed that in both those cases, the delegating function (lv_list_add, lv_win_add_btn) is returning the internal object, so a workaround could be to set the event on the returned object.
something like:

btn_close = list1.add(lv.SYMBOL.CLOSE, "Close", None)
btn_close.set_event_cb(my_close_callback);

How about requiring the user to always do it like this? just remove the callback parameter from the delegating function.

[kisvegabor]

I created the "complex" functions because usually the only thing you want to set for a list element or window control button is its event function. I'd like to keep these functions at least in C.

I think it's the same case than lv_task_create and lv_task_create_basic.

However lv_list_add_basic would look ugly because lv_list_add is not that complex either.

So IMO the solution would be to have two functions but still don't know how to name them.

@embeddedt @amirgon
Do you have any a good name idea for lv_list_add_with_event and lv_list_add_without_event?

[embeddedt]

I think it's better to just remove the callback parameter. It's not that much extra typing to type lv_obj_set_event_cb immediately below. Plus, it's more consistent with other objects.

[amirgon]

I think it's better to just remove the callback parameter. It's not that much extra typing to type lv_obj_set_event_cb immediately below. Plus, it's more consistent with other objects.

@kisvegabor, I agree with @embeddedt .
If, however, you decide to have both functions, please add the "xcb" marker to the with_event versions, because we would like to prevent Micropython users from using the with_event versions.

[kisvegabor]

I think it's better to just remove the callback parameter. It's not that much extra typing to type lv_obj_set_event_cb immediately below. Plus, it's more consistent with other objects.

I also don't have a better idea, in lack of good names. So I removed the event parameters. I also reneamed lv_list_add to lv_list_add_btn for consistency.

[kisvegabor]

@amirgon
I there anything else with the callbacks?

[amirgon]

@amirgon
I there anything else with the callbacks?

@kisvegabor At the moment, I don't see any additional problems! 😄
I'm closing for now. Thank you for the efforts!

[kisvegabor]

Awesome! Hopefully, no API breaking issues will turn out.