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

LVGL: Add native_posix input driver #22426

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

pavlohamov
Copy link
Contributor

@pavlohamov pavlohamov commented Feb 3, 2020

  1. Implement native_posix input driver (kscan)
  2. Support input in lvgl display sample

@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 3, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pavlohamov pavlohamov changed the title Posix input LVGL: Add Posix input Feb 3, 2020
@nashif nashif changed the title LVGL: Add Posix input LVGL: Add native_posix input driver Feb 3, 2020
Copy link
Member

@vanwinkeljan vanwinkeljan left a comment

Choose a reason for hiding this comment

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

Some minor remarks and I also have a feeling the dts config is a bit redundant in this case

drivers/kscan/Kconfig.sdl Outdated Show resolved Hide resolved
drivers/kscan/Kconfig.sdl Outdated Show resolved Hide resolved
@pavlohamov
Copy link
Contributor Author

pavlohamov commented Feb 11, 2020

Some minor remarks and I also have a feeling the dts config is a bit redundant in this case

Don't know why, but i thought it not gonna work without dts entry. Removed. Now style is consistent with SDL display.

@@ -2,3 +2,8 @@ CONFIG_SDL_DISPLAY=y
CONFIG_SDL_DISPLAY_DEV_NAME="DISPLAY"
CONFIG_SDL_DISPLAY_X_RES=320
CONFIG_SDL_DISPLAY_Y_RES=240

CONFIG_KSCAN=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move it to boards/posix/native_posix/Kconfig.defconfig, it already has DISPLAY configuration there. The same for native_posix_64.conf

Copy link
Member

Choose a reason for hiding this comment

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

@jfischer-phytec-iot You mean CONFIG_KSCAN_SDL if CONFIG_KSCAN?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aescolar Yes similar as it was done for mimxrt boards 7ccea08

Copy link
Contributor Author

@pavlohamov pavlohamov Feb 11, 2020

Choose a reason for hiding this comment

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

@jfischer-phytec-iot Does it make sense to add

config DISPlAY
	default y if LVGL

config KSCAN
	default y if LVGL

as well?

UPD: not relevant anymore

Copy link
Collaborator

@jfischer-no jfischer-no Feb 12, 2020

Choose a reason for hiding this comment

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

I still do not agree to add new options to samples/.../boards/*.conf files. This way it has to be done for every sample that would use display and an input device. And on this architecture is will be always LVGL and CONFIG_LVGL_POINTER_KSCAN, there is nothing else that could be used with it.

Copy link
Member

Choose a reason for hiding this comment

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

@jfischer-phytec-iot

I still do not agree to add new options to samples/.../boards/*.conf files

Actually the real problem here is that the basic LVGL sample has been reused to act as touchscreen sample, the best solution in this case is to split both samples and put KSCAN in the prj.conf.

And on this architecture is will be always LVGL and CONFIG_LVGL_POINTER_KSCAN, there is nothing else that could be used with it.

I think native_posix should enable as least as possible features, e.g. if I would like to test LVGL without touchscreen support I don't want to be forced to disable KSCAN in my project config files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This sample is to simple to have two version, I suggest to move CONFIG_KSCAN=y to prj_kscan.conf

boards/posix/native_posix/Kconfig.defconfig Outdated Show resolved Hide resolved
boards/posix/native_posix/Kconfig.defconfig Outdated Show resolved Hide resolved
drivers/kscan/kscan_sdl.c Outdated Show resolved Hide resolved
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

+1 Thanks. I see no issue with this, but I leave the +2s for @vanwinkeljan or others more involved in the display area

@pavlohamov
Copy link
Contributor Author

rebased on master

@pavlohamov
Copy link
Contributor Author

@franciscomunoz @albertofloyd @scottwcpg any comments?

@aescolar
Copy link
Member

aescolar commented Feb 19, 2020

@pavlohamov Note that the merge window is closed now until the 2.2 release. So by default this PR will be merged when it reopens again

@franciscomunoz
Copy link
Contributor

@pavlohamov This API was intended for laptop keyboards. I see you are doing a mouse.

https://docs.zephyrproject.org/latest/reference/peripherals/kscan.html

This PR doesn't play very well with the intend of the documentation. But I don't have the last word.

@pavlohamov
Copy link
Contributor Author

@pavlohamov This API was intended for laptop keyboards. I see you are doing a mouse.

https://docs.zephyrproject.org/latest/reference/peripherals/kscan.html

This PR doesn't play very well with the intend of the documentation. But I don't have the last word.

Thank you for noticing. I was referencing to #22055
Unfortunate there no "touch" API right now

@MaureenHelm
Copy link
Member

@pavlohamov Note that the merge window is closed now until the 2.2 release. So by default this PR will be merged when it reopens again

The merge window is back open. Will you please rebase to retrigger CI checks?

Pavlo Hamov and others added 2 commits March 11, 2020 09:48
Introduces a new SDL mouse driver for the keyboard scan (kscan)
interface. Driver is implemented as SDL event filter

Signed-off-by: Pavlo Hamov <pavlo_hamov@jabil.com>
Add support of SDL mouse events as input for the LVGL
platforms: native_posix(_64)

Signed-off-by: Pavlo Hamov <pasha.gamov@gmail.com>
@pavlohamov
Copy link
Contributor Author

@pavlohamov Note that the merge window is closed now until the 2.2 release. So by default this PR will be merged when it reopens again

The merge window is back open. Will you please rebase to retrigger CI checks?

Thank you for letting me know. Done

@MaureenHelm MaureenHelm merged commit 44b4371 into zephyrproject-rtos:master Mar 11, 2020
@pavlohamov pavlohamov deleted the posix_input branch February 17, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants