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 is another slice of the ongoing GSoC 2026 Dynamic ELF loading and
nxpkg work for NuttX.

An earlier draft PR (#3474) carries the initial nxpkg package
lifecycle helper. This PR is independent of the nxpkg/nxstore work
itself (#3642, #3643) — it's a set of bring-up and hardening fixes to
games/NXDoom found while porting it to run as one of the packages
those PRs' frontend can install and launch.

Series order for this PR:

base series context: system/nxpkg: add a basic package lifecycle helper #3474 (merged)
independent of: apps: add missing module metadata for executable tools #3519, examples/sotest: support packaged shared library fixtures #3526, examples/elf: extend nxpkg validation coverage #3531, system/nxpkg: network sync, install hardening, and CLI completion #3642, system/nxstore: add LVGL touchscreen app-store frontend #3643

Concretely, it:

adds 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. Branches on
pinfo.bpp: 16bpp uses RGBTO16(), 32bpp keeps the existing
ARGBTO32() path, anything else fails loudly via i_error() instead
of 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
adds CONFIG_GAMES_NXDOOM_PREFDIR to the IWAD search path — that
Kconfig option is documented as where DOOM WAD files are stored, but
was previously only used for the config/save file location
builds NXDoom as a standalone loadable module (MODULE = m)
unconditionally, so it can be installed/launched via nxpkg/nxstore
rather than only ever being a firmware built-in
hardens config-file parsing: a truncated/corrupted config line (e.g.
left mid-write by an unclean shutdown) was silently overriding a
variable's compiled-in default with an empty/unparsable value instead
of being skipped — this is what let a corrupted screenblocks line
through as screenblocks=0, and the renderer's view-size math divides
by a value derived from screenblocks, so that reached a
divide-by-zero hardware exception. Now clamps screenblocks to its
own valid range [3, 11] as an independent second layer of defense
fixes a real out-of-bounds array write in r_map_plane(): its bounds
check was gated behind the debug-only CONFIG_GAMES_NXDOOM_RANGECHECK
and, when tripped, called the fatal i_error(). Both were wrong — the
check guards a real out-of-bounds write (observed on real hardware
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 just rendering the glitch. The check is
now unconditional, checks against the true array bound
(SCREENHEIGHT, not viewheight), and skips the draw instead of
touching memory or aborting
converts visplanes/openings/drawsegs/vissprites from static
arrays to heap-allocated buffers (allocated once, freed via
i_at_exit(..., true) so cleanup also runs on the i_error() fatal
path) — these buffers are large enough as static arrays to threaten
this kind of target's internal DRAM budget once a full application is
linked into the same image; this target's user heap is PSRAM-backed,
so heap allocation removes that pressure. Also reduces
statdump.c's MAX_CAPTURES from 32 to 4 for the same reason (this
buffer is diagnostic capture storage, not required for gameplay)
fixes an under-allocated argv array: myargv was sized for argc
pointers, but this codebase's own argument-handling assumes argv is
NULL-terminated at argv[argc] (standard C/exec convention). The
under-sized allocation left myargv[argc] pointing at whatever the
allocator happened to return next, which only read as NULL by
chance depending on heap layout — root-caused via a real crash dump
and symbol resolution against the built ELF

Impact

User impact:

NXDoom now renders correctly on RGB565 framebuffers, not just 32-bit
ARGB ones
NXDoom is now installable/launchable as a standalone package instead
of only ever being compiled into the firmware image
fixes multiple real hardware crashes (divide-by-zero, out-of-bounds
write, bad pointer dereference)

Build impact:

games/NXDoom's MODULE is now unconditionally m

Runtime impact:

visplanes/openings/drawsegs/vissprites move from static to
heap allocation, changing this app's memory footprint profile (lower
static/BSS usage, one-time heap allocation at startup instead)

Compatibility impact:

none for other applications; this PR only touches games/NXDoom

Testing

Host used for build:

OS: macOS 26.5 arm64
compiler: xtensa-esp-elf-gcc (crosstool-NG esp-14.2.0_20241119) 14.2.0

Target used for verification:

arch: xtensa
board/config used during runtime verification: esp32s3-touch-lcd-7:nsh
board: Waveshare ESP32-S3-Touch-LCD-7 (custom board directory, not
yet upstream)

Runtime flow verified on hardware, repeatedly, across this project's
full install/launch/close lifecycle:

boot to the touchscreen app list (see system/nxstore: add LVGL touchscreen app-store frontend #3643)
install and launch NXDoom as a standalone package
renders correctly on this board's RGB565 framebuffer, to the title
screen and into gameplay
play for an extended session without hitting the divide-by-zero,
out-of-bounds-write, or bad-argv-pointer crashes this PR fixes —
each of those was found and root-caused via an actual crash on this
exact board before being fixed here
close cleanly and return to the app list (the cooperative-quit side
of that handshake is a separate PR)

This is a draft PR — opening now to get the series in front of
reviewers; happy to split further if any single commit above should be
its own PR.

…ixes 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. Branches on pinfo.bpp: 16bpp uses RGBTO16(), 32bpp keeps the existing ARGBTO32() path, and 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). Builds as a standalone loadable module (MODULE = m) unconditionally so it can be installed/launched via nxpkg/nxstore. 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 write (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, checks against the true array bound (SCREENHEIGHT), and skips the draw instead of touching memory or aborting. Also converts visplanes/openings/drawsegs/vissprites from static arrays to heap allocation (freed via i_at_exit so cleanup also runs on the i_error() fatal path) to relieve DRAM pressure once a full application is linked into the same image. - 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. Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>

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?

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

 extern boolean skymap;

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


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?

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

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

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


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?

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

+#ifdef CONFIG_GAMES_NXDOOM_PREFDIR
+  add_iwad_dir(CONFIG_GAMES_NXDOOM_PREFDIR);
+#endif


This value should always be defined. No need for ifdef.

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?

linguini1 · 2026-07-16T15:33:00Z

+          g_graphics_state.xoffset *
+              (g_graphics_state.pinfo.bpp == 16 ? 2 : 4);
+
+  if (g_graphics_state.pinfo.bpp == 16)


The only thing different about these loops is the pixel format. Put the if statement inside the loop at the pixel conversion step.

Do you have a 16-bit device to test on?

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?

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

 # Program options

-MODULE    = $(CONFIG_GAMES_NXDOOM)
+MODULE    = m


Why do this?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

games/NXDoom: RGB565 support, loadable-module conversion, and crash fixes#3644

games/NXDoom: RGB565 support, loadable-module conversion, and crash fixes#3644
aviralgarg05 wants to merge 1 commit into
apache:masterfrom
aviralgarg05:gsoc/nxdoom-bringup-fixes-pr7

aviralgarg05 commented Jul 16, 2026

Uh oh!

linguini1 left a comment

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

linguini1 Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

aviralgarg05 commented Jul 16, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants