Replace all LVGL driver I2C code with I2C Manager

[ropg]

This PR integrates my I2C Manager code into lvgl_esp32_drivers. It replaces various bits and pieces of I2C code (which was a bit ... ehm ... organically grown) with one code-base for all I2C comms. This makes everything more modular and more readable. I2C manager is thread-safe, which is important when others, from other threads, want frequent access to I2C while LVGL needs to talk to a touchpad or display many times a second, because I2C using ESP-IDF is not thread-safe.

Single submenu

There is now one submenu for the drivers that has a TFT, a Touch and optionally an I2C submenu:

menu "LVGL ESP Drivers"

    rsource "lvgl_tft/Kconfig"

    rsource "lvgl_touch/Kconfig"

    rsource "i2c_manager/Kconfig"

endmenu

 

Below is the README file that is in the i2c_manager subdirectory.

Information for users

I2C Manager support

lvgl_esp32_drivers comes with built-in I2C support by integrating I2C Manager, which is used in case your touch interface or screen uses the I2C bus. The native I2C support offered by ESP-IDF is not thread-safe. Maybe you use LVGL with a touch sensor that has an i2c port, and maybe your device also has another i2c device that needs to be read frequently, such as a 3D-accelerometer. If you read that from another task than the lvgl uses to read the touch data, you need some kind of mechanism to keep these communications from interfering.

If you have other components that can use I2C Manager (or Mika Tuupola's I2C HAL abstraction that I2C Manager is compatible with) then put I2C Manager in your components directory by cloning the repository from below and in your main program do:

#include "i2c_manager.h"
#include "lvgl_helpers.h"

[...]

lvgl_locking(i2c_manager_locking());
lv_init();
lvgl_driver_init();

The lvgl_locking part will cause the LVGL I2C driver to play nice with anything else that uses the I2C port(s) through I2C Manager.

See the I2C Manager GitHub repository for much more information.

 

Information for driver developers

I2C support in the LVGL ESP drivers is provided exclusively by the files in this directory. Code from all over the project that was talking to the I2C hardware directly has been replaced by code that communicates through the functions provided in lvgl_i2c.h. I2C is handled by the I2C Manager that was built into lvlg_esp32_drivers, but the code would be the same if it was routed through I2C Manager as a separate component. If you are providing a driver, you need not worry about any of this.

Using I2C in a driver, a multi-step guide

Step 1

The Kconfig entries for your driver only need to specify that you will be using I2C. This is done by select LV_I2C_DISPLAY or select LV_I2C_TOUCH.

Step 2

To use the I2C port in your code you would do something like:

#include "i2c_manager/i2c_manager.h"

uint8_t data[2];
lvgl_i2c_read(CONFIG_LV_I2C_TOUCH_PORT, 0x23, 0x42, &data, 2);

This causes a touch driver to read two bytes at register 0x42 from the IC at address 0x23. Replace CONFIG_LV_I2C_TOUCH_PORT by CONFIG_LV_I2C_DISPLAY_PORT when this is a display instead of a touch driver. lvgl_i2c_write works much the same way, except writing the bytes from the buffer instead of reading them.

The example above ignores it but these functions return esp_err_t so you can check if the i2c communication worked.

Step 3

There is no step 3, you are already done.

Behind the scenes

If anything in lvgl_esp32_drivers uses I2C, the config system will pop up an extra menu under the lvgl drivers menu. This will allow you to select an I2C port for screen and one for the touch driver, if applicable. An extra menu allows you to set the GPIO pins and bus speed of any port you have selected for use. It's perfectly fine for a display and a touch driver to use the same I2C port or different ones.

More information

If you need more documentation, please refer to the I2C Manager GitHub repository for more detailed information on how I2C manager works.

Closes lvgl/lv_port_esp32#282

[C47D]

(which was a bit ... ehm ... organically grown)

Totally agree 😄 , life happened and I didn't had time to work on clean it up.

I requested the review of @tore-espressif as he is more familiar with ESP32 and I don't have time until weekend to review this and @kisvegabor and @embeddedt because they know the future work we're planning with this repo.

Thanks for the efford you've placed into this @ropg 😸

[tore-espressif]

First off all, thank you very much for you thorough work. Thread-safety is something that we definitely need for I2C.

Before I get to the real code-review (beginning of next week?) I'd like to ask you some higher-level questions:

1. what are the long term plans with https://github.com/ropg/i2c_manager ? could we use it as a submodule?
2. I can see that you have contributions in LVGL, would you mind having a look on my refactoring proposal? Design proposal #72. Mostly the section about Kconfig, which I'd like to wipe out from this repo :)

[ropg]

1. what are the long term plans with https://github.com/ropg/i2c_manager ? could we use it as a submodule?

Presently I2C Manager has a few different ways of using it, the README has lots of details. I built it such that it can be a component by itself, or it can be built into a component. The problem with ESP-IDF that I found is that you CANNOT do conditional dependencies, as all dependencies are resolved before any of the Kconfig stuff is parsed. So depending on it as a component means everyone has to install it, even if no I2C is used.

That leaves incorporating it in our component. I built it such that one only has to incorporate the inner directory (also called i2c_manager), not the repo root, and I just copied the files. (It's just 4 files: .c, .h, Kconfig and README). It's important that these files do net end up in a dir that is in INCLUDE_DIRS, as it is only to be included from this component. I would prefer it not to be a submodule as I'd have to change how it works a bit then. (Presently the .h file has #define I2C_OEM lvgl in it which tells I2C Manager what some functions are called.) I'll make sure that if anything were to change I update the version here also.

(If people want to get thread-safety between something they are doing and LVGL they need to install and use the I2C Manager component and tell the version incorporated in LVGL to use its locking semaphore pointers, which is in the README.)

 

2. I can see that you have contributions in LVGL, would you mind having a look on my refactoring proposal? Design proposal #72. Mostly the section about Kconfig, which I'd like to wipe out from this repo :)

Will do and will respond there.

[C47D]

#69 just got merged, I guess the changes it did to the conflicted Kconfig files are making this PR not merging cleanly, will try to take a look at those changes, it also contained (IIRC) I2C related code.

-Carlos

[tore-espressif]

• There are few nitpicks but nothing too important
• Although I didn't have a chance to try it on HW
• Well structured code that cleans our mess in I2C

In light of comment #72 (comment) I'm 100% OK with merging this into our current master, the so to be version 1.0.0.
But unless I'm brutally outvoted, I wouldn't recommend using this in version 2.0.0 and higher.

[tore-espressif]

Nitpick: Maybe ESP_LOGD might be more appropriate? To make the driver less talkative...

[tore-espressif]

commented-out code

[tore-espressif]

There is comment below about ESP pins that applies here too.

[ropg]

In light of comment #72 (comment) I'm 100% OK with merging this into our current master, the so to be version 1.0.0.
But unless I'm brutally outvoted, I wouldn't recommend using this in version 2.0.0 and higher.

Naturally I like I2C Manager because I built it. But I don't get too attached to any of my projects, and if something else is better for LVGL, naturally it should be used instead.

That said:

• Can you expand on why you wouldn't recommend it? It's sort of left hanging, and I'd like to convince you otherwise.

• How would it need to change to be suitable?

or

• What do you recommend in its place?

[ropg]

@dastarling Your gt911 driver uses I2C. This Pull Request takes out all driver I2C code and replaces with shared I2C code. I'd be happy to refactor your code unless you'd like to. I'd also be happy to add in the multi-byte reads that you have in a TODO as well.

[ropg]

I just force-pushed, and then realized that too many people are looking at this and that we're not merging until things are resolved anyway. So from now I'll just do regular commits and then consolidate when we're done.

• I2C Manager now also supports ESP32C3 GPIO range
• gt911 driver I2C code is not replaced yet. But Kconfig side of it is done, just so everything but that driver works.

 

Some thoughts on GPIO pins:

• I can do in I2C manager whatever we think is the right thing, which means all other code doesn't have to do it for I2C, but does for all other pins. I'd have to test whether multiple ranges (when the range is broken) are supported.

• More broadly: how bad is it if pin numbers are just an 'int' and that's it. Sure: you can set the wrong value. You can set lots of wrong values all over the project. If you have a wrong GPIO pin, it's either a typo or because you have a wrong idea of what GPIO pins something is connected to. In both cases the wrong value can just as easily be in the correct range, and then things still wouldn't work. How many people do we really help by enforcing a range here? Because there is significant cost: having to change code every time there's a new ESP32 out, just for the sake of a range in Kconfig.

• If we do enforce ranges, it's an extra argument for centralizing SPI comms, and then it's still a pain for all the one-offs like interrupts and what not.

[C47D]

Got the following error (after updating master with the last commit):

../components/lvgl_esp32_drivers/lvgl_tft/ssd1306.c: In function 'send_data':
../components/lvgl_esp32_drivers/lvgl_tft/ssd1306.c:218:59: error: invalid type argument of unary '*' (have 'int')
     err = lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, *data[0], data + 1, bytes_len - 1 );
                                                           ^~~~~~~~
[1076/1084] Building C object esp-idf/lvgl_esp32_drivers/CMakeFiles/__idf_lvgl_esp32_drivers.dir/lvgl_helpers.c.ob
ninja: build stopped: subcommand failed.
ninja failed with exit code 1

Fixed it with:

diff --git a/lvgl_tft/ssd1306.c b/lvgl_tft/ssd1306.c
index c806feb..21231c4 100644
--- a/lvgl_tft/ssd1306.c
+++ b/lvgl_tft/ssd1306.c
@@ -215,7 +215,7 @@ static uint8_t send_data(lv_disp_drv_t *disp_drv, void *bytes, size_t bytes_len)

     uint8_t *data = (uint8_t *) bytes;

-    err = lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, *data[0], data + 1, bytes_len - 1 );
+    err = lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, data[0], data + 1, bytes_len - 1 );

 //     i2c_cmd_handle_t cmd = i2c_cmd_link_create();
 //

@ropg I don't have access to the hardware right now, is that the right fix?

[ropg]

@ropg I don't have access to the hardware right now, is that the right fix?

Looks like it. Whoops. I'll be happy when this driver is actually tested on the hardware.

[tore-espressif]

More broadly: how bad is it if pin numbers are just an 'int' and that's it. Sure: you can set the wrong value. You can set lots of wrong values all over the project. If you have a wrong GPIO pin, it's either a typo or because you have a wrong idea of what GPIO pins something is connected to. In both cases the wrong value can just as easily be in the correct range, and then things still wouldn't work. How many people do we really help by enforcing a range here? Because there is significant cost: having to change code every time there's a new ESP32 out, just for the sake of a range in Kconfig

At seems to me that you are slowly getting the idea of getting rid of Kconfig :D 🍰
Every ESP-IDF driver that uses GPIO pins checks whether the gpio index is valid for given peripheral (be it I2C, or GPIO) and for given chip (ESP32-xx). So we are just reinventing the wheel (not your fault, this is how it is and why I'd like to change it).

This is how I see it in this PR:
i2c_manager = Mutex + Convenience functions(read/write) + Kconfig

This is my idea of a future-proof solution:
i2c_manager = Mutex + Convenience functions + Runtime configuration

[ropg]

I much appreciate thinking about this with you and others.

This is how I see it in this PR:
i2c_manager = Mutex + Convenience functions(read/write) + Kconfig

This is my idea of a future-proof solution:
i2c_manager = Mutex + Convenience functions + Runtime configuration

How would that work assuming multiple components (example: LVGL and a 3D motion sensor thread) need I2C in terms of mutex vis-a-vis other ESP-IDF components? You'll need to share a single configuration for the I2C port(s) that is not specific to LVGL's runtime configuration (or LVGL period). If I were writing a cool 3D accelerometer thing, I would want to not invent the wheel and I'd look for something lightweight for thread-safety wrt various other users of I2C ports like LVGL. But I would not want to depend on LVGL, or even have to know it exists. What if that 3D accelerometer software, or the I2S audio amplifier/filter/ADC-thing, or ..., also has a runtime configuration that wants to set port parameters? Fight?

I agree (and had agreed already) that the move away from Kconfig makes total sense overall. I've though about it for a while, and feel that in the particular case of I2C the way the port works in ESP-IDF creates an unescapable need for a separate component and Kconfig is where you would configure the ports parameters (only). Bonus points if it's one that also has a mutex-compatible version that can be built into a bigger component so that not every user of something like LVGL needs to install it just because ESP-IDF lacks conditional depends. This sounds very much like I2C Manager to me.

Maybe I simply do not understand, but nothing I can envision under "Mutex + Convenience functions + Runtime configuration" would address the problems that I2C Manager now addresses. Maybe you can elaborate?

[C47D]

I'm getting the following error message

E (20) i2c: i2c_param_config(644): i2c clock choice is invalid, please check flag and frequency
E (30) lvgl_i2c: Failed to initialise I2C port 0.
W (30) lvgl_i2c: If it was already open, we'll use it with whatever settings were used to open it. See I2C Manager README for details.
E (1050) i2c: i2c_set_pin(825): scl and sda gpio numbers are the same
W (1050) lvgl_i2c: Error writing addr 0x3c reg 0x00 port 0
assertion "0 == err" failed: file "../components/lvgl_esp32_drivers/lvgl_tft/ssd1306.c", line 133, function: ssd1306_init

Clock configuration is the default one, I'm setting pin 21 as SDA and 22 as SCL

I think I'm missing the lvgl_i2c_init function call, I can't find it, but it seems to be called from the monitor output.

EDIT
See review, this error went away with that change.

[ropg]

I'll squash these back into one commit when you're done testing.

• I left in the 'overengineered' HAS_CLK_FLAGS because it did give me grief not to initialize that.

• I already noticed that the I2C README does not render pretty on GitHub, it does in my previewer. Will fix.

[ropg]

As an aside, I added i2cdev support to I2C Manager. Doesn't affect what's in lvgl_esp32_drivers, but means that an additional ton of i2c drivers written for it can now run thread-safe with LVGL. (I2C Manager already emulated I2C HAL, the other abstraction layer.)

[tore-espressif]

OK?

Pleasure to work with you :D

I'll add another commit with a change I've made to the Kconfig,

Good idea, it will imply that i2c_manager does not belong solely under LVGL, but should be share across the whole app

I'll do my best to check and do some basic tests by tomorrow.

EDIT: Actually I2C_manager Kconfig menu is still under LVGL ESP Drivers menu

[tore-espressif]

I hope that these are my last comments :D
Apart from them, LGTM!

[ropg]

@tore-espressif

Sorry about this late change...

Yesterday I noticed a logic error in how things work if components with I2C Manager depend on one another (something I had here). In that case #include i2c_manager/i2c_manager.h is not unique.

So I renamed the directory to lvgl_i2c (which fits our scheme better). I put that in a separate commit because it touches a lot of files and is easier to verify that way.

Also I noticed that i2c_manager would not have compiled under ESP-IDF 3.x, so I fixed that. (I think, haven't tested)

I'll do the other things you noticed today.

[ropg]

Alright: I made your changes, looked at the menuconfig and tested with my hardware.

[C47D]

Looks good to me!

@kisvegabor @embeddedt ANy thoughts?

[embeddedt]

No comments.

There are a few extra diff lines that are just adding/deleting trailing whitespace, but those can be filtered easily when looking at the diff down the road, so I don't personally think they matter.

[C47D]

Ok, I will merge this awesome job, thanks a lot @ropg 😸

[ropg]

Woot!

This has to have been one of the longer threads under a pull request, but everything was needed and things were so much improved. It has been a great pleasure working with you, and we have a better lvgl_esp32_drivers as well as a better I2C Manager to show for it.

[ropg]

( Just a pity I didn't squash the commits before the final merge, which would have made the commit history more readable since it now looks out of order because things happened in between my partial commits. But this works... )