tore-espressif commented 3 years ago

Refactoring proposal

Here is a summary of major/refactoring issues that must be tackled systematically in order to make this SW component maintainable and extandable:

Issues collection

Previous refactoring plans:
- Kconfig refactoring https://github.com/lvgl/lvgl_esp32_drivers/issues/40
- Long-term plans https://github.com/lvgl/lvgl_esp32_drivers/issues/44
LVGL v8 https://github.com/lvgl/lvgl_esp32_drivers/issues/68
Versioning https://github.com/lvgl/lv_port_esp32/issues/272
Multiple displays https://github.com/lvgl/lvgl_esp32_drivers/issues/31 https://github.com/lvgl/lv_port_esp32/issues/276 https://github.com/lvgl/lv_port_esp32/issues/277
New graphical interfaces (RGB, parallel) https://github.com/lvgl/lvgl_esp32_drivers/issues/64
Runtime configuration and Kconfig https://github.com/lvgl/lvgl_esp32_drivers/issues/65 https://github.com/lvgl/lv_port_esp32/issues/282
New Espressif devices https://github.com/lvgl/lv_port_esp32/issues/269

Proposal:

There are many hints in the issues' discussions that imply that the repo needs a major refactoring, so I'd like to present my high-level view of what and how should be done. Please don't take it as high-jacking or critique of your wonderful project, but rather as a willingness to support it :)

1. Abandon Kconfig

Nice article about What not to turn into Kconfig option in Zephyr's project documentation, most notably pin numbers.

In case of ESP-LVGL drivers it means all the options.

Many issues from users are about Kconfig configuration, and at the same time it gives us no benefit. On the contrary, it complicates our effort on any new feature, as it must be reflected in Kconfig.

Getting rid of Kconfig over runtime configuration would help us tackle issues in group 1, 4, 5, 6, 7 on top of a good design.

1.1. Eval board support

Support for evaluation boards (Wrover-kit, M5Stack...) is currently done only through Kconfig. As a replacement, examples of how to configure the drivers and LVGL will be provided in examples folder (see point 4).

2. Use `esp_lcd` component

esp_lcd is a component in ESP-IDF. Its intention is to abstract away various graphical interfaces that are introduced in new Espressif chips starting from ESP32. Namely RGB and parallel graphical interfaces. This component is maintained by Espressif, which would offload maintainer of this repo by taking care of peripheral drivers (not to be confused with display drivers) for newly released chips (ESP32-S3, ESP32-C3...).

There is also a rotation support, that is required in LVGL v8.

esp_lcd is written in a modular way, so it is possible to eventually carve out the display drivers and use them on different platforms. (I would leave this as a very last step).

esp_lcd can tackle issue in groups 5 and 7.

3. LVGL v8

Breaking changes compared to v7.

Nothing more to add, we have to support this.

4. Folder structure

Currently there are two repositories which serve similar purpose lvgl/lvgl_esp32_drivers and lvgl/lv_port_esp32. This causes a lot of confusion as the users don't know which repo is responsible for what and are often firing issues at wrong place.

I'd like to follow an idea: Single repo = single purpose.

lv_port_esp32 would be discontinued and its (revised) content moved to lvgl_esp32_drivers/examples subfolder.

lvgl_esp32_drivers folder structure:

lvgl_esp32_drivers
├───tft
├───touch
├───(esp) - to isolate esp-specific stuff?
└───examples

5. Versioning

All proposed changes above are breaking changes and there is no versioning or release cycle for esp_lvgl drivers.

I'm personally for rather slack-off option of tagging current state with version e.g. 0.0.1 which would be only bugfixed in the future. And start a new branch with new refactored drivers e.g. 0.1.0.

But I'd like to hear your opinion on this too.

tore-espressif commented 3 years ago

@C47D @embeddedt @kisvegabor please leave your comments on this. So far it is only a high-level plan, that needs to be agreed on before the heavy-lifting on the codebase begins. I'll prepare an example to give you something more tangible next week.

Looking forward to hearing from you.

C47D commented 3 years ago

@tore-espressif Thanks for the summary, the lv_port_esp32 repo (as I see it) is a project to showcase LVGL in ESP32 microcontrollers. The ultimate goal of lvgl_esp32_drivers is to make the drivers (both display and indev) platform agnostic and move them into lv_drivers, if we move all the ESP specific stuff into another directory we should be able to do this more easily.

Folder structure

lvgl_esp32_drivers
├───tft
├───touch
├───(esp) - to isolate esp-specific stuff?
└───examples

I would like to rename touch to indev and tft to display, because there's also monochrome displays in there.

Abandon Kconfig

I also see a lot of issues with the Kconfig, specially when configuring the drivers, I would like to be able to configure the displays manually, so we can support multi-display configurations, etc.

What do you think?

tore-espressif commented 3 years ago

lv_port_esp32 ... a project to showcase LVGL in ESP32

And it will stay like that for a while. After we finish with the refactoring, we can think again whether it makes sense to have two repos for one platform.

would like to rename touch to indev and tft to display

Agreed.

I would like to be able to configure the displays manually, so we can support multi-display configurations, etc.

By 'manually' you mean runtime? Yes, that's exactly the idea. I think it is going to be clear after I post an example.

C47D commented 3 years ago

By 'manually' you mean runtime? Yes, that's exactly the idea. I think it is going to be clear after I post an example.

Yes, at runtime. An example would be great 😸 . The current CMakeLists.txt in this repo depends on the symbols generated by the menuconfig, can we also consider updating CMakeLists.txt.

ropg commented 3 years ago

I'm new to LVGL and more of a jack-of-all-trades than highly experienced in any of the languages/environments I touch. But a few things:

Aaargh. I just wrapped my head around Kconfig and now you're telling me that was all for nothing? 😉
- Given the special status of I2C (and SPI) as potentially being shared by multiple ESP-IDF components outside of LVGL, we could consider exempting the underlying port parameters from the "kill Kconfig' principle. I2C Manager is already there and very easy to use from the drivers. All that then potentially needs to be configured on the lvgl_esp32_drivers side of things is whether I2C port 0 or 1 is to be used, if both are configured in I2C Manager.
- Otherwise this would mean me reworking my I2C Manager proposal (#70). It is really built around Kconfig as that is the ESP-IDF way and it is also meant for other things to use (so we get thread-safety for I2C). I can't presently oversee how the new way would work and how to interface to it, but if need be I'm sure I'll figure something.
esp_lcd

esp_lcd is written in a modular way, so it is possible to eventually carve out the display drivers and use them on different platforms. (I would leave this as a very last step).

I don't know esp_lcd, but I am very familiar with ESP-ADF, the Espressif Audio Development Framework. Must say I was not that impressed (examples). Again, I don't know esp_lcd, but I recommend having a critical look to see if it really does everything we need the way we need it before tying our fate to it.
ESP specific

One big advantage to getting rid of Kconfig is that what replaces it will be more generic, right?. Invariably, the way esp_lcd works will influence how we do things, reintroducing ESP-specificity. Does this not conflict with the broader goal of making things more and more platform-agnostic?
Shared code
- My proposed changes in #70 "Replace all LVGL driver I2C code with I2C Manager" introduce code shared between tft and touch. This code "prefers" to live in 'i2c_manager' under the root because it needs to be included from driver code and not be in INCLUDE_DIRS (see #70 for details).
- SPI would potentially also benefit from having shared code, possibly also between multiple ESP-IDF components) that knows about GPIOs and stuff instead of bits and pieces all over the various drivers. Something analogous to I2C Manager could be built.

ropg commented 3 years ago

Just noticed I didn't answer one of your questions:

what are the long term plans with https://github.com/ropg/i2c_manager ?

I don't think it needs to do that much more than it presently does. I do plan to emulate i2cdev, a competing thread-safe I2C thing because there's lots of drivers for that one, but support would be in a separate file. Also there are (older) ICs that do not use an address, I need to support that. Can't think of anything more it could possibly do.

embeddedt commented 3 years ago

I think there's nothing wrong with using Kconfig to turn features on/off (like whether the driver for a certain display is compiled or not), but it should not be used for specific options like which pins to use, because it's not a dynamic configuration language and it can't support an arbitrary display setup easily.

ropg commented 3 years ago

I agree when it comes to displays because of multiple display setups etc. There you transcend the limits of Kconfig and it's all internal to LVGL. But I beg to differ in the specific case of I2C: multiple ESP-IDF components use the same port(s), configuration is extremely straightforward and Kconfig is, IMHO, the proper way to configure resources shared between components.

Have a look at I2C Manager the way I've proposed it now (#70). The way we interface with it may change: if we do not use Kconfig to configure drivers, we'll need some other way to configure which of the two I2C ports a driver uses. Normally that would then include pins, speed, etc. (With all the problems that come with that: only the first component's port initialization actually works, the rest error.) The only difference if we use I2C Manager is that pins, speed, etc would now be abstracted away, giving us thread-safety and more modular, less duplicated code.

kisvegabor commented 3 years ago

As it was already mentioned one of the long-term goals is to have a platform-agnostic driver library emerged from lvgl_esp32_drivers. I think we should view the questions in light of this goal too.

@C47D already made some experiments with the hardware interface.

So we need to figure out what level of hardware settings we want to support:

Very low level: pin config, mapping, periphery init and settings
- can be very complex for different vendors
- usually done by UI tools (CubeMX, MCUXpresso) or menuconfig
- IMO it'd be too much to reimplement all vendors' all MCUs' configs
  1. Use configured peripheries
- Implement only e.g. lv_drv_spi_transfer which calls an ESP, STM, NXP or any other SDK/HAL API function
- We can assume that the SDK/HAL library is part of the project anyway and we can use it.
- User selects e.g. SPI2 for TFT and I2C3 for touch.
- It's much easier to handle for us but more work for the user

I find 2) much more reasonable.

Abandon Kconfig

With the 2nd point above it seems possible to use Kconfig but I'm not experienced with Kconfig enough to make a suggestion about this topic.

Eval board support

It'd be great if supporting eval boards would mean "only" examples because we killed two birds with one stone

examples
board support

Besides adding a new example is probably easier than hardwiring a new board.

Use esp_lcd component

I like the idea. We can reuse other existing driver libs too such as TFT_eSPI . So I think the goal here is to prepare the system to handle external driver libs.

Folder structure

I assume in the end there will be functions like lv_drv_set_gpio, lv_drv_spi_transfer, etc. Where to place the vendor-specific codes? As they are related to both the displays and indevs I can imagine something like this:

lvgl_esp32_drivers
├───display
├───indev
├───vendor
│   ├ esp
│   ├ nxp
│   └ stm
└───examples

Versioning

To follow semver I'd rather call the current state 1.0.0 and the refactored version 2.0.0 to imply that the new version is a breaking change.

C47D commented 3 years ago

The first issue on this repo #1 pointed out a lot of points we should consider, the indev drivers being aware of display rotation is something I would like to add to the current set of drivers and that's why I asked LVGL to have rotation awareness, in #1 an struct with both display and indev drivers were suggested, we might revisit the ideas there and consider (if any) the suggested options.

kisvegabor commented 3 years ago

@C47D do you mean the points listed in this comment? https://github.com/lvgl/lvgl_esp32_drivers/issues/1#issuecomment-652948156

tore-espressif commented 3 years ago

I find 2) much more reasonable.

So do I.

I'd suggest keeping one repo for the specific vendor's port and one for platform-agnostic drivers. Something like this for ESP:

lvgl_esp32_drivers
├───lv_drivers (submodule)
│    ├───display
│    └───indev
├───esp (implementation of Espressif's port)
└───examples

lv_drivers would contain

display/indev drivers common for all vendors
header file defining an interface that must be implemented by each vendor's port

Apart from the folder structure I agree with https://github.com/lvgl/lvgl_esp32_drivers/issues/72#issuecomment-865256557.

I requested some changes in esp_lcd to make it easier to port it to platform-agnostic drivers, so the example I promised will be a little later.

kisvegabor commented 3 years ago

I started to reply to your proposed directory structure but meanwhile something came to my mind. esp_lcd already contains a high level abstraction of the displays. So a vendor-specific library can handle the whole display. It's the same for TFT_eSPI.

So far my view was that the display folder contains something like that:

//ili9341.c

ili9341_flush_cb(lv_disp_drv_t * drv, lv_area_t * area, lv_color_t * colors)
{
   io_clear(PIN_DC);
   spi_send(DISPLAY_SPI, set_window);
   io_set(PIN_DC);
   spi_send(DISPLAY_SPI, colors);
   lv_disp_flush_ready(drv);
}

where io_clear and io_set and spi_send are coming from the vendor libs. However, if the lib can handle the display we can

ili9341_flush_cb(lv_disp_drv_t * drv, lv_area_t * area, lv_color_t * colors)
{
   disp_flush(area, colors);
}

But I think both need to be supported. That is once we write an ILI9341 driver on SPI level it should work with all the vendors if the required IO and SPI functions are implemented. But if a lib supports that display directly we should use the provided high-level functions to handle the display.

I'm sure we can find a smart way to automatically see if there is a handler for display-level management and if not try using HAL functions.

Do you agree with this?

C47D commented 3 years ago

@kisvegabor Based on your code snippet I think we could add:

lv_disp_drv_t * drv parameter to functions that 'talk' to the hardware.
Avoid using names such as spi_send because some displays have support for multiple interfaces, like parallel, I2C, etc.
The user should be responsible of calling lv_disp_flush_ready, we currently call it on a transfer done callback.

#include "lvgl_bsp.h"

ili9341_flush_cb(lv_disp_drv_t * drv, lv_area_t * area, lv_color_t * colors)
{
   io_write(drv, PIN_DC, 1);
   interface_send(drv, set_window, window_len);
   io_write(drv, PIN_DC, 0);
   interface_send(drv, colors, colors_len);
   lv_disp_flush_ready(drv);
}

void io_write(lv_disp_drv_t * drv, uint32_t pin, uint32_t state)
{
     bsp_io_write(drv, pin, state);
}

void interface_write(lv_disp_drv_t * drv, void *data, size_t len)
{
     bsp_if_write(drv, data, len);
}

Both bsp_io_write and bsp_if_write are declared in lvgl_bsp.h but implemented by the user (they should include them in their project compilation setup), in this repo they could be implemented in the esp specific directory. The first parameter lv_disp_drv_t can be used in case of having multiple displays. We could also have bsp_if_write_async for DMA or interrupt driven transfers.

What do you think?

kisvegabor commented 3 years ago

Sorry for the late reply.

I like the idea of a higher-level API function (interface instead of spi).

In your code snippet interface_send is a typo and should be interface_write?

But what's the point of wrapping bsp_if_write to interface_write? What is the added value?

So using the lv_drivers repo the user

gets the init codes of ILI9341 with abstract interface calls
still needs an SPI/parallel driver
- Will we provide bsp_if_write implementations for common platforms? E.g. select SPI index, speed, mode, DMA etc on ESP, NXP, STM, etc platforms?

If lv_disp_flush_ready is called in ili9341_flush_cb it will always work in blocking mode (no double buffering can be used). Maybe we need a ready_cb parameter in interface_write or bsp_if_write_async and call lv_disp_flush_ready there?

If bsp_if_write is used for interacting with the display shouldn't there be bsp_disp_write and bsp_indev_write instead?

@tore-espressif Can we integrate esp-lcd into this model?

C47D commented 3 years ago

In your code snippet interface_send is a typo and should be interface_write?

Yes, it's a typo.

But what's the point of wrapping bsp_if_write to interface_write? What is the added value?

Didn't leave any note about that back then, totally forgot about the purpose 😅

gets the init codes of ILI9341 with abstract interface calls

Yes, it will call bsp functions.

Will we provide bsp_if_write implementations for common platforms? E.g. select SPI index, speed, mode, DMA etc on ESP, NXP, STM, etc platforms?

I don't know what's easier, provide the code, leave the users implement it, provide it and allow the user optimize the functions according to their needs (maybe marking the provided functions as weak).

If lv_disp_flush_ready is called in ili9341_flush_cb it will always work in blocking mode (no double buffering can be used). Maybe we need a ready_cb parameter in interface_write or bsp_if_write_async and call lv_disp_flush_ready there?

I would personally leave the implementation to the user, not try to implement every possibility.

If bsp_if_write is used for interacting with the display shouldn't there be bsp_disp_write and bsp_indev_write instead?

Yes, I agree on that.

kisvegabor commented 3 years ago

I don't know what's easier, provide the code, leave the users implement it, provide it and allow the user optimize the functions according to their needs (maybe marking the provided functions as weak).

The user always can select no platform and use custom implementations. I suggest going for no platform-specific functions for first as weak can add them later.

AFAIK weak is GCC specific.

I would personally leave the implementation to the user, not try to implement every possibility.

As drv is also provided as a parameter it's also ok.

ropg commented 3 years ago

I was thinking about something said here:

As a replacement, examples of how to configure the drivers and LVGL will be provided in examples folder [...]

I think it would be amazing to have a small but complete working example project for each supported piece of hardware, and it's really much better than all these Kconfig settings. We seem to have concluded that I2C Manager is part of this future. It's probably obvious, but I would recommend just putting an sdkconfig.defaults file in the example folder for a given piece of hardware. Like for instance mine has:

CONFIG_I2C_MANAGER_0_ENABLED=y
CONFIG_I2C_MANAGER_0_SDA=21
CONFIG_I2C_MANAGER_0_SCL=22
CONFIG_I2C_MANAGER_0_FREQ_HZ=1000000

It could also contain other device-specific things that are not part of LVGL, such as ESPTOOL settings and whatever else is needed to make the basic example run on the device, so the file is needed anyway for everything to 'just work'. And nothing quite motivates to use something as relatively complex as LVGL like having a running example on your own hardware.

As an aside: I find the way examples are done in LVGL a bit overly complex. We might may want to agree on something that is used in all or most of the configuration examples in this repository, and we may want something that is very simple and not something that demonstrates all the cool LVGL bling and distracts from how LVGL is configured for a specific piece of hardware.

C47D commented 3 years ago

Just saw this blog post and maybe we can use some of their ideas for our branches https://blog.stratifylabs.co/device/2021-07-12-Semantic-Versioning-and-Github/

kisvegabor commented 3 years ago

Just saw this blog post and maybe we can use some of their ideas for our branches

This is how the lvgl repo is versioned now :slightly_smiling_face: I found it under the name of GitLab flow: https://docs.gitlab.com/ee/topics/gitlab_flow.html#release-branches-with-gitlab-flow

C47D commented 3 years ago

What do you think about using it in here? branch v7 for LVGL v7.x, branch v8 for LVGL v8.x and master or development for working. Maybe you have better names tho.

kisvegabor commented 3 years ago

I agree to use the same branching here too. IMO release/v7, release/v8.0, release/v8.1 are good and conventional names.

C47D commented 2 years ago

Folder structure

Currently there are two repositories which serve similar purpose lvgl/lvgl_esp32_drivers and lvgl/lv_port_esp32. This causes a lot of confusion as the users don't know which repo is responsible for what and are often firing issues at wrong place.

I'd like to follow an idea: Single repo = single purpose.

lv_port_esp32 would be discontinued and its (revised) content moved to lvgl_esp32_drivers/examples subfolder.

lvgl_esp32_drivers folder structure:

lvgl_esp32_drivers ├───tft ├───touch ├───(esp) - to isolate esp-specific stuff? └───examples

I have some questions regarding this in regards of including the display and indev drivers in LVGL:

Would the LVGL examples be able to run in this project? Or are we talking about having some ESP32 specific examples?
Would we add the lv_port espressif specific files into LVGL or here?

I would also want to change the driver directories names, from tft to display and touch to indev, as that is the names being used in the LVGL documentation. What do you think @tore-espressif? I could do it in the develop branch.

kisvegabor commented 2 years ago

Would the LVGL examples be able to run in this project? Or are we talking about having some ESP32 specific examples?

I think "examples" mean HW config examples.

Would we add the lv_port espressif specific files into LVGL or here?

Ultimately, I think we should have all drivers in LVGL. This way lv_port espressif would be a demonstration for configuring LVGL for ESP32 (selecting the drivers, platform, board, or whatever we find out to describe hardware) and show how to initialize LVGL, the driver, and open a demo or example UI.

tore-espressif commented 2 years ago

Would the LVGL examples be able to run in this project? Or are we talking about having some ESP32 specific examples?

I think "examples" mean HW config examples.

Would we add the lv_port espressif specific files into LVGL or here?

Ultimately, I think we should have all drivers in LVGL. This way lv_port espressif would be a demonstration for configuring LVGL for ESP32 (selecting the drivers, platform, board, or whatever we find out to describe hardware) and show how to initialize LVGL, the driver, and open a demo or example UI.

Exactly. I think we already agreed on the dispaly/indev folder names.

In the light of our recent discussions, this proposal is rather obsolete...

C47D commented 2 years ago

Ok, so can we close this issue?

I think we missed writing a public minute of the meeting we had.

kisvegabor commented 2 years ago

I think we missed writing a public minute of the meeting we had.

More or less this comment should serve as a summary of the discussed things.

lvgl / lvgl_esp32_drivers

Design proposal #72

Refactoring proposal

Issues collection

Proposal:

1. Abandon Kconfig

1.1. Eval board support

2. Use `esp_lcd` component

3. LVGL v8

4. Folder structure

5. Versioning

lvgl / lvgl_esp32_drivers

Design proposal #72

Refactoring proposal

Issues collection

Proposal:

1. Abandon Kconfig

1.1. Eval board support

2. Use esp_lcd component

3. LVGL v8

4. Folder structure

5. Versioning

2. Use `esp_lcd` component