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.

Sign up for GitHub

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

Jump to bottom

lvgl/contrib: allow for SDL display driver height/width to be adjusted #18463

Merged
merged 1 commit into from
Sep 2, 2022
Merged

lvgl/contrib: allow for SDL display driver height/width to be adjusted #18463

merged 1 commit into from
Sep 2, 2022

Conversation

tvanfossen-bissell
Copy link

Contribution description

When running a RIOT native app using SDL, the display window for SDL is adjustable, but the display driver used by LVGL is not - it always defaults to the 320x240 size provided in the lv_disp_drv_init.

When driving an LCD, the resolution for the disp driver is updated post-init of the driver. Following the similar conventions, this tidbit references LCD_SCREEN_HEIGHT/WIDTH defines, which can be passed to an app as a CFLAG in a Makefile.include, or defaulting to the SDL_HOR/VER_RES defines passed in through lv_drv_conf.h in lv_drivers package.

This enables SDL native applications to better emulate the exact resolution of an LCD screen.

Testing procedure

Using a test package for a custom LVGL RIOT application, I verified that SDL_HOR/VER_RES can adjust the SDL window, but not the display driver used by LVGL when driving an SDL window.

No other defines provided by lv_conf.h, lv_drv_conf.h, or lvgl_riot_conf.h adjust the size of the SDL emulated display driven by LVGL. The simplest fix to me was to adjust the size of the display driver post-init as is done for the non-SDL display driver. This fix enables the emulated display size to be adjusted

Issues/PRs references

Resolves issue #18461

@github-actions github-actions bot added the Area: pkg Area: External package ports label Aug 17, 2022
@tvanfossen-bissell tvanfossen-bissell force-pushed the sdl_allow_resizing_of_lvgl_disp branch from 5e29cee to 6dcf0b6 Compare August 17, 2022 14:43
@benpicco benpicco requested review from aabadie and fjmolinas August 17, 2022 14:50
maribu
maribu approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Code change looks good to me. I sadly have a 64 bit only distro and running SDL from within docker is not trivial, so I cannot easily test this. But I guess I can just trust your testing.

If you agree to drop the addition of the white space at an unrelated function, please squash right in. Otherwise I'd merge as is.

pkg/lvgl/contrib/lvgl.c Outdated Show resolved Hide resolved
@maribu maribu changed the title Sdl allow resizing of lvgl disp lvgl/contrib: allow for SDL display driver height/width to be adjusted Sep 1, 2022
@tvanfossen-bissell
Copy link
Author

ACK. Code change looks good to me. I sadly have a 64 bit only distro and running SDL from within docker is not trivial, so I cannot easily test this. But I guess I can just trust your testing.

If you agree to drop the addition of the white space at an unrelated function, please squash right in. Otherwise I'd merge as is.

Updated, was not intentional

image
with flags:
CFLAGS += -DSDL_HOR_RES=300
CFLAGS += -DSDL_VER_RES=400
CFLAGS += -DLCD_SCREEN_WIDTH=240
CFLAGS += -DLCD_SCREEN_HEIGHT=320

image
with flags
with flags:
CFLAGS += -DSDL_HOR_RES=300
CFLAGS += -DSDL_VER_RES=400
CFLAGS += -DLCD_SCREEN_WIDTH=320 //runs off sdl screen
CFLAGS += -DLCD_SCREEN_HEIGHT=240

@maribu
Copy link
Member

maribu commented Sep 1, 2022 •
edited
Loading

Would you mind squashing the two commits into one?

You can do so e.g. using

git rebase -i HEAD~2

and then replace pick by fixup in front of the Update: ... commit.

(And finally a git push --force-with-lease)

@tvanfossen-bissell tvanfossen-bissell force-pushed the sdl_allow_resizing_of_lvgl_disp branch from 1db919a to 55681ff Compare September 1, 2022 17:15
@tvanfossen-bissell
Copy link
Author

fixed - thanks for the pointer

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 1, 2022
@maribu maribu enabled auto-merge September 1, 2022 18:02
maribu
maribu reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Looks like something went wrong during squashing. You could add the semicolons locally and commit with --amend to directly add this into the current commit. A git push --force-with-lease is again needed to push the changed history into the PR.

pkg/lvgl/contrib/lvgl.c Outdated Show resolved Hide resolved
pkg/lvgl/contrib/lvgl.c Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 2, 2022 12:47

Head branch was pushed to by a user without write access

@tvanfossen-bissell tvanfossen-bissell force-pushed the sdl_allow_resizing_of_lvgl_disp branch from 55681ff to ec27956 Compare September 2, 2022 12:47
@tvanfossen
lvgl/contrib: allow for SDL display driver height/width to be adjusted
4831fd6 
Enables the SDL driver for LVGL to utilize a user prescribed width/height for display resolution when utilizing SDL, or rely on the SDL_HOR/VER_RES provided by lv_drv_conf.h in lv_drivers
@tvanfossen-bissell tvanfossen-bissell force-pushed the sdl_allow_resizing_of_lvgl_disp branch from ec27956 to 4831fd6 Compare September 2, 2022 12:52
@maribu maribu enabled auto-merge September 2, 2022 13:49
@maribu maribu merged commit 04df37c into RIOT-OS:master Sep 2, 2022
@maribu
Copy link
Member

maribu commented Sep 6, 2022

Thx for your contribution and congratulations for your first PR merged :)

I almost forget this during the excitement of the annual RIOT summit

@maribu maribu mentioned this pull request Sep 14, 2022
Closed
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
3 participants
@tvanfossen-bissell @maribu @tvanfossen