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

Integration as external c module with unmodified micropython. #242

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

andrewleech
Copy link

This PR provides changes that allow lv_binding_micropython to be used as an External C Module with un-modified / upstream micropython.

This takes advantage of the relativly new macro MP_REGISTER_ROOT_POINTER to remove the need to manually include the LV roots in the mpconfigport.h files.

Other port-specific settings can be added to micropython.mk and micropython.cmake

An example of usage to build a project can be found here: https://github.com/andrewleech/lvgl_mpy_example

Note: I've currently only tested on stm32 and unix - I have not yet built for esp32 or any other cmake based port.

It also includes an update to the binding generator script to support the new types syntax in current upstream micropython, for details see: https://github.com/micropython/micropython/wiki/Build-Troubleshooting#define_const_obj_type

@amirgon
Copy link
Collaborator

amirgon commented Nov 15, 2022

Thank you @andrewleech for this contribution!
It would be very convenient to use lv_binding_micropython as an external module.

I would like to adapt this change in lv_micropython.
We use lv_micropython for GitHub workflows in lv_binding_micropython and in LVGL. lv_micropython is also used directly in many projects. It contains an important patch that was not accepted yet in Micropython (for very long time) and is critical due to the size of LVGL library API.
For that reason (and future patches like this) I think it's best to keep lv_micropython as a fork of Micropython and not use Micropython directly like you did.

I'm trying to keep lv_micropython aligned to official Micropython releases. Latest release v1.19 does not yet contain these breaking API changes that you fixed (DEFINE_CONST_OBJ_TYPE), so options are:

  • Wait for next Micropython release that includes these changes. I'm not sure when next release is expected, Micropython is not very clear about release schedule.
  • Update lv_micropython to current Micropython master. I know Micropython master branch is relatively stable but I'm still not comfortable with this. For clarity it's best to align to well known releases instead of a random commit.

@andrewleech
Copy link
Author

Hi @amirgon , ah yes I'd been watching that PR and it certainly looks like a worthy change, hopefully it can be ready for merge soon! I see there's a few failing unit tests there, that on its own would be enough to block merge :-( perhaps adding a #define MICROPY_PY_* setting to enable/disable the new functionality would be enough to get it across the line, disabling it where it fails (for whatever reason) and/or on smaller ports.

But in the mean time yes there's no harm in working from a fork of upstream (with minimal change) I often do this at work when I'm using new features I'm writing!

I suspect a new realease of Micropython may be imminent, so it's probably fine to hold off with this until that's released - there's probably a few things to clean up with the PR anyway.

Most notably; I'm failing to get esp32 to build. I've fixed the way root pointers are handled and updated the cmake scripts enough to support the rp2 build but am getting tripped up on the esp32 one. It's giving cmake failures like:

$ cd lvgl_mpy_example
$ make esp
...
-- Configuring done
CMake Error at /opt/esp/idf/tools/cmake/project.cmake:428 (add_executable):
  Cannot find source file:

    /code/lib/micropython/ports/esp32/build-GENERIC/lv_mp.c

however adding some status messages shows that the lv_bindings() function is being run correctly with the output file /code/lib/micropython/ports/esp32/build-GENERIC/lv_mp.c but that's apparently not enough to satisft cmake. Perhaps it's got something to do with https://stackoverflow.com/a/57825938 but I'm not that familiar with cmake to follow it all.

@M-D-777
Copy link

M-D-777 commented Mar 25, 2023

Hi, @andrewleech Any updates on this Topic?
I have tested https://github.com/andrewleech/lvgl_mpy_example for esp32, but failed.
Someone here had the same problem, https://forum.lvgl.io/t/build-lv-bindings-with-micropython-nightly-for-esp32/10967
Looking forward to your update,Thanks.

CMake Error at /project/micropython/py/usermod.cmake:9 (get_target_property):
  get_target_property() called with non-existent target "lvgl_interface".
Call Stack (most recent call first):
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:47 (usermod_gather_sources)
  main/CMakeLists.txt:14 (include)


CMake Error at /project/micropython/py/usermod.cmake:15 (get_target_property):
  get_target_property() called with non-existent target "lvgl_interface".
Call Stack (most recent call first):
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:47 (usermod_gather_sources)
  main/CMakeLists.txt:14 (include)


CMake Error at /project/micropython/py/usermod.cmake:21 (get_target_property):
  get_target_property() called with non-existent target "lvgl_interface".
Call Stack (most recent call first):
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:24 (usermod_gather_sources)
  /project/micropython/py/usermod.cmake:47 (usermod_gather_sources)
  main/CMakeLists.txt:14 (include)


Found User C Module(s): usermod_lv_bindings, lvgl_interface
CMake Error at /project/esp-idf/tools/cmake/component.cmake:470 (add_library):
  add_library cannot create target "__idf_main" because another target with
  the same name already exists.  The existing target is a static library
  created in source directory
  "/project/micropython/ports/esp32/main".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  main/CMakeLists.txt:158 (idf_component_register)

@andrewleech
Copy link
Author

I'm pretty sure I got rp2 working, but esp cmake is weirdly different to rp2 and I never managed to make it build.

I generally really dislike esp. I've never had a good experience with them, always crashing / Guru Meditation Error anytime I try to do anything interesting. I'm also not a huge cmake fan, things there just seem even harder to get working than Makefile, though maybe that's just me.

As such I'm really not motivated to get esp working... I'm always going to choose rp2 or stm for my projects.

But as for the MR, well micropython v1.20 hasn't been released yet (there's some upstream rp2 ble issues getting in the way) but once it does I'll probably try to get this rebased and ready to go... for some platforms at least!

@amirgon
Copy link
Collaborator

amirgon commented Jun 2, 2023

@andrewleech I'm starting to look at this in the context of #272.

Why did you choose to register root pointers with MP_STATE_VM(lvgl) = m_new0(lvgl_root_pointers_t, 1); instead of calling the more standard MP_REGISTER_ROOT_POINTER?

I imagined calling the LV_ITERATE_ROOTS in some init function, in order to call MP_REGISTER_ROOT_POINTER for each global. What do you think?

Also, instead of tweaking lv_init in gen_mpy.py, a cleaner solution could be to call it explicitly from lv_init in LVGL.

My plan is to first align to Micropython v1.20.0 without an external C module.
Once this is ready we could try rebasing your PR. That means the alignment to the new types syntax and to the new way of registering root pointers need to be done first on the other PR I'm working on. Once it's ready, your PR could focus specifically on the integration as external C module.

@andrewleech
Copy link
Author

andrewleech commented Jun 2, 2023

Sounds great @amirgon !

I'm pretty sure when I first did this, MP_REGISTER_ROOT_POINTER did not yet exist, it came later. It's definitely the right thing to use here.

I'm not sure I follow your other comments about init, but I was aiming to not change the current end user usage of lvgl with my PR, trying to ensure any init things were called automatically at an appropriate time.

Having a seperate / manual init function might make sense, certainly of there's times you might want to reuse it after soft reset etc. As long as things don't segfault if it's accidentally not run beforehand.

Working on that PR was the first time I used lvgl at all though so I was figuring out a lot as I went through it, probably did some things wrong!

@amirgon
Copy link
Collaborator

amirgon commented Jun 18, 2023

@andrewleech - lv_binding_micropython is now aligned to Micropython v1.20.0, see #272, lvgl/lv_micropython#65

Would you like to rebase this PR?
I understand you got this working for unix/stm32/rp2 ports, right?
Do you have any plans for esp32?

@andrewleech
Copy link
Author

Nice work! Yep I'll rebase mine when I get a chance.

I did have Unix/stm tested and working... though stm was quite unstable for me.

That's probably something wrong with the driver I wrote for my board.
Unix was also not working very well for me due to the known nvidia driver issue iirc.
I don't think I actually tested rp2 due to lack of hardware.
I tried and tried to get esp32 to work but gave up, the idf cmake trickery got the better of me.

@dmazzella
Copy link

@andrewleech @amirgon progress on this? need help to complete some tasks?

@andrewleech
Copy link
Author

No I haven't rebased / reworked it for the updated micropython yet; too many projects / too little time.

I never got it working with esp either.

I'm definitely keen to still see this updated and working!

@PGNetHun PGNetHun added the enhancement New feature or request label Jan 6, 2024
@Carglglz
Copy link

Carglglz commented Apr 25, 2024

I'm definitely keen to still see this updated and working!

@andrewleech
Thanks to this PR I got it working with latest MicroPython v1.23.0-preview-324-gd11ca092f7 in unix and esp32 (this one was tricky) too , I will try to open a PR this week in case you want to review it 👍🏼

Here are the changes https://github.com/Carglglz/lv_binding_micropython/tree/userCmod

@MathyV
Copy link

MathyV commented Apr 30, 2024

@Carglglz I tried your code (on esp32) and have some issues with it:

  • In micropython.cmake -Wno-unused-function is added to COMPILE_DEFINITIONS which introduces a spurious -D in front of the gcc command which (on my system at least) fails to build. Since it is not a definition, it should be added to COMPILE_OPTIONS. Here's a patch:
