STM32 USB serial can't recover from EndpointError::BufferOverflow

[PegasisForever]

Reproduce MCU code: https://gist.github.com/PegasisForever/4b449b383dce5f7f9a546151020005d4 (notice line 91 and line 98 are changed) (MCU: STM32F412)

Reproduce host code: this python script basically sends 9 bytes to the mcu which triggers the buffer overflow:

from serial import Serial

serial = Serial('/dev/ttyACM1', 115200, timeout=1, writeTimeout=10000)

serial.write(b'0'*9)
serial.flush()
serial.read()

[Dirbaio]

This is sort of intended. The CDC ACM class we have now is "low level", it reads/writes USB packets from the endpoint. Reading an USB packet with a too small buffer errors with BufferOverflow and intentionally doesn't pop the packet. The rationale for this was that the user might want to retry reading the packet with a bigger buffer. I can't think of any sane reason a program would want to do that, but it felt cleaner to me than dropping the packet.

If you want to force the host to send smaller packets, you can set a smaller max packet size for the CdcAcmClass (the 64 in line 63).

We could have a "high level" CDC ACM, where received packets are enqueued in a buffer, and the user would read bytes out of that buffer. So if a 9-byte packet arrives, you could read it as an 8-byte read and then a 1-byte read. This is more in line with what people expect out of a serial port, but we don't have it today.

What are you trying to do that is running into this problem?

[PegasisForever]

Thanks for your explaination!

it's really about resilience to unexpected inputs by default. In my case, my program on mcu is expecting a 8 byte long command so it reads the data using a 8 byte buffer. The logic is something like this:

let mut buffer = [0u8; 8];
loop {
    let command = usb.read(&mut buffer).await; // some wrapper struct around cdc acm, returns Result<[u8;8],EndpointError>

    if let Some(command) =  command {
         // do something based on command
    }
}

I imagine most people will write something like above and the logic seems fine on paper. However, if the host sends something longer than 8 bytes, this will become a dead loop because usb.read will always return overflow error.

This happened to me when I have cura open on my desktop and it automatically tries to communicate with the device, thinking it's a 3D printer.

[PegasisForever]

I think poping the overflowing packet is a better way to handle this.

[Dirbaio]

You really can't rely on the packet length. For example a write of 72 bytes from the host will be fragmented as two packets of 64 bytes and 8 bytes. You'd pop the 64 bytes one, then parse the 8 bytes as garbage, defeating the point of the sanity check.

Hosts are also allowed to buffer writes. For example two writes of 8 bytes can be received by the device as 1 packet of 16 bytes. This would also break your code.

A serial port is fundamentally a sequence of bytes. The packets are just an artifact of the implementation of how USB sends these bytes on the wire. They're not intended to be used for framing, you should add your own framing such as COBS if you want resilience.

Also, the "don't pop" behavior is not from CDC ACM, it's from deeper in the USB stack. Changing it would have affect all classes. Popping the packet might make some sense for CDC ACM, but it doesn't necessarily so for other classes.

[Dirbaio]

IMO the low-level CDC ACM is working as intended. See the warnings in the docs: "read_packet must be called with a buffer large enough to hold max_packet_size bytes.".

If we had a "high level" CDC ACM, that implements the embedded-io traits, where you read bytes from it and never interact with USB packets directly, would that work for you?

[PegasisForever]

See the warnings in the docs: "read_packet must be called with a buffer large enough to hold max_packet_size bytes.".

oops, my bad, didn't read the docs :sweat_smile: Ya I think a high level CDC ACM would be the way to go. Can you point me to where should I add this struct / what should it be called? I can do a pr for this