Mount a microSD card at /sd, addressing ewowi's PR #134 review

[TouchMyLight]

Adds microSD read/write as a chip-capability service on boards with flash to spare. The card mounts as a second folder in the existing File Manager tree, with no new HTTP routes and no new client UI: /api/dir and /api/file already dispatch on path, so the card gets upload, download, mkdir and recursive delete for free. A large transfer resumes a bounded chunk at a time across many ticks rather than blocking one, so a multi-minute upload never stalls rendering.

Flash: esp32p4rev1-eth +150640 B (2177776 -> 2328416), esp32s3-zero +13152 B (2219120 -> 2232272).
Account: src +1279/-51 lines | test +596/-1 lines | code findings 1620 -> 1629 (blocking call on the render path +6, complex function +6, large stack frame -3) | duplicated lines 3733 -> 3733 (+0)

Core:

• New SdCardModule (pins, format button, KiB usage gauge) and platform::sdMount/sdUnmount/sdFormat/sdCardUsed/sdCardTotal (uint64_t, so a >4 GB card is read correctly; the shared, size_t, flash-sized Progress gauge is fed KiB instead of being widened for one caller).
• platform::fsXxx (read/write/list/exists/remove/mkdir) each gained a path-prefix check: /sd routes to the card, everything else keeps going to LittleFS/the desktop root. FileManagerModule, HttpServerModule's /api/dir and /api/file, and FilesystemModule::removeTree needed no changes at all to pick the card up.
• New FsReadStream/FsWriteStream (explicit open/step/close) replace the reopen-per-chunk fsReadAt pattern for both flash and SD downloads, fixing a FAT cluster-chain re-traversal cost on every chunk.
• New SdTransfer slot pair + HttpServerModule::driveSdTransfers, called from tick20ms next to drainPreviewSend/drainStateSend: a non-blocking read/writeSome per active slot per tick, bounded to one buffer's worth, with a 10 s stall guard (a liveness check, not a size or time cap on the transfer itself). /sd uploads and downloads hand off into this instead of the flash path's blocking-but-small synchronous handler.
• esp32/platform_esp32_sdmmc.cpp: mount/unmount/format (field-by-field slot config, explicit NO_CD/NO_WP, GPIO validation before any pin-mask shift, the power-switch and LDO-domain sequencing a P4 needs) plus the file ops, written directly against FatFs (f_open/f_read/f_write) rather than through the stdio/VFS layer, to drop stdio's share of the flash cost. A write lands in a .tmp file and is renamed into place only on success (f_rename itself won't replace an existing file, so the old one is unlinked first); every failure path - write error, a stalled peer, a dropped connection - discards the .tmp instead, so a failed upload never leaves a partial file under its real name. Verified by killing a 10 MB upload mid-transfer: the card came back to exactly its prior contents.
• A real bug from live hardware testing: driveSdUpload read into a 4 KB stack-local every tick, which a multi-minute upload stacked deeply enough to panic the board (a crash a browser's fetch() reports as the opaque "NetworkError when attempting to fetch resource.", not a clue to the real cause). Fixed by reading into the SdTransfer struct's own scratch buffer instead, the same way driveSdDownload already did it. Verified with a 40 MB upload (10x the file that first crashed it): one continuous 3m50s connection, board uptime and heap unmoved, file landed byte-exact.
• hasSdCard gates on CONFIG_SOC_SDMMC_HOST_SUPPORTED and a new project Kconfig bool (MM_SD_CARD, set per-board: esp32p4rev1-eth, esp32s31, esp32s3-n16r8), not on chip capability alone - classic ESP32's SD host only works on fixed pins anyway, and the point is shipping it only where there's flash to spare. Desktop's hasSdCard is unconditionally true, backed by a real /sd subfolder, so its tests exercise the real mount/format/gauge logic.

UI:

• app.js: zero changes for the File Manager tree itself (the unified /api/dir and /api/file routes make the card appear automatically); the format button renders through the existing generic confirm-button machinery.
• A second bug from live use: the drop-upload path still applied FM_UPLOAD_CAP (256 KiB, sized for flash config files) to a /sd drop, where the server has no cap of its own beyond the card's free space - an 8.7 MB upload was rejected client-side before any request was sent. Fixed with fmIsSdPath(destDir), gating both the size check and the "not uploaded" message's wording.
• A progress bar for the upload, requested after the fix: fmDropUpload now goes through uploadWithProgress (the same XMLHttpRequest helper the firmware-flash UI already uses - fetch cannot report upload progress at all), driving a small floating, non-modal panel (one row per file, closes on completion) so a long /sd transfer is visible without blocking the rest of the page the way the firmware-update overlay deliberately does.
• style.css: .fm-upload-panel / .fm-upload-panel-row, bottom-left (the PiP preview pane already claims bottom-right), matching its fixed-position/border/shadow convention.

Scripts/MoonDeck:

• check_devices.py: "SD card" added to SUPPORTED_VOCAB.

Tests:

• unit_SdCardModule.cpp: pins visible/hidden by hasSdCard, mount/remount/release/format/tick1s-gauge round trip against a real backing folder.
• unit_HttpServerModule_sdcard.cpp: traversal rejection, a multi-chunk upload/download round trip, the stall timeout, 507 (card full), 413 (FAT32 ceiling), the root listing advertising "sd", DELETE removeTree on a non-empty SD folder.
• file-manager-sd-upload.test.mjs: fmIsSdPath's gating, a flash-bound file over cap skipped client-side, the same file bound for /sd sent instead, a dropped connection reported plainly rather than as a generic network error, a 507 surfacing the server's own message.

Docs/CI:

• docs/moonmodules/core/system.md: new "SD Card" section (pins, format, gauge, "appears under /sd in the File Manager"). Its card image reference is kept (check_docgen requires the tag even though the asset doesn't exist yet - a disclosed, unresolved gap, not a regression).
• docs/reference/hardware/gpio-usage.md, esp32-p4-eth.md: dropped the "(unimplemented)" / "MoonLight does not mount it" claims the feature now makes false.
• esp32/main/Kconfig.projbuild, three sdkconfig.defaults.*: MM_SD_CARD.
• mooninstaller/deviceModels.json: the Waveshare ESP32-P4-ETH's "SD card" moved planned -> supported, with its SdCardModule pin catalog entry.

Reviews (🐇 ewowi, PR #134):

1. Mount as /sd, delete the parallel API - done. The original commit's sdXxx public functions, /api/sddir + /api/sdfile routes, and the File Manager's separate SD panel are gone; one tree, one toggle.
2. Ship only where there's flash to spare; use FatFs directly, not stdio - done, via the new MM_SD_CARD Kconfig bool and the raw-FatFs rewrite above. The hoped-for flash saving didn't fully materialize (~240 B measured, not the ~15 KB estimated) because platform_esp32_fs.cpp's LittleFS side still links stdio regardless; said here rather than left to look like a wasted rewrite.
3. Large transfers blocking the render thread - done, as a real resumable per-tick transfer (SdTransfer + driveSdTransfers) rather than the smaller interim fallback of a size cap; no SD-specific ceiling beyond the card's own free space, which the 507 check already enforces. feedWatchdog() is gone: nothing blocks long enough to need it.
4. Cards over 4 GB - done, via the uint64_t sdCardUsed/Total above.
5. One open handle per download - done, via FsReadStream/FsWriteStream.
6. Smaller items (GPIO validation, doc fixes, positive doc-comment headings) - done; see platform_esp32_sdmmc.cpp's validOutputPin and its own "Drive 0 is assumed, not queried" / "Built field by field" sections.
7. Desktop test coverage - done: hasSdCard is unconditionally true on desktop, backed by a real subfolder, so unit_SdCardModule.cpp and the HTTP route tests exercise the real logic rather than a no-hardware stub.
8. Start again from current main - done; this branched from main after Longer MoonLive scripts, saved script settings, scaled knob rings #135 merged, with the four reusable files (platform_esp32_sdmmc.cpp, SdCardModule.h/.cpp, unit_SdCardModule.cpp) rewritten against it rather than cherry-picked, since 17 of the original 21 files had changed on main since the old branch point.

Checks:

• check_specs: PASS - 136/136 modules
• check_prose: PASS - 1 finding in added lines, a verified Vale tool artifact (isolating one changed table row without its header mis-parses it as a long sentence; a direct Vale run against the real file shows zero findings there)
• build_docs --strict: PASS
• check_docgen: PASS - 0 errors, 4314 warnings (was 4286; the new SD files' own docs, not a regression in a touched existing file)
• test_host --python: PASS - 377 passed
• check_taglines: SKIP - no front-page .md touched
• check_code: FAIL - ratchet rose (blocking call on the render path +6, complex function +6); large stack frame improved (-3). Both remaining deltas are structural, not debt: one more per-tick resumable transfer mirroring drainPreviewSend/drainStateSend's already-accepted shape, and one more periodic gauge read mirroring filesystemUsed's. Already simplified what this change touched (driveSdTransfers split into driveSdUpload/driveSdDownload, handleWriteFile's /sd leg extracted into handleWriteFileSd) before accepting the rest per coding-standards.md's own escape valve: "a function may stay over a limit when splitting it would hide a sequence that belongs together."
• build_desktop: PASS - zero errors/warnings in touched files
• test_desktop: PASS - 2427/2427
• run_scenario: PASS - 30 scenarios, 17 passed / 13 skipped, 0 failed
• check_esp32_built: PASS - fresh after the real rebuild+flash
• build_desktop --no-jit: SKIP - MoonLive not touched
• collect_kpi: PASS - recorded above
• test_host --js: PASS - 243 passed
• check_devices: PASS - 38 devices, 0 issues
• check_firmwares: PASS - 14 variants, 0 issues
• improv_smoke_test: SKIP - provisioning path not touched
• Real hardware (esp32p4rev1-eth, over the network - serial unavailable this session): mount, File Manager discoverability, upload/download round trip, a 40 MB /sd upload completing without a crash, an interrupted upload leaving no partial file. The progress panel's wiring has its own JS tests; its on-screen appearance still wants a look in a real browser.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: MoonModules/MoonLight/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: f9266dc9-a680-4357-bf1b-24608f2d66eb

---

📥 Commits

Reviewing files that changed from the base of the PR and between 17dae99 and 745339a.

---

📒 Files selected for processing (33)

• CMakeLists.txt
• docs/moonmodules/core/system.md
• docs/reference/hardware/esp32-p4-eth.md
• docs/reference/hardware/gpio-usage.md
• docs/reference/metrics/code.md
• docs/reference/metrics/docgen.md
• docs/reference/metrics/repo-health.json
• docs/reference/metrics/repo-health.md
• esp32/main/CMakeLists.txt
• esp32/main/Kconfig.projbuild
• esp32/sdkconfig.defaults.esp32p4rev1-eth
• esp32/sdkconfig.defaults.esp32s3-n16r8
• esp32/sdkconfig.defaults.esp32s31
• moondeck/check/check_devices.py
• mooninstaller/deviceModels.json
• src/core/system/HttpServerModule.cpp
• src/core/system/HttpServerModule.h
• src/core/system/SdCardModule.cpp
• src/core/system/SdCardModule.h
• src/module_types.cpp
• src/platform/desktop/platform_config.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_fs.cpp
• src/platform/esp32/platform_esp32_sdmmc.cpp
• src/platform/esp32/platform_esp32_sdmmc.h
• src/platform/platform.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/js/file-manager-sd-upload.test.mjs
• test/unit/core/unit_HttpServerModule_sdcard.cpp
• test/unit/core/unit_SdCardModule.cpp

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[ewowi]

Thanks for the rework, this is the shape we asked for in #134: one tree under /sd inside the existing fsXxx functions, no SD routes or panel, the backend behind MM_SD_CARD, 64-bit card sizes, one handle per stream, transfers chunked per tick, and desktop-backed tests. Seven of the eight #134 points are done. What remains is local; nothing needs a redesign.

Merge order: we hold the Teensy 4.1 PR (#137) until this one is merged, so you will not get conflicts from it.

To keep this round short:

• 🔴 blocks the merge (two items).
• 🟠 yours, in your code: in this round, or a follow-up PR where it says so.
• 🟡 ours, because it needs tooling or hardware we have set up (screenshots, the ESP32 builds, the metrics): we do these, nothing needed from you.

Blocking 🔴

1. 🔴 Remount under open streams. SdCardModule::prepare() unmounts and remounts on every tree-wide prepare. That prepare runs on any affectsPrepare change anywhere in the tree (a layout width, a driver pin) and after every flash upload or delete. A transfer in flight then calls f_write on a FIL whose FATFS context esp_vfs_fat_sdcard_unmount just freed. It also costs an SDMMC re-init (100 ms or more) on the render thread for unrelated edits. The fix:
   • remember the mounted pin set and return early when it is unchanged;
   • refuse unmount and format while a stream is open, for example with an open-handle count in the platform layer, and report "busy: transfer in progress";
   • pin both with a desktop test, such as format during an active download.
2. 🔴 A 4 KB object on the main-task stack. FIL f; as a local in four routines in platform_esp32_sdmmc.cpp puts a 4 KB sector buffer on the 12 KB main-task stack. That is the crash class you already hit. Use a file-scope static FIL, single-threaded by construction, or new FIL as the streams do. The comment near driveSdTransfers describes the mechanism as accumulation across calls; it is one 4 KB frame on a 12 KB stack.

Ours 🟡

• The two ratchets rose: code findings 1620 to 1629, and docgen warnings 4286 to 4314. We accept the rise for this merge and clear it in a sweep of our own afterwards.
• S3 and S31 enable the backend but were never built: CI runs no ESP32 builds on a PR, so we build your branch for both before merging and tell you if either fails.
• SdCardModule.png is still referenced and missing: we record the card's screenshot with our capture tooling.

Yours 🟠

3. 🟠 Cost on boards without SD. SdTransfer sdTransfers_[2] (about 8 KB) and the per-tick drive run on every board, and SdCardModule boots everywhere with a format button. Register it with hasSdCard as its build flag, as WiFi uses hasWiFi, and wrap the slots and the tick call in if constexpr (platform::hasSdCard). The hidden-pin lines and the "no SD hardware" branch then go. Unset pins are an idle state, so Severity::Status fits better than Warning.
4. 🟠 An sd folder with no card. The root lists sd whenever hasSdCard is set, even with no card mounted, and an upload there then fails with 500. Gate the entry on sdCardMounted(), and answer "no card mounted" in handleWriteFileSd. On desktop, hasSdCard = true shows the folder and a Services warning to every user; a runtime test hook next to setTestSdCardTotal keeps the test seam without the product default.
5. 🟠 DELETE /api/dir?path=/sd wipes the card, then fails on ESP32 or removes the mount folder on desktop. Refuse removeTree on / and on a mount root with one rule (it also closes DELETE /api/dir?path=/ on flash), and pin it.

Yours, lower priority 🟠

6. 🟠 Two primitives for one job. fsWriteStream (pull) now sits next to the push stream, and fsReadAt has no caller in src/ anymore. Principle 3: the old one goes. fsWriteStream can become one core helper over the push primitives, and fsReadAt can go, with its tests moved to the stream.
7. 🟠 release() leaks transfers. HttpServerModule::release() does not finish active sdTransfers_, so a release mid-transfer leaks a FIL and a socket.
8. 🟠 Headers before a slot. streamFsFile writes the 200 header before it knows it has a slot. Claim the slot first, and answer 503 as the upload side does.
9. 🟠 2 GiB file sizes. fsSize is long, 32-bit on ESP32, so a file of 2 GiB or more reports a negative size. Widen it to int64_t, or note the limit.
10. 🟠 Comment contradicts code. platform_esp32_sdmmc.h says the rename on commit is the caller's, while sdCardWriteStreamClose does it itself.
11. 🟠 The /sd prefix has five homes: two isSdApiPath, requestNamesSdPath/isSdPath, and fmIsSdPath in JS. One platform::isSdPath serves the C++ sites.
12. 🟠 f_getfree on the render thread. It is fast on cards with valid FSINFO and slow on cards without it; fine for now if the header says so.
13. 🟠 Throughput (a follow-up PR is fine). One 4 KB chunk per 20 ms tick caps a transfer at about 200 KB/s. A per-tick time budget (several chunks while under a few ms) lifts it without changing the design; a follow-up is fine.

Nits 🟠

• ldoCfg.ldo_chan_id = 4 wants a name and a reason (VO4 is the P4's SDMMC IO supply).
• A few multi-line // comments say one thing (the strtoull one, the rebuildControls() one).
• Positive form in system.md and SdCardModule.h ("No panel of its own" and similar).
• Two bullets under the card in system.md are not controls.