diff --git a/micropython.cmake b/micropython.cmake
index 5343ef0..2ff55da 100644
--- a/micropython.cmake
+++ b/micropython.cmake
@@ -7,8 +7,9 @@ idf_build_set_property(LV_MICROPYTHON 1)
 idf_build_component(${CMAKE_CURRENT_LIST_DIR}/lvgl)
 idf_build_set_property(COMPILE_DEFINITIONS "-DLV_KCONFIG_IGNORE" APPEND)
 separate_arguments(LV_CFLAGS_ENV UNIX_COMMAND $ENV{LV_CFLAGS})
-list(APPEND LV_CFLAGS ${LV_CFLAGS_ENV} -Wno-unused-function)
+list(APPEND LV_CFLAGS ${LV_CFLAGS_ENV})
 idf_build_set_property(COMPILE_DEFINITIONS "${LV_CFLAGS}" APPEND)
+idf_build_set_property(COMPILE_OPTIONS "-Wno-unused-function" APPEND)
 idf_build_set_property(SRCS "${LV_SRC}" APPEND)
 idf_build_set_property(INCLUDE_DIRS "${LV_INCLUDE}" APPEND)
  • I notice you commented a lot of stuff out in the mkrules_usermod.cmake file which prevents the esp32 drivers from building. How did you test on esp32 and with which driver? Everything builds and links fine with your code and I can import the lvgl modules but I haven't immediately found a driver that works for my (st7789) display in combination with LVGL.

@Carglglz
Copy link

Carglglz commented Apr 30, 2024

@MathyV thanks for testing this!

In micropython.cmake -Wno-unused-function is added to COMPILE_DEFINITIONS which introduces a spurious -D in front of the gcc command which (on my system at least) fails to build. Since it is not a definition, it should be added to COMPILE_OPTIONS

That makes sense, my knowledge in Cmake / esp-idf is limited, so I'm not 100 % sure that everything in micropython.cmake is right.

I notice you commented a lot of stuff out in the mkrules_usermod.cmake file which prevents the esp32 drivers from building. How did you test on esp32 and with which driver? Everything builds and links fine with your code and I can import the lvgl modules but I haven't immediately found a driver that works for my (st7789) display in combination with LVGL.

Yes I tried to implement those drivers in the build too, but then I realised that they were written with IDF-v4.x and now MicroPython is at IDF-v5.x. So they need to be updated first, which right now it is out of my scope.

In the meantime I've been looking at the drivers at https://github.com/kdschlosser/lvgl_micropython and https://github.com/bdbarnett/mpdisplay, which seem promising.

Let me know if you get it working.

I'm mostly developing/testing on unix port, on a esp32 I've only tested this with a "dummy" display, i.e. just writing the buffers and it seems to work... see https://docs.lvgl.io/master/porting/display.html#

@Carglglz
Copy link

Carglglz commented May 9, 2024

I confirm this is working on esp32 with the drivers at https://github.com/bdbarnett/mpdisplay, in any case I think the drivers shouId be independent from the bindings so people can choose/implement its own...
I will try to make a PR soon 👍🏼

Carglglz added a commit to Carglglz/lvgl that referenced this pull request May 12, 2024
Allow using `MICROPY_MALLOC_USES_ALLOCATED_SIZE` option
required to build `lv_bindings_micropython` with upstream
MicroPython.

Required for lvgl/lv_binding_micropython#242
@kdschlosser
Copy link
Contributor

OK so the only thing you are wanting to do is provide LVGL as a user C module. The user would be responsible for writing and implementing their own drivers for the bus and for the display IC and proper handling of flushing the display buffer.

I was expecting the functionality to be the same as it is right now.

@Carglglz
Copy link

Carglglz commented May 22, 2024

OK so the only thing you are wanting to do is provide LVGL as a user C module. The user would be responsible for writing and implementing their own drivers for the bus and for the display IC and proper handling of flushing the display buffer.

Yes exactly, and that's the logic behind the specific test split in api, display and indev at #341.

The functionality doesn't change at all if using lv_micropython.

In the future example board definitions could be added to showcase how to add the .py or USER_C_MODULE set of drivers, but for now I think this is a good step in the right direction. 👍🏼

@Lzw655
Copy link

Lzw655 commented Nov 11, 2024

Hi @andrewleech,

Thank you very much for this PR! It has allowed me to use LVGL9 on MicroPython 1.23.0 and run it successfully on the ESP32 development board.

However, when I use Squareline to generate UI files (ui_images.py.txt, ui.py.txt) compatible with MicroPython, it produces the following error.

MicroPython v1.23.0-4.gf4ec8a07c.dirty on 2024-11-11; linux [GCC 11.4.0] version
Use Ctrl-D to exit, Ctrl-E for paste mode

# First try
>>> import ui
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/x/Desktop/work/repository/work/MicroPython/scripts/ui.py", line 10, in <module>
AttributeError: 'module' object has no attribute 'theme_simple_init'

