rust-embedded-community / ssd1306

SSD1306 OLED driver
Apache License 2.0
301 stars 68 forks source link

Code shuffling for output modes #26

Closed therealprof closed 6 years ago

therealprof commented 6 years ago

These changes shuffle the code a around a little bit to make room for the addition of new operating modes with alternative APIs.

jamwaffles commented 6 years ago

Can I close this in favour of #27?

therealprof commented 6 years ago

As mentioned in #27 I'd like to keep things separate so they can be worked on independently. #27 is really just a proof of concept for #26. From my POV we just need to agree on a name for egfx to land this.

jamwaffles commented 6 years ago

As of right now, I see the mode names and functions being this:

I'd like to see a trait called DisplayMode that has the methods reset, init and set_rotation. These are common to all modes so should always be available.

therealprof commented 6 years ago

NoMode: Expose an interface to init the display and send a &[u8] to the display. The simplest driver possbile really.

You mean as in expose just the raw commands? I'd suggest I start with the renaming and then we'll take it from there.

therealprof commented 6 years ago

Instead of NoMode we could also call it RawMode.

jamwaffles commented 6 years ago

You mean as in expose just the raw commands? I'd suggest I start with the renaming and then we'll take it from there.

Erm, I don't think so. Not for now at least; I think commands need a bit more thought. It would essentially just expose send_data() along with the DisplayMode trait fns.

Yeah RawMode sounds better actually. NoMode implies the driver won't do anything by default which isn't true.

therealprof commented 6 years ago

@jamwaffles Is this good to go? I'd like to work on the stuff depending on these changes. I can also squash the changes if you'd like to get rid of the intermediate state.

jamwaffles commented 6 years ago

I'm liking where this is going, but two last things:

  1. Is it worth adding a send_raw_bytes() (or some other named) method to RawMode? We're exposing it from the crate but it isn't very useful to the user right now. Could just not make it public?
  2. I'd really like to see a trait that implements init() and set_rotation() (and maybe get_dimensions() if that makes sense for all the graphics modes you can think of). What do you think?
therealprof commented 6 years ago

I agree we should have that. I just don't want to overload this one with too much stuff and would rather have this as a dedicated follow up. At the moment this is blocking the CharacterMode implementation and a number of consolidation and cleanup changes so I'd rather have this completed ASAP so we're not restricted with the order of the followup changes.

jamwaffles commented 6 years ago

Okiedoke, LGTM then :). Sorry for holding onto it.