Skip to content

show: savePrefs playlist blob on short-lived heap - #447

Merged
engmung merged 1 commit into
mainfrom
simone/ffat-cross-core-lock
Sep 25, 2026
Merged

engmung merged 1 commit into
mainfrom
simone/ffat-cross-core-lock

Conversation

@SimonePDA

@SimonePDA SimonePDA commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • savePrefs() builds the comma-joined playlist string in a malloc/free buffer (~2.5 KB) instead of on the loop-task stack.
  • No FatFS mutex (dropped after review).

Why

A full 64-slug list on the stack is a plausible hang/stack-pressure source. Permanent static would cost Performance DRAM permanently; tree rule is no static buffers over 1 KB.

Test plan

  • Save/load a long playlist (up to 64 slugs)
  • Sequence mode prefs still restore across reboot
  • Performance / default builds stay within footprint pin (no DRAM re-pin)

Contributed by @SimonePDA

@vercel

vercel Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
pattern-flow_origin Building Building Preview Sep 24, 2026 4:25am UTC
pattern-flow-open Ready Ready Preview Sep 24, 2026 4:25am UTC

@engmung

engmung commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Thanks for this one. I'm holding it for now: as written the lock can't do anything, and I'd rather find what actually hung.

  • loadFile() is the only place that takes PFFfat::Guard, and every path to it and to savePrefs() already runs on the loop task. /api/shows/control, /schedule and /delete all go through PFLoopSync::run. Nothing on Core 0 takes the lock: /api/status's totalBytes(), the library and uploads all skip it. So the lock never has anything to wait for. TryGuard isn't used anywhere.
  • ESP-IDF already builds FatFS with FF_FS_REENTRANT 1. FatFS itself serializes calls on the same volume from both cores; a waiter times out after CONFIG_FATFS_TIMEOUT_MS instead of corrupting anything.
  • If the hang went away with this change, I suspect the other half is what fixed it: savePrefs()'s 2.5 KB blob sitting on the loop task's stack. That part is worth keeping, but as a short-lived heap buffer (malloc/free, or PFMem) rather than static. A static costs Performance 2.8 KB of DRAM for good. The tree's rule is no static buffers over 1 KB, because one of them broke console page delivery in August.

Could you send the serial log from a hang, with any panic, backtrace or watchdog line, and say what was running at the time? Then we can fix the actual cause. I'm happy to take a trimmed PR with just the savePrefs() change.

… heap.

A 64-slug NVS string is ~2.5 KB; keep it off the stack without a permanent static (tree rule: no static buffers over 1 KB). Dropped the earlier FatFS mutex - loadFile paths already run on the loop task via PFLoopSync, and IDF FatFS is FF_FS_REENTRANT.

Co-authored-by: Cursor <cursoragent@cursor.com>
@SimonePDA SimonePDA changed the title FatFS cross-core lock for show .pfs loads show: savePrefs playlist blob on short-lived heap Sep 24, 2026
@SimonePDA

Copy link
Copy Markdown
Collaborator Author

Thanks — agreed on all three points.

Trimmed this PR to just the savePrefs() change: short-lived malloc/free for the ~2.5 KB playlist blob (not static). Removed core_ffat_lock.h and the loadFile guard.

I don't have a clean serial capture of the original hang handy; if it shows up again I'll grab panic/backtrace + what was running and open a follow-up. Happy for you to take this trimmed form.

@SimonePDA
SimonePDA force-pushed the simone/ffat-cross-core-lock branch from d00a014 to 1e3a0ad Compare September 24, 2026 04:24
@engmung
engmung merged commit 3eb4f65 into main Sep 25, 2026
8 checks passed

This branch was successfully deployed

1 active deployment
Preview – pattern-flow-open — 1e3a0ad7 Deployed Sep 24, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants