Experimental refactoring

[rfuest]

I never really liked the way the builder and ModelOptions worked. This PR is an experiment to improve that code a bit. At this point this is only an experiment and it might not be the best way of implementing this. The PR does only implement the changes for the ILI9341 controller at the moment and different display sizes, offsets and orientations won't work correctly.

Changes:

• Models no longer implement constructors like Builder::ili9341_rgb565. All controller types now use Builder::new, with an optional type annotation like Builder::new::<ILI9341<Rgb565>>(di).
• This also means that variants like Builder::st7789_pico1 are no longer supported. IMO this isn't a problem and we should focus on supporting controllers and not different display modules. There are too many modules to support them all and many generic modules can be different even if they seem to have the same name. Having good documentation on how to get started with a new display module is more useful than some builtin variants. For hardware with a BSP crate the display initialization could be handled in that crate instead.
• ILI9341Rgb565 and ILI9341Rgb666 were replaced by ILI9341<Rgb565> and ILI9341<Rgb666>.
• The reset pin is no longer passed to init and is a now a builder setting instead. I'm not sure why it was implemented this way before, but the Display struct does take ownership of the pin and it isn't only used by init (like the delay).
• ModelOptions is removed and all options are now handled by Display directly. The builder uses a Display struct internally and the controller specific init methods can now simply take a &mut Display argument instead of manually passing dcs, options, and rst.

I would appreciate some feedback, whether you think that this is an improvement or not worth the effort.

[almindor]

I like this approach. I'm also thinking (from GrantM11235's discussion on matrix) that removing the Model from the type could be done and init could just take it as &dyn Model for the individual init calls.

This way Display ends up being one type leaner but people can still implement their models in external crates, which I think is what we'd want in the end. I cannot keep supporting models in-tree here since I cannot actually even test most of them.

Perhaps there's an easier way to allow outside crates to provide the model functionality tho?

[rfuest]

I'm also thinking (from GrantM11235's discussion on matrix) that removing the Model from the type could be done and init could just take it as &dyn Model for the individual init calls.

I can't immediately see how this could work. Model isn't object safe and I don't think that it would be easy to make it object safe.

This way Display ends up being one type leaner...

But if the Model doesn't provide the color type we would need a new color parameter and end up with the same number of types. Keeping the Model type does also provide us more options for possible future extensions which aren't common across all models.

... but people can still implement their models in external crates, which I think is what we'd want in the end. I cannot keep supporting models in-tree here since I cannot actually even test most of them.

Perhaps there's an easier way to allow outside crates to provide the model functionality tho?

I prefer to keep all models in one crate, but I agree that testing is an issue. Making it possible to implement support for new controller types externally shouldn't be to hard, by reverting init back to something that is similar to before this PR. ModelOptions could be changed into a simple data object with public fields, which gets filled by the Builder and then passed to init.

[rfuest]

The only thing remaining in this PR, that could be a good idea to implement at some point is to use a type attribute to distinguish between the different color formats that are supported by a controller, e.g. ILI9431<Rgb565> and ILI9341<Rgb666> instead of ILI9431Rgb565 and ILI9431Rgb666. At the moment this would primarily be a stylistic change, but it could later have the benefit of being able to add a Display::into_color_format method to change the color format after initialization. But that's a really uncommon operation and I think we should just stick with the current way the different color formats for the next stable release.

[almindor]

The only thing remaining in this PR, that could be a good idea to implement at some point is to use a type attribute to distinguish between the different color formats that are supported by a controller, e.g. ILI9431<Rgb565> and ILI9341<Rgb666> instead of ILI9431Rgb565 and ILI9431Rgb666. At the moment this would primarily be a stylistic change, but it could later have the benefit of being able to add a Display::into_color_format method to change the color format after initialization. But that's a really uncommon operation and I think we should just stick with the current way the different color formats for the next stable release.

I don't disagree with doing this but I think it might be time to get the next major version out the door. I'm trying to get my hardware in order again to test this but if you say this version works fine I think we can release now. We can refactor further later, it's already a huge release and we're not promising 1.0 stability yet.

[rfuest]

I don't disagree with doing this but I think it might be time to get the next major version out the door.

Yes, finally getting a new release out is the reason I don't want to work on this at the moment.

I'm trying to get my hardware in order again to test this but if you say this version works fine I think we can release now.

I haven't done any thorough testing with the latest revision, but ST7789 did work last month, when I tried to fix the CS pin issue reported in #129. The last time I tried GC9A01 and ILI9341 displays was in February and I don't think any of the changes after that should have broken support for these displays.