-
Notifications
You must be signed in to change notification settings - Fork 139
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
Issue #973: Differentiate multiple USB programmers of the same VID/PI… #1588
Conversation
…e VID/PID (libusb or hidapi or libftdi) Added code for USBasp to check for bus:device match using the -P option. Syntax is -P usb:<bus>:<device> (same as current USBTiny implementation). This also supports serial number check with the alternative -P usb:<serial_number> format (no ':'). In verbose mode, prints out bus/device/serial number for any found USBasps. Only tested on Linux, but it works with HAVE_LIBUSB_1_0 on or off (there's some slightly different code in the two versions of usbOpenDevice()).
Thanks for submitting a PR! I'll add a few tips on how to make the code easier to maintain (and read) and harden it against failure. Once that's been addressed it will need testing. |
@@ -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) { |
There was a problem hiding this comment.
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()
.
src/usbasp.c
Outdated
return 1; | ||
} | ||
return 0; | ||
} else { |
There was a problem hiding this comment.
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.
src/usbasp.c
Outdated
return 0; | ||
} else { | ||
// serial number case | ||
if (!str_eq(serial_num, port)) { |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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));
src/usbasp.c
Outdated
char *colon_pointer = strchr(port, ':'); | ||
if (colon_pointer) { | ||
// Value contains ':' character. Compare with bus/device. | ||
if (strncmp(port, bus, colon_pointer - port)) { |
There was a problem hiding this comment.
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.
src/usbasp.c
Outdated
return 1; | ||
} | ||
port = colon_pointer + 1; | ||
if (strcmp(port, device)) { |
There was a problem hiding this comment.
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);
src/usbasp.c
Outdated
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]; |
There was a problem hiding this comment.
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.
src/usbasp.c
Outdated
libusb_get_string_descriptor_ascii(handle, descriptor.iSerialNumber, (unsigned char*)string, sizeof(string)); | ||
char bus_num[4]; | ||
sprintf(bus_num, "%d", libusb_get_bus_number(dev)); | ||
char dev_addr[4]; |
There was a problem hiding this comment.
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];
Apart from a few niggles, the code looks good. Thank you @MikeRaffa. Don't forget to document the new superpower. |
Thanks for the feedback! I'll try to look into all those items within the next couple of days. |
Question from me: has it been tested on at least two different OSes? It'd be interesting in particular to name some examples in the documentation. |
I've just tested this on Windows using MYSY2 Mingw64. Seems to work correctly. I am getting this error: "avrdude warning: cannot open USB device: Function not implemented", but avrdude is in fact communicating with the USBasp, so I'm not sure what's up with that. |
Change sense of check_for_port_argument_match() to return nonzero when there is a match. Increase size of 'bus_num' and 'dev_addr' from 4 to 21. Various cleanup, formatting changes, and simplifications. Match trailing end of serial numbers.
Working fine under Windows. USBASP unit 1, snL 6666, connected to ATmega32A
|
The PR is also good for Linux. USBASP unit 1, snL 6666, connected to ATmega32A
|
@MikeRaffa Try running avrdude with |
src/usbasp.c
Outdated
return str_eq(port, device); | ||
} | ||
// serial number case | ||
return (*port && str_ends(serial_num, port)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outer parentheses not needed
src/usbasp.c
Outdated
sprintf(bus_num, "%d", libusb_get_bus_number(dev)); | ||
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary braces
src/usbasp.c
Outdated
// -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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary braces
src/usbasp.c
Outdated
@@ -399,6 +399,28 @@ 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) { | |||
|
|||
if (verbose) { |
There was a problem hiding this comment.
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
Remove some unnecessary braces and parens.
This is the output I got with -vvv on Windows. It is correctly reading the signature from the MCU so I'm not sure what to make of that. Maybe something to do with Windows libusb setup? I was getting the same behavior without my changes. Note: this was run with the latest pushed version so the line number (457) reflects that.
|
Interesting. The key seems to be why line 457 of usbasp.c is visited twice... Any idea? |
I was able to get it run cleanly (without that error) by changing the driver to libusbK with the "Zadig" tool.
|
Interesting. Zadig 2.8 comes with libusb0.sys 1.2.7.3 version, can you try that as well? I suspect that you were using libusb0.sys 1.0.26 version. You can also try WinUSB and I believe it should work as well. Or you can use the MSVC build which use avrdude's own avrdude-libusb implementation. MinGW build uses libusb-1.0 (eg: USBASP) or libusb-1.0+libusb-compat-0.1 (other programmers). BTW, latest libusb-win32 version is 1.3.0.1 and you need to use a customized version of Zadig to install it. |
It also ran cleanly with the WinUSB from Zadig 2.8. |
@MikeRaffa |
Worked with that one as well. |
Thanks. Then it should be fine. There are some issues with the older libusb0.sys 1.2.6.0 version with some avrdude programmers. It is great that you can get libusb0.sys 1.2.7.3, libusbK.sys and WinUSB working fine. |
This PR looks good to me based on my test results. |
…D (libusb or hidapi or libftdi)
Added code for USBasp to check for bus:device match using the -P option. Syntax is -P usb:: (same as current USBTiny implementation).
This also supports serial number check with the alternative -P usb:<serial_number> format (no ':').
In verbose mode, prints out bus/device/serial number for any found USBasps.
Only tested on Linux, but it works with HAVE_LIBUSB_1_0 on or off (there's some slightly different code in the two versions of usbOpenDevice()).