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 the support for displays without a RST pin #4

Merged
merged 4 commits into from
Feb 25, 2022
Merged

add the support for displays without a RST pin #4

merged 4 commits into from
Feb 25, 2022

Conversation

pyaillet
Copy link
Contributor

The choice was made here to rely on an internal NoPin implementation to hide the details
and expose it as an Alias type.

Proposal fix for #3

It would be used like this :

let display = DisplayNoRST::st7789_without_rst(di);

And it would be defined as:

  pub struct Twatch<'a> {
      pmu: Pmu<'a>,
      display: DisplayNoRST<EspSpi2InterfaceNoCS, mipidsi::models::ST7789>,
      [...]
  }

The choice was made here to rely on an internal NoPin implementation to hide the details
and expose it as an Alias type.
src/lib.rs Outdated
@@ -54,6 +55,8 @@ where
orientation: Orientation,
}

pub type DisplayNoRST<DI, MODEL> = Display<DI, no_pin::NoPin, MODEL>;
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need another type tho? I'm thinking making rst: Option<RST> with the additional no_rst contstructor variants will be less confusing.

Copy link
Owner

Choose a reason for hiding this comment

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

It'll also force model implementors to account for this, which atm. the ST7789 doesn't for example (if it works it works by accident. I think I should do a soft reset in that case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the NoPin type or the DisplayNoRST type ?

  1. I am using a custom NoPin type to avoid users to choose a random Pin when they mean to pass a None value for the RST Pin.
  2. I am defining a DisplayNoRST type to avoid leaking the NoPin type externally

I don't know how it would be possible to use Option without needing to provide a specific Pin as a driver user, but any suggestion is welcome.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean 2.

Leaking NoPin is inevitable in this case, but at least the user won't really care, they just do Display::st7789_no_rst(di) or such and that's it from their perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to rewrite according to your remarks.

However you will see in the commit 48ed7ca that I took the init sequence from your old driver, as it solved the problem with the inverted colors for my TTGO T-Watch-2020.

Copy link
Owner

@almindor almindor left a comment

Choose a reason for hiding this comment

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

Great job! thanks. Just a few nits + let's keep the init code change into a separate PR so we don't mix things too much.

src/no_pin.rs Outdated Show resolved Hide resolved

use core::convert::Infallible;

#[derive(Default)]
Copy link
Owner

Choose a reason for hiding this comment

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

could you add a basic documentation point here? just explain it's a dummy for the no-cs case.

src/no_pin.rs Show resolved Hide resolved
src/models/st7789.rs Outdated Show resolved Hide resolved

write_command(di, Instruction::COLMOD, &[0b0101_0101])?; // 16bit 65k colors
write_command(di, Instruction::INVOFF, &[])?;
Copy link
Owner

Choose a reason for hiding this comment

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

let's keep this to a separate PR so we don't mix logical changes together.

@pyaillet
Copy link
Contributor Author

Many thanks for the review :)

I removed the init seq from this PR and most of the comments shoulds be resolved now.

Copy link
Owner

@almindor almindor left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! I'll make a release after we get the init code in and tested out.

@almindor almindor merged commit 1b7d8e2 into almindor:master Feb 25, 2022
@pyaillet pyaillet deleted the optional-rst branch February 25, 2022 21:17
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