-
Notifications
You must be signed in to change notification settings - Fork 34
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
Upstream Arduino modifications #23
base: master
Are you sure you want to change the base?
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. |
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.
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: