Skip to content

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Jan 14, 2020

Contribution description

This PR adds support to the LittlevGL package to RIOT. I've been looking to it for quite some time and now that the ili9341 driver is in, I think it's time to move forward with this.

This PR also introduces a generic (and still basic) API for display devices, it follows the same scheme as netdev.

There's also a test application that display a system monitor on a screen. It was tested on the stm32f429i-disc1 board.

I know other people in the community (@bergzand, @fjmolinas) have already done some work with this package and maybe they can provide some good advice here so we can move forward faster.

Ideas for future improvements:

  • improve package configuration macros: maybe even in this PR (I think it's the weakest point of this PR)
  • provide a driver for the stm32f429i-disco touch screen, some months ago I started to write a basic driver but it still needs some work
  • provide support for lvgl-examples in another package (that depends on this one), this way it will be simpler to try different lvgl features.

Testing procedure

Grab an stm32f429i-disc1 board and run the following command

make -C tests/pkg_lvgl flash term

Issues/PRs references

Based on ##13262

@aabadie aabadie added the Area: pkg Area: External package ports label Jan 14, 2020
@aabadie aabadie requested review from bergzand and fjmolinas January 14, 2020 15:58
@aabadie aabadie changed the title pkg/littlevgl: add initial support to RIOT pkg/lvgl: add initial support for LittlevGL to RIOT Jan 14, 2020
@kaspar030
Copy link
Contributor

I tested this in the PineTime. Seems to work, with just minor modifications!

  1. the pinetime has only 240x240 pixels. needed to adjust that. I think this needs to become a parameter if the ili9341, and I think the disp_dev needs a way to query that.

  2. the pinetime uses gpios to control the backlight. Maybe support for that also needs to be added to disp_dev?

I have a picture, but somehow my Gnome/Wayland doesn't let me paste it here. ;(

@aabadie
Copy link
Contributor Author

aabadie commented Jan 14, 2020

I think this needs to become a parameter if the ili9341, and I think the disp_dev needs a way to query that

It's not that simple without patching lvgl. The LV_HOR_RES_MAX and LV_VER_RES_MAX are used internally when initializing the display, see here.
in lv_conf.h, these macro can be overridden: this provides compile time configuration.

Maybe support for that also needs to be added to disp_dev?

Probably doable, with get/set functions.

@kaspar030
Copy link
Contributor

It's not that simple without patching lvgl. The LV_HOR_RES_MAX and LV_VER_RES_MAX are used internally when initializing the display, see here.
in lv_conf.h, these macro can be overridden: this provides compile time configuration.

Hm, looking at the "porting" example here, they first initialize the driver struct (using lv_disp_drv_init(), which just clears and sets values), then afterwards override the resolution and buffers. Maybe we can do it similarly?

BTW, at this point, does it make sense to introduce disp_dev_t? We could just use lv_disp_t. It even provides a method to add user defined fields (https://github.com/littlevgl/lvgl/blob/51928059d2dda465ab76ee66cf37b9d2edb9f64e/src/lv_hal/lv_hal_disp.h#L115-L117).

@bergzand
Copy link
Member

I see the merits of having a display_dev abstraction for display devices. However for now I would like to propose to either:

  • split the display_dev header to a separate PR and continue this with only support for the ili9341 device driver, it is the only display supported at the moment anyway.

or:

  • Keep the display_dev as it is now but mark it as experimental, I think it is a good start and we can iterate over it in a follow up PR.

@aabadie
Copy link
Contributor Author

aabadie commented Jan 15, 2020

I see the merits of having a display_dev abstraction for display devices

The abstraction design has no external dependency so each display driver can provide the required driver functions (only one for the moment). If we reuse the abstraction API that comes from lvgl, then this introduce a hard dependency to it. I would prefer to avoid this.
@fjmolinas has a driver for other LCD modules that could benefit from this very easily.

So for now I would use option B, as proposed by @bergzand: keep the disp_dev header and improve later.

Regarding the while loop in the task thread, I agree it's not nice, especially because of power consumption.
We could use a blocking msg_receive function there when the system is inactive and send a message to this thread on wakeup. Something like:

while (1) {
    /*Normal operation (no sleep) in < 1 sec inactivity*/
    if (lv_disp_get_inactive_time(NULL) < 1000) {
        lv_task_handler();
    }
    /* Block after 1 sec inactivity */
    else {
        msg_t msg;
        msg_receive(&msg);
    }
    xtimer_usleep(5 * US_PER_MS);
}

Then the screen could be explicitly restarted from user code with a simple msg_send.

@kaspar030
Copy link
Contributor

So for now I would use option B, as proposed by @bergzand: keep the disp_dev header and improve later.

ok

@kaspar030
Copy link
Contributor

Then the screen could be explicitly restarted from user code with a simple msg_send.

better use thread flags. Cannot get lost (e.g., if set from ISR), doesn't block caller.

@aabadie
Copy link
Contributor Author

aabadie commented Jan 16, 2020

better use thread flags.

@kaspar030, I just pushed a couple of commits that implement a mechanism to allow stopping the lvgl task handler thread after some inactivity period (defined by the LVGL_ACTIVITY_PERIOD macro). The glue code now provides an extra function lvgl_wakeup that needs to be called when lvgl should be restarted (or the inactivity limit be kicked).

In the sysmon example, this function is called each time the sysmon_task is called: this ensures the chart is always refreshed. But if you tweak the REFR_TIME macro in main.c to be greater than LVGL_ACTIVITY_PERIOD, then lvgl is stopped after LVGL_ACTIVITY_PERIOD is elapsed and an explicit call to lvgl_wakeup is needed to restart it (this can be done in a gpio callback for example).

What do you think of the approach ?

@aabadie
Copy link
Contributor Author

aabadie commented Jan 16, 2020

I added a bit of documentation.

@kaspar030, if you think the thread_flags is good enough, this PR is close to be mergeable I think.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2020
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 17, 2020
@aabadie aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 2, 2020
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

first round

#define LVGL_TASK_HANDLER_DELAY (5 * US_PER_MS)
#endif

static char _task_thread_stack[THREAD_STACKSIZE_MAIN];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "THREAD_STACKSIZE_DEFAULT" (or maybe _MEDIUM or _LARGE?).
A user might have a large main and thus overrides "THREAD_STACKSIZE_MAIN", which shouldn't affect lvgl's thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to THREAD_STACKSIZE_LARGE, otherwise it doesnt fit.

static lv_color_t buf[LVGL_COLOR_BUF_SIZE];
static disp_dev_t *_dev = NULL;

void *_task_thread(void *arg)
Copy link
Contributor

Choose a reason for hiding this comment

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

static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all fixed

return NULL;
}

void _disp_map(lv_disp_drv_t *drv, const lv_area_t *area, lv_color_t *color_p)
Copy link
Contributor

Choose a reason for hiding this comment

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

static?


USEPKG += lvgl
USEMODULE += ili9341
USEMODULE += xtimer
Copy link
Contributor

Choose a reason for hiding this comment

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

lvgl already depends on xtimer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a left-over from previous versions, removed.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 3, 2020
@aabadie aabadie self-assigned this Mar 3, 2020
@aabadie aabadie force-pushed the pr/pkg/littlevgl branch from 9434f1d to 96297fb Compare March 3, 2020 15:57
@aabadie
Copy link
Contributor Author

aabadie commented Mar 3, 2020

I think I addressed all your comments @bergzand :)

@aabadie aabadie assigned bergzand and unassigned aabadie Mar 4, 2020
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Second round of reviewing

@aabadie aabadie force-pushed the pr/pkg/littlevgl branch from 96297fb to 8d4379e Compare March 5, 2020 14:33
@aabadie
Copy link
Contributor Author

aabadie commented Mar 5, 2020

Comments addressed @bergzand :)

@aabadie
Copy link
Contributor Author

aabadie commented Mar 10, 2020

ping @bergzand :)

@bergzand
Copy link
Member

Please squash after addressing my last comment

@aabadie aabadie force-pushed the pr/pkg/littlevgl branch 2 times, most recently from 90a116c to 6b7be34 Compare March 18, 2020 13:50
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Ack!

@kaspar030 kaspar030 merged commit 94ade93 into RIOT-OS:master Mar 18, 2020
@aabadie aabadie deleted the pr/pkg/littlevgl branch March 19, 2020 08:06
@aabadie
Copy link
Contributor Author

aabadie commented Mar 19, 2020

Awesome ! Thanks a lot for the reviews and tests :)

@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants