Skip to content

LED Strip: add rainbow overlay - #11816

Open
HereComesWhitey wants to merge 11 commits into
iNavFlight:masterfrom
HereComesWhitey:rgb-sweep
Open

LED Strip: add rainbow overlay#11816
HereComesWhitey wants to merge 11 commits into
iNavFlight:masterfrom
HereComesWhitey:rgb-sweep

Conversation

@HereComesWhitey

@HereComesWhitey HereComesWhitey commented Aug 24, 2026

Copy link
Copy Markdown

Adds a new RGB sweep/rainbow overlay ('V') to the LED strip system.

When enabled on any LED, the overlay continuously sweeps through the colour spectrum using a fixed-point accumulator for efficient hue advancement. The sweep rate and per-LED hue offset are configurable via two new settings:

  • ledstrip_rainbow_sweep_rate (0-255, default 10) — controls sweep speed. Higher values sweep faster. 0 freezes the rainbow.
  • ledstrip_rainbow_delta_deg (0-359, default 30) — hue offset in degrees between adjacent rainbow LEDs. Larger values spread more of the spectrum across the strip.

Companion configurator PR: iNavFlight/inav-configurator#2714

Testing: Verified on SKYSTARSH743HD with 14 LEDs configured with the rainbow overlay active.

@qodo-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@github-actions

Copy link
Copy Markdown

Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

  • maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.

  • maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.


This is an automated suggestion to help route contributions to the appropriate branch.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

LED strip: add rainbow sweep overlay ('V') with configurable rate and spacing

✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add a new LED strip overlay ('V') that renders a continuously sweeping rainbow.
• Introduce configurable sweep speed and per-LED hue spacing via new settings.
• Bump LED strip config parameter-group version and add defaults for new fields.
Diagram

graph TD
  A["settings.yaml"] --> B[("PG_LED_STRIP_CONFIG")]
  C["io/ledstrip.h"] --> B[("PG_LED_STRIP_CONFIG")]
  B[("PG_LED_STRIP_CONFIG")] --> D["io/ledstrip.c"] --> E(["applyLedRainbowLayer"])
  E(["applyLedRainbowLayer"]) --> F{"LED has RAINBOW overlay?"} --> G["setLedHsv()"] --> H["ws2811UpdateStrip()"]

  subgraph Legend
    direction LR
    _cfg["Config/schema"] ~~~ _pg[("Param group")]
    _fn(["Layer function"]) ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Make sweep rate purely time-based (dt-driven)
  • ➕ Animation speed becomes independent of the LED task cadence or timer quantization
  • ➕ Easier to reason about units (deg/sec) and configurator UX
  • ➖ Requires passing/deriving precise delta time and careful wrap/overflow handling
  • ➖ Slightly more CPU than the current fixed-step accumulator approach
2. Index rainbow LEDs by ‘rainbow position’ (only LEDs with overlay)
  • ➕ Hue spacing applies only across participating LEDs, not across physical indices with gaps
  • ➕ More predictable appearance when rainbow LEDs are sparse/non-contiguous
  • ➖ Needs an extra pass or state to assign rainbow-order indices each frame
  • ➖ Slightly more complexity in the layer implementation
3. Align settings semantics to allow true ‘freeze’
  • ➕ Matches the PR description (0 freezes) and code intent (speed==0 implies no advancement)
  • ➕ Avoids confusing UI/CLI constraints
  • ➖ Requires changing settings.yaml min to 0 and ensuring configurator validation matches

Recommendation: The overall approach (a dedicated timed layer using a fixed-point accumulator) is appropriate for an embedded LED task and keeps CPU cost low. The main follow-up worth considering is aligning the configuration constraints with the stated behavior: the description says sweep_rate=0 freezes, and the code supports it, but settings.yaml currently enforces min=1—either change min to 0 or adjust docs/behavior for consistency. If users commonly configure non-contiguous rainbow LEDs, consider using a ‘rainbow-only’ index for spacing in a later refinement.

Files changed (3) +63 / -4

Enhancement (2) +46 / -4
ledstrip.cImplement timed rainbow overlay layer and register new overlay code 'V' +41/-2

Implement timed rainbow overlay layer and register new overlay code 'V'

• Bumps the LED strip parameter-group version and initializes new rainbow defaults in the reset function. Adds overlay code 'V' and introduces applyLedRainbowLayer(), a 100Hz timed layer using a fixed-point accumulator to advance hue and apply per-LED hue offsets when LED_OVERLAY_RAINBOW is set; wires the layer into the existing timer/layer table.

src/main/io/ledstrip.c

ledstrip.hAdd RAINBOW overlay ID and config fields for sweep parameters +5/-2

Add RAINBOW overlay ID and config fields for sweep parameters

• Increases LED_OVERLAY_COUNT to 8, adds LED_OVERLAY_RAINBOW to the overlay enum, and extends ledStripConfig_t with sweep rate and delta-degree configuration fields for the new overlay.

src/main/io/ledstrip.h

Other (1) +17 / -0
settings.yamlExpose rainbow overlay sweep rate and hue spacing settings +17/-0

