fix: prevent USB CDC TX stall on ring buffer wrap - #5538
Conversation
|
Please note test failure: |
|
I'd suggest we reduce code size and simplify instead of adding more complexity on top of an already needlessly complex implementation, but oh well, such are the PRs we merge in tinygo |
|
Ideally, we should read 64 bytes at a time from the ring buffer and write them directly to _usbDPSRAM. |
The test in |
code to reproduce the issueserver.go: package main
import (
"strings"
"time"
"machine"
)
func main() {
for !machine.Serial.DTR() {
time.Sleep(100 * time.Millisecond)
}
tick := time.NewTicker(100 * time.Microsecond)
for range tick.C {
println(strings.Repeat("*", 259))
}
}
client.go: package main
import (
"bufio"
"go.bug.st/serial"
)
func main() {
port, err := serial.Open("COM3", &serial.Mode{})
if err != nil {
panic(err)
}
scanner := bufio.NewScanner(port)
cnt := 0
for scanner.Scan() {
text := scanner.Text()
if cnt > 0 && len(text) != 259 {
println()
panic("wrong length: " + text)
} else {
print("\rcount:", cnt)
}
cnt++
}
} |
c90a4af to
ff8149b
Compare
|
I tested the suggested case. The same issue also occurs with TinyGo 0.41, so it does not appear to be a regression caused by the recent improvements in 0.42-dev. On the other hand, this test does not run correctly with scheduler=cores. However, it still stops even after removing all CDC handling and replacing it with an LED-only test, so I believe this is a separate issue from CDC TX. At this point, it seems more likely to be an issue around the interaction between scheduler=cores and time.Ticker, so I think the results with cores should be considered separately from the evaluation of this PR. Also, the reproducer writes ~260 bytes every 100µs (~2.6 MB/s), which is well above what USB Full Speed CDC can sustain. The TX ring will therefore eventually fill regardless of packet ordering, so I'm not sure that the observed data loss by itself demonstrates the short-packet issue this PR is intended to address. |
Fix USB CDC TX stall when TX ring buffer wraps
Fixes #5537
This patch fixes an occasional USB CDC TX stall when the TX ring buffer wraps.
When the ring buffer wrapped, the TX path could send a short packet before the remaining data.
Example:
Before:
Expected:
A short packet indicates the end of a USB bulk transfer. Although the packet sequence is valid at the USB protocol level, it can cause interoperability issues with some USB CDC host implementations.
The root cause was that the TX path only used the first contiguous segment returned by the ring buffer. When the buffer wrapped, the first segment could be smaller than the endpoint packet size, causing an unintended short packet.
This patch combines wrapped ring buffer segments to fill the USB endpoint packet size whenever possible.
The zero-copy path is preserved for contiguous data:
Only wrapped data requires a temporary buffer:
After this change, the TX stall could no longer be reproduced.