Skip to content

Add epd7in5_v2#36

Merged
caemor merged 6 commits intocaemor:masterfrom
asaaki:epd7in5_v2
Feb 14, 2020
Merged

Add epd7in5_v2#36
caemor merged 6 commits intocaemor:masterfrom
asaaki:epd7in5_v2

Conversation

@asaaki
Copy link
Contributor

@asaaki asaaki commented Jan 11, 2020

When I ordered such 7.5" display and tried to use this library I was confused, as nothing worked. After some time I realized the very red note and thought »Oh, I must be a lucky one« since nobody besides Waveshare has support for V2 yet.

After spending hours in the python code and the spec PDF I came up with the following code, which works fine for my display. Not sure if anyone is willing to spend the money to confirm it. 😅

I also got rid of the deprecation warnings from embedded-hal, shouting at me to not use digital::v1 anymore. My workaround might look hacky, but should not be really worse than the logic before, though the let _ = … might be a bit unpleasing, as well as the .unwrap_or(false) — yes, I make probably a stupid assumption here to unlock the busy state on error, we could also negate and potentially lock forever; dunno what's best. Given that we ignored potentially failing state changes, I think it should be mostly fine.


Commit asaaki/epd-waveshare@19e3d51 was an attempt to fix the digital::v2 fallability, but I discovered that it would at least not work with linux-embedded-hal due to error type mismatches. And the examples step of CI is also pretty unhappy about it.

@codecov-io
Copy link

codecov-io commented Jan 11, 2020

Codecov Report

Merging #36 into master will increase coverage by 1.28%.
The diff coverage is 43.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   35.74%   37.03%   +1.28%     
==========================================
  Files          18       21       +3     
  Lines         996     1145     +149     
==========================================
+ Hits          356      424      +68     
- Misses        640      721      +81
Impacted Files Coverage Δ
src/epd1in54b/mod.rs 3.1% <ø> (ø) ⬆️
src/epd2in9/mod.rs 3.41% <ø> (ø) ⬆️
src/traits.rs 0% <ø> (ø) ⬆️
src/epd7in5/mod.rs 4.59% <ø> (ø) ⬆️
src/epd4in2/mod.rs 2.83% <ø> (ø) ⬆️
src/epd1in54/mod.rs 3.3% <ø> (ø) ⬆️
src/interface.rs 0% <0%> (ø) ⬆️
src/epd7in5_v2/graphics.rs 100% <100%> (ø)
src/epd7in5_v2/command.rs 100% <100%> (ø)
src/epd7in5_v2/mod.rs 4.76% <4.76%> (ø)
... and 3 more

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 636b314...d0e1d55. Read the comment docs.

@caemor
Copy link
Owner

caemor commented Jan 14, 2020

Commit asaaki/epd-waveshare@19e3d51 was an attempt to fix the digital::v2 fallability, but I discovered that it would at least not work with linux-embedded-hal due to error type mismatches. And the examples step of CI is also pretty unhappy about it.

In my first attempt to update to digital::v2 the result also wasn't what I wanted so I am currently waiting for the digital::v3/other results of the discussion (rust-embedded/wg#393)

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.

I'm a little bit unsure about calling this display Display7in5/EPD7in5 even though they are in the separate v2 module. But since every new newly bought 7in5 Display would be one of v2 it should be the better decision 👍

  • Would it make sense to add your update_and_display_frame to the main trait?

@caemor
Copy link
Owner

caemor commented Jan 14, 2020

Also thanks for your work :-) 👍

@asaaki
Copy link
Contributor Author

asaaki commented Jan 14, 2020

Thanks for the pointer. Seems to be a very messy situation. Will undo all digital::* changes then.

@asaaki
Copy link
Contributor Author

asaaki commented Jan 14, 2020

Would it make sense to add your update_and_display_frame to the main trait?

Could make sense, waveshare's python reference implementation uses a single display function for non-v2 as well, so providing a consolidated function in the main trait could be beneficial for everyone.
Will give it a try.

@caemor
Copy link
Owner

caemor commented Jan 14, 2020

Will undo all digital::* changes then

I think we can leave your current v2 impl/changes and just keep in mind that we need to improve this situation later on (with v3) for which I made a new issue earlier.

@caemor
Copy link
Owner

caemor commented Feb 14, 2020

I am gonna merge this for now and make the change to the main trait in a seperate pr

@caemor caemor merged commit 8829f9a into caemor:master Feb 14, 2020
@asaaki
Copy link
Contributor Author

asaaki commented Feb 20, 2020

Sorry, was occupied with other stuff. Thanks for taking it in. 🙇🏻‍♂️

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.

3 participants