Restructure crate to simplify usage/contributing

[jamwaffles]

Hi! Thank you for helping out with SSD1306 development! Please:

• 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

This PR heavily refactors the crate structure. It does away with the builder, now requiring the passing in of all 4 config params. It also changes how modes work; instead of wrapping Properties in various other structs, it now has various impl blocks on the Ssd1301 struct with different modes in the type params. IMO this makes the code a lot cleaner whilst still allowing switching between modes easily.

The code structure is a lot more approachable now. The "entry point" Ssd1306 struct is a good jumping off point for contributors. Previously, there was a bunch of Properties and *Mode stuff to dig through, without a clear structure.

I'm interested to hear peoples' thoughts on the changes here.

One thing we miss in terms of type safety is a particular mode still allows raw draw() calls on the display. This allows e.g. drawing stuff with embedded-graphics in GraphicsMode, but also allows blatting whatever you've drawn by writing garbage to the display with display.draw(&[0xab; 1024]) whilst in the same mode. This was a deliberate choice to give users the freedom to do "in-band" stuff in whatever mode they're in, but with an escape hatch to do naughtier things if required.

The noise_i2c example demonstrates a "raw" mode:

let mut display = Ssd1306::new(
    interface,
    DisplaySize128x64,
    NoMode,
    DisplayRotation::Rotate0,
);

GraphicsMode and TerminalMode look similar:

let interface = I2CDIBuilder::new().init(i2c);
let mut display = Ssd1306::new(
    interface,
    DisplaySize128x64,
    TerminalMode::new(),
    DisplayRotation::Rotate0,
);
display.init().unwrap();

let interface = I2CDIBuilder::new().init(i2c);
let mut display = Ssd1306::new(
    interface,
    DisplaySize128x64,
    BufferedGraphicsMode::new(),
    DisplayRotation::Rotate0,
);
display.init().unwrap();

[bugadani]

To be fair, if you're transitioning away from the main builders, maybe this could get some love as well. This struct is just 2 constructors - one with a default address and one with a specific one.

[jamwaffles]

Good shout. I've changed it to a struct with static methods. There's an argument to make them free functions in a module, but it should be better now either way.

[bugadani]

Time for const generics, maybe?

[jamwaffles]

Yes definitely! I decided I want to do them in another PR to keep this one a little shorter.

[rfuest]

I'm not very familiar with this crate and this is only based on the initial comment: Did you consider multiple constructors instead of having to pass a mode object to new?

let raw = Ssd1306::new( // or new_raw
    interface,
    DisplaySize128x64,
    DisplayRotation::Rotate0,
);

let terminal = Ssd1306::new_terminal(
    interface,
    DisplaySize128x64,
    DisplayRotation::Rotate0,
);

let graphics = Ssd1306::new_graphics(
    interface,
    DisplaySize128x64,
    DisplayRotation::Rotate0,
);

This wouldn't be scalable to lots of modes, but for three modes it might be easier to use. And you could still have a Ssd1306::with_mode constructor if necessary.

[jamwaffles]

I did consider that, but decided to go with a single constructor and a mode argument. I'm not sure if there are any benefits between these two approaches, but if there's a drawback to mine I've missed please let me know. Otherwise, I think I'm going to leave it as is.

[rfuest]

I'm not sure if there are any benefits between these two approaches, but if there's a drawback to mine I've missed please let me know.

No, it probably is just personal preference.

But if I try to dig deep this would be the only objective advantage I can come up with (so feel free to just ignore it): The constructors are easier to discover by using code completion, while the values that can be used for the mode parameter in new are unclear without looking at the docs or an example. And the slight inconsistency between NoMode, which can be used without calling new, and the other modes, which require new to be called, is another thing to remember.

You did write that you modeled the code based on the types states in the HAL crates. How about removing the mode from new entirely and rely on into_mode (or into_graphics and into_terminal) to set the actual mode? This would be similar to how HAL handle GPIO pin modes.

