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

adds Rgb444 and RawU12 #732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

catherinejones
Copy link

@catherinejones catherinejones commented Aug 19, 2023

This PR adds Rgb444 and RawU12 to support 12bit RGB on displays like the ST7789.

This fixes 1/2 of #726 (missing BGR444).

@catherinejones
Copy link
Author

(Hi! This is my first PR to embedded-graphics so my apologies if I've missed anything!)

@BroderickCarlin
Copy link
Contributor

It appears CI is failing on this PR just as a result of failing to fetch https://img.shields.io/matrix/rust-embedded-graphics:matrix.org. This is most likely an intermittent error and rerunning CI should allow it to pass

@rfuest
Copy link
Member

rfuest commented Nov 7, 2023

The Rgb444 type looks good, but the RawU12 storage type will cause issues without additional code changes. There is no support for using RawU12 values for ImageRaws or Framebuffers. At the moment I would suggest to use RawU16 as a storage type of Rgb444, which will allow Rgb444 to used for images and FBs, but with 4 wasted bits per pixel. When/if PR #711 gets merged the RawU12 type could be revisited to store 2 Rgb444 pixels per 3 bytes, without any wasted bits. This packing is also used by some display controllers and would be a good long term addition IMO.

@catherinejones
Copy link
Author

@rfuest I'll take a look at adding support to ImageRaw and Framebuffer! I've been using these types locally for a ST7789 display and it's been working relatively well thus far.

@BroderickCarlin I've merged the latest changes into my branch which seemed to prompt CI to run again.

Sorry about the late reply, I guess I don't have email notifications enabled 😅

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

3 participants