Skip to content
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

Added generic shield for ST7789V display controller #20570

Merged
merged 11 commits into from
Jan 2, 2020

Conversation

vanwinkeljan
Copy link
Member

Added generic shield for ST7789V display controller

Fixes: #20278

@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 11, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.


This is a generic shield for display shields based on ST7789V display
controller. More information about the controller can be found in
`ST7789V Datasheet_`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Underscore (indicating an outgoing link) is after the back-quoted text:

`ST7789V Datasheet`_.

Copy link
Member Author

Choose a reason for hiding this comment

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

thx for the input!

+-----------------------+---------------------------------------------+
| Arduino Connector Pin | Function |
+=======================+=============================================+
+-----------------------+---------------------------------------------+
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this line (it creates an unnecessary blank row in the generated HTML)

Requirements
************

This shield can only be used with a board which provides a configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

change which to that

@jfischer-no
Copy link
Collaborator

@vanwinkeljan I do not have TL019FQV01 display but the Waveshare 1.3inch 240x240. TL019FQV01 does not seem to be so common and I think it should not be the default one. I try to find some time to add support for the Waveshare display. Also the Adafruit 1.3" 240x240 seems to be identical to one from Waveshare.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

LGTM, with a potential table format edit.

+=======================+=============================================+
| D8 | ST7789V Reset |
+-----------------------+---------------------------------------------+
| D9 | ST7789V DC (Data/Command) |
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is OK, if you really want to align the description part (in parentheses), you should add another column under the heading bar:

+-----------------------+---------------------------------------------+
| Arduino Connector Pin | Function                                    |
+=======================+================+============================+
| D8                    | ST7789V Reset  |                            |
+-----------------------+----------------+----------------------------+
| D9                    | ST7789V DC     | (Data/Command)             |

etc.

@jfischer-no
Copy link
Collaborator

jfischer-no commented Nov 13, 2019

Draft for the Waveshare 1.3inch 240x240 - https://github.com/jfischer-phytec-iot/zephyr/tree/st7789v_240x240

@vanwinkeljan
Copy link
Member Author

@jfischer-phytec-iot do you want me to take along the waveshare support or will you push this through a separate PR?

@jfischer-no
Copy link
Collaborator

@vanwinkeljan see #20778

vanwinkeljan and others added 9 commits November 21, 2019 17:55
Added a generic display shield for boards based on the ST7789V
display controller.

Signed-off-by: Jan Van Winkel <[email protected]>
Use the ST7789V generic shield in ST7789V display sample

Signed-off-by: Jan Van Winkel <[email protected]>
Fix style, move init function to the bottom.

Signed-off-by: Johann Fischer <[email protected]>
Obtain resolution and offsets from DT.
Fix style.

Signed-off-by: Johann Fischer <[email protected]>
Make privat functions static, remove unused st7789v_cmd_read8().

Signed-off-by: Johann Fischer <[email protected]>
Obtain panel settings and parameters from DT.

Signed-off-by: Johann Fischer <[email protected]>
Remove obsolete Kconfig options.

Signed-off-by: Johann Fischer <[email protected]>
Remove obsolete display_st7789v_tl019fqv01.c

Signed-off-by: Johann Fischer <[email protected]>
…rties

Add TL019FQV01 parameter and settings properties.

Signed-off-by: Johann Fischer <[email protected]>
Rename shield to TL019FQV01.

Signed-off-by: Johann Fischer <[email protected]>
Add support for Waveshare 1.3inch IPS LCD display.

Signed-off-by: Johann Fischer <[email protected]>
@vanwinkeljan vanwinkeljan removed the block: HW Test Testing on hardware required before merging label Nov 22, 2019
@vanwinkeljan
Copy link
Member Author

@jfischer-phytec-iot I assume you tested this on Waveshare as such I removed the label blocked: HW test?

@jfischer-no
Copy link
Collaborator

@jfischer-phytec-iot I assume you tested this on Waveshare as such I removed the label blocked: HW test?

Yes, I did

@vanwinkeljan vanwinkeljan added this to the v2.2.0 milestone Dec 7, 2019
@jfischer-no
Copy link
Collaborator

@erwango please review

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

I'm fine with the change despite conflict with #20934.
Let's see which one goes in first and which one has to rebase.

@vanwinkeljan
Copy link
Member Author

Let's see which one goes in first and which one has to rebase.

Looks like #20934 as you request changes on this one ;)

@erwango
Copy link
Member

erwango commented Dec 18, 2019

Looks like #20934 as you request changes on this one ;)

Oups! Seems I clicked the wrong button.. approved!

@@ -121,6 +121,7 @@ void main(void)
*(buf + idx + 1) = (color_rgb >> 0) & 0xFFU;
}
#else
int color = cnt % 4;
Copy link
Member

Choose a reason for hiding this comment

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

is this a stray change? it doesn't seem related to the commit message samples: display: Use ST7789V generic shield

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it, but it is a bug fix for the sample in case RGB565 is not defined

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not nice but this sample (and any other controller type specific) should be removed after we merged #20580

@jfischer-no
Copy link
Collaborator

@MaureenHelm @nashif @carlescufi please revise/merge

@nashif nashif merged commit 7d0d845 into zephyrproject-rtos:master Jan 2, 2020
@vanwinkeljan vanwinkeljan deleted the st7789v_shield branch January 4, 2020 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Something is wrong when trying ST7789V sample
7 participants