If you want to keep the current solution I would suggest to change the order of the new arguments. IMO the display size and rotation should be next to each other.

[jamwaffles]

I've just pushed a commit which updates the initialisation to the following:

let mut display = Ssd1306::new(interface, DisplaySize128x64, DisplayRotation::Rotate0)
    .into_buffered_graphics_mode();

(plus into_terminal_mode). I quite like this as it's simple to switch between modes, but still retains most of the other benefits of the original design. I'd be interested to hear everyone's thoughts on this approach.

[rfuest]

I like it.

Is there a way to get rid of the separate init call, which is required after changing the mode? IMO this is something that could easily be missed.

[jamwaffles]

I did think about that. I left it as a separate method to give some flexibility to the user if they wanted to init the display in another place (e.g. an RTIC interrupt on device wakeup) but that might be too contrived and it's a better idea to initialise the display inside into_*_mode. Thoughts?

[rfuest]

Maybe mode and display initialization should be separated from each other. Like this:
Ssd1306::new(...) -> Result<Self, DisplayError> initializes the basic display settings (everything in init_with_addr_mode). This will need to use some default addressing mode, but changing it later has only minimal overhead. And the into_..._mode methods change the addressing mode and any other required settings.

If directly initializing the display in Ssd1306::new later turns out to be a problem we could also add a new_uninitialized method, which returns a Ssd1306 with a new Uninitialized mode.

[rfuest]

Methods like Ssd1306::draw are currently available for all modes. Shouldn't these only be available in basic mode?

[therealprof]

Sweet!

[jamwaffles]

Maybe mode and display initialization should be separated from each other. ...

That's a good idea. I'll write that up and see what it looks like.

Methods like Ssd1306::draw are currently available for all modes. Shouldn't these only be available in basic mode?

That was a deliberate choice I described in the PR description:

One thing we miss in terms of type safety is a particular mode still allows raw draw() calls on the display. This allows e.g. drawing stuff with embedded-graphics in GraphicsMode, but also allows blatting whatever you've drawn by writing garbage to the display with display.draw(&[0xab; 1024]) whilst in the same mode. This was a deliberate choice to give users the freedom to do "in-band" stuff in whatever mode they're in, but with an escape hatch to do naughtier things if required.

Personally I think this is ok, although there may be some methods that could be moved to BasicMode. Do you have any suggestions?

[rfuest]

OK, sorry I had missed to comment in the PR description. IMO using draw together with the partial flushing in the buffered mode could result in hard to predict results. But I guess as long as possible side effects are documented it's OK.

Is there a way to force a full flush to restore the image to the buffer content after draw is used? Should there also be a method that is similar to draw, but which changes the buffer instead?

[rfuest]

Here are some random comments/questions about the Ssd1306 struct I noticed while reading the docs:

• Why does set_pixel use a u8 for value, instead of a bool or an enum?
• get_position in terminal mode should be called position according to the naming conventions and all other getters also don't use a get_ prefix.
• Is there a reason why it is change_addr_mode instead of set_addr_mode?
• display_on(&mut self, on: bool) is a bit confusing. As a setter it should have a set_ prefix and if it is a command I would expect separate display_on and display_off methods instead of a bool argument.
• There is a possible integer underflow in set_draw_area if end.0 or end.1 is zero
• Does the reset method need to be part of Ssd1306? A simple function in the crate root would also work. I don't think the reset is exclusive to the SPI interface. It might not be exposed in the cheap SSD1306 modules, but the signal can still be used for I2C and parallel interfaces.
• set_rotation exists in Ssd1306 and DisplayConfig. I'm surprised that the compiler doesn't complain if this method is called, but maybe I'm missing something. Is it ensured that the correct version is called if someone uses display.set_rotation?

As I said these are just some things I noticed and are I wanted to leave them as a comment to give some outside perspective on the API. There might be good reasons for the current implementation I simply don't know about.

[rfuest]

One more thing I noticed: set_row and set_column currently panic if the addressing mode isn't page. But the commands also work in the other addressing modes.

[rfuest]

And the column part of set_draw_area would also be useful for page mode.

[rfuest]

set_row and set_draw_area can panic if row is >= 64, which isn't documented.

But perhaps it would be better to use Page parameters in this case? IMO this would make it clearer that the row can only be set to multiples of 8px. If someone is using the low level API they should be familiar with the memory layout and what a page is.

[rfuest]

The basic mode could also use a clear method.

[jamwaffles]

I was taking a look at moving the init around and realised it results in two unwrap()s (or two Results anyway):

let mut display = Ssd1306::new(interface, DisplaySize72x40, DisplayRotation::Rotate0)
    .unwrap()
    .into_buffered_graphics_mode()
    .unwrap();

Which is quite cumbersome IMO, so maybe it's a better idea to add Ssd1306::new_basic (or just new), Ssd1306::new_buffered_graphics, Ssd1306::new_terminal as previously suggested, as well as keep the .into_* methods for conversions at runtime.

Here are some random comments/questions about the Ssd1306 struct I noticed while reading the docs:

Thanks for looking this over. Most of this is old stuff and inconsistencies that should be fixed indeed. Responding in order:

1. Good question. It's a bool now.
2. Changed, it should indeed be just position
3. None at all, I've reverted it back to set_addr_mode as in master
4. Personal preference could lean this either way, but IMO set_display_on that takes a bool is fine, but I've added the set_ prefix at least.
5. Well spotted! I'm using saturating_sub now.
6. IMO it's nicer to provided it as part of the Ssd1306 impl as all the methods are then attached to the instance.
7. Uhh... magic? I still need to test this, but hopefully rustc is smart enough to figure it out. As a half-joke: if it compiles, should it not run correctly?

One more thing I noticed: set_row and set_column currently panic if the addressing mode isn't page. But the commands also work in the other addressing modes.

Hm, maybe there was a historic reason for this but I don't recall. @therealprof can you remember? Either way, I've pushed a commit to allow them to be used in any mode which I can revert if it's incorrect.

[rfuest]

IMO it's nicer to provided it as part of the Ssd1306 impl as all the methods are then attached to the instance.

OK, but should it be limited to SPI?

[rfuest]

Which is quite cumbersome IMO, so maybe it's a better idea to add Ssd1306::new_basic (or just new), Ssd1306::new_buffered_graphics, Ssd1306::new_terminal as previously suggested, as well as keep the .into_* methods for conversions at runtime.

Or: Ssd1306::new returns a mode Uninitialized display, which doesn't require any communication and cannot fail. And then use into_..._mode methods, which can fail and return a Result. In contrast to my last suggestion this would also require a into_basic_mode method.

[rfuest]

Regarding set_draw_area: The docs for this method don't explain that the end coordinates are not included in the drawing area.

[jamwaffles]

I've made a couple of different attempts at the init API now and either they don't work or I'm not happy with them. This crate has seen a lot of downloads, and I haven't come across people having issues forgetting to call init(), so I think I'm going to leave that API as is and merge this refactor PR unless there are any strong objections to it.

This crate doesn't have the highest quality docs for sure, but this PR was originally focussed on the structure of the code more than anything else.

[rfuest]

I've made a couple of different attempts at the init API now and either they don't work or I'm not happy with them. This crate has seen a lot of downloads, and I haven't come across people having issues forgetting to call init(), so I think I'm going to leave that API as is and merge this refactor PR unless there are any strong objections to it.

OK.

This crate doesn't have the highest quality docs for sure, but this PR was originally focussed on the structure of the code more than anything else.

Sorry for all the off topic comments.

[jamwaffles]

Sorry for all the off topic comments.

Not a problem! You brought up some good points which I've added in, so thank you for those :)