diff --git a/claude_notes/usb-serial-blocking-fix-2026-01-28.md b/claude_notes/usb-serial-blocking-fix-2026-01-28.md new file mode 100644 index 0000000..fabf93b --- /dev/null +++ b/claude_notes/usb-serial-blocking-fix-2026-01-28.md @@ -0,0 +1,132 @@ +# USB Serial Blocking Issue - Root Cause and Fix + +**Date:** 2026-01-28 +**Issue:** Device blocking/hanging when USB is not connected at boot + +--- + +## Problem Description + +The device would hang or behave unpredictably when booted without USB connected. This was traced to improper Serial handling on ESP32-C3 with USB CDC. + +## Root Cause Analysis + +### Factor A: `checkForFlashCommand()` Called Without Serial Initialization + +The most critical issue was in `checkForFlashCommand()`, which is called at the start of every `loop()` iteration: + +```cpp +void loop() { + checkForFlashCommand(); // Called EVERY loop iteration + // ... +} + +void checkForFlashCommand() { + while (Serial.available()) { // Called even when Serial.begin() was never called! + char c = Serial.read(); + // ... + } +} +``` + +When USB is not connected at boot, `Serial.begin()` is never called. Then in `loop()`, `checkForFlashCommand()` calls `Serial.available()` and `Serial.read()` on an uninitialized Serial object. On ESP32-C3 with USB CDC, this causes undefined behavior or blocking. + +### Factor B: Removed `while (!Serial)` Wait Loop + +The upstream 0.16.0 code included a 3-second wait loop after `Serial.begin()`: + +```cpp +if (isUsbConnected()) { + Serial.begin(115200); + unsigned long start = millis(); + while (!Serial && (millis() - start) < 3000) { + delay(10); + } +} +``` + +This wait loop was removed in an earlier attempt to fix boot delays, but it may be necessary for proper USB CDC initialization. + +### Factor C: `Serial.setTxTimeoutMs(0)` Added Too Early + +`Serial.setTxTimeoutMs(0)` was added immediately after `Serial.begin()` to make TX non-blocking. However, calling this before the Serial connection is fully established may interfere with USB CDC initialization. + +--- + +## The Fix + +### 1. Guard `checkForFlashCommand()` with Serial Check + +```cpp +void checkForFlashCommand() { + if (!Serial) return; // Early exit if Serial not initialized + while (Serial.available()) { + // ... rest unchanged + } +} +``` + +### 2. Restore Upstream Serial Initialization Pattern + +```cpp +void setup() { + t1 = millis(); + + // Only start serial if USB connected + pinMode(UART0_RXD, INPUT); + if (isUsbConnected()) { + Serial.begin(115200); + // Wait up to 3 seconds for Serial to be ready to catch early logs + unsigned long start = millis(); + while (!Serial && (millis() - start) < 3000) { + delay(10); + } + } + // ... rest of setup +} +``` + +### 3. Remove `Serial.setTxTimeoutMs(0)` + +This call was removed entirely as it's not present in upstream and may cause issues. + +### 4. Remove Unnecessary `if (Serial)` Guards + +The 15 `if (Serial)` guards added to `EpubReaderActivity.cpp` were removed. `Serial.printf()` is safe to call when Serial isn't initialized (it simply returns 0), so guards around output calls are unnecessary. + +**Key distinction:** +- `Serial.printf()` / `Serial.println()` - Safe without guards (no-op when not initialized) +- `Serial.available()` / `Serial.read()` - **MUST** be guarded (undefined behavior when not initialized) + +--- + +## Files Changed + +| File | Change | +|------|--------| +| `src/main.cpp` | Removed `Serial.setTxTimeoutMs(0)`, restored `while (!Serial)` wait, added guard to `checkForFlashCommand()` | +| `src/activities/reader/EpubReaderActivity.cpp` | Removed all 15 `if (Serial)` guards | + +--- + +## Testing Checklist + +After applying fixes, verify: + +1. ✅ Boot with USB connected, serial monitor open - should work +2. ✅ Boot with USB connected, NO serial monitor - should work (3s delay then continue) +3. ✅ Boot without USB - should work immediately (no blocking) +4. ✅ Sleep without USB, plug in USB during sleep, wake - should work +5. ✅ Sleep with USB, unplug during sleep, wake - should work + +--- + +## Lessons Learned + +1. **Always guard Serial input operations**: `Serial.available()` and `Serial.read()` must be guarded with `if (Serial)` or `if (!Serial) return` when Serial initialization is conditional. + +2. **Serial output is safe without guards**: `Serial.printf()` and similar output functions are safe to call even when Serial is not initialized - they simply return 0. + +3. **Don't remove initialization waits without understanding why they exist**: The `while (!Serial)` wait loop exists for proper USB CDC initialization and shouldn't be removed without careful testing. + +4. **Upstream patterns exist for a reason**: When diverging from upstream behavior, especially around low-level hardware initialization, be extra cautious and test thoroughly. diff --git a/src/activities/reader/EpubReaderActivity.cpp b/src/activities/reader/EpubReaderActivity.cpp index d142d2a..97b6cea 100644 --- a/src/activities/reader/EpubReaderActivity.cpp +++ b/src/activities/reader/EpubReaderActivity.cpp @@ -127,9 +127,8 @@ void EpubReaderActivity::onEnter() { nextPageNumber = pageNumber; hasContentOffset = true; - if (Serial) - Serial.printf("[%lu] [ERS] Loaded progress v1: spine %d, page %d, offset %u\n", millis(), currentSpineIndex, - nextPageNumber, savedContentOffset); + Serial.printf("[%lu] [ERS] Loaded progress v1: spine %d, page %d, offset %u\n", millis(), currentSpineIndex, + nextPageNumber, savedContentOffset); } else { // Unknown version, try legacy format f.seek(0); @@ -138,9 +137,8 @@ void EpubReaderActivity::onEnter() { currentSpineIndex = data[0] + (data[1] << 8); nextPageNumber = data[2] + (data[3] << 8); hasContentOffset = false; - if (Serial) - Serial.printf("[%lu] [ERS] Loaded legacy progress (unknown version %d): spine %d, page %d\n", millis(), - version, currentSpineIndex, nextPageNumber); + Serial.printf("[%lu] [ERS] Loaded legacy progress (unknown version %d): spine %d, page %d\n", millis(), + version, currentSpineIndex, nextPageNumber); } } } else if (fileSize >= 4) { @@ -150,9 +148,8 @@ void EpubReaderActivity::onEnter() { currentSpineIndex = data[0] + (data[1] << 8); nextPageNumber = data[2] + (data[3] << 8); hasContentOffset = false; - if (Serial) - Serial.printf("[%lu] [ERS] Loaded legacy progress: spine %d, page %d\n", millis(), currentSpineIndex, - nextPageNumber); + Serial.printf("[%lu] [ERS] Loaded legacy progress: spine %d, page %d\n", millis(), currentSpineIndex, + nextPageNumber); } } f.close(); @@ -164,9 +161,8 @@ void EpubReaderActivity::onEnter() { int textSpineIndex = epub->getSpineIndexForTextReference(); if (textSpineIndex != 0) { currentSpineIndex = textSpineIndex; - if (Serial) - Serial.printf("[%lu] [ERS] Opened for first time, navigating to text reference at index %d\n", millis(), - textSpineIndex); + Serial.printf("[%lu] [ERS] Opened for first time, navigating to text reference at index %d\n", millis(), + textSpineIndex); } } @@ -639,8 +635,7 @@ void EpubReaderActivity::renderScreen() { if (!section) { const auto filepath = epub->getSpineItem(currentSpineIndex).href; - if (Serial) - Serial.printf("[%lu] [ERS] Loading file: %s, index: %d\n", millis(), filepath.c_str(), currentSpineIndex); + Serial.printf("[%lu] [ERS] Loading file: %s, index: %d\n", millis(), filepath.c_str(), currentSpineIndex); section = std::unique_ptr
(new Section(epub, currentSpineIndex, renderer)); const uint16_t viewportWidth = renderer.getScreenWidth() - orientedMarginLeft - orientedMarginRight; @@ -651,7 +646,7 @@ void EpubReaderActivity::renderScreen() { if (!section->loadSectionFile(SETTINGS.getReaderFontId(), SETTINGS.getReaderLineCompression(), SETTINGS.extraParagraphSpacing, SETTINGS.paragraphAlignment, viewportWidth, viewportHeight, SETTINGS.hyphenationEnabled)) { - if (Serial) Serial.printf("[%lu] [ERS] Cache not found, building...\n", millis()); + Serial.printf("[%lu] [ERS] Cache not found, building...\n", millis()); sectionWasReIndexed = true; // Progress bar dimensions @@ -697,12 +692,12 @@ void EpubReaderActivity::renderScreen() { if (!section->createSectionFile(SETTINGS.getReaderFontId(), SETTINGS.getReaderLineCompression(), SETTINGS.extraParagraphSpacing, SETTINGS.paragraphAlignment, viewportWidth, viewportHeight, SETTINGS.hyphenationEnabled, progressSetup, progressCallback)) { - if (Serial) Serial.printf("[%lu] [ERS] Failed to persist page data to SD\n", millis()); + Serial.printf("[%lu] [ERS] Failed to persist page data to SD\n", millis()); section.reset(); return; } } else { - if (Serial) Serial.printf("[%lu] [ERS] Cache found, skipping build...\n", millis()); + Serial.printf("[%lu] [ERS] Cache found, skipping build...\n", millis()); } // Determine the correct page to display @@ -714,9 +709,8 @@ void EpubReaderActivity::renderScreen() { // Use the offset to find the correct page const int restoredPage = section->findPageForContentOffset(savedContentOffset); section->currentPage = restoredPage; - if (Serial) - Serial.printf("[%lu] [ERS] Restored position via offset: %u -> page %d (was page %d)\n", millis(), - savedContentOffset, restoredPage, nextPageNumber); + Serial.printf("[%lu] [ERS] Restored position via offset: %u -> page %d (was page %d)\n", millis(), + savedContentOffset, restoredPage, nextPageNumber); // Clear the offset flag since we've used it hasContentOffset = false; } else { @@ -728,7 +722,7 @@ void EpubReaderActivity::renderScreen() { renderer.clearScreen(); if (section->pageCount == 0) { - if (Serial) Serial.printf("[%lu] [ERS] No pages to render\n", millis()); + Serial.printf("[%lu] [ERS] No pages to render\n", millis()); renderer.drawCenteredText(UI_12_FONT_ID, 300, "Empty chapter", true, EpdFontFamily::BOLD); renderStatusBar(orientedMarginRight, orientedMarginBottom, orientedMarginLeft); renderer.displayBuffer(); @@ -736,9 +730,8 @@ void EpubReaderActivity::renderScreen() { } if (section->currentPage < 0 || section->currentPage >= section->pageCount) { - if (Serial) - Serial.printf("[%lu] [ERS] Page out of bounds: %d (max %d)\n", millis(), section->currentPage, - section->pageCount); + Serial.printf("[%lu] [ERS] Page out of bounds: %d (max %d)\n", millis(), section->currentPage, + section->pageCount); renderer.drawCenteredText(UI_12_FONT_ID, 300, "Out of bounds", true, EpdFontFamily::BOLD); renderStatusBar(orientedMarginRight, orientedMarginBottom, orientedMarginLeft); renderer.displayBuffer(); @@ -748,7 +741,7 @@ void EpubReaderActivity::renderScreen() { { auto p = section->loadPageFromSectionFile(); if (!p) { - if (Serial) Serial.printf("[%lu] [ERS] Failed to load page from SD - clearing section cache\n", millis()); + Serial.printf("[%lu] [ERS] Failed to load page from SD - clearing section cache\n", millis()); section->clearCache(); section.reset(); return renderScreen(); @@ -756,7 +749,7 @@ void EpubReaderActivity::renderScreen() { // Handle empty pages (e.g., from malformed chapters that couldn't be parsed) if (p->elements.empty()) { - if (Serial) Serial.printf("[%lu] [ERS] Page has no content (possibly malformed chapter)\n", millis()); + Serial.printf("[%lu] [ERS] Page has no content (possibly malformed chapter)\n", millis()); renderer.drawCenteredText(UI_12_FONT_ID, 280, "Chapter content unavailable", true, EpdFontFamily::BOLD); renderer.drawCenteredText(UI_10_FONT_ID, 320, "(File may be malformed)"); renderStatusBar(orientedMarginRight, orientedMarginBottom, orientedMarginLeft); @@ -766,7 +759,7 @@ void EpubReaderActivity::renderScreen() { const auto start = millis(); renderContents(std::move(p), orientedMarginTop, orientedMarginRight, orientedMarginBottom, orientedMarginLeft); - if (Serial) Serial.printf("[%lu] [ERS] Rendered page in %dms\n", millis(), millis() - start); + Serial.printf("[%lu] [ERS] Rendered page in %dms\n", millis(), millis() - start); } // Save progress with content offset for position restoration after re-indexing @@ -782,9 +775,8 @@ void EpubReaderActivity::renderScreen() { serialization::writePod(f, contentOffset); f.close(); - if (Serial) - Serial.printf("[%lu] [ERS] Saved progress: spine %d, page %d, offset %u\n", millis(), currentSpineIndex, - section->currentPage, contentOffset); + Serial.printf("[%lu] [ERS] Saved progress: spine %d, page %d, offset %u\n", millis(), currentSpineIndex, + section->currentPage, contentOffset); } } diff --git a/src/main.cpp b/src/main.cpp index 9f2912e..6759e4d 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -134,6 +134,7 @@ void logMemoryState(const char* tag, const char* context) { static String flashCmdBuffer; void checkForFlashCommand() { + if (!Serial) return; // Early exit if Serial not initialized while (Serial.available()) { char c = Serial.read(); if (c == '\n') { @@ -457,14 +458,10 @@ bool isWakeupByPowerButton() { void setup() { t1 = millis(); - // Always initialize Serial but make it non-blocking - // This prevents Serial.printf from blocking when USB is disconnected - Serial.begin(115200); - Serial.setTxTimeoutMs(0); // Non-blocking TX - critical for USB disconnect handling - - // Only wait for Serial to be ready if USB is connected + // Only start serial if USB connected pinMode(UART0_RXD, INPUT); if (isUsbConnected()) { + Serial.begin(115200); // Wait up to 3 seconds for Serial to be ready to catch early logs unsigned long start = millis(); while (!Serial && (millis() - start) < 3000) { @@ -560,8 +557,7 @@ void loop() { const unsigned long sleepTimeoutMs = SETTINGS.getSleepTimeoutMs(); if (millis() - lastActivityTime >= sleepTimeoutMs) { - if (Serial) - Serial.printf("[%lu] [SLP] Auto-sleep triggered after %lu ms of inactivity\n", millis(), sleepTimeoutMs); + Serial.printf("[%lu] [SLP] Auto-sleep triggered after %lu ms of inactivity\n", millis(), sleepTimeoutMs); enterDeepSleep(); // This should never be hit as `enterDeepSleep` calls esp_deep_sleep_start return;