feat: update usb bulk trans and add linux test driver#23
Conversation
WalkthroughThis update introduces a Linux kernel USB driver for bulk transfer testing, enhances the ESP32-S3 device-side example to support framed packet reception with CRC validation, and expands documentation. The previous Python test script is removed, replaced by the new Linux driver. Build instructions and .gitignore for the Linux driver are added. Changes
Sequence Diagram(s)sequenceDiagram
participant LinuxHost as Linux Host Driver
participant USB as USB Bus
participant ESP32 as ESP32-S3 Device
LinuxHost->>USB: Bulk OUT transfer (packet with header + payload)
USB->>ESP32: Delivers USB packet(s)
ESP32->>ESP32: Buffers data, parses header, checks CRC
ESP32-->>ESP32: (Optional) Logs payload if CRC matches
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
examples/usb/device/bulk_trans/linux_driver/Makefile (1)
11-20: Add phony target declarations for better Makefile practices.Declare non-file targets as phony to prevent conflicts with files of the same name.
+.PHONY: modules modules_install install clean + modules: $(MAKE) -C $(KERNEL_SOURCE_DIR) M=$(PWD) modulesexamples/usb/device/bulk_trans/linux_driver/usb.c (2)
24-38: Consider using kernel's built-in CRC16 function.You include
<linux/crc16.h>but implement your own CRC function. The kernel providescrc16()with various polynomials.-static uint16_t crc16_ccitt(const uint8_t *data, uint16_t len, uint16_t init) -{ - uint16_t crc = init; - while (len--) { - crc ^= (*data++) << 8; - for (int i = 0; i < 8; i++) { - if (crc & 0x8000) { - crc = (crc << 1) ^ 0x1021; - } else { - crc <<= 1; - } - } - } - return crc; -}Then replace the call at line 81:
- hdr->crc16 = cpu_to_le16(crc16_ccitt(tx_buf + sizeof(*hdr), total_len, 0x0000)); + hdr->crc16 = cpu_to_le16(crc_ccitt(0x0000, tx_buf + sizeof(*hdr), total_len));Note: The kernel's
crc_ccitt()function uses the same polynomial (0x1021) and provides the same result.
40-94: Unusual driver pattern - consider the implications.This driver performs a one-shot USB transfer during probe and always returns 0 (success). This means:
- The transfer happens immediately when the device is connected
- No cleanup or further interaction is possible
- The driver remains bound but inactive after the initial transfer
This pattern is acceptable for a test driver but differs from typical USB drivers that set up for ongoing communication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/usb/device/bulk_trans/README.md(1 hunks)examples/usb/device/bulk_trans/linux_driver/.gitignore(1 hunks)examples/usb/device/bulk_trans/linux_driver/Makefile(1 hunks)examples/usb/device/bulk_trans/linux_driver/usb.c(1 hunks)examples/usb/device/bulk_trans/main/bulk_trans.c(1 hunks)examples/usb/device/bulk_trans/test.py(0 hunks)
💤 Files with no reviewable changes (1)
- examples/usb/device/bulk_trans/test.py
🧰 Additional context used
🪛 checkmake (0.2.2)
examples/usb/device/bulk_trans/linux_driver/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 17-17: Target "install" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
examples/usb/device/bulk_trans/linux_driver/.gitignore (1)
1-7: LGTM!The .gitignore file correctly excludes all common Linux kernel module build artifacts.
examples/usb/device/bulk_trans/README.md (1)
3-7: Clear documentation of the Linux driver integration.The README effectively explains the packet size mismatch and encoding requirements, providing clear compilation and testing instructions.
examples/usb/device/bulk_trans/main/bulk_trans.c (2)
44-58: CRC16-CCITT implementation is correct.The CRC calculation correctly implements CRC16-CCITT with polynomial 0x1021, matching the Linux driver implementation.
60-133: Well-structured framed packet reception logic.The implementation correctly handles:
- Header parsing with length and CRC fields
- Multi-packet buffering for frames larger than endpoint size
- CRC validation after complete frame reception
- Proper state management and error handling
| obj-m := $(DRIVER_NAME).o | ||
|
|
||
| DRIVER_FILES := usb.o | ||
| CFLAGS_main.o = -O0 |
There was a problem hiding this comment.
Fix incorrect CFLAGS variable name.
The CFLAGS variable references main.o but your source file compiles to usb.o. This flag setting will have no effect.
-CFLAGS_main.o = -O0
+CFLAGS_usb.o = -O0📝 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.
| CFLAGS_main.o = -O0 | |
| -CFLAGS_main.o = -O0 | |
| +CFLAGS_usb.o = -O0 |
🤖 Prompt for AI Agents
In examples/usb/device/bulk_trans/linux_driver/Makefile at line 7, the CFLAGS
variable is incorrectly named CFLAGS_main.o, but the source file being compiled
is usb.o. Rename the variable to CFLAGS_usb.o to ensure the compiler flags apply
correctly to the usb.o compilation.
| printf("\n"); | ||
| } | ||
|
|
||
| // Reset statet |
There was a problem hiding this comment.
Fix typo in comment.
- // Reset statet
+ // Reset state🤖 Prompt for AI Agents
In examples/usb/device/bulk_trans/main/bulk_trans.c at line 126, there is a typo
in the comment "Reset statet". Correct the comment to "Reset state" to fix the
spelling mistake.
|
|
||
| int in_pipe = 0, out_pipe = 0; | ||
|
|
||
| // find input and oupt endpoint |
There was a problem hiding this comment.
Fix typo in comment.
- // find input and oupt endpoint
+ // find input and output endpoint📝 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.
| // find input and oupt endpoint | |
| // find input and output endpoint |
🤖 Prompt for AI Agents
In examples/usb/device/bulk_trans/linux_driver/usb.c at line 49, there is a typo
in the comment "find input and oupt endpoint". Correct the typo by changing
"oupt" to "output" to improve clarity and professionalism in the code comments.
| #define BULK_PACKET_MAX_SIZE 64 | ||
| #define PROTOCOL_HEADER_SIZE 6 | ||
| #define PAYLOAD_SIZE (BULK_PACKET_MAX_SIZE - PROTOCOL_HEADER_SIZE) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused macro definitions.
The macros BULK_PACKET_MAX_SIZE, PROTOCOL_HEADER_SIZE, and PAYLOAD_SIZE are defined but never used in the code. The actual header size is correctly calculated using sizeof(struct usb_packet_header).
-#define BULK_PACKET_MAX_SIZE 64
-#define PROTOCOL_HEADER_SIZE 6
-#define PAYLOAD_SIZE (BULK_PACKET_MAX_SIZE - PROTOCOL_HEADER_SIZE)
-📝 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.
| #define BULK_PACKET_MAX_SIZE 64 | |
| #define PROTOCOL_HEADER_SIZE 6 | |
| #define PAYLOAD_SIZE (BULK_PACKET_MAX_SIZE - PROTOCOL_HEADER_SIZE) |
🤖 Prompt for AI Agents
In examples/usb/device/bulk_trans/linux_driver/usb.c around lines 6 to 8, the
macros BULK_PACKET_MAX_SIZE, PROTOCOL_HEADER_SIZE, and PAYLOAD_SIZE are defined
but not used anywhere in the code. Remove these unused macro definitions to
clean up the code since the header size is already correctly calculated using
sizeof(struct usb_packet_header).
YanKE01
force-pushed
the
feat/add_driver_for_bulk_trans
branch
from
80528aa to
096fdbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
examples/usb/device/bulk_trans/linux_driver/Makefile (1)
7-7: Fix incorrect CFLAGS variable name.The CFLAGS variable references
main.obut your source file compiles tousb.o. This flag setting will have no effect.-CFLAGS_main.o = -O0 +CFLAGS_usb.o = -O0examples/usb/device/bulk_trans/main/bulk_trans.c (1)
155-155: Fix typo in comment.- // Reset statet + // Reset stateexamples/usb/device/bulk_trans/linux_driver/usb.c (2)
7-9: Remove unused macro definitions.The macros
BULK_PACKET_MAX_SIZE,PROTOCOL_HEADER_SIZE, andPAYLOAD_SIZEare defined but never used in the code. The actual header size is correctly calculated usingsizeof(struct usb_packet_header).-#define BULK_PACKET_MAX_SIZE 64 -#define PROTOCOL_HEADER_SIZE 6 -#define PAYLOAD_SIZE (BULK_PACKET_MAX_SIZE - PROTOCOL_HEADER_SIZE) -
51-51: Fix typo in comment.- // find input and oupt endpoint + // find input and output endpoint
🧹 Nitpick comments (2)
examples/usb/device/bulk_trans/linux_driver/Makefile (1)
11-21: Add missing phony target declarations.The Makefile should declare phony targets to prevent conflicts with files of the same name and improve build reliability.
+.PHONY: all modules modules_install install clean + +all: modules + modules: $(MAKE) -C $(KERNEL_SOURCE_DIR) M=$(PWD) modules modules_install: $(MAKE) -C $(KERNEL_SOURCE_DIR) M=$(PWD) modules_install install: modules_install clean: $(MAKE) -C $(KERNEL_SOURCE_DIR) M=$(PWD) cleanexamples/usb/device/bulk_trans/linux_driver/usb.c (1)
95-108: Improve error handling for receive buffer allocation.The function returns after failing to allocate the receive buffer, but the transmit operation has already completed successfully. Consider this a partial success rather than complete failure.
uint8_t *rx_buf = kzalloc(RX_BUFFER_SIZE, GFP_KERNEL); if (!rx_buf) { pr_err("Failed to allocate receive buffer\n"); - return -ENOMEM; + pr_warn("Skipping response verification due to memory allocation failure\n"); + return 0; // Transmit was successful }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/usb/device/bulk_trans/README.md(1 hunks)examples/usb/device/bulk_trans/linux_driver/.gitignore(1 hunks)examples/usb/device/bulk_trans/linux_driver/Makefile(1 hunks)examples/usb/device/bulk_trans/linux_driver/usb.c(1 hunks)examples/usb/device/bulk_trans/main/bulk_trans.c(1 hunks)examples/usb/device/bulk_trans/main/tusb/tusb_config.h(1 hunks)examples/usb/device/bulk_trans/test.py(0 hunks)
💤 Files with no reviewable changes (1)
- examples/usb/device/bulk_trans/test.py
✅ Files skipped from review due to trivial changes (1)
- examples/usb/device/bulk_trans/main/tusb/tusb_config.h
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/usb/device/bulk_trans/linux_driver/.gitignore
- examples/usb/device/bulk_trans/README.md
🧰 Additional context used
🪛 checkmake (0.2.2)
examples/usb/device/bulk_trans/linux_driver/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 17-17: Target "install" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
examples/usb/device/bulk_trans/main/bulk_trans.c (4)
14-24: LGTM! Well-structured packet header and state management.The packed header structure and static state variables are properly defined for the framed packet protocol.
44-58: LGTM! Correct CRC16-CCITT implementation.The CRC16-CCITT implementation matches the standard algorithm with polynomial 0x1021. This aligns with the Linux driver implementation for proper packet validation.
69-91: LGTM! Proper header parsing and validation.The code correctly handles header extraction, validates payload size against buffer limits, and manages frame state initialization.
128-133: Verify memory allocation error handling.The code allocates a response buffer but doesn't reset the frame state if allocation fails, which could leave the system in an inconsistent state.
Consider resetting the frame state on allocation failure:
uint8_t *response_buf = malloc(sizeof(usb_packet_header_t) + response_len); if (!response_buf) { ESP_LOGE(TAG, "Failed to allocate response buffer"); + // Reset frame state on allocation failure + frame_active = false; + received_total = 0; + expected_total = 0; + expected_crc16 = 0; return; }examples/usb/device/bulk_trans/linux_driver/usb.c (3)
26-40: LGTM! CRC implementation matches device side.The CRC16-CCITT implementation is identical to the device-side code, ensuring proper packet validation between host and device.
76-83: LGTM! Proper endianness handling.The code correctly uses
cpu_to_le16()andle16_to_cpu()for cross-platform compatibility, ensuring the USB packet format is consistent regardless of host architecture.
121-127: LGTM! Comprehensive CRC verification.The CRC verification logic properly extracts the payload, calculates the checksum, and provides clear logging for both success and failure cases.
| int ret = 0; | ||
| ret = tud_vendor_n_write(0, response_buf, sizeof(usb_packet_header_t) + response_len); | ||
| if (ret < 0) { | ||
| ESP_LOGE(TAG, "USB bulk write failed: %d", ret); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for partial USB writes.
The code checks for write failure but doesn't handle the case where tud_vendor_n_write returns a partial write (0 < ret < expected_size).
-int ret = 0;
-ret = tud_vendor_n_write(0, response_buf, sizeof(usb_packet_header_t) + response_len);
-if (ret < 0) {
- ESP_LOGE(TAG, "USB bulk write failed: %d", ret);
-}
+int expected_size = sizeof(usb_packet_header_t) + response_len;
+int ret = tud_vendor_n_write(0, response_buf, expected_size);
+if (ret < 0) {
+ ESP_LOGE(TAG, "USB bulk write failed: %d", ret);
+} else if (ret != expected_size) {
+ ESP_LOGE(TAG, "Partial USB write: %d/%d bytes", ret, expected_size);
+}📝 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 ret = 0; | |
| ret = tud_vendor_n_write(0, response_buf, sizeof(usb_packet_header_t) + response_len); | |
| if (ret < 0) { | |
| ESP_LOGE(TAG, "USB bulk write failed: %d", ret); | |
| } | |
| int expected_size = sizeof(usb_packet_header_t) + response_len; | |
| int ret = tud_vendor_n_write(0, response_buf, expected_size); | |
| if (ret < 0) { | |
| ESP_LOGE(TAG, "USB bulk write failed: %d", ret); | |
| } else if (ret != expected_size) { | |
| ESP_LOGE(TAG, "Partial USB write: %d/%d bytes", ret, expected_size); | |
| } |
🤖 Prompt for AI Agents
In examples/usb/device/bulk_trans/main/bulk_trans.c around lines 147 to 152, the
code checks for a negative return value from tud_vendor_n_write but does not
handle partial writes where the return value is positive but less than the
expected size. Update the code to detect if ret is greater than zero but less
than the total bytes intended to write, and implement appropriate error handling
or retry logic to ensure the full data is sent.
1 participant
Summary by CodeRabbit