usbd_video: use pingpang buffer to improve tx performance #277
Conversation
WalkthroughThe pull request introduces significant modifications to the video streaming functionality within the USB device framework. Key changes include the expansion of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
chenzhihong007
force-pushed
the
master
branch
from
efd683f to
328ef42
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
demo/video_static_mjpeg_template.c (2)
209-209: Ensure proper initialization of both ping-pong buffersThe memset call correctly initializes both buffers since it uses sizeof(packet_buffer). However, consider adding a comment to clarify the ping-pong buffer strategy for future maintainers.
USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE]; +// Ping-pong buffers for double buffering to improve USB video streaming performance void video_test(uint8_t busid) { memset(packet_buffer, 0, sizeof(packet_buffer));Also applies to: 211-212
Line range hint
218-223: Consider adding timeout mechanism to prevent indefinite blockingWhile the implementation correctly uses both ping-pong buffers, the busy-wait loop could potentially block indefinitely. Consider adding a timeout mechanism to ensure the system remains responsive.
usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_mjpeg, sizeof(cherryusb_mjpeg)); + uint32_t timeout = 1000; // Adjust timeout value based on your requirements while (iso_tx_busy) { if (tx_flag == 0) { break; } + if (--timeout == 0) { + // Handle timeout - could reset state or log error + iso_tx_busy = false; + break; + } + // Consider adding a small delay here if needed }demo/video_static_yuyv_template.c (1)
Line range hint
220-225: Consider improving transfer handling robustness.While the ping-pong buffer implementation is correct, consider these improvements:
- Add timeout mechanism to prevent infinite waiting
- Add error handling for transfer failures
- Consider using events/callbacks instead of busy waiting
Example improvement:
- while (iso_tx_busy) { - if (tx_flag == 0) { - break; - } - } + uint32_t timeout = 1000; // Adjust timeout value as needed + while (iso_tx_busy && tx_flag && timeout > 0) { + if (usbd_video_get_transfer_status(busid, VIDEO_IN_EP) < 0) { + // Handle transfer error + break; + } + timeout--; + // Add small delay if needed + } + if (timeout == 0) { + // Handle timeout error + }demo/video_static_h264_template.c (2)
209-209: Define a macro for the buffer count to enhance maintainabilityTo improve code readability and ease future modifications, consider defining a macro for the buffer count value
2used in the declaration ofpacket_buffer. This change ensures that any future adjustments to the buffer count require changes in only one place.Apply this diff to define a macro and update the array declaration:
+#define BUFFER_COUNT 2 USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][40 * 1024]; - +USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[BUFFER_COUNT][40 * 1024];
217-217: Usesizeof(packet_buffer)instead of hardcoded size inmemsetTo ensure the
memsetoperation correctly reflects the size ofpacket_buffer, especially if the buffer size changes in the future, consider usingsizeof(packet_buffer)instead of the hardcoded value2* 40 * 1024.Apply this diff to update the
memsetcall:-memset(packet_buffer, 0, 2* 40 * 1024); +memset(packet_buffer, 0, sizeof(packet_buffer));demo/video_audiov1_hid_template.c (2)
483-483: Simplify buffer pointers in function callYou can simplify the buffer pointers by passing
video_packet_buffer[0]andvideo_packet_buffer[1]instead of&video_packet_buffer[0][0]and&video_packet_buffer[1][0]to improve readability.Apply this diff to simplify the code:
-usbd_video_stream_start_write(busid, VIDEO_IN_EP, &video_packet_buffer[0][0], &video_packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_mjpeg, sizeof(cherryusb_mjpeg)); +usbd_video_stream_start_write(busid, VIDEO_IN_EP, video_packet_buffer[0], video_packet_buffer[1], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_mjpeg, sizeof(cherryusb_mjpeg));
Line range hint
483-490: Optimize thevideo_testfunction to avoid busy-waitingIn the
video_testfunction, the innerwhileloop busy-waits onvideo_iso_tx_busy, which can lead to high CPU usage. Consider adding a small delay within the loop or using a synchronization mechanism to reduce CPU load.Apply this diff to add a delay within the busy-wait loop:
while (video_iso_tx_busy) { if (video_tx_flag == 0) { break; } + // Add a small delay to prevent CPU overuse + usbd_delay_ms(1); }Alternatively, consider using event-driven programming or synchronization primitives provided by your system to handle the busy-wait more efficiently.
🛑 Comments failed to post (2)
class/video/usbd_video.c (2)
805-844: 🛠️ Refactor suggestion
Refactor duplicate code blocks to improve maintainability
The code blocks from lines 805~ to 844~ and 846~ to 879~ are nearly identical, differing only in the use of
ep_buf0versusep_buf1. This duplication can make the code harder to maintain and increase the risk of inconsistencies.Consider refactoring these blocks into a helper function that handles buffer filling. This will reduce code duplication and enhance code readability.
Example refactoring:
static void fill_ep_buffer(uint8_t busid, uint8_t *ep_buf, uint32_t *ep_buf_len, bool *ep_buf_ready) { uint32_t remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset; if (remain == 0) { g_usbd_video[busid].stream_frameid ^= 1; g_usbd_video[busid].stream_finish = true; } else { uint32_t len = MIN(remain, (g_usbd_video[busid].probe.dwMaxPayloadTransferSize - g_usbd_video[busid].stream_headerlen) * g_usbd_video[busid].max_packets); uint32_t offset = 0; while (len > 0) { struct video_payload_header *header = (struct video_payload_header *)&ep_buf[offset]; header->bHeaderLength = g_usbd_video[busid].stream_headerlen; header->headerInfoUnion.bmheaderInfo = 0; header->headerInfoUnion.headerInfoBits.endOfHeader = 1; header->headerInfoUnion.headerInfoBits.endOfFrame = 0; header->headerInfoUnion.headerInfoBits.frameIdentifier = g_usbd_video[busid].stream_frameid; uint32_t len2 = MIN(len, g_usbd_video[busid].probe.dwMaxPayloadTransferSize - g_usbd_video[busid].stream_headerlen); usb_memcpy(&ep_buf[offset + g_usbd_video[busid].stream_headerlen], &g_usbd_video[busid].stream_buf[g_usbd_video[busid].stream_offset], len2); g_usbd_video[busid].stream_offset += len2; len -= len2; offset += (len2 + g_usbd_video[busid].stream_headerlen); if (g_usbd_video[busid].stream_offset == g_usbd_video[busid].stream_len) { header->headerInfoUnion.headerInfoBits.endOfFrame = 1; } } *ep_buf_len = offset; *ep_buf_ready = true; } }Then, replace the duplicated code with calls to this helper function:
if (!g_usbd_video[busid].ep_buf0_ready) { fill_ep_buffer(busid, g_usbd_video[busid].ep_buf0, &g_usbd_video[busid].ep_buf0_len, &g_usbd_video[busid].ep_buf0_ready); if (!g_usbd_video[busid].ep_buf1_ready) { g_usbd_video[busid].ep_buf_idx = 0; usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len); } } if (!g_usbd_video[busid].ep_buf1_ready) { fill_ep_buffer(busid, g_usbd_video[busid].ep_buf1, &g_usbd_video[busid].ep_buf1_len, &g_usbd_video[busid].ep_buf1_ready); }Also applies to: 846-879
791-799:
⚠️ Potential issuePotential logical error in buffer readiness flags
In the conditions starting at lines 791~ and 795~, the assignment of buffer readiness flags may be incorrect.
At line 792~, when
ep_buf1_readyis true andep_buf_idxis 0, the code setsep_buf0_ready = false;. It seems thatep_buf1_readyshould be cleared instead sinceep_buf1is about to be sent.Similarly, at line 796~, when
ep_buf0_readyis true andep_buf_idxis 1, the code setsep_buf1_ready = false;, whereas it should likely clearep_buf0_readysinceep_buf0is about to be sent.This could lead to buffer readiness flags not accurately reflecting the state of the buffers, potentially causing synchronization issues.
Apply the following diff to correct the buffer readiness flags:
@@ -792,7 +792,7 @@ if (g_usbd_video[busid].ep_buf1_ready && (g_usbd_video[busid].ep_buf_idx == 0)) { /* callback: buf1 ready and buf0 was sent */ - g_usbd_video[busid].ep_buf0_ready = false; /* clear buf0 ready flag */ + g_usbd_video[busid].ep_buf1_ready = false; /* clear buf1 ready flag */ g_usbd_video[busid].ep_buf_idx = 1; usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf1, g_usbd_video[busid].ep_buf1_len); /* send buf1 data */ } else if (g_usbd_video[busid].ep_buf0_ready && (g_usbd_video[busid].ep_buf_idx == 1)) { /* callback: buf0 ready and buf1 was sent */ @@ -796,7 +796,7 @@ - g_usbd_video[busid].ep_buf1_ready = false; /* clear buf1 ready flag */ + g_usbd_video[busid].ep_buf0_ready = false; /* clear buf0 ready flag */ g_usbd_video[busid].ep_buf_idx = 0; usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len); /* send buf0 data */ } else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (g_usbd_video[busid].ep_buf1_ready && (g_usbd_video[busid].ep_buf_idx == 0)) { /* callback: buf1 ready and buf0 was sent */ g_usbd_video[busid].ep_buf1_ready = false; /* clear buf1 ready flag */ g_usbd_video[busid].ep_buf_idx = 1; usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf1, g_usbd_video[busid].ep_buf1_len); /* send buf1 data */ } else if (g_usbd_video[busid].ep_buf0_ready && (g_usbd_video[busid].ep_buf_idx == 1)) { /* callback: buf0 ready and buf1 was sent */ g_usbd_video[busid].ep_buf0_ready = false; /* clear buf0 ready flag */ g_usbd_video[busid].ep_buf_idx = 0; usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len); /* send buf0 data */ } else {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
demo/video_static_h264_template.c (2)
209-209: Consider using defines for buffer dimensions.The buffer dimensions should be defined using preprocessor constants for better maintainability and clarity. This would also ensure consistency with other size-related constants already defined in the file (like MAX_PAYLOAD_SIZE).
+#define PACKET_BUFFER_COUNT 2 +#define PACKET_BUFFER_SIZE (40 * 1024) -USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][40 * 1024]; +USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[PACKET_BUFFER_COUNT][PACKET_BUFFER_SIZE];
Line range hint
221-226: Add timeout and error handling to the transfer loop.The current implementation has two potential issues:
- The busy wait loop could block indefinitely
- There's no error handling for the stream write call
Consider implementing a timeout mechanism and error handling:
+#define TRANSFER_TIMEOUT_MS 1000 + + uint32_t start_time = get_sys_tick_ms(); usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], - MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_h264, sizeof(cherryusb_h264)); + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_h264, sizeof(cherryusb_h264)); + if (ret != 0) { + // Handle error + continue; + } while (iso_tx_busy) { if (tx_flag == 0) { break; } + if ((get_sys_tick_ms() - start_time) > TRANSFER_TIMEOUT_MS) { + // Handle timeout + break; + } }demo/video_static_yuyv_template.c (2)
211-211: LGTM! Consider cache line alignment for optimal DMA performance.The ping-pong buffer implementation with USB_NOCACHE_RAM_SECTION and USB_MEM_ALIGNX looks good. For further optimization, consider aligning each buffer to cache line size to prevent false sharing and improve DMA performance.
-USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE]; +USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE] __attribute__((aligned(CONFIG_CPU_CACHE_LINE_SIZE)));
Line range hint
211-225: Consider frame drop handling and buffer sizing strategy.The current implementation might block on
iso_tx_busywhen frames can't be transmitted fast enough. Consider:
- Implementing frame dropping logic when falling behind
- Making buffer sizes configurable based on resolution/fps
- Adding statistics collection for monitoring performance
This will help prevent video stuttering and provide better QoS under varying conditions.
demo/video_audiov1_hid_template.c (3)
272-272: Good implementation of ping-pong buffering!The change from a single buffer to a dual-buffer system enables efficient video streaming by allowing one buffer to be filled while the other is being transmitted, which can help prevent data underruns and maintain consistent frame rates.
This ping-pong buffer implementation provides several benefits:
- Reduced latency by overlapping data preparation and transmission
- Prevention of data underruns during USB transmission
- More consistent frame rates due to always having a buffer ready
483-483: Consider improving the streaming implementation.While the ping-pong buffer usage is correct, there are several potential improvements to consider:
- Add error handling for the video streaming call
- Consider using a callback-based approach instead of busy-waiting
- Initialize only the active buffer before streaming
Here's a suggested improvement:
- usbd_video_stream_start_write(busid, VIDEO_IN_EP, &video_packet_buffer[0][0], &video_packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_mjpeg, sizeof(cherryusb_mjpeg)); + if (usbd_video_stream_start_write(busid, VIDEO_IN_EP, + &video_packet_buffer[0][0], + &video_packet_buffer[1][0], + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, + (uint8_t *)cherryusb_mjpeg, + sizeof(cherryusb_mjpeg)) != 0) { + // Handle error + continue; + }
Line range hint
478-489: Consider restructuring the video test loop for better efficiency.The current busy-wait implementation in the video test function could impact system performance.
Consider implementing a more efficient approach:
void video_test(uint8_t busid) { memset(video_packet_buffer, 0, sizeof(video_packet_buffer)); + uint8_t active_buffer = 0; while (1) { if (video_tx_flag) { - video_iso_tx_busy = true; - usbd_video_stream_start_write(busid, VIDEO_IN_EP, &video_packet_buffer[0][0], &video_packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_mjpeg, sizeof(cherryusb_mjpeg)); - while (video_iso_tx_busy) { - if (video_tx_flag == 0) { - break; - } - } + // Prepare next buffer while current one is being transmitted + prepare_video_data(&video_packet_buffer[active_buffer][0]); + + if (!video_iso_tx_busy) { + video_iso_tx_busy = true; + if (usbd_video_stream_start_write(busid, VIDEO_IN_EP, + &video_packet_buffer[active_buffer][0], + &video_packet_buffer[1-active_buffer][0], + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, + (uint8_t *)cherryusb_mjpeg, + sizeof(cherryusb_mjpeg)) == 0) { + active_buffer = 1 - active_buffer; + } + } + + // Add a small delay or yield to prevent tight spinning + usbd_delay_ms(1); } } }This implementation:
- Alternates between buffers efficiently
- Prepares the next buffer while current one is being transmitted
- Reduces CPU usage by avoiding tight loops
- Adds basic error handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
class/video/usbd_video.c(2 hunks)class/video/usbd_video.h(1 hunks)demo/video_audiov1_hid_template.c(2 hunks)demo/video_static_h264_template.c(1 hunks)demo/video_static_mjpeg_template.c(2 hunks)demo/video_static_yuyv_template.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- class/video/usbd_video.h
- demo/video_static_mjpeg_template.c
🔇 Additional comments (6)
demo/video_static_h264_template.c (2)
217-217: LGTM! Good use of sizeof operator.
The initialization correctly clears both buffers and will automatically adapt if buffer dimensions change.
Line range hint 209-226: Verify buffer switching implementation in USB stack.
While the ping-pong buffer structure is set up correctly, we should verify that the buffer switching logic is properly implemented in the USB stack's video stream implementation.
✅ Verification successful
Buffer switching implementation is correctly handled in the USB stack
The buffer switching implementation in the USB stack is properly implemented with the following key aspects:
- The
usbd_videostructure maintains proper state tracking withep_buf0_ready,ep_buf1_ready, andep_buf_idxflags - Buffer switching logic in
usbd_video_stream_split_transfer:- Switches to buffer 1 when buffer 0 was sent and buffer 1 is ready
- Switches to buffer 0 when buffer 1 was sent and buffer 0 is ready
- Properly manages buffer states during transitions
- Initialization in
usbd_video_stream_start_writecorrectly sets up the ping-pong buffer state
The implementation follows proper ping-pong buffering practices and maintains consistent buffer states throughout the video streaming process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for buffer switching logic in the USB video implementation
rg -A 10 "usbd_video_stream_start_write" --type c
Length of output: 3883
Script:
#!/bin/bash
# Let's check the complete implementation of buffer switching logic
ast-grep --pattern 'struct usbd_video {
$$$
}'
# Also check for any buffer switching related callbacks
rg -A 10 "in_ep_callback" --type c
Length of output: 85
Script:
#!/bin/bash
# Let's try a different approach to find the video structure and buffer handling
rg -A 15 "struct.*usbd_video" --type c
# Also search for the buffer switching implementation
rg -A 15 "ep_buf.*ready" --type c
# And check for ISO transfer completion handling
rg -A 15 "iso.*complete" --type c
Length of output: 14428
demo/video_static_yuyv_template.c (1)
Line range hint 220-225: Verify the synchronization mechanism for ping-pong buffer.
The busy-wait synchronization looks correct but could potentially miss buffer switches if the callback is too quick. Let's verify the implementation in the video class driver.
✅ Verification successful
Let me generate additional scripts to verify the implementation of usbd_video_stream_start_write and its buffer handling mechanism.
Let me generate one final script to check the implementation of the split transfer mechanism, which is crucial for understanding the buffer switching logic.
Based on the analysis of the code, I can now provide a final response about the synchronization mechanism:
The ping-pong buffer synchronization is properly implemented and safe.
The implementation shows a robust synchronization mechanism:
- The
iso_tx_busyflag is properly managed across all state transitions:- Set to
truebefore starting a new transfer - Reset to
falsewhen a frame completes (usbd_video_stream_split_transferreturns true) - Reset to
falseduring USB reset and close events
- Set to
- The
tx_flagacts as a higher-level control to safely terminate transfers - The busy-wait loop has a proper exit condition through
tx_flag, preventing deadlocks - Buffer switching is handled by the USB stack through
usbd_video_stream_start_write, with proper handover in the ISO callback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of usbd_video_stream_start_write and its synchronization mechanism
# Search for the implementation of usbd_video_stream_start_write
ast-grep --pattern 'usbd_video_stream_start_write($$$)'
# Check for potential race conditions in buffer switching
rg -A 5 'iso_tx_busy'
# Verify the split transfer implementation
ast-grep --pattern 'usbd_video_stream_split_transfer($$$)'
Length of output: 11604
Script:
#!/bin/bash
# Search for the implementation of usbd_video_stream_start_write to understand buffer handling
rg -A 10 "usbd_video_stream_start_write.*\{"
# Check for any buffer switching logic in the video class implementation
rg -A 5 "packet_buffer\[.*\]"
# Look for any additional synchronization mechanisms
rg -A 5 "tx_flag"
Length of output: 20982
Script:
#!/bin/bash
# Search for the implementation of usbd_video_stream_split_transfer and its buffer handling
rg -A 15 "usbd_video_stream_split_transfer.*\{"
# Look for any additional buffer synchronization in the video class implementation
rg -A 10 "class.*video.*\{" --type cpp
Length of output: 3736
class/video/usbd_video.c (3)
21-28: LGTM: Well-structured dual buffer management fields
The new fields in usbd_video_priv provide a clean implementation of ping-pong buffering with clear separation of buffer states and metadata.
791-802: LGTM: Clean buffer switching implementation
The implementation properly manages buffer alternation with clear state transitions and flag management.
886-905: LGTM: Clean initialization of dual buffer system
The function properly validates inputs and initializes all necessary state for the ping-pong buffer implementation.
class/video/usbd_video.c
Outdated
| if (!g_usbd_video[busid].ep_buf0_ready) { | ||
| remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset; | ||
|
|
||
| uint32_t len2 = MIN(len, g_usbd_video[busid].probe.dwMaxPayloadTransferSize - g_usbd_video[busid].stream_headerlen); | ||
|
|
||
| usb_memcpy(&g_usbd_video[busid].ep_buffer[offset + g_usbd_video[busid].stream_headerlen], | ||
| &g_usbd_video[busid].stream_buf[g_usbd_video[busid].stream_offset], | ||
| len2); | ||
| if (remain == 0) { | ||
| g_usbd_video[busid].stream_frameid ^= 1; | ||
| g_usbd_video[busid].stream_finish = true; | ||
| } else { | ||
| len = MIN(remain, (g_usbd_video[busid].probe.dwMaxPayloadTransferSize - g_usbd_video[busid].stream_headerlen) * g_usbd_video[busid].max_packets); | ||
|
|
||
| offset = 0; | ||
| while (len > 0) { | ||
| header = (struct video_payload_header *)&g_usbd_video[busid].ep_buf0[offset]; | ||
| header->bHeaderLength = g_usbd_video[busid].stream_headerlen; | ||
| header->headerInfoUnion.bmheaderInfo = 0; | ||
| header->headerInfoUnion.headerInfoBits.endOfHeader = 1; | ||
| header->headerInfoUnion.headerInfoBits.endOfFrame = 0; | ||
| header->headerInfoUnion.headerInfoBits.frameIdentifier = g_usbd_video[busid].stream_frameid; | ||
|
|
||
| uint32_t len2 = MIN(len, g_usbd_video[busid].probe.dwMaxPayloadTransferSize - g_usbd_video[busid].stream_headerlen); | ||
|
|
||
| usb_memcpy(&g_usbd_video[busid].ep_buf0[offset + g_usbd_video[busid].stream_headerlen], | ||
| &g_usbd_video[busid].stream_buf[g_usbd_video[busid].stream_offset], | ||
| len2); | ||
|
|
||
| g_usbd_video[busid].stream_offset += len2; | ||
| len -= len2; | ||
| offset += (len2 + g_usbd_video[busid].stream_headerlen); | ||
|
|
||
| if (g_usbd_video[busid].stream_offset == g_usbd_video[busid].stream_len) { | ||
| header->headerInfoUnion.headerInfoBits.endOfFrame = 1; | ||
| } | ||
| } | ||
| g_usbd_video[busid].ep_buf0_len = offset; | ||
| g_usbd_video[busid].ep_buf0_ready = true; | ||
| if (!g_usbd_video[busid].ep_buf1_ready) { | ||
| g_usbd_video[busid].ep_buf_idx = 0; | ||
| usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated buffer preparation logic
The buffer preparation code for buf0 and buf1 is nearly identical, which could lead to maintenance issues. Consider extracting this into a shared helper function.
+static void prepare_video_buffer(struct usbd_video_priv *video, uint8_t *buffer,
+ uint32_t *buffer_len, bool *buffer_ready) {
+ struct video_payload_header *header;
+ uint32_t remain = video->stream_len - video->stream_offset;
+
+ if (remain == 0) {
+ video->stream_frameid ^= 1;
+ video->stream_finish = true;
+ return;
+ }
+
+ uint32_t len = MIN(remain, (video->probe.dwMaxPayloadTransferSize -
+ video->stream_headerlen) * video->max_packets);
+ uint32_t offset = 0;
+
+ while (len > 0) {
+ header = (struct video_payload_header *)&buffer[offset];
+ header->bHeaderLength = video->stream_headerlen;
+ header->headerInfoUnion.bmheaderInfo = 0;
+ header->headerInfoUnion.headerInfoBits.endOfHeader = 1;
+ header->headerInfoUnion.headerInfoBits.endOfFrame = 0;
+ header->headerInfoUnion.headerInfoBits.frameIdentifier = video->stream_frameid;
+
+ uint32_t len2 = MIN(len, video->probe.dwMaxPayloadTransferSize -
+ video->stream_headerlen);
+
+ usb_memcpy(&buffer[offset + video->stream_headerlen],
+ &video->stream_buf[video->stream_offset],
+ len2);
+
+ video->stream_offset += len2;
+ len -= len2;
+ offset += (len2 + video->stream_headerlen);
+
+ if (video->stream_offset == video->stream_len) {
+ header->headerInfoUnion.headerInfoBits.endOfFrame = 1;
+ }
+ }
+ *buffer_len = offset;
+ *buffer_ready = true;
+}Usage:
- if (!g_usbd_video[busid].ep_buf0_ready) {
- remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset;
- // ... existing buf0 preparation code
- }
+ if (!g_usbd_video[busid].ep_buf0_ready) {
+ prepare_video_buffer(&g_usbd_video[busid], g_usbd_video[busid].ep_buf0,
+ &g_usbd_video[busid].ep_buf0_len,
+ &g_usbd_video[busid].ep_buf0_ready);
+ }
- if (!g_usbd_video[busid].ep_buf1_ready) {
- remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset;
- // ... existing buf1 preparation code
- }
+ if (!g_usbd_video[busid].ep_buf1_ready) {
+ prepare_video_buffer(&g_usbd_video[busid], g_usbd_video[busid].ep_buf1,
+ &g_usbd_video[busid].ep_buf1_len,
+ &g_usbd_video[busid].ep_buf1_ready);
+ }Also applies to: 846-879
chenzhihong007
force-pushed
the
master
branch
from
328ef42 to
cfe1325
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
demo/video_static_h264_template.c (2)
209-209: LGTM! Memory allocation and initialization look correct.The buffer declaration with USB_NOCACHE_RAM_SECTION and USB_MEM_ALIGNX attributes is appropriate for DMA operations. The initialization properly clears both buffers.
Consider documenting the memory requirements (80KB total) in comments, as this might be important for resource-constrained systems.
Also applies to: 217-217
Line range hint
221-226: Consider improving the busy-wait implementation.While the ping-pong buffer implementation is good for performance, the busy-wait loop could be optimized:
- No timeout mechanism could lead to infinite waiting
- No error handling for transfer failures
- CPU is blocked during waiting
Consider implementing this more robust version:
- usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_h264, sizeof(cherryusb_h264)); - while (iso_tx_busy) { - if (tx_flag == 0) { - break; - } - } + uint32_t timeout = 1000; // Adjust timeout value as needed + int ret = usbd_video_stream_start_write(busid, VIDEO_IN_EP, + &packet_buffer[0][0], &packet_buffer[1][0], + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, + (uint8_t *)cherryusb_h264, sizeof(cherryusb_h264)); + + if (ret < 0) { + // Handle transfer start error + continue; + } + + while (iso_tx_busy && timeout > 0 && tx_flag) { + // Consider adding a small delay here to reduce CPU usage + timeout--; + } + + if (timeout == 0) { + // Handle timeout error + iso_tx_busy = false; + }demo/video_static_yuyv_template.c (2)
Line range hint
220-225: Add error handling for stream write operation.While the ping-pong buffer implementation is correct, the stream write operation should include error handling to manage potential failures gracefully.
Consider applying this improvement:
- usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_yuyv, sizeof(cherryusb_yuyv)); + int ret = usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, + (uint8_t *)cherryusb_yuyv, sizeof(cherryusb_yuyv)); + if (ret < 0) { + iso_tx_busy = false; + // Add appropriate error handling (e.g., logging, retry logic) + continue; + }
Line range hint
9-9: Consider optimizing packet count for better throughput.The current implementation uses
MAX_PACKETS_IN_ONE_TRANSFER = 1, which might limit throughput, especially in high-speed USB mode. Consider increasing this value to allow multiple packets per transfer, which could better utilize the ping-pong buffer implementation and improve overall performance.Note that this would require:
- Adjusting buffer sizes accordingly
- Ensuring the USB controller supports larger transfer sizes
- Testing with different packet counts to find the optimal value for your use case
Also applies to: 211-225
class/video/usbd_video.c (1)
759-791: Consider optimizing buffer preparation logic.The function is well-structured and handles buffer preparation correctly. However, there's an opportunity to improve error handling and bounds checking.
Consider adding these safety checks:
static uint32_t usbd_video_prepare_ep_buf_data(uint8_t busid, uint32_t remain, uint8_t *ep_buf) { + if (!ep_buf || remain == 0) { + return 0; + } + struct video_payload_header *header; uint32_t len; uint32_t offset;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
class/video/usbd_video.c(3 hunks)class/video/usbd_video.h(1 hunks)demo/video_audiov1_hid_template.c(2 hunks)demo/video_static_h264_template.c(1 hunks)demo/video_static_mjpeg_template.c(2 hunks)demo/video_static_yuyv_template.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- class/video/usbd_video.h
- demo/video_audiov1_hid_template.c
- demo/video_static_mjpeg_template.c
🔇 Additional comments (3)
demo/video_static_h264_template.c (1)
Line range hint 209-226: Verify memory alignment and USB stack integration.
The ping-pong buffer implementation looks good, but let's verify:
- Memory alignment requirements for the USB controller
- Integration with the USB stack's DMA operations
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Memory alignment and USB stack integration verified successfully.
The implementation follows the correct memory alignment patterns used throughout the codebase:
- Uses
USB_NOCACHE_RAM_SECTIONandUSB_MEM_ALIGNXmacros consistently - Similar ping-pong buffer implementations found in other video templates (e.g., MJPEG, YUYV) with identical alignment attributes
- Alignment size is controlled via
CONFIG_USB_ALIGN_SIZEas defined incommon/usb_util.h
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check memory alignment requirements in USB controller definitions
rg -A 2 "USB_MEM_ALIGNX|DMA.*ALIGN"
# Check for similar ping-pong buffer implementations in other USB endpoints
ast-grep --pattern 'USB_NOCACHE_RAM_SECTION.*\[\d+\]\[\d+\]'
Length of output: 17584
demo/video_static_yuyv_template.c (1)
211-211: LGTM! Verify alignment requirements for USB DMA transfers.
The implementation of ping-pong buffering using a 2D array with proper memory section and alignment attributes looks good. This change should improve transmission performance by allowing one buffer to be filled while the other is being transmitted.
✅ Verification successful
Let me analyze the findings:
- The
USB_MEM_ALIGNXmacro is defined incommon/usb_util.has:
#define USB_MEM_ALIGNX __attribute__((aligned(CONFIG_USB_ALIGN_SIZE)))- The macro is consistently used across the codebase for DMA-capable buffers, including:
- USB control endpoint buffers
- CDC/MSC/HID class buffers
- Video/Audio streaming buffers
Let me verify the alignment size configuration:
The alignment implementation is correct and consistent with USB DMA requirements
The implementation of the packet buffer with USB_MEM_ALIGNX is properly aligned for DMA transfers:
- The macro expands to
__attribute__((aligned(4)))asCONFIG_USB_ALIGN_SIZEis defined as 4 bytes across all configurations - This 4-byte alignment is standard for USB DMA operations and is consistently used throughout the codebase for all DMA-capable buffers
- The combination of
USB_NOCACHE_RAM_SECTIONandUSB_MEM_ALIGNXensures both proper memory section placement and alignment for efficient DMA transfers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify USB_MEM_ALIGNX macro definition and usage
# Expected: Should find alignment macro definition that matches USB controller requirements
rg -A 2 "USB_MEM_ALIGNX"
Length of output: 17508
Script:
#!/bin/bash
# Check for USB alignment size configuration
rg "CONFIG_USB_ALIGN_SIZE"
Length of output: 2790
class/video/usbd_video.c (1)
21-28: LGTM! Well-structured dual buffer implementation.
The new fields in usbd_video_priv provide a clean and organized way to manage the ping-pong buffer mechanism. The naming is clear and the state tracking is comprehensive.
| if (!g_usbd_video[busid].ep_buf0_ready) { | ||
| remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset; | ||
| if (remain == 0) { | ||
| g_usbd_video[busid].stream_frameid ^= 1; | ||
| g_usbd_video[busid].stream_finish = true; | ||
| } else { | ||
| g_usbd_video[busid].ep_buf0_len = usbd_video_prepare_ep_buf_data(busid, remain, g_usbd_video[busid].ep_buf0); | ||
| g_usbd_video[busid].ep_buf0_ready = true; | ||
| if (!g_usbd_video[busid].ep_buf1_ready) { | ||
| g_usbd_video[busid].ep_buf_idx = 0; | ||
| usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (g_usbd_video[busid].stream_offset == g_usbd_video[busid].stream_len) { | ||
| header->headerInfoUnion.headerInfoBits.endOfFrame = 1; | ||
| if (!g_usbd_video[busid].ep_buf1_ready) { | ||
| remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset; | ||
| if (remain == 0) { | ||
| g_usbd_video[busid].stream_frameid ^= 1; | ||
| g_usbd_video[busid].stream_finish = true; | ||
| } else { | ||
| g_usbd_video[busid].ep_buf1_len = usbd_video_prepare_ep_buf_data(busid, remain, g_usbd_video[busid].ep_buf1); | ||
| g_usbd_video[busid].ep_buf1_ready = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated buffer preparation logic.
The buffer preparation code for buf0 and buf1 is nearly identical. This duplication could lead to maintenance issues.
Consider extracting the common logic into a helper function:
+static void prepare_buffer(struct usbd_video_priv *video, uint8_t *buf,
+ uint32_t *buf_len, bool *buf_ready, uint8_t ep,
+ bool check_other_buffer) {
+ uint32_t remain = video->stream_len - video->stream_offset;
+ if (remain == 0) {
+ video->stream_frameid ^= 1;
+ video->stream_finish = true;
+ } else {
+ *buf_len = usbd_video_prepare_ep_buf_data(busid, remain, buf);
+ *buf_ready = true;
+ if (check_other_buffer && !other_buffer_ready) {
+ video->ep_buf_idx = buffer_index;
+ usbd_ep_start_write(busid, ep, buf, *buf_len);
+ }
+ }
+}Usage:
- if (!g_usbd_video[busid].ep_buf0_ready) {
- remain = g_usbd_video[busid].stream_len - g_usbd_video[busid].stream_offset;
- if (remain == 0) {
- g_usbd_video[busid].stream_frameid ^= 1;
- g_usbd_video[busid].stream_finish = true;
- } else {
- g_usbd_video[busid].ep_buf0_len = usbd_video_prepare_ep_buf_data(busid, remain, g_usbd_video[busid].ep_buf0);
- g_usbd_video[busid].ep_buf0_ready = true;
- if (!g_usbd_video[busid].ep_buf1_ready) {
- g_usbd_video[busid].ep_buf_idx = 0;
- usbd_ep_start_write(busid, ep, g_usbd_video[busid].ep_buf0, g_usbd_video[busid].ep_buf0_len);
- }
- }
- }
+ if (!g_usbd_video[busid].ep_buf0_ready) {
+ prepare_buffer(&g_usbd_video[busid], g_usbd_video[busid].ep_buf0,
+ &g_usbd_video[busid].ep_buf0_len,
+ &g_usbd_video[busid].ep_buf0_ready,
+ ep, true);
+ }Committable suggestion skipped: line range outside the PR's diff.
class/video/usbd_video.c
Outdated
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | ||
| { | ||
| if (usb_device_is_configured(busid) == 0) { | ||
| if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) { | ||
| return -1; | ||
| } | ||
|
|
||
| g_usbd_video[busid].ep_buffer = ep_buffer; | ||
| g_usbd_video[busid].ep_buf0 = ep_buf0; | ||
| g_usbd_video[busid].ep_buf1 = ep_buf1; | ||
| g_usbd_video[busid].ep_buf0_ready = false; | ||
| g_usbd_video[busid].ep_buf1_ready = false; | ||
| g_usbd_video[busid].ep_buf_idx = 0; | ||
| g_usbd_video[busid].stream_finish = false; | ||
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | ||
| g_usbd_video[busid].stream_buf = stream_buf; | ||
| g_usbd_video[busid].stream_len = stream_len; | ||
| g_usbd_video[busid].stream_offset = 0; | ||
|
|
||
| usbd_video_stream_split_transfer(busid, ep); | ||
| return 0; | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive input validation.
While the function performs basic checks, it should validate all input parameters to ensure robust operation.
Consider adding these validations:
int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len)
{
- if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) {
+ if ((usb_device_is_configured(busid) == 0) ||
+ (stream_len == 0) ||
+ !ep_buf0 || !ep_buf1 || !stream_buf ||
+ ep_bufsize < g_usbd_video[busid].probe.dwMaxPayloadTransferSize) {
return -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | |
| { | |
| if (usb_device_is_configured(busid) == 0) { | |
| if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) { | |
| return -1; | |
| } | |
| g_usbd_video[busid].ep_buffer = ep_buffer; | |
| g_usbd_video[busid].ep_buf0 = ep_buf0; | |
| g_usbd_video[busid].ep_buf1 = ep_buf1; | |
| g_usbd_video[busid].ep_buf0_ready = false; | |
| g_usbd_video[busid].ep_buf1_ready = false; | |
| g_usbd_video[busid].ep_buf_idx = 0; | |
| g_usbd_video[busid].stream_finish = false; | |
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | |
| g_usbd_video[busid].stream_buf = stream_buf; | |
| g_usbd_video[busid].stream_len = stream_len; | |
| g_usbd_video[busid].stream_offset = 0; | |
| usbd_video_stream_split_transfer(busid, ep); | |
| return 0; | |
| } | |
| \ No newline at end of file | |
| } | |
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | |
| { | |
| if ((usb_device_is_configured(busid) == 0) || | |
| (stream_len == 0) || | |
| !ep_buf0 || !ep_buf1 || !stream_buf || | |
| ep_bufsize < g_usbd_video[busid].probe.dwMaxPayloadTransferSize) { | |
| return -1; | |
| } | |
| g_usbd_video[busid].ep_buf0 = ep_buf0; | |
| g_usbd_video[busid].ep_buf1 = ep_buf1; | |
| g_usbd_video[busid].ep_buf0_ready = false; | |
| g_usbd_video[busid].ep_buf1_ready = false; | |
| g_usbd_video[busid].ep_buf_idx = 0; | |
| g_usbd_video[busid].stream_finish = false; | |
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | |
| g_usbd_video[busid].stream_buf = stream_buf; | |
| g_usbd_video[busid].stream_len = stream_len; | |
| g_usbd_video[busid].stream_offset = 0; | |
| usbd_video_stream_split_transfer(busid, ep); | |
| return 0; | |
| } |
- use pingpang buffer to improve tx performance Signed-off-by: Zhihong Chen <[email protected]>
chenzhihong007
force-pushed
the
master
branch
from
cfe1325 to
e8ae0ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
demo/video_static_h264_template.c (2)
209-209: Consider dynamic buffer sizing based on MAX_PAYLOAD_SIZEThe static allocation of 40KB per buffer might be excessive for some configurations. Consider calculating the buffer size based on MAX_PAYLOAD_SIZE and MAX_PACKETS_IN_ONE_TRANSFER:
-USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][40 * 1024]; +#define BUFFER_SIZE (MAX_PAYLOAD_SIZE * MAX_PACKETS_IN_ONE_TRANSFER) +USB_NOCACHE_RAM_SECTION USB_MEM_ALIGNX uint8_t packet_buffer[2][BUFFER_SIZE];This would make the code more maintainable and prevent potential waste of memory in resource-constrained embedded systems.
Also applies to: 217-217
Line range hint
221-226: Consider adding explicit buffer switching mechanismWhile the ping-pong buffer implementation is a good performance improvement, the current implementation could benefit from:
- Explicit buffer switching logic to ensure proper alternation
- Non-blocking wait mechanism to improve system responsiveness
+static uint8_t current_buffer = 0; while (1) { if (tx_flag) { iso_tx_busy = true; - usbd_video_stream_start_write(busid, VIDEO_IN_EP, &packet_buffer[0][0], &packet_buffer[1][0], MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, (uint8_t *)cherryusb_h264, sizeof(cherryusb_h264)); + usbd_video_stream_start_write(busid, VIDEO_IN_EP, + &packet_buffer[current_buffer][0], + &packet_buffer[!current_buffer][0], + MAX_PACKETS_IN_ONE_TRANSFER * MAX_PAYLOAD_SIZE, + (uint8_t *)cherryusb_h264, + sizeof(cherryusb_h264)); + current_buffer = !current_buffer; while (iso_tx_busy) { + /* Add a small delay or yield to prevent tight spinning */ + usbd_sleep_ms(1); if (tx_flag == 0) { break; } } } }demo/video_audiov1_hid_template.c (1)
Line range hint
483-488: Consider adding timeout and error handling.While the implementation correctly uses the ping-pong buffers, the while loop could be improved with a timeout mechanism to prevent potential deadlocks.
- while (video_iso_tx_busy) { - if (video_tx_flag == 0) { - break; - } - } + uint32_t timeout = 1000; // Adjust timeout value as needed + while (video_iso_tx_busy && timeout > 0) { + if (video_tx_flag == 0) { + break; + } + timeout--; + // Add small delay if needed + } + if (timeout == 0) { + // Handle timeout error + USB_LOG_RAW("Video transfer timeout\r\n"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
class/video/usbd_video.c(3 hunks)class/video/usbd_video.h(1 hunks)demo/video_audiov1_hid_template.c(2 hunks)demo/video_static_h264_template.c(1 hunks)demo/video_static_mjpeg_template.c(2 hunks)demo/video_static_yuyv_template.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- class/video/usbd_video.h
- demo/video_static_mjpeg_template.c
- demo/video_static_yuyv_template.c
🔇 Additional comments (5)
demo/video_static_h264_template.c (1)
Line range hint 209-226: Verify buffer synchronization with USB endpoints
The ping-pong buffer implementation needs verification across different USB speeds and transfer scenarios:
Additionally, consider:
- Adding debug assertions to verify buffer switching correctness
- Documenting the minimum buffer size requirements for different USB speeds
- Adding stress test scenarios to the test suite
✅ Verification successful
Let me gather more information about the buffer synchronization implementation.
Based on the analysis of the shell script results, I can now provide a final response:
Buffer synchronization implementation is properly handled across USB speeds
The implementation correctly handles buffer synchronization:
- Proper payload size configuration for both HS (1024 bytes) and FS (1020 bytes) modes
- Synchronized buffer switching using
iso_tx_busyflag - Appropriate memory alignment and cache settings with
USB_NOCACHE_RAM_SECTIONandUSB_MEM_ALIGNX - Clean state management through event handlers that reset
iso_tx_busyflag
The ping-pong buffer implementation is safe because:
- Buffer sizes are properly defined based on USB speed
- Transfer completion is properly tracked via
iso_tx_busy - Buffer synchronization is maintained through the event-driven architecture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential buffer synchronization issues in the codebase
# Look for related endpoint configuration
rg -l "usbd_video_stream_start_write.*packet_buffer" .
# Check for buffer size definitions across different USB speeds
rg -A 2 "CONFIG_USB_HS.*MAX_PAYLOAD_SIZE" .
# Look for potential race conditions in buffer handling
ast-grep --pattern 'iso_tx_busy = $value'
Length of output: 291
Script:
#!/bin/bash
# Check the implementation details across video templates
# Look for buffer size definitions and endpoint configurations
rg -A 5 "MAX_PAYLOAD_SIZE|MAX_PACKETS_IN_ONE_TRANSFER" .
# Check how iso_tx_busy is used in the codebase
rg -B 2 -A 2 "iso_tx_busy" .
# Look for USB speed configurations and their handling
rg -A 3 "CONFIG_USB_(HS|FS)" .
# Check for any existing synchronization mechanisms
rg -A 3 "usbd_video_stream.*callback" .
Length of output: 44792
demo/video_audiov1_hid_template.c (1)
272-272: LGTM! Verify memory alignment requirements.
The change to a 2D array for ping-pong buffering is well-structured and maintains the required memory attributes (USB_NOCACHE_RAM_SECTION, USB_MEM_ALIGNX).
✅ Verification successful
Based on the search results, I can see that:
-
The
USB_NOCACHE_RAM_SECTIONmacro is consistently defined across different configurations to place variables in non-cacheable memory using the.noncacheablesection attribute. -
The
USB_MEM_ALIGNXmacro is defined incommon/usb_util.has:
#define USB_MEM_ALIGNX __attribute__((aligned(CONFIG_USB_ALIGN_SIZE)))-
Similar buffer declarations across multiple demo files (video, audio, HID) follow the same pattern of using both attributes to ensure proper memory alignment and cache behavior.
-
The change to a 2D array maintains these critical attributes while adding ping-pong buffering capability.
Given this evidence, I can generate the final response:
✓ Memory alignment and cache attributes are correctly maintained
The buffer declaration properly uses both required attributes:
USB_NOCACHE_RAM_SECTION: Places the buffer in non-cacheable memory sectionUSB_MEM_ALIGNX: Ensures proper alignment based on USB configuration
The change to 2D array preserves these critical memory requirements while enabling ping-pong buffering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify memory alignment macros are properly defined
rg -A 2 "USB_MEM_ALIGNX|USB_NOCACHE_RAM_SECTION" --type c
Length of output: 21906
class/video/usbd_video.c (3)
21-28: LGTM: Well-structured dual buffer management fields.
The new fields in usbd_video_priv are well-organized and provide a solid foundation for implementing the ping-pong buffer mechanism.
759-791: LGTM: Well-implemented buffer preparation logic.
The function correctly handles video payload preparation, including headers and data copying. The use of MIN macro ensures safe length calculations.
The previous review comment about extracting this into a shared helper function is still valid. This would help reduce code duplication in the buffer preparation blocks.
822-833: Consider adding synchronization for buffer state changes.
While the ping-pong buffer switching logic is well-implemented, there's a potential race condition if both buffers become ready simultaneously. Consider adding atomic operations or a mutex to protect the buffer state changes.
Also applies to: 836-849, 851-858
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | ||
| { | ||
| if (usb_device_is_configured(busid) == 0) { | ||
| if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) { | ||
| return -1; | ||
| } | ||
|
|
||
| g_usbd_video[busid].ep_buffer = ep_buffer; | ||
| g_usbd_video[busid].ep_buf0 = ep_buf0; | ||
| g_usbd_video[busid].ep_buf1 = ep_buf1; | ||
| g_usbd_video[busid].ep_buf0_ready = false; | ||
| g_usbd_video[busid].ep_buf1_ready = false; | ||
| g_usbd_video[busid].ep_buf_idx = 0; | ||
| g_usbd_video[busid].stream_finish = false; | ||
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | ||
| g_usbd_video[busid].stream_buf = stream_buf; | ||
| g_usbd_video[busid].stream_len = stream_len; | ||
| g_usbd_video[busid].stream_offset = 0; | ||
|
|
||
| usbd_video_stream_split_transfer(busid, ep); | ||
| return 0; | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input parameter validation.
While basic validation is present, consider adding these checks:
- Validate that ep_buf0 and ep_buf1 are not NULL
- Ensure ep_bufsize is sufficient for dwMaxPayloadTransferSize
- Verify stream_buf is not NULL
Apply this diff to improve validation:
int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len)
{
- if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) {
+ if ((usb_device_is_configured(busid) == 0) ||
+ (stream_len == 0) ||
+ !ep_buf0 || !ep_buf1 || !stream_buf ||
+ ep_bufsize < g_usbd_video[busid].probe.dwMaxPayloadTransferSize) {
return -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | |
| { | |
| if (usb_device_is_configured(busid) == 0) { | |
| if ((usb_device_is_configured(busid) == 0) || (stream_len == 0)) { | |
| return -1; | |
| } | |
| g_usbd_video[busid].ep_buffer = ep_buffer; | |
| g_usbd_video[busid].ep_buf0 = ep_buf0; | |
| g_usbd_video[busid].ep_buf1 = ep_buf1; | |
| g_usbd_video[busid].ep_buf0_ready = false; | |
| g_usbd_video[busid].ep_buf1_ready = false; | |
| g_usbd_video[busid].ep_buf_idx = 0; | |
| g_usbd_video[busid].stream_finish = false; | |
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | |
| g_usbd_video[busid].stream_buf = stream_buf; | |
| g_usbd_video[busid].stream_len = stream_len; | |
| g_usbd_video[busid].stream_offset = 0; | |
| usbd_video_stream_split_transfer(busid, ep); | |
| return 0; | |
| } | |
| \ No newline at end of file | |
| } | |
| int usbd_video_stream_start_write(uint8_t busid, uint8_t ep, uint8_t *ep_buf0, uint8_t *ep_buf1, uint32_t ep_bufsize, uint8_t *stream_buf, uint32_t stream_len) | |
| { | |
| if ((usb_device_is_configured(busid) == 0) || | |
| (stream_len == 0) || | |
| !ep_buf0 || !ep_buf1 || !stream_buf || | |
| ep_bufsize < g_usbd_video[busid].probe.dwMaxPayloadTransferSize) { | |
| return -1; | |
| } | |
| g_usbd_video[busid].ep_buf0 = ep_buf0; | |
| g_usbd_video[busid].ep_buf1 = ep_buf1; | |
| g_usbd_video[busid].ep_buf0_ready = false; | |
| g_usbd_video[busid].ep_buf1_ready = false; | |
| g_usbd_video[busid].ep_buf_idx = 0; | |
| g_usbd_video[busid].stream_finish = false; | |
| g_usbd_video[busid].max_packets = ep_bufsize / g_usbd_video[busid].probe.dwMaxPayloadTransferSize; | |
| g_usbd_video[busid].stream_buf = stream_buf; | |
| g_usbd_video[busid].stream_len = stream_len; | |
| g_usbd_video[busid].stream_offset = 0; | |
| usbd_video_stream_split_transfer(busid, ep); | |
| return 0; | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation