Skip to content

Improve OSD Glide Slope implementation#11615

Open
y-decimal wants to merge 26 commits into
iNavFlight:release/9.1from
y-decimal:osd-glide-refactor
Open

Improve OSD Glide Slope implementation#11615
y-decimal wants to merge 26 commits into
iNavFlight:release/9.1from
y-decimal:osd-glide-refactor

Conversation

@y-decimal
Copy link
Copy Markdown

@y-decimal y-decimal commented Jun 3, 2026

This PR refactors the original instantaneous velocity based OSD Glide Slope implementation to instead use absolute altitude and total travel distance sampling and a linear regression model to calculate a more reliable and stable glide slope.

This leads to a more stable and consistent reading of glide related OSD elements.

This is a remake of my older PR (#11520) which tried to achieve the same results via filtering on the instantaneous velocity inputs, but that proved insufficient so I went with this approach instead.

For reference, there are two new variables added to the CLI settings:

  • name: osd_glide_sample_rate
    description: "Glide slope sampling rate in Hz (1-4 Hz). Higher rates give more responsive glide slope calculations but use more CPU."
    field: glide_sample_rate
    type: uint8_t
    min: 1
    max: 4
    default_value: 2
  • name: osd_glide_sample_time_frame
    description: "Glide slope sampling time frame in seconds (5-60 seconds). Longer frames provide more stable glide slope estimates."
    field: glide_sample_time_frame
    type: uint8_t
    min: 5
    max: 60
    default_value: 10

These values are ultimately user preference, especially the time frame.
Longer time frame = more stable glide slope but slower to react to changes in conditions.
Shorter time frame = more unstable glide slope but faster to react to changes in conditions

@y-decimal
Copy link
Copy Markdown
Author

Hm, not sure why the commit history includes those dshot fixes, that's odd. Might be because I based my branch on master?

@y-decimal
Copy link
Copy Markdown
Author

Anyway, I currently still need to implement the following:

  • Dynamic buffer resets during periods of bad data (steep bank angle, motor on, extended period of time with positive vertical velocity (i.e. in a thermal)
  • Configurable sample rate and sample window to allow user to tweak how reactive the glide data is to short term changes
  • Small, high alpha PT1 filter on the calculated glide slope to smooth out the display just a little

y-decimal added 12 commits June 3, 2026 17:19
…onditions

fix: reset glide buffer when glide ratio conditions are not met
fix: update error display in osdDrawSingleElement function
…raw call and make glideslope available for other functions to allow glideTime and glideDistance to use it for their calculations
refactor: enhance glide range calculation with current glide ratio
fix: call enableGlideRatioCalculation in all osd elements using it
@y-decimal y-decimal marked this pull request as ready for review June 3, 2026 19:07
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Refactor OSD glide slope to use linear regression model

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Refactor glide slope calculation from instantaneous velocity filtering to linear regression model
  using altitude and distance samples
  - Implements lazy-allocated circular buffer for position sampling
  - Adds configurable sample rate (1-4 Hz) and time frame (5-60 seconds) via CLI
  - Calculates glide ratio using least-squares linear regression for stability
• Improve glide-related OSD elements (glide time, glide range, glide slope) to use calculated glide
  ratio
  - Updates glide time calculation to use glide ratio and ground speed
  - Enhances glide range calculation with current glide ratio and altitude
  - Ensures glide ratio calculation runs when any glide element is enabled
• Add validation checks for glide ratio calculation
  - Validates descent conditions and throttle state before calculating
  - Resets buffer when conditions become invalid
  - Requires minimum sample count before providing valid glide ratio
• Fix DShot beeper ignore list handling to prevent unwanted beeping
Diagram
flowchart LR
  A["Position Samples<br/>Distance + Altitude"] -->|"Circular Buffer<br/>Lazy Allocated"| B["Linear Regression<br/>Calculation"]
  B -->|"Glide Ratio"| C["OSD Elements<br/>Glide Time/Range/Slope"]
  D["Validation<br/>Descent + Throttle"] -->|"Valid Data"| B
  E["CLI Settings<br/>Sample Rate/TimeFrame"] -->|"Configure"| A

Loading

Grey Divider

File Changes

1. src/main/io/beeper.c 🐞 Bug fix +3/-1

Fix DShot beeper ignore list handling

• Add beeper timeout check to prevent DShot beeping on "off" state
• Add beeper ignore mask check to respect DShot beacon ignore list
• Fixes issue where DShot beeper would activate incorrectly

src/main/io/beeper.c


2. src/main/io/osd.c ✨ Enhancement +212/-32

Implement linear regression glide slope calculation

• Add glide position sample structure and lazy-allocated circular buffer for storing distance and
 altitude samples
• Implement calculateGlideRatioFromBuffer() using least-squares linear regression to compute glide
 ratio from samples
• Add isDataValidForGlideRatio() validation function checking descent conditions and throttle
 state
• Add updateGlideRatioCalculation() to maintain glide ratio buffer and calculations
• Add enableGlideRatioCalculation() to manage glide element tracking
• Refactor osdGetRemainingGlideTime() to use calculated glide ratio instead of instantaneous
 velocity filtering
• Update OSD_GLIDESLOPE element to use currentGlideRatio from buffer calculations
• Update OSD_GLIDE_TIME_REMAINING element to call enableGlideRatioCalculation()
• Update OSD_GLIDE_RANGE element to calculate range using glide ratio and altitude instead of glide
 time
• Add glide sample rate and time frame settings to OSD configuration defaults
• Call updateGlideRatioCalculation() in osdRefresh() when glide elements are enabled

src/main/io/osd.c


3. src/main/io/osd.h ⚙️ Configuration changes +2/-0

Add glide configuration fields to OSD config

• Add glide_sample_rate field to osdConfig_t for configurable sampling rate in Hz
• Add glide_sample_time_frame field to osdConfig_t for configurable time frame in seconds

src/main/io/osd.h


View more (2)
4. docs/Settings.md 📝 Documentation +20/-0

Document glide slope configuration settings

• Document osd_glide_sample_rate setting with range 1-4 Hz and default value 2
• Document osd_glide_sample_time_frame setting with range 5-60 seconds and default value 10
• Explain that higher sample rates provide more responsive calculations but use more CPU
• Explain that longer time frames provide more stable glide slope estimates

docs/Settings.md


5. src/main/fc/settings.yaml ⚙️ Configuration changes +14/-0

Add glide slope settings to YAML configuration

• Add osd_glide_sample_rate setting definition with type uint8_t, range 1-4, default 2
• Add osd_glide_sample_time_frame setting definition with type uint8_t, range 5-60, default 10
• Include descriptions for both settings explaining their purpose and impact

src/main/fc/settings.yaml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 3, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

Grey Divider


Action required

1. Unvalidated sampleRate divide-by-zero ✓ Resolved 📘 Rule violation ☼ Reliability
Description
osdConfig()->glide_sample_rate is used as a divisor without validating it is non-zero, which can
crash/hang the flight controller if the config value is invalid/corrupted. This violates the
requirement to validate variable/external inputs before use and fail deterministically.
Code

src/main/io/osd.c[R1948-1966]

Evidence
PR Compliance ID 2 requires validating variable/external inputs before use. In
updateGlideRatioCalculation(), sampleRate comes from configuration and is used in `1000 /
sampleRate without any check for 0`, which can cause a deterministic fault when invalid data is
present.

src/main/io/osd.c[1948-1966]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`osdConfig()->glide_sample_rate` and `osdConfig()->glide_sample_time_frame` are treated as trustworthy, but they can be 0/out-of-range (e.g., corrupted config/EEPROM), leading to `1000 / sampleRate` divide-by-zero and other undefined behavior.
## Issue Context
Even though settings metadata defines min/max, runtime code still needs defensive validation because stored config can be invalid.
## Fix Focus Areas
- src/main/io/osd.c[1948-1966]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. realloc(..., 0) UAF risk ✓ Resolved 📘 Rule violation ⛨ Security
Description
ensureGlideBufferAllocated() calls realloc(glideBuffer, requiredSize * sizeof(...)) without
guarding requiredSize == 0, which can free the existing buffer and return NULL, then the code
treats it as an allocation failure and continues assuming the old buffer is kept. This can lead to
use-after-free if invalid config drives requiredSize to 0.
Code

src/main/io/osd.c[R1846-1863]

Evidence
PR Compliance ID 2 requires validating variable inputs before use. The new code uses requiredSize
derived from config to size an allocation without handling the 0 case, and the realloc/NULL
handling comment "keep old buffer" is not guaranteed when realloc(ptr, 0) frees ptr, creating a
memory-safety risk.

src/main/io/osd.c[1848-1863]
src/main/io/osd.c[1948-1954]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ensureGlideBufferAllocated()` does not validate `requiredSize` is non-zero before calling `realloc()`. If `requiredSize == 0`, `realloc(ptr, 0)` may free `ptr` and return `NULL`, but the function treats `NULL` as "allocation failed, keep old buffer", which is unsafe.
## Issue Context
`requiredSize` is derived from configurable values (`glide_sample_rate` * `glide_sample_time_frame`) and should be clamped/validated before use.
## Fix Focus Areas
- src/main/io/osd.c[1846-1863]
- src/main/io/osd.c[1948-1954]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Glide buffer OOB write ✓ Resolved 🐞 Bug ☼ Reliability
Description
When osd_glide_sample_rate or osd_glide_sample_time_frame changes, the glide buffer may be
reallocated to a smaller size but the static glideBufferIndex is not reset.
updateGlideRatioCalculation() then writes to glideBuffer[glideBufferIndex] before applying the
modulo, which can write out of bounds and corrupt heap memory.
Code

src/main/io/osd.c[R1846-1983]

Evidence
The allocator resets only glideBufferCurrentSize on resize, while updateGlideRatioCalculation keeps
static glideBufferIndex and writes using it before modulo wrapping; if the buffer shrinks,
glideBufferIndex can exceed the new buffer size and the pre-modulo write corrupts memory.

src/main/io/osd.c[1844-1871]
src/main/io/osd.c[1947-1984]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The glide-slope ring buffer is resized via `realloc()`, but sampling state (`glideBufferIndex`, `samplesSinceLastClear`, and optionally `glideLastSampleTime`) is not reset when the buffer size changes. If the buffer shrinks, the next sample write uses the stale index and performs the write *before* applying modulo, leading to an out-of-bounds write.
## Issue Context
- `ensureGlideBufferAllocated()` claims to “Reset sample tracking when buffer changes size” but only resets `glideBufferCurrentSize` (which is not used by the sampling code).
- `updateGlideRatioCalculation()` uses static state and writes `glideBuffer[glideBufferIndex]` before wrapping the index.
## Fix Focus Areas
- src/main/io/osd.c[1846-1871]
- src/main/io/osd.c[1947-1997]
### Implementation notes
- Track the previous `bufferSize` inside `updateGlideRatioCalculation()` (or have `ensureGlideBufferAllocated()` report whether it resized).
- If size changed (or if `glideBufferIndex >= bufferSize`), reset:
- `glideBufferIndex = 0`
- `samplesSinceLastClear = 0`
- (optionally) clear buffer / reset `currentGlideRatio`
- Also consider moving the modulo/wrap to occur *before* indexing, e.g. clamp index before writing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Glide range symbol clobbered ✓ Resolved 🐞 Bug ≡ Correctness
Description
The OSD_GLIDE_RANGE invalid-data path writes the placeholder string to buff instead of buff+1,
overwriting the leading SYM_GLIDE_DIST that was just set. This makes the element render without its
intended icon and can also disrupt expected fixed-width formatting.
Code

src/main/io/osd.c[R3349-3356]

Evidence
The code sets buff[0] to SYM_GLIDE_DIST and then immediately calls tfp_sprintf on buff (not buff+1),
which overwrites the symbol. A nearby element shows the correct pattern of writing to buff+1 to keep
the leading symbol intact.

src/main/io/osd.c[3326-3345]
src/main/io/osd.c[3347-3362]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In the `OSD_GLIDE_RANGE` element, the invalid/empty display path uses `tfp_sprintf(buff, ...)` which overwrites `buff[0]` (the glide-distance symbol). The placeholder text should start at `buff + 1` to preserve the icon.
## Issue Context
`buff[0]` is used as a leading symbol for many OSD elements. Nearby code (e.g., glide time) uses `buff + 1` when writing textual content.
## Fix Focus Areas
- src/main/io/osd.c[3348-3362]
### Implementation notes
- Change `tfp_sprintf(buff, "%s%c", "---", SYM_BLANK);` to `tfp_sprintf(buff + 1, "%s%c", "---", SYM_BLANK);`
- Keep the string terminator consistent with the expected field width (currently `buff[5] = '\0'`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Realloc failure disables glide 🐞 Bug ☼ Reliability
Description
If realloc() fails during glide buffer sizing, ensureGlideBufferAllocated() returns 0 even though
the previous buffer remains valid. updateGlideRatioCalculation() treats this as fatal and disables
glide ratio calculation instead of continuing with the existing allocation.
Code

src/main/io/osd.c[R1859-1959]

Evidence
ensureGlideBufferAllocated returns 0 on realloc failure, and updateGlideRatioCalculation immediately
aborts and zeros the glide ratio, even though the comment states the old buffer is kept and could
still be used.

src/main/io/osd.c[1858-1863]
src/main/io/osd.c[1953-1959]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
On `realloc()` failure, the code returns 0 and stops glide calculation, despite the fact that `realloc()` failure leaves the original buffer intact. This causes unnecessary feature dropout under memory pressure.
## Issue Context
- `realloc(oldPtr, newSize)` returns NULL on failure and keeps `oldPtr` valid.
- The current code path returns 0 and forces `currentGlideRatio = 0.0f`.
## Fix Focus Areas
- src/main/io/osd.c[1858-1867]
- src/main/io/osd.c[1953-1959]
### Implementation notes
- If `realloc()` fails and `glideBuffer != NULL`, return `glideBufferAllocatedSize` (current capacity) instead of 0.
- Clamp `requiredSize` down to `glideBufferAllocatedSize` in the caller logic and continue sampling within the smaller buffer.
- Ensure sampling state resets appropriately if the code decides to continue with a different (fallback) buffer size.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/main/io/osd.c Outdated
Comment thread src/main/io/osd.c
Comment thread src/main/io/osd.c
@y-decimal
Copy link
Copy Markdown
Author

y-decimal commented Jun 3, 2026

I've tested this in HITL and it seems to work a lot better than my approach in the old PR. The default values for the osd_glide_sample_time_frame and osd_glide_sample_rate might need adjusting, but they are ultimately preference.
Faster sample rate makes the calculation more robust against rapid spontaneous changes in position data, while longer sample time frames provide an overall more stable reading that is less affected by short term changes, but also means long term changes take longer to be reflected in the number.
I think the default values are a good middle ground, but one could argue for setting them such that they act more similar to the old behaviour, i.e. more reactive to short term changes. Even with a low sample time frame like 5 it would still be a lot more stable than the old estimation.

One thing of note is that this implementation can eat a bit of ram. In the worst case, the buffer stores 240x4x2 = 1920 Bytes of data, I have no reference frame for how much RAM usage is too much ram usage in iNav so I can't really tell if this is a concern or not. In most cases the cost will be lower though, for instance at default values it is actually just 20x4x2 = 160 Bytes.

@sensei-hacker sensei-hacker changed the base branch from release/9.1 to maintenance-9.x June 4, 2026 04:36
@sensei-hacker sensei-hacker changed the base branch from maintenance-9.x to release/9.1 June 4, 2026 04:36
@y-decimal
Copy link
Copy Markdown
Author

I'm gonna keep an updated list of things that still need to be tested here and check it off as I go:

  • Changing OSD profiles between one with glide elements and one without
  • Only one of each glide elements enabled at a time
  • Verify it works outside of HITL
  • Test at negative altitudes (glide slope should work at negative altitudes, glide distance and glide time should not)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants