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

SDL based display emulation driver #9149

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

vanwinkeljan
Copy link
Member

@vanwinkeljan vanwinkeljan commented Jul 27, 2018

This driver introduces an emulated LCD display for the native POSIX board.
The emulated display driver makes use of SDL2 to render the displays frame
buffer into a dedicated desktop window.

Bellow is a screenshot of the LVGL sample build and running for native_posix board:
sdl_display_driver

@aescolar
Copy link
Member

aescolar commented Jul 27, 2018

Related to #7833 & #6826

ext/lib/gui/Kconfig Outdated Show resolved Hide resolved
Copy link
Collaborator

@SebastianBoe SebastianBoe left a comment

Choose a reason for hiding this comment

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

From a build perspective this is OK.

Except that it disallows using cmake 3.8, but 3.8 is arguably the best version because it is not affected by bug: #8355

So to merge this one must either drop the cmake bump or keep the cmake bump and fix #8355 somehow.

Also, bumping the version requires an email to the mailing list that warns about the dependency change and an update of the getting started docs.

@vanwinkeljan
Copy link
Member Author

@SebastianBoe Note that only the two last commits are the actual pull request, the remaining commits are part of #6826 and #7833. I will take your comments along in #6826.

@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Jul 31, 2018
@pizi-nordic pizi-nordic self-requested a review July 31, 2018 13:49
This was referenced Jul 31, 2018
cmake/app/boilerplate.cmake Outdated Show resolved Hide resolved
include/display.h Outdated Show resolved Hide resolved
include/display.h Outdated Show resolved Hide resolved
include/display.h Outdated Show resolved Hide resolved
drivers/display/display_ili9340.c Outdated Show resolved Hide resolved
ext/lib/gui/lvgl/lv_conf.h Outdated Show resolved Hide resolved
ext/lib/gui/lvgl/lv_conf.h Outdated Show resolved Hide resolved
samples/gui/lvgl/src/main.c Outdated Show resolved Hide resolved
drivers/display/display_sdl.c Outdated Show resolved Hide resolved
samples/gui/lvgl/src/main.c Outdated Show resolved Hide resolved
samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
samples/gui/lvgl/README.rst Outdated Show resolved Hide resolved
@vanwinkeljan
Copy link
Member Author

@aescolar Could review the last commit (native_posix: Added polling for SDL events)?

boards/posix/native_posix/CMakeLists.txt Show resolved Hide resolved
boards/posix/native_posix/hw_models_top.c Outdated Show resolved Hide resolved
boards/posix/native_posix/sdl_model.c Outdated Show resolved Hide resolved
boards/posix/native_posix/sdl_model.c Outdated Show resolved Hide resolved
drivers/display/Kconfig.sdl Show resolved Hide resolved
drivers/display/Kconfig.sdl Outdated Show resolved Hide resolved
drivers/display/Kconfig.sdl Outdated Show resolved Hide resolved
drivers/display/display_sdl.h Outdated Show resolved Hide resolved
drivers/display/Kconfig.sdl Outdated Show resolved Hide resolved
@vanwinkeljan vanwinkeljan force-pushed the sdl_display branch 2 times, most recently from 56bc8c1 to e3352d5 Compare August 18, 2018 20:56
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.

I think it is much better now. this should lead to less overhead, more responsiveness on the GUI part, and better decoupling of future code.

boards/posix/native_posix/hw_models_top.c Outdated Show resolved Hide resolved
boards/posix/native_posix/hw_models_top.c Outdated Show resolved Hide resolved
boards/posix/native_posix/hw_models_top.c Outdated Show resolved Hide resolved
boards/posix/native_posix/hw_models_top.c Outdated Show resolved Hide resolved
boards/posix/native_posix/sdl_event_loop.c Outdated Show resolved Hide resolved
drivers/display/Kconfig.sdl Outdated Show resolved Hide resolved
ext/lib/gui/lvgl/lvgl_mem_kernel.c Outdated Show resolved Hide resolved
@vanwinkeljan vanwinkeljan force-pushed the sdl_display branch 2 times, most recently from fa32576 to ab24fda Compare October 3, 2018 19:15
@vanwinkeljan vanwinkeljan changed the title [DNM][RFC] SDL based display emulation driver SDL based display emulation driver Oct 20, 2018
@vanwinkeljan
Copy link
Member Author

Changes:

@codecov-io
Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #9149 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9149      +/-   ##
==========================================
+ Coverage   48.13%   48.34%   +0.21%     
==========================================
  Files         281      264      -17     
  Lines       43425    42164    -1261     
  Branches    10406    10135     -271     
==========================================
- Hits        20902    20386     -516     
+ Misses      18368    17700     -668     
+ Partials     4155     4078      -77
Impacted Files Coverage Δ
boards/posix/native_posix/hw_models_top.c 92.15% <ø> (ø) ⬆️
drivers/clock_control/nrf5_power_clock.c 40.17% <0%> (-11.93%) ⬇️
lib/cmsis_rtos_v1/cmsis_semaphore.c 60% <0%> (-10%) ⬇️
drivers/timer/nrf_rtc_timer.c 89.13% <0%> (-5.61%) ⬇️
kernel/work_q.c 94.73% <0%> (-5.27%) ⬇️
drivers/ethernet/eth_native_posix.c 23.07% <0%> (-3.71%) ⬇️
lib/fdtable.c 26.08% <0%> (-3.25%) ⬇️
subsys/net/ip/udp.c 61.45% <0%> (-3.13%) ⬇️
include/net/net_if.h 60% <0%> (-2.83%) ⬇️
drivers/net/loopback.c 77.77% <0%> (-2.23%) ⬇️
... and 99 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49466e0...c375313. Read the comment docs.

@vanwinkeljan
Copy link
Member Author

@aescolar Thx for all the review effort!

boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
samples/display/cfb/README.rst Outdated Show resolved Hide resolved
samples/display/cfb/README.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

Docs LGTM, thanks.

@vanwinkeljan
Copy link
Member Author

@SebastianBoe could you revisit you review?

@SebastianBoe
Copy link
Collaborator

SebastianBoe commented Nov 23, 2018

@SebastianBoe could you revisit you review?

I see that the original review comment stated that this would require a bump in the CMake version. Which has not happened. Is this still valid?

@vanwinkeljan
Copy link
Member Author

vanwinkeljan commented Nov 23, 2018

I see that the original review comment stated that this would require a bump in the CMake version. Which has not happened. Is this still valid?

There is no need to up step CMake anymore as west will be used instead and actually the up step was not needed for this PR but for PR #6826.

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

please replace subsys: in your commit messages and put the actual subsys you are changing.

@nashif
Copy link
Member

nashif commented Nov 23, 2018

@aescolar please review

@aescolar
Copy link
Member

aescolar commented Nov 23, 2018

@nashif : I did. This PR is DNM, being blocked by #11351.
Once #11351 is merged those extra commits can be removed from this PR, and then it could be merged.

@aescolar aescolar added the Blocked Blocked by another PR or issue label Nov 23, 2018
@vanwinkeljan
Copy link
Member Author

vanwinkeljan commented Nov 23, 2018

@aescolar Thx was just typing my self that this PR is blocked

@vanwinkeljan
Copy link
Member Author

@nashif replaced subsys with cfb in commit message

@nashif nashif removed the Blocked Blocked by another PR or issue label Dec 10, 2018
@nashif
Copy link
Member

nashif commented Dec 10, 2018

please rebase

This driver introduces an emulated LCD display for the native POSIX
board. The emulated display driver makes use of SDL2 to render the
displays frame buffer into a dedicated desktop window.

Signed-off-by: Jan Van Winkel <[email protected]>
Added support for native posix board to character framebuffer sample.

Signed-off-by: Jan Van Winkel <[email protected]>
@vanwinkeljan vanwinkeljan removed the DNM This PR should not be merged (Do Not Merge) label Dec 10, 2018
@vanwinkeljan
Copy link
Member Author

@nashif done

@nashif nashif merged commit eef1099 into zephyrproject-rtos:master Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants