Add support for v2 7.5" EPD #2
Conversation
|
Hello there! Your changes are welcome, but I apologise, I have been working on some pretty major changes in the background. Had I realised this was starting to pick up some traction, I would have made these changes more incrementally and given some warning. Sorry again! Would you mind taking a look at the new interface structure to see if this will work for you? I have split the single |
MorganR
left a comment
MorganR
left a comment
There was a problem hiding this comment.
Just a couple of comments from a quick pass. I really appreciate the effort you put in to document and test the new behaviour! I've found the datasheets to be pretty opaque at times, and I know how much work it takes to fully decipher them.
samples/rp/src/bin/epd7in5_v2/hw.rs
Outdated
| assign_resources::assign_resources! { | ||
| spi_hw: SpiP { | ||
| spi: SPI1, | ||
| clk: PIN_10, | ||
| tx: PIN_11, | ||
| dma_tx: DMA_CH3, | ||
| cs: PIN_9, | ||
| }, | ||
| epd_hw: DisplayP { | ||
| reset: PIN_12, | ||
| dc: PIN_8, | ||
| busy: PIN_13, | ||
| power: PIN_14, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This should be all you need after syncing with main, the rest is now shareable in lib.rs with proper support for different SPI devices.
There was a problem hiding this comment.
Still needed to add some bits for the power pin but this setup does indeed not need separate hardware code for the devices.
There was a problem hiding this comment.
Good point. WDYT of splitting the EpdHw trait, like I did for the old Epd trait? We could have something like SpiHw, PowerHw, etc, that each driver can combine as needed. I have to work out how to make the error type easy to use still.
| // Draw white text over only the black part of the check. | ||
| buffer.clear(BinaryColor::Off).unwrap(); | ||
| text.character_style.text_color = Some(BinaryColor::On); | ||
| text.draw(&mut buffer).unwrap(); | ||
| let buffer_bounds = buffer.bounding_box(); | ||
| let half_width = buffer_bounds.size.width / 2; | ||
| let top_middle = buffer_bounds.top_left + Point::new(half_width as i32, 0); | ||
| let right_half = Rectangle::new(top_middle, Size::new(half_width, buffer_bounds.size.height)); | ||
| // Cover the right-side of the text. | ||
| buffer.fill_solid(&right_half, BinaryColor::Off).unwrap(); | ||
| // Just display white pixels (i.e. same as before plus left side of text). | ||
| expect!( | ||
| epd.display_partial_buffer(&mut spi, &buffer, text.bounding_box()) | ||
| .await, | ||
| "Failed to display white half of text" |
There was a problem hiding this comment.
This test might not make sense if the display doesn't support the "bypass" option that the 2.9" display does. Don't feel beholden to the tests I ran in the other file.
There was a problem hiding this comment.
Well spotted, forgot i left that part in there.
| // Full bytes start at next multiple of 8 from x_start (inclusive if x_start is multiple) | ||
| let x_full_bytes_start = min(((x_start + 7) / 8) * 8, x_end); | ||
| // Full bytes end at last multiple of 8 before x_end (inclusive if x_end is multiple) | ||
| let x_full_bytes_end = max(x_end / 8 * 8, x_start); |
| } | ||
|
|
||
| // On this display the busy pin is active low. | ||
| impl<HW: EpdHw> BusyWait for HW { |
There was a problem hiding this comment.
This display has the busy pin active low. I couldn't figure out a proper way to encode this information in the type system, maybe you have an idea? Since i couldn't find a proper way i decided to copy it and swap high for low.
There was a problem hiding this comment.
I have an idea, I'll get back to you
There was a problem hiding this comment.
What if the EpdHw trait has a busy_when() function returning either PinState::High or PinState::Low? That should keep things fairly tidy and avoid the duplication.
There was a problem hiding this comment.
Or, if I can work out a nice way to split EpdHw into smaller traits, we can have both an BusyWhenLow and BusyWhenHigh trait, and implement this differently for both of them. I think that would be preferable because it means the driver can be clear about what it expects
|
No worries, it wasn't too much effort the incorporate the changes. In the end the code became a bit cleaner cause you only have the implement the features that are actually supported by the hardware. Could you take a look at the new changes? |
MorganR
left a comment
MorganR
left a comment
There was a problem hiding this comment.
I still need to give the main driver file a proper read, but here are a few suggestions in the meantime. Thank you for your patience 🙏
| &mut self, | ||
| spi: &mut SPI, | ||
| buf: &dyn BufferView<BITS, FRAMES>, | ||
| area: Rectangle, |
| // Display text with fast refresh mode | ||
| let mut epd = expect!( | ||
| epd.set_refresh_mode(&mut spi, RefreshMode::Fast).await, | ||
| "Failed to switch to fast refresh m" |
| .unwrap(); | ||
| expect!( | ||
| epd.display_framebuffer(&mut spi, &buffer).await, | ||
| "Failed to display check buffer" |
There was a problem hiding this comment.
Nit: wrong message?
Unwrap is probably fine here. I tried to at least use expect the first time a method is called, but I think most of them can be unwrap after that.
| info!("Displaying text with partial refresh over black buffer"); | ||
| // Clear first. | ||
| buffer | ||
| .fill_solid(&buffer.bounding_box(), BinaryColor::On) |
There was a problem hiding this comment.
You can use .clear(BinaryColor::On) when you want to fill the whole buffer.
| "Failed to initialize EPD" | ||
| ); | ||
|
|
||
| for i in 0..5 { |
There was a problem hiding this comment.
It might be nice to have another info! before this section to explain what is being tested
| info!("Sleeping EPD"); | ||
| let epd = expect!(epd.sleep(&mut spi).await, "Failed to put EPD to sleep"); | ||
| Timer::after_secs(2).await; | ||
|
|
||
| info!("Waking EPD"); | ||
| let epd = expect!(epd.wake(&mut spi).await, "Failed to wake EPD"); | ||
| Timer::after_secs(1).await; |
There was a problem hiding this comment.
This might not be needed, since you already tested sleeping above
| } | ||
|
|
||
| // On this display the busy pin is active low. | ||
| impl<HW: EpdHw> BusyWait for HW { |
There was a problem hiding this comment.
What if the EpdHw trait has a busy_when() function returning either PinState::High or PinState::Low? That should keep things fairly tidy and avoid the duplication.
|
@protowouter would merging this PR make it easier to handle the busy pin state, and make it easier to add the |
2 participants
Hi,
I decided to add support for the v2 7.5" EPD, since I'm working on a project with that screen.
My goal was to stay as close to the API you had established for the 2.9" screen. Since this model has a separate power input I had to change things around a bit.
Please let me know if you are interested in incorporating these changes, I would be happy to make any changes if required.
Kind regards,
Wouter