Upstream Arduino modifications #23
Conversation
This allows users to supply symlinks created e.g. by udev rules instead of the actual device names.
The first one would be included for Windows too which seems to be an error. The second one is obviously redundant.
strrchr() can return NULL if the character is not found. Since "composite" is true in some special cases with only one level of nesting, the math operation was failing badly. This should solve a couple of bugs about Arduino IDE crashing due to libListSerialJ
|
Thanks Martino, we'll have a look. |
|
|
||
| return utf8_str; | ||
| static char* wc_to_utf8(const wchar_t* wc, ULONG size) { | ||
| int ulen = WideCharToMultiByte(CP_UTF8, 0, wc, -1, NULL, 0, NULL, NULL); |
There was a problem hiding this comment.
What's happened to the size parameter?
Is the input to this function actually always null terminated? The previous code didn't assume so. Should you be passing size instead of -1 to the third argument (cchWideChar) of WideCharToMultiByte?
If the input is always null terminated, we should just make wc_to_utf8 take a single parameter.
There was a problem hiding this comment.
USB 2.0 spec 9.6.7: "The UNICODE string descriptor (shown in Table 9-16) is not NULL-terminated."
Also, code in a couple of other programs assumes no null terminator, so it might be wise to assume that there isn't one - https://android.googlesource.com/platform/development/+/8267511c96e3226e45a0be773ee442b66261824d/host/windows/usb/api/adb_winusb_interface.cpp#215
Documentation for cchWideChar:
"If this parameter is -1, the function processes the entire input string, including the terminating null character. "
"If this parameter is set to a positive integer ... If the provided size does not include a terminating null character, the resulting character string is not null-terminated, and the returned length does not include this character."
So either null terminate the input; or pass cchWideChar=size/2 and then null terminate the UTF8 string afterwards (the second of which is what this changed code is doing, except for the passing cchWideChar part).
Incidentally, that sentence in the spec about null terminating is followed by an interesting one which indicates another bug in libserialport:
"The string length is computed by subtracting two from the value of the first byte of the descriptor" - bLength is the size of the descriptor not just the string. So the size being passed from get_string_descriptor() to wc_to_utf8() is slightly too large. (I've tested this by printing out bLength and the resulting string, and bLength is indeed 2 bytes too large.)
| port->usb_path[pch-port->usb_path] = '\0'; | ||
| if (pch != NULL) { | ||
| port->usb_path[pch-port->usb_path] = '\0'; | ||
| } |
There was a problem hiding this comment.
I think this fix should just be squashed into your commit 289ef21, rather than introducing the bug and then fixing it in a separate commit.
| @@ -56,7 +56,7 @@ SP_PRIV enum sp_return get_port_details(struct sp_port *port) | |||
| result = CFStringGetCString(cf_property, path, sizeof(path), | |||
| kCFStringEncodingASCII); | |||
| CFRelease(cf_property); | |||
| if (!result || strcmp(path, port->name)) { | |||
| if (!result || strcmp(path, port->name) || strstr(path, "wwan")) { | |||
There was a problem hiding this comment.
Is this something that should be going upstream? A wwan device (presumably some 3G/4G modem) isn't going to be an Arduino, but it's a valid serial device that other software might want to talk to, no?
| snprintf(description, sizeof(description), | ||
| "%s - %s", port->description, port->usb_serial); | ||
| "%s - %s", port->description, interface); |
There was a problem hiding this comment.
I'm OK with adding the interface to the description but I think we should keep the serial number in there if it's present.
| if (port->composite) { | ||
| //remove last part of the path | ||
| char * pch; | ||
| pch=strrchr(port->usb_path,'.'); |
There was a problem hiding this comment.
As a minor detail, you may want to better follow the code style by using the following spacing:
char *pch;
pch = strrchr(port->usb_path, '.');
| p += strlen(p) + 1; | ||
| } | ||
| if (*p) | ||
| continue; |
There was a problem hiding this comment.
I doubt that removing this continue is correct. I think it was actually useful.
|
@facchinm, any update on this? It would be good if you could respond to the concerns raised in the comments. |
This PR contains all the patches currently in production at Arduino except: