You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/15 12:49:03 UTC
[GitHub] [incubator-nuttx-apps] w8jcik opened a new pull request #334: Don't copy headers outside of libraries
w8jcik opened a new pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334
## Summary
LittlevGL is an example of library that copies it's header outside of it's folder.
This PR removes this behavior, so the library exposes one of it's folders as include directory.
LittlevGL is just an example and this PR is proof of concept if you accept such change.
## Impact
Applications are not separable because they have some files outside of their folders. In particular LittlevGL is copying `lvgl.h` to `include/graphics/lvgl.h` when building. This file organisation doesn't seem to help with anything. If someone finds it useful to have one `include` directory with all possible libraries it also doesn't work because normally `lvgl.h` header is not there.
If this change accepted I can change the other applications to be like this.
Long term purpose of this is to be able to plug-in and out libraries. (package management)
## Testing
```
mkdir test
(
cd test
git clone https://github.com/apache/incubator-nuttx.git
git clone https://github.com/apache/incubator-nuttx-apps.git
(
cd nuttx
tools/configure.sh stm32f4discovery/nsh
make menuconfig
# enable GRAPHICS_LVGL and EXAMPLES_LVGLDEMO (http://nuttx-config-dev.nxtlabs.pl/#/?filter=LVGL)
make -j 12
)
)
```
It is just bulid system change and it builds. Also if anything is wrong with `apps/graphics/lvgl`, the `apps/examples/lvgldemo` fails to bulid which shows that one is using the other. I didn't run it on a board.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455274811
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
@v01d So there is some emulation layer that can be enabled? I don't see explicit option here http://nuttx-config.nxtlabs.pl/#/?current=VIDEO_FB. Maybe we should add information about this emulation and your custom solution to the README to help people who want to use LVGL. Readmes are empty for many of the apps. I am not sure why. Could you describe in three sentences something that could be put into this README?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455054469
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Why copy lvgl.h to exports directory? How about we export?
```
CFLAGS += ${shell $(INCDIR) $(INCDIROPT) "$(CC)" $(APPDIR)/graphics/lvgl}
CXXFLAGS += ${shell $(INCDIR) $(INCDIROPT) "$(CC)" $(APPDIR)/graphics/lvgl}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455561274
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
@v01d why not enhance the driver layer? @w8jcik also look for a solution which mean this is a common requirement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I am also be in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying to add it few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
Anyway I think it is enough for this PR then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 7.24). Or I didn't know how to use it better.
NuttX framebuffer as far as I understand needs width x height x depth amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I am also be in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying to add it few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes because it sits in graphics? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup, I also reminded myself branding change when writing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455142090
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
I think "#include <lvgl/lvgl.h>" as Maciej modified is fine. Currently only "lvgldemo" is using the LVGL. Maciej, avoid using the name LittlevGL because it is "deprecated", see: https://blog.lvgl.io/2020-06-01/announcement
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455136951
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
We should follow LVGL convention:
https://github.com/lvgl/lv_examples/blob/master/lv_examples.h#L16
either ```#include <lvgl.h>``` or ```#include <lvgl/lvgl.h> is good, but ```#include <graphics/lvgl.h>``` just make the life of programmer is hard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes because it sits in graphics? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup @acassis, I also reminded myself branding change when writing this. Ok, I am going with making the life of programmer easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455103797
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Sure, but then we need to `#include <lvgl.h>` instead of `#include <graphics/lvgl.h>`. I don't mind but I though that this prefix is intentional to keep things within namespaces like `graphics` here. Do we want to keep prefixes or do they go? So far they were used consistently.
Here is additional PR which shows necessary change to introduce this suggestion https://github.com/w8jcik/incubator-nuttx-apps/pull/1/files
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it in such way. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 6.xx). Or I didn't know how to use it better.
NuttX framebuffer as far as I understand needs [width x height x color depth] amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455274811
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
So there is some emulation layer that can be enabled? Maybe we should add this information and your custom solution to the README to help people who want to use it. Readmes are empty for many of the apps. I am not sure why.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I am also be in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying again to add it, few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
Anyway, it should be enough for this PR then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes because it sits in graphics? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup @acassis, I also reminded myself branding change when writing this. Ok @xiaoxiang781216, I am going with making the life of programmer easier. @acassis in this repository only lvgldemo, but maybe users do :D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455274811
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
So there is some emulation layer that can be enabled? Maybe we should add this information and your custom solution to the README to help people who want to use LVGL. Readmes are empty for many of the apps. I am not sure why.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455790689
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
@xiaoxiang781216 I can open a PR for my LCD character driver. As per the ability to send multiple rows, I propose that on the mailing list and did not get much response. We can discuss it further again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455274811
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
@v01d So there is some emulation layer that can be enabled? I don't see explicit option here http://nuttx-config.nxtlabs.pl/#/?current=VIDEO_FB. Maybe we should add information about this emulation and your custom solution to the README to help people who want to use LVGL. Could you describe in three sentences something that could be put into this README? Readmes are empty for many of the apps. I am not sure why.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455134281
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Usually libraries assume some sort of layout for the include directory and the user only needs to account for installation prefix. In this case, lvgl is assuming a layout of <prefix>/lvgl/lvgl.h, so any extra component in the path should be part of the -I flag passed to the compiler. Adding extra prefixes could break existing programs following the expected inclusion format.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455118690
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Update: Actually it will be `#include <lvgl/lvgl.h>` not `#include <lvgl.h>`, there was one more explicit include present in `lvgldemo` `Makefile` which I just noticed.
https://github.com/w8jcik/incubator-nuttx-apps/pull/1/commits/7f62736c771922aa8c8e6bd3d4976d083ef66ef8
But this is LittlevGL specific. The question remains, is it fine to drop prefixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it in such way. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 7.24). Or I didn't know how to use it better.
NuttX framebuffer as far as I understand needs [width x height x color depth] amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455274811
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
So there is some emulation layer that can be enabled? Maybe we should add this information and your custom solution to the README to help people who want to use it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it in such a way. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 6.xx). Or I didn't know how to use it better.
NuttX framebuffer, as far as I understand, needs [width x height x color depth] amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658932777
@w8jcik please squash all commit into one by git rebase --interactive HEAD~8. Also we need remove lvgl.h from apps/include/graphics/.gitignore too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes because it sits in graphics? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup @acassis, I also reminded myself branding change when writing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 7.24). Or I didn't know how to use it better.
NuttX framebuffer as far as I understand needs width x height x color depth amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 7.24). Or I didn't know how to use it better.
NuttX framebuffer as far as I understand needs [width x height x color depth] amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455260733
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Not sure the second approach is possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455749048
##########
File path: examples/lvgldemo/Make.defs
##########
@@ -36,4 +36,6 @@
ifneq ($(CONFIG_EXAMPLES_LVGLDEMO),)
CONFIGURED_APPS += $(APPDIR)/examples/lvgldemo
+CFLAGS += ${shell $(DEFINE) "$(CC)" LV_LVGL_H_INCLUDE_SIMPLE}
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455694018
##########
File path: examples/lvgldemo/Makefile
##########
@@ -86,11 +86,18 @@ endif
MAINSRC = lvgldemo.c
-LVGL_SRC_DIR=$(APPDIR)/graphics/lvgl/lvgl
-LVGL_DIR=$(APPDIR)/graphics/lvgl
+# Upstream `lv_examples/lv_examples.h` includes LVGL
+# in following way `#include "../lvgl/lvgl.h"`, for no clean reason.
+#
+# The way that we export LVGL offers `#include <lvgl/lvgl.h>`, so it won't work.
+# Solution below is not pretty but it fixes `lv_examples` without patching.
+#
+# Remove this fix when you will get better idea.
+
+LVGL_EXAMPLE_INCLUDE_FIX=$(APPDIR)/graphics/lvgl/lvgl
Review comment:
For this to work I added one more export in LVGL. Now LVGL can be included both with `lvgl/lvgl.h` and `lvgl.h` paths.
`CXXFLAGS` as well, right?
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455269862
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
Yes, I am using it in such way. Although I bypassed NuttX display driver because it was considerably slower at that time (NuttX 6.xx). Or I didn't know how to use it better.
NuttX framebuffer, as far as I understand, needs [width x height x color depth] amount of memory. It means that it requires external RAM. But LVGL don't have such limitation. It can draw almost directly with some few kilobyte buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455054469
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Why copy lvgl.h to exports directory? How about we export?
CFLAGS += ${shell $(INCDIR) $(INCDIROPT) "$(CC)" $(APPDIR)/graphics/lvgl}
CXXFLAGS += ${shell $(INCDIR) $(INCDIROPT) "$(CC)" $(APPDIR)/graphics/lvgl}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658934851
> Ok, it should be fine now. I have three more questions:
>
> * Is what I put into README correct? Regarding framebuffer device and direct drawing?
The description for framebuffer is good, but I am not sure thtat it's possible to do the direct drawing.
> * Where should I put deprecation note that users should use `lvgl/lvgl.h` instead of `graphics/lvgl.h` now?
Yes, I think we should follow the convention from the upstream project.
> * Can I fix other libraries that sit in `apps/include`?
The modulization is a good practice, but other people may has other ideal, you can send a query @dev.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658934851
> Ok, it should be fine now. I have three more questions:
>
> * Is what I put into README correct? Regarding framebuffer device and direct drawing?
The description for framebuffer is good, but I am not sure thtat it's possible to do the direct drawing.
> * Where should I put deprecation note that users should use `lvgl/lvgl.h` instead of `graphics/lvgl.h` now?
Yes, I think we should follow the convention from the upstream project.
> * Can I fix other libraries that sit in `apps/include`?
The modulization is always a good practice, but other people may has other ideal, you can send a query @dev.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I would also be in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying to add it few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
Anyway I think it is enough for this PR then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455118690
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Update: Acutally it will be `#include <lvgl/lvgl.h>` not `<lvgl.h>`, there was one more explicit include present in `lvgldemo` which I just noticed.
https://github.com/w8jcik/incubator-nuttx-apps/pull/1/commits/7f62736c771922aa8c8e6bd3d4976d083ef66ef8
But this is LittlevGL specific. The question remains, is it fine to drop prefixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658934851
> Ok, it should be fine now. I have three more questions:
>
> * Is what I put into README correct? Regarding framebuffer device and direct drawing?
The description for framebuffer is good, but I am not sure thtat it's possible to do the direct drawing.
> * Where should I put deprecation note that users should use `lvgl/lvgl.h` instead of `graphics/lvgl.h` now?
Yes, I think we should follow the convention from the upstream project.
> * Can I fix other libraries that sit in `apps/include`?
The modulization is a good practice, but other people may has other idle, you can send a query @dev.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455289170
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
No, this is not standard, I use it on my own code. I could propose it in a pull-request. I never did since I extended the LCD interface to accept drawing multiple rows at once (also due to overhead of individual putrun()), but I could leave that out. I would not add something to the README if it is not part of NuttX codebase itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455733108
##########
File path: examples/lvgldemo/Make.defs
##########
@@ -36,4 +36,6 @@
ifneq ($(CONFIG_EXAMPLES_LVGLDEMO),)
CONFIGURED_APPS += $(APPDIR)/examples/lvgldemo
+CFLAGS += ${shell $(DEFINE) "$(CC)" LV_LVGL_H_INCLUDE_SIMPLE}
Review comment:
It's better to move these into examples/lvgldemo/Makefile, only global setting should put here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 merged pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455103797
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Sure, but then we need to `#include <lvgl.h>` instead of `#include <graphics/lvgl.h>`. I don't mind but I though that this prefix is intentional to keep things within namespaces like `graphics` here. Do we want keep prefixes or do they go? So far they were used consistently.
Here is additional PR which shows necessary change to introduce this suggestion https://github.com/w8jcik/incubator-nuttx-apps/pull/1/files
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455134281
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Usually libraries assume some sort of layout for the include directory and the user only needs to account for installation prefix. In this case, lvgl is assuming a layout of `<prefix>/lvgl/lvgl.h`, so any extra component in the path should be part of the -I flag passed to the compiler. Adding extra prefixes could break existing programs following the expected inclusion format.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455694018
##########
File path: examples/lvgldemo/Makefile
##########
@@ -86,11 +86,18 @@ endif
MAINSRC = lvgldemo.c
-LVGL_SRC_DIR=$(APPDIR)/graphics/lvgl/lvgl
-LVGL_DIR=$(APPDIR)/graphics/lvgl
+# Upstream `lv_examples/lv_examples.h` includes LVGL
+# in following way `#include "../lvgl/lvgl.h"`, for no clean reason.
+#
+# The way that we export LVGL offers `#include <lvgl/lvgl.h>`, so it won't work.
+# Solution below is not pretty but it fixes `lv_examples` without patching.
+#
+# Remove this fix when you will get better idea.
+
+LVGL_EXAMPLE_INCLUDE_FIX=$(APPDIR)/graphics/lvgl/lvgl
Review comment:
For this to work I added one more export in LVGL. Now LVGL can be included both with `lvgl/lvgl.h` and `lvgl.h`.
`CXXFLAGS` as well, right?
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658890154
Ok, it should be fine now. I have three more questions:
* Is what I put into README correct? Regarding framebuffer device.
* Where should I put deprecation note that users should use `lvgl/lvgl.h` instead of `graphics/lvgl.h` now?
* Can I fix other libraries that sit in `apps/include`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik edited a comment on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik edited a comment on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-658890154
Ok, it should be fine now. I have three more questions:
* Is what I put into README correct? Regarding framebuffer device and direct drawing?
* Where should I put deprecation note that users should use `lvgl/lvgl.h` instead of `graphics/lvgl.h` now?
* Can I fix other libraries that sit in `apps/include`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently littlevgl is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all lvgl includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455561274
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
@v01d why not enhance the driver layer? @w8jcik is looking for a solution which mean this is a common requirement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455256739
##########
File path: examples/lvgldemo/Makefile
##########
@@ -86,11 +86,18 @@ endif
MAINSRC = lvgldemo.c
-LVGL_SRC_DIR=$(APPDIR)/graphics/lvgl/lvgl
-LVGL_DIR=$(APPDIR)/graphics/lvgl
+# Upstream `lv_examples/lv_examples.h` includes LVGL
+# in following way `#include "../lvgl/lvgl.h"`, for no clean reason.
+#
+# The way that we export LVGL offers `#include <lvgl/lvgl.h>`, so it won't work.
+# Solution below is not pretty but it fixes `lv_examples` without patching.
+#
+# Remove this fix when you will get better idea.
+
+LVGL_EXAMPLE_INCLUDE_FIX=$(APPDIR)/graphics/lvgl/lvgl
Review comment:
We can mdoify the inclusion to lvgl.h by LV_LVGL_H_INCLUDE_SIMPLE macro like this:
CFLAGS += ${shell $(DEFINE) "$(CC)" LV_LVGL_H_INCLUDE_SIMPLE}
Here is the related code: https://github.com/lvgl/lv_examples/blob/master/lv_examples.h#L16
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +76,15 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
-
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-context:: $(LVGL_UNPACKNAME) $(APPDIR)/include/graphics/lvgl.h
+context:: $(LVGL_UNPACKNAME) lvgl/lvgl.h
Review comment:
Remove lvgl/lvgl.h, don't need anymore.
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +76,15 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
-
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
Review comment:
Remove too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I am also be in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying to add it few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
Anyway, it should be enough for this PR then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently littlevgl is copying some header to `graphics`. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all lvgl includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik edited a comment on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik edited a comment on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-659336374
@xiaoxiang781216 Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently littlevgl is copying some header to `include/graphics` to be consistent with all NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all lvgl includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455271541
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
In my own use of littevgl in NuttX I actually draw using a custom LCD character driver to avoid the overhead of using the framebuffer->LCD emulation layer of NuttX (you can see the driver here: https://gitlab.com/nuttx_projects/lcd_dev).
Just FYI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455118690
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Update: Acutally it will be `#include <lvgl/lvgl.h>` not `#include <lvgl.h>`, there was one more explicit include present in `lvgldemo` `Makefile` which I just noticed.
https://github.com/w8jcik/incubator-nuttx-apps/pull/1/commits/7f62736c771922aa8c8e6bd3d4976d083ef66ef8
But this is LittlevGL specific. The question remains, is it fine to drop prefixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455694018
##########
File path: examples/lvgldemo/Makefile
##########
@@ -86,11 +86,18 @@ endif
MAINSRC = lvgldemo.c
-LVGL_SRC_DIR=$(APPDIR)/graphics/lvgl/lvgl
-LVGL_DIR=$(APPDIR)/graphics/lvgl
+# Upstream `lv_examples/lv_examples.h` includes LVGL
+# in following way `#include "../lvgl/lvgl.h"`, for no clean reason.
+#
+# The way that we export LVGL offers `#include <lvgl/lvgl.h>`, so it won't work.
+# Solution below is not pretty but it fixes `lv_examples` without patching.
+#
+# Remove this fix when you will get better idea.
+
+LVGL_EXAMPLE_INCLUDE_FIX=$(APPDIR)/graphics/lvgl/lvgl
Review comment:
For this to work I added one more export in LVGL. Now LVGL can be included both with `lvgl/lvgl.h` and `lvgl.h`. I don't like ambiguity but it seems that it was introduced by the author of library, so it follows.
`CXXFLAGS` as well, right?
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455694018
##########
File path: examples/lvgldemo/Makefile
##########
@@ -86,11 +86,18 @@ endif
MAINSRC = lvgldemo.c
-LVGL_SRC_DIR=$(APPDIR)/graphics/lvgl/lvgl
-LVGL_DIR=$(APPDIR)/graphics/lvgl
+# Upstream `lv_examples/lv_examples.h` includes LVGL
+# in following way `#include "../lvgl/lvgl.h"`, for no clean reason.
+#
+# The way that we export LVGL offers `#include <lvgl/lvgl.h>`, so it won't work.
+# Solution below is not pretty but it fixes `lv_examples` without patching.
+#
+# Remove this fix when you will get better idea.
+
+LVGL_EXAMPLE_INCLUDE_FIX=$(APPDIR)/graphics/lvgl/lvgl
Review comment:
For this to work I added one more export in LVGL. Now LVGL can be included both with `lvgl/lvgl.h` and `lvgl.h`. I don't like ambiguity but it seems that it was introduced by the author of library, so this follows.
`CXXFLAGS` as well, right?
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455295092
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
The topic of drawing multiple-lines comes back from time to time. I am also in favor of having such interface beacuse it seems to be a basic use case. Somebody was trying again to add it, few months ago, but I don't know where it ended up.
I understand, these READMEs seems to be more serious. For me README is just a way to communicate. If someone needs to know something and there are some solutions, README would be the place.
Anyway, it should be enough for this PR then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455118690
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Update: Acutally it will be `#include <lvgl/lvgl.h>` not `#include <lvgl.h>`, there was one more explicit include present in `lvgldemo` which I just noticed.
https://github.com/w8jcik/incubator-nuttx-apps/pull/1/commits/7f62736c771922aa8c8e6bd3d4976d083ef66ef8
But this is LittlevGL specific. The question remains, is it fine to drop prefixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455694145
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +76,15 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
-
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-context:: $(LVGL_UNPACKNAME) $(APPDIR)/include/graphics/lvgl.h
+context:: $(LVGL_UNPACKNAME) lvgl/lvgl.h
Review comment:
Done
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +76,15 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
-
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-659330572
> The modulization is always a good practice, but other people may has other ideal, you can send a query @dev.
I was particularly interested in LittlevGL right now. I might come back to this later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently littlevgl is copying some header to `include/graphics` to be consistent with all NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility. Breaking compatibility is fine for me. But what is the official strategy. Do we want `graphics` in front of all lvgl includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to have `#include <graphics/lvgl/...>`. The question is do we want it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455818230
##########
File path: graphics/lvgl/README.md
##########
@@ -0,0 +1,21 @@
+# Usage
+
+Import with `#include <lvgl/lvgl.h>`.
+
+Upstream example is present at `examples/lvgldemo`.
+
+LVGL can be used with framebuffer device. To find example boards with this
+preconfigured, search for `CONFIG_GRAPHICS_LVGL=y` in `defconfig` files.
+All of them have also `CONFIG_VIDEO_FB=y` present.
+
+As a second option, LVGL can talk to a display driver and explicitly draw line by line.
+For this case, there is no preconfigured board present. Go to _Porting_ section
+of upstream documentation for more hints.
Review comment:
At least, @w8jcik is also interesting your solution. We can talk about the detail in your PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#issuecomment-659336374
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes because it sits in graphics? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup @acassis, I also reminded myself branding change when writing this. Ok @xiaoxiang781216, I am going with making the life of programmer easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] w8jcik commented on a change in pull request #334: Don't copy headers outside of LittlevGL library
Posted by GitBox <gi...@apache.org>.
w8jcik commented on a change in pull request #334:
URL: https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper is copying some header to `include/graphics` to be consistent with other NuttX apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was asking should we really remove it, because this way we break compatibility and violate consistency. Breaking compatibility is fine for me. But what is the official guideline here. Do we want `graphics` in front of all LVGL includes? Right now it seems to be in front of one of the headers and probably not in front of the others. I think it should be still possible to make `#include <graphics/lvgl/...>` work. The question is do we want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup, I also reminded myself branding change when writing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org