Passthrough, USB Improvements #11256
Open
+370
−57
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
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}
Contributor
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;
}
Comment on lines
+557
to
+589
| 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; | ||
| } |
Contributor
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]
Suggested change
| 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; | |
| } |
Comment on lines
+677
to
683
| 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, |
Contributor
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);
Comment on lines
320
to
349
| 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); | ||
| } |
Contributor
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]
Suggested change
| 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; | |
| } |
Collaborator
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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