fb: cfb: Support color display #58817
Conversation
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
2 times, most recently
from
05cdc53
to
e30ac83
Compare
soburi
changed the title
soburi
requested review from
jfischer-no,
nashif,
carlescufi,
galak and
MaureenHelm
as code owners
|
dev-review: ping @jfischer-no |
|
@jfischer-no @danieldegrasse will you please take a look? |
There was a problem hiding this comment.
Just some initial comments- but I am still confused about the goal of this rework. First off, the subsystem itself is called Monochrome Character Framebuffer. The subsystem is explicitly:
- monochrome
- intended for font rendering
If we are going to extend CFB to be what I would consider a small graphics framework, I would like to see the following:
- Documentation updates- the current CFB naming is confusing
- Additional tests or samples demonstrating usage of this framework, so we can verify rendering functions work as expected
Additionally, I am slightly confused about where a graphics framework would fit versus LVGL. Is the intention of this framework to fit where LVGL can't?
@jfischer-no ping- I'll defer to you on any of these comments, I just want to make sure that if we are extending CFB we keep documentation and samples up to date.
subsys/fb/cfb.c
Outdated
| desc.pitch = fb->x_res; | ||
| if ((fb->pixel_format == PIXEL_FORMAT_MONO01 || fb->pixel_format == PIXEL_FORMAT_MONO10) && | ||
| (fb->screen_info & SCREEN_INFO_MONO_VTILED)) { | ||
| /* A VTILED format display usually does not support partial region transfers. |
There was a problem hiding this comment.
Why is the code enforcing this restriction? Wouldn't a user know their display, and if it did not support partial region transfers simply call cfb_framebuffer_finalize instead?
include/zephyr/display/cfb.h
Outdated
| * @param a The alpha channel of the foreground color | ||
| * | ||
| */ | ||
| void cfb_framebuffer_set_foreground_color(const struct device *dev, uint8_t r, uint8_t g, uint8_t b, |
There was a problem hiding this comment.
I do not think we can go with this never ending identifiers, so please use common abbreviation and find shorter names, e.g. cfb_set_fg_color(). And there is no reason to have additional framebuffer to cfb.
subsys/fb/Kconfig
Outdated
| @@ -31,6 +31,26 @@ config CHARACTER_FRAMEBUFFER_SHELL_DRIVER_NAME | |||
| help | |||
| Character Framebuffer Display Driver Name | |||
|
|
|||
| config CHARACTER_FRAMEBUFFER_TRANSFER_WITH_PIXEL_FORMAT_CONVERSION | |||
There was a problem hiding this comment.
Please just CFB_TRANSFER_..., for that you will need to change the three option above to CFB_USE_DEFAULT_FONTS, CFB_SHELL, CFB_SHELL_DRIVER_NAME (this one looks outdated, not sure it is still used).
subsys/fb/cfb.c
Outdated
| return api->write(dev, 0, 0, &desc, fb->buf); | ||
| } | ||
|
|
||
| #ifdef CONFIG_CHARACTER_FRAMEBUFFER_TRANSFER_WITH_PIXEL_FORMAT_CONVERSION |
There was a problem hiding this comment.
if(ENABLED(CFB_TRANSFER_WITH_PIXEL_FORMAT_CONVERSION)) {
cfb_transfer_with_pixel_conversion(..);
}I do not understand why this code should not always be built, and why it cannot be broken into smaller chunks. As it is now, it is difficult to understand and review.
|
@soburi do you plan to return to this PR? |
I'm in bit busy state. I'll late to update it. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
from
e30ac83
to
9b78a29
Compare
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
from
9b78a29
to
7b8e69b
Compare
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
from
7b8e69b
to
453afc5
Compare
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
3 times, most recently
from
d0e1ec7
to
de85a26
Compare
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
from
ccf38ce
to
0d08faf
Compare
zephyrbot
added
area: Samples
zephyrbot
requested review from
57300,
aescolar,
jeremybettis,
kartben,
nordicjm and
tejlmand
Change the arguments of `cfb_invert_area`, `cfb_print`, and `cfb_draw_(point|line|rect)`, which used unsigned integers to specify coordinates to signed integers. This is because coordinate calculations are usually based on real numbers, so it is difficult to use unless negative numbers are accepted. We clip drawings outside the drawable area on the implementation side. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
`cfb_get_font_size` and `cfb_get_numof_fonts` take a `const struct driver *` as a first argument, but not use it. Remove the argument and clean up related sources. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
There are three major changes. - Introducing `struct cfb_display` - Changed the argument `struct device *` to `struct cfb_framebuffer *` - Rename APIs starting with `cfb_framebuffer_*` Introduces struct cfb_display as a data structure to represent the display. As a result, it is now possible to create multiple pieces of data managed using a single static variable in previous implementations, making it possible to handle multiple displays. The old `struct cfb_framebuffer` remains but is now part of the `struct cfb_display` data structure. You can get cfb_framebuffer from cfb_display using the added API `cfb_display_get_framebuffer`. All places where a `struct device *` argument was passed to specify the target will be replaced with a `struct cfb_framebuffer *`. In line with this, API names starting with `cfb_framebuffer_*`, which were redundant and inconsistent names, will remove `_framebuffer` and change to `cfb_*` . Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Add `display select` command to select display. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Add a note about the changes of CFB(Character framebuffer) APIs. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
The pixel per tile value can be calculated from pixel_format, so delete the ppt field. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
If a character that does not exist in the font was specified, it was replaced with a space character, but it will now draw with 0 data. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Added support for color displays. Along with color support, we are introducing the concept of foreground and background colors. Drawing fonts and lines is done with the foreground color, and clear operation fills with the background color. With the introduction of drawing colors, we have removed the process of inverting data before transfer if the inverted flag was enabled during finalizing. Replacing the "Invert" operation by swapping the foreground and background colors. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
I have removed the process to set the display format to monochrome. Also, I added board settings conf files for native_sim and native_sim_64 to change the initial value of the SDL display driver's pixel format. CFB operates according to the pixel format set here. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Added a test to check the display on a color display. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Add the `fgcolor` `bgcolor` and `display pixel_format` to support color display Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
CHARACTER_FRAMEBUFFER_SHELL_DRIVER_NAME is not used. Remove it. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
CFB was an abbreviation for Character Framebuffer. However, it currently supports characters and graphic drawing, so it has become far from actual. However, since there are only a few API changes, I don't want to change symbol names, etc. Therefore, we will keep the abbreviation CFB and change the name to `Compact Framebuffer`. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Update README to update to current specs. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
soburi
force-pushed
the
cfb_transfer_to_color_display
branch
from
0d08faf
to
10b6d1c
Compare
Add color display support for CFB.
The concept of "Invert" depended on monochrome displays.
I reimplemented it with foreground color and background color concepts that are newly introduced with color support.
The CFB is no longer a "(Monochrome) Character Frame Buffer" but
the concept and usage of the API remain the same.
Since I would like to maintain the CFB abbreviation, I am considering renaming it "Compact Frame Buffer."
This PR is part of a series of CFB enhancements (third of four #72177).
I would appreciate it if you could check the #67881 part first.