Separate lv_conf.h into 2 files: vendor specific + application specific

kisvegabor commented 7 months ago

Problem to solve

Continue https://github.com/lvgl/lvgl/issues/5624 As it was shown in https://github.com/lvgl/lvgl/pull/5614 and https://github.com/lvgl/lvgl/issues/5391 adding vendor/Platform specific default configs can be useful.

Success criteria

The solution should be backward compatible.
The same lv_conf.h should work on PC and an embedded device too.
Kconfig needs to be supported.

Solution outline

Keep lv_conf.h but move the followings to lv_platform_conf.h:

LV_CONF_SKIP in Kconfig: I think we can set it to y unconditionally as we can assume that if there is Kconfig there is no lv_conf.h
LV_USE_STDLIB_*: Some might want to use Clib or custom STDLIB implementations
LV_USE_DRAW_SW_ASM settings
Profiler config (Not in the config files now)
LV_USE_OS
LV_DRAW_BUF_STRIDE_ALIGN to match the requirements of GPUs by default
LV_USE_DRAW_<GPU>: enable automatically
All LV_ATTRIBUTE_*s
Drivers, e.g. LV_USE_SDL
Enable some platform specific libraries: e.g. lodepng vs libpng
Anything else?

lv_conf.h should start with including lv_platform_conf.h. If it's not included LVGL will fall back to the default values in lv_conf_internal.h. lv_conf_internal.h should be generated from lv_conf_template.h and lv_platform_conf.h.

Rabbit holes

Shall we have the platform config options in Kconfig too, or the vendors will set it in lv_platform_conf.h file?
Carefully review what configs to add to the new config file.

Testing

Testing on ESP/PC should cover most of the cases.

Teaching

Update the docs and example projects

Considerations

No response

kisvegabor commented 7 months ago

cc @tore-espressif @espzav, @MouriNaruto

nicusorcitu commented 7 months ago

@cristian-stoica

sukesh-ak commented 7 months ago

Problem to solve

Continue #5624 As it was shown in #5614 and #5391 adding vendor/Platform specific default configs can be useful.

Success criteria

The solution should be backward compatible.

The same lv_conf.h should work on PC and an embedded device too.

Kconfig needs to be supported.

Solution outline

Keep lv_conf.h but move the followings to lv_platform_conf.h:

This is a wrong assumption to make

if there is Kconfig there is no lv_conf.h

This assumption is causing the issue as I mentioned here

LV_CONF_SKIP in Kconfig: I think we can set it to y unconditionally as we can assume that if there is Kconfig there is no lv_conf.h

I think the solution should be slightly different.

Default case where internal LVGL defaults are used

CONFIG_LV_CONFIG=LV_CONFIG_DEFAULT

Where custom lv_conf.h should be used

CONFIG_LV_CONFIG=LV_CONFIG_CUSTOM

You can also expand this to platform defaults

CONFIG_LV_CONFIG=LV_CONFIG_WINDOWS

kisvegabor commented 6 months ago

@sukesh-ak I think it's a different issue, but we can handle with this one.

In your suggestion people still need to go to menuconfig to set CONFIG_LV_CONFIG, right?

sukesh-ak commented 6 months ago

@sukesh-ak I think it's a different issue, but we can handle with this one.

In your suggestion people still need to go to menuconfig to set CONFIG_LV_CONFIG, right?

Nope, we don't need to goto menuconfig since we can set in the sdkconfig.default https://github.com/sukesh-ak/BSP-IDF5-ESP_LCD-LVGL9/blob/main/sdkconfig.defaults

kisvegabor commented 6 months ago

Oh, I see. If it's ok to edit sdkconfig.default by hand, it's ok for me too. :slightly_smiling_face:

kisvegabor commented 2 months ago

I've just realized that my comment from yesterday wasn't sent... :cry:

So, I believe there are 3 different configs:

1. Framework config

The user shouldn't touch it
No Kconfig needed
E.g. ESP, MicroPython, UEFI can have it's own config supplied by the vendor or by us

These belong here:

#define LV_USE_STDLIB_MALLOC    LV_STDLIB_BUILTIN
#define LV_USE_STDLIB_STRING    LV_STDLIB_BUILTIN
#define LV_USE_STDLIB_SPRINTF   LV_STDLIB_BUILTIN
#define LV_STDINT_INCLUDE       <stdint.h>
#define LV_STDDEF_INCLUDE       <stddef.h>
#define LV_STDBOOL_INCLUDE      <stdbool.h>
#define LV_INTTYPES_INCLUDE     <inttypes.h>
#define LV_LIMITS_INCLUDE       <limits.h>
#define LV_STDARG_INCLUDE       <stdarg.h>
#define LV_BIG_ENDIAN_SYSTEM 0
#define LV_ATTRIBUTE_TICK_INC
#define LV_ATTRIBUTE_TIMER_HANDLER
#define LV_ATTRIBUTE_FLUSH_READY
#define LV_ATTRIBUTE_MEM_ALIGN_SIZE 1
#define LV_ATTRIBUTE_MEM_ALIGN
#define LV_ATTRIBUTE_LARGE_CONST
#define LV_ATTRIBUTE_LARGE_RAM_ARRAY
#define LV_ATTRIBUTE_FAST_MEM
#define LV_EXPORT_CONST_INT(int_value) struct _silence_gcc_warning  /**< The default value just prevents GCC warning */
#define LV_ATTRIBUTE_EXTERN_DATA

2. Hardware config

GPU and driver-related
Can be configured by the user
Compile time checks can ensure that the configs are sane. E.g. LV_DRAW_BUF_STRIDE_ALIGN based on the selected GPU

Related configs:

#define LV_DRAW_<GPU/ASM/etc>
#define LV_USE_<SDL/ILI9341/etc>

3. Application specific

the rest: widget, layout, demo, example, theme, memory size, debug, log, etc

In light of that, we can have an lv_env_conf.h file where "1. Framework config" can be adjusted and the normal lv_conf.h could have:

#include "lv_env_conf_internal.h"

And in lv_env_conf_internal.h we can have


#ifdef ESP
#include "lv_env_conf_esp.h"
#endif 

#ifdef MICROPYTHON
#include "lv_env_conf_mp.h"
#endif 

#ifdef UEFI
#include "lv_env_conf_uefi.h"
#endif 

#ifdef ZEPHYR
#include "lv_env_conf_zephyr.h"
#endif 

#ifdef LV_ENV_CONF_CUSTOM_PATH
#include LV_ENV_CONF_CUSTOM_PATH
#endif

What do yu think? @tore-espressif @espzav @faxe1008

kisvegabor commented 2 months ago

cc @MouriNaruto

cristian-stoica commented 2 months ago

@kisvegabor Historically, our SDK team used to (and do) add a line #include "display_support.h" at the top of lv_conf.h.

That header lets the user choose a display and sets macros for LVGL that are otherwise irrelevant for an application writer: e.g.

#define LV_ATTRIBUTE_MEM_ALIGN         PLATFORM_MEMALIGN
#define LV_DRAW_BUF_STRIDE_ALIGN        PLATFORM_STRIDE_ALIGN
#define LV_COLOR_DEPTH                DEMO_COLOR_DEPTH
etc.

In this new scheme which config file would be most appropriate for the SDK engineers to add their #include ?

nicusorcitu commented 2 months ago

@cristian-stoica Exactly this is the reason to split the config. Our platform needs will go to the platform config file.

Hi @kisvegabor I am glad we get back to this topic. I suggest to:

keep one generic config: lv_config.h with everything except 2
create an addition config: lv_platform_config.h and agree with the other vendors what needs to be moved in

From my point of view I would say 2 should have everything from:

/*====================
   COLOR SETTINGS
 *====================*/

/*====================
   HAL SETTINGS
 *====================*/

/*=================
 * OPERATING SYSTEM
 *=================*/

/*========================
 * RENDERING CONFIGURATION
 *========================*/

/*=====================
 *  COMPILER SETTINGS
 *====================*/

kisvegabor commented 2 months ago

@nicusorcitu

LV_COLOR_DEPTH depends on the actual hardware, so it's not something that can be set the NXP once, and the user shouldn't touch aynmore.
Same for HAL settings. These can be configured by the user according to the needs of the given project
Operating system can be really part of lv_platform_config.h
Render configurations: It's up to the user to select how many CPU cores to use and which GPUs to enable. E.g. someone might disable PXP to use it only for copying the rendered image to the frame buffer with rotation.
Compiler settings: I agree to move these to lv_platform_config.h

nicusorcitu commented 2 months ago

What is the purpose of this separation? Is not to have an application config that will stay unchanged due to multiple HW setups?

All the above IMO should be platform specific. Not vendor, but platform specific. NXP, for example, will have different versions of platform specific configurations for different NXP platforms.

The key with this separation is not to separate vendors NXP, Xiaomi, Renesas, but somehow to separate the lvgl platform config to the lvgl application config.

For lv_platform_config.h:

COLOR SETTINGS: LV_COLOR_DEPTH is mandatory, this will be set based on different display
HAL_SETTINGS: LV_DEF_REFR_PERIOD should match the display refresh rate
OPERATING SYSTEM: for LV_USE_OS
RENDERING CONFIGURATION: for GPU draw enablement or SW draw configuration - which is also platform specific (one platform can work GPU only, others can have a dual core CPU so 2 SW draw units)
COMPILER SETTINGS: for LV_ATTRIBUTE_***

kisvegabor commented 2 months ago

The key with this separation is not to separate vendors NXP, Xiaomi, Renesas, but somehow to separate the lvgl platform config to the lvgl application config.

I think both. See this comment.

Letting the vendors provide a "framework config" is great, as they can pre-configure a few things for their environment. However, IMO from this point the user land comes to configure

LVGL according to the given hardware (MCU capabilities, display spec, etc)
LVGL according to the given application (widgets, theme, etc)

We agree that 2) should be done by the user, however I believe 1) should remain in the hands of the user as well. As e.g. NXP can't decide whether the user wan't to use PXP during rendering or only for flushing. The vendor can't disable the unused LV_DRAW_SW_SUPPORT_RGB888/RGB565/etc options either.

In order to make lv_conf.h platform independent the user can do this:

#define SIMULATOR   1

#define LV_USE_ASSERT_OBJ  SIMULATOR
#define LV_DRAW_USE_PXP  (!SIMULATOR)

We can even promote this practice in lv_conf_template.h.

What do you think?

I still don't know how it applies to Kconfig.

nicusorcitu commented 2 months ago

Are you suggesting 3 config files?

kisvegabor commented 1 month ago

Nope, I mean 2 files:

lv_platform_config
- LV_ATTRIBUTE_*
- LV_USE_STD*_
- Maybe LV_USE_OS
- lv_conf.h
- Same as now just start promoting how to make it portable among desktop and embedded (see #define SIMULATOR here)

lvgl-bot commented 1 month ago

We need some feedback on this issue.

Now we mark this as "Abandoned" because there was no activity here for 14 days.

Remove the "Stale" label or comment else this will be closed in 7 days.

lvgl / lvgl