lvgl / lv_drivers

TFT and touch pad drivers for LVGL embedded GUI library
https://docs.lvgl.io/master/porting/index.html
MIT License
291 stars 310 forks source link

fix: hide the SDL2 include from public LVGL functions #227

Closed Viatorus closed 1 year ago

Viatorus commented 2 years ago

With this MR, the required internal functions between sdl.h and sdl_gpu.h are put into a separate header: sdl_common_internal.h.

Why is this needed?

Since this commit: https://github.com/lvgl/lv_drivers/commit/462d9cdeb83df9838f3f02e6f156fc37599c9add the SDL2/SDL.h include is visible to other translation units from "public" LVGL headers.

Since this commit: https://github.com/lvgl/lv_drivers/commit/2a872a6912ca0dfcc7d8e243a5399d8f8c71844c SDL2/SDL.h is only included, if SDL is actually used.

Remaining problem: You still include the heavy SDL2/SDL.h from "public" LVGL headers into other translation units of the user. This wasn't the case before, and isn't the case for other display drivers (e.g. wayland, gtk, win32dr).

This can lead to other problems like one I faced today with our static code analyzer tool (clang-tidy), who tries to parse the included <SDL2/SDL.h> header while using GCC 11.

/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/adxintrin.h:36:10: error: use of undeclared identifier '__builtin_ia32_sbb_u32'; did you mean '__builtin_ia32_subborrow_u32'? [clang-diagnostic-error]
  return __builtin_ia32_sbb_u32 (__CF, __X, __Y, __P);
         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/adxintrin.h:36:10: note: '__builtin_ia32_subborrow_u32' declared here
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/adxintrin.h:61:10: error: use of undeclared identifier '__builtin_ia32_sbb_u64'; did you mean '__builtin_ia32_subborrow_u64'? [clang-diagnostic-error]
  return __builtin_ia32_sbb_u64 (__CF, __X, __Y, __P);
         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/adxintrin.h:61:10: note: '__builtin_ia32_subborrow_u64' declared here
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/cetintrin.h:42:33: error: too few arguments to function call, expected 1, have 0 [clang-diagnostic-error]
  return __builtin_ia32_rdsspq ();
         ~~~~~~~~~~~~~~~~~~~~~  ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/hresetintrin.h:41:3: error: use of undeclared identifier '__builtin_ia32_hreset' [clang-diagnostic-error]
  __builtin_ia32_hreset (__EAX);
  ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/ia32intrin.h:41:10: error: use of undeclared identifier '__builtin_ia32_bsrsi' [clang-diagnostic-error]
  return __builtin_ia32_bsrsi (__X);
         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/ia32intrin.h:112:1: error: definition of builtin function '__rdtsc' [clang-diagnostic-error]
__rdtsc (void)
^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/ia32intrin.h:134:10: error: use of undeclared identifier '__builtin_ia32_rolqi'; did you mean '__builtin_ia32_korqi'? [clang-diagnostic-error]
  return __builtin_ia32_rolqi (__X, __C);
         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/ia32intrin.h:41:10: note: '__builtin_ia32_korqi' declared here
  return __builtin_ia32_bsrsi (__X);
         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/include/ia32intrin.h:142:10: error: use of undeclared identifier '__builtin_ia32_rolhi'; did you mean '__builtin_ia32_korhi'? [clang-diagnostic-error]
  return __builtin_ia32_rolhi (__X, __C);
         ^
Viatorus commented 2 years ago

Tested with SDL and SDL_GPU port. Worked.

Viatorus commented 2 years ago

Would had been great if this would be part of LVGL 8.3 @kisvegabor :/

embeddedt commented 2 years ago

IMO the fix is simple enough that we can merge it to release/v8.3.

EDIT: There is also a minor compilation issue affecting 8.3.0 so perhaps an 8.3.1 release in a few days would be worthwhile to accumulate these fixes.

Viatorus commented 2 years ago

Should I change the target branch to release/8.3?

kisvegabor commented 1 year ago

Sorry for the late reply.

Let's merge it to master and I'll cherry-pick it to release/v8.3

I cherry-picked some fixes in the LVGL repo too, so we can release v8.3.1 next week or so.