Fix pid/vid getting in hid_enumerate() for Bluetooth devices #62
Conversation
|
One remark: Right now, there's no checking for errors (e.g. when the sysfs path would not end in /hidraw/hidraw). One assumption is that all sysfs paths would be looking good when returned from udev_list_entry_get_name(). If this assumption isn't safe in your opinion, I'll be happy to add more error checking to get_pid_vid_from_sysfs_path(). |
|
Hi Thomas, I like the idea but I wonder if there's a better way. Unfortunately I currently can't make my Bluetooth trackpad work with any of my Linux boxes to try anything out. My bluetooth used to work :( .... Anyway, my idea would be to instead of doing string manipulations on the path, to maybe instead call udev_device_get_parent_with_subsystem_devtype() with subsystem of "hid" and devtype NULL[1] to get a handle to the HID device, then calling udev_device_get_sysname() to get the string with the VID/PID in it. I suppose I can try this out here on USB and see if it works and send you code to try on Bluetooth since I've all but given up on mine. Not sure what it is.... Maybe my new kernel with old userland, but it doesn't work on my other computer with Ubuntu 12.04. Strange. |
|
Your patch removes the initialization for dev, which is used by the calls to copy_udev_string(). Does this not crash for you? |
|
Also, based on what your udev/sysfs looks like, is there a good way to get the serial, manufacturer, and product strings from anywhere else in the sysfs tree for a bluetooth device? |
|
I finally got my BT device to associate (on a different computer still.....) It looks like there's no support for manufacturer and product strings currently in sysfs, but there is a hardware address. Perhaps we could make use of that somehow, maybe stick it in the serial number field or something. What do you think? |
|
In your example directory name: The 0005 means bluetooth. 0003 would mean USB. So maybe we call udev_device_get_parent_with_subsystem_devtype() with "hid" and NULL, parse the bus type from the name, and then if USB (3), do what we do now. If not (5), then parse just the VID/PID like you suggested. |
|
Hi Alan, thanks for your feedback! I've read up on udev a bit, and played with the pyudev bindings and with getting the "hid" parent as you suggested. I've come up with the following test program: import pyudev
context = pyudev.Context()
for device in context.list_devices(subsystem='hidraw'):
print 'device.sys_path =', device.sys_path
print 'device.device_node =', device.device_node
print 'device uevent attributes:'
print device.attributes['uevent']
print '-'*10
parent = device.find_parent(subsystem='hid')
print 'parent.sys_path =', parent.sys_path
print 'parent uevent attributes:'
print parent.attributes['uevent']
print '\n'Running this program on my computer gives the following result (hidraw3 = a Move controller connected via USB, hidraw6 = a Move controller connected via Bluetooth): I'm not sure if the "uevent" attribute is always present there (can you think of a situation where it is not?). If it is always present, I'd suggest parsing out the PID/VID from the HID_ID= attribute there. Also, the HID_UNIQ= attribute contains the Bluetooth address, which should ideally be put into the serial number field (IIRC the hidapi also gives the Bluetooth address as serial number on Mac OS X). I could update my patch to use the parent "hid" object and parse the serial and VID/PID out of the uevent attribute. I can also parse the bus type and use the current code for USB devices - would that work for you? |
|
Updated patch. Things to do:
|
|
I've just rebooted into OS X and checked it out (for two different controllers, one connected via USB, one via Bluetooth):
Should I special-case the hid_get_manufacturer_string(), hid_get_product_string() and hid_get_serial_number_string() functions and look up the serial number and product ID with the new method ("hid" subsystem parent and "uevent" file) and return "Unknown" (or the empty string) for the manufacturer if the device is a Bluetooth device? If it is a USB device, we would simply use the existing method to get the strings. |
|
Hi Thomas, Good work. This is definitely in the right direction. Some comments:
parse_uevent_info()
if (device_type == 3 /* 3=USB */) {
//do all the USB stuff
}
else if (device_type == 5 /* 5=Bluetooth */) {
// do all the Bluetooth stuff
}
Thanks for taking a look at this. I think we're really close to having a good solution here. Alan. |
|
Hi Alan, I've now fixed the points you mentioned, and make the code cleaner:
Still, for some parts I need your input:
I still have to implement the get_device_string part that we talked about above. I plan to make parse_uevent_info() more generic, so both functions can use its features, as you suggested. Thanks for taking the time to review the patch and the feedback so far :) |
|
Gar, I didn't realize comments on the commit would show up in this thread. Oh well, they also show up on the commit, so that you have context. I think at this point, if we can't get the VID/PID from the uevent, we just fail for that device. I'm not sure how far we should be expected to go in order to insulate against changes we can't predict. Thomas, thanks for your hard work on this. It is appreciated and I think we're getting close to having it done. |
|
Updated merge request with your feedback. Please have a look. Right now, I don't have any Bluetooth device with me (will be back on the weekend and have access to some devices then), and I even though I ran Valgrind on it, I wasn't able to understand its output, so it would be great if you could double-check the free()s. Thanks :) |
|
I've updated and tested the patch with my PS Move API library (which does enumeration of HID devices). Valgrind shows the following output: |
|
ok, I think those two errors are bugs in wcscmp: |
|
Change it so that hid_init() and hid_exit() don't move. This will make the commit smaller and cleaner. Why does Add a comment to In Also in this function, I can see why you need to do the hard-coded string match for "manufacturer", etc, but since that string is now in two places, let's make it a #define up at the top and use it in |
|
The moving of the functions was accidental, it's fixed now. The I've also added the comment about free'ing the result of the I also reworked For the string-getting functions, I've now introduced a new enum ( |
|
I made a couple of comments about one more thing I'd like to change. Other than that I think we're there. Good work. |
|
I've made the changes. One thing I've been wondering: The |
|
Lowercase is fine with me. |
|
Ok then :) So, anything left to do or can this go in? |
|
Whoops - I just found two stray "printf"s that should not be there obviously. Will submit an updated patch in just a minute. |
|
I made a couple changes, checkout the branch named thp_bluetooth and let me know if you agree with the changes and if it works for your device. It seems to work here for my USB device. If you're good with it, I'm ready to squash the commits and push. btw: that branch is based on before you took the printf()s out. I diffed the diffs, and that was indeed the only change between your last two revisions. |
|
Thanks for the update. In the I've tested it here and it was able to successfully detect a HID device connected via USB and also via Bluetooth, so from my perspective, this can go in. |
|
ok looks good. Good catch on the other mbstowcs(). |
|
Ok, so I really screwed up here. I inadvertently pushed the change last night with the printf() stuff in it. Since it's really bad form to rebase stuff that's been upstream, I pushed my commit from yesterday and yours from today on top of it. It really hacks me off that I screwed this up, since I put so much effort into trying to get this to go in as one clean commit. Oh well. It's done now so, so we move on. pushed: Thanks for doing this, and thanks for your patience. |
Searching for the USB host and extracting the vendor ID and product
ID from there only works for USB-based HID devices. In the case of
HID devices connected via Bluetooth, we would get the VID/PID of the
Bluetooth host adapter (if it's a USB device).
To work around this, we don't use the USB VID/PID, but extract the
information directly from the sysfs path - this will return the right
VID/PID for Bluetooth devices and USB devices.
Tested on Linux 3.2.0 for both USB and Bluetooth devices.