Implement DisplayMode and DisplayTrait for better mode handling

[therealprof]

Signed-off-by: Daniel Egger daniel@eggers-club.de

[therealprof]

@jamwaffles Putting this out there to get some feedback. I was hoping that there was a generic way to use From/Into traits to stay with the DisplayMode object and just make it change modes on the fly and allow for easy introduction of new modes without having to teach the DisplayMode impl about it. Also I wanted to use Deref to get automatic type coercions to the underlying mode but I couldn't make it work with generic types.

[jamwaffles]

So I've had a quick play around with this and I seem to have been able to get rid of the DisplayMode trait and impl From for GraphicsMode. My only beef is that this way we'd have to make the user specify the type of display they want to .into(), which gets pretty noisy. It does mean the driver is quite a bit cleaner though.

[therealprof]

Can the mode still be switched at a later point? Specifying the mode is quite okay for me because that's exactly what a user should do, I'd rather try to get rid of other noise (like specifying the DisplayInterface all the time).

[therealprof]

The idea behind the trait is to have a common interface that would allow to easily implement new modes and have a guaranteed API for this.

[jamwaffles]

Sorry, was half way through typing this when you commented so I'll post it as a FWIW:

I've also managed to nicely fix the DisplayTrait naming problem.

I've also just readded into_graphicsmode().

---

Can the mode still be switched at a later point?

Haven't tested yet! I'll implement a dummy mode and see how far I get.

I'd rather try to get rid of other noise (like specifying the DisplayInterface all the time).

Where do you mean? In the driver or in application code?

The idea behind the trait is to have a common interface

Right, of course! I'll keep playing... there's still the naming problem. I'll try and think of something.

[therealprof]

I've also managed to nicely fix the DisplayTrait naming problem.

Uhm... :-D

I've also just readded into_graphicsmode().

That's specifically what I wanted to get rid of: It's not scalable (requires methods in every mode for every other available mode) and does not allow for custom modes.

Where do you mean? In the driver or in application code?

Everywhere, I find it a major PITA to manage all the different generics and bounds plus if you get a small detail wrong it's really easy to get a crapload of non-saying errors (it actually is a lot more readable when rendered by GH compared to a terminal):

error[E0308]: mismatched types
  --> examples/blinky_i2c.rs:91:15
   |
91 |         DISP: disp,
   |               ^^^^ expected struct `ssd1306::mode::GraphicsMode`, found struct `ssd1306::mode::displaymode::DisplayMode`
   |
   = note: expected type `ssd1306::mode::GraphicsMode<ssd1306::interface::I2cInterface<blue_pill::i2c::I2c<blue_pill::<unnamed>::I2C1, (blue_pill::gpio::gpiob::PB8<blue_pill::gpio::Alternate<blue_pill::gpio::OpenDrain>>, blue_pill::gpio::gpiob::PB9<blue_pill::gpio::Alternate<blue_pill::gpio::OpenDrain>>)>>>`
              found type `ssd1306::mode::displaymode::DisplayMode<ssd1306::properties::DisplayProperties<ssd1306::interface::I2cInterface<blue_pill::i2c::I2c<blue_pill::<unnamed>::I2C1, (blue_pill::gpio::gpiob::PB8<blue_pill::gpio::Alternate<blue_pill::gpio::OpenDrain>>, blue_pill::gpio::gpiob::PB9<blue_pill::gpio::Alternate<blue_pill::gpio::OpenDrain>>)>>>, ssd1306::mode::GraphicsMode<_>>`

[jamwaffles]

The diff was supposed to show that I had removed the DisplayMode struct and replaced it with a trait :D

I see your point, it's ok for maybe two modes but for any more than that it gets unwieldy. That said, aren't we going to have to do some sort of "web of into()s" anyway? Even implementing From for a bunch of modes still requires writing that code everywhere, or am I missing something?

I found those error messages earlier. They're just awful.

[therealprof]

The diff was supposed to show that I had removed the DisplayMode struct and replaced it with a trait :D

Well, in my version there's both a struct and a trait. Maybe we can get rid of the struct and pack the into magic into the trait...

That said, aren't we going to have to do some sort of "web of into()s" anyway?

I was kind of hoping we could find a neat trick with with the trait types/states that would automatically change the interface to whatever type the DisplayMode is in, similarly as we do it with the GPIO pins in the stm32f103xx-hal. Maybe keep the "properties" in the DisplayMode struct and provide a way for a DisplayTrait to access it from there.

Even implementing From for a bunch of modes still requires writing that code everywhere, or am I missing something?

The point of the trait was/is that all concrete modes provide the same new()/release() interface to switch into them or out of them so that one could have a generic from()/into() implementation that just says: For ever mode implementing the trait, just call release() on the current instance to get the display properties (configuration, state and interface) and then call new() on the new mode passing the same in.

I really haven't had much luck with the from() and into() implementation though because the compiler complains that they have the same specificity as the automatic generic implementations. :(

But every attempt I'm going after takes forever just to dance around the <DI> parameter everywhere to get the bounds straight. If there's any way to eliminate that low level detail from the higher level interface we should totally go for it.

[jamwaffles]

I've been trying to write something nice using impl From but it doesn't seem possible; I think you need a concrete type instead of a trait bound, which seems odd to me as traits have defined method descriptions... It seems the web of impl Froms is the way to go for now.

I like the idea of the common trait having new()/release(). For now, could we just impl From for the two graphics modes we want to support? It would be great to find a more generic solution in the future, but with two modes you'd just need to add From<RawMode> and From<TheOtherMode>, which are both nearly oneliners as far as I can see.

Also, I still like the cleanliness of not having to define a spaghetti type for .into_<mode>mode() present on the builder.

[therealprof]

I managed to make the into() work:

     ///
     pub fn into<DI, NMODE: DisplayTrait<DI>>(self) -> DisplayMode<NMODE>
     where
         DI: DisplayInterface,
         MODE: DisplayTrait<DI>,
     {
         let properties = self.0.release();
         DisplayMode(NMODE::new(properties))
     }

However what's missing is the automatic coercion from DisplayMode to concrete mode... and I again cannot figure it out so the only way to use it is to make the MODE pub and fetch the .0 like:

let mut disp : GraphicsMode<_> = Builder::new().connect_spi(spi, dc).into().0;

But again that prevents later mode changes...

[jamwaffles]

Looking good! I was going through the diff and didn't see a .0 so is that a typo in your comment? The GraphicsMode<_> fixes one of my big worries. The SPI type declaration gets pretty ridiculous, so it's nice not to have to type the whole thing out.

Could you impl From instead of having an into() method on DisplayMode itself? If I understand correctly, that would let you get rid of .into() when you declare a mode to cast to.

Would impl From for each mode with your pub fn into<DI, NMODE: DisplayTrait<DI>>(self) -> DisplayMode<NMODE> signature allow casting between modes?

[jamwaffles]

Out of curiosity, what are you use cases for switching mode on the fly? I'm thinking something like exception handling where you can switch the display into char mode and print a message but I might be off the mark there. It's something I'd like to support, but to keep this PR rolling if it means implementing a web of into()s for each mode for the time being, I'm ok with that.

[therealprof]

Looking good! I was going through the diff and didn't see a .0 so is that a typo in your comment? The GraphicsMode<_> fixes one of my big worries. The SPI type declaration gets pretty ridiculous, so it's nice not to have to type the whole thing out.

There're two options: Turning into a DisplayMode which requires the .0 (and also making this part of the struct pub) or turning into a mode directly. I chose the latter.

Could you impl From instead of having an into() method on DisplayMode itself? If I understand correctly, that would let you get rid of .into() when you declare a mode to cast to.

Nope and nope. 😉

I cannot impl either From or the Into traits (at least I haven't managed to do so the last few days) because I cannot make the impl specific enough to not clash with the generic implementations.

Would impl From for each mode with your pub fn into<DI, NMODE: DisplayTrait>(self) -> DisplayMode signature allow casting between modes?

Yes! But I cannot make it work. 🙁

Out of curiosity, what are you use cases for switching mode on the fly?

I haven't really though about it. Just thought it would be nice to have.

It's something I'd like to support, but to keep this PR rolling if it means implementing a web of into()s for each mode for the time being, I'm ok with that.

Hm, I maybe I can implement the same trick I did with the into from DisplayMode. Let me try that.

[therealprof]

Hm, I maybe I can implement the same trick I did with the into from DisplayMode. Let me try that.

Nope, doesn't seem so. :(

[jamwaffles]

Shame :(. I'd rather add an API method in the future that's right than add one now that we'll have to break later. I think we should reduce the scope of this feature a bit and just focus on a clean .into() implementation without the ability to switch between modes for now. I think that'd be useful, but I don't see it as a required feature for a 0.1 or 1.0 release. Is there any cleanup that needs to be done to merge this PR with just the changes that make .into_graphicsmode() become .into() with a type? If you can get that ready I'm happy to merge this.

P.s. I've pushed a couple of small hotfixes to master so you'll have to rebase again, sorry.

[therealprof]

Should be all there?

[jamwaffles]

Yeah looks like it :) shame it didn't work out perfectly but I like the .into() changes.