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.

Sign up for GitHub

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

Issue #973: Differentiate multiple USB programmers of the same VID/PI… #1588

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Issue #973: Differentiate multiple USB programmers of the same VID/PI… #1588

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
447b176
Issue #973: Differentiate multiple USB programmers of the same VID/PI…
MikeRaffa Dec 4, 2023
fa2e69d
Updates based on feedback from stefanrueger.
MikeRaffa Dec 4, 2023
e351dc3
Documentation for Issue #973 changes.
MikeRaffa Dec 4, 2023
5dbbb4e
Remove check for 'verbose' on call to pmsg_notice().
MikeRaffa Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 63 additions & 9 deletions src/usbasp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,9 @@ static int usbasp_transmit(const PROGRAMMER *pgm, unsigned char receive,
unsigned char functionid, const unsigned char *send,
unsigned char *buffer, int buffersize);
#ifdef USE_LIBUSB_1_0
static int usbOpenDevice(libusb_device_handle **device, int vendor, const char *vendorName, int product, const char *productName);
static int usbOpenDevice(libusb_device_handle **device, int vendor, const char *vendorName, int product, const char *productName, const char *port);
#else
static int usbOpenDevice(usb_dev_handle **device, int vendor, const char *vendorName, int product, const char *productName);
static int usbOpenDevice(usb_dev_handle **device, int vendor, const char *vendorName, int product, const char *productName, const char *port);
#endif
// interface - prog.
static int usbasp_open(PROGRAMMER *pgm, const char *port);
Expand Down Expand Up @@ -399,6 +399,37 @@ static int usbasp_transmit(const PROGRAMMER *pgm,
return nbytes;
}

static int check_for_port_argument_match(const char *port, char *bus, char *device, char *serial_num) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function returns a 1 if the port arguments are not matched and 0 if they do. So I suggest renaming the function to port_argument_not_matched().


if (verbose) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pmsg_notice() only outputs sth if verbose is one or greater. Hence, the if test is unnecessary

pmsg_notice("usbOpenDevice(): found USBasp, bus:device: %s:%s, serial_number: %s\n", bus, device, serial_num);
}
const size_t usb_len = strlen("usb");
if(str_starts(port, "usb") && ':' == port[usb_len]) {
port += usb_len + 1;
char *colon_pointer = strchr(port, ':');
if (colon_pointer) {
// Value contains ':' character. Compare with bus/device.
if (strncmp(port, bus, colon_pointer - port)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not wrong to use braces here, but it's unnecessary and costs an extra line. I generally prefer code to be crisp, to lessen the need for scrolling for maintenance. Those unnecessary braces appear a few times.

return 1;
}
port = colon_pointer + 1;
if (strcmp(port, device)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 4 lines can be replaced by return !str_eq(port, device);

return 1;
}
return 0;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not wrong, but I am not in favour of elsing a return. It feels wrong.

// serial number case
if (!str_eq(serial_num, port)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These five lines can be replaced by return !str_eq(serial_num, port);

Copy link
Collaborator

@stefanrueger stefanrueger Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and actually, taking Jörg's comment about matching trailing SNs into account, it would be better to simply use

  return !str_ends(serial_num, port);

or if you don't want an empty serial number by the user to match everything

  return !(*port && str_ends(serial_num, port));

return 1;
} else {
return 0;
}
}
}
// Invalid -P option.
return 1;
}

/*
* Try to open USB device with given VID, PID, vendor and product name
Expand All @@ -407,8 +438,8 @@ static int usbasp_transmit(const PROGRAMMER *pgm,
* shared VID/PID
*/
#ifdef USE_LIBUSB_1_0
static int usbOpenDevice(libusb_device_handle **device, int vendor,
const char *vendorName, int product, const char *productName)
static int usbOpenDevice(libusb_device_handle **device, int vendor, const char *vendorName,
int product, const char *productName, const char *port)
{
libusb_device_handle *handle = NULL;
int errorCode = USB_ERROR_NOTFOUND;
Expand Down Expand Up @@ -463,6 +494,19 @@ static int usbOpenDevice(libusb_device_handle **device, int vendor,
if((productName != NULL) && (productName[0] != 0) && !str_eq(string, productName))
errorCode = USB_ERROR_NOTFOUND;
}
if (errorCode == 0) {
if(!str_eq(port, "usb")) {
// -P option given
libusb_get_string_descriptor_ascii(handle, descriptor.iSerialNumber, (unsigned char*)string, sizeof(string));
char bus_num[4];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhhm, bus_num needs to have space to hold the ASCII representation of any integer as we don't control what number range libusb_get_bus_number() returns. Worst case could mean 21 characters are needed.

sprintf(bus_num, "%d", libusb_get_bus_number(dev));
char dev_addr[4];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: I suggest char dev_addr[21];

sprintf(dev_addr, "%d", libusb_get_device_address(dev));
if (check_for_port_argument_match(port, bus_num, dev_addr, string)) {
errorCode = USB_ERROR_NOTFOUND;
}
}
}
if (errorCode == 0)
break;
libusb_close(handle);
Expand All @@ -477,8 +521,8 @@ static int usbOpenDevice(libusb_device_handle **device, int vendor,
return errorCode;
}
#else
static int usbOpenDevice(usb_dev_handle **device, int vendor,
const char *vendorName, int product, const char *productName)
static int usbOpenDevice(usb_dev_handle **device, int vendor, const char *vendorName,
int product, const char *productName, const char *port)
{
struct usb_bus *bus;
struct usb_device *dev;
Expand Down Expand Up @@ -533,6 +577,16 @@ static int didUsbInit = 0;
if((productName != NULL) && (productName[0] != 0) && !str_eq(string, productName))
errorCode = USB_ERROR_NOTFOUND;
}
if (errorCode == 0) {
if(!str_eq(port, "usb")) {
// -P option given
usb_get_string_simple(handle, dev->descriptor.iSerialNumber,
string, sizeof(string));
if (check_for_port_argument_match(port, bus->dirname, dev->filename, string)) {
errorCode = USB_ERROR_NOTFOUND;
}
}
}
if (errorCode == 0)
break;
usb_close(handle);
Expand Down Expand Up @@ -566,14 +620,14 @@ static int usbasp_open(PROGRAMMER *pgm, const char *port) {
pid = USBASP_SHARED_PID;
}
vid = pgm->usbvid? pgm->usbvid: USBASP_SHARED_VID;
if (usbOpenDevice(&PDATA(pgm)->usbhandle, vid, pgm->usbvendor, pid, pgm->usbproduct) != 0) {
if (usbOpenDevice(&PDATA(pgm)->usbhandle, vid, pgm->usbvendor, pid, pgm->usbproduct, port) != 0) {
/* try alternatives */
if(str_eq(pgmid, "usbasp")) {
/* for id usbasp autodetect some variants */
if(str_caseeq(port, "nibobee")) {
pmsg_error("using -C usbasp -P nibobee is deprecated, use -C nibobee instead\n");
if (usbOpenDevice(&PDATA(pgm)->usbhandle, USBASP_NIBOBEE_VID, "www.nicai-systems.com",
USBASP_NIBOBEE_PID, "NIBObee") != 0) {
USBASP_NIBOBEE_PID, "NIBObee", port) != 0) {
pmsg_error("cannot find USB device NIBObee with vid=0x%x pid=0x%x\n",
USBASP_NIBOBEE_VID, USBASP_NIBOBEE_PID);
return -1;
Expand All @@ -582,7 +636,7 @@ static int usbasp_open(PROGRAMMER *pgm, const char *port) {
}
/* check if device with old VID/PID is available */
if (usbOpenDevice(&PDATA(pgm)->usbhandle, USBASP_OLD_VID, "www.fischl.de",
USBASP_OLD_PID, "USBasp") == 0) {
USBASP_OLD_PID, "USBasp", port) == 0) {
/* found USBasp with old IDs */
pmsg_error("found USB device USBasp with old VID/PID; please update firmware of USBasp\n");
return 0;
Expand Down