Fix: Correctly determine number of PID profiles#544
Fix: Correctly determine number of PID profiles#544nerdCopter wants to merge 1 commit intoemuflight:masterfrom
Conversation
The logic for determining the number of PID profiles was flawed, preventing users from copying to profile 3. This commit corrects the logic to trust the `numProfiles` value from the firmware when available (API version >= 1.20.0), and falls back to a default of 3 for older versions. This ensures the correct number of profiles is displayed and the copy functionality works as expected.
📝 WalkthroughWalkthroughModified the conditional logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/tabs/pid_tuning.js`:
- Around line 1712-1714: The assignment to numberOfProfiles uses
CONFIG.numProfiles directly when semver.gte(CONFIG.apiVersion, "1.20.0"), which
can be undefined/0/NaN and will cause the profile selector loop
(numberOfProfiles) to produce zero options; change the assignment in that branch
to validate and fall back to a safe default (e.g., DEFAULT_NUM_PROFILES or a
constant) by coalescing only valid positive integers—e.g., set numberOfProfiles
= (Number.isFinite(Number(CONFIG.numProfiles)) && Number(CONFIG.numProfiles) >
0) ? Number(CONFIG.numProfiles) : DEFAULT_NUM_PROFILES—so semver.gte(...),
CONFIG.numProfiles, numberOfProfiles and the profile-generation loop are
protected.
| if (semver.gte(CONFIG.apiVersion, "1.20.0")) { | ||
| numberOfProfiles = CONFIG.numProfiles; | ||
| } |
There was a problem hiding this comment.
Guard against falsy CONFIG.numProfiles.
If CONFIG.numProfiles is undefined, 0, or NaN (e.g., firmware returns an unexpected value), numberOfProfiles becomes falsy and the loop on line 1717 produces zero options, breaking the profile selector entirely. Consider falling back to the default.
🛡️ Proposed fix
if (semver.gte(CONFIG.apiVersion, "1.20.0")) {
- numberOfProfiles = CONFIG.numProfiles;
+ numberOfProfiles = CONFIG.numProfiles || 3;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (semver.gte(CONFIG.apiVersion, "1.20.0")) { | |
| numberOfProfiles = CONFIG.numProfiles; | |
| } | |
| if (semver.gte(CONFIG.apiVersion, "1.20.0")) { | |
| numberOfProfiles = CONFIG.numProfiles || 3; | |
| } |
🤖 Prompt for AI Agents
In `@src/js/tabs/pid_tuning.js` around lines 1712 - 1714, The assignment to
numberOfProfiles uses CONFIG.numProfiles directly when
semver.gte(CONFIG.apiVersion, "1.20.0"), which can be undefined/0/NaN and will
cause the profile selector loop (numberOfProfiles) to produce zero options;
change the assignment in that branch to validate and fall back to a safe default
(e.g., DEFAULT_NUM_PROFILES or a constant) by coalescing only valid positive
integers—e.g., set numberOfProfiles =
(Number.isFinite(Number(CONFIG.numProfiles)) && Number(CONFIG.numProfiles) > 0)
? Number(CONFIG.numProfiles) : DEFAULT_NUM_PROFILES—so semver.gte(...),
CONFIG.numProfiles, numberOfProfiles and the profile-generation loop are
protected.
The logic for determining the number of PID profiles was flawed, preventing users from copying to profile 3.
This commit corrects the logic to trust the
numProfilesvalue from the firmware when available (API version >= 1.20.0), and falls back to a default of 3 for older versions. This ensures the correct number of profiles is displayed and the copy functionality works as expected.Summary by CodeRabbit