Closed jamwaffles closed 3 years ago
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.
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.
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.
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.
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.
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?
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.
Methods like Ssd1306::draw
are currently available for all modes. Shouldn't these only be available in basic mode?
Sweet!
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?
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?
Here are some random comments/questions about the Ssd1306
struct I noticed while reading the docs:
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.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.set_draw_area
if end.0
or end.1
is zeroreset
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.
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.
And the column part of set_draw_area
would also be useful for page mode.
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.
The basic mode could also use a clear
method.
I was taking a look at moving the init around and realised it results in two unwrap()
s (or two Result
s 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:
bool
now.position
set_addr_mode
as in master
set_display_on
that takes a bool
is fine, but I've added the set_
prefix at least.saturating_sub
now.Ssd1306
impl as all the methods are then attached to the instance.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.
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?
Which is quite cumbersome IMO, so maybe it's a better idea to add
Ssd1306::new_basic
(or justnew
),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.
Regarding set_draw_area
: The docs for this method don't explain that the end coordinates are not included in the drawing area.
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.
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.
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 :)
Hi! Thank you for helping out with SSD1306 development! Please:
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
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 variousimpl
blocks on theSsd1301
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 ofProperties
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 inGraphicsMode
, but also allows blatting whatever you've drawn by writing garbage to the display withdisplay.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:GraphicsMode
andTerminalMode
look similar: