games/NXDoom: Add RGB565, module support, and crash fixes.#3644
games/NXDoom: Add RGB565, module support, and crash fixes.#3644aviralgarg05 wants to merge 8 commits into
Conversation
a0328da to
6fe6008
Compare
linguini1
left a comment
There was a problem hiding this comment.
The PR description needs to be revised and made more concise. It reads totally AI generated and is hard to follow, and also has some subtle mistakes (i.e. " happy to split further if any single commit above should be its own PR" when there is only one). Pretty much the entirety of this PR is just patches to NXDoom, we don't really need to know about the other module loading PRs here, just the build system change you made to load this as a module. You are expected to review the output of AI agents before submitting here, even as a draft :)
You should also use the Assisted-by: field in your commit messages, see the new updates to the contribution guide :)
Again, the testing section cannot just be a description of what you tested. Can you show the simulator playing DOOM after the changes, or your target device? Could you show some before/after output for the divide-by-zero and other issues reported here?
I don't see the need to malloc the arrays that were previously statically allocated. Using malloc on embedded devices is not a good practice and should be avoided where possible. I think those changes should be reverted.
|
Thanks for the thorough review @linguini1 going through each point: PR description: You're right, that was sloppy, I squashed the commits down but didn't re-read the description afterward, so it still had a line referring to multiple commits that no longer existed. Rewriting it now Assisted-by field: Didn't know this was now expected, will add it going forward, including updating this PR's commit message. Testing section: Fair — I have this on real hardware (ESP32-S3 board, RGB565 framebuffer) but didn't capture it for the PR. I'll get a screen recording of it running plus the crash logs from before the fixes and attach both. malloc vs static arrays: The reason was DRAM budget, these buffers (visplanes/openings/drawsegs/vissprites) are sized generously above vanilla DOOM's limits, and as static arrays they were eating into internal SRAM budget once this actually gets linked into a full firmware image alongside everything else. Heap allocation moves them onto this target's PSRAM-backed heap instead. But you're right that malloc-by-default isn't great practice, I'll put it behind a Kconfig option instead, so boards that don't need it keep static allocation as the default, and boards that are DRAM-constrained can opt in. Will push an update addressing all of this plus the inline comments one by one |
Can we resolve this with FAR designators? |
The actual problem here is static/BSS footprint at link time — a large static array is always-resident regardless of what pointer qualifier you put on it, |
6fe6008 to
9dd0915
Compare
Add RGB565 framebuffer support to blit_screen() - it previously assumed a 32-bit ARGB framebuffer unconditionally, a real limitation for any board whose framebuffer is FB_FMT_RGB16_565. A single blit loop now branches on pinfo.bpp only at the pixel-write step (RGBTO16() for 16bpp, the existing ARGBTO32() path for 32bpp); anything else fails loudly via i_error() rather than reading/writing past the intended pixel bounds silently. Also centers the scaled viewport within the framebuffer instead of pinning it to the top-left corner, and adds CONFIG_GAMES_NXDOOM_PREFDIR to the IWAD search path (previously only used for the config/save file location; the Kconfig entry always has a default, so no #ifdef guard is needed around using it). Makes GAMES_NXDOOM tristate and lets MODULE follow it (MODULE = $(CONFIG_GAMES_NXDOOM)) instead of hardcoding MODULE = m, so it can still be built in as before or selected as a standalone loadable module installable via nxpkg/nxstore, same as every other tristate-capable app in apps/. Fix two real hardware crashes found while bringing this up as a loadable module: - A truncated/corrupted config line was silently overriding a variable's compiled-in default with an empty/unparsable value instead of being skipped - this let a corrupted "screenblocks" line through as screenblocks=0, and the renderer's view-size math divides by a value derived from screenblocks, reaching a divide-by-zero hardware exception. Also clamps screenblocks to its own valid range [3, 11] as an independent second layer of defense. - r_map_plane()'s bounds check was gated behind the debug-only CONFIG_GAMES_NXDOOM_RANGECHECK and, when tripped, called the fatal i_error() - both wrong: the check guards a real out-of-bounds array access (observed with values far past even viewheight), so it cannot be optional, and killing the whole process over one glitched plane span is worse than vanilla DOOM's own behavior of rendering the glitch. Now unconditional and clamps y into range instead of touching memory outside the buffers' bounds, so the span still renders (as one glitched row) rather than leaving a gap. Also adds CONFIG_GAMES_NXDOOM_HEAP_BUFFERS: the renderer's visplanes/openings/drawsegs/vissprites scratch buffers remain static arrays by default (matching vanilla DOOM), with heap allocation available as an opt-in for targets where their combined size threatens the internal DRAM budget once linked into a full application image. CONFIG_GAMES_NXDOOM_STATDUMP_MAX_CAPTURES makes statdump's diagnostic capture-buffer size (unrelated to gameplay) a Kconfig option instead of a hardcoded value, default unchanged from vanilla (32). Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
r_map_plane()'s existing bounds check clamped an out-of-range `y` to SCREENHEIGHT - 1, believing that to be the array bound in need of protection. On real hardware that clamp itself caused a crash: `y` is stored into ds_y and later used by r_draw_span() (r_draw.c) to index ylookup[], which r_init_buffer() only populates for [0, viewheight) - viewheight can be smaller than SCREENHEIGHT (a sub-window within the physical screen), so entries from viewheight up to SCREENHEIGHT are zero-initialized (NULL) pointers. Clamping to SCREENHEIGHT - 1 traded the original out-of-bounds write for a NULL-pointer-plus-offset framebuffer write, confirmed on real hardware as a load/store exception at a small virtual address. viewheight is always <= SCREENHEIGHT, so clamping to viewheight - 1 instead is safe for cachedheight[]/cacheddistance[]/cachedxstep[]/cachedystep[] too. r_draw.c's own RANGECHECK-gated debug assertions are updated to match (they previously compared against SCREENHEIGHT as well). Separately, r_make_spans() indexes spanstart[t1]/spanstart[b1] (read) and writes spanstart[t2]/spanstart[b2] using row indices taken directly from a visplane's top[]/bottom[] arrays, before r_map_plane() is ever called - so its clamp can't protect these. In valid play these rows are either a real screen row or vanilla DOOM's 0xff (255) "no span here" sentinel, and the surrounding while-loop guards are written so the sentinel can never reach spanstart[] except at a plane's own edge columns, where writing spanstart[255] is part of the normal algorithm - already out-of-bounds on this port, since spanstart[] is only sized SCREENHEIGHT (200). A corrupted BSP/segment can also hand these a genuinely arbitrary value (this is what the row-255 crash above traced back to). Guard every touch of spanstart[] directly instead of altering t1/b1/t2/b2 themselves, so the span-tracking state machine's comparisons - including the sentinel logic they rely on - are completely unaffected; a row outside the array just contributes 0 as its span start instead of corrupting or reading past memory. Also gives GAMES_NXDOOM_STATDUMP_MAX_CAPTURES an explicit range (1 1024) - this diagnostic capture-buffer size Kconfig option had no bound at all. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
load_default_collection()'s `while (!feof(f))` loop decided whether to keep looping off feof() rather than fscanf()'s own return value - the classic version of this bug: on a config file whose bytes don't line up with "%s %[^\n]\n" at all (e.g. leftover binary/corrupted content from a previous write), fscanf() can fail a conversion without the stream ever reaching EOF, and feof() has no way to know that. On real hardware this hung NXDoom completely at startup with a corrupted default.cfg on disk - no crash, no output, and no way to close the game either, since the hang happened before the main loop (and its SIGTERM poll point) was ever reached. Terminate directly off EOF/error from fscanf() instead. A failed conversion is only guaranteed to consume nothing, not to advance the stream, so also track the file position directly and force one byte of progress (or bail out) if a scan attempt didn't move it - corrupt/ binary content can now only ever cost one pass over the file, never an infinite loop. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
…ors. A supervisor process (e.g. an app-store UI that owns the framebuffer while a launched game runs directly against /dev/fb0) has no reachable in-game quit path to drive - no keyboard/touch input is wired up for that - so it can only ask NXDoom to exit from the outside. The only existing option for that was a forced task_delete() from the supervisor's side, which was found on real hardware to hang the entire board (not just this one task) when it landed mid framebuffer/ heap access, on this flat-memory build where a bad access in one task isn't contained to that task. Add i_install_quit_signal()/i_poll_quit_signal()/a SIGTERM handler (i_system.c/i_system.h): the handler itself only sets a volatile sig_atomic_t flag - it must not call i_quit() (or anything it does: munmap, fclose, exit()'s atexit chain) directly, since a signal can land at literally any point in this process's own execution, including mid-malloc()/mid-blit, the same "unsafe mid-operation teardown" risk as being force-killed from outside, just moved into this process's own context. i_poll_quit_signal() defers the actual shutdown to a call in the main per-frame loop (d_doomloop(), d_main.c) - a point that's definitely safe (outside any framebuffer/heap access) and reached every frame regardless of what else NXDoom is doing, which is what's actually verified working end-to-end against a real supervisor's SIGTERM/close path on hardware. i_install_quit_signal() is called once from main() (i_main.c), and also: - Checks sigaction()'s return value and logs via syslog on failure instead of ignoring it silently - the game still runs either way, but silently leaving close non-functional with no trace of why would make a real close-path bug harder to diagnose than it needs to be. - Resets quit_requested and exit_funcs at the start of i_install_quit_signal(). This board's flat, single address-space build normally relaunches NXDoom as a fresh loadable ELF module with its own zeroed .bss, but GAMES_NXDOOM is a tristate Kconfig symbol and can also be built in as a true built-in sharing this process's address space across "launches" with no fresh .bss at all - a leaked quit_requested flag would call i_quit() again before the game even starts on a second invocation, and a leaked exit_funcs chain would re-run every previous invocation's exit handlers a second time. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
blit_screen() branches purely on pinfo.bpp (16 vs 32) to decide between the RGB565 and RGB32 conversion paths, but bit depth alone doesn't determine pixel layout - multiple incompatible formats share the same depth. i_init_graphics() now checks vinfo.fmt against the one format blit_screen() actually emits for each depth (FB_FMT_RGB16_565 for 16bpp, FB_FMT_RGB32 for 32bpp) and fails loudly via i_error() on any mismatch or unsupported depth, instead of silently misinterpreting the framebuffer's actual pixel layout. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
Read each physical configuration line independently so malformed or truncated input cannot consume a following setting as its value. Discard overlong lines and retain defaults for incomplete entries. Assisted-by: Codex:gpt-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
Move the maximum-visplane guard ahead of the new plane initialization. At full capacity, the previous ordering wrote three fields one element past the visplanes array before reporting the overflow. Assisted-by: Codex:gpt-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
Check integer conversions before updating bound configuration values. This keeps compiled defaults intact for malformed non-numeric values instead of propagating an uninitialized result from sscanf(). Assisted-by: Codex:gpt-5 Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
9dd0915 to
1fbe5cc
Compare
|
Thanks for the detailed review. I went back through the PR and addressed the
The original exception log was not retained, and the Testing section now says |
Note: Please adhere to Contributing Guidelines.
Summary
Update NXDoom for RGB565 framebuffer targets and optional loadable-module
builds, and fix crashes observed during ESP32-S3 bring-up.
The changes:
CONFIG_GAMES_NXDOOMtristate and deriveMODULEfrom it;FB_FMT_RGB16_565while retainingFB_FMT_RGB32;CONFIG_GAMES_NXDOOM_HEAP_BUFFERS;malformed integer values, and clamp
screenblocks;SIGTERMat a safe point in the frame loop.Impact
=yor as a loadable ELFwith
=m, and can render to RGB565.MODULE.changing a framebuffer driver.
compatibility and correctness fixes.
fixes prevent invalid memory accesses.
storage is opt-in. Unsupported framebuffer layouts now fail initialization.
Testing
Build host:
xtensa-esp-elf-gcc 14.2.0(
esp-14.2.0_20251107)Target:
Static verification:
nxstyleandgit diff --checkThe exact final source head was built and flashed. NXDoom reached gameplay on
RGB565, malformed configuration input retained safe defaults, and the process
returned cleanly to nxstore after
SIGTERM.Hardware bring-up photograph from July 11 (the later signed-tree checks are
recorded below):
Testing logs after change:
Malformed-configuration reproduction:
The original divide-by-zero or renderer-exception log was not retained, so it
cannot be included as before-change output.
The final restored image remained stable while idle; the earlier glitch
coincided with a failed transfer whose Base64 payload was interpreted as NSH
commands, not ordinary nxpkg file writing.
PR verification Self-Check