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
Design proposal #72
Comments
@C47D @embeddedt @kisvegabor please leave your comments on this. Looking forward to hearing from you. |
@tore-espressif Thanks for the summary, the
I would like to rename
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? |
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.
Agreed.
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 |
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:
|
Just noticed I didn't answer one of your questions:
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. |
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. |
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. |
As it was already mentioned one of the long-term goals is to have a platform-agnostic driver library emerged from @C47D already made some experiments with the hardware interface. So we need to figure out what level of hardware settings we want to support:
I find 2) much more reasonable.
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.
It'd be great if supporting eval boards would mean "only" examples because we killed two birds with one stone
Besides adding a new example is probably easier than hardwiring a new board.
I like the idea. We can reuse other existing driver libs too such as
I assume in the end there will be functions like
To follow semver I'd rather call the current state |
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. |
@C47D do you mean the points listed in this comment? |
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:
Apart from the folder structure I agree with #72 (comment). I requested some changes in |
I started to reply to your proposed directory structure but meanwhile something came to my mind. So far my view was that the //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 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? |
@kisvegabor Based on your code snippet I think we could add:
#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 What do you think? |
Sorry for the late reply. I like the idea of a higher-level API function ( In your code snippet But what's the point of wrapping So using the
If If @tore-espressif Can we integrate esp-lcd into this model? |
Yes, it's a typo.
Didn't leave any note about that back then, totally forgot about the purpose 😅
Yes, it will call bsp functions.
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
I would personally leave the implementation to the user, not try to implement every possibility.
Yes, I agree on that. |
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
As |
I was thinking about something said here:
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
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. |
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/ |
This is how the lvgl repo is versioned now 🙂 |
What do you think about using it in here? branch |
I agree to use the same branching here too. IMO |
I have some questions regarding this in regards of including the display and indev drivers in LVGL:
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. |
I think "examples" mean HW config examples.
Ultimately, I think we should have all drivers in LVGL. This way |
Exactly. I think we already agreed on the dispaly/indev folder names. In the light of our recent discussions, this proposal is rather obsolete... |
Ok, so can we close this issue? 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. |
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
Unable to use multiple display types. #31
Using two GC9A01 multiple displays at the same time lv_port_esp32#276
A few questions about setting up a project via menuconfig lv_port_esp32#277
Support of 8 / 16 bit parallel bus for ESP32 #64
Allow for manual offset setting, and automatic swapping of resoultion and offsets when changing screen orientation #65
ESP32S2 can't change display DC pin to gpio45 in vscode by using SDK configureation editor lv_port_esp32#282
No support for ESP32C3 lv_port_esp32#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
componentesp_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
andlvgl/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 tolvgl_esp32_drivers/examples
subfolder.lvgl_esp32_drivers
folder structure: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.
The text was updated successfully, but these errors were encountered: