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 EPD 3in7 #129

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Add support for EPD 3in7 #129

merged 1 commit into from
Dec 8, 2022

Conversation

mangelajo
Copy link
Contributor

Matches implementation from waveshare/e-Paper code

https://github.com/waveshare/e-Paper/blob/master/RaspberryPi_JetsonNano/c/lib/e-Paper/EPD_3in7.c

Co-Authored-By: Miguel Angel Ajo [email protected]

@auto-assign auto-assign bot requested a review from caemor November 30, 2022 17:17
@mangelajo mangelajo force-pushed the epd-3in7 branch 2 times, most recently from 5074964 to ec223ba Compare November 30, 2022 17:27
@mangelajo
Copy link
Contributor Author

ok, it's ignoring #![allow(non_camel_case_types)]

I guess I will have to change the enum names.

src/epd3in7/command.rs Outdated Show resolved Hide resolved
@mangelajo
Copy link
Contributor Author

@caemor is there any driver doing grayscale or shall I define a new color type?

@mangelajo
Copy link
Contributor Author

@peckpeck
Copy link
Contributor

I'd suggest to make the Color a generic parameter and vary it in impl, this way you could factor some of he code

@mangelajo
Copy link
Contributor Author

I'd suggest to make the Color a generic parameter and vary it in impl, this way you could factor some of he code

Yes, I was thinking about that, but there are a couple of things holding me back:

  • The code paths and initialization for both modes (see Gray1/Gray4 functions here [1]) are wildly different, they only seem to share SPI bus access.
  • It would special-case this driver in terms of instantiation, I guess we could may be have a default instance in one of the modes -not sure if it's possible, new to rust here-, for other drivers (tri-color vs bw, the drivers are duplicated)

[1] https://github.com/waveshare/e-Paper/blob/master/RaspberryPi_JetsonNano/c/lib/e-Paper/EPD_3in7.c#L171

@mangelajo
Copy link
Contributor Author

mangelajo commented Nov 30, 2022

Oh, I will go and implement partial updates for this driver in a separate PR first, while I keep rumiating about the Grayscales ;)

@peckpeck
Copy link
Contributor

If you use a generic type for color, you can easily separate the code for init with an impl for Color and and impl for Gray4. But update_frame will still be difficult, maybe smaller trait would help implementing this kind of cases.

@mangelajo mangelajo requested a review from caemor December 1, 2022 08:43
@peckpeck
Copy link
Contributor

peckpeck commented Dec 1, 2022

If there is no real improvement besides memory consumption, why have a separate BW initialization ? A BW update_frame on a gray initialized device should work no ?

@mangelajo
Copy link
Contributor Author

If there is no real improvement besides memory consumption, why have a separate BW initialization ? A BW update_frame on a gray initialized device should work no ?

I really have no numbers to compare yet. I will eventually be working on Gray4 support on a different PR. At that point we can analyze it from both the timing and memory perspective and decide.

But it sounds like a good point, if they were to have the same refresh rates, why keep two code paths :)

@mangelajo
Copy link
Contributor Author

Note: I may need some time to get into Gray4, currently I am working on the application that uses the implementation of this PR.

Hopefully we can merge this if everything looks good, and then treat Gray4 support as an upgrade PR.

@peckpeck
Copy link
Contributor

peckpeck commented Dec 2, 2022

Indeed, since we in 0.x releases we should merge things quickly to allow for easier testing.

src/epd3in7/mod.rs Outdated Show resolved Hide resolved
src/epd3in7/mod.rs Outdated Show resolved Hide resolved
src/epd3in7/mod.rs Outdated Show resolved Hide resolved
@mangelajo
Copy link
Contributor Author

I am not sure what's going on, I see the errors here:

https://github.com/caemor/epd-waveshare/actions/runs/3608595550/jobs/6081293541#step:5:105

But with my architecture, it builds all just fine.

Any advice?

src/epd3in7/mod.rs Outdated Show resolved Hide resolved
@peckpeck
Copy link
Contributor

peckpeck commented Dec 7, 2022

I am not sure what's going on, I see the errors here:

You need to rebase your code, the main branch has been updated with new commits that impact your code.
Something like git rebase -i origin/main

@mangelajo
Copy link
Contributor Author

I am not sure what's going on, I see the errors here:

You need to rebase your code, the main branch has been updated with new commits that impact your code. Something like git rebase -i origin/main

Ack, I will rebase and let's see how it goes, I would have expected the unit testing/etc to pass even if it needed rebase, of course github may complain if it's unable to merge your changes.

src/epd3in7/mod.rs Outdated Show resolved Hide resolved
@caemor caemor mentioned this pull request Dec 7, 2022
5 tasks
@mangelajo
Copy link
Contributor Author

Ok, I rebased and squashed the commits, now at least I get it reproduced locally:

error[E0407]: method `is_busy` is not a member of trait `WaveshareDisplay`
   --> src/epd3in7/mod.rs:260:5
    |
260 | /     fn is_busy(&self) -> bool {
261 | |         self.interface.is_busy(IS_BUSY_LOW)
262 | |     }
    | |_____^ not a member of trait `WaveshareDisplay`

error[E0277]: the trait bound `DELAY: _embedded_hal_blocking_delay_DelayUs<u32>` is not satisfied
  --> src/epd3in7/mod.rs:50:37
   |
50 | impl<SPI, CS, BUSY, DC, RST, DELAY> InternalWiAdditions<SPI, CS, BUSY, DC, RST, DELAY>
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `_embedded_hal_blocking_delay_DelayUs<u32>` is not implemented for `DELAY`
   |
note: required by a bound in `traits::InternalWiAdditions`
  --> src/traits.rs:36:12
   |
29 | pub(crate) trait InternalWiAdditions<SPI, CS, BUSY, DC, RST, DELAY>
   |                  ------------------- required by a bound in this
...
36 |     DELAY: DelayUs<u32>,
   |            ^^^^^^^^^^^^ required by this bound in `traits::InternalWiAdditions`
help: consider further restricting this bound
   |
58 |     DELAY: DelayMs<u8> + embedded_hal::prelude::_embedded_hal_blocking_delay_DelayUs<u32>,
   |                        ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `DELAY: _embedded_hal_blocking_delay_DelayUs<u32>` is not satisfied
   --> src/epd3in7/mod.rs:125:37
    |
125 | impl<SPI, CS, BUSY, DC, RST, DELAY> WaveshareDisplay<SPI, CS, BUSY, DC, RST, DELAY>
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `_embedded_hal_blocking_delay_DelayUs<u32>` is not implemented for `DELAY`
    |
note: required by a bound in `traits::WaveshareDisplay`
   --> src/traits.rs:146:12
    |
139 | pub trait WaveshareDisplay<SPI, CS, BUSY, DC, RST, DELAY>
    |           ---------------- required by a bound in this
...
146 |     DELAY: DelayUs<u32>,
    |            ^^^^^^^^^^^^ required by this bound in `traits::WaveshareDisplay`
help: consider further restricting this bound
    |
133 |     DELAY: DelayMs<u8> + embedded_hal::prelude::_embedded_hal_blocking_delay_DelayUs<u32>,
    |                        ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

I will try to have it fixed by tomorrow.

@caemor
Copy link
Owner

caemor commented Dec 7, 2022

If it helps:
#125 and #131 should be the issues:

  • Replace DelayMs with DelayUs
  • And replace is_busy with wait_until_idle in the Waveshare Trait Implementation: image

@mangelajo
Copy link
Contributor Author

If it helps: #125 and #131 should be the issues:

* Replace DelayMs with DelayUs

* And replace is_busy with wait_until_idle in the Waveshare Trait Implementation: ![image](https://user-images.githubusercontent.com/11088935/206298426-4e76ed07-19e5-4985-9b30-0d5f7b127a1e.png)

Thanks a lot, it helps, yes! #59

Matches implementation from waveshare/e-Paper code

https://github.com/waveshare/e-Paper/blob/master/RaspberryPi_JetsonNano/c/lib/e-Paper/EPD_3in7.c

Co-Authored-By: Miguel Angel Ajo <[email protected]>
Co-Authored-By: Christoph Gross <[email protected]>
@mangelajo
Copy link
Contributor Author

hopefully compile now after rebase :-)

@mangelajo mangelajo requested review from peckpeck and caemor and removed request for peckpeck December 8, 2022 15:37
@caemor
Copy link
Owner

caemor commented Dec 8, 2022

Thanks for your great work!

@caemor caemor merged commit f98c121 into caemor:main Dec 8, 2022
@mangelajo
Copy link
Contributor Author

Thanks for your great work!

Thank you!! 🙏🏻

@caemor caemor mentioned this pull request Dec 4, 2024
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.

4 participants