# After comment out these code: `theme = lv.theme_simple_init(dispp)` `dispp.set_theme(theme)`
>>> import ui
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/x/Desktop/work/repository/work/MicroPython/scripts/ui.py", line 651, in <module>
SyntaxError: Cannot convert 'function' to pointer!

I found that some APIs are indeed missing in the generated lv_mpy.c. Also, compared to the lv_mpy(official).c.txt generated by official lv_binding_micropython, the event_cb of mp_lv_obj_add_event_cb in lv_mpy(pr).c.txt does seem to have potential issues.

// This PR
static mp_obj_t mp_lv_obj_add_event_cb(size_t mp_n_args, const mp_obj_t *mp_args, void *lv_func_ptr)
{
    ...
    lv_event_cb_t event_cb = mp_to_ptr(mp_args[1]);
    ...
}

// Official
STATIC mp_obj_t mp_lv_obj_add_event_cb(size_t mp_n_args, const mp_obj_t *mp_args, void *lv_func_ptr)
{
    ...
    void *event_cb = mp_lv_callback(mp_args[1], &lv_obj_add_event_cb_event_cb_callback, MP_QSTR_lv_obj_add_event_cb_event_cb, &user_data, NULL, (mp_lv_get_user_data)NULL, (mp_lv_set_user_data)NULL);
    ...
}

Could you help further improve it? Thank you very much!

@andrewleech
Copy link
Author

andrewleech commented Nov 11, 2024

Hi @Lzw655 that's interesting, how did you build it? I haven't managed to successfully compile for esp32.

This PR is very much still in progress, I've been integrating the excellent additions done by @Carglglz and trying to simplify the build and integration processes.

Update: Ah, I see, the failures I'm getting are like this:

CMake Error at /opt/esp/idf/tools/cmake/component.cmake:5 (get_property):
  get_property could not find TARGET ___idf_lvgl.  Perhaps it has not yet
  been created.
Call Stack (most recent call first):
  /opt/esp/idf/tools/cmake/project.cmake:198 (__component_get_property)
  /opt/esp/idf/tools/cmake/project.cmake:311 (__all_component_info)
  /opt/esp/idf/tools/cmake/project.cmake:780 (__project_info)
  CMakeLists.txt:82 (project)

I've figured out this is a problem when using IDF 5.2.2 (the current default version I use). However this failure doesn't occur on IDF 5.0.4 or 5.1.1
I'll keep testing to figure out which IDF version has changed the c component interface enough to break this...

@Lzw655
Copy link

Lzw655 commented Nov 12, 2024

@andrewleech I encountered the same issue. You can add this code snippet after here to resolve it. (example)

    idf_build_get_property(component_targets __COMPONENT_TARGETS)
    string(REPLACE "___idf_lvgl" "" component_targets "${component_targets}")
    idf_build_set_property(__COMPONENT_TARGETS "${component_targets}")

@andrewleech
Copy link
Author

@Lzw655 Oh strange, how did you come up with that snippet? I'm not yet sure of the IDF change that's triggering this issue (5.1.1 works, 5.1.5 does not).

@Lzw655
Copy link

Lzw655 commented Nov 12, 2024

I located the issue through the error message, identifying that, for some reason, the IDF recognized LVGL as a component. This code snippet detaches LVGL from the components to resolve it.

And I compiled it directly on IDF v5.2.2 without trying other versions. So I'm not sure how it would perform on other versions.

@Lzw655
Copy link

Lzw655 commented Nov 12, 2024

😥 I'm very sorry. Since this PR and #341 have similar content, I mixed them up. In fact, I'm using the code from #341.

@andrewleech
Copy link
Author

No apology necessary, that information really helps.
I'm merging and updating the esp integration from #341 into my branch here so it's directly relevant thanks!

@Lzw655
Copy link

Lzw655 commented Nov 12, 2024

Great! And I found that the following error was caused by updating LVGL to v9.2.2. When I reverted to the original version, the error no longer occurred.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/x/Desktop/work/repository/work/MicroPython/scripts/ui.py", line 651, in <module>
SyntaxError: Cannot convert 'function' to pointer!

@Carglglz
Copy link

@Lzw655 I've just updated my PR with your fix 🚀 (Thanks!) It seems the fix also works here Carglglz@4ff1ea9, i.e. no need to modify micropython esp32 cmake file. 👍🏼

@andrewleech
Copy link
Author

@Lzw655 I just ran into that function / pointer error myself, have written up the description here: #361

@Lzw655
Copy link

Lzw655 commented Nov 13, 2024

@Lzw655 I just ran into that function / pointer error myself, have written up the description here: #361

Thank you for your attention to this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants