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

[SDL2] Adding input and FFB support for Logitech G29(PS3) on hidapi #11591

Open
wants to merge 18 commits into
base: SDL2
Choose a base branch
from

Conversation

Kethen
Copy link

@Kethen Kethen commented Dec 5, 2024

Description

These changes enable the Logitech G29 wheel to run on hidapi with both SDL_Joystick and SDL_Haptic interfaces.

While it is already possible to use the wheel on Linux in WINE + SDL2 thanks to the in-tree evdev driver as well as new-lg4ff, these set of changes allow the G29 to be used with WINE under MacOS and FreeBSD

These wheels should also be supported, but I can only test them from G29's compat modes: G27, G25, DFGT, DFP, DFEX

Haptic and led support are ported from https://github.com/berarma/new-lg4ff so giving @berarma a ping here

Companion test program for testing SDL_Joystick input events, changing wheel compat mode and testing SDL_Haptic calls

https://github.com/Kethen/sdl_lg4ff_util/

Help wanted for this pull request

  • Makefile.os2, Makefile.w32, VisualC project files
  • More testing and code review

Help wanted for future pull requests

  • Support for G923 and other wheels that runs on the same ffb protocol but cannot be emualted by my G29
  • Shifter input, it might already work but I don't own one as of writing so I can't test that

Existing Issue(s)

#6540

@slouken
Copy link
Collaborator

slouken commented Dec 5, 2024

Can you retarget this PR for SDL3?

@Kethen
Copy link
Author

Kethen commented Dec 6, 2024

Can you retarget this PR for SDL3?

#11598

@sezero
Copy link
Contributor

sezero commented Dec 7, 2024

I don't know whether or not this will be accepted into SDL2 branch, but
here is a patch that adds new files to Makefile.os2 / Makefile.w32, and
fixes C89 issues, fixes some warnings, and missing end-of-file issues.
(a few bits taken from SDL3 version. Also note the signedness issue in
HIDAPI_DriverLg4ff_UpdateDevice() is also present in the SDL3 version.)
patch.txt

One issue is SDL_hidapihaptic_lg4ff.c does libc function calls sprintf,
abs and llabs, and especially windows can be built without libc support.
For that, sprintf can be replaced with SDL_snprintf, which I did in the
patch like you did in the SDL3 version. abs() with SDL_abs, but if you
want the compiler to optimize it, you can add a static inline for it in
the lg4ff source. There is no SDL_llabs though: either it can be added
as a new SDL api call, or llabs can be added to lg4ff source as a static
inline helper so that the compiler can optimize it. (Note: the abs and
llabs() issue of directly calling into libc seems to present in the SDL3
version too.)

@Kethen
Copy link
Author

Kethen commented Dec 7, 2024

@sezero Thank you so much for the patch, can I ask for a version with git format-patch so that I can include your work as your commits?

As for whether this will be accepted or not, I'm unsure, and there seems to be a sentiment to use sdl2-compat in some linux distros eg. https://fedoraproject.org/wiki/Changes/SDL2onSDL3

@sezero
Copy link
Contributor

sezero commented Dec 7, 2024

@sezero Thank you so much for the patch, can I ask for a version with git format-patch so that I can include your work as your commits?

There you go: 0001.patch.txt

Note that the abs() and llabs() issue is still there. (SDL_abs() is the worst thing added to SDL api...)
(Maybe you should add abs32() and abs64() static inlines in there and use them instead.)

@berarma
Copy link

berarma commented Dec 8, 2024

Implementing a hardware driver inside SDL is something I would never expect. I'd say it's a bad idea but who knows, maybe I'm old fashioned.

I would have thought porting the driver would be a better idea but maybe it's not possible.

@Kethen
Copy link
Author

Kethen commented Dec 8, 2024

Hey @berarma , thanks for looking at and commenting on this! Please also see #11598 (comment) , as the author of new-lg4ff, you can decide whether code ported from your driver can be part of SDL or not

Implementing a hardware driver inside SDL is something I would never expect. I'd say it's a bad idea but who knows, maybe I'm old fashioned.

I would have thought porting the driver would be a better idea but maybe it's not possible.

On open platforms like FreeBSD, I'd agree, someone well versed in FreeBSD development should be able to develop/port drivers to FreeBSD evdev, yielding a better performing driver, even when a userspace driver might be "good enough"

On other platforms however, it's not always possible

On Android, how the kernel is compiled is up to device manufacturers (and they orphan their kernels way way too often), the Paddleboat API does not have any feedback support beyond vibration motors, there's sure no HAL implemented for more complicated evdev FFB; However it is possible to get raw usb access

On Apple operating systems, they introduced GCRacingWheel in 2022, but it lacks force feedback currently and which wheel they support is entirely up to them; To support custom devices on a per application baisis, they provide IOHIDManager and IOKit

SDL's various hidapi controller implementations can be used in these situations

@slouken
Copy link
Collaborator

slouken commented Dec 8, 2024

Yeah, we'll need official permission from @berarma to relicense code based on their work under the Zlib license for SDL.

@berarma
Copy link

berarma commented Dec 8, 2024

Yeah, we'll need official permission from @berarma to relicense code based on their work under the Zlib license for SDL.

My work is based on the Linux lg4ff module. I added code to implement more effects other than the constant effect and some tweaking features. I'm not sure I'm in a position to decide this.

@Kethen
Copy link
Author

Kethen commented Dec 8, 2024

Then I believe yet another person to ping would be @mungewell, going by the header of hid-lg4ff.c

Kethen and others added 15 commits December 28, 2024 16:33
Current implementation does not include SDL_Haptic integration
for force feedback

Supported devices:
G29, G27, G25, DFGT, DFP

Features:
Joystick button, hat and axis input
5 level rev light control on SDL_JoystickSetLED
raw hid command sending on SDL_JoystickSendEffect

all command sending are based on https://github.com/berarma/new-lg4ff

tested on G29 in various compat modes

shifter input is not tested because I don't have one
If a SDL_Joystick internal lookup manages to define a joystick
type that is not game controller, it should stay as a joystick
- add new files to Makefile.os2 and Makefile.w32
- fix negative return check from SDL_hid_read()
- fix SDL_HIDAPI_HapticDestroyEffect() as it returns void
- fix C89 build issues
- change an sprintf() call to SDL_snprintf()
- fix a -Wuninitialized warning
- add casts Sint32 and float casts to avoid warnings
- add missing newlines to new files
@Kethen
Copy link
Author

Kethen commented Dec 29, 2024

Happy holiday seasons!

  • After reviewing git blame of ported code, ported functions are now individually labeled with more detailed authorship information, license conversion should require permission from @mungewell , @MadCatX and @berarma
  • Added native mode switching routine, but not tested on wheels other than the G29
  • Fixed building for github action available targets
  • Added various other bug fixes

Attaching email exchange with @mungewell , @MadCatX and @slouken here, once again thank you so much @mungewell @MadCatX for being so forth coming with the license conversion:

Permission to port hid-lg4ff to SDL2_3.mbox.txt

Yeah, we'll need official permission from @berarma to relicense code based on their work under the Zlib license for SDL.

My work is based on the Linux lg4ff module. I added code to implement more effects other than the constant effect and some tweaking features. I'm not sure I'm in a position to decide this.

@berarma Your effect rendering code still brings enhancement to the driver, and I'd love to be able to include them in this port. To do so, I'd still need your explicit permission

@sezero
Copy link
Contributor

sezero commented Dec 29, 2024

Here is a minor clean-up patch. (Possibly applies to the SDL3 version too.) Pick from it as you like.

- make _abs,_llabs, get_time_ms, effect_is_periodic, effect_is_condition
  helpers static inline.
- rename _abs and _llabs to abs32 and abs64 to avoid analyzers complain
  about reserved names.
- make the drivers[] array static.
- NULL terminate the drivers[] array to avoid an empty array in case if
  SDL_HAPTIC_HIDAPI_LG4FF isn't configured.
- minor formatting clean-ups here and there.
- remove #endif comments about SDL_JOYSTICK_HIDAPI for readability.
   those #if / #endif blocks are short enough already. 

patch1.txt

@berarma
Copy link

berarma commented Dec 29, 2024

I give permission to re-licence my code in new-lg4ff under the Zlib license.

- make _abs,_llabs, get_time_ms, effect_is_periodic, effect_is_condition
  helpers static inline.
- rename _abs and _llabs to abs32 and abs64 to avoid analyzers complain
  about reserved names.
- make the drivers[] array static.
- NULL terminate the drivers[] array to avoid an empty array in case if
  SDL_HAPTIC_HIDAPI_LG4FF isn't configured.
- minor formatting clean-ups here and there.
- remove #endif comments about SDL_JOYSTICK_HIDAPI for readability.
   those #if / #endif blocks are short enough already.
@Kethen
Copy link
Author

Kethen commented Dec 29, 2024

Here is a minor clean-up patch. (Possibly applies to the SDL3 version too.) Pick from it as you like.

- make _abs,_llabs, get_time_ms, effect_is_periodic, effect_is_condition
  helpers static inline.
- rename _abs and _llabs to abs32 and abs64 to avoid analyzers complain
  about reserved names.
- make the drivers[] array static.
- NULL terminate the drivers[] array to avoid an empty array in case if
  SDL_HAPTIC_HIDAPI_LG4FF isn't configured.
- minor formatting clean-ups here and there.
- remove #endif comments about SDL_JOYSTICK_HIDAPI for readability.
   those #if / #endif blocks are short enough already. 

patch1.txt

Thanks for the patch!

I give permission to re-licence my code in new-lg4ff under the Zlib license.

Thank you!

@slouken
Copy link
Collaborator

slouken commented Jan 15, 2025

Hi @Kethen, it sounds like you have permission from the authors to use this code. Great job!

Can you please note that on the SDL3 PR and link to the relevant comments in this pull request?

We're getting close to 3.2.0 release, so I'll bump your SDL3 PR to the 3.4.0 milestone where it is likely to be accepted.

@slouken slouken added this to the 2.x milestone Jan 15, 2025
@berarma
Copy link

berarma commented Jan 15, 2025

I see that our names don't appear in the headings of the files where our code is used, only above the corresponding functions. Is that correct?

@slouken
Copy link
Collaborator

slouken commented Jan 15, 2025

@Kethen, the convention is to add copyright holders to the license at the top, e.g.

Copyright (C) 1997-2025 Sam Lantinga <slouken@libsdl.org>
Copyright (C) 2024 Wim Taymans <wtaymans@redhat.com>

@Kethen
Copy link
Author

Kethen commented Jan 15, 2025

@Kethen, the convention is to add copyright holders to the license at the top, e.g.

Copyright (C) 1997-2025 Sam Lantinga <slouken@libsdl.org>
Copyright (C) 2024 Wim Taymans <wtaymans@redhat.com>

Hello @slouken, currently ported functions are annotated this way

/*
*Ported*
Original functions by:
Simon Wood <simon@mungewell.org>
Michal Malý <madcatxster@devoid-pointer.net> <madcatxster@gmail.com>
lg4ff_set_autocenter_default lg4ff_set_autocenter_ffex
`git blame v6.12 drivers/hid/hid-lg4ff.c`, https://github.com/torvalds/linux.git
*/
static int SDL_HIDAPI_HapticDriverLg4ff_SetAutocenter(SDL_HIDAPI_HapticDevice *device, int autocenter)

What would be appropriate to add to the header of files in this PR (and the SDL3 PR) containing ported code?

@slouken
Copy link
Collaborator

slouken commented Jan 15, 2025

Hello @slouken, currently ported functions are annotated this way

/*
*Ported*
Original functions by:
Simon Wood <simon@mungewell.org>
Michal Malý <madcatxster@devoid-pointer.net> <madcatxster@gmail.com>
lg4ff_set_autocenter_default lg4ff_set_autocenter_ffex
`git blame v6.12 drivers/hid/hid-lg4ff.c`, https://github.com/torvalds/linux.git
*/
static int SDL_HIDAPI_HapticDriverLg4ff_SetAutocenter(SDL_HIDAPI_HapticDevice *device, int autocenter)

What would be appropriate to add to the header of files in this PR (and the SDL3 PR) containing ported code?

Maybe you can ask the contributors how they would like to be credited?

@berarma
Copy link

berarma commented Jan 15, 2025

@Kethen, the convention is to add copyright holders to the license at the top, e.g.

Copyright (C) 1997-2025 Sam Lantinga <slouken@libsdl.org>
Copyright (C) 2024 Wim Taymans <wtaymans@redhat.com>

This is what I expected. In my very basic understanding of copyright law, in the current form, it looks as if we had transferred copyrights. Sincerely, I don't know how important this might be, but is there any issue in following the convention?

@slouken
Copy link
Collaborator

slouken commented Jan 15, 2025

@Kethen, the convention is to add copyright holders to the license at the top, e.g.

Copyright (C) 1997-2025 Sam Lantinga <slouken@libsdl.org>
Copyright (C) 2024 Wim Taymans <wtaymans@redhat.com>

This is what I expected. In my very basic understanding of copyright law, in the current form, it looks as if we had transferred copyrights. Sincerely, I don't know how important this might be, but is there any issue on following the convention?

No, no issue at all.

@Kethen
Copy link
Author

Kethen commented Jan 16, 2025

@Kethen, the convention is to add copyright holders to the license at the top, e.g.

Copyright (C) 1997-2025 Sam Lantinga <slouken@libsdl.org>
Copyright (C) 2024 Wim Taymans <wtaymans@redhat.com>

This is what I expected. In my very basic understanding of copyright law, in the current form, it looks as if we had transferred copyrights. Sincerely, I don't know how important this might be, but is there any issue on following the convention?

No, no issue at all.

@slouken In that case, copyrights of authors of ported code are now added to license headers

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.

4 participants