serial: Read USB strings for Windows display names [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@rei...@chromium.org — I thought you'd be the best fit based on what I know so far, but let me know if I should assign someone else.
Happy to address anything you'd like changed. See the issue for screenshots; there's also sample Adafruit firmware here:
https://github.com/hjanuschka/usb-serial-tester/blob/main/usb_serial_tester/usb_serial_tester.ino
Related details
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
This change duplicates a significant amount of code from `//services/device/usb`. Given that that code is running in parallel with `//services/device/serial` can you reuse the USB device enumeration it is already going to be doing to get the USB strings you need?
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
This change duplicates a significant amount of code from `//services/device/usb`. Given that that code is running in parallel with `//services/device/serial` can you reuse the USB device enumeration it is already going to be doing to get the USB strings you need?
from what i understand, i see a few options here,
[1] reuse `ParseUsbStringDescriptor` from `usb_descriptors.h` so we at least stop hand-rolling the UTF-16 string parse. Smallest diff, but it only de-dupes a little, the hub open/read and config parsing stay.
[2] extract the hub descriptor-read into a shared helper so we stop duplicating the code. removes most of the duplicated code, but we'd still read the hub twice at runtime.
[3] Consume the strings the USB service already reads via `UsbDeviceManager` so we don't read the hub twice at all, but it moves name enrichment onto an async path, and i am afraid it blows up the complexity (not afraid; just wanted to ask back)
what option do you prefer?
Related details
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaThis change duplicates a significant amount of code from `//services/device/usb`. Given that that code is running in parallel with `//services/device/serial` can you reuse the USB device enumeration it is already going to be doing to get the USB strings you need?
from what i understand, i see a few options here,
[1] reuse `ParseUsbStringDescriptor` from `usb_descriptors.h` so we at least stop hand-rolling the UTF-16 string parse. Smallest diff, but it only de-dupes a little, the hub open/read and config parsing stay.
[2] extract the hub descriptor-read into a shared helper so we stop duplicating the code. removes most of the duplicated code, but we'd still read the hub twice at runtime.
[3] Consume the strings the USB service already reads via `UsbDeviceManager` so we don't read the hub twice at all, but it moves name enrichment onto an async path, and i am afraid it blows up the complexity (not afraid; just wanted to ask back)
what option do you prefer?
Thinking on the complexity of the various options, particularly as there are also threading and object lifetime issues to consider unless you use the `UsbDeviceManager` Mojo interface and make everything even more asynchronous... I think it does make sense to just query the hub directly from this code.
What I ask is that we reuse any descriptor parsing code out of `usb_descriptors.h` so we only have one copy of that code since it is security-sensitive (parsing untrustworthy inputs). If you can deduplicate any of the hub request code that's a bonus but that might be too intertwined with how `UsbDeviceHandleWin` works.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaThis change duplicates a significant amount of code from `//services/device/usb`. Given that that code is running in parallel with `//services/device/serial` can you reuse the USB device enumeration it is already going to be doing to get the USB strings you need?
Reilly Grantfrom what i understand, i see a few options here,
[1] reuse `ParseUsbStringDescriptor` from `usb_descriptors.h` so we at least stop hand-rolling the UTF-16 string parse. Smallest diff, but it only de-dupes a little, the hub open/read and config parsing stay.
[2] extract the hub descriptor-read into a shared helper so we stop duplicating the code. removes most of the duplicated code, but we'd still read the hub twice at runtime.
[3] Consume the strings the USB service already reads via `UsbDeviceManager` so we don't read the hub twice at all, but it moves name enrichment onto an async path, and i am afraid it blows up the complexity (not afraid; just wanted to ask back)
what option do you prefer?
Thinking on the complexity of the various options, particularly as there are also threading and object lifetime issues to consider unless you use the `UsbDeviceManager` Mojo interface and make everything even more asynchronous... I think it does make sense to just query the hub directly from this code.
What I ask is that we reuse any descriptor parsing code out of `usb_descriptors.h` so we only have one copy of that code since it is security-sensitive (parsing untrustworthy inputs). If you can deduplicate any of the hub request code that's a bonus but that might be too intertwined with how `UsbDeviceHandleWin` works.
Done, kept the direct hub read and switched `ReadUsbStringDescriptor()` to reuse `device::ParseUsbStringDescriptor()`
Related details
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |