SInput: Version as a capabilities vehicle

[mitchellcairns]

Name Clarification for Sub-Types

I've re-named sub-type to sub_product. This is to more clearly communicate the intent to have sub_product represent a unique physical device that shares a PID and has may require a unique mapping.

USB Version as a Capabilities Transport

Adopted the approach proposed by @antheas; this PR implements a mixed-radix encode to put more capabilities into 16 bits. (Idea is merged from #13565)

The capabilities value is stored in the appropriate guid bytes reserved for version, so that it may be parsed out from the GUID value. When the device is initialized, the actual version value is stored in the driver context in case it needs to be used at another time.

With this implementation, when a generic type device with a sub_product of 0x1F (maximum for the sub_product), it will use the button mask and other reported capabilities to dynamically build a mapping string. The face style is also used.

Helper functions to decode the capabilities information into an appropriate map sting are included in SDL_gamepad.c to align with other already existing mapping string helper functions.

Co-authored-by: Antheas Kapenekakis [email protected]

[antheas]

Can you give me writes on your fork so I can work on it?

[antheas]

Yesterday, I kindly asked you to start from my PR.

Why did you rewrite it while missing a bunch of features from my PR? I get that open source can be difficult, but this is not a way to collaborate. I spent around 8 hours yesterday working on that PR.

If you do not know how to pull my changes so you can work on them that's fair and we can work on it. But how I see it this version is not particularly better, it's missing features from my version that were important to me.

[AL2009man]

Yesterday, I kindly asked you to start from my PR.

Why did you rewrite it while missing a bunch of features from my PR? I get that open source can be difficult, but this is not a way to collaborate. I spent around 8 hours yesterday working on that PR.

If you do not know how to pull my changes so you can work on them that's fair and we can work on it. But how I see it this version is not particularly better, it's missing features from my version that were important to me.

can you be specific on the current list of features missing? right now: this is a Draft PR, those missing features can be added back in (while adhering to SDL format) no prob.

[antheas]

Yesterday, I kindly asked you to start from my PR.
Why did you rewrite it while missing a bunch of features from my PR? I get that open source can be difficult, but this is not a way to collaborate. I spent around 8 hours yesterday working on that PR.
If you do not know how to pull my changes so you can work on them that's fair and we can work on it. But how I see it this version is not particularly better, it's missing features from my version that were important to me.

can you be specific on the current list of features missing? right now: this is a Draft PR, those missing features can be added back in (while adhering to SDL format) no prob.

E.g., dynamic loading support, analog axis mapping thats correct, a refactor that cleans up feature handling, and centralized handling of how the version bits are computed (it's duplicated here). This PR also has vid pid pairs spread out, where in my PR they are all handled uniformly in the header file.

But I don't need to justify myself. My PR was there first. So from my perspective it was thrown away without being reviewed, extended, or being provided a reason for it. Or perhaps I closed it prematurely.

[mitchellcairns]

First, I’m still fairly new to GitHub’s collaboration workflows. I wasn’t sure how to cleanly branch from your PR even if I had wanted to, but more importantly, I made a deliberate decision to reimplement this from scratch. That wasn’t a dismissal of your work, but a result of needing something that:

• aligned more closely with SDL’s structure and conventions,
• fit cleanly into the SInput protocol driver as I wrote it,
• was manageable for me to maintain and explain, both to Sam and the community

Second, the “my PR was there first” framing is frustrating. I’ve been actively developing this over the course of multiple work-weeks, defining the protocol, testing compatibility, and working to ensure Sam is comfortable with the integration. You joined late in the process, began pushing large, opinionated changes, and didn’t take the time to walk me through the features or make sure that every feature would be permissible.

To then hear “I spent 8 hours on this”, while ignoring the weeks of work, planning, and back-and-forth I’ve already done to get this into shape, is discouraging. It feels like my effort is being dismissed, while I’m being asked to prioritize work I had no opportunity to weigh in on before it was written.

Third, regarding “missing features”. From my perspective, dynamic axis mapping is already implemented, and dynamic capability-based loading is already part of this PR. Features that aren’t present yet are not “missing,” they’re pending review or deliberately scoped out for now.

I’m not opposed to expanding the feature set (within the context of this driver), but it has to be done in a way that’s consistent with SDL’s conventions and sustainable long-term. You’re welcome to suggest clearly defined, incremental additions, ideally with a brief rationale or context, and I’ll consider them carefully. But I won’t be restructuring this PR to match yours wholesale, and I won’t continue responding to emotionally loaded comparisons or attempts to assert yourself on this project as you have been.

[antheas]

First, I’m still fairly new to GitHub’s collaboration workflows. I wasn’t sure how to cleanly branch from your PR even if I had wanted to

This is fine, I can walk you through it 🙂

but more importantly, I made a deliberate decision to reimplement this from scratch. That wasn’t a dismissal of your work, but a result of needing something that:

• aligned more closely with SDL’s structure and conventions,
• fit cleanly into the SInput protocol driver as I wrote it,
• was manageable for me to maintain and explain, both to Sam and the community

We should not get emotional here. I tried my best to do 1 and 2 with my PR. I know 3 is somewhat valid as code that you wrote is more familiar to you, but perhaps this is somewhere we can work on.

My PR has some valid architectural benefits that are important long term, so I think it is better to start with as a foundation. The main one is that all vid/pid handling is done in a single place (the header). It is not done in the hid driver or SDL_gamepad.c. And I think that is very important, as it will be very easy to add new gamepads in the future (in this PR, generic handling is done specifically for the generic vid:pid, which is hardcoded in three places in the code).

There are other minor issues such as treating Misc1 (share) and Misc2 (touchpad2) differently than other misc buttons, which is something I realized while writing the code. Which is why my code only has a single touchpad click and guide click implemented as ButtonStyles. And then it allows for up to 7 misc buttons, which are placed last so that they can be extended to 10+ without affecting existing GUIDs. Perhaps a separate touchpad2 button could be implemented though if it makes sense.

As I said to @slouken I can remove dynamic loading as it was not relevant to that PR (but placing a vid:pid I could use somewhere was important for me to test).

[mitchellcairns]

Thanks for the follow-up. I want to clarify a few things.

I'm open to learning more about GitHub collaboration workflows, and I appreciate any pointers you’re willing to share. That said, Discord might be a more appropriate place to go over those kinds of details so we can keep the PR thread focused on implementation.

Regarding the VID/PID handling, this pull request follows SDL’s current convention, where mapping strings and capability logic are handled within SDL_gamepad.c.

This approach is consistent with how other controllers, like those from 8BitDo and Nintendo, are supported. If there’s interest in centralizing VID/PID declarations in a header file, I’m not opposed to that discussion, but I think it should be considered as a follow-up effort after the foundational work here is in place and working correctly.

The fact that the generic VID/PID shows up in three locations is intentional right now. Each location handles a different part of the device initialization flow. This separation was done on purpose to keep things clear and modular during implementation. Re-architecting that structure would be a separate concern, and one I think we should revisit only after the core functionality is solid.

I also want to acknowledge that your radix-based capability encoding was a great idea. It offered an efficient way to compact information into the version field, and I was glad to incorporate it. You’re credited in the PR as a co-author to reflect that contribution.

Lastly, I understand that you were trying to align your work with the goals of the project. From my perspective, it’s difficult to collaborate when I wasn’t looped in about the full span of the PR plans until after the code was already written. That created a mismatch, and it’s the reason I decided to rewrite rather than try to adapt your code directly.

Let's talk more about any other features you have in mind which are needing implementation or refinement.
I understand the goal of having the handheld daemon supported and it's worth the effort to make it work in a way that benefits gamepad makers and custom software. I think we ultimately want the same thing, more clear discussion and questions will get us there quickly.

[antheas]

Let me break it down in three chunks.

Feature refactor

For me, it is very important for the capability refactor that I had made as a separate PR (you re-implemented and merged without tagging me) go through. Because it is important to validate the axes and buttons are matched to the capability string. It is not currently the case with this PR.

That change also removes having vid:pid hardcoded in joystick init (so hardcoded from 3 to 2 places), as the number of axes is always correct, and makes feature parsing a bit cleaner. With this PR you make too many changes for that to go through (specifically in DeviceDynamicEncodingSetup) and I really do not want to rewrite it.

This PR vs Mine

Having a common capability struct between SDL_gamepad.c and hidapi that both the button masks and mapping string are derived from the version is important to avoid coding errors. The reason it is placed in the header is that it needs to compile when SINPUT is not enabled and it does not belong in SDL_gamepad.c. The way it is implemented in this PR, where the version is implemented in parallel with mutating the bit mask, is very hard to follow for correctness.

I personally do not see deferring having a clean implementation from the get-go as a reason to forgo cleanups like this, as they do not take more time to implement and are not more complex than an alternative (my PR is smaller than this one).

Finally, given that computing dynamic capabilities will always be correct (or be a small superset of capabilities), I do not see a reason there should be a full mapping capability anymore or that it should be reserved for specific vid:pids. Which is why on my version it is the default. Unless there is a technical reason for reducing GUID cardinality.

Overall thoughts

My thoughts are that all of the above is something we should have discussed before you wrote this code, however, in the other PR discussion. I really do not feel like duping that work by foregoing the other PR, and then having to relay the changes to this one.

So I hope you have a look at my PR again and merge the code cleanups from it and the radix format. I'm not motivated to work on further improvements to sinput or adopt it until I see some of that happening. You may also choose to continue my PR as well instead.

If we can't get the basics right then whats the point?

if you want some pointers for checking out my PR:

git remote add antheas https://github.com/antheas/SDL/
git fetch antheas
git checkout sinput-capabilities

After you pull the remote you can also use something like git history to browse it.

Or you can hit . on https://github.com/antheas/SDL/ and select my branch to see the code.

[mitchellcairns]

Having a common capability struct between SDL_gamepad.c and hidapi that both the button masks and mapping string are derived from the version is important to avoid coding errors. The reason it is placed in the header is that it needs to compile when SINPUT is not enabled and it does not belong in SDL_gamepad.c.

That’s exactly what this implementation does. The common capability enums and struct are defined in SDL_hidapi_sinput.h, which is included in both SDL_gamepad.c and the HIDAPI driver. This follows the same pattern used across other SDL HIDAPI drivers, and was styled to match existing conventions:

SDL/src/joystick/hidapi/SDL_hidapi_sinput.h

Lines 34 to 91 in 40a79d3

	typedef enum
	{
	SINPUT_TRIGGERSTYLE_NONE,
	SINPUT_TRIGGERSTYLE_BUMPERS,
	SINPUT_TRIGGERSTYLE_ANALOG,
	SINPUT_TRIGGERSTYLE_DIGITAL,
	SINPUT_TRIGGERSTYLE_MAX,
	} SINPUT_TRIGGER_STYLE_E;
	typedef enum
	{
	SINPUT_PADDLESTYLE_NONE,
	SINPUT_PADDLESTYLE_TWO,
	SINPUT_PADDLESTYLE_FOUR,
	SINPUT_PADDLESTYLE_MAX,
	} SINPUT_PADDLE_STYLE_E;
	typedef enum
	{
	SINPUT_METASTYLE_NONE,
	SINPUT_METASTYLE_GUIDE,
	SINPUT_METASTYLE_GUIDESHARE,
	SINPUT_METASTYLE_MAX,
	} SINPUT_META_STYLE_E;
	typedef enum
	{
	SINPUT_TOUCHSTYLE_NONE,
	SINPUT_TOUCHSTYLE_SINGLE,
	SINPUT_TOUCHSTYLE_DOUBLE,
	SINPUT_TOUCHSTYLE_MAX,
	} SINPUT_TOUCH_STYLE_E;
	typedef enum
	{
	SINPUT_MISCSTYLE_NONE,
	SINPUT_MISCSTYLE_1,
	SINPUT_MISCSTYLE_2,
	SINPUT_MISCSTYLE_3,
	SINPUT_MISCSTYLE_4,
	SINPUT_MISCSTYLE_MAX,
	} SINPUT_MISC_STYLE_E;
	typedef enum
	{
	SINPUT_GENERIC_DEVMAP = 0x00,
	SINPUT_GENERIC_DYNMAP = 0x1F,
	} SDL_SInputGenericDevices_t;
	typedef struct
	{
	Uint16 analog_style;
	Uint16 trigger_style;
	Uint16 paddle_style;
	Uint16 meta_style;
	Uint16 touch_style;
	Uint16 misc_style;
	} SDL_SInputStyles_t;

The way it is implemented in this PR, where the version is implemented in parallel with mutating the bit mask, is very hard to follow for correctness.

If a device is using dynamic capabilities, and we assume minimum expected functions (start, back, ABXY, D-Pad), the goal goes beyond simply mutating a bitmask. It includes validating that all required buttons and axes are both present and mapped according to how the decoded capabilities will be interpreted by SDL. This ensures that the runtime behavior matches expectations, regardless of whether the gamepad uses every feature fully.

So I hope you have a look at my PR again and merge the code cleanups from it and the radix format. I'm not motivated to work on further improvements to sinput or adopt it until I see some of that happening. You may also choose to continue my PR as well instead.

There are several cleanups and restructurings in your PR that are definitely useful. Right now, the goal is to complete full feature coverage and support for all relevant devices. Once that’s in place and working well for everyone, I’m open to revisiting structure and style refinements.

Would you be open to aligning on that order of operations? I’d really like to support your needs for HHD and virtual device testing, and I’m happy to prioritize implementation for anything you need to see working. I just ask for space to stabilize the core driver before moving into deeper architectural changes.

[AL2009man]

My thoughts are that all of the above is something we should have discussed before you wrote this code, however, in the other PR discussion. I really do not feel like duping that work by foregoing the other PR, and then having to relay the changes to this one.

So I hope you have a look at my PR again and merge the code cleanups from it and the radix format. I'm not motivated to work on further improvements to sinput or adopt it until I see some of that happening. You may also choose to continue my PR as well instead.

If we can't get the basics right then whats the point?

ok, i'll need to chime in a bit.

right now: the bigger priority should be getting improved handheld and virtual SInput support first, let's focus on that instead of jumping into conclusion, derailing and refusing to learn from mistakes while learning how to write (and code vibe) on a SDL repository. Once this PR gets merged: we (expect for me) can move onto styles and organization next, in the meantime: i suggest you take a break from SInput work until this PR gets merged.

as a reminder: SDL is a big repository that is used across commercially shipped games, sourceports, input remappers and of course: a entire client that really doesn't rely on a "special sauce". There's rules in place when it comes to structure style. you can't just go ahead and write it "your way" without creating a legacy issue. remember: SInput SDL Driver is a HIDAPI Driver.

save the "emotional" part for when John SDL maintainer refuses to add Motion Sensor support to Steam Virtual Gamepad/Steam Input Legacy mode, ok?

[slouken]

You should keep the trailing comma. Some parsing code relies on it.

[slouken]

This should be added to the commit message, not the copyright.

[antheas]

I should say that I already have a branch that implements what I need from sinput that I am happy with and this one merging would mean that I would need to rewrite it due to the way it is structured.

It also strips my copyright by being remade from scratch. So I will excuse myself from the sinput protocol in the future and let you do as you wish.

Here is the link if you want to reference it:
https://github.com/antheas/SDL/commits/sinput-feature-v2/