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

Added support for LittlevGL library #6826

Merged
merged 3 commits into from
Jan 7, 2019

Conversation

vanwinkeljan
Copy link
Member

@vanwinkeljan vanwinkeljan commented Mar 27, 2018

Added support for LittlevGL GUI library.

From LittlevGL website

LittlevGL is a free and open-source graphics library providing everything you need to create embedded GUI with easy-to-use graphical elements, beautiful visual effects and low memory footprint.

@carlescufi
Copy link
Member

@vanwinkeljan can you rebase this against the latest master? There's a bunch of commits that don't belong to the PR

@vanwinkeljan
Copy link
Member Author

@carlescufi What is the normal approach if you have dependencies to other PR?
I could issue a PR without the dependencies but then nobody can test it.

@lpereira
Copy link
Collaborator

While I can imagine this being useful, this adds a lot of code (over 180k lines) that has to be tracked and updated each Zephyr release.

samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
samples/display/ili9340/README.rst Outdated Show resolved Hide resolved
samples/display/ili9340/README.rst Outdated Show resolved Hide resolved
samples/display/ili9340/README.rst Outdated Show resolved Hide resolved
samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
@nashif
Copy link
Member

nashif commented Mar 28, 2018

While I can imagine this being useful, this adds a lot of code (over 180k lines) that has to be tracked and updated each Zephyr release.

this will hopefully be one of those dependencies that will have to be maintained externally to zephyr, i.e. in its own repo in the project or similar to openthread.

@nashif
Copy link
Member

nashif commented Mar 28, 2018

@vanwinkeljan can you rebase this against the latest master? There's a bunch of commits that don't belong to the PR

those are not-merged patches from the SPI PR..

@carlescufi
Copy link
Member

those are not-merged patches from the SPI PR..

@vanwinkeljan @nashif sorry, I didn't realize. Sorry for the noise.

@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Mar 28, 2018
@vanwinkeljan
Copy link
Member Author

@nashif @laperie I had a look at how this is handled for openthread, as lvgl has no dedicated makefiles I used the FetchContent module instead. Only downside is that the FetchContent module was only introduced in cmake 3.11

@jukkar
Copy link
Member

jukkar commented Apr 17, 2018

Thanks for pushing this. It would be great to get some gui support for zephyr. I am also hoping that someone would create a driver for the small oled displays that are used a lot in this field.

I had a look at how this is handled for openthread, as lvgl has no dedicated makefiles I used the FetchContent module instead.

I did not quite understand your comment about makefiles. Could you elaborate a bit more why we would not do similar things as what subsys/net/lib/openthread is doing? Adding a version requirement for cmake should be avoided IMHO.

@vanwinkeljan
Copy link
Member Author

@jukkar Problem is that lvgl repo has no top level makefile so if we would like to use the ExternallModule cmake module instead of a FetchContent module a top level makefile needs to be added after fetching the lvgl repo, in case of the FetchContent module ext/lib/gui/lvgl/CMakeLists.txt can be used to build the lvgl library.

Further note that there is already a requirement for a minimal cmake version (3.8.2) and I just bumped it to 3.11.

drivers/display/display_ili9340.c Outdated Show resolved Hide resolved
include/display.h Outdated Show resolved Hide resolved
include/display.h Outdated Show resolved Hide resolved
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

OK on the docs, thanks!

cmake/app/boilerplate.cmake Outdated Show resolved Hide resolved
@vanwinkeljan
Copy link
Member Author

@nashif @pfalcon @embeddedt In the end I pushed the CMake FetchContent solution and removed the lvgl source from ext/lib

@nashif
Copy link
Member

nashif commented Jan 6, 2019

Why do we add gazillion of files not directly related to being "The Zephyr Project is a scalable real-time operating system (RTOS) supporting multiple hardware architectures, optimized for resource constrained devices, and built with security in mind." ?

how exactly adding "gazillion" files to the source conflicts with the the project being a scalable RTOS? if you do not want this feature, it will not go into your resource constrained device. Note that some hals go into 100s of MBs and we still have them in the tree.

Beyond that, who's going to update this codedrop? Why endure commit content noise related to these updates?

the same way we update any other external components.

Why not git submodule? Why not modules supported by that west tool?

We do not support git sub-modules.

West is not ready, this PR has been open for almost a year now waiting for west or something similar, it is time to move on and use west when west is supported and ready for this.

@nashif
Copy link
Member

nashif commented Jan 6, 2019

@nashif @pfalcon @embeddedt In the end I pushed the CMake FetchContent solution and removed the lvgl source from ext/lib

ok, thanks. I have no problem with that.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 7, 2019

@vanwinkeljan:

In the end I pushed the CMake FetchContent solution and removed the lvgl source from ext/lib

Thanks, that's great step, makes things much more clear/manageable!

@pfalcon
Copy link
Contributor

pfalcon commented Jan 7, 2019

how exactly adding "gazillion" files to the source conflicts with the the project being a scalable RTOS? if you do not want this feature, it will not go into your resource constrained device. Note that some hals go into 100s of MBs and we still have them in the tree.

Does that mean that we should add more stuff to the tree indiscriminately? I maintain JerryScript and MicroPython ports for Zephyr, should I push them into the tree?

the same way we update any other external components.

Well, we're already regularly late with their updates, delivering stale/buggy content the users. Not just we do that alone, we actually promote that way, by effectively making forks of projects for nothing else but them becoming stale in our tree.

We do not support git sub-modules.

Poor us.

West is not ready, this PR has been open for almost a year now waiting for west or something similar

Well, I've overlooked that it's from March last year. And I'm definitely all for gratifying contributors with merging their patches sooner rather than later (applies to my patches too). I'm just not sure that the compromise between the choices - "slurp yet another large 3rd-party project as a codedrop into the tree" vs "give in to the logic and use git submodules" - was the right one.

I'm glad that a solution was found.

Regarding west, we can learn our lessons as we go. If we'd have chosen git submodules (or another existing tool), there wouldn't be such problems. And if we made west a frontend on top of git submodules, we could use submodules "in raw" while waiting for west to complete.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 7, 2019

@nashif: I also wanted to tell that for a long time, just couldn't think of a good way do that. So let me just do it straight: github does not notify in any way an author of a review when their review is dismissed. Consequently, if you want to give an author of such a review an opportunity to check that the issues were actually fixed, they need to be somehow notified - maybe by including @mention in the dismissal description, but I'm not sure if that works (need to test), and otherwise by a straight comment with mention. And conversely, if the aim is to sneak some code past the normal review process and leave the impression that all PRs are approved unanimously, make sure to not mention the dismissal to affected persons, and they'll never know ;-).

@embeddedt
Copy link

@pfalcon

Well, we're already regularly late with their updates, delivering stale/buggy content the users.

As a heads-up: we're planning to release the next version of LittlevGL (5.3) this month. The release after that is planned for June.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 7, 2019

@embeddedt: Thanks for heads-up.

Generally, my idea is that people should be motivated to work directly with upstream projects (bug fixes, etc.). And including an upstream project by reference motivates doing that, whereas including it by copy arguably demotivates that, driving towards "compromises" like "oh, we need to cut the release soon, let's just push that fix to our copy, we'll deal with all that later". And later only becomes more and more later. And as pointed by @nashif, we already doing/have to do that with vendor HALs. Well, that should be the motivation to NOT do that with community-maintained open source projects.

samples/gui/lvgl/sample.yaml Outdated Show resolved Hide resolved
samples/gui/lvgl/sample.yaml Outdated Show resolved Hide resolved
Extend display API screen info with double buffer enum entry.

Signed-off-by: Jan Van Winkel <[email protected]>
Added glue logic to interface Zephyr with LittlevGL GUI library

This includes:
 * KConfig options for all lvgl options
 * Kernel & user space memory management
 * Zephyr to lvgl FS call mapping
 * Color space conversion function

Signed-off-by: Jan Van Winkel <[email protected]>
Added a basic sample showing how to use the LittlevGL library in an
application.

Signed-off-by: Jan Van Winkel <[email protected]>
@vanwinkeljan
Copy link
Member Author

@nashif Updated the sample.yaml file, I did remove native POSIX target as it depends on SDL2. I don't think it is included in the CI docker container and as suche the build will fail.
We have three options here:

  1. Add SDL2 to the CI image
  2. Use the dummy display driver through a overlay conf file
  3. Do not include native posix target in CI

Another question should I add myself to CODEWONERS for lib/gui ?

@nashif
Copy link
Member

nashif commented Jan 7, 2019

Another question should I add myself to CODEWONERS for lib/gui ?

yes please

@nashif nashif merged commit 81301bf into zephyrproject-rtos:master Jan 7, 2019
@vanwinkeljan vanwinkeljan deleted the lvgl branch January 7, 2019 21:31
@kisvegabor
Copy link

Hi,

@vanwinkeljan notified us at about you have integrated LittlevGL. As the author of LittlevGL I was very happy to hear that!

My question is if you would be interested in writing a blog post on LittlevGL's blog about getting started with Zephyr and LittlevGL.

I found this description but I meant a something that is for beginners with Zephyr. It could a good option to introduce and promote Zephyr.

So would you be interested?

@marc-hb
Copy link
Collaborator

marc-hb commented May 22, 2019

West is not ready, this PR has been open for almost a year now waiting for west or something similar, it is time to move on and use west when west is supported and ready for this.

West is in full swing now. Is there already a github issue filed or work in progress to move LittleVGL to west? If not I can file one. I'd like it not to be low priority because right now this component seems to be the only one that breaks using sanitycheck offline. git grep shows no other component using FetchContent. I found a few ExternalProject but they either don't download anything or are not part of the sanitycheck defaults.

@vanwinkeljan
Copy link
Member Author

@marc-hb I double checked with @nashif today and lvgl will be part of a second wave of migrating components out of tree

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 10, 2019

Thx @vanwinkeljan . I realized the problem is not whether the lvgl module has its own git repo or not. From what I've seen so far, moving modules out of the main repo is a Good Thing But it never makes any module magically more modular or optional. Sorry for this west digression.

FetchContent is the real issue. FetchContent is just plain bad as it makes builds depend on an external resource and the network. There are (at least) two ways to fix this:

  • Either lvgl is made truly optional and never ever included in any automated build, so no automated build ever fails unexpectedly unless it explicitly requested lvgl. Vote with "Thumbs up" emoji.
  • Or https://github.com/littlevgl/lvgl.git gets added as a new west module and true dependency and FetchContent is not needed anymore. Vote with "Hooray" emoji.
  • Other, vote with "Thumbs down" emoji and elaborate.

@vanwinkeljan
Copy link
Member Author

@marc-hb FetchContent is just a stopgap solution until the lvgl dependency is migrated to west

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 10, 2019

lvgl will be part of a second wave of migrating components out of tree

This "lvgl" above sounded like zephyr/lib/gui/* but you meant https://github.com/littlevgl/lvgl.git ?

@vanwinkeljan
Copy link
Member Author

@marc-hb yes with lvgl I did mean https://github.com/littlevgl/lvgl.git; zephyr/lib/gui/* is merely glue logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.