.pr_agent_auto_best_practices - iNavFlight/inav GitHub Wiki

Pattern 1: Validate external inputs (env vars, CLI args, and incoming protocol payload sizes) before use, and fail deterministically with a clear error when inputs are missing or malformed.

Example code before:

// env var / CLI arg used without validation
int pr = parseInt(getenv("PR_NUMBER"));      // could be NaN/NULL
const char *path = argv[1];                 // argc not checked

// protocol handler reads without checking length
uint8_t item = buf[0];
uint16_t pos = read_u16(buf + 1);

Example code after:

// validate env var / CLI args
const char *s = getenv("PR_NUMBER");
if (!s) { fprintf(stderr, "PR_NUMBER missing\n"); return 1; }
char *end = NULL;
long pr = strtol(s, &end, 10);
if (*s == '\0' || *end != '\0') { fprintf(stderr, "PR_NUMBER invalid\n"); return 1; }

if (argc < 2) { fprintf(stderr, "usage: tool <path>\n"); return 2; }

// validate payload size before reading
if (dataSize < 3) return MSP_RESULT_ERROR;
uint8_t item = buf[0];
uint16_t pos = read_u16(buf + 1);

Relevant past accepted suggestions:

Suggestion 1:

[reliability] `parseInt` result not validated

`parseInt` result not validated

The PR comment step uses `parseInt(process.env.PR_NUMBER)` without checking for `NaN` or enforcing base-10 parsing, which can lead to non-deterministic behavior if the env var is missing/malformed. This violates the requirement to validate external inputs and handle invalid values deterministically.

Issue description

The workflow parses PR_NUMBER from an environment variable using parseInt(...) and then uses it without checking for NaN (and without specifying radix 10). If the env var is missing or malformed, this can produce non-deterministic behavior (e.g., NaN in URLs / API params) instead of a clear, deterministic failure.

Issue Context

This job runs with elevated permissions (pull-requests: write) and should validate external inputs (including env vars derived from artifacts/outputs) before use.

Fix Focus Areas

.github/workflows/pr-test-builds.yml[107-110] բավ

Suggestion 2:

Add payload size validation check

Add a payload size check in the MSP_OSD_CUSTOM_POSITION handler to ensure the incoming data is at least 3 bytes before reading from the buffer.

src/main/fc/fc_msp.c [2718-2731]

 case MSP_OSD_CUSTOM_POSITION: {
+    if (dataSize < 3) {
+        return MSP_RESULT_ERROR;
+    }
     uint8_t item;
     sbufReadU8Safe(&item, src);
     if (item < OSD_ITEM_COUNT){ // item == addr
         osdEraseCustomItem(item);
         osdLayoutsConfigMutable()->item_pos[0][item] = sbufReadU16(src) | (1 << 13);
         osdDrawCustomItem(item);
     }
     else{
         return MSP_RESULT_ERROR;
     }
 
     break;
 }

Suggestion 3:

[learned best practice] Validate `"$#"` before reading `$1` and print a short usage message on error so failures are deterministic and user-friendly.

Suggestion 4:

[general] Add a check to the firmware renaming script to ensure `.hex` files exist before attempting to loop through and rename them, preventing potential errors.

Pattern 2: Avoid build fragility by explicitly including required headers/macros and by removing/ignoring generated build outputs that contain machine-specific paths or state.

Example code before:

// header relies on transitive includes for macros/types
#pragma once
#define MY_ID CONCAT4(A, B, C, D)   // CONCAT4 not defined here

// generated build files accidentally committed
set(CMAKE_ASM_TARGET_INCLUDE_PATH "/Users/alice/project/include")

Example code after:

// header includes what it uses
#pragma once
#include "common/utils.h"           // defines CONCAT4
#define MY_ID CONCAT4(A, B, C, D)

// generated build outputs removed and ignored
# .gitignore
CMakeFiles/
CMakeCache.txt

Relevant past accepted suggestions:

Suggestion 1:

[reliability] Headers rely on transitive CONCAT4

Headers rely on transitive CONCAT4

The new bus_spi_stm32{h7,f7}xx.h headers use CONCAT4 but do not include the header that defines it, relying on current include order/transitive includes from other headers. This is a latent build fragility: a future refactor or reuse of these headers elsewhere can cause compile failures (e.g., CONCAT4 undefined).

Issue description

The new SPI AF lookup table headers use CONCAT4(...) but do not include the header that defines it (common/utils.h). They currently compile only due to transitive includes (via drivers/io.h → io_def.h → common/utils.h) and therefore are fragile to include-order changes or reuse.

Issue Context

This is a latent build fragility / maintainability issue: it may not break today, but it can break later during refactors or if another file includes these headers without including common/utils.h first.

Fix Focus Areas

src/main/drivers/bus_spi_stm32h7xx.h[32-38]
src/main/drivers/bus_spi_stm32f7xx.h[36-42]

Suggestion 2:

Remove generated build file from repository

Remove the generated CMake build file from the repository. It contains user-specific absolute paths that will cause build failures for other developers and should be added to .gitignore.

src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/DependInfo.cmake [1-474]

-# Consider dependencies only in project.
-set(CMAKE_DEPENDS_IN_PROJECT_ONLY OFF)
+# This file should be removed from the repository.
 
-# The set of languages for which implicit dependencies are needed:
-set(CMAKE_DEPENDS_LANGUAGES
-  "ASM"
-  )
-# The set of files for implicit dependencies of each language:
-set(CMAKE_DEPENDS_CHECK_ASM
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/CMSIS/DSP/Source/TransformFunctions/arm_bitreversal2.S" "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/__/__/__/__/lib/main/CMSIS/DSP/Source/TransformFunctions/arm_bitreversal2.S.obj"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/main/startup/startup_stm32f722xx.s" "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/src/src/main/target/AXISFLYINGF7PRO/CMakeFiles/AXISFLYINGF7PRO_for_bl.elf.dir/__/__/startup/startup_stm32f722xx.s.obj"
-  )
-set(CMAKE_ASM_COMPILER_ID "GNU")
-
-# Preprocessor definitions for this target.
-set(CMAKE_TARGET_DEFINITIONS_ASM
-...
-
-# The include file search paths:
-set(CMAKE_ASM_TARGET_INCLUDE_PATH
-  "main/target/AXISFLYINGF7PRO"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/STM32F7/Drivers/STM32F7xx_HAL_Driver/Inc"
-  "/Users/ahmed/Desktop/Projects/INAV-RPiOSD/inav/lib/main/STM32F7/Drivers/CMSIS/Device/ST/STM32F7xx/Include"
-...
-

Suggestion 3:

Remove generated file from version control

Remove the generated Makefile.cmake file from version control. This file is environment-specific and should be ignored by adding the CMakeFiles directory to .gitignore.

src/CMakeFiles/Makefile.cmake [1-9]

-# CMAKE generated file: DO NOT EDIT!
-# Generated by "Unix Makefiles" Generator, CMake Version 4.1
+# This file should be removed from the pull request and repository.
 
-# The generator used is:
-set(CMAKE_DEPENDS_GENERATOR "Unix Makefiles")
-
-# The top level Makefile was generated from the following files:
-set(CMAKE_MAKEFILE_DEPENDS
-  "CMakeCache.txt"
-...
-

Pattern 3: Add explicit timeout handling for polling/wait loops and DMA/IO state transitions; if a timeout is hit, return a specific error instead of breaking/continuing with partial state.

Example code before:

while (!(REG & READY_BIT)) {
    if (--timeout == 0) break;  // continues afterward with incomplete state
}
disable_dma_stream();
// assume DMA is disabled; proceed to reconfigure
configure_dma_stream();

Example code after:

while (!(REG & READY_BIT)) {
    if (--timeout == 0) return ERR_TIMEOUT;
}

uint32_t t = DMA_DISABLE_TIMEOUT;
disable_dma_stream();
while (dma_stream_enabled() && --t) { /* wait */ }
if (t == 0) return ERR_DMA_STILL_ACTIVE;

configure_dma_stream();

Relevant past accepted suggestions:

Suggestion 1:

[possible issue] In `SD_StartBlockTransfert`, check if the DMA disable loop timed out. If it did, set an error and return to avoid reconfiguring an active DMA stream, which could cause unpredictable behavior.

Suggestion 2:

[possible issue] In `SD_HighSpeed`, if the `swTimeout` is reached while waiting for SDIO status, return an error like `SD_TIMEOUT` instead of just breaking the loop to prevent processing incomplete data. [possible issue, importance: 7]

New proposed code:

Suggestion 3:

[general] Add a timeout or retry counter to the polling loops in `STORAGE_Read` to prevent the system from hanging if the SD card becomes unresponsive.

Pattern 4: Guard numeric edge cases in calculations (e.g., division by zero and vector normalization of near-zero magnitude), and use consistent sentinel values for invalid/no-data states.

Example code before:

float tpa = (airspeed - ref) / ref;          // ref could be 0
normalize(v);                                // v could be ~0 length

// failed read only partially resets data
mag.x = 0; mag.z = 0;                        // mag.y left stale

measurement_cm = 0;                          // looks like valid "0cm"

Example code after:

float tpa = 0.0f;
if (ref > 0.0f) tpa = (airspeed - ref) / ref;

if (norm2(v) > 0.01f) normalize(v);

// reset all axes consistently on failure
mag.x = mag.y = mag.z = 0;

// sentinel for "no new data"
measurement_cm = RANGEFINDER_NO_NEW_DATA;

Relevant past accepted suggestions:

Suggestion 1:

Prevent division by zero during normalization

Add a check to ensure vCoGlocal has a non-zero magnitude before normalization to prevent a potential division-by-zero error.

src/main/flight/imu.c [472-482]

-if (vectorNormSquared(&vHeadingEF) > 0.01f) {
+if (vectorNormSquared(&vHeadingEF) > 0.01f && vectorNormSquared(&vCoGlocal) > 0.01f) {
     // Normalize to unit vector
     vectorNormalize(&vHeadingEF, &vHeadingEF);
     vectorNormalize(&vCoGlocal, &vCoGlocal);
 
     // error is cross product between reference heading and estimated heading (calculated in EF)
     vectorCrossProduct(&vCoGErr, &vCoGlocal, &vHeadingEF);
 
     // Rotate error back into body frame
     quaternionRotateVector(&vCoGErr, &vCoGErr, &orientation);
 }

Suggestion 2:

Prevent division-by-zero in TPA calculation

Add a check to prevent division-by-zero when referenceAirspeed is zero in the tpaThrottle calculation, falling back to the standard throttle value if necessary.

src/main/flight/pid.c [501-502]

 const float referenceAirspeed = pidProfile()->fixedWingReferenceAirspeed; // in cm/s
-tpaThrottle = currentControlRateProfile->throttle.pa_breakpoint + (uint16_t)((airspeed - referenceAirspeed) / referenceAirspeed * (currentControlRateProfile->throttle.pa_breakpoint - getThrottleIdleValue()));
+if (referenceAirspeed > 0) {
+    tpaThrottle = currentControlRateProfile->throttle.pa_breakpoint + (uint16_t)((airspeed - referenceAirspeed) / referenceAirspeed * (currentControlRateProfile->throttle.pa_breakpoint - getThrottleIdleValue()));
+} else {
+    // Fallback to regular throttle if reference airspeed is not configured
+    tpaThrottle = rcCommand[THROTTLE];
+}

Suggestion 3:

[learned best practice] Initialize the measurement field to a fail-safe sentinel (e.g., `RANGEFINDER_NO_NEW_DATA`) so startup/first-read states don't appear as a valid `0cm` reading.

Suggestion 4:

Reset all magnetometer axes

The Y-axis is not being reset to zero in case of failed read, which could leave stale data. All three axes should be reset to maintain consistency and prevent potential issues with outdated magnetometer readings.

src/main/drivers/compass/compass_qmc5883p.c [131-133]

 // set magData to zero for case of failed read
 mag->magADCRaw[X] = 0;
+mag->magADCRaw[Y] = 0;
 mag->magADCRaw[Z] = 0;

Pattern 5: Keep behavior and semantics consistent across modules and documentation by aligning shared logic, operand conventions, and API signatures (and updating docs/examples when code behavior changes).

Example code before:

// module A priority
if (FAILSAFE) mode="FS";
else if (MANUAL) mode="MANU";
else if (GEOZONE) mode="GEO";

// module B priority differs
if (FAILSAFE) mode="FS";
else if (GEOZONE) mode="GEO";
else if (MANUAL) mode="MANU";

// docs/example call doesn't match function signature
createLogicCondition(id, op1, op2, rawValue, flags);

Example code after:

// shared/consistent priority order (or centralized helper)
const char *mode = getFlightModeLabel();  // same logic used by both modules

// docs/examples match actual API contract
createLogicCondition(id,
                     (operand_t){ .type=OP_FLIGHT, .field=FIELD_WIND_DIR },
                     (operand_t){ .type=OP_CONST,  .value=123 },
                     flags);

Relevant past accepted suggestions:

Suggestion 1:

Harmonize operand semantics and docs

Mirror the sign and invalid-return conventions with the new relative wind field in docs and keep operand meanings consistent; update docs and ensure both operands follow the same invalid sentinel and frame definitions.

src/main/programming/logic_condition.c [763-785]

-case LOGIC_CONDITION_OPERAND_FLIGHT_WIND_DIRECTION: // deg
+case LOGIC_CONDITION_OPERAND_FLIGHT_WIND_DIRECTION: // deg, 0..359, -1 invalid
 #ifdef USE_WIND_ESTIMATOR
     {
         if (isEstimatedWindSpeedValid()) {
-            uint16_t windAngle;
-            getEstimatedHorizontalWindSpeed(&windAngle);
-            int32_t windHeading = windAngle + 18000; // Correct heading to display correctly.
-    
-            windHeading = (CENTIDEGREES_TO_DEGREES((int)windHeading));
-            while (windHeading < 0) {
-                windHeading += 360;
-            }
-            while (windHeading >= 360) {
-                windHeading -= 360;
-            }
-            return windHeading;
-        } else
+            uint16_t windAngleCd;
+            getEstimatedHorizontalWindSpeed(&windAngleCd);
+            // Convert wind vector angle (towards) to heading-from (meteorological) in degrees [0,360)
+            int32_t headingDeg = CENTIDEGREES_TO_DEGREES((int)(windAngleCd + 18000));
+            while (headingDeg < 0) headingDeg += 360;
+            while (headingDeg >= 360) headingDeg -= 360;
+            return headingDeg;
+        } else {
             return -1;
+        }
     }
 #else
-        return -1;
+    return -1;
 #endif
-        break;
+    break;

Suggestion 2:

Align flight mode priority logic

Align the flight mode priority logic in crsf.c with osd.c. The current implementation has a different priority for MANUAL_MODE and GEOZONE between the two files, which should be synchronized.

src/main/telemetry/crsf.c [356-368]

 #ifdef USE_FW_AUTOLAND
         if (FLIGHT_MODE(NAV_FW_AUTOLAND)) {
             flightMode = "LAND";
         } else
 #endif
         if (FLIGHT_MODE(FAILSAFE_MODE)) {
             flightMode = "!FS!";
+        } else if (FLIGHT_MODE(MANUAL_MODE)) {
+            flightMode = "MANU";
 #ifdef USE_GEOZONE
         } else if (FLIGHT_MODE(NAV_SEND_TO) && !FLIGHT_MODE(NAV_WP_MODE)) {
             flightMode = "GEO";
-#endif            
-        } else if (FLIGHT_MODE(MANUAL_MODE)) {
-            flightMode = "MANU";
+#endif

Suggestion 3:

[learned best practice] Make the replacement guidance exact per command (get vs set) to avoid ambiguity and divergence with docs/clients; reference only the corresponding replacement for each define.

Suggestion 4:

[possible issue] Correct the `createLogicCondition` call in the `codegen.js` example. The function expects four arguments with operand objects, not five arguments with a mix of raw values and objects.

[Auto-generated best practices - 2026-04-01]

.pr_agent_auto_best_practices - iNavFlight/inav GitHub Wiki

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

⚠️ **GitHub.com Fallback** ⚠️

⚠️ GitHub.com Fallback ⚠️