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

pkg/lvgl: bump to 8.2.0 #17681

Merged
merged 5 commits into from
Feb 28, 2022
Merged

pkg/lvgl: bump to 8.2.0 #17681

merged 5 commits into from
Feb 28, 2022

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Feb 19, 2022

Contribution description

This PR bumps the version of the LVGL package to 8.2.0. The sample test applications are adapted to the new API. Quite some changes were needed with this new version:

  • widgets and extra widgets need to be explicitly included to the build. This is required to limit code size
  • the LVGL API has a lot of changes, incompatible with previous versions, see the modifications added to the sample applications

Open question: the management of widgets with RIOT modules makes the integration with the LVGL Kconfig a bit redundant since it also provides a different way to select widgets when using menuconfig. Since we didn't fully move to Kconfig, I had to keep a compatibility with make dependency resolution mecanism. It's still possible to configure LVGL via menuconfig to select the required widgets, but in the current situation, one also has to select corresponding RIOT modules explicitly. That's a drawback that will be fixed when we move to Kconfig but maybe there's a solution already ?

Testing procedure

tests/pkg_lvgl and tests/pkg_lvgl_touch are still working and should not increase (too much) memory consumption: tested with success on stm32f746g-disco. I get similar memory consumption results but one has to take care of the LVGL widgets/features included to the build.

Issues/PRs references

None

Thanks to @kaspar030, @bergzand and @fjmolinas who helped me to move forward with this one.

@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework labels Feb 19, 2022
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 19, 2022
@aabadie aabadie force-pushed the pr/pkg/lvgl-v8 branch 3 times, most recently from 9dd31fd to 980b06d Compare February 20, 2022 08:26
@leandrolanzieri
Copy link
Contributor

That's a drawback that will be fixed when we move to Kconfig but maybe there's a solution already ?

Can we maybe select from our modules the required symbols in LVGL's Kconfig?

@aabadie
Copy link
Contributor Author

aabadie commented Feb 21, 2022

Can we maybe select from our modules the required symbols in LVGL's Kconfig?

Yes that's an option, I'll give it a try

@aabadie
Copy link
Contributor Author

aabadie commented Feb 21, 2022

68c9a1e should do that: the unused widgets are correctly unchecked and the ones selected via modules are correctly selected in the menuconfig UI. But then the build fails because the defines 'LV_USE_' are missing, so internally it seems that lvgl enables them still. Is there a way to force the symbol to be defined to 0 if unchecked ? (Maybe I'm wrong because I'm missing something).

@leandrolanzieri
Copy link
Contributor

68c9a1e should do that: the unused widgets are correctly unchecked and the ones selected via modules are correctly selected in the menuconfig UI. But then the build fails because the defines 'LV_USE_' are missing, so internally it seems that lvgl enables them still. Is there a way to force the symbol to be defined to 0 if unchecked ? (Maybe I'm wrong because I'm missing something).

Hmm I can't reproduce the build fail locally, but I see the undefined symbols in menuconfig. That makes sense, as the Kconfig file in the package is not downloaded yet, because the build system does not know that we want to use it. This would not change with the completion of the migration so we should think what is the best way to deal with external Kconfig files. Although at first I thought that the approach of sourcing it from the build directory was great and transparent, it will not work when using dependency resolution with Kconfig (the package is downloaded only after). One option I can think right now would be to have our own version of the upstream Kconfig, with the changes needed by the build system.

@aabadie
Copy link
Contributor Author

aabadie commented Feb 21, 2022

I can't reproduce the build fail locally, but I see the undefined symbols in menuconfig

Weird, after running menuconfig, the build of tests/pkg_lvgl fails both with and without TEST_KCONFIG=1. What undefined symbols are you talking about ? In menuconfig, what is important to check is that the corresponding widgets (for example, btn and label in widgets menu + chart and win in the extra widgets menu) are selected and the others not. This reflects what's included in the build but for some reason, the build fails trying to apparently build an unrelated widget lv_slider.

@aabadie
Copy link
Contributor Author

aabadie commented Feb 21, 2022

I'm wondering if that doesn't come from a bug on their side, it builds just fine with the following change;

diff --git a/pkg/lvgl/Makefile.include b/pkg/lvgl/Makefile.include
index f900ae4af1..665c7cfe1f 100644
--- a/pkg/lvgl/Makefile.include
+++ b/pkg/lvgl/Makefile.include
@@ -40,3 +40,5 @@ PSEUDOMODULES += lvgl_widget_slider
 PSEUDOMODULES += lvgl_widget_switch
 PSEUDOMODULES += lvgl_widget_textarea
 PSEUDOMODULES += lvgl_widget_table
+
+CFLAGS += -DCONFIG_LV_USE_SLIDER=0

@leandrolanzieri
Copy link
Contributor

leandrolanzieri commented Feb 22, 2022

What undefined symbols are you talking about ? In menuconfig, what is important to check is that the corresponding widgets (for example, btn and label in widgets menu + chart and win in the extra widgets menu) are selected and the others not.

Before I tested I removed the build directory, as the package is not downloaded, the Kconfig can't be sourced, so the LVGL symbols don't exist at that point in time. That's the issue I was describing above.

@aabadie
Copy link
Contributor Author

aabadie commented Feb 22, 2022

I removed the build directory, as the package is not downloaded, the Kconfig can't be sourced

I already noticed that. How about we move the closing endifs of if PACKAGE_LVGL and TEST_KCONFIG to the end of the Kconfig file ?

