drivers/lcd: code deduplication for st7735 and ili9341 #19816

gschorcht · 2023-07-11T10:42:07Z

Contribution description

In preparation for the parallel interface support the following changes were made:

The code for basic communication (acquire/release SPI device, SPI transfers), which were always duplicated by copy & paste in each display driver again and again, has been moved to the LCD driver as low-level functions that are now used by the display drivers. These low level function allow

code deduplication and
to define a more abstract communication interface that can then be extended later by parallel communication interface

Identical GPIO initializations has also been moved from display drivers to the LCD driver.
Using a default implementation of lcd_set_area function allows further code deduplication.

Finally, the ili9341 and st7735 drivers only implement the inialization sequence for the LCD driver IC.

Testing procedure

tests/drivers/ili9341 should still work for a board with an ILI9341. Tested with esp32-wrover-kit.
tests/drivers/st7735 should still work for a board with a ST77xx. Tested with esp32s3-usb-otg.

Issues/PRs references

aabadie · 2023-07-12T08:12:47Z

drivers/lcd/lcd.c

+        spi_transfer_byte(dev->params->spi, dev->params->cs_pin, cont, data);
+    }
+    else {
+        assert(false);


Why not just use assert(dev->params->spi != SPI_UNDEF); at the beginning of the function? (Here and below)

Yeah, the reason is that this change is a split-off from parallel interface implementation where it will be

#if IS_USED(MODULE_LCD_SPI) if (dev->params->spi != SPI_UNDEF) { /* SPI serial interface is used */ spi_transfer_byte(dev->params->spi, dev->params->cs_pin, cont, data); } else { #endif #if IS_USED(MODULE_LCD_PARALLEL) /* MCU 8080 8-/16-bit parallel interface is used */ _lcd_write_byte_par(dev, cont, data); #else assert(false); #endif #if IS_USED(MODULE_LCD_SPI) } #endif

I can change it, if you want 😄

Let's keep it like this if it's easier for you to rebase once that's merged.

aabadie · 2023-07-12T08:16:37Z

drivers/st7735/Kconfig

@@ -22,19 +22,19 @@ config HAVE_ST7735
    help
      Indicates that an ST7735 display is present.

+config HAVE_ST7789


This seems unrelated and actually not needed. The pattern here was to group driver module variants module with their HAVE_VARIANT just after. With that change it looks messed up.

Maybe a better approach is to use:

config MODULE_VARIANT1 ... config MODULE_VARIANT2 ... config HAVE_VARIANT1 ... config HAVE_VARIANT2 ...

It is from the history. When @maribu tried to fix master compilation in CI with PR #19813, he added

config MODULE_ST7789 - bool "ST7789 display driver" - select MODULE_ST7735 + bool + depends on HAVE_ST7789 + default y if MODULE_ST7735 + help ST7789 display driver

according to my suggestion. This was just from the commit cb0aba6 in this PR. When I rebased, the order has been changed.

Regarding the order, my feeling was that we define a HAVE_* symbol before it is used in MODULE_*. I can change it if you want.

Comparing to other drivers the fix doesn't seem correct (I'm not expert enough with this kind of kconfig stuff, that's why I look at other drivers written by HAW). And the order of definition is not a problem apparently. IMHO it's better to have the "MODULE_DRIVER" definition before the "HAVE_DRIVER" since the latter is more for internal use.

I have changed the order.

Why is this fix not correct? MODULE_ST7789 is only available if the board enables HAVE_ST7789. The MODULE_ST7789 has to be enabled by default if MODULE_ST7735 is enabled. This corresponds to makefile dependencies.

"HAVE_ST7759" indeed tells the board has it so it can be pulled in automatically. But I manually plug the device to a dev kit and want to pull in myself, I'll also have to pullin the HAVE_ symbol which is not needed for other drivers AFAICT.

But I manually plug the device to a dev kit and want to pull in myself, I'll also have to pullin the HAVE_ symbol

Yes, indeed. Unfortunatly, there was no other a approach to enable the st7789 variant if the board pulls in the module in makefile dependencies. This should become obsolete once the driver varaiants are splitted.

aabadie

This is just code moved around and will simplify the work for coming PRs on new LCD drivers (not only SPI based). I saw some screenshot in privates that prove the PR is working.

Please squash!

ACK

riot-ci · 2023-07-12T13:27:13Z

Murdock results

✔️ PASSED

5cb51b1 driver/lcd: use a default implementation of lcd_set_area used

Success	Failures	Total	Runtime
6936	0	6936	09m:50s

Artifacts

Documentation preview

MODULE_ST7789 is enabled if MODULE_ST7735 is enabled and the board has selected HAVE_ST7789.

In preparation for the parallel interface support the following changes were made: 1. The code for basic communication (acquire/release SPI device, SPI transfers), which were always implemented identically in the individual display drivers again and again, have been moved to the LCD driver as low-level functions and are now used by the display drivers. These low level function allow - code deduplication on one hand and - to define a more abstract communication interface on the other hand that can then be extended by parallel communication 2. Identical GPIO initialization has also been moved from display drivers to the LCD driver.

Using a default implementation of `lcd_set_area` function allows further code deduplication.

aabadie · 2023-07-12T14:08:37Z

bors merge

bors · 2023-07-12T16:55:38Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

gschorcht · 2023-07-12T21:02:04Z

@aabadie Thanks for reviewing and merging.

gschorcht requested review from leandrolanzieri, jia200x and MrKevinWeiss as code owners July 11, 2023 10:42

github-actions bot added Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels Jul 11, 2023

gschorcht requested review from bergzand and fjmolinas July 11, 2023 10:42

benpicco requested review from aabadie and maribu July 11, 2023 10:44

gschorcht mentioned this pull request Jul 11, 2023

drivers/st7735: faulty driver initialization #19818

Closed

6 tasks

aabadie reviewed Jul 12, 2023

View reviewed changes

aabadie approved these changes Jul 12, 2023

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 12, 2023

gschorcht added 5 commits July 12, 2023 15:31

drivers/st7735: fix Kconfig logic for ST7789

b30401a

MODULE_ST7789 is enabled if MODULE_ST7735 is enabled and the board has selected HAVE_ST7789.

drivers/ili9341: move basic communication functions to drivers/lcd

c51496f

drivers/st7735: move basic communication functions to drivers/lcd

878af4f

driver/lcd: use a default implementation of lcd_set_area used

5cb51b1

Using a default implementation of `lcd_set_area` function allows further code deduplication.

gschorcht added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Jul 12, 2023

gschorcht force-pushed the drivers/lcd_st7735_ili9341_code_deduplication branch from 7914f95 to 5cb51b1 Compare July 12, 2023 13:37

bors bot merged commit ae2118c into RIOT-OS:master Jul 12, 2023

gschorcht deleted the drivers/lcd_st7735_ili9341_code_deduplication branch July 12, 2023 21:16

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/lcd: code deduplication for st7735 and ili9341 #19816

drivers/lcd: code deduplication for st7735 and ili9341 #19816

gschorcht commented Jul 11, 2023

aabadie Jul 12, 2023

gschorcht Jul 12, 2023

aabadie Jul 12, 2023

aabadie Jul 12, 2023 •

edited

Loading

gschorcht Jul 12, 2023 •

edited

Loading

aabadie Jul 12, 2023

gschorcht Jul 12, 2023

aabadie Jul 12, 2023

gschorcht Jul 12, 2023 •

edited

Loading

aabadie left a comment

riot-ci commented Jul 12, 2023 •

edited

Loading

aabadie commented Jul 12, 2023

bors bot commented Jul 12, 2023

gschorcht commented Jul 12, 2023

drivers/lcd: code deduplication for st7735 and ili9341 #19816

drivers/lcd: code deduplication for st7735 and ili9341 #19816

Conversation

gschorcht commented Jul 11, 2023

Contribution description

Testing procedure

Issues/PRs references

aabadie Jul 12, 2023

Choose a reason for hiding this comment

gschorcht Jul 12, 2023

Choose a reason for hiding this comment

aabadie Jul 12, 2023

Choose a reason for hiding this comment

aabadie Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

gschorcht Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

aabadie Jul 12, 2023

Choose a reason for hiding this comment

gschorcht Jul 12, 2023

Choose a reason for hiding this comment

aabadie Jul 12, 2023

Choose a reason for hiding this comment

gschorcht Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

riot-ci commented Jul 12, 2023 • edited Loading

Murdock results

Artifacts

aabadie commented Jul 12, 2023

bors bot commented Jul 12, 2023

gschorcht commented Jul 12, 2023

aabadie Jul 12, 2023 •

edited

Loading

gschorcht Jul 12, 2023 •

edited

Loading

gschorcht Jul 12, 2023 •

edited

Loading

riot-ci commented Jul 12, 2023 •

edited

Loading