.pr_agent_accepted_suggestions - iNavFlight/inav GitHub Wiki

✅ Fix incorrect manual yaw rate display

Correct the OSD element for Manual Yaw Rate (OSD_MANUAL_YAW_RATE) to use the manual yaw rate value and adjustment function instead of the stabilized ones.

src/main/io/osd.c [3344-3346]

 case OSD_MANUAL_YAW_RATE:
-    osdDisplayAdjustableDecimalValue(elemPosX, elemPosY, "MYR", 0, currentControlProfile->stabilized.rates[FD_YAW], 3, 0, ADJUSTMENT_YAW_RATE);
+    osdDisplayAdjustableDecimalValue(elemPosX, elemPosY, "MYR", 0, currentControlProfile->manual.rates[FD_YAW], 3, 0, ADJUSTMENT_MANUAL_YAW_RATE);
     return true;

Suggestion importance[1-10]: 7

__

Why: This is a valid bug fix for the OSD, as OSD_MANUAL_YAW_RATE was incorrectly using the stabilized yaw rate instead of the manual one, which would display the wrong value and adjust the wrong setting.

✅ Remove leftover debugging code

Remove the call to gvSet from logicConditionGetFlightOperandValue to eliminate unintended side effects, as it appears to be leftover debugging code.

src/main/programming/logic_condition.c [793]

-gvSet(3, DECIDEGREES_TO_DEGREES(attitude.values.yaw));
 
+

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that a 'getter' function should not have side effects, and the gvSet call is likely leftover debugging code that could cause unpredictable behavior.

✅ 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 importance[1-10]: 6

__

Why: Relevant best practice - Align mode/enum priorities and documented fields across modules and docs to keep a single source of truth.

✅ Normalize angles in one unit

Keep all math in centidegrees until final conversion and normalize once; avoid redundant casts and flips that can introduce off-by-one errors.

src/main/programming/logic_condition.c [787-816]

-case LOGIC_CONDITION_OPERAND_FLIGHT_RELATIVE_WIND_OFFSET: // deg
+case LOGIC_CONDITION_OPERAND_FLIGHT_RELATIVE_WIND_OFFSET: // deg, signed [-180,180], 0 = headwind, left negative
 #ifdef USE_WIND_ESTIMATOR
     {
         if (isEstimatedWindSpeedValid()) {
-            uint16_t windAngle;
-            getEstimatedHorizontalWindSpeed(&windAngle);
-            gvSet(3, DECIDEGREES_TO_DEGREES(attitude.values.yaw));
-            int32_t relativeWindHeading = windAngle + 18000 - DECIDEGREES_TO_CENTIDEGREES(attitude.values.yaw);
-    
-            relativeWindHeading = (CENTIDEGREES_TO_DEGREES((int)relativeWindHeading));
-            while (relativeWindHeading < 0) {
-                relativeWindHeading += 360;
-            }
-            while (relativeWindHeading >= 360) {
-                relativeWindHeading -= 360;
-            }
-            
-            relativeWindHeading = -relativeWindHeading;
-            if (relativeWindHeading <= -180) {
-                relativeWindHeading = 180 + (relativeWindHeading + 180);
-            }
-
-            return relativeWindHeading;
-        } else
+            uint16_t windAngleCd;
+            getEstimatedHorizontalWindSpeed(&windAngleCd); // [0,36000)
+            // Convert wind vector angle (towards) to heading-from
+            int32_t windHeadingCd = (int32_t)windAngleCd + 18000; // centidegrees
+            // Normalize to [0,36000)
+            while (windHeadingCd < 0) windHeadingCd += 36000;
+            while (windHeadingCd >= 36000) windHeadingCd -= 36000;
+            // Aircraft yaw is in decidegrees; convert to centidegrees
+            int32_t yawCd = DECIDEGREES_TO_CENTIDEGREES(attitude.values.yaw);
+            // Relative offset: wind heading relative to aircraft nose (right positive)
+            int32_t relCd = windHeadingCd - yawCd; // centidegrees
+            // Normalize to (-18000, 18000]
+            while (relCd <= -18000) relCd += 36000;
+            while (relCd > 18000) relCd -= 36000;
+            // Convert to degrees with sign convention: left negative
+            int32_t relDeg = CENTIDEGREES_TO_DEGREES(relCd);
+            return relDeg;
+        } else {
             return 0;
+        }
     }
 #else
-        return 0;
+    return 0;
 #endif
-        break;
+    break;

Suggestion importance[1-10]: 5

__

Why: Relevant best practice - Use correct operators and types; avoid unnecessary casts and ensure angle normalization matches units without mixing decidegrees/centidegrees.

✅ 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 importance[1-10]: 8

__

Why: The suggestion correctly identifies that the PR fails to fully align the flight mode logic in crsf.c with osd.c, which was an explicit goal, thus fixing a bug introduced by the PR.

✅ 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 importance[1-10]: 9

__

Why: The suggestion correctly identifies that a generated build file containing user-specific absolute paths (/Users/ahmed/...) has been added, which will break the build for other developers and is bad practice for version control.

✅ 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 importance[1-10]: 8

__

Why: The suggestion correctly identifies a missing payload size check, which could lead to a buffer over-read and undefined behavior. This is a significant robustness and security improvement.

✅ Prevent use of uninitialized variable

In osdEraseCustomItem, add a default case to the switch statement that returns from the function. This prevents unintended side effects when an item that is not a custom element is passed.

src/main/io/osd.c [6593-6620]

 switch(item){
     case 147:
         customElementIndex = 0;
         break;
     case 148:
         customElementIndex = 1;
         break;
     case 149:
         customElementIndex = 2;
         break;
     case 154:
         customElementIndex = 3;
         break;
     case 155:
         customElementIndex = 4;
         break;
     case 156:
         customElementIndex = 5;
         break;
     case 157:
         customElementIndex = 6;
         break;
     case 158:
         customElementIndex = 7;
         break;
+    default:
+        return;
 }
 
 uint8_t len = customElementLength(customElementIndex);

Suggestion importance[1-10]: 7

__

Why: While the variable customElementIndex is initialized, the suggestion correctly identifies that an invalid item would cause the function to unintentionally modify the element at index 0, which is a logic bug.

✅ 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"
-...
-

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that a generated build file, which includes user-specific absolute paths, should not be committed to version control, as this is a critical repository health and build process issue.

✅ Avoid using a hardcoded layout index

Instead of hardcoding the OSD layout index to 0, consider using the currently active layout or extending the MSP command to allow specifying the layout index.

src/main/fc/fc_msp.c [2723]

-osdLayoutsConfigMutable()->item_pos[0][item] = sbufReadU16(src) | (1 << 13);
+// Suggestion: use the current layout instead of a hardcoded one.
+// Note: `currentLayout` is a global variable defined in `osd.c` and might need to be made accessible here.
+extern uint8_t currentLayout;
+osdLayoutsConfigMutable()->item_pos[currentLayout][item] = sbufReadU16(src) | (1 << 13);

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out the use of a hardcoded layout index 0, which limits the feature's flexibility. Proposing to use the current layout or a new MSP parameter is a valid design improvement.

✅ Fix broken enum links in tables

Description: Sets navigation position hold and general manual/auto flight parameters.

Request Payload: -| Field | C Type | Size (Bytes) | Units | Description | +|Field|C Type|Size (Bytes)|Units|Description| |---|---|---|---|---| | userControlMode | uint8_t | 1 | ENUM_NAME | Sets navConfigMutable()->general.flags.user_control_mode | | maxAutoSpeed|uint16_t| 2 | cm/s | SetsnavConfigMutable()->general.max_auto_speed | @@ -442,7 +441,7 @@ | maxManualSpeed | uint16_t | 2 | cm/s | Sets navConfigMutable()->general.max_manual_speed | | maxManualClimbRate|uint16_t| 2 | cm/s | Setsfw.max_manual_climb_rateormc.max_manual_climb_rate | | mcMaxBankAngle | uint8_t | 1 | degrees | Sets navConfigMutable()->mc.max_bank_angle | -| mcAltHoldThrottleType|uint8_t| 1 | [navMcAltHoldThrottle_e](https://github.com/iNavFlight/inav/wiki/inav_enums_ref.md#enum-navmcaltholdthrottle_e) | EnumnavMcAltHoldThrottle_eSets 'navConfigMutable()->mc.althold_throttle_type' | +|mcAltHoldThrottleType|uint8_t| 1 | [navMcAltHoldThrottle_e](https://github.com/iNavFlight/inav/wiki/Enums-reference#enum-navmcaltholdthrottle_e) | EnumnavMcAltHoldThrottle_eSets 'navConfigMutable()->mc.althold_throttle_type' | |mcHoverThrottle|uint16_t| 2 | PWM | SetscurrentBatteryProfileMutable->nav.mc.hover_throttle |

**Fix the broken link for navMcAltHoldThrottle_e in the MSP_NAV_POSHOLD table to point to the correct enum documentation.**

[docs/development/msp/msp_ref.md [430]](https://github.com/iNavFlight/inav/pull/11088/files#diff-bd5d16f1d7479f638d11be77b5b1f15c8adeca02e5c0abc7426f046ece93fd95R430-R430)

```diff
-| `mcAltHoldThrottleType` | `uint8_t` | 1 | [navMcAltHoldThrottle_e](https://github.com/iNavFlight/inav/wiki/inav_enums_ref.md#enum-navmcaltholdthrottle_e) | Enum `navMcAltHoldThrottle_e` Sets 'navConfigMutable()->mc.althold_throttle_type' |
+| `mcAltHoldThrottleType` | `uint8_t` | 1 | [navMcAltHoldThrottle_e](https://github.com/xznhj8129/msp_documentation/blob/master/docs/inav_enums_ref.md#navmcaltholdthrottle_e) | Enum `navMcAltHoldThrottle_e` Sets 'navConfigMutable()->mc.althold_throttle_type' |

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies and fixes a broken link, improving the documentation's usability by pointing to the correct reference mentioned in the document's header.

✅ Avoid premature type casting to float

In osdGet3DSpeed, declare vert_speed and hor_speed as float to prevent loss of precision from premature casting before the Pythagorean calculation.

src/main/io/osd_common.c [202-207]

 int16_t osdGet3DSpeed(void)
 {
-    int16_t vert_speed = getEstimatedActualVelocity(Z);
-    int16_t hor_speed = gpsSol.groundSpeed;
+    float vert_speed = getEstimatedActualVelocity(Z);
+    float hor_speed = gpsSol.groundSpeed;
     return (int16_t)calc_length_pythagorean_2D(hor_speed, vert_speed);
 }

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a loss of precision by prematurely casting a float return value to int16_t before a calculation, and the proposed change improves the accuracy of the result.

✅ Consider a unified target approach

Instead of creating three separate build targets for the board's different pin configurations, consolidate them into a single target. The pinout variations should be managed through runtime configuration, such as CLI presets.

src/main/target/VANTAC_RF007/CMakeLists.txt [1-3]

target_stm32f722xe(VANTAC_RF007)
target_stm32f722xe(VANTAC_RF007_9SERVOS)
target_stm32f722xe(VANTAC_RF007_NOI2C)

src/main/target/VANTAC_RF007/target.h [50-119]

#if defined(VANTAC_RF007) || defined(VANTAC_RF007_9SERVOS)
#define USE_I2C_DEVICE_2 // clashes with UART3
#define I2C2_SCL                PB10
#define I2C2_SDA                PB11
#define DEFAULT_I2C BUS_I2C2
#else
#define DEFAULT_I2C BUS_I2C1
#endif

#define USE_BARO

 ... (clipped 60 lines)

// src/main/target/VANTAC_RF007/CMakeLists.txt
target_stm32f722xe(VANTAC_RF007)
target_stm32f722xe(VANTAC_RF007_9SERVOS)
target_stm32f722xe(VANTAC_RF007_NOI2C)

// src/main/target/VANTAC_RF007/target.h
#if defined(VANTAC_RF007)
#define MAX_PWM_OUTPUT_PORTS        7
#define USE_UART1
...
#elif defined(VANTAC_RF007_9SERVOS)
#define MAX_PWM_OUTPUT_PORTS        9
...
#elif defined(VANTAC_RF007_NOI2C)
#define MAX_PWM_OUTPUT_PORTS        9
#define USE_UART3
...
#endif

// src/main/target/VANTAC_RF007/CMakeLists.txt
target_stm32f722xe(VANTAC_RF007)

// src/main/target/VANTAC_RF007/target.h
// Define all possible resources for a single target
#define MAX_PWM_OUTPUT_PORTS 9
#define USE_UART1
#define USE_UART3
#define USE_I2C_DEVICE_1
#define USE_I2C_DEVICE_2
...

// In a configuration file or via CLI presets, users can select the desired pinout.
// Example preset for "VANTAC_RF007" default:
// resource MOTOR 8 NONE
// resource MOTOR 9 NONE
// resource SERIAL_TX 3 NONE
// resource SERIAL_RX 3 NONE

Suggestion importance[1-10]: 8

__

Why: The suggestion addresses a significant architectural choice, proposing a unified target which improves maintainability and user experience over the PR's multi-target approach using compile-time flags.

✅ Use dot operator for struct

Replace the incorrect pointer member access operator -> with the direct member access operator . when accessing fields of satelites[i], as it is a struct, not a pointer.

src/main/io/gps_ublox.c [748-751]

 for(int i =_buffer.svinfo.numSvs; i < UBLOX_MAX_SIGNALS; ++i) {
-    satelites[i]->gnssId = 0xFF;
-    satelites[i]->svId = 0xFF;
+    satelites[i].gnssId = 0xFF;
+    satelites[i].svId = 0xFF;
 }

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug in the PR where the -> operator is used on a struct instance (satelites[i]) instead of a pointer, which would cause a compilation error.

✅ Initialize static variable at declaration

Initialize the static variable mavSystemId to a default value of 1 at declaration. This prevents it from being zero-initialized to an invalid system ID, making the code more robust.

src/main/telemetry/mavlink.c [186-187]

 // Set mavSystemId from telemetryConfig()->mavlink.sysid
-static uint8_t mavSystemId;
+static uint8_t mavSystemId = 1;

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the static mavSystemId will be zero-initialized, which is an invalid ID for a vehicle. Initializing it to a safe default at declaration is a good defensive programming practice that improves robustness.

✅ Use logical AND for conditions

Replace the bitwise AND operator (&) with the logical AND operator (&&) in the if condition to correctly evaluate the two boolean expressions.

src/main/telemetry/mavlink.c [1144]

-if (IS_RC_MODE_ACTIVE(BOXGCSNAV) & (posControl.navState == NAV_STATE_POSHOLD_3D_IN_PROGRESS)) {
+if (IS_RC_MODE_ACTIVE(BOXGCSNAV) && (posControl.navState == NAV_STATE_POSHOLD_3D_IN_PROGRESS)) {

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a common C programming error where a bitwise AND (&) is used instead of a logical AND (&&), which could lead to incorrect behavior depending on the return value of IS_RC_MODE_ACTIVE.

✅ Handle yaw from MAVLink command

Handle the yaw parameter (param4) from the MAV_CMD_DO_REPOSITION command instead of ignoring it, by assigning it to wp.p1 to control the vehicle's heading.

src/main/telemetry/mavlink.c [1150-1153]

-wp.p1 = wp.p2 = wp.p3 = 0;
+if (!isnan(msg.param4) && msg.param4 >= 0.0f && msg.param4 < 360.0f) {
+    wp.p1 = msg.param4;
+} else {
+    wp.p1 = -1; // Use -1 to indicate no heading change
+}
+wp.p2 = wp.p3 = 0;
 wp.flag = 0;
 
 setWaypoint(255, ℘);

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the yaw parameter from the MAV_CMD_DO_REPOSITION command is being ignored, which is a missed feature. Implementing this would improve functionality and adherence to the MAVLink specification.

✅ Support additional MAVLink coordinate frames

Add support for MAV_FRAME_GLOBAL_RELATIVE_ALT and MAV_FRAME_GLOBAL_TERRAIN_ALT coordinate frames to improve compatibility with various Ground Control Stations.

src/main/telemetry/mavlink.c [1131-1142]

-if (msg.frame != MAV_FRAME_GLOBAL) {
+if (msg.frame != MAV_FRAME_GLOBAL && msg.frame != MAV_FRAME_GLOBAL_RELATIVE_ALT && msg.frame != MAV_FRAME_GLOBAL_TERRAIN_ALT) {
 
         mavlink_msg_command_ack_pack(mavSystemId, mavComponentId, &mavSendMsg,
                                     msg.command,
                                     MAV_RESULT_UNSUPPORTED,
                                     0,  // progress
                                     0,  // result_param2
                                     mavRecvMsg.sysid,
                                     mavRecvMsg.compid);
         mavlinkSendMessage();
         return true;
     }

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that supporting only MAV_FRAME_GLOBAL is too restrictive and could cause compatibility issues with GCS software. Adding support for other common global frames would make the implementation more robust.

✅ Fix typo in preprocessor directive

Fix the typo NEXUS_9SERVOS to NEXUSX_9SERVOS in the preprocessor directive to ensure correct I2C configuration for that target variant.

src/main/target/NEXUSX/target.h [50-57]

-#if defined(NEXUSX) || defined(NEXUS_9SERVOS)
+#if defined(NEXUSX) || defined(NEXUSX_9SERVOS)
 #define USE_I2C_DEVICE_2 // clashes with UART3
 #define I2C2_SCL                PB10
 #define I2C2_SDA                PB11
 #define DEFAULT_I2C BUS_I2C2
 #else
 #define DEFAULT_I2C BUS_I2C3
 #endif

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a typo in a preprocessor directive (NEXUS_9SERVOS instead of NEXUSX_9SERVOS) that would cause an incorrect I2C bus to be selected for a specific build variant, breaking its functionality.

✅ Correct maximum PWM output ports

Conditionally define MAX_PWM_OUTPUT_PORTS to 9 for the NEXUSX_9SERVOS and NEXUSX_NOI2C variants to match their 9 defined servo outputs.

src/main/target/NEXUSX/target.h [161]

+#if defined(NEXUSX_9SERVOS) || defined(NEXUSX_NOI2C)
+#define MAX_PWM_OUTPUT_PORTS        9
+#else
 #define MAX_PWM_OUTPUT_PORTS        8
+#endif

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that MAX_PWM_OUTPUT_PORTS is hardcoded to 8, while two target variants define 9 servo outputs, which would prevent the 9th servo from functioning on those variants.

✅ Remove unsupported magnetometer from sensors

Remove the unsupported SENSOR_MAG from the SENSORS_SET macro, as the target hardware does not include a magnetometer.

src/main/target/NEXUSX/target.h [129]

-#define SENSORS_SET (SENSOR_ACC|SENSOR_MAG|SENSOR_BARO)
+#define SENSORS_SET (SENSOR_ACC|SENSOR_BARO)

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that an unsupported magnetometer (SENSOR_MAG) is enabled in SENSORS_SET, which could cause initialization issues. This is a valid bug fix that improves the target's stability.

✅ 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 importance[1-10]: 9

__

Why: The suggestion correctly identifies a potential division-by-zero bug when vCoGlocal is a zero vector, which can occur if there is no GPS-measured acceleration. This prevents NaN propagation into the attitude estimation, which is a critical issue.

✅ Update max enum values

Update CURRENT_SENSOR_MAX and VOLTAGE_SENSOR_MAX in their respective enums to CURRENT_SENSOR_SMARTPORT to correctly reflect the new maximum value.

src/main/sensors/battery_config_structs.h [28-45]

 typedef enum {
     CURRENT_SENSOR_NONE = 0,
     CURRENT_SENSOR_ADC,
     CURRENT_SENSOR_VIRTUAL,
     CURRENT_SENSOR_FAKE,
     CURRENT_SENSOR_ESC,
     CURRENT_SENSOR_SMARTPORT,
-    CURRENT_SENSOR_MAX = CURRENT_SENSOR_FAKE
+    CURRENT_SENSOR_MAX = CURRENT_SENSOR_SMARTPORT
 } currentSensor_e;
 
 typedef enum {
     VOLTAGE_SENSOR_NONE = 0,
     VOLTAGE_SENSOR_ADC,
     VOLTAGE_SENSOR_ESC,
     VOLTAGE_SENSOR_FAKE,
     VOLTAGE_SENSOR_SMARTPORT,
-    VOLTAGE_SENSOR_MAX = VOLTAGE_SENSOR_FAKE
+    VOLTAGE_SENSOR_MAX = VOLTAGE_SENSOR_SMARTPORT
 } voltageSensor_e;

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug where the _MAX values in two enums were not updated after adding new members, which could lead to out-of-bounds memory access.

✅ The TPA implementation is overly complex

The current TPA implementation is overly complex, creating a "virtual throttle" from airspeed. It should be replaced with a more direct approach that scales PID gains inversely with dynamic pressure (airspeed^2).

src/main/flight/pid.c [498-503]

    if (usedPidControllerType == PID_TYPE_PIFF && pitotValidForAirspeed() && currentControlRateProfile->throttle.airspeed_tpa) {
        // Use airspeed instead of throttle for TPA calculation
        const float airspeed = getAirspeedEstimate(); // in cm/s
        const float referenceAirspeed = pidProfile()->fixedWingReferenceAirspeed; // in cm/s
        tpaThrottle = currentControlRateProfile->throttle.pa_breakpoint + (uint16_t)((airspeed - referenceAirspeed) / referenceAirspeed * (currentControlRateProfile->throttle.pa_breakpoint - getThrottleIdleValue()));
        //upper and lower limits will be applied in calculateFixedWingTPAFactor()

function updatePIDCoefficients() {
  // ...
  tpaThrottle = 0;
  if (airspeed_tpa_enabled && airspeed_is_valid) {
    // Convert airspeed to a "virtual throttle" value
    airspeed = getAirspeedEstimate();
    referenceAirspeed = pidProfile()->fixedWingReferenceAirspeed;
    tpaThrottle = breakpoint + (airspeed - referenceAirspeed) / referenceAirspeed * (breakpoint - idle_throttle);
  } else {
    tpaThrottle = rcCommand[THROTTLE];
  }

  // Feed virtual throttle into the old TPA formula
  tpaFactor = calculateFixedWingTPAFactor(tpaThrottle);

  // Scale PIDs
  pidState.kP = baseP * tpaFactor;
  // ...
}

function updatePIDCoefficients() {
  // ...
  tpaFactor = 1.0;
  if (airspeed_tpa_enabled && airspeed_is_valid) {
    // Directly calculate scaling factor based on dynamic pressure (airspeed^2)
    airspeed = getAirspeedEstimate();
    referenceAirspeed = pidProfile()->fixedWingReferenceAirspeed;
    if (airspeed > 0) {
      tpaFactor = (referenceAirspeed * referenceAirspeed) / (airspeed * airspeed);
    }
    // Apply user-configured TPA amount and limits
    tpaFactor = 1.0 + (tpaFactor - 1.0) * (tpa_rate / 100.0);
    tpaFactor = constrain(tpaFactor, 0.5, 2.0);
  } else {
    tpaFactor = calculateFixedWingTPAFactor(rcCommand[THROTTLE]);
  }
  // Scale PIDs
  pidState.kP = baseP * tpaFactor;
  // ...
}

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a convoluted design choice and proposes a more direct, physically-grounded, and standard industry approach, significantly improving the feature's clarity and tunability.

✅ 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 importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical division-by-zero risk in the new TPA calculation, which could cause a flight controller crash or loss of control.

✅ 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;

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where only the X and Z axes are cleared, potentially leaving stale data in the Y axis upon a failed read, which could lead to incorrect sensor fusion.

✅ Use static allocation for array

The array servoMixerSwitchHelper is allocated on the stack with a size that could be large (MAX_SERVO_RULES/2). This could cause stack overflow in embedded systems with limited stack space. Consider using static allocation or dynamic allocation instead.

src/main/flight/servos.c [211-227]

 //move the rate filter to new servo rules
 int maxMoveFilters = MAX_SERVO_RULES/2;
 int movefilterCount = 0;
-servoMixerSwitch_t servoMixerSwitchHelper[maxMoveFilters]; // helper to keep track of servoSpeedLimitFilter of servo rules
+static servoMixerSwitch_t servoMixerSwitchHelper[MAX_SERVO_RULES/2]; // helper to keep track of servoSpeedLimitFilter of servo rules
 memset(servoMixerSwitchHelper, 0, sizeof(servoMixerSwitchHelper));
 for (int i = 0; i < servoRuleCount; i++) {
     if(currentServoMixer[i].inputSource == INPUT_MIXER_SWITCH_HELPER || movefilterCount >= maxMoveFilters) {
         break;
     }
     if(currentServoMixer[i].speed != 0 && servoSpeedLimitFilter[i].state !=0) {
         servoMixerSwitchHelper[movefilterCount].targetChannel = currentServoMixer[i].targetChannel;
         servoMixerSwitchHelper[movefilterCount].speed = currentServoMixer[i].speed;
         servoMixerSwitchHelper[movefilterCount].rate = currentServoMixer[i].rate;
         servoMixerSwitchHelper[movefilterCount].speedLimitFilterState = servoSpeedLimitFilter[i].state;
         movefilterCount++;
     }
 }

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a potential stack overflow risk by allocating a large array on the stack in an embedded environment and proposes using static allocation, which is a critical fix for system stability.

✅ Remove duplicate macro definition

The USE_OSD macro is defined twice in the file. This duplication is unnecessary and could lead to preprocessor warnings or confusion during compilation.

src/main/target/HUMMINGBIRD_FC305/target.h [81-161]

 #define USE_OSD
 #define USE_MAX7456
-MAX7456_SPI_BUS		    BUS_SPI3
+#define MAX7456_SPI_BUS		    BUS_SPI3
 #define MAX7456_CS_PIN          PC13
-...
-#define USE_OSD

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that the USE_OSD macro is defined twice (lines 81 and 161), and removing the redundant definition improves code quality.

✅ Use correct timer auto-detection macro

In src/main/target/BOTWINGF722/target.c, replace TIM_USE_ANY with the correct TIM_USE_OUTPUT_AUTO macro for all motor and servo timer definitions to ensure proper timer allocation.

src/main/target/BOTWINGF722/target.c [19-25]

-// not able to fins any "TIM_USE_AUTO" DEFINED INSIDE THE typedef enum in timer.h file so used "   TIM_USE_ANY" INSTEAD .
-DEF_TIM(TIM3, CH1, PB4,  TIM_USE_ANY, 0, 0), // S1
-DEF_TIM(TIM3, CH2, PB5,  TIM_USE_ANY, 0, 0), // S2
-DEF_TIM(TIM3, CH3, PB0,  TIM_USE_ANY, 0, 0), // S3
-DEF_TIM(TIM3, CH4, PB1,  TIM_USE_ANY, 0, 0), // S4
-DEF_TIM(TIM2, CH1, PA15, TIM_USE_ANY, 0, 0),// Servo 1
-DEF_TIM(TIM2, CH2, PB3,  TIM_USE_ANY, 0, 0),// Servo 2
+DEF_TIM(TIM3, CH1, PB4,  TIM_USE_OUTPUT_AUTO, 0, 0), // S1
+DEF_TIM(TIM3, CH2, PB5,  TIM_USE_OUTPUT_AUTO, 0, 0), // S2
+DEF_TIM(TIM3, CH3, PB0,  TIM_USE_OUTPUT_AUTO, 0, 0), // S3
+DEF_TIM(TIM3, CH4, PB1,  TIM_USE_OUTPUT_AUTO, 0, 0), // S4
+DEF_TIM(TIM2, CH1, PA15, TIM_USE_OUTPUT_AUTO, 0, 0),// Servo 1
+DEF_TIM(TIM2, CH2, PB3,  TIM_USE_OUTPUT_AUTO, 0, 0),// Servo 2

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that TIM_USE_OUTPUT_AUTO should be used instead of TIM_USE_ANY, fixing a mistake the developer explicitly noted in a comment, which improves correctness.

✅ Configure second PINIO box as defined

Configure the second PINIO box (USER2) in src/main/target/BOTWINGF722/config.c to match the two PINIO pins defined in the corresponding target.h file.

src/main/target/BOTWINGF722/config.c [25]

 pinioBoxConfigMutable()->permanentId[0] = BOX_PERMANENT_ID_USER1;
+pinioBoxConfigMutable()->permanentId[1] = BOX_PERMANENT_ID_USER2;

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the configuration for the second PINIO pin, defined in the header, is missing, likely an oversight. Applying this change would enable the intended hardware feature.