Hi! Mauro Gamba wrote: > I start the porting of the usb jtag drivers to the libusb-1.0 library.
Awesome. Thank you! > +++ b/configure.ac > @@ -994,7 +994,7 @@ fi > if test $build_ft2232_libftdi = yes ; then > # We assume: the package is preinstalled in the proper place > # these present as 2 libraries.. > - LIBS="$LIBS -lftdi -lusb" > + LIBS="$LIBS -lftdi -lusb-1.0" > # > # Try to build a small program. > AC_MSG_CHECKING([Build & Link with libftdi...]) > @@ -1055,8 +1055,8 @@ build_usb=no > if test $build_jlink = yes -o $build_vsllink = yes -o $build_usbprog = yes > -o \ > $build_rlink = yes -o $build_ulink = yes -o $build_armjtagew = yes > then > - AC_CHECK_HEADERS([usb.h],[], > - [AC_MSG_ERROR([usb.h is required to build some OpenOCD driver(s)])]) > + AC_CHECK_HEADERS([libusb-1.0/libusb.h],[], > + [AC_MSG_ERROR([libusb.h is required to build some OpenOCD driver(s)])]) Please use pkg-config. pkg-config(1) http://linux.die.net/man/1/pkg-config mentions the autoconf macro to use: PKG_CHECK_MODULES(VARIABLE-PREFIX,MODULES[,ACTION-IF-FOUND,[ACTION-IF-NOT-FOUND]]) as well as the added variables that will be available. Suggest use: PKG_CHECK_MODULES([LIBUSB1], [libusb-1.0 >= 1.0.8]) > +++ b/src/Makefile.am > @@ -82,13 +82,13 @@ endif > endif > > if USBPROG > -LIBUSB = -lusb > +LIBUSB = -lusb-1.0 > else > if JLINK > -LIBUSB = -lusb > +LIBUSB = -lusb-1.0 > else > if RLINK > -LIBUSB = -lusb > +LIBUSB = -lusb-1.0 > else These would use LIBUSB1_LIBS instead. You may have to AC_SUBST LIBUSB1_CFLAGS and LIBUSB1_LIBS explicitly, I'm not sure if they will travel over from autoconf to automake automagically. > @@ -1426,7 +1425,7 @@ static struct jlink* jlink_usb_open() > > #if IS_WIN32 == 0 > > - usb_reset(dev); > + libusb_reset_device(devh); Unsure. libusb_reset_device() shouldn't really need to be used in the common case.. (Also, libusb-1.0 works to some degree on Windows.) > @@ -1448,10 +1447,10 @@ static struct jlink* jlink_usb_open() > > #endif > > - /* 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); The interface must always be claimed, but maybe this is done in the common open function now? > @@ -1460,27 +1459,46 @@ static struct jlink* jlink_usb_open() .. > + dev = libusb_get_device(devh); > + struct libusb_config_descriptor *config; > + libusb_get_config_descriptor(dev, 0, &config); > + > + const struct libusb_interface *inter; > + const struct libusb_interface_descriptor *interdesc; > + const struct libusb_endpoint_descriptor *epdesc; > + > + for(int i=0; i<(int)config->bNumInterfaces; i++) > + for(int j=0; j<inter->num_altsetting; j++) > + for(int k=0; k<(int)interdesc->bNumEndpoints; k++) Is this neccessary? Aren't the endpoint addresses always the same? Or does the driver support so many jlink devices with so many different USB interface variants that there's no way to get around it? > +static int wrap_usb_bulk_write(libusb_device_handle *dev, int ep, > char *buff, int size, int timeout) > { > + int transferred; > /* usb_bulk_write() takes const char *buff */ > - return usb_bulk_write(dev, ep, buff, size, timeout); > + libusb_bulk_transfer(dev, ep, (unsigned char *)buff, size, > &transferred, timeout); > + return transferred; > } This should do error checking of the return value of libusb_bulk_transfer(). Better add it right from the beginning, so that it doesn't get overlooked anywhere. > +++ b/src/jtag/drivers/rlink.c .. > @@ -137,13 +138,15 @@ ep1_generic_commandl( > sizeof(usb_buffer) - (usb_buffer_p - usb_buffer) > ); > > - usb_ret = usb_bulk_write( > + usb_ret = libusb_bulk_transfer( > pHDev_param, > USB_EP1OUT_ADDR, > - (char *)usb_buffer, sizeof(usb_buffer), > + (unsigned char *)usb_buffer, sizeof(usb_buffer), > &transferred, > USB_TIMEOUT_MS > ); > > + /* If command succesfull, return the nr of byte transferred */ > + if (usb_ret == LIBUSB_SUCCESS) usb_ret = transferred; > return(usb_ret); > } Note that libusb-0.1 returns errno.h errors, while libusb-1.0 returns libusb.h errors. They are not compatible, so depending on who calls ep1_generic_commandl() and how they check the return value, you may have to adjust that code too. > @@ -194,9 +197,9 @@ ep1_memory_read( > break; > } > > - usb_ret = usb_bulk_read( > + libusb_bulk_transfer( > pHDev, USB_EP1IN_ADDR, > - buffer, length, > + (unsigned char *)buffer, length, &usb_ret, > USB_TIMEOUT_MS > ); Check return value for errors. Everywhere. > @@ -1002,17 +1014,17 @@ void rlink_reset(int trst, int srst) > 1 > ); > if (usb_err < 0) { Error checking is good, but please check for != LIBUSB_ERROR_SUCCESS rather than < 0. > - LOG_ERROR("%s", usb_strerror()); > + LOG_ERROR("Usb Error:%d", usb_err); With next libusb-1.0 release there's libusb_error_name() which is slightly more friendly than the number, but since the release isn't out yet it may be too much of a bleeding edge dependency. > + if (transferred < 1) { > + LOG_ERROR("Usb Error:%d", usb_err); > exit(1); > } Ouch. I would actually focus all effort on this matter instead. Transferring one byte over USB is incredibly inefficient. The overhead is too big. > @@ -1541,22 +1555,28 @@ int rlink_khz( > static > int rlink_init(void) > { .. > const uint16_t vids[] = { USB_IDVENDOR, 0 }; > const uint16_t pids[] = { USB_IDPRODUCT, 0 }; > if (jtag_usb_open(vids, pids, &pHDev) != ERROR_OK) > return ERROR_FAIL; > > - struct usb_device *dev = usb_device(pHDev); > - if (dev->descriptor.bNumConfigurations > 1) > + libusb_device *dev = libusb_get_device(pHDev); > + struct libusb_device_descriptor dev_descriptor; > + struct libusb_config_descriptor *config; > + libusb_get_device_descriptor(dev, &dev_descriptor); > + libusb_get_config_descriptor(dev, 0, &config); > + > + if (dev_descriptor.bNumConfigurations > 1) > { > LOG_ERROR("Whoops! NumConfigurations is not 1, don't know what > to do..."); > return ERROR_FAIL; > } > - if (dev->config->bNumInterfaces > 1) > + if (config->bNumInterfaces > 1) > { > LOG_ERROR("Whoops! NumInterfaces is not 1, don't know what to > do..."); > return ERROR_FAIL; I don't know.. Since the driver only supports one pid+vid I don't think it is meaningful to do all this checking. The interface will never change for that one vidpid. > +++ b/src/jtag/drivers/usb_common.c .. > +static bool jtag_usb_match(struct libusb_device *dev, > const uint16_t vids[], const uint16_t pids[]) Hm, maybe this function should simply be thrown out? > int jtag_usb_open(const uint16_t vids[], const uint16_t pids[], > - struct usb_dev_handle **out) > + libusb_device_handle **out) And this could just loop over libusb_open_device_with_vid_pid() with vid+pid. > +void jtag_usb_close(void) > +{ > + /** Free the device list **/ > + libusb_free_device_list(devs, 1); > + > + libusb_exit(jtag_libusb_context); > +} > + > \ No newline at end of file The last line has trailing whitespace. I didn't look for any whitespace issues in this patch, I assume you'll notice them and fix them before making a patch. > +++ b/src/jtag/drivers/usb_common.h > @@ -22,9 +22,10 @@ > > #include <helper/types.h> > > -#include <usb.h> > +#include <libusb-1.0/libusb.h> Please #include <libusb.h> and rely on pkg-config to provide the path to the libusb-1.0 directory. > +++ b/src/jtag/drivers/usbprog.c .. > struct usbprog_jtag* usbprog_jtag_open(void) > { .. > - usb_set_configuration(dev, 1); > - usb_claim_interface(dev, 0); > - usb_set_altinterface(dev, 0); > + libusb_set_configuration(dev, 1); > + libusb_claim_interface(dev, 0); > + libusb_set_interface_alt_setting(dev, 0, 0); Should look into the device, I don't think all of this is neccessary. I have one, so I can check, but I don't have it with me right now. The claim is probably all that should be done here. > static unsigned char usbprog_jtag_message(struct usbprog_jtag *usbprog_jtag, > char *msg, int msglen) > { > - int res = usb_bulk_write(usbprog_jtag->usb_handle, 3, msg,msglen, 100); > + int res; > + libusb_bulk_transfer(usbprog_jtag->usb_handle, 3, (unsigned char *)msg, > msglen, &res, 100); Check return value. > + libusb_bulk_transfer(usbprog_jtag->usb_handle, 0x82, (unsigned > char *)msg, 2, &res, 100); Check return value. > - while (usb_bulk_read(usbprog_jtag->usb_handle, 0x82, > tmp, 64, 1000) < 1) > + transfered = 0; > + while (transfered < 1) Maybe use do { } while() construct here instead, it makes the code much nicer. > + libusb_bulk_transfer(usbprog_jtag->usb_handle, > 0x82, (unsigned char *)tmp, 64, &transfered, 1000); Better check that libusb_bulk_transfer() will actually set transfered to 0 also in case of errors. I'm not sure if it does. And of course, check return value. You are definately going in the right direction, but you need to also look into trivial things like complete error checking. I urge you to do this immediately, so that you get into the habit of doing it, and so that no places are left behind. While you are touching this code anyway, there may at the same time be potential for significant improvements, both in code clarity and in performance. Please take the opportunity to look into them since you are working on the code anyway. And last, I noticed a few cases of whitespace problems in the patch. It's still very early work in progress, but please please find the habits of writing clean code, and of reviewing your changes yourself before sending them. This way you can catch so many silly issues that it is unneccessary for anyone else to deal with. git in particular makes the whitespace part very easy, when configured to use color outpt it will show trailing whitespace and even some cases of broken whitespace in very clear red color. Running: git config --global diff.color auto git config --global color.ui auto should enable the coloured output on any modern terminal. Thanks again for starting this! I'm really looking forward to your continued work! I'm thinking a little about how to best get your work into OpenOCD. I think the best way is to start with one driver, and send a patch that changes only that one driver over to use libusb-1.0, but leaves the existing ones untouched. This means that you may need to create some duplicated functionality outside the driver, for drivers which use libusb-1.0, but I think this is OK, it will not likely be any significant amount of code, so be fairly quick to create, and not take much place. Once it is in place, you and others can more easily work on moving other drivers to libusb-1.0 in parallell. It's not at all neccessary that you are the only doing that work. //Peter _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
