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

Update to work with latest e-g and tinybmp #196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bemyak
Copy link

@bemyak bemyak commented Mar 11, 2024

I had some issues with rendering simple black & white images with tinybmp crate, and had to apply these patches to make it work. Let me know if it makes sense.

@bemyak bemyak mentioned this pull request Mar 12, 2024
@avlec
Copy link

avlec commented Apr 1, 2024

I've had some success on the 3.7in (which doesn't have the best support) without any modifications but yes the color is inverted so I worked around that by just inverting the BMP.

Just wondering if this change makes working with these slightly easier (no color_converted() in the draw call?)

use embedded_graphics::pixelcolor::BinaryColor;
use embedded_graphics::{prelude::*, image::Image};

// ...
    let mut display = Display3in7::default();
    const SPLASH: &[u8] = include_bytes!("../splashnewbw.bmp");
    let splash_bmp = tinybmp::Bmp::<BinaryColor>::from_slice(SPLASH).unwrap();
    Image::new(&splash_bmp, Point::zero()).draw(&mut display.color_converted()).unwrap_or_else(|_| error!("failed to render splash screen"));

@diajowe
Copy link

diajowe commented May 16, 2024

Managed to draw some text with your PR for a epd7in5_v2::Display7in5, this currently doesn't work in master or version 0.5.0 as seen in issue #194

@diajowe
Copy link

diajowe commented May 23, 2024

I see this PR is a draft? Is there something missing for completion?

@bemyak
Copy link
Author

bemyak commented May 23, 2024

Yeah, there were some arguable changes related to color inversion, so I was waiting to get any feedback.

I've dropped them for now, and users are encouraged to use display.color_converted() as mentioned by @avlec.

I'll mark this as ready for review now.

@bemyak bemyak marked this pull request as ready for review May 23, 2024 17:38
@auto-assign auto-assign bot requested a review from caemor May 23, 2024 17:38
@diajowe
Copy link

diajowe commented May 23, 2024

Yeah, there were some arguable changes related to color inversion, so I was waiting to get any feedback.

I've dropped them for now, and users are encouraged to use display.color_converted() as mentioned by @avlec.

I'll mark this as ready for review now.

Would you mind going a bit more into detail on what could go wrong?
Maybe I could test a bit further, as currently, I only displayed some data using 7.5inch BW version of waveshare EPD

@bemyak
Copy link
Author

bemyak commented May 23, 2024

Would you mind going a bit more into detail on what could go wrong?

BinaryColor is supposed to represent something like a LED pixel, so BinaryColor::On means the LED is emitting light, thus the value should (in my opinion) correspond to Color::White on an EPD.

However, in the current implementation, it's the opposite:

epd-waveshare/src/color.rs

Lines 297 to 304 in 1244f03

impl From<BinaryColor> for Color {
fn from(b: BinaryColor) -> Color {
match b {
BinaryColor::On => Color::Black,
BinaryColor::Off => Color::White,
}
}
}

I guess it's because in most cases when rendering text, people expect to see white on black for LED panels and black on white for EPD. Displaying images becomes tricky now since you need to invert colours to display them properly.

Inverting all the colours to be "On=White, Off=Black" is a breaking change and it must be discussed thoroughly.

tinybmp and other crates require Color to implement all Rgb transformations
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