Skip to content

manifest: update LVGL to v8.3.3 #53974

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

Closed

Conversation

diegoherranz
Copy link
Contributor

Related to zephyrproject-rtos/lvgl#33

Thanks!

@zephyrbot zephyrbot added manifest manifest-lvgl DNM This PR should not be merged (Do Not Merge) labels Jan 21, 2023
@zephyrbot
Copy link
Collaborator

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
lvgl zephyrproject-rtos/lvgl@5ab8309 (zephyr) zephyrproject-rtos/lvgl#33 zephyrproject-rtos/lvgl#33/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@diegoherranz
Copy link
Contributor Author

As far as I can tell, the fail on the "twister-build (4)" tests is due to some warnings about certain LVGL code itself (from upstream), which are treated as error.

@pdgendt
Copy link
Collaborator

pdgendt commented Jan 23, 2023

As far as I can tell, the fail on the "twister-build (4)" tests is due to some warnings about certain LVGL code itself (from upstream), which are treated as error.

It's not only warnings treated as errors, but the warnings should be looked at as well. Can you investigate and/or report upstream if the lvgl samples have the same issues.
It might be caused by some (default) configuration?

@carlescufi
Copy link
Member

@pdgendt @brgl @jfischer-no would be great if you could take a look at this

@carlescufi
Copy link
Member

@diegoherranz could you please fix the CI test issues? These do seem related to your change.

@MaureenHelm
Copy link
Member

@kisvegabor we're looking for someone to maintain the lvgl integration in zephyr. would you be interested or know someone?

@kisvegabor
Copy link

Hi Maureen,

We are working on some updates in LVGL. Due to these LVGL will contain built-in display drivers and interfaces to exiting systems. It will be possible to attache SPI, parallel port, etc driver to the built-in display and touch pad drivers. So all we need is a wrapper around Zephyr's SPI, parallel, etc driver API to allow using all the built-in display driver of LVGL. I think it will make maintaining LVGL in Zephyr much easier.

Other than that, could you tell in more detail what are the complications with maintaining LVGL in Zephyr?
Ideally LVGL should be just a drop-in library in Zephyr and we'd be happy to improve LVGL in a way to really make the integration that simple.

@jfischer-no
Copy link
Collaborator

We are working on some updates in LVGL. Due to these LVGL will contain built-in display drivers and interfaces to exiting systems. It will be possible to attache SPI, parallel port, etc driver to the built-in display and touch pad drivers. So all we need is a wrapper around Zephyr's SPI, parallel, etc driver API to allow using all the built-in display driver of LVGL. I think it will make maintaining LVGL in Zephyr much easier.

Good news. We already have a display controller API and support multiple controllers. The best thing for your plans would be to remove the LVGL module from Zephyr completely. How you manage the Zephyr port is up to you, SPI/I2C driver API is publicly available.

@kisvegabor
Copy link

I'm glad that you like the idea. However, I think it still would be convenient for the user if LVGL were part of Zephyr in some form and could be simply enabled in Kconfig or so.

How complicated would it be if we ship a ready to use Kconfig file?

@jfischer-no
Copy link
Collaborator

I'm glad that you like the idea. However, I think it still would be convenient for the user if LVGL were part of Zephyr in some form and could be simply enabled in Kconfig or so.

I do not think so, there is no benefit to the project. Rather the opposite, suppression of own development, maintenance overhead. The user could add LVGL to his downstream project as a module if needed, there is no need to keep anything upstream.

How complicated would it be if we ship a ready to use Kconfig file?

Please check the docs https://docs.zephyrproject.org/latest/develop/west/manifest.html

@pdgendt
Copy link
Collaborator

pdgendt commented Mar 3, 2023

It feels like we're running in circles again like the discussions for thrift or pigweed (see #54013 (comment)).
Please don't shoot me, the following is my humble opinion.

Having a GUI library inside zephyr feels like an addition instead of something that we have to drag along. We could probably benefit from having optional (lazy) modules, but that is entire other ongoing discussion.
It's a one-stop-getting-started with a sample instead of going to a project which may or may not support some version zephyr.

Regarding the question how to integrate LVGL in zephyr, I know there were suggestions in the past to have a GPU API in zephyr, which could in turn be used by something like LVGL as an abstraction layer.

@kisvegabor
Copy link

How complicated would it be if we ship a ready to use Kconfig file?

Please check the docs https://docs.zephyrproject.org/latest/develop/west/manifest.html

Finally what would be goal? Having or not having LVGL in west.yml?

Regarding the question how to integrate LVGL in zephyr, I know there were suggestions in the past to have a GPU API in zephyr, which could in turn be used by something like LVGL as an abstraction layer.

It's great! I have no doubt that we can figure out something the use this GPU API.


I understand that dealing with a lot of libraries is quite challenging and it's very reasonable that want to simplify the maintenance of Zephyr.

In the end what would be important for me is to make it simple for the users to get started with Zephyr+LVGL. They can enable a Zephyr config, or just clone LVGL into a Zephyr folder, or run a script. Any of these are fine, just have a clean, simple and well documented way for the integration.

If LVGL had a "Zephyr driver" things were quite simple. So we can implement the "Zephyr drivers" but they will work only with a given Zephyr version. But how can we detect if (due to e.g. Zephyr API changes) the drivers become broken? Or can we just say we support some given Zephyr versions and do the tests manually?

Did this problem come up with other libraries too? Is there a best practice? Maybe some reference CI workflow that we can check?

As I mentioned I'm open for having Zephyr support in LVGL, but I'd like to check how we can mange and test the integration in a simple and sustainable way.

@diegoherranz
Copy link
Contributor Author

diegoherranz commented Mar 23, 2023

Just my 2 cents here as a Zephyr and LVGL user: I know that it wouldn't take long to integrate upstream's LVGL library "manually" in a Zephyr project, but having it as a built-in module and having all the kconfig integration feels very nice to use. However, I see both points of view and understand that this brings some maintenance task into Zephyr.

@diegoherranz could you please fix the CI test issues? These do seem related to your change.

I haven't had the time to have a look at this yet, but I plan to do so. If that gets fixed, would we be OK to move to LVGL v8.3.3 given the discussion above?
If yes, afterwards I would push updates to v8.3.5, which are smaller than this big update (8.20 -> 8.3.3)

Thanks!

@dleach02
Copy link
Member

dleach02 commented Apr 4, 2023

Will the twister errors be addressed soon?

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 4, 2023
@pdgendt
Copy link
Collaborator

pdgendt commented Jun 16, 2023

@diegoherranz Do you think we could bring this PR (and the LVGL one) up-to-date? We can pick this up after the 3.4 feature freeze.

@pdgendt pdgendt removed the Stale label Jun 16, 2023
@faxe1008
Copy link
Collaborator

@pdgendt I am very interested in moving this PR forward, from the discussion I gather that some twister issues need to be addressed, is that correct? And the preferred way of doing it would be to upstream those fixes into LVGL itsself and wait for another release thereafter?

@pdgendt
Copy link
Collaborator

pdgendt commented Jun 22, 2023

@pdgendt I am very interested in moving this PR forward, from the discussion I gather that some twister issues need to be addressed, is that correct? And the preferred way of doing it would be to upstream those fixes into LVGL itsself and wait for another release thereafter?

zephyr has an LVLG fork where we accept patches: https://github.com/zephyrproject-rtos/lvgl
The twister logs from this PR are no longer available, so I'm not sure what is missing.

@diegoherranz are you still working on this? Could you rebase the changes, so we can verify what is needed to fix the build issues?

@faxe1008
Copy link
Collaborator

faxe1008 commented Jun 22, 2023

@pdgendt I took some time and fixed the twister issues that I found. I created my own fork based on @diegoherranz work right here: https://github.com/faxe1008/zephyr-lvgl/tree/update_8.3.7. While I was at it, I also merged the version 8.3.7.

The tests run fine locally, tomorrow I will do some tests with actual hardware.

I have found some issues regarding allocation on my rebased version. I bisected the issue down to this commit:

commit 657e752e7ed661e46cb2c765315dfb2d17fda09f
Author: Gabor Kiss-Vamosi <[email protected]>
Date:   Mon May 23 16:42:14 2022 +0200

    fix(disp): make lv_scr_load work better with lv_scr_load_anim and auto_del = true

    related to: https://forum.lvgl.io/t/crash-when-delete-a-screen-by-using-lv-obj-del/8930

Need some more time to investigate this.

@kisvegabor
Copy link

I have found some issues regarding allocation on my rebased version. I bisected the issue down to this commit:

How can I reproduce this issue? Do you see it with the latest commit in the release/v8.3 branch?

@faxe1008
Copy link
Collaborator

faxe1008 commented Jun 23, 2023

@pdgendt I got the tests running locally and did a test with the lvgl sample, seems to run fine.

PR in zephyr project: #59656
PR in lvgl project: zephyrproject-rtos/lvgl#41

Any feedback/help verifying the fixes added would be appreciated :^)

@kisvegabor For testing I ran the twister test suite:

./scripts/twister -v --testsuite-root tests/lib/gui/lvgl

In particular https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/lib/gui/lvgl/src/main.c#L54 crashes.

Do you see it with the latest commit in the release/v8.3 branch?

I tried applying https://github.com/lvgl/lvgl/commit/1caafc55dde46e1b7e3d17d8c5349fbf7cccba9f but the tests still crashed.

@kisvegabor
Copy link

This test deletes the active screen:

  lv_obj_t *default_screen = lv_scr_act();
  lv_obj_t *new_screen = lv_obj_create(NULL);
  lv_scr_load(new_screen);
  lv_task_handler();

  lv_obj_t *act_screen = lv_scr_act(); //act_screen == new_screen
  lv_obj_del(new_screen);              //Deletes the active screen
  lv_scr_load(default_screen);         //This should be called before deleting the active screen
  lv_task_handler();

  act_screen = lv_scr_act();

@faxe1008
Copy link
Collaborator

@kisvegabor when checking the issue I also noticed that and indeed reordering the statements works. However I was not sure if you are supposed to be able to do this. Since the test already existed and ran fine in 8.2.0 I figured it was expected that this works.

@kisvegabor
Copy link

Since the test already existed and ran fine in 8.2.0 I figured it was expected that this works.

In v8.2 it was working only by accident and not by design 🙂

@faxe1008
Copy link
Collaborator

@kisvegabor Ah ok, good to know. Anyways I reverted the "revert" :^), and changed the tests over at:
#59656
Thanks 👍

faxe1008 added a commit to faxe1008/zephyr that referenced this pull request Jul 10, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos#53974.

Signed-off-by: Fabian Blatz <[email protected]>
carlescufi pushed a commit that referenced this pull request Jul 10, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in #53974.

Signed-off-by: Fabian Blatz <[email protected]>
coreboot-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Jul 11, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos/zephyr#53974.

(cherry picked from commit ec46da4)

Original-Signed-off-by: Fabian Blatz <[email protected]>
GitOrigin-RevId: ec46da4
Change-Id: I4fba2439cfdcfe3ef599fbfb6e9d909fececb5be
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4677430
Commit-Queue: Al Semjonovs <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Tested-by: Al Semjonovs <[email protected]>
Reviewed-by: Al Semjonovs <[email protected]>
DRuffer-tmo pushed a commit to tmobile/DevEdge-IoTDevKit-ZephyrRTOS that referenced this pull request Jul 27, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos/zephyr#53974.

Signed-off-by: Fabian Blatz <[email protected]>
DRuffer-tmo pushed a commit to tmobile/DevEdge-IoTDevKit-ZephyrRTOS that referenced this pull request Jul 27, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos/zephyr#53974.

Signed-off-by: Fabian Blatz <[email protected]>
DRuffer-tmo pushed a commit to tmobile/DevEdge-IoTDevKit-ZephyrRTOS that referenced this pull request Aug 8, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos/zephyr#53974.

Signed-off-by: Fabian Blatz <[email protected]>
DRuffer-tmo pushed a commit to tmobile/DevEdge-IoTDevKit-ZephyrRTOS that referenced this pull request Aug 9, 2023
Load the default screen before the deleting the newly created one.
This used to work in LVGL 8.2.0 but was not intended according to the
discussion in zephyrproject-rtos/zephyr#53974.

Signed-off-by: Fabian Blatz <[email protected]>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 26, 2023
@kartben kartben removed the Stale label Aug 26, 2023
@faxe1008
Copy link
Collaborator

This PR can be closed as 8.3.7 was merged to the module. Thanks @diegoherranz for the initial impulse and work on this 👍

@faxe1008 faxe1008 closed this Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM This PR should not be merged (Do Not Merge) manifest manifest-lvgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants