Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add target_include_directories for lvgl #2713

Closed
wants to merge 1 commit into from

Conversation

JojoS62
Copy link
Contributor

@JojoS62 JojoS62 commented Oct 21, 2021

Description of the feature or fix

Better cmake integration for projects that use lvgl.
With target_include_directories, the lvgl path is emitted as include for compilers in projects that use lvgl. Otherwise, the projects must define where lvgl includes are located.
This follows https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1#declare-include-directories-with-target_include_directories
and is also use by zephyr already.

Checkpoints

@higaski
Copy link
Contributor

higaski commented Oct 21, 2021

Although technically correct I wouldn't use the INTERFACE keyword in this case since LVGL isn't an INTERFACE target.

The old version of the CMakeLists.txt file uses include_directories (bad practice) and the root project path (CMAKE_SOURCE_DIR) which obviously is wrong if you include it somewhere else. Both of these issues are already addressed by PR #2656. I therefor see no reason to merge this.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Oct 21, 2021

INTERFACE means here that the include path is exposed to users of the lib,, but it is not included for the own target. See also:
https://github.com/Kitware/CMake/blob/master/Help/guide/tutorial/Step4/MathFunctions/CMakeLists.txt

When it is used also internaly, then PUBLiC maybe better. But it is missing also for the MICROPY version. I'm using this for getting the interface lib. Otherwise, I would like to add a configuration for Mbed users.

@higaski
Copy link
Contributor

higaski commented Oct 21, 2021

With the exception of "lvgl.h" itself the library only uses relative paths as far as I know. The problem are libraries which depend on lvgl. Take lv_drivers for example. Most of its headers contain the following code

#ifdef LV_LVGL_H_INCLUDE_SIMPLE
#include "lvgl.h"
#else
#include "lvgl/lvgl.h"
#endif

Now your PR makes lvgl.h visible to consumers of lvgl but without the definition of LV_LVGL_H_INCLUDE_SIMPLE lv_drivers would still fail to compile because they would be looking for lvgl/lvgl.h.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Oct 21, 2021

ok, I wanted to avoid a lot of different cases for different target, but it seems these exist already.
I will close this PR and suggest an own case for Mbed, this makes is independt from other changes.
The usage of the static lib works also, but I have the problem that the resulting binary is about 100 kb larger than with the interface lib (as also MicroPython does).

@JojoS62 JojoS62 closed this Oct 21, 2021
@higaski
Copy link
Contributor

higaski commented Oct 21, 2021

How would Mbed differ from a "usual" CMake target? I can see how ESP, ZEPHYR and Micropython need to be treated differently, but Mbed? I mean sure, if it has to be. The current CMake situation needs to be changed anyhow. If needs be why not add a special Mbed target.

A 100kB larger binary just because of some missing inline opportunities... (static lib vs interface) that sounds like there is something wrong with your compiler settings? Either you forgot to turn optimizations on or the compiler isn't able to discard unused code for some reason...

@JojoS62
Copy link
Contributor Author

JojoS62 commented Oct 21, 2021

Mbed is not using special functions like zephyr or ESP, I can use the MicroPy version of interface lib with 'set(MICROPY_DIR true)'. The static lib works with simply add_subdirectory(). Only the target_include_directories was missing, but that will be included with your PR.
I have tried to figure out why the static lib build is much larger, compiler and linker options for optimization are set. The lib build contains modules that are removed in interface build.
There is some discussion in the Mbed Forum, the static libs are preferred over interface libs. So I really would like to use also the static built lib.

LANGUAGE_COMPILE_FLAGS = -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -fmessage-length=0 -fno-exceptions -ffunction-sections -fdata-sections -funsigned-char -fomit-frame-pointer -g3 -mthumb -mcpu=cortex-m4 -mfpu=fpv4-sp-d16 -mfloat-abi=softfp

Linker:
FLAGS = -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -fmessage-length=0 -fno-exceptions -ffunction-sections -fdata-sections -funsigned-char -fomit-frame-pointer -g3 -mthumb -mcpu=cortex-m4 -mfpu=fpv4-sp-d16 -mfloat-abi=softfp
  LINK_FLAGS = -Wl,-static     -Wl,--gc-sections  ... 

@higaski
Copy link
Contributor

higaski commented Oct 21, 2021

Have you tried turning optimizations on? (e.g. -Os)
Maybe even LTO? (-flto)

There is a great tool to analyze GCC map files out there called Amap. Maybe this can give you a clue.

@embeddedt
Copy link
Member

The static library needs to be compiled with -ffunction-sections -fdata-sections in order for the linker to discard the unused code.

Currently I don't think we pass any compiler-specific flags in the generic case, so that is probably not happening.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Oct 21, 2021

The static library needs to be compiled with -ffunction-sections -fdata-sections

yes, these are used. The above LANGUAGE_COMPILE_FLAGS are a from the build.ninja log.

There is a great tool to analyze GCC map files out there called Amap.

I will try it. Thanks for your help!

edit:
@embeddedt your clue seems to be right, -Os is missing for building the lib files.

Success!

this works now:

        add_subdirectory(${LIB_PATH}lvgl ${CMAKE_CURRENT_BINARY_DIR}/lvgl)
        target_compile_options(lvgl PRIVATE -Os)

thank you so much, I have spent a lot of time to figure it out, but I have started only 2 weeks ago configuring cmake.

@JojoS62 JojoS62 deleted the add_target_includes branch October 21, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants