lvgl / lv_binding_micropython

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

Consider tracking object validity #177

Closed amirgon closed 2 years ago

amirgon commented 2 years ago

Consider implementing a feature that invalidates the Micropython wrapper object when LVGL object is deleted. Related: https://forum.lvgl.io/t/how-to-delete-lvgl-objects/6770

Invalid object can have a NULL lv_obj. Same C callback can be registered to all objects. We would need to check lv_obj != NULL on every wrapper and throw an exception if false.

jdtsmith commented 2 years ago

Came from the forum. Is perhaps a simpler approach to provide a psuedo-wrapper function like obj.valid() that simply checks if lv_obj != NULL? I.e. provide no active validity checking on function calls, but at least expose to MP whether an object still has an LV object connected inside it.

And it occurs to me... is there a way to accomplish this already? I haven't found it if so.

jdtsmith commented 2 years ago

And to expand, this simple approach would allow your MP code at least to check for and re-build LVGL objects ala:

if not (self.label and self.label.valid()):
    self.label = lv.label(...)

Also, I presume deleting a MP wrapper object (directly or via GC) also releases the underlying LVGL object.

amirgon commented 2 years ago

I.e. provide no active validity checking on function calls, but at least expose to MP whether an object still has an LV object connected inside it.

In fact, the "active validity checking" is relatively cheap (check that a pointer that we reference anyway is NULL).
I would rather add this validity check and throw an exception (which?), instead of expanding the API by adding another member function to each object. Some even consider EAFP approach more "Pythonic".

What might cost more is the event listener that should be added to each object, to detect the "delete event".

Also, I presume deleting a MP wrapper object (directly or via GC) also releases the underlying LVGL object.

No, the MP wrapper is a reference. Deleting it (del x) does not delete the LVGL object, and doesn't even guarantee any resources are freed as we don't really have a "destructor" in MP that is triggered by del. Calling the delete method (x.delete()) would delete the LVGL object.

jdtsmith commented 2 years ago

OK, so a summary is (correct me if mistaken):

Sounds like some kind of (light) MP wrapper delete <--> LVGL resource delete mapping would be useful. For direction B, the idea of throwing an exception (RuntimeError?) could work. For direction A, what about adding a __del__ method on the obj superclass, that simply calls self.delete()?

amirgon commented 2 years ago
  • If the MP wrapper object is deleted, the LVGL object is not freed, and is no longer accessible (potential memory leak).

Not exactly.

  • Direction B: If the LVGL object is deleted with .delete() directly or indirectly, the MP wrapper object is not "informed" and will call into the freed memory, leading to segfaults.

This is true and that's what this GH issue is about.

For direction A, what about adding a __del__ method on the obj superclass, that simply calls self.delete()?

As explain we don't necessarily want to delete the underlying object. It can also have several references (and we don't manage reference counts).
Even if we did, __del__ is not guaranteed to be called on MP when object goes out of scope, for example.

jdtsmith commented 2 years ago

I see. I do agree Direction B is the more pressing issue. And everything is still accessible from the screen as you say. Happy to test any scheme you come up with. Thanks for your work on LV-MP.

amirgon commented 2 years ago

Happy to test any scheme you come up with

Actually, you can already test it. Just use git to checkout the related PR: https://github.com/lvgl/lv_binding_micropython/pull/178

amirgon commented 2 years ago

Fixed by #178. Closing.