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

Add support for Adafruit PiTFT 1.14" #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GavinHigham
Copy link

I took a stab at adding support for the "Adafruit Mini PiTFT - 135x240 Color TFT Add-on for Raspberry Pi". It took me several evenings of fiddling to even get anything on the screen, and then a few more to determine the offsets / display orientation / figure out how to implement the changes in fbcp-ili9341.

I've tested this on a Raspberry Pi Zero W using the "Hello Teapot" demo from /opt/vc/src/ with a clock divisor of 6 and didn't notice any artifacts (video here).

@@ -202,6 +202,9 @@ elseif(FREEPLAYTECH_WAVESHARE32B)
elseif(ADAFRUIT_HX8357D_PITFT)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DHX8357D -DADAFRUIT_HX8357D_PITFT")
message(STATUS "Targeting Adafruit 3.5 inch PiTFT with HX8357D")
elseif (ADAFRUIT_ST7789_PITFT_114)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DADAFRUIT_ST7789_PITFT_114")
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of duplicating in all places below defined(ST7789) || defined(ADAFRUIT_ST7789_PITFT_114), let's instead add here

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DADAFRUIT_ST7789_PITFT_114 -DST7789")

since ADAFRUIT_ST7789_PITFT_114 uses ST7789. Then all ST7789 code blocks will automatically apply to ADAFRUIT_ST7789_PITFT_114.

#define DISPLAY_NATIVE_COVERED_TOP_SIDE 40
#define DISPLAY_NATIVE_COVERED_BOTTOM_SIDE 40
#define DISPLAY_NATIVE_COVERED_LEFT_SIDE 53
#define DISPLAY_NATIVE_COVERED_RIGHT_SIDE 52
Copy link
Owner

Choose a reason for hiding this comment

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

This reads like you chose in /boot/config.txt to have hdmi_cvt=320 240 60 1 0 0 0 for a 320x240 display mode, and then here are opting to crop a centered subrectangle of 240x135 pixels in size to display? This means that the full display output buffer will not be displayed, but only a centered subrectangle will be. What if a user wanted to display the full display area?

Would it be better to have here by default

#define DISPLAY_NATIVE_WIDTH 135
#define DISPLAY_NATIVE_HEIGHT 240

and no cropping. And then in /boot/config.txt have

hdmi_cvt=240 135 60 1 0 0 0

to get the full screen display outputted? For e.g. console use cases, that would probably be a sensible default?

@@ -0,0 +1,18 @@
#pragma once
Copy link
Owner

Choose a reason for hiding this comment

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

Now I realize that the naming of these adafruit specific files has not been that consistent. Would you mind renaming this file and the two existing files to have adafruit_ prefix? (like waveshare_, tontec_ and freeplaytech_ also have). I.e.

adafruit_pitft_114r_st7789.h
adafruit_pitft_28r_ili9341.h
adafruit_pitft_35r_hx8357d.h
```

@juj
Copy link
Owner

juj commented Feb 3, 2021

Great work overall, looks like a nice addition! :)

@GavinHigham
Copy link
Author

Thanks! I'll have a look at addressing these when I have some free time later this week. This is my first time working with one of these displays, so I'm still developing my understanding. My understanding so far is that the ST7789 is a display controller for a 240x320 display, which happens to be used for smaller displays by sort of "cropping" its memory into a smaller (possibly non-contiguous between rows?) rectangular region. The vertical scroll controls the row where it starts reading (wrapping around to the first row, depending on the total number of rows to be rendered), and then your DISPLAY_COVERED mechanism is a way to define the "crop" done in display controller memory. I was trying to find a way to apply the x/y offsets I found for this display in other libraries, which I accomplished by taking what I assumed to be the full resolution of the display controller, 240x320, and "covering" the first and last 40 rows, and the first 53 and last 52 columns, to write to the 135x240 region that is actually displayed. Does that sound right? I have my HDMI settings set for 240x135, and fbcp comes up with a 240x135 displayable resolution for the SPI display, so it gives a pixel-perfect full-screen image.

@juj
Copy link
Owner

juj commented Feb 4, 2021

My understanding so far is that the ST7789 is a display controller for a 240x320 display, which happens to be used for smaller displays by sort of "cropping" its memory into a smaller (possibly non-contiguous between rows?) rectangular region.

That is probably true. I had similar experiences when working on the 240x240 ST7789 display. However that does not mean that the Linux framebuffer on the Pi will need to be 240x320 in size.

your DISPLAY_COVERED mechanism is a way to define the "crop" done in display controller memory.

That is incorrect. The DISPLAY_COVERED mechanism crops away parts of the source framebuffer image. This was implemented for Freeplaytech's gaming device, where part of the display is physically obstructed by the game case.

I was trying to find a way to apply the x/y offsets I found for this display in other libraries, which I accomplished by taking what I assumed to be the full resolution of the display controller, 240x320, and "covering" the first and last 40 rows, and the first 53 and last 52 columns, to write to the 135x240 region that is actually displayed. Does that sound right? I have my HDMI settings set for 240x135, and fbcp comes up with a 240x135 displayable resolution for the SPI display, so it gives a pixel-perfect full-screen image.

If I understood correctly, in this case it does sound then like if you set

#define DISPLAY_NATIVE_COVERED_TOP_SIDE 0
#define DISPLAY_NATIVE_COVERED_BOTTOM_SIDE 0
#define DISPLAY_NATIVE_COVERED_LEFT_SIDE 0
#define DISPLAY_NATIVE_COVERED_RIGHT_SIDE 0
// (or just don't set the above at all)
// and
#define DISPLAY_NATIVE_WIDTH 135
#define DISPLAY_NATIVE_HEIGHT 240

it should give you the appropriate output size. However pay attention to this line

https://github.com/juj/fbcp-ili9341/blob/master/st7735r.cpp#L95

to make sure it gets the right offset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants