almindor / mipidsi

MIPI Display Serial Interface unified driver
MIT License
117 stars 46 forks source link

Limit `clear` to visible part of the framebuffer #46

Closed rfuest closed 7 months ago

rfuest commented 1 year ago

At the moment the DrawTarget::clear implementation clears the entire framebuffer, even for smaller display. The unnecessary writes makes clearing the display slower than necessary on these displays. I would suggest to change the implementation of clear or simply to remove it and rely on the default implementation, that uses fill_solid.

I'm not sure if it is ever necessary to clear the entire framebuffer, but if that is the case we could add another method clear_all to clear the entire framebuffer.

almindor commented 1 year ago

So this is conflicting. I think we should change it to fn clear(entire_framebuffer: bool) since clearing the whole thing makes sense for HW scroll displays for example.

rfuest commented 1 year ago

I think we should change it to fn clear(entire_framebuffer: bool)

That wouldn't work for the DrawTarget implementation.

rfuest commented 1 year ago

A solution, that would be compatible with the embedded-graphics drawing model, is using a second DrawTarget for the entire framebuffer:

let mut display = Builder::st7789_pico1(di).init(&mut delay, Some(rst))?;

// clear only the visible area
display.clear(Rgb565::BLACK)?;
// display is DrawTarget with 135x240 pixel

// clear the entire framebuffer
display.framebuffer().clear(Rgb565::BLACK)?
// display.framebuffer() returns a DrawTarget with 240x320 pixel
// display.framebuffer() could also be used to draw anything else outside the visible area
almindor commented 1 year ago

That'd work and should probably be good enough for the hw scroll edge cases. The change will need to be documented as it's breaking expectations.

almindor commented 8 months ago

@rfuest how do you feel about this issue? I'm trying to collect things that need to be done before the next release. I'm on the fence on this one.

rfuest commented 8 months ago

I accidentally implemented this change in https://github.com/almindor/mipidsi/commit/5f455eb8171153d4ccb9080905782ee17ae0439f, otherwise the display wasn't cleared correctly. I would prefer to leave it this way because it can improve performance in the most common use case, when scrolling isn't used.

almindor commented 7 months ago

Closing as done