rust-embedded-community / ssd1306

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

Using `DrawTarget::clear` is difficult in buffered graphics mode #174

Closed rfuest closed 1 year ago

rfuest commented 1 year ago

Description of the problem/feature request/other

There are two different clear methods for Ssd1306<DI, SIZE, BufferedGraphicsMode<SIZE>>: One in an impl (https://docs.rs/ssd1306/latest/src/ssd1306/mode/buffered_graphics.rs.html#66-191) and the other in the DrawTarget trait. The non trait impl seems to take precedence and the compiler doesn't even output an error message that multiple clear methods exist, like it would when both clear methods would be declared in traits.

I'm not sure what the best solution to this is. From an e-g standpoint I would suggest to remove the non DrawTarget clear method, because a user might expect clearing to work like in the examples (https://github.com/embedded-graphics/examples/blob/main/eg-0.7/examples/demo-analog-clock.rs#L195) or other drivers (https://github.com/almindor/mipidsi#example). But that would create some inconsistency between the different modes.

Test case (if applicable)

display.clear(BinaryColor::On).unwrap(); 
error[E0061]: this function takes 0 arguments but 1 argument was supplied
  --> examples/hello-world.rs:88:15
   |
88 |     ssd1306_0.clear(BinaryColor::On).unwrap();
   |               ^^^^^ --------------- argument of type `BinaryColor` unexpected

This works as expected:

DrawTarget::clear(&mut ssd1306_0, BinaryColor::On).unwrap();
bugadani commented 1 year ago

One (bad?) option would be to only provide the inherent clear if the graphics feature is not enabled. Inconsistent for sure, but I'm guessing DrawTarget will be most likely imported.

rfuest commented 1 year ago

One (bad?) option would be to only provide the inherent clear if the graphics feature is not enabled. Inconsistent for sure, but I'm guessing DrawTarget will be most likely imported.

A simpler solution would be to remove the clear method just for the BufferedGraphicsMode: https://github.com/jamwaffles/ssd1306/blob/cafc5a112b02d4ff61d23865ad48bd65a61dfe79/src/mode/buffered_graphics.rs#L72-L82 But it's a trade-off between a consistent API for all Ssd1306 modes and consistency between different e-g targets.

bugadani commented 1 year ago

Buffered graphics mode has limited utility without embedded-graphics anyway. I mean, it can draw pixels one by one, but that's probably not an API people would actively rely on, I hope. Still, removing the option seems a bit drastic if such users exits.

bugadani commented 1 year ago

Yeeeeah, let's not remove buffered graphics mode's clear. On a 240MHz ESP32-S3, clearing the SSD1306 through embedded-graphics takes 1.3ms. This clear on the other hand is in the us range.

rfuest commented 1 year ago

Yeeeeah, let's not remove buffered graphics mode's clear. On a 240MHz ESP32-S3, clearing the SSD1306 through embedded-graphics takes 1.3ms. This clear on the other hand is in the us range.

The DrawTarget impl doesn't currently override the default clear implementation, which makes clear fall back to the slow default implementation, which uses set_pixel. But that should be easily fixed, by moving the current clear method into the DrawTarget impl.