LED strip: add rainbow overlay GUI controls - #2714
Open
HereComesWhitey wants to merge 2 commits into
Open
Conversation
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 |
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Summary by QodoLED strip: add Rainbow ('V') overlay toggle and settings controls
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
Code Review by Qodo
1. NaN rainbow delta saved
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+830
to
+833
| var freq = Math.max(1, Math.min(255, parseInt($('#rainbowFreqInput').val(), 10) || 1)); | ||
| var delta = parseInt($('#rainbowDeltaInput').val(), 10); | ||
|
|
||
| mspHelper.setSetting('ledstrip_rainbow_sweep_rate', freq, function () { |
There was a problem hiding this comment.
1. Nan rainbow delta saved 🐞 Bug ≡ Correctness
On Save, led_strip.js parses the rainbow delta with parseInt(...) without a default/clamp, so an empty/disabled input becomes NaN and is encoded as 0 when sent via MSPHelper.setSetting, overwriting the FC setting unexpectedly. This can silently reset ledstrip_rainbow_delta_deg (and potentially write out-of-range values) even when the overlay isn’t active.
Agent Prompt
### Issue description
Saving the LED strip always writes `ledstrip_rainbow_delta_deg` using `parseInt(...)` with no fallback/clamp. When the input is empty/disabled, `parseInt('')` yields `NaN`, and the MSP encoding bitwise-coerces it to `0`, unintentionally resetting the FC setting.
### Issue Context
- `save_rainbow_settings()` runs on every Save click.
- `MSPHelper.encodeSetting()` uses `data.push8/push16`, which uses bitwise ops that convert `NaN` to `0`.
### Fix Focus Areas
- tabs/led_strip.js[829-836]
- js/msp/MSPHelper.js[3459-3499]
- js/injected_methods.js[21-33]
### Suggested fix
- Compute `delta` with a safe numeric fallback and clamp, e.g.:
- `const deltaRaw = Number($('#rainbowDeltaInput').val());`
- `const delta = Number.isFinite(deltaRaw) ? clamp(deltaRaw, 0, 359) : /* keep existing */ 0;`
- Prefer skipping `setSetting('ledstrip_rainbow_delta_deg', ...)` when the input is disabled or not finite.
- Clamp `delta` to the FC-supported range (0–359) before sending.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+235
to
+238
| console.log('[Rainbow] ledstrip_rainbow_delta_deg =', result); | ||
| if (result && result.value !== null && result.value !== undefined) { | ||
| $('#rainbowDeltaInput').val(result.value); | ||
| } |
There was a problem hiding this comment.
2. Debug logs left in ui 🐞 Bug ⚙ Maintainability
The PR adds a console.log that prints rainbow setting results on every tab load, and a console.error for failures, which will create noisy logs in production use. This makes real issues harder to spot during troubleshooting.
Agent Prompt
### Issue description
A persistent `console.log` and `console.error` were added for rainbow setting reads. This produces unnecessary noise for every user session and can drown out actionable logs.
### Issue Context
The LED strip tab runs these calls on load in `process_html()`.
### Fix Focus Areas
- tabs/led_strip.js[228-241]
### Suggested fix
- Remove the `console.log` entirely.
- Consider handling errors consistently (either silent like the sweep-rate call, or via a UI-visible warning), but avoid unconditional console spam.
- If logging is desired, gate it behind an explicit debug flag.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+143
to
+145
| <div class="rainbowSettings" id="rainbowSettings" style="margin-top: 8px; margin-left: 4px;"> | ||
| <div style="font-weight: bold; margin-bottom: 6px; color: #1dacf2;">Rainbow Overlay Settings</div> | ||
| <div class="rainbowSettingRow" style="display: flex; align-items: center; margin-bottom: 4px;"> |
There was a problem hiding this comment.
3. Unlocalized title string 🐞 Bug ⚙ Maintainability
The HTML hardcodes the “Rainbow Overlay Settings” title text even though an i18n key (ledStripRainbowSettingsTitle) was added. This breaks localization and leaves dead/unused locale content.
Agent Prompt
### Issue description
The rainbow settings title is hardcoded in English, but a localization key was added and is unused.
### Issue Context
The project uses `i18n="..."` attributes for UI text.
### Fix Focus Areas
- tabs/led_strip.html[143-145]
- locale/en/messages.json[5949-5955]
### Suggested fix
Replace the hardcoded title with a localized element, e.g.:
- `<div ...><span i18n="ledStripRainbowSettingsTitle"></span></div>`
(or apply `i18n` directly to the div if supported by your localization loader).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| $('#rainbowFreqInput, #rainbowDeltaInput').prop('disabled', !rainbowEnabled).css('pointer-events', rainbowEnabled ? 'auto' : ''); | ||
|
|
||
|
|
||
| updateBulkCmd(); |
There was a problem hiding this comment.
4. Redundant updatebulkcmd call 🐞 Bug ➹ Performance
The selection handler now calls updateBulkCmd twice back-to-back, rebuilding FC.LED_STRIP twice for a single selection change. This adds unnecessary work on a UI-hot path and can degrade responsiveness.
Agent Prompt
### Issue description
`updateBulkCmd()` is invoked twice consecutively in the LED selection callback, causing redundant DOM scanning and reconstruction of `FC.LED_STRIP`.
### Issue Context
`updateBulkCmd()` clears and rebuilds `FC.LED_STRIP` by iterating over `.gPoint` elements, so repeating it is unnecessarily expensive.
### Fix Focus Areas
- tabs/led_strip.js[623-636]
### Suggested fix
Remove one of the two consecutive calls and keep a single `updateBulkCmd()` at the correct point after all UI state changes that affect the grid are complete.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds GUI support for the new rainbow overlay ('V') introduced in the companion FC PR.
Changes:
Companion FC PR: iNavFlight/inav#11816
Testing: Verified with SKYSTARSH743HD running the companion firmware build.