-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Display api #7833
Display api #7833
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7833 +/- ##
=======================================
Coverage 53.24% 53.24%
=======================================
Files 209 209
Lines 25630 25630
Branches 5537 5537
=======================================
Hits 13647 13647
Misses 9787 9787
Partials 2196 2196 Continue to review full report at Codecov.
|
include/display.h
Outdated
*/ | ||
inline int display_write_bitmap(const struct device *dev, const u16_t x, | ||
const u16_t y, const u16_t w, const u16_t h, | ||
const u8_t *data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have another parameter, specifying the pixel format. Right now, the format is "whatever the display expects". Given the wide variety of displays and pixel formats, this is important.
At the very least, it would be useful to have a function to query which pixel format this particular driver/device supports. This would let the application adapt itself prior to calling write_bitmap, which might be even better.
Some display controllers will accept 16-bit RGB values (e.g. RGB565), some will be 1bpp only, some will accept colors in weird order (e.g. BGR565), some will accept palette indexed colors and work with 8bpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lpereira Added an extra parameter to pass the pixel format and one extra function to request if a specific format is supported by the display.
8428706
to
61ebe92
Compare
61ebe92
to
15e4b91
Compare
15e4b91
to
1993dc5
Compare
include/display.h
Outdated
* @brief Callback API to check if pixel format is supported by display | ||
* See display_is_pixel_format_supported() for argument description | ||
*/ | ||
typedef bool (*display_is_pixel_format_supported_api)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from #9149
@pizi-nordic
IMHO a bit mask might be better to list supported pixel formats. Then a "pixel format set" function should be available, as some devices are able to change pixel format. On the other hand, we might want a display_write() accepting multiple data formats, however I think that might be tricky for planar framebuffers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, we might want a display_write() accepting multiple data formats, however I think that might be tricky for planar framebuffers.
I see no reason to complicate the display driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. Then a "pixel format set" function should be available, as some devices are able to change pixel format. ...
I start wondering if this should not be all configured at compile time instead of adding code for dynamically switching at run time.
include/display.h
Outdated
* @brief Callback API for writing bitmap | ||
* See display_write_bitmap() for argument description | ||
*/ | ||
typedef int (*display_write_bitmap_api)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from #9149
@pizi-nordic
Have you considered a bit more flexible approach based on bit blitting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the intention of this particular api is only to write a flat bitmap, in case any bit blitting is needed it is up to the higher layer software to support this.
An additional api could be add in case HW support of some kind is available to perform the bit blitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that a API for read operation might be useful?
Double buffering is the best option, however on memory constrained systems there might be not enough RAM. I think that such systems might be forced to sacrifice speed and read data from display in order to perform some operations.
There could be also systems, which have memory-mapped framebuffer. In such system reading is cheap and direct access is possible. I think that mmap()-like API (returning fixed pointer) might be the fastest solution in such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read API could be useful, I will add one but cant test it on the my current LCD as it does not allow read operations.
I could add a function to get a pointer to a memory-mapped framebuffer but is there any MCU that supports this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EFM32 Giant Gecko Family has so called TFT Direct Drive:
The EBI contains a TFT controller which can drive a TFT via a 565 RGB interface. The TFT controller supports programmable display
and port sizes and offers accurate control of frequency and setup and hold timing. Direct Drive is supported for TFT displays which do
not have their own frame buffer. In that case TFT Direct Drive can transfer data from either on-chip memory or from an external memo-
ry device to the TFT at low CPU load. Automatic alpha-blending and masking is also supported for transfers through the EBI interface
Details: https://www.silabs.com/documents/public/data-sheets/efm32gg-datasheet.pdf
* @brief Display driver API | ||
* API which a display driver should expose | ||
*/ | ||
struct display_driver_api { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from #9149
@pizi-nordic
Have you considered putting here API for backlight control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet but I will add a proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function added to set brightness
drivers/display/display_ili9340.c
Outdated
{ | ||
struct ili9340_data *data = (struct ili9340_data *)dev->driver_data; | ||
|
||
if (pixel_format != PIXEL_FORMAT_RGB_888) { | ||
SYS_LOG_ERR("Pixel format not supported by ILI9340"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from #9149
@pizi-nordic
Could you please use new logger infrastructure?
include/display.h
Outdated
*/ | ||
typedef int (*display_write_bitmap_api)( | ||
const struct device *dev, const u16_t x, const u16_t y, const u16_t w, | ||
const u16_t h, const enum display_pixel_format pixel_format, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does display_pixel_format have to be handed over here? What should the display driver do with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added on request (@lpereira):
It would be useful to have another parameter, specifying the pixel format. Right now, the format is "whatever the display expects". Given the wide variety of displays and pixel formats, this is important.
But how more I think about it is maybe better to drop the support form the write function and leave the conversion up to a higher layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes sense. In fact, the suggestion to have the pixel format be a compile-time setting is the best option here -- the upper layer would perform the conversion if necessary, and the API would be easier.
Drivers can expose, as Kconfig options, the display formats it supports; maybe if a device supports more than one, you could have an API to select which one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed pixel format from write and added API function to set pixel format at run time (if supported)
include/display.h
Outdated
* @brief Callback API to check if pixel format is supported by display | ||
* See display_is_pixel_format_supported() for argument description | ||
*/ | ||
typedef bool (*display_is_pixel_format_supported_api)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, we might want a display_write() accepting multiple data formats, however I think that might be tricky for planar framebuffers.
I see no reason to complicate the display driver.
* @brief Display driver API | ||
* API which a display driver should expose | ||
*/ | ||
struct display_driver_api { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It lacks basic functions to query display resolution (or capabilities) and set contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a function to set the contrast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function to set contrast and query capabilities added to API
1993dc5
to
71fc84e
Compare
include/display.h
Outdated
* @brief Callback API for writing bitmap | ||
* See display_write_bitmap() for argument description | ||
*/ | ||
typedef int (*display_write_bitmap_api)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that a API for read operation might be useful?
Double buffering is the best option, however on memory constrained systems there might be not enough RAM. I think that such systems might be forced to sacrifice speed and read data from display in order to perform some operations.
There could be also systems, which have memory-mapped framebuffer. In such system reading is cheap and direct access is possible. I think that mmap()-like API (returning fixed pointer) might be the fastest solution in such case.
include/display.h
Outdated
* See display_write_bitmap() for argument description | ||
*/ | ||
typedef int (*display_write_bitmap_api)( | ||
const struct device *dev, const u16_t x, const u16_t y, const u16_t w, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered support for images with stride?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the interface was designed with embedded LCD displays in mind. Most of the time these devices require a window where they operate on and will write pixels consecutively in to this window. So if a stride is used the window needs to repositioned for every stride that takes place.
An option would be to extend the interface to take a pointer to an array of bitmaps:
struct bitmap {
void* data
u16_t x;
u16_t y;
u16_t w;
u16_t h;
void *data;
};
typedef int (*display_write_bitmap_api)(
const struct device *dev, cosnt struct bitmap *bitmaps, u32_t nbr_of_bitmaps);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about other side - the image in memory. Having support for stride would reduce memory requirements of resizing operations, as they could be performed in place. From API point of view, you need only u16_t p
or u16_t s
(pitch or stride) parameter after width and height. Then you are defining window as usual, then you are writing line after line adding pitch/stride instead of width while advancing to the new line. For pitch/stride equal to width, you can directly write whole buffer in one step.
include/display.h
Outdated
struct display_capabilities { | ||
u16_t x_resolution; | ||
u16_t y_resolution; | ||
enum display_pixel_format supported_pixel_formats; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is mask, then u32_t will be better than enum, which is intended to hold only for single value from set.
134ce5e
to
9f80471
Compare
@vanwinkeljan I have tried the API in #9035, please take a look at it. I will write a few more comments here later. |
include/display.h
Outdated
* | ||
* @retval 0 on success else negative errno code. | ||
*/ | ||
inline int display_write_bitmap(const struct device *dev, const u16_t x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree with that. Generally, not every controller allows writing any size of data to any address. As generous as the API is currently it will make the drivers too complex. I would remove w, h and p (???) and define the region by x, y, the size of the buffer in octets and current_pixel_format. Ditto for the display_read_bitmap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to X,Y only would not be an option as this would require to update the remainder of the line and this will significantly slow down the driver and increase the memory buffer requirements. For example with a 320x240 LCD writing a data of 10(w)x10(h) pixels on coordinates 0 (x),0(y) would require 3200 pixels instead of 100 pixels.
Most LCDs with integrated framebuffer for embedded applications support windowing and you can tell them to only to write to this window by setting x, y, w and h.
On the other hand some (monochrome) devices pack multiple pixels in a single byte either in horizontal or vertical direction and such devices can not accept freely all possible inputs.
Would it be acceptable to add 2 extra parameters (minW, minH) to the display capabilities structure and the caller of the API need to comply to them? So if we would assume we have a display with 8 pixel vertical packing and we would like to write 10x10 data we actually have to pass 16x10 data.
On the p (pitch parameter) see following comment:
#7833 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be acceptable to add 2 extra parameters (minW, minH) to the display capabilities structure and the caller of the API need to comply to them? So if we would assume we have a display with 8 pixel vertical packing and we would like to write 10x10 data we actually have to pass 16x10 data.
I think it is sufficient to specify the direction, e.g. SCREEN_INFO_MONO_VTILED see 588e7b6
On the p (pitch parameter) see following comment:
#7833 (comment)
I do not get it. I know pitch of as the number of octets for each row on the screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is sufficient to specify the direction, e.g. SCREEN_INFO_MONO_VTILED see 588e7b6
lgtm, how do we proceed on this, move the code to this PR or keep it in the char framebuffer PR?
I do not get it. I know pitch of as the number of octets for each row on the screen.
Buffer memory consumption could be reduced by performing some operations in place (resize, rotation) bit possibly at expense of more transfer operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, how do we proceed on this, move the code to this PR or keep it in the char framebuffer PR?
It can stay as it is.
Buffer memory consumption could be reduced by performing some operations in place (resize, rotation) bit possibly at expense of more transfer operations
I'm still not sure how it will be used. Can you give an example and insert it into the description of the parameter?
It would also be nice to have an API to rotate the image on the display by 180 °. Specifically, to change scanning start position if it is supported by the controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to have an API to rotate the image on the display by 180 °. Specifically, to change scanning start position if it is supported by the controller.
Could be useful to allow changing the screen orientation at runtime but would it not be better the to allow increments of 90 degrees (if supported)?
E.g.
enum display_orientation {
display_orientation_normal,
display_orientation_flipped, or display_orientation_180_degrees,
display_orientation_clockwise_90_degrees,
display_orientation_counter_clockwise_90_degrees,
};
int display_set_orientation(const struct device *dev, enum display_orientation orientation);
9f80471
to
bf5f5a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except pitch parameter only minor notes.
* | ||
*/ | ||
struct display_capabilities { | ||
u16_t x_resolution; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use shorter identifiers where possible, e.g. x_resolution -> x_res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is more clear to use the full named identifiers in the api structs
include/display.h
Outdated
* @param y y coordinate of the upper left corner | ||
* @param w width of the bitmap in pixels | ||
* @param h height of the bitmap in pixels | ||
* @param p pitch between lines in pixels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, only the parameter pitch
disturbs me. As I see in ili9340_write_bitmap the same can be achieved with sequential calls of display_write_bitmap with approximately the same performance. If pitch
is necessary, why can not it be realized through higher layers, why should every driver handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter was requested by @pizi-nordic and as the impact to the driver is minimal, splitting a single transfer in multiple transfers, it was added giving the higher layers more flexibility.
E.g. if you perform a image transformation on 30x30 of data you could transfer already 10x10 data once it is ready. Of course the higher layer could split it in 10 calls of 10x1, but for the current ili9340 driver this results in more SPI transfers as it reposition the window for every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course the higher layer could split it in 10 calls of 10x1, but for the current ili9340 driver this results in more SPI transfers as it reposition the window for every call.
It would be acceptable for me as it keeps the drivers simple. Otherwise, I fear higher probability for the errors, there is also no possibility to check whether calculated pointer is still within the limits of the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True that there is no way to check the buffer limit but it is also the case without the p parameter, the only solution here is to add yet another parameter to the function that contains the buffer length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about separating the function display_write/display_read
into display_set_mem_region(x, y, w, h)
and display_write(ptr, len)/dispay_read(ptr, len)
?
I hope you still have patience with me :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the point of the multi threading splitting the call in two will both increase the complexity of the higher layer software and the driver.
From the point of view of the higher layer software windowing is a useful feature for performance/memory reasons and lot of code will just result in calling display_set_mem_region
followed by a call to display_write
instead of a single call to display_write
.
For the driver the complexity goes up as it will need to cache the parameters passed in display_set_mem_region
to use the lather on in display_write
, e.g. because of the pitch (p) or for window bound checking (w*h < len). And then if you look to the point of @pizi-nordic regarding the multithreading it would be come even more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of view of the higher layer software windowing is a useful feature for performance/memory reasons and lot of code will just result in calling display_set_mem_region followed by a call to display_write instead of a single call to display_write.
Your suggestion of the display_write will do the same, set scanning region and then write to controller ram. I do not see any big impact on performance. My point remains: the function is too error prone. Which interface is safer: display_set_mem_region(dev, x, y, w, h) followed by display_write(dev, buf, len)/display_write(dev, buf, len, stride)
or display_write(dev, x, y, w, h, p, buf)
? Has no one bad feeling with display_read(dev, x, y, w, h, p, buf)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still dont understand that splitting an API in two makes things safer the only thing it does is complicate the API and as such makes it less safe.
The only valid point is that with the proposed API buffer size checking can only be done with the passed p and h parameters + pixel format and this could be more error prone then just a simple length parameter.
What about adding the len param to the proposed API and put the x, y, w and h in
a struct display_write(dev, dim, buf, len, p)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding the len param to the proposed API and put the x, y, w and h in
a struct display_write(dev, dim, buf, len, p)?
Alternatively, you can insert a structure that describes the display buffer (and perhaps the selected region). The buffer may also be larger than the memory of the display, e.g. to realize text scrolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfischer-phytec-iot updated api with a buffer descriptor
bf5f5a8
to
dc444d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove DNM to make it more attractive to potential reviewers
include/display.h
Outdated
* @param y y coordinate of the upper left corner | ||
* @param w width of the bitmap in pixels | ||
* @param h height of the bitmap in pixels | ||
* @param p pitch between lines in pixels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course the higher layer could split it in 10 calls of 10x1, but for the current ili9340 driver this results in more SPI transfers as it reposition the window for every call.
It would be acceptable for me as it keeps the drivers simple. Otherwise, I fear higher probability for the errors, there is also no possibility to check whether calculated pointer is still within the limits of the buffer.
dc444d8
to
e14ced1
Compare
@jfischer-phytec-iot Removed DNM and RFC from the tile but I cant remove the labels. On the other hand there are pending changes (#10029) to migrate the ili9340 driver to the new logging framework that should be merged first. |
@jfischer-phytec-iot can you revisit this so that we can move it forward? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- display_[on,off]_api should be renamed or removed, see my comments about "Callback API to turn display on"
- I am still not convinced with display_write/display_read, see my comments, but I do not want to slow it down any further.
include/display.h
Outdated
* @param y y coordinate of the upper left corner | ||
* @param w width of the bitmap in pixels | ||
* @param h height of the bitmap in pixels | ||
* @param p pitch between lines in pixels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of view of the higher layer software windowing is a useful feature for performance/memory reasons and lot of code will just result in calling display_set_mem_region followed by a call to display_write instead of a single call to display_write.
Your suggestion of the display_write will do the same, set scanning region and then write to controller ram. I do not see any big impact on performance. My point remains: the function is too error prone. Which interface is safer: display_set_mem_region(dev, x, y, w, h) followed by display_write(dev, buf, len)/display_write(dev, buf, len, stride)
or display_write(dev, x, y, w, h, p, buf)
? Has no one bad feeling with display_read(dev, x, y, w, h, p, buf)
?
e14ced1
to
9a11c1e
Compare
Renamed |
@jfischer-phytec-iot @pizi-nordic ready to approve? |
9a11c1e
to
5c8a184
Compare
Updated |
Migrate from SYS_LOG to LOG logging mechanism. Signed-off-by: Jan Van Winkel <[email protected]>
Migrate from SYS_LOG to LOG logging mechanism. Signed-off-by: Jan Van Winkel <[email protected]>
API for display drivers, supporting: * Turning on/off display blanking * Writing/Reading a bit map towards/from the display * Requesting framebuffer pointer * Setting display contrast and brightness * Querying display capabilities * Changing pixel format * Changing display orientation Signed-off-by: Jan Van Winkel <[email protected]>
Updated ILI9340 display driver and sample application to make use off the display API Signed-off-by: Jan Van Winkel <[email protected]>
5c8a184
to
cfc6241
Compare
Proposal for a minimal display API that can be used by a higher layer GUI library (see #6826)
Depends on: #10029