Allow reclaiming display pins

[bugadani]

• Check that you've added documentation to any new methods
• Rebase from master if you're not already up to date
• Add or modify an example if there are changes to the public API
• Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)
• Run rustfmt on the project with cargo fmt --all - CI will not pass without this step
• Check that your branch is up to date with master and that CI is passing once the PR is opened

PR description

Hi!

I have a device that can physically cut supply to the display. In order to do that, I must ensure that the pins are floating or low, so I must be able to deconstruct the display driver to reclaim the pins.

This is a no-brainer implementation of the above functionality. Any feedback is welcome, if there is a better place to do this, or if the feature is already there but I'm blind. I've also bumped the display-interface dependencies because I rely on those as well.

Other todo items will be checked once this PR can get a green light.

Thank you :)

[jamwaffles]

Hi, sorry for the late review! Two issues I found while testing this PR:

2. You actually end up needing to call disp.release().release() to free up the interface, however...
3. ... ssd1306::mode::displaymode::DisplayModeTrait must be imported for the first release() to be resolved.

@therealprof Do you have any thoughts on this?

[therealprof]

I think either way should be fine (single or double-release that is).

[bugadani]

In fact, to release everything, I need 3 :)

let interface = graphics_mode.release().release();
let (spi, dc, cs) = interface.release();

Personally, I don't mind these. It's kind of an onion that we have to peel, but in the end, everything should release just its own resources IMO.

Although, on second thought, the user doesn't necessarily face/know about the DisplayProperties object that is returned by GraphicsMode::release() - it was actually a surprise for me, I thought, based on the Builder interface, that the SPIInterface object would be returned...

Anyway, this PR is kind of the minimal change to implement a functionality I need, without breaking existing API. I admit, using it is kinda unwieldy.

[jamwaffles]

In fact, to release everything, I need 3 :)

Ah jeez that's not good 😂

Although, on second thought, the user doesn't necessarily face/know about the DisplayProperties object that is returned by GraphicsMode::release()

Yeah, it's a somewhat internal object to the driver, even though it's pub.

How about changing DisplayModeTrait<DI>::release to free the actual interface resource and add DisplayModeTrait<DI>::into_mode<MODE: DisplayModeTrait<DI>>(self) -> MODE to convert display modes?

[bugadani]

My concern with that is breaking existing code. But if you think that can be justified, it sounds fine.

[jamwaffles]

I'd be ok with that personally. I don't think the current release method is used much/at all in the wild. @therealprof might have a different opinion though

[therealprof]

I think it is wildly used but I don't mind breakage for quality-of-life improvements at all. In the end we're still learning and evolving the whole ecosystem quite a bit...

[bugadani]

All right, I'll cook something up soon :) Thanks!

[bugadani]

I've implemented release as discussed, but not into_mode yet. Currently, DisplayMode has a similar into method that is used to covert the object that Builder returns, but nothing else. I wonder if DisplayMode shouldn't be removed and the Into trait implemented for DisplayModeTrait object.

Also, this still needs use ssd1306::mode::displaymode::DisplayModeTrait

[therealprof]

Yeah, I guess we can remove some more layers some time.

[therealprof]

@jamwaffles How do we want to proceed with this with #119 in mind? Land it before or defer until #119 is in?

[jamwaffles]

I don't have a lot of bandwidth to check the conflicts at the moment, but my gut feeling would be to land #119 first

[bugadani]

This doesnt have conflicts and 119 is easy to rebase, but their order matters little IMO

[jamwaffles]

After another quick look, I think we should rename .properties() to .into_properties() or something else that conveys the consumption of the display into a Properties object. Currently, .properties() sounds to me like it should return a reference or something else non-destructive.

[bugadani]

I think we should rename .properties() to .into_properties()

Agreed & done.

[bugadani]

uh I didn't notice 08a7b70 accidentally made it in #119... sorry about that, I guess I messed up rebasing :/ Anyway, I updated this one failed to update this one, looks like

[jamwaffles]

Found some small issues

[jamwaffles]

Thanks!