feat(port/cherryusb): add cherryusb for usb usage, based on cdc acm#6
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds support for a CherryUSB-based USB CDC-ACM transport layer for ctshell. It includes conditional build configuration, USB descriptor setup, data transmission/reception handlers, DTR state management, and documentation. ChangesCherryUSB Port Addition
Sequence DiagramsequenceDiagram
participant Host as Host/PC
participant USB as USB Device<br/>(CherryUSB)
participant Shell as ctshell<br/>Application
Note over Host,Shell: Data Reception Path
Host->>USB: Sends data (bulk OUT)
USB->>USB: Bulk OUT endpoint handler
USB->>Shell: ctshell_input(byte)
Shell->>Shell: Process input
Note over Host,Shell: Data Transmission Path
Shell->>USB: shell_usb_write(buffer)
USB->>USB: Check DTR enabled & busy flag
USB->>USB: Copy to IN buffer
USB->>Host: Sends data (bulk IN)
Host->>Host: Receives data
Note over Host,Shell: DTR Control
Host->>USB: Sets/clears DTR
USB->>USB: usbd_cdc_acm_set_dtr(dtr_state)
USB->>Shell: Gates transmission capability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@port/cherryusb/ctshell_cherryusb.c`:
- Around line 179-191: cdc_acm_init stores the active USB bus via its busid but
shell_usb_write is still hardcoding bus 0 and truncating payloads to 2048 bytes;
update the code so shell_usb_write uses the configured busid from cdc_acm_init
(e.g., store busid in a static/global like current_bus or pass it through)
instead of 0, and remove the silent 2048-byte truncation so the function
transmits the full payload length (or fragments/sends in a loop if underlying
USB endpoint limits require it) using the write_buffer/its actual length when
calling the send/submit routines.
- Around line 116-141: The USB event handler must clear session state when the
link is lost: in usbd_event_handler handle USBD_EVENT_RESET and
USBD_EVENT_DISCONNECTED by resetting dtr_enable = false and ep_tx_busy_flag =
false (and any other per-session flags your transmit path uses) so subsequent
writes won't spin; keep the existing behavior for USBD_EVENT_CONFIGURED (start
read) but ensure you also stop/cancel or ignore pending transfers if applicable.
Apply the same changes to the other equivalent event block noted (the second
handler around lines 194-203) so both RESET and DISCONNECTED clear dtr_enable
and ep_tx_busy_flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55b8315c-ceea-4c4c-b016-2ae52f6609e7
📒 Files selected for processing (4)
CMakeLists.txtKconfigport/cherryusb/README.mdport/cherryusb/ctshell_cherryusb.c
| static void usbd_event_handler(uint8_t busid, uint8_t event) | ||
| { | ||
| switch (event) { | ||
| case USBD_EVENT_RESET: | ||
| break; | ||
| case USBD_EVENT_CONNECTED: | ||
| break; | ||
| case USBD_EVENT_DISCONNECTED: | ||
| break; | ||
| case USBD_EVENT_RESUME: | ||
| break; | ||
| case USBD_EVENT_SUSPEND: | ||
| break; | ||
| case USBD_EVENT_CONFIGURED: | ||
| ep_tx_busy_flag = false; | ||
| /* setup first out ep read transfer */ | ||
| usbd_ep_start_read(busid, CDC_OUT_EP, read_buffer, 2048); | ||
| break; | ||
| case USBD_EVENT_SET_REMOTE_WAKEUP: | ||
| break; | ||
| case USBD_EVENT_CLR_REMOTE_WAKEUP: | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
Clear the session flags on reset/disconnect.
dtr_enable is only driven from usbd_cdc_acm_set_dtr(). If the host drops off the bus after setting DTR, later writes still enter the transmit path and spin until timeout because the link state was never invalidated.
Proposed fix
-volatile bool ep_tx_busy_flag = false;
+volatile bool ep_tx_busy_flag = false;
+static volatile uint8_t dtr_enable = 0;
@@
case USBD_EVENT_RESET:
+ dtr_enable = 0;
+ ep_tx_busy_flag = false;
break;
@@
case USBD_EVENT_DISCONNECTED:
+ dtr_enable = 0;
+ ep_tx_busy_flag = false;
break;
@@
case USBD_EVENT_SUSPEND:
+ dtr_enable = 0;
+ ep_tx_busy_flag = false;
break;
@@
-volatile uint8_t dtr_enable = 0;
-
void usbd_cdc_acm_set_dtr(uint8_t busid, uint8_t intf, bool dtr)Also applies to: 194-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@port/cherryusb/ctshell_cherryusb.c` around lines 116 - 141, The USB event
handler must clear session state when the link is lost: in usbd_event_handler
handle USBD_EVENT_RESET and USBD_EVENT_DISCONNECTED by resetting dtr_enable =
false and ep_tx_busy_flag = false (and any other per-session flags your transmit
path uses) so subsequent writes won't spin; keep the existing behavior for
USBD_EVENT_CONFIGURED (start read) but ensure you also stop/cancel or ignore
pending transfers if applicable. Apply the same changes to the other equivalent
event block noted (the second handler around lines 194-203) so both RESET and
DISCONNECTED clear dtr_enable and ep_tx_busy_flag.
| void cdc_acm_init(uint8_t busid, uintptr_t reg_base) | ||
| { | ||
| const uint8_t data[10] = { 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30 }; | ||
|
|
||
| memcpy(&write_buffer[0], data, 10); | ||
| memset(&write_buffer[10], 'a', 2038); | ||
|
|
||
| usbd_desc_register(busid, cdc_descriptor); | ||
| usbd_add_interface(busid, usbd_cdc_acm_init_intf(busid, &intf0)); | ||
| usbd_add_interface(busid, usbd_cdc_acm_init_intf(busid, &intf1)); | ||
| usbd_add_endpoint(busid, &cdc_out_ep); | ||
| usbd_add_endpoint(busid, &cdc_in_ep); | ||
| usbd_initialize(busid, reg_base, usbd_event_handler); |
There was a problem hiding this comment.
Make shell_usb_write() use the configured bus and send the full payload.
Line 228 hard-codes bus 0 even though cdc_acm_init() accepts busid, and Line 220 silently truncates anything larger than 2048 bytes. That makes non-zero bus configurations fail and cuts off longer shell output.
Proposed fix
+static uint8_t cdc_busid;
+
void cdc_acm_init(uint8_t busid, uintptr_t reg_base)
{
+ cdc_busid = busid;
const uint8_t data[10] = { 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x30 };
memcpy(&write_buffer[0], data, 10);
memset(&write_buffer[10], 'a', 2038);
@@
void shell_usb_write(const char *buf, uint16_t len, void *priv)
{
if (!dtr_enable || len == 0) {
return;
}
- uint16_t send_len = (len > 2048) ? 2048 : len;
- uint32_t timeout = 0xFFFFF;
- while (ep_tx_busy_flag && timeout > 0) {
- timeout--;
- }
- if (timeout == 0) return;
- memcpy(write_buffer, buf, send_len);
- ep_tx_busy_flag = true;
- usbd_ep_start_write(0, CDC_IN_EP, write_buffer, send_len);
+ while (len > 0) {
+ uint16_t send_len = (len > sizeof(write_buffer)) ? sizeof(write_buffer) : len;
+ uint32_t timeout = 0xFFFFF;
+
+ while (ep_tx_busy_flag && timeout > 0) {
+ timeout--;
+ }
+ if (timeout == 0) {
+ return;
+ }
+
+ memcpy(write_buffer, buf, send_len);
+ ep_tx_busy_flag = true;
+ usbd_ep_start_write(cdc_busid, CDC_IN_EP, write_buffer, send_len);
+
+ buf += send_len;
+ len -= send_len;
+ }
}Also applies to: 215-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@port/cherryusb/ctshell_cherryusb.c` around lines 179 - 191, cdc_acm_init
stores the active USB bus via its busid but shell_usb_write is still hardcoding
bus 0 and truncating payloads to 2048 bytes; update the code so shell_usb_write
uses the configured busid from cdc_acm_init (e.g., store busid in a
static/global like current_bus or pass it through) instead of 0, and remove the
silent 2048-byte truncation so the function transmits the full payload length
(or fragments/sends in a loop if underlying USB endpoint limits require it)
using the write_buffer/its actual length when calling the send/submit routines.
feat(port/cherryusb): add cherryusb for usb usage, based on cdc acm
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation