check_screensize_usage.sh (1)

1-197: Script provides good automation for pattern analysis.

The script effectively scans directories, aggregates pattern matches, and surfaces inconsistencies. It complements the SCREENSIZE_ANALYSIS.md documentation well. Error handling with set -e and 2>/dev/null redirection is appropriate for a scanning utility.

Minor suggestions:

• Consider adding an exit code check after grep commands (some fail silently with || true)
• The logic at lines 64–68 could be simplified using grep's -l flag directly

src/Device/Port/TTYPort.cpp (2)

26-60: Consider using <linux/serial.h> instead of local definitions.

Defining serial_struct locally works but risks divergence from kernel definitions. The kernel's serial struct is part of the stable UAPI, so including <linux/serial.h> would be more maintainable and automatically provide TIOCGSERIAL, TIOCSSERIAL, and ASYNC_SPD_CUST.

If there's a specific reason to avoid the header (e.g., cross-compilation issues), a comment explaining this choice would be helpful.

 #ifdef __linux__
 #include <sys/ioctl.h>
-/* For TIOCSSERIAL method (works on USB-to-serial drivers like FTDI) */
-/* TIOCSSERIAL has been available since kernel 2.2 */
-#ifndef TIOCGSERIAL
-#define TIOCGSERIAL 0x541E
-#endif
-#ifndef TIOCSSERIAL
-#define TIOCSSERIAL 0x541F
-#endif
-/* ASYNC_SPD_CUST may not be defined in very old kernel headers (pre-2.4) */
-#ifndef ASYNC_SPD_CUST
-#define ASYNC_SPD_CUST 0x0030
-#endif
-struct serial_struct {
-  int type;
-  int line;
-  unsigned int port;
-  int irq;
-  int flags;
-  int xmit_fifo_size;
-  int custom_divisor;
-  int baud_base;
-  unsigned short close_delay;
-  char io_type;
-  char reserved_char[1];
-  int hub6;
-  unsigned short closing_wait;
-  unsigned short closing_wait2;
-  unsigned char *iomem_base;
-  unsigned short iomem_reg_shift;
-  unsigned int port_high;
-  unsigned long iomap_base;
-};
+#include <linux/serial.h>
 #endif

---

163-178: Reduce code duplication in termios attribute setup.

The termios configuration (lines 167-178) is nearly identical to the standard path (lines 200-210). Consider extracting a helper function to set the common attributes.

static void
ConfigureRawMode(struct termios &attr) noexcept
{
  attr.c_iflag &= ~(BRKINT | PARMRK | ISTRIP | INLCR | IGNCR | ICRNL | IXON);
  attr.c_iflag |= (IGNPAR | IGNBRK);
  attr.c_oflag &= ~OPOST;
  attr.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
  attr.c_cflag &= ~(CSIZE | PARENB | CRTSCTS);
  attr.c_cflag |= (CS8 | CLOCAL);
  attr.c_cc[VMIN] = 0;
  attr.c_cc[VTIME] = 1;
}

Also applies to: 196-211

build_quick_test.sh (1)

59-60: Remove trailing blank lines.

The script has unnecessary blank lines at the end. This is a minor style issue.

Apply this diff:

 echo "Summary: $SUCCESS successful, $FAILED failed"
-
-
-

build_all_configs.sh (4)

31-33: Use explicit no-op command for file truncation.

Shellcheck warns that these redirections lack commands. While > file works in bash, using : > file or true > file is more explicit and portable.

 # Initialize result files
-> "$RESULTS_FILE"
-> "$FAILED_BUILDS"
-> "$SUCCESSFUL_BUILDS"
+: > "$RESULTS_FILE"
+: > "$FAILED_BUILDS"
+: > "$SUCCESSFUL_BUILDS"

---

128-135: Remove unused TARGET_SPECIFIC_FLAGS variable.

This array is defined but never referenced. If it's intended as documentation, consider converting it to a comment. If it's planned for future use, add a # shellcheck disable=SC2034 annotation or remove it.

-# Flags that are target-specific and should be skipped for certain targets
-TARGET_SPECIFIC_FLAGS=(
-    "ENABLE_SDL"      # Only for UNIX-like targets
-    "USE_FB"          # Only for Linux targets
-    "VFB"             # Only for certain targets
-    "ENABLE_MESA_KMS" # Only for Linux targets
-    "GREYSCALE"       # Only for certain targets
-    "DITHER"          # Only for certain targets
-)
+# Note: Target-specific flags (ENABLE_SDL, USE_FB, VFB, ENABLE_MESA_KMS, 
+# GREYSCALE, DITHER) are filtered in is_valid_combination()

---

192-197: Separate variable declaration and assignment.

Shellcheck warns that combining local declaration with command substitution can mask return values. While low risk here, it's good practice to separate them.

-    local flag_name=$(echo "$flags" | sed 's/ /_/g' | sed 's/=/-/g')
+    local flag_name
+    flag_name=$(echo "$flags" | sed 's/ /_/g' | sed 's/=/-/g')
     build_name="${target}_${flag_name}"
   fi
   
-  local log_file="${LOG_DIR}/${build_name}.log"
-  local start_time=$(date +%s)
+  local log_file start_time
+  log_file="${LOG_DIR}/${build_name}.log"
+  start_time=$(date +%s)

---

210-211: Add comment explaining intentional word splitting.

The unquoted $flags is intentional to allow word splitting for multiple make arguments. Adding a brief comment would clarify this for maintainers and silence potential linter warnings.

     # Build with make
+    # Note: $flags intentionally unquoted for word splitting (multiple make args)
     if make TARGET="$target" $flags > "$log_file" 2>&1; then

src/Device/Driver/LX/Internal.hpp (2)

131-152: Consider consolidating overlapping struct fields.

TrackedPolar and DevicePolar share several common fields (a, b, c, empty_mass/empty_weight). While they serve different purposes (tracking XCSoar's view vs. device-reported data), consider whether a common base or shared structure could reduce duplication.

This is optional since the current separation clearly delineates the two different data sources and their distinct usage patterns.

---

139-139: Minor: Remove extra blank line.

There's a double blank line between tracked_polar and DevicePolar struct definitions.

src/NMEA/Info.cpp (1)

125-127: Reset of IGC pressure altitude looks correct; consider matching expiry semantics

Resetting igc_pressure_altitude_available and igc_pressure_altitude here is good and avoids stale values after a full reset. If igc_pressure_altitude is intended to behave like the other altitude sources, you may also want to expire igc_pressure_altitude_available in NMEAInfo::Expire() with a similar timeout to pressure_altitude_available, to avoid an IGC fix using a very old IGC altitude.

build_logs/successful_builds.txt (1)

1-8: Build log format looks consistent with multi-config workflow

The entries follow a clear, parseable name|status|duration|flags format and align with the described multi-config build logging. If 0s durations are not expected for some configs, it may be worth double-checking the timing logic in the generator script, but the file itself looks fine.

src/Plane/Plane.hpp (1)

39-43: Consider default‑initialising plane_profile_active to avoid indeterminate state

The new plane_profile_active flag is useful for distinguishing default vs. profile-based planes. To guard against default-constructed Plane instances where this flag might not be explicitly set, it would be safer to give it an in-class default:

-  bool plane_profile_active;
+  bool plane_profile_active = false;

This ensures a well-defined default and avoids relying on all construction paths to remember to set it.

BUILD_ALL_CONFIGS.md (2)

79-81: Consider minor wording improvements.

The phrase "Only successful builds" and "Only failed builds" on consecutive lines creates repetition. Consider rewording for better readability:

 The script creates a `build_logs/` directory with:
 - Individual log files for each build (`<target>_<flags>.log`)
 - `build_results.txt` - All results
-- `successful_builds.txt` - Only successful builds
-- `failed_builds.txt` - Only failed builds
+- `successful_builds.txt` - Successful builds only
+- `failed_builds.txt` - Failed builds only

---

104-104: Simplify "with success/failure counts" phrase.

-- The script provides a summary at the end with success/failure counts
+- The script provides a summary at the end showing success/failure counts

src/Dialogs/Settings/dlgBasicSettings.cpp (1)

134-143: Consider using static_cast instead of C-style cast.

The C-style cast on line 137 is flagged by static analysis. Using C++ style casts is preferred for type safety and clarity.

-    DataFieldFloat &df = *(DataFieldFloat *)control.GetDataField();
+    DataFieldFloat &df = *static_cast<DataFieldFloat *>(control.GetDataField());

FLARM_FTD014_SPECIFICATION_ANALYSIS.md (1)

50-52: Add language specifier to fenced code block.

The code block lacks a language specifier, which is flagged by markdownlint.

-```
+```text
 $PFLAC,A,<name>*<checksum>

</blockquote></details>
<details>
<summary>src/Device/Driver/LX/LXNAVVario.hpp (1)</summary><blockquote>

`106-112`: **Consider using `snprintf` for bounds safety.**

While the buffer size (32 bytes) is sufficient for this format string, using `snprintf` would be safer and more consistent with secure coding practices. This applies to other functions in this file as well.



```diff
   static inline void
   SetVolume(Port &port, OperationEnvironment &env, unsigned volume)
   {
     char buffer[32];
-    sprintf(buffer, "PLXV0,VOL,W,%.1f", (double)volume);
+    snprintf(buffer, sizeof(buffer), "PLXV0,VOL,W,%u", volume);
     PortWriteNMEA(port, buffer, env);
   }

Note: The (double) cast and %.1f format are unnecessary for an integer value; %u would be more direct.

src/InfoBoxes/Panel/AltitudeSetup.cpp (2)

48-49: Replace C-style casts with static_cast for DataField types

In OnModified() and Prepare(), the casts:

• DataFieldFloat &df = (DataFieldFloat &)_df;
• DataFieldInteger &df = (DataFieldInteger &)_df;
• DataFieldFloat &df = *(DataFieldFloat *)qnh_control->GetDataField();

are safe given how the controls are created, but trigger the static-analysis warning and are less explicit than necessary.

Consider switching to static_cast to make the intent clear and keep tools quiet:

-  DataFieldFloat &df = (DataFieldFloat &)_df;
+  auto &df = static_cast<DataFieldFloat &>(_df);

-  DataFieldInteger &df = (DataFieldInteger &)_df;
+  auto &df = static_cast<DataFieldInteger &>(_df);

-  DataFieldFloat &df = *(DataFieldFloat *)qnh_control->GetDataField();
+  auto &df = static_cast<DataFieldFloat &>(*qnh_control->GetDataField());

Also applies to: 61-62, 86-89

---

94-113: UI thread blocks up to 1 s while polling device_blackboard

The 10×Sleep(100) loop runs on the UI thread during Prepare(), which can freeze the dialog for up to 1 second if the device is slow or absent. If this becomes noticeable, consider reducing the wait or making the elevation request/refresh asynchronous instead of busy-waiting in the dialog.

src/Device/Driver/LX/Settings.cpp (1)

150-211: Clarify unsupported-device semantics and avoid redundant last_sent_* updates

The new methods generally look good, but there are two small consistency points:

1. Return values for unsupported devices

   • PutCrewMass() / PutEmptyMass() return true when !IsLXNAVVario(), i.e. they no-op but report success.
   • PutElevation() / RequestElevation() return false for non‑LXNAV varios.

   If callers interpret false as “feature not supported” (rather than a hard error), it might be clearer to use the same convention for all three families (either always true/no-op or always false for unsupported devices) and document it. Right now DeviceDescriptor wrappers will see different statuses depending on which setter is called.

2. last_sent_* tracking in two layers

   • PutBallast(), PutBugs(), and PutMacCready() already set last_sent_ballast_overload, last_sent_bugs, and last_sent_mc under mutex.
   • LXDevice::SyncBallast(), SyncBugs(), and SyncMacCready() in Mode.cpp set the same fields again after calling these methods.

   This duplication is harmless but makes ownership of the “last sent” state a bit fuzzy. You can simplify by relying on the driver methods to own last_sent_* updates and dropping the extra assignments in Sync*, which reduces the chance of the two drifting apart during future refactors.

Also applies to: 213-255, 270-295

src/Device/Driver/LX/Mode.cpp (1)

192-544: Verify ballast overload calculation uses the correct mass units

The new OnCalculatedUpdate() / Sync* logic is comprehensive and the direction-of-truth handling (default plane → read from vario, plane profile active → push to vario) is well structured. One detail worth double‑checking is the ballast overload computation in SyncBallast():

const GlidePolar &polar = calculated.glide_polar_safety;
...
const double ballast_litres = polar.GetBallastLitres();
const double dry_mass = polar.GetDryMass();
const double reference_mass = polar.GetReferenceMass();
...
const double overload = (dry_mass + ballast_litres) / reference_mass;
...
PutBallast(0.0, overload, env);

This assumes that ballast_litres can be added directly to dry_mass (i.e. that 1 litre of ballast is effectively treated as 1 kg and that GetBallastLitres() is already “mass-like”). If GlidePolar ever changes to distinguish litres from kilograms more explicitly (e.g. with a density factor or a separate GetBallastMass()), this formula could drift.

It’s probably fine today, but to future‑proof the code you might want to:

• either switch to a mass-returning API if one exists (e.g. GetTotalMass() or GetBallastMassKg()), or
• add a short comment documenting the 1‑litre‑≈‑1‑kg assumption so it’s clear this is intentional.

The rest of the sync logic (MC, bugs, crew/empty mass echo checks, and polar change tracking via tracked_polar and device_polar) looks consistent and avoids unnecessary device traffic.

Also applies to: 578-617

src/Engine/GlideSolvers/GlidePolar.hpp (2)

266-283: Clarify and harden semantics of new ballast setters/getters

The new trio SetBallastLitres / SetBallastFraction / SetBallastOverload plus GetBallastLitres / GetBallastFraction / GetBallastOverload looks like a clean API split, but a couple of edge-case behaviours are worth nailing down in the implementation:

• For SetBallastFraction(const double fraction) (Lines 273-276), it’s not obvious whether callers may pass values outside [0, 1] (or NaNs) and whether you clamp, assert, or allow them through. Given it’s a config-like API, clamping to [0, 1] (or at least documenting the expectation) would avoid surprising states.
• Similarly for SetBallastOverload(const double overload) (Lines 279-282), it would be good to define behaviour for values < 1.0 (or <= 0) and for pathological values. If you treat overload as “total_mass / reference_mass”, then overload < 1 probably deserves either clamping or an assertion.
• For GetBallastFraction() and GetBallastOverload() (Lines 299-304, 305-309), please ensure the implementation handles max_ballast <= 0 and/or reference_mass <= 0 gracefully (e.g. returning 0 rather than risking division by zero or NaNs in degenerate configurations).
• HasBallast() (Lines 287-289) now uses ballast_litres > 0, which is consistent with the new storage. If in practice you often end up with tiny residuals (e.g. numerical noise after dumps), consider using a small epsilon threshold rather than strict > 0, but that’s more of a style choice.

Tightening these behaviours (and documenting them briefly in the comments) would make the new API safer to use in the various UI, device, and test call sites.

Also applies to: 287-297, 299-310

---

49-50: Keep max_ballast, ballast_ratio, and reference_mass invariants explicit

The introduction of ballast_litres and max_ballast plus the changes in SetMaxBallast and SetBallastRatio (Lines 545-558, 566-573) nicely encode a relationship:

• SetMaxBallast() derives ballast_ratio = max_ballast / reference_mass when reference_mass > 0.
• SetBallastRatio() derives max_ballast = ballast_ratio * reference_mass when reference_mass > 0.

A couple of design points are worth considering to avoid surprising states:

• SetReferenceMass() (Lines 507-512) currently just updates reference_mass and calls Update(), without re-synchronising max_ballast / ballast_ratio. If the intent is that these three always satisfy the same physical relation, you might want to recompute one from the other in SetReferenceMass() (or document that changing reference_mass does not affect the configured capacity/ratio).
• Conversely, if you intend max_ballast and ballast_ratio to be independent configuration knobs, automatically deriving one from the other inside SetMaxBallast() / SetBallastRatio() may be surprising. In that case, it may be clearer to drop the implicit recomputation and leave them orthogonal.
• SetMaxBallast() currently always calls Update() by default, although changing the capacity alone doesn’t change the actual all-up mass unless you also change the current ballast. If Update() is relatively expensive in real use (e.g. called often from settings flows), you might consider making update=false the default here or deferring updates to the point where current ballast or mass really changes.

Clarifying and, if needed, slightly refactoring these interactions would make the invariants around ballast configuration more obvious to future maintainers.

Also applies to: 76-80, 545-558, 566-573

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: lordfolken
Repo: XCSoar/XCSoar PR: 0
File: :0-0
Timestamp: 2025-09-04T20:52:26.947Z
Learning: In XCSoar's ApplyExternalSettings.cpp:178, when processing transponder data from external devices like Air Control Display, both transponder_code.IsDefined() and transponder_mode.IsDefined() must be checked before calling ActionInterface::SetTransponderCode() to prevent assertion failures when the transponder is switched off.

Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1714
File: src/ProgressWindow.cpp:0-0
Timestamp: 2025-08-02T01:25:49.521Z
Learning: In XCSoar's ProgressWindow.cpp, the iOS layout positioning issues that previously required magic number workarounds (3x progress_height multipliers) have been resolved with a proper fix that eliminates the need for hardcoded adjustments.

Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1714
File: src/ProgressWindow.cpp:0-0
Timestamp: 2025-08-02T01:25:49.521Z
Learning: In XCSoar's ProgressWindow.cpp, iOS layout positioning issues that previously required magic number workarounds (3x progress_height multipliers) and platform-specific conditional code have been resolved with a proper fix, eliminating the need for hardcoded adjustments in the UpdateLayout method.