fix(app-shell,usb-bridge): improve flex usb communication #14170
Conversation
Desktop OS serial drivers often have very small incoming data buffers on their serial connections because well serial is typically slow... unless you've actually got usb on both sides. The default value of 2048/.01s (204.8kB/s) should be further developed but it's going to be a good idea to keep this smaller than we think necessary to support slow client hardware.
There's a bunch of weird stuff in here that we found via too much logging. May our loss be your gain. Co-Authored-By: Jamey Huffnagle <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.1.0 #14170 +/- ##
=======================================================
+ Coverage 70.45% 70.47% +0.01%
=======================================================
Files 2512 2512
Lines 71217 71225 +8
Branches 8971 8974 +3
=======================================================
+ Hits 50177 50194 +17
+ Misses 18844 18835 -9
Partials 2196 2196
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Does the host not have a proper flow control mechanism to apply backpressure to the Flex? Some mechanism must exist at the USB protocol level, right? Are we accidentally subverting it because we're layering a virtual serial port (?) atop raw USB? Solving this by metering bandwidth seems dicey. Like, it will depend on the load that the host device is under, right? |
You know, that's a pretty good question, since flow control mechanisms do in fact exist for both actual rs-232 and usb cdc-acm, and we're just not using them. I think it's worth taking a pass at that. |
| if (urls === null) { | ||
| log.warn('No release files in manifest', { | ||
| version: CURRENT_VERSION, | ||
| manifest, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
is this to cut down on noise? seems like a useful warning since we expect there to be urls in the release set
There was a problem hiding this comment.
yeah the noise is just brutal
| keepAliveMsecs: 10000, | ||
| keepAliveMsecs: Infinity, |
| data: response.data, | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
| usbLog.debug(`usbListener error ${e?.message ?? 'unknown'}`) | ||
| console.log(`axios request error ${e?.message ?? 'unknown'}`) |
There was a problem hiding this comment.
was this changed cuz reading logs from the usb logger was annoying? cuz it seems like thats the right place to report logs here
There was a problem hiding this comment.
yeah how does one get logs out of the usb logger?
Closes RSS-420, RQA-2053, RQA-1615
This branch has fixes for a couple of the underlying issues with flex usb communication.
First, we need to limit the flex->host directional bandwidth (which we do by limiting the size of the USB packets we send on the python side) because if we send too much we will blow through the host-side serial buffers in their drivers, which are very variable. When we do that, we lose data which is part of an HTTP response; and the node http parser can't really handle that and just hangs. Even if it didn't and found the error, we need that error not to happen.
Second, we need to fix a couple socket lifetime issues on the node side. These were basically
(1) has some gross results because the code that uses sockets is really gross and we have not gone high when they went low, but it's a minimal changeset.
(2) is a real pain because the control flow bounces around 3 different events on like 3 different emitters and you need to trace it through; what it boils down to is noting that both the readable and writable streams AND the serialport have a "close" event they fire, and that only means the serial port is actually closed if it was the serial port that did it.
This doesn't get us all the way there almost certainly; we do occasionally still drop parts of packets on heavy usage like fetching logs sometimes, and therefore probably need to introduce more timeouts. We also haven't fully tested all the OS's and flows yet but we do get a whole lot farther with that testing than we previously did.
Testing more
At least the following: