Closed bugadani closed 4 years ago
Hi, sorry for the late review! Two issues I found while testing this PR:
disp.release().release()
to free up the interface, however...ssd1306::mode::displaymode::DisplayModeTrait
must be imported for the first release()
to be resolved.@therealprof Do you have any thoughts on this?
I think either way should be fine (single or double-release that is).
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.
In fact, to release everything, I need 3 :)
Ah jeez that's not good :joy:
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?
My concern with that is breaking existing code. But if you think that can be justified, it sounds fine.
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
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...
All right, I'll cook something up soon :) Thanks!
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
Yeah, I guess we can remove some more layers some time.
@jamwaffles How do we want to proceed with this with #119 in mind? Land it before or defer until #119 is in?
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
This doesnt have conflicts and 119 is easy to rebase, but their order matters little IMO
I think we should rename
.properties()
to.into_properties()
Agreed & done.
uh I didn't notice https://github.com/jamwaffles/ssd1306/pull/119/commits/08a7b70c9942c86ffd339c8128465c63a12d6d64 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
Thanks!
master
if you're not already up to dateCHANGELOG.md
entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)rustfmt
on the project withcargo fmt --all
- CI will not pass without this stepPR 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 :)