rust-embedded-community / ssd1306

SSD1306 OLED driver
Apache License 2.0
282 stars 69 forks source link

Some general cleanup, address `clear` conflict #185

Closed bugadani closed 1 year ago

bugadani commented 1 year ago

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

PR description

This PR is a set of smaller changes extracted from the other pending pull requests, that are otherwise independent of the bigger changes proposed.

A few notes:

rfuest commented 1 year ago

My opinion on this matter might not be important, but I really don't like the idea of removing a method when a feature is added.

bugadani commented 1 year ago

My opinion on this matter might not be important, but I really don't like the idea of removing a method when a feature is added.

I'm interested in other ways of resolving the conflict while preserving the ability to clear the display without embedded-graphics, if you have any.

rfuest commented 1 year ago

My solution would be to just require e-g for the buffered graphics mode. I think that most users will use e-g anyway and it isn't a huge dependency if you don't want to use e-g and just set pixels. But I'm not sure everyone would like this approach.

rfuest commented 1 year ago

If the buffered mode is changed to use e-g internally (see https://github.com/jamwaffles/ssd1306/pull/169) e-g will become a mandatory dependency anyway.

bugadani commented 1 year ago

My concern is that requiring e-g for the buffered graphics mode will require other potential graphics libraries to implement a compatibility display mode for this driver.

While e-g's framebuffer is a good general idea, this display is pretty much designed to work with a framebuffer, or at least to draw rectangles of pixels to it. I expect a built-in framebuffer to remain useful without embedded-graphics, especially considering the current status of the library.

The underlying issue in my opinion is the poor naming of DrawTarget::clear, which should probably been called fill. clear to me suggest a return to a default state, exactly how the inherent method does here. That the default state isn't configurable, is a side-effect of the display being a monochrome OLED.

While I agree that removing methods by enabling a feature is inconvenient, and has a sizeable surprise factor, the only other viable solution I see is to rename it. But I would prefer other solutions as I feel like clear is... well, pun not intended, pretty clear.

rfuest commented 1 year ago

The underlying issue in my opinion is the poor naming of DrawTarget::clear, which should probably been called fill. clear to me suggest a return to a default state, exactly how the inherent method does here. That the default state isn't configurable, is a side-effect of the display being a monochrome OLED.

The name clear was chosen to match similar operations in other APIs, like OpenGL and Vulkan.

bugadani commented 1 year ago

Considering that clear just cleared the framebuffer and not the actual display (and of course, your input, too), I've changed my opinion and renamed it to clear_buffer. Hopefully this is both a good compromise and a change that will make the function's behaviour clearer.