EPIC-11: allocation failures are survivable - #12
Merged
Merged
Conversation
PICO_MALLOC_PANIC defaults to 1, which turns a failed allocation into
panic("Out of memory") and defeats every library written to handle NULL.
FatFs's dir_clear() is the case that bites: creating a folder asks for up
to 32 KB and halves the request until it fits, so on this device, whose
whole heap is 23 KB in debug builds, the first request killed the RP.
Booting with a card that has no app folder was a reboot loop.
The block is ported from Booster's booster/src/CMakeLists.txt, comment
included, and must stay above pico_sdk_init(): the SDK's own malloc.c is
compiled as a subdirectory from there, so setting it later would compile
only our sources with it and do nothing.
The shipped v1.0.1beta image contains the "Out of memory" string; neither
build type contains it now. Release is 72 bytes smaller.
With the malloc panic off, FatFs returns FR_NOT_ENOUGH_CORE and settings_save returns an error where the device used to die. Both were invisible. FatFs failures on the HTTP side went through 24 copies of the same generic "500 disk_error". They now go through one write_fs_error(), which reports FR_NOT_ENOUGH_CORE as 503 insufficient_memory (retryable) and everything else as before. GEMDRIVE maps it to GEMDOS ENSMEM (-39) in Dcreate, Ddelete, Fdelete and Frename instead of EINTRN. The seven settings_save calls in emul.c ignored their result. They go through saveAppSettings(), which stops the countdown and puts "Saving the settings failed: the change was not stored." on the menu's status line, so a lost setting is visible on the ST. A failed callback registration in chandler_addCB now traces instead of returning silently; it would leave GEMDRIVE or the Runner unreachable. The audit found the remaining allocation sites already correct: the four in settings.c check and return -1, and the only right() caller falls back to the untruncated value.
…-04) settings_init guarded its flash size, offset and default-entry count with assert(), and both build types define NDEBUG, so the checks existed in no shipping firmware. They guard the erase and program ranges of the config sectors, which Booster shares. They are runtime checks now: each traces what was wrong and returns -1. Both callers already treat a negative result as "settings not initialised" and let main.c jump to Booster, so nothing else changes.
…STORY-05) DEVHOOKS_APP_HEAP_HOLD holds the given number of KB in the debug mailbox, in as many steps as the test needs, and frees it all when asked for 0 KB; swd.py app now takes payload words, so it is `swd.py app heap_hold 4`. Debug builds only. With it the firmware was squeezed to 3,528 bytes free: a folder listing and a folder creation still worked, and changing a setting from the menu reported "Error: Unable to allocate padding buffer" and put "Saving the settings failed: the change was not stored." on the ST's menu instead of panicking. Releasing the heap and saving again cleared it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth epic of v1.1.0. A failed allocation used to call
panic("Out of memory")and reboot the RP, which defeated every library written to handle NULL. The worst case: a card without the app folder made the device reboot about every 0.3 s, because creating a folder asks FatFs for 32 KB and the whole heap is 23 KB in a debug build.What changes
mallocreturns NULL instead of panicking.PICO_MALLOC_PANIC 0, ported from Booster'sbooster/src/CMakeLists.txtwith its comment, and placed abovepico_sdk_init(): the SDK's ownmalloc.cis compiled from there, so setting it later compiles only our sources with it and does nothing. The shipped v1.0.1beta image contains theOut of memorystring; neither build type contains it now.500 disk_error; they go through onewrite_fs_error(), which answers503 insufficient_memoryforFR_NOT_ENOUGH_CORE(retryable) and is unchanged otherwise. GEMDRIVE maps it to GEMDOSENSMEM(-39) inDcreate,Ddelete,FdeleteandFrenameinstead ofEINTRN.settings_savecalls inemul.cignored their result. They go throughsaveAppSettings(), which stops the countdown and puts "Saving the settings failed: the change was not stored." on the menu's status line, so a lost setting shows on the ST. A failed callback registration inchandler_addCBtraces instead of returning silently.settings_initguarded its flash size, offset and default-entry count withassert(), and both build types defineNDEBUG, so those checks were in no shipping firmware while they guard the erase and program ranges of the config sectors Booster shares. They are runtime checks that trace and return -1; both callers already treat that as "settings not initialised" and jump to Booster.DEVHOOKS_APP_HEAP_HOLD, EPIC-17's mailbox) holds heap in steps so low-memory behaviour can be tested;swd.py app heap_hold 4,0frees it. Release builds contain none of it.The audit found the rest already correct: the four allocations in
settings.ccheck and return -1, and the onlyright()caller falls back to the untruncated value.releaseis 72 bytes smaller than before.Verified on hardware
debugandreleaseDcreatefrom the ST, API folder createfr=0and201on the 29.1 GB FAT32 card (32 KB clusters, read over SWD)200, folder create201, and the ST wrote an 83 KB file through GEMDRIVE; no crashUnable to allocate padding buffer, and the menu showed "Saving the settings failed: the change was not stored."; a later save cleared it201releasecrashes 0, no crash loopAn 8 KB hold was refused while 13,808 bytes were free, so the heap is already fragmented at idle: input for EPIC-12.
Found, not fixed here
9a2c0dcfrom before this epic and with a full heap, so it is the fast-upload path (EPIC-13 STORY-02).volumeanswers503but listings and folder creates answer500 disk_error, and the menu shows nothing about the missing card (EPIC-15 STORY-02, which also now carries Diego's requirement that the app must refuse to launch without a valid card).