search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.49k stars 6.42k forks source link

ILI9341 (ILI9XXX) set orientation function fails to update the display area correctly #33269

Closed zpm1066 closed 3 years ago

zpm1066 commented 3 years ago

Describe the bug The ili9xxx_set_orientation() function rotates the display but doesn't update the display width and height after rotation, if applicable.

In the ILI9XXX driver (display_ili9xxx.c), ILI9XXX configuration is held in a static const struct with display size attributes x_resolution = 240 & y_resolution = 320. The display is configured for portrait mode. Display rotation works fine for 0 (normal) & 180 degrees but not for 90 or 270 degrees. After rotation, the display area is not being adjusted to cover the whole display area in landscape.

If the board overlay is configured for landscape mode (i.e. x_resolution = 320 & y_resolution = 240), then rotation for landscape views (90 & 270) are fine but incorrect when in portrait mode. Reason being that the display area is not being adjusted after rotation.

What have you tried to diagnose or workaround this issue? I have used several ILI9341 displays from 2.8", 2.4" and 2" sizes.

To Reproduce Build the Adafruit 2." TFT Touch Shield v2 examples, /zephyrproject/zephyr/samples/display/lvgl. I adapted the sample for Particle Xenon and BBC MicroBit v2 boards. The Particle Xenon board overlay is attached below.

Expected behavior The ili9xxx_set_orientation() function should correctly rotate the display for all supported rotations 0, 90, 180 and 270 degrees, and adjust the display area to cover whole of the screen size.

Impact Unable to use the whole display area for presentation of text and graphics.

Logs and console output Display size is defined in a static const ili9xxx_config structure and therefor fixed as 240x320 ili9xxx_set_orientation() can be used to rotate the display

Environment (please complete the following information):

Additional context Here's the devicetree section from the Particle Xenon nRF52840 board overlay.

&spi0 { compatible = "nordic,nrf-spi"; status = "okay"; sck-pin = <47>; mosi-pin = <45>; miso-pin = <46>; cs-gpios = <&gpio1 8 GPIO_ACTIVE_LOW>; / D4 / 

ili9340@0 {
    compatible = "ilitek,ili9340";
    label = "ILI9340";
    spi-max-frequency = <25000000>;
    reg = <0>;
    cmd-data-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;  /* D5 */
    reset-gpios = <&gpio0 31 GPIO_ACTIVE_LOW>;     /* A5 */
    pixel-format = <ILI9XXX_PIXEL_FORMAT_RGB565>; 
    rotation = <0>;
            frmctr1 = [00 18];
    pwctrl1 = [23 00];
    vmctrl1 = [3e 28];
    vmctrl2 = [86];
    pgamctrl = [0f 31 2b 0c 0e 08 4e f1 37 07 10 03 0e 09 00];
    ngamctrl = [00 0e 14 03 11 07 31 c1 48 08 0f 0c 31 36 0f];
};
gmarull commented 3 years ago

The current display width/height can be obtained using display_get_capabilities. As you can see in https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/display/display_ili9xxx.c#L297, the returned values will take into account the current rotation. Is that part failing?

zpm1066 commented 3 years ago

Thank you for your response.

The ili9xxx_get_capabilities() function does return the expected display size, howeverili9xxx_set_orientation() in /zephyr/drivers/display/display_ili9xxx.c does not update the display width (x_resolution) & height (y_resolution) depending on the rotation.

From my understanding, in ili9xxx_set_orientation, for example if we had display_width and display_height then for the rotations we would add the following:

   0 & 180 degrees:  display_width = ILI##t##_X_RES, display_height = ILI##t##_Y_RES
   90 & 270 degrees: display_width = ILI##t##_Y_RES, display_height = ILI##t##_X_RES

In ili9xxx_set_orientation(), the display orientation is correctly updated via data->orientation = orientation, so that ili9xxx_get_capabilities() returns the correct display resolution. Note that in ili9xxx_get_capabilities(), for 90 & 270 degrees, width & height get updated correctly. However, the ILI9XXX controller still has the display size prior to rotation. Therefore, after rotation, we'll see part of the display not used (see photo 2 below) thoughili9xxx_get_capabilities() will think it's working with the new resolution.

I have a patched version to work but since the x_resolution & y_resolution (and inversion ) are being held in a static const struct ili9xxx_config ili9xxx_config_##n and we appear to be using struct ili9xxx_data in display_ili9xxx.c, we may want to extend ili9xxx_data to add display_width, display_height (and display_inversion) so that rotation works correctly (and we can also add a new ili9xxx_set_inversion() function).

I assume we have the ili9xxx_data so that we leave ili9xxx_config_##n as is.

Do you know where ILI##t##_X_RES and ILI##t##_Y_RES are defined?

Thank you for looking at this issue.

Attached show the portrait image, portrait rotated by 90 degrees, and portrait rotated by 90 degrees correctly.

240x320_portrait 240x320_portrait_rotated jjpg 240x320_portrait_rotated_correctly

vanwinkeljan commented 3 years ago

@key1066 from the screenshot it looks like you are using LVGL. Is this a correct observation?

If yes did you updated the field rotated inlv_disp_drv_t when you called ili9xxx_set_orientation?

zpm1066 commented 3 years ago

Thank you for your suggestion. Yes, I'm using LVGL. No, I haven't updated the field rotated in lv_disp_drv_t. I'll take a look at your link and preview further.

From my earlier review of LVGL, my understanding from LVGL is that it doesn't rotate the output data itself, it only changes the logical screen size. The ILI9341 driver would still need to inform the ILI92431 controller of the new display area after sending the command, for example, to rotate the display from portrait to landscape 320x240.

In photo #2, LVGL won't be able to access the area outside the display screen size (non-white area), hence part of the logical 320x240 display that LVGL sees is off screen.

In the case of the Zephyr driver, ili9xxx_set_orientation changes the logical screen size from portrait to landscape so that display_get_capabilities reflects the landscape screen size but the ILI9341 controller still has the screen in portrait mode.

I have also reviewed the Adafruit ILI9341 driver source. They have a similar rotation method like ili9xxx_set_orientation but in addition to issuing the rotation command to the ILI9341 controller, their ILI9341 driver also updates the display screen size when rotating from portrait to landscape (or landscape to portrait), and the GFX basically works with the logical screen size. This is currently what I do in my patched version but like to see a proper fix.

I have the same ILI9341 displays working under the Arduino Framework using the Adafruit ILI9341 driver. Their demo app cycles through the rotations quite fast.

gmarull commented 3 years ago

I think the problem here is that LVGL configures its resolution when it is initialized (see https://github.com/zephyrproject-rtos/zephyr/blob/master/lib/gui/lvgl/lvgl.c#L99) but if you later change it via the display API you need to manually call https://docs.lvgl.io/latest/en/html/porting/display.html#_CPPv418lv_disp_drv_updateP9lv_disp_tP13lv_disp_drv_t with the new settings. @key1066 can you try?

zpm1066 commented 3 years ago

@vanwinkeljan, @gmarull, Thank you. Much appreciated. As per your suggestion, I've used lv_disp_drv_update() in a local function to update the display orientation at run time, as in below.

All works fine.

static void disp_update_orientation(enum display_orientation orientation) {
    lv_disp_t *lv_disp;
    const struct device *disp_dev;
    lv_disp_drv_t *lv_disp_drv;
        struct display_capabilities cap; 

    lv_disp = lv_disp_get_default();
    disp_dev = lv_disp->driver.user_data;
    lv_disp_drv = &lv_disp->driver;

        display_set_orientation(disp_dev,orientation);
        display_get_capabilities(disp_dev, &cap); 

    if (cap.current_orientation == DISPLAY_ORIENTATION_ROTATED_90 ||
         cap.current_orientation == DISPLAY_ORIENTATION_ROTATED_270) {
                lv_disp_drv->rotated = 1;
    } else 
        lv_disp_drv->rotated = 0;

       // update display driver
    lv_disp_drv_update(lv_disp, lv_disp_drv);
    lv_obj_invalidate(lv_scr_act());
}

I also use ST7789V displays and noticed that st7789v_set_orientation in https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/display/display_st7789v.c#L256 isn't currently implemented.

Are there plans to implement this function at some stage or is there another reason as to why it is not implemented? Thanks.

vanwinkeljan commented 3 years ago

I also use ST7789V displays and noticed that st7789v_set_orientation in https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/display/display_st7789v.c#L256 isn't currently implemented.

Are there plans to implement this function at some stage or is there another reason as to why it is not implemented?

@jfischer-no is there any reason why the function is not implemented?

@key1066 I assume we can close the issue?

zpm1066 commented 3 years ago

Regarding ST7789: A few months ago, I did open an enhancement request for ST7789 set orientation but got no response, https://github.com/zephyrproject-rtos/zephyr/issues/32286.

zpm1066 commented 3 years ago

Shall I open a separate bug for ST7789 set orientation in the hope it may get some attention?

zpm1066 commented 3 years ago

FYI - @vanwinkeljan, some of ST7789 displays like from Waveshare and others with CS can use the ILI9XXX driver.