samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates #84458

hakehuang · 2025-01-23T17:01:30Z

fix runtime memory issue on mimxrt1060_evkc

also add regx support for this sample

fixes: #84456

faxe1008

Not against this per se but some thoughts:

This needs to be split into two commits, one deals with fixing the memory requirement and the other adds a test harness.
Do not know if it is a usual thing to have a test harness for the samples. I would have thought thats what the tests are for.

faxe1008 · 2025-01-24T07:42:07Z

samples/subsys/display/lvgl/src/main.c

 	while (1) {
 		if ((count % 100) == 0U) {
 			sprintf(count_str, "%d", count/100U);
 			lv_label_set_text(count_label, count_str);
+			printf("run %d\n", count/100U);


count is uint32_t, needs to be %u

kartben · 2025-01-24T07:59:32Z

Do not know if it is a usual thing to have a test harness for the samples. I would have thought thats what the tests are for.

I was hoping someone would say that :) I don't really think it adds any value (having the text show up on the console proves literally nothing as to whether the sample actually works and display things correctly), and it unnecessarily clutters the user's console with "unhelpful" messages.
I remember making a similar comment in #78988

It is useful to add tests for checking lvgl works correctly, no doubt about that, but I think it should be done in... tests :) The samples' primary objective should remain to provide to the point code that people can use as a starting point.

kartben · 2025-01-24T08:01:45Z

samples/subsys/display/lvgl/boards/mimxrt1060_evk_mimxrt1062_qspi_C.conf

+# Enable PXP DMA engine and set rotation angle to 0 degrees.
+# This allows us to verify the DMA driver functions without altering
+# the output image


the comment here also seems to imply this conf is really only here for testing purposes? ("verify the DMA driver"??). This doesn't sound right. Also, maybe CONFIG_MCUX_ELCDIF_PXP should simply default to Y at the board/soc level when DMA is enabled?

this is reused from former mimxrt1060_evk board, CONFIG_MCUX_ELCDIF_PXP consumes a lot, we do not want it enabled by default if user do not use display.

Not enabling it by default is okay, but the comment needs to be changed.
I'd honestly just drop it - why would you enable DMA and PXP related settings if not to use/test them?

kartben · 2025-01-24T08:01:54Z

samples/subsys/display/lvgl/boards/mimxrt1060_evk_mimxrt1062_qspi_C.conf

+# Copyright 2023 NXP
+# SPDX-License-Identifier: Apache-2.0
+
+# Enable PXP DMA engine and set rotation angle to 0 degrees.


where is rotation angle set to 0?

@kartben this is reused from former platform, in PXP engine we can rotate the image by 90/180/270 degree, so this is default setting.

This still needs to be addressed, imo. The comment is not needed if it is a default. It tends to confuse anyone who reads it, because nowhere is there any config about rotation in here.

hakehuang · 2025-01-24T08:50:34Z

I was hoping someone would say that :) I don't really think it adds any value (having the text show up on the console proves literally nothing as to whether the sample actually works and display things correctly), and it unnecessarily clutters the user's console with "unhelpful" messages. I remember making a similar comment in #78988

It is useful to add tests for checking lvgl works correctly, no doubt about that, but I think it should be done in... tests :) The samples' primary objective should remain to provide to the point code that people can use as a starting point.

without this fix, the build will fail by twister #84456 , as the regex is a must in harness: console, this adds a very basic check on the process, if you look at the source code below, there are several checks, if those fail, we can catch that in this pr's change. all in one, just to make the samples can pass in CI process. otherwise we can mark it as build only, as there is already a test for lvgl.

		LOG_ERR("Device not ready, aborting test");
		return 0;

faxe1008 · 2025-01-29T12:18:27Z

Hmm ok, the build issue is annoying. Either remove the console harness completely or go with this change then.
However the board specific change still has to go into a seperate commit either way (And also the commit title needs to be adjusted).

hakehuang · 2025-01-30T05:30:23Z

Hmm ok, the build issue is annoying. Either remove the console harness completely or go with this change then.
However the board specific change still has to go into a seperate commit either way (And also the commit title needs to be adjusted).

@faxe1008 thanks, I have split the PR into two commits, and update the commit message. Please check.

faxe1008

For the 48d8493be4ffd7ba91deb8dd2496f8f7bad7c51f the commit message needs adjusting:

The board is mimxrt1060_evkc and this specific commit does not fix tests: samples/subsys/display/lvgl/samples: HARNESS:Console:no regex patterns configured. #84456

faxe1008 · 2025-01-30T07:38:59Z

samples/subsys/display/lvgl/boards/mimxrt1060_evk_mimxrt1062_qspi_C.conf

+# Copyright 2023 NXP
+# SPDX-License-Identifier: Apache-2.0
+
+# Enable PXP DMA engine and set rotation angle to 0 degrees.


This still needs to be addressed, imo. The comment is not needed if it is a default. It tends to confuse anyone who reads it, because nowhere is there any config about rotation in here.

faxe1008 · 2025-01-30T07:40:34Z

samples/subsys/display/lvgl/boards/mimxrt1060_evk_mimxrt1062_qspi_C.conf

+# Enable PXP DMA engine and set rotation angle to 0 degrees.
+# This allows us to verify the DMA driver functions without altering
+# the output image


Not enabling it by default is okay, but the comment needs to be changed.
I'd honestly just drop it - why would you enable DMA and PXP related settings if not to use/test them?

hakehuang · 2025-01-30T16:03:55Z

@faxe1008 , thanks for reminding, I miss your former change request, and update now. please review again. sorry for troubles. and thanks again.

kartben · 2025-01-30T16:17:27Z

FWIW I agree with @faxe1008 of removing the console harness altogether

hakehuang · 2025-02-03T16:58:30Z

FWIW I agree with @faxe1008 of removing the console harness altogether

@kartben , if in this case, samples can not covered in CI board run testing with at least minimal coverage.

faxe1008 · 2025-02-04T07:11:30Z

There is still the tests/lib/gui/lvgl for which you can of course provide a new test config. As for the general point if samples should be used for testing:
https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#samples-must-not-be-used-to-test-features-or-verify-platforms

If a sample can provide output that can be verified, then output should be evaluated against expected value to have some level of testing for the sample itself.

The PR does provide some output, but I feel like it does not provide any verification besides "the LVGL event loop has run at least once". The changes themselves look fine now. Not gonna +1 for now, but I will remove my nack.

Changes look good, but unsure if the overall distinction between samples and tests is correctly applied here.

hakehuang · 2025-02-05T06:01:29Z

any verification besides "the LVGL event loop has run at least once"

@faxe1008 @kartben , the ideas are:

for each samples in zephyr, try to make them as more as possible testing coverage in CI (board farm CI)
tests is to ensure the framework/driver/api works, but samples is to demo that platform with necessary setting can run with combined user case, and I would say it is good that we also have some commitment that it at least runs.

add console output for sanity check in CI. this will make twister build pass with harness: console fixes: zephyrproject-rtos#84456 Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

unify the configs and remove platform_allow as possible Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

add mipi and lcd_if tag for NXP platforms Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

zephyrbot added area: Display area: Samples Samples labels Jan 23, 2025

zephyrbot requested review from danieldegrasse, jfischer-no, kartben and nashif January 23, 2025 17:02

zephyrbot assigned kartben Jan 23, 2025

hakehuang added the platform: NXP NXP label Jan 23, 2025

kartben assigned faxe1008 and unassigned kartben Jan 23, 2025

hakehuang force-pushed the fix_lvgl_rt1060_evkc branch from 421c52a to 66537f9 Compare January 23, 2025 17:08

JarmouniA added the area: LVGL Light and Versatile Graphics Library Support label Jan 23, 2025

faxe1008 requested changes Jan 24, 2025

View reviewed changes

kartben requested changes Jan 24, 2025

View reviewed changes

hakehuang force-pushed the fix_lvgl_rt1060_evkc branch from 66537f9 to dfaf544 Compare January 24, 2025 08:58

hakehuang requested review from faxe1008 and kartben January 24, 2025 09:01

hakehuang force-pushed the fix_lvgl_rt1060_evkc branch from dfaf544 to 48d8493 Compare January 30, 2025 05:28

hakehuang changed the title ~~tests: lvgl: fix run time memory issue~~ samples: lvgl: fix run time memory issue Jan 30, 2025

hakehuang changed the title ~~samples: lvgl: fix run time memory issue~~ samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates Jan 30, 2025

faxe1008 previously requested changes Jan 30, 2025

View reviewed changes

hakehuang force-pushed the fix_lvgl_rt1060_evkc branch from 48d8493 to dff399b Compare January 30, 2025 16:01

hakehuang mentioned this pull request Feb 7, 2025

Zephyr in tree samples CI coverage criteria and elastic quality maintain policy enhancement #85359

Open

6 tasks

hakehuang added 3 commits March 4, 2025 09:35

samples: lvgl: add console output for sanity check

cc7c3fa

add console output for sanity check in CI. this will make twister build pass with harness: console fixes: zephyrproject-rtos#84456 Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

samples: lvgl: revise NXP related settings

893b71d

unify the configs and remove platform_allow as possible Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

boards: meta: add mipi and lcd_if tag

3324962

add mipi and lcd_if tag for NXP platforms Signed-off-by: Hake Huang <hake.huang@oss.nxp.com>

hakehuang force-pushed the fix_lvgl_rt1060_evkc branch from dff399b to 3324962 Compare March 4, 2025 16:12

zephyrbot requested review from butok, decsny, DerekSnell, dleach02, EmilioCBen and mmahadevan108 March 4, 2025 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates #84458

samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates #84458

hakehuang commented Jan 23, 2025 •

edited

Loading

faxe1008 left a comment

faxe1008 Jan 24, 2025

kartben commented Jan 24, 2025

kartben Jan 24, 2025

hakehuang Jan 24, 2025 •

edited

Loading

faxe1008 Jan 30, 2025

kartben Jan 24, 2025

hakehuang Jan 24, 2025 •

edited

Loading

faxe1008 Jan 30, 2025

hakehuang commented Jan 24, 2025 •

edited

Loading

faxe1008 commented Jan 29, 2025 •

edited

Loading

hakehuang commented Jan 30, 2025

faxe1008 left a comment

faxe1008 Jan 30, 2025

faxe1008 Jan 30, 2025

hakehuang commented Jan 30, 2025

kartben commented Jan 30, 2025

hakehuang commented Feb 3, 2025

faxe1008 commented Feb 4, 2025

hakehuang commented Feb 5, 2025

samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates #84458

Are you sure you want to change the base?

samples: lvgl: enhacements for CI check and mimxrt1060_evkc updates #84458

Conversation

hakehuang commented Jan 23, 2025 • edited Loading

faxe1008 left a comment

Choose a reason for hiding this comment

faxe1008 Jan 24, 2025

Choose a reason for hiding this comment

kartben commented Jan 24, 2025

kartben Jan 24, 2025

Choose a reason for hiding this comment

hakehuang Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

faxe1008 Jan 30, 2025

Choose a reason for hiding this comment

kartben Jan 24, 2025

Choose a reason for hiding this comment

hakehuang Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

faxe1008 Jan 30, 2025

Choose a reason for hiding this comment

hakehuang commented Jan 24, 2025 • edited Loading

faxe1008 commented Jan 29, 2025 • edited Loading

hakehuang commented Jan 30, 2025

faxe1008 left a comment

Choose a reason for hiding this comment

faxe1008 Jan 30, 2025

Choose a reason for hiding this comment

faxe1008 Jan 30, 2025

Choose a reason for hiding this comment

hakehuang commented Jan 30, 2025

kartben commented Jan 30, 2025

hakehuang commented Feb 3, 2025

faxe1008 commented Feb 4, 2025

hakehuang commented Feb 5, 2025

hakehuang commented Jan 23, 2025 •

edited

Loading

hakehuang Jan 24, 2025 •

edited

Loading

hakehuang Jan 24, 2025 •

edited

Loading

hakehuang commented Jan 24, 2025 •

edited

Loading

faxe1008 commented Jan 29, 2025 •

edited

Loading