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

hidapi/windows: fixed PS4 controllers over Bluetooth on Windows 7 #578

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slouken
Copy link
Contributor

@slouken slouken commented May 26, 2023

No description provided.

@slouken slouken force-pushed the 0001-hidapi-windows-fixed-PS4-controllers-over-Bluetooth-.patch branch from a0b7394 to 4f7de87 Compare May 26, 2023 16:27
@mcuee mcuee added the Windows Related to Windows backend label May 27, 2023
static int hid_write_output_report(hid_device *dev, const unsigned char *data, size_t length)
{
BOOL res;
res = HidD_SetOutputReport(dev->device_handle, (void *)data, (ULONG)length);
Copy link
Member

Choose a reason for hiding this comment

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

please see #224
I'm confident we need to do the same thing for HidD_SetOutputReport as well.

Copy link
Member

Choose a reason for hiding this comment

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

@slouken any thoughs/comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

@slouken It seems, you missed to respond to this question

Copy link
Contributor Author

@slouken slouken Mar 10, 2024

Choose a reason for hiding this comment

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

It looks like #224 was just pre-allocating the buffer. Were you referring to another change that made sure the buffer size matched feature_report_length? I think, but I'm not sure, that the feature_report_length is the wrong size in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that the buffer size has to be at least output_report_length bytes in size.
If user passes less data - logically it is not a proble, but WinAPI implementation is known to crash if bufer is too small.
The workaround (as in #224) is to pre-allocate buffer that is large-enough if needed.

@Youw
Copy link
Member

Youw commented May 31, 2023

This looks similar to a problem described in #513.
@DJm00n what do you think?

@mcuee
Copy link
Member

mcuee commented Oct 19, 2023

@DJm00n

Sorry to ping.

Copy link
Contributor

@DJm00n DJm00n left a comment

Choose a reason for hiding this comment

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

Overall looks good

windows/hid.c Outdated Show resolved Hide resolved
windows/hid.c Outdated Show resolved Hide resolved
@slouken slouken force-pushed the 0001-hidapi-windows-fixed-PS4-controllers-over-Bluetooth-.patch branch from 4f7de87 to 4cd8137 Compare November 11, 2023 02:54
@slouken
Copy link
Contributor Author

slouken commented Nov 11, 2023

Okay, I've updated the PR with your suggestions.

windows/hid.c Show resolved Hide resolved
@slouken slouken force-pushed the 0001-hidapi-windows-fixed-PS4-controllers-over-Bluetooth-.patch branch from 4cd8137 to bf1d2ae Compare November 11, 2023 14:49
@mcuee
Copy link
Member

mcuee commented Nov 27, 2024

Now that Windows 7 has been out of support for a while, I am not so sure if we still care about Windows 7 or not, if this issue is only specific to Windows 7.

https://learn.microsoft.com/en-us/lifecycle/products/windows-7

Listing Start Date Mainstream End Date Extended End Date
Windows 7 Oct 22, 2009 Jan 13, 2015 Jan 14, 2020

@mcuee
Copy link
Member

mcuee commented Nov 27, 2024

On the other hand, #513 does not seem to be related to Windows 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Related to Windows backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants