-
Notifications
You must be signed in to change notification settings - Fork 616
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
nsyshid: Fix SetProtocol and SetReport for Libusb Backend #1027
Conversation
e4e0f41
to
7d9b45d
Compare
7d9b45d
to
2da26bb
Compare
@ssievert42 fyi - this has worked on my Mac for the set protocol and set report methods, needed to release all interfaces, set the specified configuration then claim the interfaces for the specified configuration again in the set protocol, then in set report control transfers work :) |
Neat! |
2da26bb
to
a3f27ed
Compare
a3f27ed
to
43e7723
Compare
if (releaseSuccess < LIBUSB_SUCCESS) | ||
{ | ||
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): Failed to release interface %i", i); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning here is problematic: We still need to tell libusb to free the struct that conf
points to!
Since you are only using conf->bNumInterfaces
from that struct, how about calling libusb_get_active_config_descriptor()
, making a copy of conf->bNumInterfaces
and then immediately calling libusb_free_config_descriptor()
?
const int setConfig = libusb_set_configuration(handleLock->GetHandle(), protocol); | ||
if (setConfig == LIBUSB_SUCCESS) | ||
{ | ||
getConfig = libusb_get_active_config_descriptor(dev, &conf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (in its current form) causes us to lose all references to the struct that conf
previously pointed to.
Just made a small test program, to confirm that I am not talking utter nonsense, and valgrind agrees with me that the following snippet leaks:
struct libusb_config_descriptor *conf = nullptr;
libusb_get_active_config_descriptor(dev, &conf);
libusb_get_active_config_descriptor(dev, &conf);
libusb_free_config_descriptor(conf);
Unfortunately having to abandon this PR - I have become extremely busy due to personal reasons, but if someone wanted to pick this work up in future and get these methods working for libusb hopefully this can be a starting point. |
No problem and the effort is appreciated regardless. I recently got more USB portals so that I can help with this myself. Will see if I can pick this up. |
What is the current state with skylander portal on linux? |
@Caspersonn I don't think the methods have been created yet to handle all skylander portal commands on linux yet so the games won't work just yet |
Ooh that sucks, is there any way i could help? |
If you have dev experience I would try and have a go at trying to figure out the structure of libusb_control_transfer methods to send commands to the portal :) |
Any updates? |
The initial implementation of the libusb backend didn't implement SetProtocol and SetReport, and these methods are important for the Skylanders Games as the portal device uses these methods to receive commands from the game, and then respond via the HIDRead methods.
I have tested this as working on my Mac, with Giants and Swap Force. Will also require testing via linux. At the moment I have hard coded the values in the libusb_control_transfer method, but I will attempt to find the correct enum values for this.