search

almindor / mipidsi

MIPI Display Serial Interface unified driver
MIT License
108 stars 46 forks source link

add invert_colors support #36

Closed almindor closed 1 year ago

almindor commented 1 year ago

Adds Builder::invert_colors(bool) to fix #33 and surplant #34

almindor commented 1 year ago

@cdsupina could you please try this PR using Builder::with_color_order(ColorOrder::bgr) and Builder::with_invert_colors(true) to see if it works ok ?

Thanks

cdsupina commented 1 year ago

I tried this branch and it appears like using just Builder::with_color_order(ColorOrder::Bgr) makes the colors look as they should. If I also use Builder::with_invert_colors(true), the colors are inverted.

almindor commented 1 year ago

I tried this branch and it appears like using just Builder::with_color_order(ColorOrder::Bgr) makes the colors look as they should. If I also use Builder::with_invert_colors(true), the colors are inverted.

Hmm I wonder if issuing INVOFF is what does it in this case. I made it so that either INVOFF or INVON now gets called depending on what was configued (default would be INVOFF). It seems your display simply has INVON by default. Could you try commenting out line 39 of src/models/st7735s.rs so that neither is called and see what happens?

rfuest commented 1 year ago

Hmm I wonder if issuing INVOFF is what does it in this case. I made it so that either INVOFF or INVON now gets called depending on what was configued (default would be INVOFF). It seems your display simply has INVON by default. Could you try commenting out line 39 of src/models/st7735s.rs so that neither is called and see what happens?

I don't think this is the case and I expect that INVOFF and no INV.. command will behave the same. The difference between inverted and non inverted display seems to be related to the LCD glass itself. I believe that TN and IPS panels have different default states, where one lets light through if no voltage is applied and the other one blocks the light with no voltage.

almindor commented 1 year ago

Hmm I wonder if issuing INVOFF is what does it in this case. I made it so that either INVOFF or INVON now gets called depending on what was configued (default would be INVOFF). It seems your display simply has INVON by default. Could you try commenting out line 39 of src/models/st7735s.rs so that neither is called and see what happens?

I don't think this is the case and I expect that INVOFF and no INV.. command will behave the same. The difference between inverted and non inverted display seems to be related to the LCD glass itself. I believe that TN and IPS panels have different default states, where one lets light through if no voltage is applied and the other one blocks the light with no voltage.

No, you're right. I got confused thinking @cdsupina 's test was with INVOFF + BGR but he said INVON + BGR was wrong, which is correct. So this should now be solved. I will do more testing with my own displays to make sure it works right there as well and merge after.

cdsupina commented 1 year ago

Thanks for tackling this issue. Let me know if you need any more tests from me.

almindor commented 1 year ago

Thanks for tackling this issue. Let me know if you need any more tests from me.

Sure thing. I have "a" solution ready, but I think I'll refactor things a bit more. Feel free to use the PR for now so you don't get stuck on your end. The final version should be compatible from the user's perspective.

almindor commented 1 year ago

@cdsupina this should now no longer require passing the invert flag for ST7789 and ST7735s display models as that seems to be the default. Still a WIP as I'm not happy with the init code atm. will want to try and refactor things into something more readable.

almindor commented 1 year ago

I'm going to merge this so we can concentrate on refactoring without having multiple PRs around. This won't get released just yet though. @cdsupina I hope this is ok for now. I'm hoping to have the final next release done in a week or two at worst, you can use this PR or the master branch in the meantime. Things shouldn't change much API wide (we might introduce the new orientation changes into this release though)