yuri91 / ili9341-rs

A WIP, no_std, generic driver for the ILI9341 (and ILI9340C) TFT LCD display
Apache License 2.0
57 stars 50 forks source link

Support for boards that need a non-standard initialization command #29

Closed ivmarkov closed 3 years ago

ivmarkov commented 3 years ago

I'm having issues with running the ILI9341 driver on my ESP32-S2-Kaluga-1 board (and BTW yes, Rust runs on ESP32 too!):

Now, none of this is news, and the C drivers support this and other boards with all sorts of #ifdefs special-casing for various boards, like here and here.

Such a support surfaced in one of the other forks of your driver here.

Rather than taking that route, I suggest we just implement an "escape hatch" for users, where they can define their own Orientation enums, by replacing the usage of the hard-coded Orientation enum with a Mode trait, that can be implemented differently for different boards. Orientation enum is still there, it is just the "default" Mode trait implementation. Over time, one can define a few such custom enums for more popular boards directly in the driver code.

With all that said, I'm agnostic as to how exactly this flexibility is modelled, as long as users (me included) have a way to configure their board. :)

yuri91 commented 3 years ago

Hi!

Thanks for bringing up this issue. I was not aware of these different modes used by some devices (also I was not aware of that fork, and since DMA was in my wishlist I may take a look at that too).

Your solution looks fine to me. As far as you know, is this "mode" the only bit of customization needed to initialize these boards, or do we possibly need to allow more complex byte sequences to be sent?

I will take a deeper look at the PR in the next days (I am pretty busy lately), but I am definitely in favor of merging it in some form.

Thanks again :+1:

ivmarkov commented 3 years ago

As far as you know, is this "mode" the only bit of customization needed to initialize these boards, or do we possibly need to allow more complex byte sequences to be sent?

Good question:

yuri91 commented 3 years ago

I was really busy the last month and I had no time to look at this further.

Before merging I would like to come up with a better name for the Orientation enum. Since some boards must implement their own mode, maybe it should be called in a more specific way.

Could you give me an example of the modes available in one of these other boards? How did you implement the trait for your ESP32-S2-Kaluga-1 board?

I am thinking that it could make sense to include here multiple implementation of the Mode trait, with meaningful names so that people can choose the right one.

ivmarkov commented 3 years ago

I was really busy the last month and I had no time to look at this further.

Before merging I would like to come up with a better name for the Orientation enum. Since some boards must implement their own mode, maybe it should be called in a more specific way.

Hmm. But in the end, it is about orientation. The fact that it might also set RGB565 - in a board-specific way - does not change the fact that it is about orientation, because all orientation modes (portrait, landscape, etc.) will have the same RGB setting.

Could you give me an example of the modes available in one of these other boards? How did you implement the trait for your ESP32-S2-Kaluga-1 board?

Check here

I am thinking that it could make sense to include here multiple implementation of the Mode trait, with meaningful names so that people can choose the right one.

I'm OK with that. But this will be an open-ended exercise as new boards are constantly popping up, so in theory you can never be sure that you support ALL boards on the market. That's why I implemented the Mode trait in the first place - this way people can write a small configuration byte sequence for their own board without having to fork and patch your crate - in case their board is not (yet) having an "Orientation" enum in your crate.

tarcieri commented 3 years ago

Not sure if it's helpful, but the accelerometer crate defines an Orientation enum

ivmarkov commented 3 years ago

Not sure if it's helpful, but the accelerometer crate defines an Orientation enum

Not sure what it has to do with the notion of "orientation" of a LED screen though.

yuri91 commented 3 years ago

I though a bit more about the alternatives, and in the end I think that your current PR is the best solution. It doesn't get in the way of people that use the default (albeit maybe now they need to import the Mode trait to make it work?), and gives a minimal escape hatch for those who need it.

I was hoping for a "clean" solution that separates the notion of orientation from the board quirks, but it seems that they are really mixed up...

ivmarkov commented 3 years ago

Do you plan to bump the version number and publish a new release on crates.io?