-
Notifications
You must be signed in to change notification settings - Fork 46
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 the ILI9342C #25
Conversation
Thank you! I'll have a closer look in a day or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just two things I think need attention before we merge it in.
DELAY: DelayUs<u32>, | ||
DI: WriteOnlyDataCommand, | ||
{ | ||
let madctl = options.madctl() ^ 0b0000_1000; // this model has flipped RGB/BGR bit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a mistake on my end with ST7789. Are you sure it's required for this model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this display is in RGB mode by default. For whatever reason my colours still seem to be displaying using BGR (setting the background to red displays as blue) without that, so I think this is why I had set that initially. I will dig into the datasheet a bit more and try to figure out what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what's going on here, as far as I can tell the display is configured correctly. I am unable to get it to work properly without inverting this bit, the byte order is always backwards. I did find a couple more drivers for this display, and they both seem to be setting the RGB/BGR bit to 1 (BGR), so I'm not totally crazy at least (well, I hope).
- https://github.com/retro-esp32/Odroid-GO-ILI9342/blob/master/ILI9342/Arduino%20Libraries/Adafruit_ILI9342/Adafruit_ILI9342.cpp#L160
- https://github.com/MilovdZee/TFTLCD-Library_ILI9342/blob/master/Adafruit_TFTLCD_ILI9342.cpp#L150
Not sure how you'd like to proceed on this. I can update the comment if that is adequate, otherwise I'm not sure what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting, the first driver inverts RGB but also sets MX bit. The second driver also inverts RGB but then sets the MY bit. So they don't even agree between each other. I guess one is aiming for portrait mirror image, while the other goes for inverted portrait orientation.
src/models/ili9342c.rs
Outdated
let madctl = options.madctl() ^ 0b0000_1000; // this model has flipped RGB/BGR bit; | ||
|
||
write_command(di, Instruction::SLPOUT, &[])?; // turn off sleep | ||
write_command(di, Instruction::COLMOD, &[0b0101_0101])?; // 16bit 65k colors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be dynamic based on the color type. It's ok for the 565 variant but I wonder if your 666 case didn't work because of this?
I made the same mistake in the ILI9486 driver (I never could've tested rgb565 because they disabled it in hw for the board I have so I just found this bug).
It can probably be made compile time choice tho based on the color type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you for pointing that out. Changing to 18bit/262k colours at least made the display not work in a different way using Rgb666
😁 I'll keep digging.
EDIT: this seems to be related to some sort of weird optimization. For context, the application I'm using for testing just sets the background to white and writes some text in red. Currently when building using the release
profile the text colour is correct but the background is entirely black. Using the dev
profile the background is white as it should be. It works correctly using release
as long as I set the opt-level
to anything other than 2 or 3. This does not happen when using Rgb565
for whatever reason. I will add this code to the PR regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this would imply some sort of timing issue. What SPI MODE are you using? Does the display have a CS pin?
I'll merge this soon but let's see if we can debug this further, I had something similar happen before on a non-CS display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ended up being a bit of a red herring.
I was originally testing using esp32s3-hal
(which is no_std
), but just as a sanity check I also tested this driver using esp-idf-hal
/esp-idf-sys
(which are std
, a separate project) and this works at all optimization levels. So it appears this issue is somewhere in the esp-hal
SPI driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! It usually is on the SPI level at that point. Thanks again for the model
d3142ca
to
b876df0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I understand the Rgb666 format still doesn't work right. I'll give it a bit more time so we can look into it but if we get stuck I'll merge as-is.
{ | ||
/// | ||
/// Creates a new [Display] instance with [ILI9342C] as the [Model] | ||
/// *WARNING* Rgb565 only works on non-SPI setups with the ILI9342C! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your pull request description mentions using this on an ESP32-S3-BOX but I also see that you were only able to use 565 and not 666. I thought the box was using SPI - is this documentation that 565 only works with non-SPI setups correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the docs are wrong. The ILI934X controllers support RGB565 and RGB666 via SPI. The comment was probably copied from the ILI9486 support, where it would be correct.
First off, thank you for your work on this project! I had previously attempted to implement this driver from scratch and lost motivation, however it was trivial to implement in this repository.
The ILI9342C is used by the ESP32-S3-BOX, which is what I used for verification. Based on some very simple testing this seems to be working as intended.
Some notes on this implementation:
ili9486
model, with some small tweaksRgb565
; I did tryRgb666
however the colours were not correct, and I did not spend much time trying to troubleshootIf you'd like anything changed or added please let me know!