Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some general cleanup, address clear conflict #185

Merged
merged 5 commits into from
Jun 1, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented May 18, 2023

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 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:

  • Fixes Using DrawTarget::clear is difficult in buffered graphics mode #174 by removing the inherent clear function to clear_buffer. This new name is hopefully more descriptive.
  • Clearing the display on an ESP32 before this patch took 1.4ms, measured with a tracing pin and an oscilloscope. After, this time got down to around 100us but I haven't precisely measured that, I was happy with seeing just the relative difference.
  • I'm deduplicating the reset fn as it's the same for all display interfaces.
  • There's allow(dead_code) on a type that doesn't have dead code warnings.

@bugadani bugadani changed the title Some general cleanup Some general cleanup, address clear conflict May 18, 2023
@rfuest
Copy link

rfuest commented May 18, 2023

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
Copy link
Contributor Author

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
Copy link

rfuest commented May 18, 2023

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
Copy link

rfuest commented May 18, 2023

If the buffered mode is changed to use e-g internally (see #169) e-g will become a mandatory dependency anyway.

@bugadani
Copy link
Contributor Author

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
Copy link

rfuest commented May 18, 2023

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
Copy link
Contributor Author

bugadani commented May 18, 2023

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.

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙏

@jamwaffles jamwaffles merged commit 23c6887 into rust-embedded-community:master Jun 1, 2023
@bugadani bugadani deleted the cleanup branch June 1, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using DrawTarget::clear is difficult in buffered graphics mode
3 participants