@aabadie
Copy link
Contributor Author

aabadie commented Feb 22, 2022

Looking at my build issue, it strongly feels like a problem in the lv_conf_internal.h header when Kconfig is used. The slider config, when not set still results on LV_USE_SLIDER being set to 1. Looking deeper, there are some inconsistencies upstream. I think I'll open a PR there.

@leandrolanzieri
Copy link
Contributor

How about we move the closing endifs of if PACKAGE_LVGL and TEST_KCONFIG to the end of the Kconfig file ?

We could do that, but it would not solve the issue of the file not being there before downloading the package. As it is, the user would have to select the package, somehow trigger the fetch, and then open menuconfig again to configure the package. Also, those two steps would not work if configuring from a .config file. I think we should have our own copy of their upstream Kconfig, adapted to our build system.

@aabadie
Copy link
Contributor Author

aabadie commented Feb 22, 2022

I think we should have our own copy of their upstream Kconfig, adapted to our build system.

That would mean maintaining a 1k+ lines file. It would be nice if you could fetch it independent of the pkg mechanism.

@leandrolanzieri
Copy link
Contributor

I think we should have our own copy of their upstream Kconfig, adapted to our build system.

That would mean maintaining a 1k+ lines file. It would be nice if you could fetch it independent of the pkg mechanism.

I totally agree, but the package is selected at configuration time, how could we fetch it before knowing we are going to source it?

@aabadie
Copy link
Contributor Author

aabadie commented Feb 22, 2022

how could we fetch it before knowing we are going to source it?

Good question. Is it possible to add the Kconfig file as a dependency of menuconfig, and only when the package is included into the build ? Do you agree that this is out of the scope of this PR and should be addressed lately ?

Otherwise, I added the change needed upstream as a patch, this way no need to wait for it to be merged and released.

@leandrolanzieri
Copy link
Contributor

how could we fetch it before knowing we are going to source it?

Good question. Is it possible to add the Kconfig file as a dependency of menuconfig, and only when the package is included into the build?

Hmm I don't think so, as the Kconfig tree is built once at the beginning of the configuration process (both when using .config and when using menuconfig) there is no possibility of dynamic sourcing.

Do you agree that this is out of the scope of this PR and should be addressed lately ?

Yes, sure. I just pointed it out here because it is a design decision that we will have to make at some point, and this package is the perfect example.

Otherwise, I added the change needed upstream as a patch, this way no need to wait for it to be merged and released.

👍🏼

@aabadie aabadie force-pushed the pr/pkg/lvgl-v8 branch 3 times, most recently from dfa6588 to c3b98c2 Compare February 22, 2022 19:04
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well, just some questions, but things you can ignore.

pkg/lvgl/Makefile.dep Show resolved Hide resolved
pkg/lvgl/contrib/lvgl.c Show resolved Hide resolved
pkg/lvgl/contrib/lvgl.c Outdated Show resolved Hide resolved
pkg/lvgl/contrib/lvgl.c Outdated Show resolved Hide resolved
@aabadie
Copy link
Contributor Author

aabadie commented Feb 24, 2022

In the last commits, I pushed changes to lv_conf.h and test applications app.config.test that allows to get the exact same generated firmware with and without Kconfig. This is something I couldn't get in v7 because the Kconfig and lv_conf_template.h files were out of sync in terms of options (probably because of a misuse on our side :) ).

In terms of code size, the port of the v8 seems to move ROM requirements to RAM (3.5KB) in tests/pkg_lvgl but with the touch version, ROM size is equivalent, while RAM usage has increased significantly (+4KB).

  • tests/pkg_lvgl

v8:

   text	   data	    bss	    dec	    hex
 109540	    192	  15604	 125336	  1e998

master

   text	   data	    bss	    dec	    hex
 113076	    244	  12144	 125464	  1ea18
  • tests/pkg_lvgl_touch

v8:

   text	   data	    bss	    dec	    hex
101640	    188	  15788	 117616	  1cb70

master

   text	   data	    bss	    dec	    hex
 101188	    244	  11772	 113204	  1ba34

@aabadie aabadie force-pushed the pr/pkg/lvgl-v8 branch 2 times, most recently from ae07e40 to e92a788 Compare February 25, 2022 11:09
@github-actions github-actions bot added the Area: doc Area: Documentation label Feb 25, 2022
@aabadie aabadie force-pushed the pr/pkg/lvgl-v8 branch 2 times, most recently from eba9b8a to 58fc4d5 Compare February 25, 2022 12:05
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Feb 25, 2022
@aabadie
Copy link
Contributor Author

aabadie commented Feb 25, 2022

After some offline discussion, it was decided to keep lvgl v7 without using it in sample applications. To use the v7 version, just use USE_PKG += lvgl7 in the application Makefile and the rest should remain the same. Kconfig is not adapted (should it?). I tested locally and previous test applications build and work the same.
Errors raised by Doxygen are added to the exclude patterns file.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, lets get this one in! I trust you have tested this one @aabadie since I do not have hardware at hand ATM.

@fjmolinas
Copy link
Contributor

BTW I tested on #16944 adapted to v8, works fine!

@fjmolinas fjmolinas merged commit 3aeba84 into RIOT-OS:master Feb 28, 2022
@kaspar030 kaspar030 deleted the pr/pkg/lvgl-v8 branch February 28, 2022 08:44
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants