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 ST7796S Support #169

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

furrysalamander
Copy link

No description provided.

@furrysalamander
Copy link
Author

Please link this with issue #160

@furrysalamander
Copy link
Author

As per my latest comment on the issue, this should be ready to merge now.

@juj
Copy link
Owner

juj commented Oct 9, 2020

Thanks! Looks great! Had a couple of notes above.

@@ -96,10 +99,15 @@ void InitST7735R()
#endif

// TODO: The 0xB1 command is not Frame Rate Control for ST7789VW, 0xB3 is (add support to it)
#ifndef ST7789VW
#if !defined(ST7789VW) || !defined(ST7796S)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be #if !defined(ST7789VW) && !defined(ST7796S) (in the current form, it will always resolve as true)

// Frame rate = 850000 / [ (2*RTNA+40) * (162 + FPA+BPA)]
SPI_TRANSFER(0xB1/*FRMCTR1:Frame Rate Control*/, /*RTNA=*/6, /*FPA=*/1, /*BPA=*/1); // This should set frame rate = 99.67 Hz
#endif
// The frame rate control on the ST7796S needs different values in order to work correctly. Borrowing these values
// from the device tree files of the MHS-4.0 inch hat device tree files. They fix all the problems I was having.
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps change this comment to

// The ST7796S does not work with the frame rate control from above for other ST77* displays. This value taken from MHS-4.0" device tree files, meaning unknown.

since a comment They fix all the problems I was having. is useful for a PR comment, but it has no value after having landed in the tree.

@@ -62,6 +61,10 @@ void InitST7735R()
madctl ^= MADCTL_ROTATE_180_DEGREES;
#endif

#if defined(ST7796S)
madctl ^= MADCTL_COLUMN_ADDRESS_ORDER_SWAP;
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

Needing to have this in here seems odd. Is this making the display landscape by default? Or portrait? The panel is a portrait scaning panel, according to the DISPLAY_NATIVE_WIDTH/HEIGHT fields from below?

Copy link
Author

Choose a reason for hiding this comment

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

It's been a few weeks, but I believe that I added this because without it the display was mirrored.

@juj
Copy link
Owner

juj commented Dec 8, 2020

Ops, very sorry, I had reviewed this code, but it looks like the comments had got stuck behind GitHub's "pending comments" feature. Published them now.

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