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

hid_enumerate doesn't return a null pointer when disconnecting two devices #102

Open
LionelB2 opened this issue Feb 11, 2013 · 14 comments
Open

Comments

@LionelB2
Copy link

When I disconnect two devices at a time and if there is no more devices connected, the function hid_enumerate doesn't return a pointer which is set at NULL.

@signal11
Copy link
Owner

Please give more detail of what it does, what you did, and what you expect it to do.

@LionelB2
Copy link
Author

When I remove two devices one by one, the function returns a null pointer (there is no more devices connected), which is normal. But when I remove two devices at the same time and there is no more devices connected, the function returns a pointer on unsetted structures (path, serial_number... are null). It should return a null pointer as well.

@signal11
Copy link
Owner

you're going to need to provide more detail than this. What version of HIDAPI, did you use the latest from the git repo? What platform?

Can you make a simple test case that demonstrates the problem? What does it do in the test GUI? When do you call hid_enumerate() with respect to when you remove the devices?

@LionelB2
Copy link
Author

The version of hidapi is the 0.7.0 available at https://github.com/signal11/hidapi/downloads.
My OS is windows vista, I develop with visual studio 2005.

In the tests GUI, it says no devices connected as expected.

The function “hid_enumerate” is called in a loop similar to:
while(true)
{
Sleep(1000); //1 sec.
hid_free_enumeration(current_devs);
current_devs = hid_enumerate(0x0, 0x0);
}

The devices are removed during the call of the function Sleep. “current_devs” is not null then.

@signal11
Copy link
Owner

Use the latest from git (ie: git clone ...)

You have to press "re-scan devices" in the GUI.

@LionelB2
Copy link
Author

I have tried the latest version. I still have the same bug. In fact with the GUI, I have the same problem, if I do the re-scan just after I remove the two devices. The information “No device connected” is not displayed, but data without the device name: "1b67:0015 (usage:ff00:0001)", instead of "1b67:0015 Toradex Orientation Sensor (usage:ff00:0001)"
Do you manage to reproduce the bug?

But with the latest version, more curious things happen. When I connect a sensor device, the hid_enumerate recognizes my usb keyboard and usb mouse. But when I disconnect the sensor device, they disappear from the list as well. They are not recognized.

@signal11
Copy link
Owner

If you re-scan again, is the problem cleared? Is the problem that it just takes a few seconds for devices to show disconnected?

I do not have Vista, but don't have this problem.

The keyboard and mouse should not ever show up in there on Windows, at least according to the documentation.

Can you debug into the hid_enumerate() function and find out if the devices are indeed showing up from Windows and see if anything jumps out at you?

@LionelB2
Copy link
Author

If I re-scan again, the problem is cleared. Well yes, it takes a few seconds to show disconnected, but it shouldn’t go through a device half initialized. I can deal with it in my program by checking that even if I have a device detected, the structure is fully initialized .
The problem of the mouse and keyboard wasn’t present in the 0.7.0 version and they don’t appear in the GUI of the latest version. They can only be seen by the list returned by hid-enumerate().
I will take a look if I can find the origin.

@signal11
Copy link
Owner

If I re-scan again, the problem is cleared. Well yes, it takes a few seconds to show disconnected, but it shouldn’t go through a device half initialized. I can deal with it in my program by checking that even if I have a device detected, the structure is fully initialized .

I agree, but this is the data Windows is giving us. Is there a way you can determine in the hidapi code whether the data is valid or not, so that we can filter out the invalid ones?

The problem of the mouse and keyboard wasn’t present in the 0.7.0 version

I remember now that someone sent a patch to make them show up. Apparently it's valid to send feature reports to KB's and mice. This should be in the git version, but not in 0.7.0.

and they don’t appear in the GUI of the latest version. They can only be seen by the list returned by hid-enumerate().

That makes me wonder whether you have the right version of the testgui. The testgui uses hid_enumerate() under the hood so it should be the same. Did you build the testgui from source (including downloading the externals package)?

I will take a look if I can find the origin.

Thanks, I appreciate your looking into this.

Alan.

@LionelB2
Copy link
Author

I agree, but this is the data Windows is giving us. Is there a way you can determine in the hidapi code whether the data is valid or not, so that we can filter out the invalid ones?

This is strange: the pointer cur_dev, which should be null, is not null. So a way to be sure that he is valid is to check one of his parameter as well (line 389 in hid.c):

         if (cur_dev && cur_dev->serial_number) {
              cur_dev->next = tmp;
         }
         else {
              root = tmp;
         }

Instead of :

         if (cur_dev) {
              cur_dev->next = tmp;
         }
         else {
              root = tmp;
         }

Well this is maybe not the nicest solution, but that’s the first one coming to my mind.

That makes me wonder whether you have the right version of the testgui. The testgui uses hid_enumerate() under the hood so it should be the same. Did you build the testgui from source (including downloading the externals package)?

Yes, I hadn’t recompiled the exe. The keyboard and the mouse actually appear in the GUI as well, even if I cannot connect to them. They disappear when I disconnect the toradex device.

I will take a look if I can find the origin.

I’ve found the origin of the problem. Line 331 to 352, it checks if it’s a HIDClass, and the mouse and keyboard are not. So the loop won’t be ended by the “break”, but by the “goto cont;”. That’s why they won’t appear unless there is a HIDClass device.

@signal11
Copy link
Owner

Most devices don't have a serial number, so you can't use the absence of one to say that a device handle is invalid.

I’ve found the origin of the problem. Line 331 to 352, it checks if it’s a HIDClass, and the mouse and keyboard are not. So the loop won’t be ended by the “break”, but by the “goto cont;”. That’s why they won’t appear unless there is a HIDClass device.

Keyboards and mice are HID devices. The loop at 331-352 is executed per_device, looking to see if any of the interfaces of that device have HIDClass bound to them. Having different devices connected in_addition to a KB and mouse shouldn't cause the KB and mouse to show or not show. If this is happening, There's something wrong with the logic. Are the KB and mouse only getting into HIDClass if there is a non-kb/mouse attached?

@LionelB2
Copy link
Author

Most devices don't have a serial number, so you can't use the absence of one to say that a device handle is invalid.

Can you do the test on the manufacturer_string or product_string? They are null as well.

Keyboards and mice are HID devices. The loop at 331-352 is executed per_device, looking to see if any of the interfaces of that device have HIDClass bound to them. Having different devices connected in_addition to a KB and mouse shouldn't cause the KB and mouse to show or not show. If this is happening, There's something wrong with the logic. Are the KB and mouse only getting into HIDClass if there is a non-kb/mouse attached?

Well, when I execute the loop, KB and mouse are not considered as HID devices: the driver_name is not "HID_Class". I know that the loop is executed per device. But as you say, the logic is wrong, because the loop will go to the break instruction if there is a HIDclass device and won’t test the others. As my KB and mouse are not considered as HIDclass device, they won’t be considered unless there is a HIDclass device connected as well.

@signal11
Copy link
Owner

Can you do the test on the manufacturer_string or product_string? They are null as well.

No. Some devices may not have these either (although less likely). You'll probably be unable to test based on anything in the hid_device struct. We'll have to test based on something that comes from Windows which is probably not in the struct.

Well, when I execute the loop, KB and mouse are not considered as HID devices: the driver_name is not "HID_Class".

What is the name of the driver (actually, it's the Setup Class)? (there may be more than one driver per device, that's what the loop on 331-252 is doing, checking each SetupClass for the device).

I know that the loop is executed per device. But as you say, the logic is wrong, because the loop will go to the break instruction if there is a HIDclass device and won’t test the others. As my KB and mouse are not considered as HIDclass device, they won’t be considered unless there is a HIDclass device connected as well.

Let's get the terminology right so we can understand each other better. There's a HID Device (Which is Windows terminology for what is actually a USB Interface on some device which reports to be of the HID class). There can be multiple HID interfaces on a single USB device, but we don't have to worry about that because Windows handles that and gives us a separate HID Device for each. 282 starts the loop for the HID Devices. Each HID Device can have multiple Setup Classes. 331-252 iterates over the setup classes for the device (i is the member index). If one of the SetupClasses is "HIDClass", it breaks out of the loop and uses that device. When it gets to the last setup class (when SetupDiEnumDeviceInfo() returns false), or if it's not able to look up the class name with SetupDiGetDeviceRegistryProperty(... SPDRP_CLASS), it skips that device (goto cont), and continues with the next HID Device.

When you say "if there is a HIDClass device [it] won't test the others," what you mean is that for each device, once if finds a "HIDClass" SetupClass, it won't test the other SetupClasses for that device. This is right and is the proper logic.

I'm wondering whether the "goto cont" on 343 should maybe be a continue. Just because getting the SPDRP_CLASS fails for one member index, doesn't mean we shouldn't check the other member indexes for that device. Try that and see what happens different. I know that when Windows starts unbinding drivers, it's not an atomic operation. Some functions will succeed for a while while the driver is unbinding itself, and others will fail. We need to find what fails immediately so we can test for it. This is what the test for "HIDClass" is for, because some version of Windows will give you a HID device before the HIDClass driver is bound on attachment. Nice, eh?

@signal11
Copy link
Owner

signal11 commented Sep 9, 2013

Any updates?

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

No branches or pull requests

2 participants