Expose rainbow overlay sweep rate and hue spacing settings

• Adds a new PG_LED_STRIP_CONFIG section with two tunables: ledstrip_rainbow_sweep_rate and ledstrip_rainbow_delta_deg, including defaults and bounds. Note: sweep_rate is documented as supporting 0=freeze, but min is set to 1 here.

src/main/fc/settings.yaml

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 24, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. LED strip config wiped ✓ Resolved 🐞 Bug ☼ Reliability
Description
Bumping PG_LED_STRIP_CONFIG version from 2 to 3 will cause pgLoad() to discard/rest defaults instead
of restoring the stored LED strip configuration, wiping existing users’ LED layout/overlays on
firmware upgrade.
Code

src/main/io/ledstrip.c[76]

+PG_REGISTER_WITH_RESET_FN(ledStripConfig_t, ledStripConfig, PG_LED_STRIP_CONFIG, 3);
Evidence
The PR changes the LED strip PG registry version to 3. The config system loads PGs from EEPROM via
pgLoad(), which resets the group and only copies stored bytes when the stored version matches the
registry version; a version bump therefore prevents restoring the previous config and leaves only
defaults.

src/main/io/ledstrip.c[75-77]
src/main/config/config_eeprom.c[221-240]
src/main/config/parameter_group.c[86-94]

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 PR increments the parameter-group version for `PG_LED_STRIP_CONFIG` from 2 to 3. The config loader (`pgLoad`) only restores EEPROM content when the stored version matches the registry version; otherwise it resets the PG to defaults. This will reset/wipe existing LED strip configurations on upgrade.
## Issue Context
`ledStripConfig_t` only appends new fields, so backward-compatible loading can be preserved by keeping the version unchanged; `pgLoad` resets to defaults first and then copies `MIN(stored_size, current_size)` bytes, leaving newly-added tail fields at their defaults.
## Fix Focus Areas
- src/main/io/ledstrip.c[76-76]
- src/main/config/parameter_group.c[86-94]
- src/main/config/config_eeprom.c[221-240]

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


2. Sweep rate 0 rejected ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new ledstrip_rainbow_sweep_rate setting is documented and implemented to allow 0 (freeze), but
settings.yaml sets min: 1, so CLI/validation will reject 0 and the freeze behavior is unreachable
via settings.
Code

src/main/fc/settings.yaml[R4511-4514]

+      description: "Rainbow overlay sweep rate. Higher values sweep faster. 0 freezes the rainbow."
+      default_value: 10
+      field: ledstrip_rainbow_sweep_rate
+      min: 1
Evidence
settings.yaml explicitly says '0 freezes the rainbow' but declares min=1. The runtime checks `if
(speed > 0)` which means 0 is a supported value. The CLI enforces
settingGetMin()/settingGetMax() and will reject values outside the declared range, so users
can’t set 0.

src/main/fc/settings.yaml[4509-4516]
src/main/io/ledstrip.c[851-863]
src/main/fc/cli.c[4008-4060]

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

## Issue description
`ledstrip_rainbow_sweep_rate` is described as allowing `0` to freeze the rainbow, and the runtime code explicitly treats `0` as a valid special value. However, settings metadata sets `min: 1`, which makes `0` invalid and causes the CLI/settings validation to reject it.
## Issue Context
The CLI enforces min/max ranges when setting values, and `settingsValidate()` enforces them as well.
## Fix Focus Areas
- src/main/fc/settings.yaml[4510-4515]
- src/main/fc/cli.c[4008-4060]
- src/main/io/ledstrip.c[851-863]

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



Informational

3. Sweep-rate comment inverted ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The ledstrip_rainbow_sweep_rate field comment states '1=fastest, 255=slowest' but the
implementation and settings description define higher values as faster, making the header comment
misleading for maintainers/users.
Code

src/main/io/ledstrip.h[R158-159]

+    uint8_t ledstrip_rainbow_sweep_rate; // hue sweep rate for rainbow overlay, 1-255, 1=fastest, 255=slowest
+    uint16_t ledstrip_rainbow_delta_deg; // hue offset between adjacent rainbow-overlay LEDs, 0-359 degrees
Evidence
The header comment claims low values are fastest, but the rainbow implementation increments its
accumulator by speed each tick (larger speed => more frequent stepCount increments => faster hue
change) and the setting description also says higher values sweep faster.

src/main/io/ledstrip.h[153-160]
src/main/io/ledstrip.c[851-861]
src/main/fc/settings.yaml[4510-4515]

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 `ledstrip_rainbow_sweep_rate` field comment describes the sweep-rate direction opposite to how the code and setting description behave, which is likely to mislead future changes.
## Issue Context
The rainbow layer advances the accumulator by `speed` and therefore advances hue faster as `speed` increases.
## Fix Focus Areas
- src/main/io/ledstrip.h[153-160]
- src/main/io/ledstrip.c[851-861]
- src/main/fc/settings.yaml[4510-4515]

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


Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/main/io/ledstrip.c Outdated
Comment thread src/main/fc/settings.yaml Outdated
Comment thread src/main/io/ledstrip.h Outdated
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.

1 participant