st7789 Support for ili9XXX hybrid driver

[russhughes]

This adds support for 320x240, 240x240 and 135x240 ST7789 displays with rotations for the ESP32 to the LVGL MicroPython bindings. Tested with TTGO T-Display and TTGO TWatch-2020. See https://github.com/russhughes/lv_st7789 for examples modified to run with this driver.

[russhughes]

GitHub Community Guidelines link is too close to the Close Button.

[russhughes]

Made requested changes and tested with TTGO TDisplay (135x240) and TWATCH-2020 (240x20) with examples from https://github.com/russhughes/lv_st7789.

[russhughes]

There is an issue with hybrid=False.

[russhughes]

Tests work for both hybrid=True and False.

[russhughes]

That change makes it obvious that colormode, rot, and display_type are not referenced in the init body as well and should be removed? https://github.com/lvgl/lv_binding_micropython/blob/96eb4f2283e49837dbcbe2cac0e9a94c4aaa3f60/driver/esp32/ili9XXX.py#L86-L91

[amirgon]

That change makes it obvious that colormode, rot, and display_type are not referenced in the init body as well and should be removed?

You are right. colormode and rot can really be removed from ili9XXX constructor. display_type on the other hand should probably stay, and set self.display_type on ili9XXX, instead of on each of the specific displays, because it is used in ili9XXX:

https://github.com/lvgl/lv_binding_micropython/blob/96eb4f2283e49837dbcbe2cac0e9a94c4aaa3f60/driver/esp32/ili9XXX.py#L149

[russhughes]

I'm going to close this pull request and look at a standalone module for this display.

[amirgon]

I'm not sure why you think that rot constants issue is a reason to make this its own module. 90% of the code is the same between these displays, I think it makes sense they would be in the same module.

Anyway, I will accept this contribution either in the same module or in different module as long as there is no code duplication, so you would probably still need to inherit from ili9xxx.

[jdtsmith]

Really looking forward to seeing this get included. I've been testing @russhughes' driver and it's been working great on my 240x135 TTGO-TDISPLAY. One has to be a bit careful with ROM and RAM, but so far so good.

[russhughes]

I'm not sure why you think that rot constants issue is a reason to make this its own module. 90% of the code is the same between these displays, I think it makes sense they would be in the same module.

The issue is with this requirement:

...should be only a single definition of rot values (PORTRAIT, LANDSCAPE, INVERSE_PORTRAIT, INVERSE_LANDSCAPE) and every display should use the same values (either directly, or translate them if needed).

The existing rot constant values are based on bits that need to be set in the MADCTL register for the ili9431 driver. Other drivers and other use cases (ie mirrored output for use in a reflective display like a heads-up display) require different bits to be set or not set. The rot parameter is sent to the display controllers MADCTL register after ANDing it with colormode. This allows a developer to configure the display the way they need for their application. Translating the ili9431 constants to the equivalent st7789 values interferes with the ability to configure the MADCTL register directly, unlike the ili9431 driver. Translation of these constants also creates side effects that must documented and can cause confusion when someone who is familiar with the ili9431 driver starts working with a st7789. You can see this issue in the forums when people are using an ili9342 and the display is mirrored using the existing constants. They are able to get around this issue by setting the MADCTL register directly. This same approach can be used the by st7789 driver as well, by documenting that users of the st7789 driver should not use the constants. This would likely lead to some confusing forum requests for help.

An alternative approach to the "one true constant" method would be to redefine the constants sequentially and use tables to retrieve the correct value for each displays rot modes. In order to keep the ability to directly set the MADCTL registers value an additional parameter would be required, so that if it was not None it could be used in place of rot. This approach changes the behavior of the rot parameter and would likely lead to some confusing forum requests for help.

The better alternative in my opinion would be to soft deprecate the module scoped constants for the rotations and add class scoped constants. Each display would have the appropriate constants it requires and retain the ability to directly set the MADCTL register. Module scoped constants made sense when the values could all be the same, that is not the case anymore.

It's only a matter of time until someone gets confused and passes the module's PORTRAIT value and passes the module's PORTRAIT value to st7789 rot parameter, or the other way around.

Documentation is the answer for this. There is no easy way around reading documentation and/or release notes. A separate module sidesteps all these issues as well at the cost of some duplicated code. I'd prefer class scoped constants as it keeps everything in one module.

[amirgon]

An alternative approach to the "one true constant" method would be to redefine the constants sequentially and use tables to retrieve the correct value for each displays rot modes.

I think that would be a good idea.
I also think that these displays should provide the same API, and translate their inputs to the correct initialization values when needed. My assumption is that most people don't know much about the registers of their display and would rather use simple enumerations to select rot, colormode etc.

In order to keep the ability to directly set the MADCTL registers value an additional parameter would be required, so that if it was not None it could be used in place of rot.

Instead of adding display specific parameters, we could provide some display specific API function to set these per display.
That way the display constructor (and other inherited functions) would be the same for all displays (same basic API), while each display could provide additional display-specific functions if needed.

The better alternative in my opinion would be to soft deprecate the module scoped constants for the rotations and add class scoped constants.

This alternative steps away from the "single basic API" idea for all displays. If each display defines these constants in class scope, eventually these constants would diverge. If I replace my display with another I would not be able to simply replace the display class name I'm using, I'll also have to go through the constructor arguments and replace them with these class specific constants.

Module scoped constants made sense when the values could all be the same, that is not the case anymore.

The advantage of module scope constants is the they are defined once. Their value is not important, every display can handle the value differently. But their names and the fact that they are grouped together is important, such that they would have the same meaning everywhere.

Documentation is the answer for this.

I think that a clear API is even more important than documentation.
And keeping the same API for all displays, including the same constants with the same meaning, is also important in order to make things simple for the users.

I'd prefer class scoped constants as it keeps everything in one module.

If we do it that way, in order to keep the same API for all displays we would have to make sure all classes have the exact same constant names with the same meaning. In my opinion, having the constants scattered in different classes is less clear to the user and in the long run would be harder to maintain than a single list of constants.

[russhughes]

Sounds like abstracting the rot constants is the clear choice forward.

[amirgon]

Sounds like abstracting the rot constants is the clear choice forward.

Probably also the COLOR_MODE_* constants.

Regarding the MADCTL_*, these are display specific right? But they are still common to at least 3 displays, so I'm not sure what to do about those.

[russhughes]

As far as I can tell all the devices use the same MADCTL_ and COLORMODES bit names and bit values. The difference is in how the display controller interprets/mis-interprets them and how the controller is connected to the actual LCD panel.

[jdtsmith]

I've unfortunately encountered a "constant memory drain" issue with this driver (with nothing else loaded). See russhughes/lv_st7789#5.

[jdtsmith]

Just following up on this "near-miss" PR. I've been using @russhughes' ST7789 patches successfully for many months, but must re-apply them every time there is an update here. It seems the final requested changes were a bit too far beyond the comfort zone for testing (since he doesn't have access to the affected hardware), and he doesn't have time to explore a separate module.

@amirgon if you were willing to make the proposed changes generalizing the display constants and testing on your hardware, I'd be happy to test such a branch on ST7789. These are pretty common low-cost devices so I suspect many people would enjoy having in-built support.

Thanks both for your work!

[amirgon]

@amirgon if you were willing to make the proposed changes generalizing the display constants and testing on your hardware, I'd be happy to test such a branch on ST7789. These are pretty common low-cost devices so I suspect many people would enjoy having in-built support.

@jdtsmith would you like to give it a try and create the PR?

[jdtsmith]

Thanks. A bit out of my comfort zone too, but happy to test. I also thought you guys had a pretty good plan on how to handle generalizing things?

[amirgon]

I also thought you guys had a pretty good plan on how to handle generalizing things?

I wouldn't say "pretty good plan", more of "some idea".

From my point of view, the important things to keep in mind are:

• Backward compatibility - The change should not affect existing API or functionality
• Avoid code duplication - Put common code in shared functions. Avoid duplication in the API, where possible.
• Uniform API - use similar API for different display devices (as much as possible, since display features can vary)
• Keep things simple - Make the API simple. Make the code easy to read and maintain.
• Small incremental changes to existing code are usually better than extensive changes, where possible.

To achieve this, in my opinion, the simplest thing would be to translate existing constants (such as PORTRAIT, LANDSCAPE, INVERSE_PORTRAIT, INVERSE_LANDSCAPE). Since st7789 is the only display in this family that defines them differently, I would translate them only on st7789 code.
But as you can see, @russhughes did not agree with me on that.

So if you are willing to try, just write it in some way that follows the points above, create a PR and we can continue the discussion there.

[amirgon]

Related: https://github.com/lvgl/lv_binding_micropython/pull/198