feat(nxp/pxp): add Zephyr Support #6298

faxe1008 · 2024-05-29T11:22:48Z

Description of the feature or fix

Reopening of #6159 since it got merged and reverted the original PR does not track changes anymore (@nicusorcitu ).

Notes

Update the Documentation if needed.
Add Examples if relevant.
Add Tests if applicable.
If you added new options to lv_conf_template.h run lv_conf_internal_gen.py and update Kconfig.
Run scripts/code-format.py (astyle version v3.4.12 needs to be installed) and follow the Code Conventions.
Mark the Pull request as Draft while you are working on the first version, and mark is as Ready when it's ready for review.
When changes were requested, re-request review to notify the maintainers.
Help us to review this Pull Request! Anyone can approve or request changes.

nicusorcitu · 2024-05-29T12:02:30Z

@cristian-stoica

nicusorcitu · 2024-05-30T06:53:23Z

src/draw/nxp/pxp/lv_pxp_osa.c

-    #include "FreeRTOS.h"
-    #include "semphr.h"
+
+#if defined(__ZEPHYR__)


Those should not be included - they will get over lv_os.h inclusion

nicusorcitu · 2024-05-30T07:10:26Z

src/draw/nxp/pxp/lv_pxp_osa.c

    NVIC_SetPriority(PXP_IRQ_ID, configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY + 1);
+    NVIC_EnableIRQ(PXP_IRQ_ID);
+#elif defined(__ZEPHYR__)
+    IRQ_CONNECT(DT_IRQN(DT_NODELABEL(pxp)), CONFIG_LV_Z_PXP_INTERRUPT_PRIORITY, PXP_ZephyrIRQHandler, NULL, 0);


I understand now, you included the ZEPHYR header just for this CONFIG_LV_Z_PXP_INTERRUPT_PRIORITY.

We should not mix any longer this with lv_osal support.

The includes are needed for the entire interrupt logic (retrieving nodes from DT, hooking up the IRQ).

src/draw/nxp/pxp/lv_pxp_osa.c

cristian-stoica · 2024-06-10T10:37:04Z


-#if defined(SDK_OS_FREE_RTOS)
-    #include "FreeRTOS.h"
-    #include "semphr.h"
+
+#if defined(__ZEPHYR__)
+    #include <zephyr/kernel.h>
+    #include <zephyr/irq.h>
#endif

I have a problem with the code above. It is not exactly OSAL as it claims and it also breaks our build. There has to be a better way

faxe1008 · 2024-06-17T12:21:07Z

-#if defined(SDK_OS_FREE_RTOS)
-    #include "FreeRTOS.h"
-    #include "semphr.h"
+
+#if defined(__ZEPHYR__)
+    #include <zephyr/kernel.h>
+    #include <zephyr/irq.h>
#endif
I have a problem with the code above. It is not exactly OSAL as it claims and it also breaks our build. There has to be a better way

Yeah, these includes are needed for the interrupts. Personally I think that an OSAL is maybe not the right place for an abstraction for this, the OSAL should be kept as thin as possible. Maybe it would be an option somehow provide the user a way to give LVGL a setup and teardown method for any of the drawing engines so that this can be handled by each OS?

src/draw/nxp/pxp/lv_pxp_osa.c

@0xFarahFl

Add semaphore and connect IRQ. Originally proposed by @0xFarahFl.

src/osal/lv_freertos.c

kisvegabor · 2024-06-21T06:34:26Z

We got a lot request about updating Zephyr to LVGL v9 so I wonder if we can help to make it happen soon. @becseya is interested in Zephyr, so he can help with LVGL related issues. We also have several NXP boards so we can help with PXP as well.

kisvegabor · 2024-06-21T06:50:21Z

155 | lv_result_t lv_thread_sync_signal_isr(lv_thread_sync_t * sync)

Please add LV_UNUSED(sync);

faxe1008 · 2024-06-21T07:25:46Z

The function var had to be renamed to pxCond :^)

kisvegabor · 2024-06-21T07:46:48Z

The function var had to be renamed to pxCond

Which one? 🙂

nicusorcitu · 2024-06-21T12:55:01Z

As a general rule, inside the FreeRTOS files I have used FreeRTOS style while naming variables, functions...
Thanks for keeping it on the same manner.

cristian-stoica · 2024-06-21T12:57:16Z

The last version of these patches fix the build regression I was looking at (PXP = 1 , OS_NONE on MCUXpresso SDK projects that always define SDK_OS_FREE_RTOS). There are other improvements that could be made but they can be addressed later.
I'm OK with this for now modulo other issues discussed in different threads.

kisvegabor · 2024-06-24T16:54:18Z

Please add lv_thread_sync_signal_isr to lv_os_none.c too.

faxe1008 · 2024-06-24T20:05:20Z

@kisvegabor added it to lv_os_none :^)

kisvegabor · 2024-06-25T08:27:29Z

Thanks, I've fixed the unused variable warnings.

liamHowatt

Nice atomic CAS loop!

In another PR we should correct the implementation of lv_mutex_lock_isr for cmsis_rtos2. See this comment.

liamHowatt · 2024-06-25T18:01:49Z

src/draw/nxp/pxp/lv_pxp_osa.c

+#if defined(__ZEPHYR__)
+static void PXP_ZephyrIRQHandler(void *)
+{
+    PXP_IRQHandler();
+}
+#endif


Please put this in the static functions section and forward declare it. Maybe also change the name to make it seem less like a platform specific global function definition (unless it is).

Ah it kinda is - I do not know how many other RTOS give the possibility to add userdata to an ISR.

What I mean is the name implies that it's overriding a weak definition in the OS like a systick handler that gets called by the OS, but it's really just a local function.

Also, does PXP_IRQHandler need to be global? Does it get called by anything external?

liamHowatt · 2024-06-25T19:03:11Z

src/osal/lv_freertos.c

+    /* Acquire the mutex. */
+    if(pdFALSE == xSemaphoreTakeFromISR(pxCond->xSyncMutex, NULL)) {
+        return LV_RESULT_INVALID;
+    }


If this fails because lv_thread_sync_wait has the mutex, will the signal never be sent and a deadlock would occur?

True - what would be the best solution for this? Not using the mutex at all and disabling interrupts instead so we can not be preempted by another ISR?

See my last commit, if the change is ok from your side, I'll squash it :^)

Yes, I think that should work, nice. Is the CAS loop no longer needed now?

Changes the PXP osal to use the thread_sync to signal PXP readyness from the ISR.

faxe1008 mentioned this pull request May 29, 2024

LVGL Integration strategy for Zephyr #6293

Open

FASTSHIFT changed the title ~~PXP Zephyr Support~~ feat(nxp/pxp): add Zephyr Support May 30, 2024

nicusorcitu reviewed May 30, 2024

View reviewed changes

cristian-stoica reviewed May 30, 2024

View reviewed changes

src/draw/nxp/pxp/lv_pxp_osa.c Outdated Show resolved Hide resolved

faxe1008 force-pushed the pxp_zephyr branch from fd1e246 to 158a93a Compare June 7, 2024 08:26

kisvegabor mentioned this pull request Jun 7, 2024

About the Release Date of New Versions #6319

Open

faxe1008 force-pushed the pxp_zephyr branch from 158a93a to 6c5985f Compare June 17, 2024 12:17

nicusorcitu reviewed Jun 17, 2024

View reviewed changes

src/draw/nxp/pxp/lv_pxp_osa.c Outdated Show resolved Hide resolved

feat(pxp): Add zephyr support

5a7c75a

Add semaphore and connect IRQ. Originally proposed by @0xFarahFl.

faxe1008 force-pushed the pxp_zephyr branch 2 times, most recently from e339205 to cb4c167 Compare June 17, 2024 14:15

