Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pxp): add zephyr support #6159

Merged
merged 2 commits into from May 24, 2024
Merged

feat(pxp): add zephyr support #6159

merged 2 commits into from May 24, 2024

Conversation

faxe1008
Copy link
Contributor

Add semaphore and connect IRQ.
Originally proposed by @0xFarahFl. zephyrproject-rtos/lvgl#52

Description of the feature or fix

A clear and concise description of what the bug or new feature is.

Notes

@kisvegabor
Copy link
Member

Hi,

We can merge it now as a hotfix, however I think a proper solution would be to

  1. Add built-in Zephyr support to LVGL. See here
  2. Use LVGL's OS API in PXP too.

What do you think?

cc @nicusorcitu @anaGrad

Add semaphore and connect IRQ.
Originally proposed by @0xFarahFl.
@faxe1008
Copy link
Contributor Author

faxe1008 commented May 3, 2024

Hey @kisvegabor

Add built-in Zephyr support to LVGL. See here

With that one I am still a bit hesitant - the OSAL needs dynamic thread creation and this is still marked as experimental so subject to change. By keeping the OSAL intree for now we can make sure that any changes will cause CI breakage and are addresed. https://github.com/zephyrproject-rtos/zephyr/blob/74ddfbc8a2e4f1932bf2b6a160f83a27a6c135a5/kernel/Kconfig#L237

Use LVGL's OS API in PXP too.

Good point I'll adjust this PR accordingly :^)

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

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Thank you! It's good for me, but I hope @nicusorcitu and/or @anaGrad can take a look too.

@nicusorcitu
Copy link
Contributor

@cristian-stoica should review it also

#endif
#if defined(__ZEPHYR__)
static K_SEM_DEFINE(s_pxpIdleSem, 0, 1);
#if LV_USE_OS
Copy link
Contributor

Choose a reason for hiding this comment

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

PXP can work with or without FreeRTOS support.
PXP in baremetal mode means that SDK_OS_FREE_RTOS = 0.

While LV_USE_OS is a feature added in LVGL v9 for drawing parallelism. Those two defines have a different meaning.
I will give you an example:

SDK_OS_FREE_RTOS = 1
LV_USE_OS = LV_OS_NONE

This means that we will use LVGL single thread mode (same as LVGL v8) but PXP will have FreeRTOS support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, so this destinction needs to be handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix later this behavior. If you fix-up the inclusions then you can merge this PR as is.

@FASTSHIFT FASTSHIFT changed the title feat(pxp): Add zephyr support feat(pxp): add zephyr support May 12, 2024
@0xFarahFl
Copy link

Sorry, it might not be directly related to this PR but my question concerns the lvgl_support.h in this PR . In order to test the zephyr support, I had to comment it and add LCD_WIDTH and LCD_HEIGHT macros but I want to know what was the intended content of this file and where to find it?

@nicusorcitu
Copy link
Contributor

LCD_WIDTH and LCD_HEIGHT were used in LVGL v8 just to define a temporary buffer needed by PXP for processing complex operations into multiple steps.
The file "lvgl_support.h" can be find in the SDK releases on official website at NXP.

That buffer has been removed in LVGl v9 so the defines are no longer needed in LVGL v9.

@0xFarahFl
Copy link

Can you provide me with the link please?

@cristian-stoica
Copy link
Contributor

Can you provide me with the link please?

https://github.com/nxp-mcuxpresso/lvgl is the repository for NXP LVGL releases.
They are integrated in the SDKs which you can configure and download at https://www.nxp.com/design/design-center/software/development-software/mcuxpresso-software-and-tools-/mcuxpresso-software-development-kit-sdk:MCUXpresso-SDK

@FASTSHIFT FASTSHIFT merged commit e1cc12e into lvgl:master May 24, 2024
19 checks passed
@nicusorcitu
Copy link
Contributor

Hi @FASTSHIFT
The PR was not yet complete, some behaviors have changed, we need to get back on this one.
Thanks,
Nicu.

FASTSHIFT added a commit that referenced this pull request May 24, 2024
@FASTSHIFT
Copy link
Collaborator

Hi @FASTSHIFT The PR was not yet complete, some behaviors have changed, we need to get back on this one. Thanks, Nicu.

Ah my bad, I revert it #6261

@nicusorcitu
Copy link
Contributor

No worries, we could fix it later.

@@ -25,6 +25,11 @@
#include "semphr.h"
#endif

#if defined(__ZEPHYR__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the above inclusions:

#if defined(SDK_OS_FREE_RTOS)
    #include "FreeRTOS.h"
    #include "semphr.h"
#endif

and replace it with:

#include "../../../osal/lv_os.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

where?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just need the include and then the PR can be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that since the PR was merged once GitHub does not display new changes. I guess a new PR is needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants