games/NXDoom: Address review comments.

Shorten the comments added by this series to what the code needs, and use
NuttX function comment blocks for the new interfaces. Drop references to
the supervisor and the package manager, which are not part of this game.

Report a failed signal handler installation through printf(), matching the
rest of the application, rather than syslog().

Free the renderer scratch buffers through r_shutdown_planes() on the
allocation failure path instead of repeating the frees, and make that
function safe to call more than once.

Assisted-by: OpenAI Codex:gpt-5.6-sol
Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>

Author: aviralgarg05

games/NXDoom/src/d_iwad.c

@@ -271,13 +271,7 @@ static void buld_iwad_dir_list(void)
 
   add_iwad_dir(m_dir_name(myargv[0]));
 
-  /* Add the board's configured DOOM data directory.  Kconfig documents
-   * CONFIG_GAMES_NXDOOM_PREFDIR as "Directory where DOOM WAD files are
-   * stored", but until now it was only used for the config/save file
-   * location -- nothing actually searched it for IWADs, forcing every
-   * launch to rely on the current directory or DOOMWADDIR/DOOMWADPATH
-   * being set by hand first.
-   */
+  /* Add the configured DOOM data directory */
 
   add_iwad_dir(CONFIG_GAMES_NXDOOM_PREFDIR);
 

games/NXDoom/src/doom/d_main.c

@@ -1294,11 +1294,6 @@ void d_doomloop(void)
 
   while (1)
     {
-      /* Safe point (outside any framebuffer/heap access) for nxstore's
-       * SIGTERM-driven close request to actually take effect - see
-       * i_install_quit_signal() in i_system.h.
-       */
-
       i_poll_quit_signal();
       d_run_frame();
     }

games/NXDoom/src/doom/r_plane.c

@@ -119,31 +119,9 @@ static void r_map_plane(int y, int x1, int x2)
   fixed_t length;
   unsigned index;
 
-  /* y indexes cachedheight[]/cacheddistance[]/cachedxstep[]/cachedystep[]
-   * below, all sized SCREENHEIGHT - a y outside that range (observed on
-   * this port: y=255 against a 200-entry array, well past even
-   * viewheight) is an out-of-bounds array write, not just a "debug
-   * assertion".  This used to be gated behind CONFIG_GAMES_NXDOOM_
-   * RANGECHECK and fatal (i_error(), which tears down the whole process
-   * on what vanilla Doom would just render as one glitched span) - both
-   * wrong: the memory-safety check must not be optional, and killing the
-   * entire game over one bad plane span is worse than just not drawing
-   * it.  Clamp y into range instead of touching memory outside the
-   * buffers' real bounds - this still renders the span (as one glitched
-   * row, the same "wrong but visible" failure mode vanilla DOOM has) so
-   * a bad plane doesn't leave a blank gap on screen either.
-   *
-   * The clamp bound must be viewheight, not SCREENHEIGHT: this y is
-   * stored into ds_y and later used by r_draw_span() to index
-   * ylookup[] (r_draw.c), 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 instead of viewheight - 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
-   * this bound is safe for cachedheight[]/etc. too.
+  /* Ensure array indices are in range before access.  The bound is
+   * viewheight rather than SCREENHEIGHT because r_init_buffer() only
+   * populates ylookup[] for [0, viewheight).
    */
 
   if (x2 < x1 || x1 < 0 || x2 >= viewwidth)
@@ -199,34 +177,14 @@ static void r_map_plane(int y, int x1, int x2)
   spanfunc();
 }
 
