Add serial port discovery #1498
Conversation
|
@MCUdude |
|
There are a few warnings when building using MinGW64. Same for my local build. |
|
github action script needs to be added as well to install For example, I need to use I am not so sure how to do that for MSVC build though. Probably @mariusgreuel can help. |
True, but I don't know how/where... @dl8dtl?
Thanks for the heads-up! I've pushed a fix for this.
The question is if we want the CI to build Avrdude with or without libserialport support. Avrdude should work perfectly fine without as well, so it might be a good test to not have libserialport installed? I don't know. @mariusgreuel probably has something to say about this. But apart from this, does the PR work for you @mcuee? |
I will carry out the tests over the weekend. |
I think we can at least build avrdude with libserialport support with Linux, macOS and MinGW where it is easier to add support. BTW, since we only provide official Windows binary using MSVC build where we have to wait for @mariusgreuel or others to help support, the above change will not change status quo. |
|
Started reviewing. There are serious problems with port assignment. Here a patch 0001-Fix-port-assignment:
|
src/main.c
Outdated
| pmsg_error("serial adapter %s with serial number %s not found\n", seradapter, port_tok[1]); | ||
| else | ||
| pmsg_error("serial adapter %s not found\n", seradapter); | ||
| exit(1); |
There was a problem hiding this comment.
Why do we exit here? Say, there is a serialadapter ch340 in avrdude.conf. -P ch340 checks if there is a serial adapter ch340 connected and does not find one. OK, rather then exiting, it could still allow the open() routine to open the file ch340; after all it could be a symbolic link to an existing port for all we know
|
@MCUdude This is a great feature, but there are pitfalls: Say I have a beloved 3d-printer that is plugged into the PC. Unbeknownst to me it uses an AVR board with a |
|
Thank you for the feedback! I've applied the patch you provided and the other suggestions as well. The code is very much "experimental", and is a result of me fighting segfaults and fooling around an entire evening. The functionality is there, but it absolutely needs a critical review and some work.
Yes, that's correct. Ideally, it should loop through the port list to make sure that the user-specified USB to serial adapter is unique, and stop the upload process if there is a chance that the incorrect serial port may be chosen. If you have suggestions on how something like this could be implemented, feel free to submit a patch or explain what you'd do. I'm not available this evening (it's my wife's birthday today), but I will be on Saturday and Sunday evening. |
|
One idea may be dreaming up a general complex use case. For example, a user has 7 boards plugged into the host: two original FT232R with a proper serial number, two cheap clones with identical serial numbers, a direct USART connector, a CH340 serial adapter (which don't have serial numbers) and a new connector CH911 that isn't known to
Here a few suggestions that the code could consider
I don't know how to turn Jörg's other suggestion into code as it only affects direct USB connections ( |
|
This PR seems to work well under Windows. |
|
Another example with Arduino Uno clone with ATmega16U2 USB CDC-ACM serial port. @MCUdude I am not so sure how to specify From libserialport example: |
and print the available options is the serial port isn't unique
|
Thanks for the valuable input @stefanrueger! Now Avrdude requires the port to be unique, and it will print the alternative options if not. The code works but needs another set of eyes to make sure it's as efficient as possible. There are a bunch of for loops in there; maybe a clever mind can simply the code a bit. One thing I didn't figure out is how to properly deal with the It would be great if you could suggest how this could be dealt with dynamically. |
|
Cool... progress!
Can do but then it's a no-no to
The code could first count how many there are and then allocate the space? When you don't need the array deconstruct via |
Fixed.
I need to borrow a big USB hub from work to iron this out. I'm sure it can be done. Thanks for pointing this out.
I had a look at the patch, and I have to admit I have mixed feelings. On one side, I'm sure the remaining issues from my core are gone, but on the other side, I don't think there are that many issues left in the first place. On the other side, the code is very different from my approach, and even though it sure is more clever, I find it more complicated and unfamiliar at first glance (I'm sure this the latter will change). Don't get me wrong, I'm very happy about all the feedback I've gotten while working on the for the last two or three weeks, but it feels a little wistful to merge a patch at the very end that replaces most of the underlying functionality I've spent all this time figuring out myself. I'll spend some time reading through the patch to figure out exactly what's going on, while still having #1500 in the back of my head. |
src/serialadapter.c
Outdated
| else | ||
| msg_warning("-P %s (via %s serial adapter)\n", sp[i].port, *portp); | ||
| if (sp[i].match) { | ||
| if (sp[i].unique && sp[i].sernum[0]) |
There was a problem hiding this comment.
How can sp[i].match and sp[i].unique determine whether ser is ambiguous wrt to the plugged-in devices?
These flags cannot possibly know this as they have never seen ser at all; when they were computed in get_libserialport_data() that function only had the plugged-in devices to look at. match and unique are not useful for the problem of matching a user-specified adapter. This code cannot possibly work. It's the wrong structure.
And it does not work: I have just plugged in four devices, two ft232r with different serial number and two ch340 without serial numbers. Here the classic test avrdude -P dunno output, which is correct on this occasion:
avrdude OS error: cannot open port dunno: No such file or directory
avrdude error: unable to open programmer urclock on port dunno
Possible candidate serial ports are:
-P /dev/ttyUSB3, -P ft232r:MCU8
-P /dev/ttyUSB1 (via ch340 serial adapter)
-P /dev/ttyUSB2, -P ft232r:A600K203
-P /dev/ttyUSB0 (via ch340 serial adapter)
Note that above ports may not necessarily be connected to a target board or an AVR programmer.
Also note there may be other direct serial ports not listed above.
Now I test
$ avrdude -P ch340
avrdude warning: -P ch340 is not unique; consider one of the below
-P /dev/ttyUSB3 or -P ch340:MCU8
-P /dev/ttyUSB1 (via ch340 serial adapter)
-P /dev/ttyUSB2 or -P ch340:A600K203
-P /dev/ttyUSB0 (via ch340 serial adapter)
avrdude OS error: cannot open port ch340: No such file or directory
avrdude error: unable to open programmer urclock on port ch340
Half of the output is wrong on more than one level. And the same happens with
$ avrdude -P ft232r
avrdude warning: -P ft232r is not unique; consider one of the below
-P /dev/ttyUSB3 or -P ft232r:MCU8
-P /dev/ttyUSB1 (via ft232r serial adapter)
-P /dev/ttyUSB2 or -P ft232r:A600K203
-P /dev/ttyUSB0 (via ft232r serial adapter)
avrdude OS error: cannot open port ft232r: No such file or directory
avrdude error: unable to open programmer urclock on port ft232r
The reason for this is that the match, unique and known flags are not needed and their use yields the wrong outputs. The current code needs some fundamental change.
|
I've connected 12 USB to serial devices to test your patch @stefanrueger. The first issue I've encountered is that it seems like it doesn't print "native" ports; ports that are built-in and don't have a VID/PID. I think these should be printed if the user has specified an unknown port. Apart from this, I've not encountered any issues. Ideally, there should have been a lot more "duplicate" Arduino boards, but I was so dumb and connected a 12V port to the USB hub, which ended up frying four boards. So I've ordered 3x ATmega16U2 chips and an ATmega32U4 to fix the broken boards. Luckily, I only fried one target microcontroller, and this was only an ATmega328P in a socket, so that's not a big loss. I've connected the following devices:
Output with all adapters connectedIn the next step I disconnected all adapters except for the FT232R ones:Arduino UNO Wifi Rev2, Arduino UNO, Curiosity Nano and UC-232A connectedSingle CH340 adapter connected |
MCUdude
force-pushed
the
port-discovery
branch
from
6359188
to
7511a5d
Compare
|
@MCUdude Brilliant stress tests! ... and commiseration re the fry-up :-( The tests have uncovered another problem: the order of the ports looks somewhat unknown/unsystematic/weird. How about sorting according to the port string in a way that I have rustled up a patch to sort the list: 0001-Sort-list-of-plugged-in-SERPORTs-according-to-port.zip With #1500 in mind, where you need a list of SERPORTS in the new list that were not in the old list, I have added in above patch a function
Yes, I wondered about this, too. |
Aha, yes. My bad, the following corrects this diff --git a/src/serialadapter.c b/src/serialadapter.c
index 5b649539..e17b7a22 100644
--- a/src/serialadapter.c
+++ b/src/serialadapter.c
@@ -122,7 +122,9 @@ static SERPORT *get_libserialport_data(int *np) {
struct sp_port *p = port_list[i];
char *q;
// Fill sp struct with port information
- if(sp_get_port_usb_vid_pid(p, &sp[j].vid, &sp[j].pid) == SP_OK && (q = sp_get_port_name(p))) {
+ if((q = sp_get_port_name(p))) {
+ if(sp_get_port_usb_vid_pid(p, &sp[j].vid, &sp[j].pid) != SP_OK)
+ sp[j].vid = sp[j].pid = 0;
sp[j].port = cfg_strdup(__func__, q);
sp[j].sernum = cfg_strdup(__func__, (q = sp_get_port_usb_serial(p))? q: "");
j++;If the native ports are printed, indeed, after reverting above maybe we can drop the output |
|
@stefanrueger we're almost there. The VID/PID shouldn't be printed for the BTW I found an FT231X based USB to serial adapter, and it too works like a charm! |
|
Wouldn't this be sufficient? After all, the PID can theoretically be zero, but the VID can't be. diff --git a/src/serialadapter.c b/src/serialadapter.c
index e17b7a22..3ee356a0 100644
--- a/src/serialadapter.c
+++ b/src/serialadapter.c
@@ -251,7 +251,8 @@ static void sa_print_specs(const SERPORT *sp, int n, int i) {
msg_warning(" -P %s", sp[i].port);
for(char **Ps = Pspecs; *Ps; Ps++) {
- msg_warning("%s %s", str_starts(*Ps, "(via ")? "": Ps[1]? ", -P": " or -P", *Ps);
+ if(sp[i].vid != 0)
+ msg_warning("%s %s", str_starts(*Ps, "(via ")? "": Ps[1]? ", -P": " or -P", *Ps);
free(*Ps);
}
msg_warning("\n"); |
I think it's smart to have the text there. Libserialport may not be able to find all ports for some reason. One known issue in MacOS is that liberserialport thinks that CH210x USB to serial adapters are native, and fail to list them. It only occurs on MacOS. See https://sigrok.org/bugzilla/show_bug.cgi?id=1698 for details. I hope it can be fixed soon. I was thinking about asking if there was any progress, but I can't create a bugzilla user; it looks like you'll have to be invited. Anyways, this shouldn't be a showstopper for Avrdude, but it's good to know at least. EDIT: regarding MacOS and the CP210x, it's not all that bad. Libserialport can still find the port, but it can't match the VID/PID, so it's treated like the internal Bluetooth port on my mac: |
How big a problem is this? The current code base will allow this port to be addressed through |
|
@MCUdude I suspect above sorted outputs also use the most recent patch. Could you push the version you have tried for your latest tests? BTW, I realise that this PR is a playground to test out ideas. Feel free to go back to the drawing board and design a new PR (perhaps also with #1500 in mind). But in the end it is this kind of functionality that we currently have that I find very useful. |
It's not a big problem, but it technically isn't correct that a native serial port can be addressed via VID and PID numbers, since it isn't a USB device at all. It's neat that a native port can be mapped, but if the user replaces its RS232 PCI card with something else, the native port (usb:0000:0000) may change.
I don't have any local changes to this PR other than the diff I suggested in a previous post.
Well, I'm happy with the result of this PR, and I'm keen to start working on something else. If you want to implement #1500, even by submitting a patch to this PR, I'm all for it. I have plenty of hardware I can use to test the "1200bps touch" functionality. |
Are you sure? The output of your most recent tests look like they are sorted in the way that my patch to sort the list: 0001-Sort-list-of-plugged-in-SERPORTs-according-to-port.zip patch would do it, but if I clone this PR the sorting isn't part of that (but I feel it should). Also the current PR has not reverted yet to the previous way the SERPORT list would have been created. That too should be done before merging. As to diff --git a/src/serialadapter.c b/src/serialadapter.c
index e17b7a22..ef9396cc 100644
--- a/src/serialadapter.c
+++ b/src/serialadapter.c
@@ -232,7 +232,7 @@ static char **sa_list_specs(const SERPORT *sp, int n, int i) {
}
}
- if(Pi == 0) { // No unique serial adapter, so maybe vid:pid[:sn] works?
+ if(Pi == 0 && sp[i].vid) { // No unique serial adapter, so maybe vid:pid[:sn] works?
if(sa_unique_by_ids(sp[i].vid, sp[i].pid, "", sp, n, i))
Plist[Pi++] = str_sprintf("usb:%04x:%04x", sp[i].vid, sp[i].pid);
else if(*sn && sa_unique_by_ids(sp[i].vid, sp[i].pid, sn, sp, n, i))
OK, how about you, @MCUdude,
And once everyone is happy I merge the resulting PR. Defo not up my road. I don't know what should be done, and cannot test as I don't have the boards. I would (as with everything that I am asked to merge) look very carefully at any PR before merging. |
Oh, sorry! I committed the changes, but I forgot to push them. Should be OK now.
I've done every step in the list, and I'll do a final stress test before I go to bed.
No problem! I can start working on a proof of concept, submit a PR, and then we can take it from there. |
|
Here are some test results. Note that I've patched libserialport in order to work properly with the CP210x chip family. See sigrokproject/libserialport#14 for details. |
|
It occurred to me that it might be nice to have an option For simplicity, I have pushed this commit onto the current PR. I noted that |
Great idea! I'll give it a try later this evening.
ft231x, ft234x, and ft230x have the same PID. The ft232h has a different PID. |
This PR adds serial port discovery functionality using libserialport. Please test and review!
It is also possible to specify a serial number like this:
-P ft232r:A50285BI, using USB VID/PID like so:-P usb:0x0403:0x6002, or using USB VID/PID/SN like so:-P usb:0x0403:0x6002:A50285BIFor some reason, I'm getting a linker error when building using make. Perhaps @dl8dtl knows what causes this?