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

macOS: send report id conditionally for hid_get_feature_report #70

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

Youw
Copy link
Member

@Youw Youw commented Sep 2, 2019

same as in hid_send_feature_report, and Windows/Linux

@Youw Youw requested review from todbot and Qbicz September 2, 2019 09:43
mac/hid.c Outdated Show resolved Hide resolved
@Youw
Copy link
Member Author

Youw commented Sep 2, 2019

This will likely reference #60.

@DanielVanNoord, @micolous, please try if this works with your device(s).
It does with mine.

same as in hid_send_feature_report, and Windows/Linux
@Youw Youw force-pushed the macos-feature-report branch from e966f66 to 47b042c Compare September 2, 2019 09:48
@Youw
Copy link
Member Author

Youw commented Sep 21, 2019

@DanielVanNoord, @micolous ping

hidapi team, your opinion/feedback is needed
with all my local tests, this implementation looks best and consistent with other systems

@z3ntu, you have originally propagated #3, so you feedback is appreciated too

@z3ntu
Copy link
Collaborator

z3ntu commented Sep 22, 2019

Apparently the devices I use hidapi with, don't use numbered reports. I'll test this PR tomorrow if it works with my application!

@Youw
Copy link
Member Author

Youw commented Sep 22, 2019

Thanks.
The device I use, does use numbered reports, and #3 did broke my app (as I mentioned earlier, I didn't notice that, since I have my own fork with my own patches).

If this works for you, I think we will accept it this was.

@z3ntu
Copy link
Collaborator

z3ntu commented Sep 23, 2019

Works fine for my use case 👍

@Youw
Copy link
Member Author

Youw commented Sep 23, 2019

@Qbicz, if you don't have comments regarding the implementation, I'd like to merge this ASAP

@DanielVanNoord
Copy link
Contributor

@Youw I'll grab a Mac and test it with my devices that use report ids today. Will let you know the results as soon as I can.

@Qbicz
Copy link
Member

Qbicz commented Sep 23, 2019

@Youw it looks good to me. I don't have a Mac setup so I didn't notice the problem before.

Let's wait for @DanielVanNoord test result.

@Youw
Copy link
Member Author

Youw commented Sep 23, 2019

meanwhile found bug in my own implementation
nothing conceptually changes though

@DanielVanNoord
Copy link
Contributor

Ran some quick tests on a Mac with a device that uses feature report ids and everything seems to be working.

@Youw
Copy link
Member Author

Youw commented Sep 24, 2019

great, we have a winner here

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

Successfully merging this pull request may close these issues.

6 participants