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

Add support for the epd1in54b display #35

Merged
merged 4 commits into from
Dec 28, 2019
Merged

Add support for the epd1in54b display #35

merged 4 commits into from
Dec 28, 2019

Conversation

jkristell
Copy link
Contributor

This RFC pull request adds support for the 2 color (red and black) display epd1in54b .

To support the two different planes of colored ink I have added a new trait called WaveshareTwoColorDisplay with on method - update_both_planes() for updating both planes at once. The old update_frame() just clears the color-layer when updating the black layer

@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #35 into master will decrease coverage by 5.52%.
The diff coverage is 2.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   41.26%   35.74%   -5.53%     
==========================================
  Files          15       18       +3     
  Lines         853      996     +143     
==========================================
+ Hits          352      356       +4     
- Misses        501      640     +139
Impacted Files Coverage Δ
src/traits.rs 0% <ø> (ø) ⬆️
src/epd1in54b/graphics.rs 0% <0%> (ø)
src/epd1in54b/command.rs 0% <0%> (ø)
src/epd1in54b/mod.rs 3.1% <3.1%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6520bbf...8f89ade. Read the comment docs.

@caemor
Copy link
Owner

caemor commented Dec 18, 2019

Looks great on a fast skim. I will make a full review next week, but could you in the meantime please add the new device to the Readme and add a short description/doc comments to the new trait?

Copy link
Owner

@caemor caemor left a comment

Choose a reason for hiding this comment

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

See the comment

src/traits.rs Show resolved Hide resolved
@jkristell
Copy link
Contributor Author

Added some documentation.

Perhaps the trait should be named WaveshareThreeColorDisplay instead as that's the terminology that Waveshare use.

@caemor
Copy link
Owner

caemor commented Dec 27, 2019

Then we should use the same terminology so it's easier for people to switch :-)
And can you please run cargo format, then it would be ready to be merged.
Greetings Chris

@jkristell
Copy link
Contributor Author

I'm not really happy about the update_both_planes() name. What do you think about renaming it to update_color_frame()?

Naming is hard :)

 fn update_color_frame(
        &mut self,
        spi: &mut SPI,
        black: &[u8],
        color1: &[u8],
    ) -> Result<(), SPI::Error>;

@jkristell
Copy link
Contributor Author

Trait and method names updated, and code was formatted with cargo fmt.

Copy link
Owner

@caemor caemor left a comment

Choose a reason for hiding this comment

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

How is the src/epd1in54b/graphics.rs working for you?
Would someone use two "display1in54b"s - one for red/color and one for black with the embedded graphics library?
I really like your naming changes! :-)

@jkristell
Copy link
Contributor Author

Graphics works. Yeah exactly like that. Two "displays". I have test code for this that I will clean up and publish as an example in my nucleo-f401re crate.

@jkristell
Copy link
Contributor Author

@caemor
Copy link
Owner

caemor commented Dec 28, 2019

Thanks for your work :-)

@caemor caemor merged commit 636b314 into caemor:master Dec 28, 2019
@jkristell jkristell deleted the rfc-1in54b-support branch December 29, 2019 07:48
@caemor caemor mentioned this pull request Apr 3, 2021
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.

None yet

3 participants