games/NXDoom: RGB565 support, loadable-module conversion, and crash fixes by aviralgarg05 · Pull Request #3644 · apache/nuttx-apps

aviralgarg05 · 2026-07-16T07:40:12Z

Note: Please adhere to Contributing Guidelines.

Summary

This PR updates NXDoom to run on RGB565 framebuffer targets and to build either
into the firmware or as a loadable module. It also fixes three crashes found
while running the module on an ESP32-S3 board.

The framebuffer code now supports both 16-bit RGB565 and the existing 32-bit
ARGB format. Pixel conversion is selected inside the shared blit loop, the
rendered viewport is centered, and unsupported pixel depths fail explicitly.

CONFIG_GAMES_NXDOOM is now a tristate option, and the Makefile derives
MODULE from it. As a result, =y keeps the existing built-in behavior while
=m produces a standalone module.

The crash fixes are:

ignore incomplete configuration values and clamp screenblocks to its valid
range, preventing a divide-by-zero after a truncated configuration file;
keep the plane-rendering bounds check enabled in normal builds and clamp an
invalid row before accessing renderer buffers; and
allocate myargv with space for the required trailing NULL, preventing an
out-of-bounds pointer read during startup.

Two optional tuning settings are also added. Renderer scratch buffers remain
static by default; CONFIG_GAMES_NXDOOM_HEAP_BUFFERS can move them to the heap
on targets with a constrained static DRAM budget.
CONFIG_GAMES_NXDOOM_STATDUMP_MAX_CAPTURES exposes the diagnostic capture count
and retains the existing default of 32.

Impact

NXDoom can now render on RGB565 framebuffers and can be selected as either a
firmware built-in or a loadable module. Existing 32-bit framebuffer support is
retained.

The default memory behavior is unchanged: renderer scratch buffers remain
statically allocated. Heap allocation is opt-in. The changes are confined to
games/NXDoom and do not affect other applications.

Testing

Build host:

macOS 26.5, arm64
xtensa-esp-elf-gcc 14.2.0 (esp-14.2.0_20241119)

Target:

Xtensa / ESP32-S3
esp32s3-touch-lcd-7:nsh on a Waveshare ESP32-S3-Touch-LCD-7
(custom, not-yet-upstream board configuration)
RGB565 framebuffer

The full nuttx.bin image was built with heap buffers both disabled and
enabled. Symbol sizes confirm that the expected storage mode was compiled:

# CONFIG_GAMES_NXDOOM_HEAP_BUFFERS is not set
$ xtensa-esp32s3-elf-nm -S apps/bin/nxdoom | grep -E " (visplanes|drawsegs|vissprites)$"
000708a0 00002400 B drawsegs
0005aa20 0000f900 B visplanes
0006a484 00001680 B vissprites

# CONFIG_GAMES_NXDOOM_HEAP_BUFFERS=y
$ xtensa-esp32s3-elf-nm -S apps/bin/nxdoom | grep -E " (visplanes|drawsegs|vissprites)$"
0005aa40 00000004 B drawsegs
00055b38 00000004 B visplanes
00055ca0 00000004 B vissprites

On the physical board, NXDoom was installed and launched as a standalone
module, rendered correctly from the title screen through gameplay on the
RGB565 framebuffer, and returned cleanly to the launcher. An extended play run
completed without reproducing the divide-by-zero, renderer bounds, or
myargv startup crashes that occurred before these fixes.

linguini1

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.

linguini1 · 2026-07-16T14:27:40Z

 sector_t *backsector;

-drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+drawseg_t *drawsegs;


Why is this change necessary?

will put it behind the same Kconfig switch.

linguini1 · 2026-07-16T14:27:48Z

 extern boolean skymap;

-extern drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+extern drawseg_t *drawsegs;


same as above

linguini1 · 2026-07-16T14:28:43Z

+
+  if (blocks < 3 || blocks > 11)
+    {
+      printf("r_set_view_size: screenblocks=%d out of range, using 10\n",
+            blocks);
+      blocks = 10;
+    }


Did you actually run into an error that caused this?

Yes, a truncated config file left screenblocks at 0, and the view-size math divides by a value derived from it, so it hit a divide-by-zero hardware exception and took the whole board down.
This clamp is the second, independent layer of defense on top of the config-parsing fix in m_config.c

linguini1 · 2026-07-16T15:28:05Z

 /* Here comes the obnoxious "visplane". */

-visplane_t visplanes[CONFIG_GAMES_NXDOOM_MAXVISPLANES];
+visplane_t *visplanes;


will put it behind the same Kconfig switch.

linguini1 · 2026-07-16T15:28:35Z

+  if (x2 < x1 || x1 < 0 || x2 >= viewwidth || y < 0 || y >= SCREENHEIGHT)
    {
-      i_error("R_MapPlane: %i, %i at %i", x1, x2, y);
+      return;


Why remove the error report?

The bounds check itself was already there, just gated behind a debug-only config and fatal when tripped,
so in a normal build it wasn't even compiled in, and the array write happened uncontrolled. I made the check unconditional, but killing the whole process over one glitched plane span felt worse than what vanilla DOOM does (renders the glitch and moves on), so I made it skip the draw instead of calling i_error(). Open to putting the i_error() back if you'd rather fail loud than render a glitch, happy to hear which you'd prefer

Maybe rendering the glitch is better if there are no other side effects? More like vanilla DOOM. I don't know which looks worse from a rendering standpoint, skipping doesn't seem like a huge deal.

I think there's a middle ground: instead of returning early (which leaves a blank gap where that scanline should be), I can clamp y into [0, SCREENHEIGHT-1] and still draw with the clamped value. That keeps it memory-safe — the actual out-of-bounds write is still gone — while still rendering something for that span instead of a hole, closer to vanilla's "wrong but present" glitch. Switching to that.

In that case I would render the glitch instead.

linguini1 · 2026-07-16T15:31:47Z

+   * +1 and an explicit NULL terminator: argv is conventionally
+   * NULL-terminated at argv[argc] (this is what the OS/exec path
+   * guarantees for the `argv` parameter above), and some of this
+   * codebase's own argument handling was written assuming that holds
+   * for myargv too - an under-sized allocation here leaves myargv[argc]
+   * pointing at whatever the allocator happens to return next, which
+   * only reads as "probably zero" by chance depending on heap layout.
+   */

  myargc = argc;
-  myargv = malloc(argc * sizeof(char *));
+  myargv = malloc((argc + 1) * sizeof(char *));
  assert(myargv != NULL);

  for (int i = 0; i < argc; i++)


Did you actually run into an error with this?

Yes, a bad-pointer-dereference crash (LoadProhibitedCause) on real hardware, root-caused through the crash dump and symbol resolution against the built ELF. myargv was allocated for exactly argc pointers but code elsewhere assumed it was NULL-terminated at argv[argc], so that slot pointed at whatever the allocator returned next (usually zero by luck)

Can you include the repro in this PR or with some before/after logs?

Honestly, a clean repro recipe is tricky here since it's heap-layout dependent — that's exactly what made it read as "NULL by luck" instead of a guaranteed crash. It wasn't triggered by anything unusual on my end though, just an ordinary nxdoom launch on the board. I'll grab the actual crash backtrace from when this first surfaced and attach it, plus a fresh before/after capture so there's real evidence in the PR instead of just my description of it.

linguini1 · 2026-07-16T15:34:04Z

  while (!feof(f))
    {
-      if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) != 2)
+      strparm[0] = '\0';


Again why change this? Did you run into an error or was this determined by an AI agent? Can you give a reproduction?

not AI-guessed -- a config file left mid-write (unclean shutdown) had an empty screenblocks= line. The old code passed that empty value straight to set_variable() regardless of whether parsing actually succeeded, which is exactly how screenblocks ended up at 0 and caused the divide-by-zero in r_main.c.
Repro: truncate any line in the save config so the value side is empty, then relaunch.

aviralgarg05 · 2026-07-16T16:23:48Z

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

linguini1 · 2026-07-16T16:41:31Z

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.

Can we resolve this with FAR designators?

aviralgarg05 · 2026-07-16T16:59:41Z

Can we resolve this with FAR designators?

FAR is defined as empty on this target (Xtensa/flat 32-bit memory model). It only expands to something real on segmented 8/16-bit architectures like 8051 (__xdata) or old x86 memory models (far/_Far), where it's about pointer segment qualification, not memory placement.

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, FAR included. That's a different axis from what FAR addresses, so it won't help with the DRAM budget concern. The Kconfig-gated heap allocation is still the mechanism that actually moves the buffer's footprint onto the PSRAM-backed heap instead of internal SRAM.

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 three 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. - myargv was under-allocated: sized for argc pointers, but this codebase's own argument handling assumes argv is NULL-terminated at argv[argc] (standard C/exec convention). Root-caused via a real crash dump and symbol resolution against the built ELF; now allocates argc + 1 pointers and explicitly NULL-terminates. 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 <noreply@anthropic.com> Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>

github-actions Bot added Size: M Area: Games labels Jul 16, 2026

aviralgarg05 mentioned this pull request Jul 16, 2026

system/nxpkg: network sync, install hardening, and CLI completion #3642

Draft

jerpelea requested a review from linguini1 July 16, 2026 07:47

aviralgarg05 force-pushed the gsoc/nxdoom-bringup-fixes-pr7 branch from a0328da to 6fe6008 Compare July 16, 2026 08:06

linguini1 requested changes Jul 16, 2026

View reviewed changes

aviralgarg05 force-pushed the gsoc/nxdoom-bringup-fixes-pr7 branch from 6fe6008 to 9dd0915 Compare July 17, 2026 19:48

Uh oh!

Conversation

aviralgarg05 commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact

Testing

Uh oh!

linguini1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linguini1 Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aviralgarg05 commented Jul 16, 2026

Uh oh!

linguini1 commented Jul 16, 2026

Uh oh!

aviralgarg05 commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aviralgarg05 commented Jul 16, 2026 •

edited

Loading

linguini1 Jul 16, 2026 •

edited

Loading