Add LCD ili9225 #181
Add LCD ili9225 #181
Conversation
Interesting, DCS should be universal. Do you have a link to the documentation you used for this? If we do need to have differing DCS between models, I think as you said, traits might be the best approach, or we could hide things in submodules conditionally, similar to your approach but module level to keep things nicer. @rfuest what do you think? |
|
According to [TFT Drivers](https://github.com/Bodmer/TFT_eSPI/tree/master/TFT_Drivers) and [this specific line in TFT_eSPI.cpp](https://github.com/Bodmer/TFT_eSPI/blob/5793878d24161c1ed23ccb136f8564f332506d53/TFT_eSPI.cpp#L3360), it's clear that the instructions are different, even the instruction widths vary. My implementation is based on these references. |
I'm on the fence about adding this. The ILI9225 doesn't follow the DCS command set at all and uses something custom. Adding support for this controller would extend the scope of this crate beyond DCS displays (excerpt from the README: "This crate provides a generic display driver to connect to TFT displays that implement the MIPI Display Command Set." But on the other hand adding something with a completely different command set would ensure that our code is flexible enough to handle more display types. If we decide to add this it shouldn't require conditional compilation. The struct Display<...> {
...
fn set_address_window<DI: ...>(&mut self, di: &mut DI, sx: u16, sy: u16, ex: u16, ey: u16) -> Result<(), DI::Error> {
// add clipping offsets if present
let mut offset = self.options.display_offset;
let mapping = MemoryMapping::from(self.options.orientation);
if mapping.reverse_columns {
offset.0 = M::FRAMEBUFFER_SIZE.0 - (self.options.display_size.0 + offset.0);
}
if mapping.reverse_rows {
offset.1 = M::FRAMEBUFFER_SIZE.1 - (self.options.display_size.1 + offset.1);
}
if mapping.swap_rows_and_columns {
offset = (offset.1, offset.0);
}
let (sx, sy, ex, ey) = (sx + offset.0, sy + offset.1, ex + offset.0, ey + offset.1);
MODEL::update_address_window(&mut self.di, sx, sy, se, ey)
}
...
}
trait Model {
...
// Default impl for DCS compliant models.
fn update_address_window<DI: ...>(di: &mut DI, sx: u16, sy: u16, ex: u16, ey: u16) -> Result<(), DI::Error> {
di.write_command(dcs::SetColumnAddress::new(sx, ex))?;
di.write_command(dcs::SetPageAddress::new(sy, ey))
}
...
}
impl Model for ILI9225Rgb565 {
...
fn update_address_window<DI: ...>(di: &mut DI, sx: u16, sy: u16, ex: u16, ey: u16) -> Result<(), DI::Error> {
di.write_raw(0x37, &sx.to_be_bytes())?;
di.write_raw(0x36, &ex.to_be_bytes())?;
di.write_raw(0x39, &sy.to_be_bytes())?;
di.write_raw(0x38, &ey.to_be_bytes())?;
di.write_raw(0x20, &sx.to_be_bytes())?;
di.write_raw(0x21, &sy.to_be_bytes())
}
...
} |
|
I remove feature flag and change other LCD using default trait. WIP: Add rotation feature |
|
Finished. I Add new trait to resolve different size of madctl. |
|
I also notice I must rerun display
.set_orientation(Orientation::default().rotate(config::ROTATION))
.unwrap();
``` |
The builder doesn't use Line 29 in 4e5610d |
src/lib.rs
Outdated
| @@ -153,7 +154,7 @@ where | |||
| // Model Options, includes current orientation | |||
| options: options::ModelOptions, | |||
| // Current MADCTL value copy for runtime updates | |||
| madctl: dcs::SetAddressMode, | |||
| madctl: MODEL::AddressMode, | |||
There was a problem hiding this comment.
Instead of the associated Model::AddressMode type, I would suggest to store the ModelOptions in this struct and add a update_options(&mut DI, options: &ModelOptions) -> Result<(), DI::Error> method to the Model trait.
There was a problem hiding this comment.
I will fix it tomorrow.
There was a problem hiding this comment.
It seems that madctl will never be used again.
There was a problem hiding this comment.
I hadn't noticed that we already store the ModelOptions in Display.
It seems that madctl will never be used again.
Good catch, if that is the case you could just remove it.
There was a problem hiding this comment.
Some test case use this field so i not remove it.
|
I noticed one more issue while I looked at the application note for the ILI9225. The reference code drives the reset pin low for 10 ms and contains a comment that this delay is necessary. According to the datasheet the reset pulse needs to be at least 1 ms long. This timing requirement is violated by our reset implementation, which only drives reset low for 10 µs: Lines 179 to 181 in 4e5610d I would suggest to add a constant for the minimum reset time to the |
There was a problem hiding this comment.
Some more things I notices:
set_tearing_effect,set_vertical_scroll_region, andset_vertical_scroll_offsetare still hardcoded to use DCS commands and would fail on the ILI9225. They should either be supported or disabled.- The init routine doesn't use the
color_inversionsetting.
There was a problem hiding this comment.
I think you missed this previous comment I made: #181 (review)
Please also add an entry to CHANGELOG.md and add the ILI9225 to the supported model lists in src/lib.rs and README.md.
@almindor: Would you like to take another look at this?
What exactly makes you unhappy about this? Would you prefer that the additional methods wouldn't exist at all or do you dislike having default implementations in |
|
color_inversion not used in TFT_eSPI: |
|
|
That looks correct. You can also use the Your code does currently set the |
First of all, I want to thank the maintainer of
mipidsi. This library has been incredibly helpful.I successfully used
mipidsito illuminate an LCD based on the ili9225 controller and wrote a new initialization procedure. However, I noticed that the commands for this controller differ significantly from other modules. Unfortunately, these commands are not implemented as traits, so I'm unsure how to handle them effectively.For now, I've implemented the changes using conditional compilation with features to avoid impacting the existing codebase. While I admit this approach is not particularly elegant, it's the best solution I could come up with. I’d love to hear your thoughts on this PR and whether you think there's room for improvement.