Skip to content

driver/lcd: add common lcd base driver #16176

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 12, 2022
Merged

driver/lcd: add common lcd base driver #16176

merged 11 commits into from
Apr 12, 2022

Conversation

fjmolinas
Copy link
Contributor

Contribution description

A year ago I wrote an st7735 driver that was based on the ili9341, most of the code is exactly the same though.

LCD controllers ST7735 and ili9341 share very similar functions, the main differences are related to the set of available commands and initialization procedure. So instead of adding a new driver for the st77335 I rename the ili9341 driver to a common lcd driver. Only available commands and initialization procedure are a bit different.

Other changes:

  • add LCD_COMMON_OFFSET_X/Y: ST7735 display might have an offset depending on rotation and size
  • allow defining the default rotation

There is a lot of renaming in this PR but ili9341 code should be mostly exactly the same, there is a very small difference in binary size mainly because "puts" have changed and the extra CONFIG_%

"make" -C /home/francisco/workspace/riot-firmwares/RIOT/sys/xtimer
   text    data     bss     dec     hex filename
  30332     112    2340   32784    8010 /home/francisco/workspace/riot-firmwares/RIOT/tests/driver_lcd_common/bin/stm32f429i-disc1/tests_driver_lcd_common.elf

   text    data     bss     dec     hex filename
  30300     112    2340   32752    7ff0 /home/francisco/workspace/riot-firmwares/RIOT/tests/driver_ili9341/bin/stm32f429i-disc1/tests_driver_ili9341.elf

Testing procedure

  • make -C tests/driver_lcd_common flash
  • LCD_CONTROLLER=st7735 make -C tests/driver_lcd_common flash
  • make -C tests/disp_dev flash
  • make -C tests/pkg_lvgl flash

@fjmolinas fjmolinas added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Mar 9, 2021
@fjmolinas fjmolinas requested review from aabadie and bergzand March 9, 2021 14:18
aabadie
aabadie reviewed Mar 10, 2021
View reviewed changes
@fjmolinas fjmolinas force-pushed the pr_lcd_common_driver branch from b0d48db to 1c05550 Compare March 15, 2021 14:24
@fjmolinas fjmolinas changed the title driver/lcd_common: rename from ili9341 driver driver/lcd: add common lcd base driver Mar 15, 2021
@fjmolinas
Copy link
Contributor Author

@aabadie I re-worked this PR quite a bit, now it's compatible with both drivers, I tested this by running tests/disp_dev on a board with a st7735 and ili9341 based display. Some things were also changed:

  • rgb_channels: added as a new parameter. Why is this not compile time? because you can have multiple screens with different sizes and that needs to be handled.
  • st7735: there is a x/y_offset that is not compile time for the same reason.

There are maybe still things to be improved regarding code duplicity, but please tell me what you think of the current approach.

fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request May 7, 2021
@fjmolinas
MERGECOMMIT: driver/lcd: add common lcd base driver RIOT-OS#16176
ce167a8
@aabadie
Copy link
Contributor

aabadie commented Jun 1, 2021

please tell me what you think of the current approach

I don't remember how it was before but looking at the current approach, I must say that I don't fully understand all changes related to the lcd common. It seems to me that it introduces another layer of abstraction and thus is kind of redundant with the disp_dev purpose. It also adds another layer of auto_init code (with driver specific code inside) that I don't like.
Why not just keep the lcd common as an external module where function are directly used by the specific ili9341/st7785 drivers ? And use disp_dev as the first attribute of each driver specific code, instead of lcd.

But anyway, with all recent changes with disp_dev test application and build system enhancement, you have to rebase and rework this again.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@fjmolinas fjmolinas force-pushed the pr_lcd_common_driver branch from 1c05550 to 74c51e1 Compare December 3, 2021 07:29
@github-actions github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Dec 3, 2021
@fjmolinas
Copy link
Contributor Author

But anyway, with all recent changes with disp_dev test application and build system enhancement, you have to rebase and rework this again.

I rebased it now.

fjmolinas
fjmolinas commented Dec 3, 2021
View reviewed changes
@aabadie aabadie mentioned this pull request Mar 15, 2022
Merged
@aabadie aabadie force-pushed the pr_lcd_common_driver branch from 74c51e1 to 74e2c67 Compare March 24, 2022 15:32
aabadie
aabadie reviewed Mar 24, 2022
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR with the pybadge screen (ST7735) and it works like a charm. I also fixed (and allowed myself to push) some rebasing remaining issues with ili9341 but otherwise everything seems fine there as well.

I have a few comments/questions, see below.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Mar 24, 2022
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 11, 2022
@aabadie aabadie force-pushed the pr_lcd_common_driver branch from a69adce to 12457e5 Compare April 11, 2022 15:13
@aabadie
Copy link
Contributor

aabadie commented Apr 11, 2022

@fjmolinas I've updated a bit your PR to something that I think is better. Tested on adafruit-clue (ili9341) and adafruit-pybadge (with #17903 included) and both are working as expected.
Main changes I added:

  • split the auto_init_lcd code in auto_init_9341 and auto_init_st7735
  • remove the use of symlink between tests/driver_st7735 and driver_ili9341
  • the lcd driver struct is now called lcd_driver_t instead of lcd_desc_t

If you are ok with my changes, then I think this is ready to go.

@aabadie aabadie force-pushed the pr_lcd_common_driver branch from 12457e5 to b255cde Compare April 12, 2022 07:09
@aabadie aabadie mentioned this pull request Apr 12, 2022
Merged
@aabadie aabadie force-pushed the pr_lcd_common_driver branch from b255cde to b2dbb8e Compare April 12, 2022 08:34
@aabadie aabadie force-pushed the pr_lcd_common_driver branch from b2dbb8e to fff7c63 Compare April 12, 2022 10:35
@aabadie aabadie force-pushed the pr_lcd_common_driver branch from fff7c63 to bf7dccf Compare April 12, 2022 10:39
@aabadie aabadie mentioned this pull request Apr 12, 2022
Merged
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 12, 2022
@aabadie aabadie enabled auto-merge April 12, 2022 17:48
aabadie
aabadie approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are good enough to me now. @fjmolinas is ok to merge this one as it is now.

ACK

@aabadie aabadie disabled auto-merge April 12, 2022 19:00
@aabadie aabadie enabled auto-merge April 12, 2022 19:00
@aabadie aabadie merged commit 8e87b77 into RIOT-OS:master Apr 12, 2022
@aabadie aabadie mentioned this pull request Apr 13, 2022
Merged
@fjmolinas fjmolinas deleted the pr_lcd_common_driver branch June 9, 2022 08:54
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

@bergzand bergzand Awaiting requested review from bergzand

@cgundogan cgundogan Awaiting requested review from cgundogan

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @aabadie @chrysn @MrKevinWeiss