samples: lvgl: enhacements for CI check updates for NXP platforms

[hakehuang]

fix runtime memory issue on mimxrt1060_evkc
also add regx support for this sample, so twsiter can run this sample in board CI.
remove NXP cases constrains with platform_allow, now it is based on tags and dts/kconfig.
Also fix a build issue in rt595 when display feature is in support list.

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]

count is uint32_t, needs to be %u

[kartben]

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]

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?

[hakehuang]

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.

[faxe1008]

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]

where is rotation angle set to 0?

[hakehuang]

@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.

[faxe1008]

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]

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]

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]

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 #84456

[faxe1008]

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]

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]

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

[kartben]

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

[hakehuang]

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]

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.

[hakehuang]

@faxe1008 @kartben now I update the PR for below, please help to review and check. Thanks

1. use right tags to control test platforms.
2. remove NXP platforms in platform_allow, and base on dts/Kconfigs
3. fix a build error, which exposed by this PR.
4. make this sample can run with twister as basic quality assurance.

[faxe1008]

Again I am not sold on the idea of this having a test harness at all. This is a sample which shows how to add widgets and assign them input devices. Currently the hardness does not verify functionality:

if ((count % 100) == 0U) {
			sprintf(count_str, "%d", count/100U);
			lv_label_set_text(count_label, count_str);
			printf("run %u\n", count/100U);
		}
		lv_timer_handler();

If no interaction on the device is executed during tests the only thing the handler has to do is triggering a partial screen update for this label text. However even that does not verify anything:

• the set_text does a bit of internal math and enqueues a screen invalidation
• printf is executed - basically the test can be now considered as passed
• the handler is executed triggering the screen writing

So the test does not actually test things to the point where you can say "yup, the test is green there are definetely no issues with the LVGL integration on Zephyr for that board". In my oppionion we would benefit more from excercising the glue code that is maintained by Zephyr and make sure that is working as intended. This patch does not deliver on that imo. Functionality of LVGL itsself like screen invalidation logic, management of input is best tested by themselves in upstream.

The changes to the board overlays and tags look fine however if you want to proceed adding them.

[faxe1008]

The commit message has a typo:
idts: mimxrt595_evk_cm33: mipi_dsi dts updates

[faxe1008]

What is the rationale to not have this as a board overlay conf instead?

[hakehuang]

What is the rationale to not have this as a board overlay conf instead?

@faxe1008 I do not want this to be added to each platform, as this is generally applies to all NXP platform. so only one config is added.

[hakehuang]

So the test does not actually test things to the point where you can say "yup, the test is green there are definetely no issues with the LVGL integration on Zephyr for that board". In my oppionion we would benefit more from excercising the glue code that is maintained by Zephyr and make sure that is working as intended. This patch does not deliver on that imo. Functionality of LVGL itsself like screen invalidation logic, management of input is best tested by themselves in upstream.

The changes to the board overlays and tags look fine however if you want to proceed adding them.

@faxe1008 , I agree with you that we should use a glue code to test the lvgl in more details, which can go to the tests, and I can work with you to implement that. the idea here is to not to make this sample as a test. Instead, what I am doing is to ensure this samples is runnable on applicable NXP platforms. So I add printf("run %u\n", count/100U) with console check. So I can include this sample in CI running, and I can at least say that this samples can run on appliable devices, combined with tests I have full confidence on both lvgl and its samples on all appliable platforms. Without this check, CI can only build on it, and run will return timeout. This is not about whether sample should be used as a test or not, it is about to maintain the quality of sample application over lifecycle in Zephyr.

[hakehuang]

pre TWG meeting, I will propose a PR to update the sample criteria for review such changes first.

#87089

[github-actions]

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.

[hakehuang]

still need follow up

[github-actions]

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.

[hakehuang]

will update after #91597 meger

[github-actions]

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.

[hakehuang]

will update with #94961

[github-actions]

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.