nicusorcitu reviewed Jun 17, 2024

View reviewed changes

src/osal/lv_freertos.c Outdated Show resolved Hide resolved

nicusorcitu reviewed Jun 17, 2024

View reviewed changes

src/osal/lv_freertos.c Outdated Show resolved Hide resolved

nicusorcitu mentioned this pull request Jun 21, 2024

feat(pxp): hookup zephyr interrupts #6158

Closed

faxe1008 force-pushed the pxp_zephyr branch 2 times, most recently from 0f12024 to 083a95b Compare June 21, 2024 06:48

faxe1008 force-pushed the pxp_zephyr branch from 083a95b to ff3254f Compare June 21, 2024 07:24

nicusorcitu approved these changes Jun 21, 2024

View reviewed changes

faxe1008 force-pushed the pxp_zephyr branch from ff3254f to bc59538 Compare June 24, 2024 20:03

kisvegabor previously approved these changes Jun 25, 2024

View reviewed changes

kisvegabor requested a review from liamHowatt June 25, 2024 08:50

liamHowatt reviewed Jun 25, 2024

View reviewed changes

faxe1008 added 2 commits June 26, 2024 08:42

feat(os): add lv_thread_sync_signal_isr to OSAL

5343050

feat(pxp): Use OSAL thread_sync for interrupt signaling

14f1fab

Changes the PXP osal to use the thread_sync to signal PXP readyness from the ISR.

faxe1008 dismissed kisvegabor’s stale review via 14f1fab June 26, 2024 06:43

faxe1008 force-pushed the pxp_zephyr branch from af8fd14 to 14f1fab Compare June 26, 2024 06:43

fix mutex inside of thread_sync_signal_isr

3ac2d39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nxp/pxp): add Zephyr Support #6298

feat(nxp/pxp): add Zephyr Support #6298

faxe1008 commented May 29, 2024

nicusorcitu commented May 29, 2024

nicusorcitu May 30, 2024 •

edited

Loading

nicusorcitu May 30, 2024

faxe1008 Jun 7, 2024

cristian-stoica commented Jun 10, 2024 •

edited

Loading

faxe1008 commented Jun 17, 2024

kisvegabor commented Jun 21, 2024 •

edited

Loading

kisvegabor commented Jun 21, 2024

faxe1008 commented Jun 21, 2024

kisvegabor commented Jun 21, 2024

nicusorcitu commented Jun 21, 2024

cristian-stoica commented Jun 21, 2024

kisvegabor commented Jun 24, 2024

faxe1008 commented Jun 24, 2024

kisvegabor commented Jun 25, 2024

liamHowatt left a comment

liamHowatt Jun 25, 2024

faxe1008 Jun 26, 2024

liamHowatt Jun 26, 2024

liamHowatt Jun 25, 2024

faxe1008 Jun 26, 2024

faxe1008 Jun 26, 2024

liamHowatt Jun 26, 2024

feat(nxp/pxp): add Zephyr Support #6298

Are you sure you want to change the base?

feat(nxp/pxp): add Zephyr Support #6298

Conversation

faxe1008 commented May 29, 2024

Description of the feature or fix

Notes

nicusorcitu commented May 29, 2024

nicusorcitu May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristian-stoica commented Jun 10, 2024 • edited Loading

faxe1008 commented Jun 17, 2024

kisvegabor commented Jun 21, 2024 • edited Loading

kisvegabor commented Jun 21, 2024

faxe1008 commented Jun 21, 2024

kisvegabor commented Jun 21, 2024

nicusorcitu commented Jun 21, 2024

cristian-stoica commented Jun 21, 2024

kisvegabor commented Jun 24, 2024

faxe1008 commented Jun 24, 2024

kisvegabor commented Jun 25, 2024

liamHowatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicusorcitu May 30, 2024 •

edited

Loading

cristian-stoica commented Jun 10, 2024 •

edited

Loading

kisvegabor commented Jun 21, 2024 •

edited

Loading