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

Enable drawing in three colors for epd2in13 #76

Merged
merged 4 commits into from
Jun 15, 2021
Merged

Conversation

lrbalt
Copy link
Contributor

@lrbalt lrbalt commented May 30, 2021

Move from BinaryColor to TriColor: use one Display for drawing

  • I use one buffer containing data for b/w and chromatic
  • added two fn's to get slice for b/w buffer and slice for chromatic buffer for updating both buffers on the display
  • you can still use just the b/w buffer if you want to use two colors
  • updated the example to draw b/w and draw using three colors

Move from BinaryColor to TriColor:  use one Display fo drawing
@auto-assign auto-assign bot requested a review from caemor May 30, 2021 12:24
@lrbalt
Copy link
Contributor Author

lrbalt commented May 30, 2021

I'd like to get your feedback @caemor before I update documentation

I'm wondering if you like to add a fn to WaveshareThreeColorDisplay which takes a buffer containing both b/w and chromatic data, i.e. fn update_bw_and_chromatic_frame(&mut self, spi: &mut SPI, bw_and_chromatic_buffer: &[u8]

and remove the individual update fn's for bw and chromatic from this trait. Or change the update_color_frrame to take one buffer. But that means that all tri-color screens need to use one big buffer for both b/w and chromatic. I.e. this exposes the internal data model in the api.

@lrbalt
Copy link
Contributor Author

lrbalt commented May 30, 2021

I've created a small application using both this patch and an emulator with one draw function. This way I can develop using emulator, knowing that it will work on the e-paper display too: https://github.com/lrbalt/epd-bootscreen

I needed to mock TriColor for using the emulator on my Mac (doesn't compile linux-embedded-hal), since it was defined in epd-waveshare, not embedded-graphics.

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 wondering if you like to add a fn to WaveshareThreeColorDisplay which takes a buffer containing both b/w and chromatic data, i.e. fn update_bw_and_chromatic_frame(&mut self, spi: &mut SPI, bw_and_chromatic_buffer: &[u8]

and remove the individual update fn's for bw and chromatic from this trait. Or change the update_color_frrame to take one buffer. But that means that all tri-color screens need to use one big buffer for both b/w and chromatic. I.e. this exposes the internal data model in the api.

I don't think we should remove the option to just use a smaller buffer and update it in between sending b/w and chromatic. So allowing both options should be the best way to go forwards for now.

@lrbalt
Copy link
Contributor Author

lrbalt commented Jun 9, 2021

I'm wondering if you like to add a fn to WaveshareThreeColorDisplay which takes a buffer containing both b/w and chromatic data, i.e. fn update_bw_and_chromatic_frame(&mut self, spi: &mut SPI, bw_and_chromatic_buffer: &[u8]
and remove the individual update fn's for bw and chromatic from this trait. Or change the update_color_frrame to take one buffer. But that means that all tri-color screens need to use one big buffer for both b/w and chromatic. I.e. this exposes the internal data model in the api.

I don't think we should remove the option to just use a smaller buffer and update it in between sending b/w and chromatic. So allowing both options should be the best way to go forwards for now.

Ok, I'll add one new fn for updating both b/w and chromatic using the one buffer

@lrbalt
Copy link
Contributor Author

lrbalt commented Jun 9, 2021

I've updated the example in mod.rs so all tests now pass. I did not add the fn mainly because it would look like:

fn update_bw_and_chromatic_frame(&mut self, spi: &mut SPI, bw_and_chromatic_buffer: &[u8]) -> Result<(), SPI::Error>;

This would mean that the trait WaveshareThreeColorDisplay would need to have knowledge of Display2in13bc to split the buffer. And we thus expose the inner data type to a trait. It doesn't feel right. It also led to conflicts on other displays that use some magic in the buffer for the chromatic part (like here) so it would not really be generic.

@caemor
Copy link
Owner

caemor commented Jun 14, 2021

Do you think this is ready or do you still want to add something here?

@lrbalt
Copy link
Contributor Author

lrbalt commented Jun 15, 2021

I think it is ready to merge.

@caemor caemor merged commit 253cc10 into caemor:main Jun 15, 2021
@lrbalt
Copy link
Contributor Author

lrbalt commented Jun 15, 2021

cool, thanks!

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

2 participants