On Mon, Oct 10, 2011 at 9:34 PM, Mauro Gamba wrote:
> Hello to everyone,
> I start the porting of the usb jtag drivers to the libusb-1.0 library.
> I tested the jlink driver under linux and it work quite well. I've
> ported also the rlink and usbprog driver but I can't test them without
> the hardware. I didn't touch other drivers but I want to submit a
> patch to have feedback and understand if I'm doing the right things.
Hi Mauro,
I have some comments after looking through your patch. I have not
looked at the complete original code, so I might have got some context
wrong. Also some of my comments are general and not directly on your
libusb transition itself.
- /* usb_set_configuration required under win32 */
- struct usb_device *udev = usb_device(dev);
- usb_set_configuration(dev, udev->config[0].bConfigurationValue);
- usb_claim_interface(dev, 0);
+// /* usb_set_configuration required under win32 */
+// struct usb_device *udev = usb_device(dev);
+// usb_set_configuration(dev, udev->config[0].bConfigurationValue);
+// usb_claim_interface(dev, 0);
You have commented out the setting of configuration. Is it not needed
any longer, even on win32? Setting configuration is delicate, and
explained in the Caveats section of the libusb documentation.
Basically, setting the configuration if it is already in the same
configuration causes a soft reset, so you should check it first.
+ dev = libusb_get_device(devh);
+ struct libusb_config_descriptor *config;
+ libusb_get_config_descriptor(dev, 0, &config);
All places where you expect libusb to allocate a buffer, you should
check the return value and bail out on failure, otherwise you risk
overwriting (and finally free'ing) random memory if the buffer was not
allocated and the buffer address is undefined.
And you do remember to free the buffers with
libusb_free_config_descriptor() etc, right?
+ usb_err = libusb_bulk_transfer(
pHDev, USB_EP1IN_ADDR,
- (char *)&bitmap, 1,
+ (unsigned char *)&bitmap, 1, &transferred,
USB_TIMEOUT_MS
);
- if (usb_err < 1) {
- LOG_ERROR("%s", usb_strerror());
+ if (transferred < 1) {
+ LOG_ERROR("Usb Error:%d", usb_err);
exit(1);
}
In this and many similar places you should test the return value
usb_err. If it returns error, transferred can be undefined and for
instance can be 1 or higher. Depending on the situation you might want
to check usb_err != <success-value> || transferred != <expected
length>.
- usb_init();
+ LOG_ERROR("rlink init");
+ //usb_init();
libusb_init() ? Or is it already done?
- usb_find_busses();
- usb_find_devices();
+ int cnt,idx,errCode;
+
+ if (libusb_init(&jtag_libusb_context) < 0)
+ return -ENODEV;
This I am not sure about, but I think you just need to libusb_init()
once at program startup, then at any time, to scan busses you just
call libusb_get_device_list().
+ errCode = libusb_open(devs[idx],out);
+ if (errCode < 0)
+ return errCode;
+ return 0;
}
return -ENODEV;
}
I might be missing some context here, but are you opening the device
and getting the handle in out, then throwing it away? Or is out a
global?
+void jtag_usb_close(void)
+{
+ /** Free the device list **/
+ libusb_free_device_list(devs, 1);
+
+ libusb_exit(jtag_libusb_context);
+}
Looks like devs is another global. I find that globals should be
avoided at almost all cost, even if it is pain to carry the necessary
variables around in every function call.
-#include <usb.h>
+#include <libusb-1.0/libusb.h>
I think it is customary to simply #include <libusb.h> and let the
build tools set up the correct header include path for the compiler.
+ libusb_set_configuration(dev, 1);
+ libusb_claim_interface(dev, 0);
+ libusb_set_interface_alt_setting(dev, 0, 0);
Hard-coded interface numbers? I realize this was the same in the
original code, but I would have used a #define macro. This is specific
to some kind of device, right?
Many places you mix variable declarations and code, probably because
the original code did...
Best regards,
Tormod
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development
