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 direct pixel writes from u8 buffers #143

Merged
merged 8 commits into from
Oct 26, 2024
Merged

Add direct pixel writes from u8 buffers #143

merged 8 commits into from
Oct 26, 2024

Conversation

almindor
Copy link
Owner

Related to #142 this change will allow direct writes of &[u8] slice data via the display-interface without needing to do pixel format conversion in cases where the user knows it is safe to do so (e.g. prepared image data).

The hope is that this can get paired up with a change in embedded-graphics that enables such operations on a higher level (e.g. draw_image)

@rfuest
Copy link
Collaborator

rfuest commented Oct 23, 2024

I think this is a valid workaround until we have something better available in the future. We just need to make sure to document possible issues a user could run into:

  • The user is responsible for getting the endianness right
  • We should document that the ranges from sx to ex and from sy and ey are inclusive (this also applies to the existing set_pixels)
  • Adding bounds checking for the coordinates wouldn't be a bad idea. IIRC there has been some confusion in the past where a user tried to use display.set_pixels(0, 0, 320, 240, colors) instead of the correct display.set_pixels(0, 0, 319, 239, colors). Without an error this problem isn't easy to catch.
  • The method won't work with a 16bit display-interface-gpio, because it pads the each byte to a u16 instead of converting each two byte chunk into a u16. (https://github.com/therealprof/display-interface/blob/8fca041b0288740678f16c1d05cce21bd3867ee5/parallel-gpio/src/lib.rs#L267)

@almindor
Copy link
Owner Author

I think this is a valid workaround until we have something better available in the future. We just need to make sure to document possible issues a user could run into:

  • The user is responsible for getting the endianness right
  • We should document that the ranges from sx to ex and from sy and ey are inclusive (this also applies to the existing set_pixels)
  • Adding bounds checking for the coordinates wouldn't be a bad idea. IIRC there has been some confusion in the past where a user tried to use display.set_pixels(0, 0, 320, 240, colors) instead of the correct display.set_pixels(0, 0, 319, 239, colors). Without an error this problem isn't easy to catch.
  • The method won't work with a 16bit display-interface-gpio, because it pads the each byte to a u16 instead of converting each two byte chunk into a u16. (https://github.com/therealprof/display-interface/blob/8fca041b0288740678f16c1d05cce21bd3867ee5/parallel-gpio/src/lib.rs#L267)

Sounds good, I'll add the documentation points. Do you plan to add the draw_image to embedded graphics? I'm wondering if I should wait for that so it's linked properly.

@rfuest
Copy link
Collaborator

rfuest commented Oct 23, 2024

Do you plan to add the draw_image to embedded graphics? I'm wondering if I should wait for that so it's linked properly.

I'm not sure when that will be added or how exactly it will be implemented. I would suggest to leave that part out and open an issue to remind us to update the comment after this has been added to e-g.

@almindor almindor marked this pull request as ready for review October 24, 2024 17:00
@almindor almindor requested a review from rfuest October 24, 2024 17:00
Copy link
Collaborator

@rfuest rfuest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more doc comment bike shedding.

mipidsi/src/lib.rs Outdated Show resolved Hide resolved
mipidsi/src/lib.rs Outdated Show resolved Hide resolved
mipidsi/src/lib.rs Outdated Show resolved Hide resolved
mipidsi/src/models.rs Outdated Show resolved Hide resolved
@almindor almindor requested a review from rfuest October 25, 2024 17:58
Copy link
Collaborator

@rfuest rfuest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@almindor almindor merged commit 84cfd9b into master Oct 26, 2024
15 checks passed
@almindor almindor deleted the write_pixels_raw branch October 26, 2024 15:41
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.

2 participants