Skip to content

Commit 76f06a1

Browse files
committed
docs: Add detailed analysis of interrupt handling behavioral change.
Document the behavioral difference between original and optimized implementations when interrupt character (Ctrl-C) is detected: Original: Continued processing remaining bytes from USB buffer after interrupt New: Discards bytes in same packet as interrupt, preserves subsequent packets Includes: - Side-by-side code comparison - Example scenarios showing the difference - Analysis of which behavior is correct for REPL usage - USB packet timing considerations - Recommendation for PR review with suggested PR note This thorough documentation ensures the behavioral change is explicitly acknowledged and justified for upstream review. Signed-off-by: Andrew Leech <[email protected]>
1 parent d903a03 commit 76f06a1

File tree

1 file changed

+168
-1
lines changed

1 file changed

+168
-1
lines changed

docs/stdin_serial_analysis.md

Lines changed: 168 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,7 @@ On 2025-11-29, implemented bulk operations in `tud_cdc_rx_cb()` (shared/tinyusb/
739739
- Uses 64-byte temporary buffer for efficient USB packet processing
740740
- Maintains interrupt character behavior (clears ringbuf, schedules interrupt)
741741

742-
**Behavioral change:** The original code had a comment "// and stop" after scheduling the interrupt, but didn't actually stop - it continued processing remaining bytes into the (now cleared) ringbuf. The new implementation correctly stops processing when interrupt character is detected, discarding remaining buffered bytes. This matches the documented intent and expected behavior when user presses Ctrl-C.
742+
**Behavioral change:** See detailed analysis in "Interrupt Character Handling Behavior Change" section below.
743743

744744
**Test Device:**
745745
- Raspberry Pi Pico (RP2040)
@@ -831,3 +831,170 @@ The test results confirm:
831831
- Keyboard interrupt handling (Ctrl-C) works as expected with bulk operations
832832
- No regressions introduced by the optimization
833833
- All core Python functionality operates correctly with the new implementation
834+
835+
## Interrupt Character Handling Behavior Change
836+
837+
**IMPORTANT:** This optimization introduces a behavioral change in how data after an interrupt character (Ctrl-C) is handled. This section documents the change for PR review consideration.
838+
839+
### Original Behavior (Before Optimization)
840+
841+
```c
842+
void tud_cdc_rx_cb(uint8_t itf) {
843+
cdc_itf_pending &= ~(1 << itf);
844+
for (uint32_t bytes_avail = tud_cdc_n_available(itf); bytes_avail > 0; --bytes_avail) {
845+
if (ringbuf_free(&stdin_ringbuf)) {
846+
int data_char = tud_cdc_read_char();
847+
#if MICROPY_KBD_EXCEPTION
848+
if (data_char == mp_interrupt_char) {
849+
// Clear the ring buffer
850+
stdin_ringbuf.iget = stdin_ringbuf.iput = 0;
851+
// and stop <-- COMMENT SAYS "and stop"
852+
mp_sched_keyboard_interrupt();
853+
} else {
854+
ringbuf_put(&stdin_ringbuf, data_char);
855+
}
856+
#endif
857+
} else {
858+
cdc_itf_pending |= (1 << itf);
859+
return;
860+
}
861+
}
862+
// NO code here to mark interface as pending
863+
}
864+
```
865+
866+
**What actually happened:**
867+
1. Loop initializes `bytes_avail` with `tud_cdc_n_available(itf)` - all bytes in USB buffer
868+
2. When interrupt char found: clears ringbuf, schedules interrupt
869+
3. **Loop continues** decrementing `bytes_avail` and reading remaining bytes
870+
4. Remaining bytes from USB buffer are placed into the (now cleared) ringbuf
871+
5. Those bytes would be processed by the next Python operation
872+
873+
**Example scenario:**
874+
```
875+
USB buffer contains: "for i in range(100): print(i)\x03print('hello')\n"
876+
^ Ctrl-C here
877+
878+
After processing:
879+
- "for i in range..." is cleared from ringbuf
880+
- Interrupt scheduled
881+
- "print('hello')\n" is added to the cleared ringbuf
882+
- Next REPL prompt sees "print('hello')\n" and executes it
883+
```
884+
885+
### New Behavior (After Optimization)
886+
887+
```c
888+
void tud_cdc_rx_cb(uint8_t itf) {
889+
cdc_itf_pending &= ~(1 << itf);
890+
uint8_t temp[64];
891+
892+
while (tud_cdc_n_available(itf) > 0 && ringbuf_free(&stdin_ringbuf) > 0) {
893+
uint32_t chunk = /* calculate chunk size */;
894+
uint32_t got = tud_cdc_n_read(itf, temp, chunk);
895+
896+
#if MICROPY_KBD_EXCEPTION
897+
if (mp_interrupt_char >= 0) {
898+
uint8_t *intr_pos = memchr(temp, mp_interrupt_char, got);
899+
if (intr_pos != NULL) {
900+
size_t pre_len = intr_pos - temp;
901+
if (pre_len > 0) {
902+
ringbuf_put_bytes(&stdin_ringbuf, temp, pre_len);
903+
}
904+
stdin_ringbuf.iget = stdin_ringbuf.iput = 0;
905+
mp_sched_keyboard_interrupt();
906+
907+
// Mark interface as pending for subsequent data
908+
if (tud_cdc_n_available(itf) > 0) {
909+
cdc_itf_pending |= (1 << itf);
910+
}
911+
return; // <-- ACTUALLY STOPS
912+
}
913+
}
914+
#endif
915+
916+
ringbuf_put_bytes(&stdin_ringbuf, temp, got);
917+
}
918+
919+
if (tud_cdc_n_available(itf) > 0) {
920+
cdc_itf_pending |= (1 << itf);
921+
}
922+
}
923+
```
924+
925+
**What now happens:**
926+
1. Reads up to 64 bytes from USB buffer into temp
927+
2. When interrupt char found: copies bytes before it, clears ringbuf, schedules interrupt
928+
3. **Returns immediately** - bytes after interrupt char in temp buffer are discarded
929+
4. Marks interface as pending if more data in USB buffer (for subsequent packets)
930+
931+
**Same example scenario:**
932+
```
933+
USB buffer packet 1: "for i in range(100): print(i)\x03print('hello')\n"
934+
^ Ctrl-C here
935+
936+
After processing packet 1:
937+
- "for i in range..." is cleared from ringbuf
938+
- Interrupt scheduled
939+
- "print('hello')\n" is DISCARDED (in same packet as Ctrl-C)
940+
- Interface marked as pending (in case more packets arrived)
941+
942+
If user then types new commands:
943+
USB buffer packet 2: "print('world')\n" (arrives after Ctrl-C)
944+
- Processed normally in next tud_cdc_rx_cb() call
945+
- Added to ringbuf and executed at next REPL prompt
946+
```
947+
948+
### Analysis: Which Behavior is Correct?
949+
950+
**Arguments for new behavior (discard bytes after interrupt in same packet):**
951+
952+
1. **Matches documented intent:** Original code had comment "// and stop" but didn't actually stop
953+
2. **Expected interrupt semantics:** When user presses Ctrl-C, they intend to abort the current operation and discard pending input
954+
3. **Consistency with paste abort:** If interrupting a paste operation, remaining pasted data should be discarded
955+
4. **Prevents confusion:** Old behavior could cause unexpected commands to execute after interrupt
956+
5. **Preserves interactive typing:** Data in subsequent USB packets (new keystrokes) is still processed via `cdc_itf_pending`
957+
958+
**Arguments for old behavior (preserve bytes after interrupt):**
959+
960+
1. **Existing behavior:** Some users might depend on current behavior
961+
2. **Data preservation:** In theory, no data loss from USB buffer
962+
963+
**USB packet timing considerations:**
964+
965+
For interactive REPL use:
966+
- Individual keystrokes typically arrive in separate USB packets (1-16ms apart)
967+
- By the time user types new commands after pressing Ctrl-C, those are in new packets
968+
- New packets trigger new `tud_cdc_rx_cb()` calls via `cdc_itf_pending` mechanism
969+
- Therefore: **Interactive typing after Ctrl-C is NOT lost**
970+
971+
Data that IS discarded:
972+
- Only bytes in the **same USB packet** as the interrupt character
973+
- Typical case: bulk paste operation where Ctrl-C appears mid-stream
974+
- This is the desired behavior for aborting a paste
975+
976+
### Implementation Details
977+
978+
**Three commits implement this change:**
979+
980+
1. **7735584e7e** - Initial bulk operations, includes behavioral change
981+
2. **9b2eed6174** - Adds `mp_interrupt_char >= 0` check for disabled interrupts
982+
3. **d903a03435** - Adds `cdc_itf_pending` marking to preserve subsequent packets
983+
984+
The final implementation ensures:
985+
- ✅ Bytes after Ctrl-C in same packet are discarded (interrupt semantics)
986+
- ✅ Bytes in subsequent packets are preserved via polling (interactive typing preserved)
987+
- ✅ No data loss for normal REPL interaction patterns
988+
989+
### Recommendation for PR Review
990+
991+
The new behavior appears more correct for REPL usage:
992+
1. Matches the documented intent ("// and stop")
993+
2. Provides expected interrupt semantics
994+
3. Doesn't lose interactive typing (preserved via `cdc_itf_pending`)
995+
4. Only discards data that should be discarded (paste data after abort)
996+
997+
However, this should be explicitly called out in the PR description for maintainer review, as it is a behavioral change that could theoretically affect user workflows that depend on the old behavior.
998+
999+
**Suggested PR note:**
1000+
> **Behavioral Change:** When interrupt character (Ctrl-C) is detected, the implementation now discards remaining bytes in the current USB packet rather than buffering them. This matches the "// and stop" comment in the original code but changes the actual behavior. Interactive typing after Ctrl-C is preserved via the `cdc_itf_pending` mechanism. This change provides expected interrupt semantics for aborting paste operations while maintaining REPL usability.

0 commit comments

Comments
 (0)