-/* Row indices into spanstart[] (sized SCREENHEIGHT) that are only ever
- * safe to use as an array index within that range - t1/b1/t2/b2 in
- * r_make_spans() below are also compared directly against each other to
- * drive the span-tracking state machine (including vanilla DOOM's 0xff
- * sentinel for "no span"/edge-of-plane), and that comparison logic must
- * see the real, un-clamped values or the sentinel handling breaks.  Only
- * the array touches themselves need guarding.
- */
-
 static inline boolean r_row_in_range(int row)
 {
   return row >= 0 && row < SCREENHEIGHT;
 }
 
 static void r_make_spans(int x, int t1, int b1, int t2, int b2)
 {
-  /* t1/b1/t2/b2 come from a visplane's top[]/bottom[] arrays.  In valid
-   * play these are either a real screen row or vanilla DOOM's 0xff
-   * (255) "no span here" sentinel; the loop conditions normally keep
-   * that sentinel away from spanstart[].  A malformed renderer state
-   * can violate that invariant, however: row 255 was observed reaching
-   * r_map_plane() on real hardware, after spanstart[t1]/[b1] had already
-   * been evaluated as the call argument.  Guard every spanstart[] touch
-   * directly instead of altering t1/b1/t2/b2, so the state-machine
-   * comparisons and normal sentinel handling remain unchanged.  An
-   * invalid closing row uses column zero as its bounded fallback; an
-   * invalid opening row is ignored.
-   */
+  /* Check that row is in range before indexing arrays. */
 
   while (t1 < t2 && t1 <= b1)
     {
@@ -270,13 +228,6 @@ static void r_make_spans(int x, int t1, int b1, int t2, int b2)
 void r_init_planes(void)
 {
 #ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
-  /* These renderer scratch buffers are sized for a comfortable margin
-   * above vanilla DOOM's original limits and, on a DRAM-constrained
-   * target, blow the internal DRAM budget as static arrays - opt-in
-   * heap allocation instead (comes out of the PSRAM-backed user heap
-   * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS.
-   */
-
   visplanes = malloc(sizeof(visplane_t) * CONFIG_GAMES_NXDOOM_MAXVISPLANES);
   openings = malloc(sizeof(short) * MAXOPENINGS);
   drawsegs = malloc(sizeof(drawseg_t) * CONFIG_GAMES_NXDOOM_MAXDRAWSEGS);
@@ -286,29 +237,12 @@ void r_init_planes(void)
   if (visplanes == NULL || openings == NULL || drawsegs == NULL ||
       vissprites == NULL)
     {
-      /* i_error() doesn't necessarily terminate the whole board on this
-       * flat, single address-space build (see the comment below on
-       * relaunch) - free whatever partially succeeded so a failed
-       * allocation attempt doesn't leak across a subsequent relaunch.
-       */
-
-      free(visplanes);
-      free(openings);
-      free(drawsegs);
-      free(vissprites);
-      visplanes = NULL;
-      openings = NULL;
-      drawsegs = NULL;
-      vissprites = NULL;
+      r_shutdown_planes();
 
       i_error("r_init_planes: failed to allocate renderer buffers");
     }
 
-  /* i_quit() can be followed by another r_init_planes() call within the
-   * same boot (relaunching the game via nxpkg on this flat, single
-   * address-space build), so these heap buffers must be freed on exit
-   * or every relaunch leaks the previous allocation permanently.
-   */
+  /* Free these on exit; the game can be started again in this process. */
 
   i_at_exit(r_shutdown_planes, true);
 #endif
@@ -323,15 +257,29 @@ void r_init_planes(void)
 void r_shutdown_planes(void)
 {
 #ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
-  free(visplanes);
-  free(openings);
-  free(drawsegs);
-  free(vissprites);
-
-  visplanes = NULL;
-  openings = NULL;
-  drawsegs = NULL;
-  vissprites = NULL;
+  if (visplanes != NULL)
+    {
+      free(visplanes);
+      visplanes = NULL;
+    }
+
+  if (openings != NULL)
+    {
+      free(openings);
+      openings = NULL;
+    }
+
+  if (drawsegs != NULL)
+    {
+      free(drawsegs);
+      drawsegs = NULL;
+    }
+
+  if (vissprites != NULL)
+    {
+      free(vissprites);
+      vissprites = NULL;
+    }
 #endif
 }
 

games/NXDoom/src/i_main.c

@@ -57,13 +57,6 @@ void d_doom_main(void);
 
 int main(int argc, char **argv)
 {
-  /* Lets nxstore (or any other supervisor) ask this process to exit
-   * cleanly via SIGTERM instead of the only other option being a forced
-   * task_delete() from outside - see i_system.h/i_system.c for why that
-   * matters on this board (a forced kill mid framebuffer/heap access was
-   * observed to hang the whole system, not just this task).
-   */
-
   i_install_quit_signal();
 
   /* save arguments */

games/NXDoom/src/i_system.c

@@ -28,7 +28,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <syslog.h>
 #include <unistd.h>
 
 #include "config.h"
@@ -332,41 +331,35 @@ void i_quit(void)
   exit(0);
 }
 
-/* i_quit_signal_handler
+/****************************************************************************
+ * Name: i_quit_signal_handler
  *
- * A supervisor process (nxstore) has no reachable in-game quit path to
- * drive (no keyboard/touch input is wired up here) - it can only ask
- * from the outside, via SIGTERM.  This handler does the one thing a
- * signal handler is safe to do: set a flag.  It must NOT call i_quit()
- * (or anything it does - munmap, fclose, exit()'s atexit chain) directly,
- * because a signal can land at literally any point in this process's own
- * execution, including mid-malloc()/mid-blit - exactly the same "unsafe
- * mid-operation teardown" risk as being force-killed from outside, just
- * moved from another task's context into this one.  i_poll_quit_signal()
- * defers the real work to a known-safe boundary instead.
- */
+ * Description:
+ *   Records that a quit was requested.  The work is deferred to
+ *   i_poll_quit_signal() so that no cleanup runs from signal context.
+ *
+ ****************************************************************************/
 
 static void i_quit_signal_handler(int signo)
 {
   (void)signo;
   quit_requested = 1;
 }
 
+/****************************************************************************
+ * Name: i_install_quit_signal
+ *
+ * Description:
+ *   Installs the SIGTERM handler used to request a clean exit.
+ *
+ ****************************************************************************/
+
 void i_install_quit_signal(void)
 {
   struct sigaction sa;
 
-  /* This board's flat, single address-space build can relaunch NXDoom
-   * (via nxpkg) as a fresh loadable ELF module - a proper posix_spawn of
-   * a new module load, which gets its own zeroed .bss/re-initialized
-   * .data - but GAMES_NXDOOM is a tristate Kconfig symbol and can also
-   * be built in as a true built-in (MODULE=n) sharing this process's
-   * address space across "launches" with no fresh .bss at all.  Reset
-   * both pieces of state a stale second invocation could see: a leaked
-   * quit_requested flag would call i_quit() again before the game even
-   * starts, and a leaked exit_funcs chain would run every previous
-   * invocation's exit handlers a second time (double free()s, etc.) in
-   * addition to this invocation's own.
+  /* Built in rather than loaded as a module, this state survives a
+   * previous run and must be reset before the handler is armed.
    */
 
   quit_requested = 0;
@@ -377,22 +370,27 @@ void i_install_quit_signal(void)
 
   if (sigaction(SIGTERM, &sa, NULL) < 0)
     {
-      /* Not fatal - the game still runs, it just can't be asked to
-       * close cleanly from the outside (nxstore's close button will
-       * have nothing to signal into).  Surface it rather than silently
-       * leaving close non-functional with no trace of why.
-       */
+      /* Not fatal: the game runs, it just cannot be asked to exit. */
 
-      syslog(LOG_WARNING,
-            "nxdoom: failed to install SIGTERM handler: %d\n", errno);
+      printf("nxdoom: failed to install SIGTERM handler: %d\n",
+             errno);
     }
 }
 
+/****************************************************************************
+ * Name: i_poll_quit_signal
+ *
+ * Description:
+ *   Exits if a quit was requested.  Called from the main loop, where the
+ *   cleanup i_quit() performs is safe to run.
+ *
+ ****************************************************************************/
+
 void i_poll_quit_signal(void)
 {
   if (quit_requested)
     {
-      syslog(LOG_WARNING, "nxdoom: quit signal seen, calling i_quit\n");
+      printf("nxdoom: quit signal seen, calling i_quit\n");
       i_quit();
     }
 }

games/NXDoom/src/i_system.h

@@ -73,19 +73,14 @@ ticcmd_t *i_base_ticcmd(void);
 
 void i_quit(void) NORETURN;
 
-/* Installs a SIGTERM handler that only sets a flag (async-signal-safe) -
- * the actual i_quit() cleanup (unmapping the framebuffer, closing fds)
- * runs later from i_poll_quit_signal(), called once per tic from a known
- * safe point in the main loop rather than from the signal handler itself,
- * so a supervisor process (nxstore) requesting an exit can never land in
- * the middle of a frame's worth of direct framebuffer/heap access.
+/* Installs the SIGTERM handler used to request a clean exit.  The handler
+ * only records the request; i_poll_quit_signal() performs the exit.
  */
 
 void i_install_quit_signal(void);
 
-/* Checks the flag set by the SIGTERM handler and calls i_quit() if it's
- * set.  Must only be called from a safe point in the main loop - see
- * i_install_quit_signal().
+/* Exits if a quit was requested.  Call only from the main loop, where the
+ * cleanup i_quit() performs is safe to run.
  */
 
 void i_poll_quit_signal(void);

games/NXDoom/src/m_config.c

@@ -2109,6 +2109,9 @@ static void load_default_collection(default_collection_t *collection)
 
       /* Parse one physical line at a time.  fscanf() with whitespace in
        * its format can consume the next line as a missing value.
+       *
+       * A line too long for the buffer cannot be a valid setting, so
+       * discard the remainder of it and move on.
        */
 
       if (strchr(line, '\n') == NULL &&
@@ -2159,6 +2162,10 @@ static void load_default_collection(default_collection_t *collection)
           memmove(strparm, strparm + 1, sizeof(strparm) - 1);
         }
 
+      /* Stripping above removes everything when the value held only
+       * non-printable characters.
+       */
+
       if (strparm[0] == '\0')
         {
           continue;