Passthrough, USB Improvements #11256
Conversation
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
High-level Suggestion
The USB CDC implementations for F4 and F7/H7 targets contain duplicated logic for circular buffers and flow control. This should be refactored into a unified structure to reduce code duplication and improve maintainability. [High-level, importance: 8]
Solution Walkthrough:
Before:
// In 'src/main/vcpf4/usbd_cdc_vcp.c' (for F4)
static uint8_t APP_Tx_Buffer[APP_TX_DATA_SIZE];
static volatile uint32_t APP_Tx_ptr_in = 0;
static volatile uint32_t APP_Tx_ptr_out = 0;
static volatile bool packetReceiveStalled;
static uint16_t VCP_DataRx(uint8_t* Buf, uint32_t Len) {
// ... custom circular buffer and flow control logic ...
if (free <= USB_RX_STALL_THRESHOLD) {
packetReceiveStalled = true;
return USBD_FAIL;
}
return USBD_OK;
}
// In 'src/main/vcp_hal/usbd_cdc_interface.c' (for F7/H7)
uint8_t AppRxBuffer[APP_RX_DATA_SIZE];
volatile uint32_t AppRxPtrIn = 0;
volatile uint32_t AppRxPtrOut = 0;
static volatile bool packetReceiveStalled;
static int8_t CDC_Itf_Receive(uint8_t* Buf, uint32_t *Len) {
// ... duplicated circular buffer and flow control logic ...
if (free <= USB_RX_STALL_THRESHOLD) {
packetReceiveStalled = true;
return USBD_FAIL;
}
return USBD_OK;
}After:
// In a new common file 'usb_cdc_buffer.c'
typedef struct {
uint8_t *buf;
size_t size;
volatile uint32_t ptr_in;
volatile uint32_t ptr_out;
volatile bool stalled;
} cdc_buffer_t;
bool cdc_buffer_write(cdc_buffer_t *b, uint8_t *data, uint32_t len);
uint32_t cdc_buffer_read(cdc_buffer_t *b, uint8_t *data, uint32_t len);
// In 'src/main/vcpf4/usbd_cdc_vcp.c' (for F4)
static cdc_buffer_t rx_buffer;
static uint16_t VCP_DataRx(uint8_t* Buf, uint32_t Len) {
if (!cdc_buffer_write(&rx_buffer, Buf, Len)) {
return USBD_FAIL; // Stall
}
return USBD_OK;
}
// In 'src/main/vcp_hal/usbd_cdc_interface.c' (for F7/H7)
static cdc_buffer_t rx_buffer;
static int8_t CDC_Itf_Receive(uint8_t* Buf, uint32_t *Len) {
if (!cdc_buffer_write(&rx_buffer, Buf, *Len)) {
return USBD_FAIL; // Stall
}
return USBD_OK;
}
| static bool serialPassthroughTransfer(serialPort_t *src, serialPort_t *dst, serialConsumer *consumer, escapeSequenceState_t *escapeState, uint32_t now) | ||
| { | ||
| uint8_t buf[64]; | ||
| uint32_t available = serialRxBytesWaiting(src); | ||
| uint32_t free = serialTxBytesFree(dst); | ||
| uint32_t count = (available < free) ? available : free; | ||
| if (count > sizeof(buf)) { | ||
| count = sizeof(buf); | ||
| } | ||
|
|
||
| if (count > 0) { | ||
| LED0_ON; | ||
| serialBeginWrite(dst); | ||
| serialReadBuf(src, buf, count); | ||
| serialWriteBuf(dst, buf, count); | ||
| for (uint32_t i = 0; i < count; i++) { | ||
| consumer(buf[i]); | ||
| // Hayes escape sequence detection: [1s silence]+++[1s silence] | ||
| // https://en.wikipedia.org/wiki/Escape_sequence#Modem_control | ||
| if (escapeState) { | ||
| escapeSequenceProcessChar(escapeState, buf[i], now); | ||
| } | ||
| } | ||
| serialEndWrite(dst); | ||
| LED0_OFF; | ||
| } else { | ||
| if (escapeState) { | ||
| return escapeSequenceCheckGuard(escapeState, now); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Suggestion: In serialPassthroughTransfer, use the return value of serialReadBuf to get the actual number of bytes read, preventing potential processing of uninitialized buffer data. [possible issue, importance: 8]
| static bool serialPassthroughTransfer(serialPort_t *src, serialPort_t *dst, serialConsumer *consumer, escapeSequenceState_t *escapeState, uint32_t now) | |
| { | |
| uint8_t buf[64]; | |
| uint32_t available = serialRxBytesWaiting(src); | |
| uint32_t free = serialTxBytesFree(dst); | |
| uint32_t count = (available < free) ? available : free; | |
| if (count > sizeof(buf)) { | |
| count = sizeof(buf); | |
| } | |
| if (count > 0) { | |
| LED0_ON; | |
| serialBeginWrite(dst); | |
| serialReadBuf(src, buf, count); | |
| serialWriteBuf(dst, buf, count); | |
| for (uint32_t i = 0; i < count; i++) { | |
| consumer(buf[i]); | |
| // Hayes escape sequence detection: [1s silence]+++[1s silence] | |
| // https://en.wikipedia.org/wiki/Escape_sequence#Modem_control | |
| if (escapeState) { | |
| escapeSequenceProcessChar(escapeState, buf[i], now); | |
| } | |
| } | |
| serialEndWrite(dst); | |
| LED0_OFF; | |
| } else { | |
| if (escapeState) { | |
| return escapeSequenceCheckGuard(escapeState, now); | |
| } | |
| } | |
| return false; | |
| } | |
| static bool serialPassthroughTransfer(serialPort_t *src, serialPort_t *dst, serialConsumer *consumer, escapeSequenceState_t *escapeState, uint32_t now) | |
| { | |
| uint8_t buf[64]; | |
| uint32_t available = serialRxBytesWaiting(src); | |
| uint32_t free = serialTxBytesFree(dst); | |
| uint32_t count = (available < free) ? available : free; | |
| if (count > sizeof(buf)) { | |
| count = sizeof(buf); | |
| } | |
| if (count > 0) { | |
| LED0_ON; | |
| serialBeginWrite(dst); | |
| uint32_t bytesRead = serialReadBuf(src, buf, count); | |
| serialWriteBuf(dst, buf, bytesRead); | |
| for (uint32_t i = 0; i < bytesRead; i++) { | |
| consumer(buf[i]); | |
| // Hayes escape sequence detection: [1s silence]+++[1s silence] | |
| // https://en.wikipedia.org/wiki/Escape_sequence#Modem_control | |
| if (escapeState) { | |
| escapeSequenceProcessChar(escapeState, buf[i], now); | |
| } | |
| } | |
| serialEndWrite(dst); | |
| LED0_OFF; | |
| } else { | |
| if (escapeState) { | |
| return escapeSequenceCheckGuard(escapeState, now); | |
| } | |
| } | |
| return false; | |
| } |
| if (APP_FOPS.pIf_DataRx(USB_Rx_Buffer, USB_Rx_Cnt) != USBD_OK) | ||
| { | ||
| return USBD_OK; | ||
| } | ||
|
|
||
| /* Prepare Out endpoint to receive next packet */ | ||
| DCD_EP_PrepareRx(pdev, |
There was a problem hiding this comment.
Suggestion: In usbd_cdc_DataOut, re-arm the USB OUT endpoint even if pIf_DataRx returns an error to prevent the USB data flow from stopping permanently. [possible issue, importance: 8]
New proposed code:
if (APP_FOPS.pIf_DataRx(USB_Rx_Buffer, USB_Rx_Cnt) != USBD_OK)
{
+ /* Even on error, re-arm the OUT endpoint to continue receiving */
+ DCD_EP_PrepareRx(pdev,
+ CDC_OUT_EP,
+ (uint8_t*)(USB_Rx_Buffer),
+ CDC_DATA_OUT_PACKET_SIZE);
return USBD_OK;
}
/* Prepare Out endpoint to receive next packet */
DCD_EP_PrepareRx(pdev,
CDC_OUT_EP,
(uint8_t*)(USB_Rx_Buffer),
CDC_DATA_OUT_PACKET_SIZE);| static int8_t CDC_Itf_Receive(uint8_t* Buf, uint32_t *Len) | ||
| { | ||
| rxAvailable = *Len; | ||
| rxBuffPtr = Buf; | ||
| if (!rxAvailable) { | ||
| // Received an empty packet, trigger receiving the next packet. | ||
| // This will happen after a packet that's exactly 64 bytes is received. | ||
| // The USB protocol requires that an empty (0 byte) packet immediately follow. | ||
| // Copy received data to ring buffer, handling wrap-around | ||
| uint32_t len = *Len; | ||
| uint32_t ptrIn = AppRxPtrIn; | ||
| uint32_t tailRoom = APP_RX_DATA_SIZE - ptrIn; | ||
|
|
||
| if (len <= tailRoom) { | ||
| // Data fits without wrapping | ||
| memcpy(&AppRxBuffer[ptrIn], Buf, len); | ||
| ptrIn = (ptrIn + len) % APP_RX_DATA_SIZE; | ||
| } else { | ||
| // Data wraps around - copy in two parts | ||
| memcpy(&AppRxBuffer[ptrIn], Buf, tailRoom); | ||
| memcpy(&AppRxBuffer[0], Buf + tailRoom, len - tailRoom); | ||
| ptrIn = len - tailRoom; | ||
| } | ||
| AppRxPtrIn = ptrIn; | ||
|
|
||
| // Check if we have room for another packet; if not, stall | ||
| uint32_t free = (APP_RX_DATA_SIZE + AppRxPtrOut - ptrIn - 1) % APP_RX_DATA_SIZE; | ||
| if (free <= USB_RX_STALL_THRESHOLD) { | ||
| packetReceiveStalled = true; | ||
| return USBD_FAIL; // Don't arm next receive | ||
| } else { | ||
| USBD_CDC_ReceivePacket(&USBD_Device); | ||
| } | ||
|
|
||
| return (USBD_OK); | ||
| } |
There was a problem hiding this comment.
Suggestion: Compute available free space before writing into AppRxBuffer and reject or truncate (*Len) when len exceeds free space to avoid overwriting unread data. [Learned best practice, importance: 6]
| static int8_t CDC_Itf_Receive(uint8_t* Buf, uint32_t *Len) | |
| { | |
| rxAvailable = *Len; | |
| rxBuffPtr = Buf; | |
| if (!rxAvailable) { | |
| // Received an empty packet, trigger receiving the next packet. | |
| // This will happen after a packet that's exactly 64 bytes is received. | |
| // The USB protocol requires that an empty (0 byte) packet immediately follow. | |
| // Copy received data to ring buffer, handling wrap-around | |
| uint32_t len = *Len; | |
| uint32_t ptrIn = AppRxPtrIn; | |
| uint32_t tailRoom = APP_RX_DATA_SIZE - ptrIn; | |
| if (len <= tailRoom) { | |
| // Data fits without wrapping | |
| memcpy(&AppRxBuffer[ptrIn], Buf, len); | |
| ptrIn = (ptrIn + len) % APP_RX_DATA_SIZE; | |
| } else { | |
| // Data wraps around - copy in two parts | |
| memcpy(&AppRxBuffer[ptrIn], Buf, tailRoom); | |
| memcpy(&AppRxBuffer[0], Buf + tailRoom, len - tailRoom); | |
| ptrIn = len - tailRoom; | |
| } | |
| AppRxPtrIn = ptrIn; | |
| // Check if we have room for another packet; if not, stall | |
| uint32_t free = (APP_RX_DATA_SIZE + AppRxPtrOut - ptrIn - 1) % APP_RX_DATA_SIZE; | |
| if (free <= USB_RX_STALL_THRESHOLD) { | |
| packetReceiveStalled = true; | |
| return USBD_FAIL; // Don't arm next receive | |
| } else { | |
| USBD_CDC_ReceivePacket(&USBD_Device); | |
| } | |
| return (USBD_OK); | |
| } | |
| static int8_t CDC_Itf_Receive(uint8_t* Buf, uint32_t *Len) | |
| { | |
| uint32_t len = *Len; | |
| uint32_t ptrIn = AppRxPtrIn; | |
| uint32_t ptrOut = AppRxPtrOut; | |
| // Free space in ring (leave 1 byte empty to distinguish full/empty) | |
| uint32_t free = (APP_RX_DATA_SIZE + ptrOut - ptrIn - 1) % APP_RX_DATA_SIZE; | |
| if (len > free) { | |
| packetReceiveStalled = true; | |
| return USBD_FAIL; | |
| } | |
| uint32_t tailRoom = APP_RX_DATA_SIZE - ptrIn; | |
| if (len <= tailRoom) { | |
| memcpy(&AppRxBuffer[ptrIn], Buf, len); | |
| ptrIn = (ptrIn + len) % APP_RX_DATA_SIZE; | |
| } else { | |
| memcpy(&AppRxBuffer[ptrIn], Buf, tailRoom); | |
| memcpy(&AppRxBuffer[0], Buf + tailRoom, len - tailRoom); | |
| ptrIn = len - tailRoom; | |
| } | |
| AppRxPtrIn = ptrIn; | |
| free = (APP_RX_DATA_SIZE + ptrOut - ptrIn - 1) % APP_RX_DATA_SIZE; | |
| if (free <= USB_RX_STALL_THRESHOLD) { | |
| packetReceiveStalled = true; | |
| return USBD_FAIL; | |
| } | |
| USBD_CDC_ReceivePacket(&USBD_Device); | |
| return USBD_OK; | |
| } |
|
Did some basic Configurator action tests with WP and Dump transfers with F765. Nothing seems to be broken so far. Will do a few more FCs as well. |
|
Now that the builds work, I wll run some tests with SpeedyBee USB-BT Adapters as well as Linux Machines. |
|
I was asked to take a look at this. I haven't had time to dig into this much yet. And may not get time to, and I don't have a lot of background knowledge. But two sentences caught my eye. Alternatively, a message (MSP format or something simpler) dedicated to MSP-RC Bind where the serial port is specified in the CLI, i.e., bind_rx 2, which would then temporarily disable MSP on UART2, send the string directly to the UART, and re-enable MSP. Could it NOT disable MSP, send the string and re-enable, instead sending the string as an MSP payload? I also noticed somewhere a comment that it "mirrors the baud rate set on the PC". I may well be missing context here, but when I looked a few weeks ago, setting the baud rate on a CDC/ACM port was a no-op in the driver. It just does nothing, and the system immediately forgets what baud rate was requested. Because the actual baud rates for USB CDC are the predefined USB rates -- 1.5 Mbps or 12 Mbps for USB 1.1, for example. Again I may be missing context, but I wonder how it can could know what's set on the PC if the PC doesn't even know what setting was requested - if it was entirely ignored because it's CDC. |
The bind thing is a different topic :D This here is just about flashing the RX with passthrough and handling it automatically.
My Idea was, to simplify the implementation for now without making complex changes in the INAV MSP handling. In that case you could send the first notes of "seven nation army" in binary to trigger the BIND mode as long as the Rx knows what to do with it. My thought was just to have a proper MSP message still, so it is standardized for future MSP-RC systems and in case we ever advance the MSP function to let the FC send this message to a specific port without trickery. Should also make the implementation on the mLRS easier if its also a MSP message instead of an arbitrary byte string? @jlpoltrack |
|
I forgot to say -- thanks for your work on this! It looks like good work, both "in the small", at the line-of-code level and more broadly, architecturally. And some of it is beyond my understanding. I have some AT32 on my desk if you let me know how to test - I don't know if you have a script or something. But I don't have and MSP receiver, I can only mock one. The one issue I see looking at the code "in the small" is what happens if the interrupt writes to the pointer while the main loop reads? Maybe it would be safer to use ATOMIC_BLOCK: |
3 participants
User description
Wanted to use INAV passthrough to flash STM32 and ESP receivers, but ran into a few things:
Changes made:
Tests ran:
PR Type
Enhancement
Description
Add USB flow control and line coding mirroring for passthrough mode
Implement Hayes escape sequence detection for CLI mode exit
Refactor passthrough transfer logic with buffered I/O operations
serialReadBufImprove USB CDC interface with proper packet handling
USBD_CDC_ReceivePacketfunction and flow control state trackingDiagram Walkthrough
File Walkthrough
12 files
Export USBD_CDC_ReceivePacket function declarationAdd packet receive function and improve SOF handlingAdd serialReadBuf for buffered serial read operationsDeclare serialReadBuf function for batch readsAdd usbVcpGetLineCoding to retrieve parity and stop bitsExport usbVcpGetLineCoding function declarationImplement Hayes escape sequence and refactored passthrough logicDefine escape sequence state structure and helper functionsRedesign USB RX circular buffer with flow control stallingExport CDC_StopBits and CDC_Parity accessor functionsImplement flow control and line coding accessors for F4Export CDC_StopBits and CDC_Parity for F4 variant