refactor: remove dead semver API version gates (floor 1.40.0)#608
refactor: remove dead semver API version gates (floor 1.40.0)#608nerdCopter wants to merge 12 commits into
Conversation
EmuFlight's minimum MSP API version is always 1.40.0, making all semver.gte(CONFIG.apiVersion, X) where X <= 1.40.0 always-true and all semver.lt(CONFIG.apiVersion, X) where X <= 1.40.0 always-false. Changes across 11 files (net -735 lines): - data_storage.js: bump apiVersionAccepted from '1.2.1' to '1.40.0' - adjustments.js: collapse 4-branch if/else chain to elementsNumber=29; unwrap gte(1.40.0) guard for element22/23 rename - configuration.js: remove ~20 dead version gates across load_* and save_* functions (1.6.0–1.37.0 range); delete lt(1.14.0/1.16.0) hide blocks; remove commented-out 1.4x.0 block - gps.js: delete lt(1.39.0) altitude /10 legacy correction - motors.js: unwrap two gte(1.36.0) guards; drop MSP_MISC fallback - onboard_logging.js: remove six always-true wrappers (1.25.0–1.36.0); delete VCP_BLOCK_SIZE and legacy rate else branches; preserve gte(1.40.0) boundary checks at lines 125/255 per CodeRabbit plan - osd.js: unwrap/delete ~15 dead version gates (1.21.0–1.39.0); preserve gte(1.40.0+), eq(1.52.0), lt(1.52.0), and all flightControllerVersion checks - pid_tuning.js: remove ~25 dead version gates across pid_and_rc_to_form, form_to_pid_and_rc, setDirty, checkUpdateProfile, updatePidControllerParameters, process_html, and rate-change handlers; collapse pid controller selector block to always-false path's else - power.js: set this.supported=true; simplify haveFc compound; unwrap three gte(1.33.0–1.36.0) guards in display and save paths - servos.js: remove dead lt(1.12.0) OR term from condition - transponder.js: set TABS.transponder.available=true directly Preserved: all semver checks >1.40.0, flightControllerVersion checks, semver.eq() exact-version checks, and pidControllerChangeMinApiVersion. lint: 0 errors (13 pre-existing complexity warnings unchanged). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemoves all legacy ChangesSemver Gating Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: 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 |
The var beeper_e = $('.tab-configuration .beepers') line was accidentally
deleted along with the semver.gte("1.36.0") if-wrapper. The variable is
still referenced at line 829 for the change event binding, causing a
ReferenceError at runtime. Restore the declaration unconditionally.
The semver.gte(CONFIG.apiVersion, "1.40.0") outer wrapper around the G_FORCE / CRSF / MAH_PERCENT / GPS_PLUS field concat block was not removed during the initial semver cleanup. Since floor is 1.40.0 this is always-true; remove the wrapper and dedent the body. Inner gates (1.49, 1.50, 1.51) are preserved unchanged.
The for (i = 0; i < PID_names.length; ...) loop in process_html used a
bare 'i' without var. In the original, 'var i' was declared inside the always-
false lt("1.31.0") block (pidControllerList loop); JavaScript hoisted it to
function scope, making the bare loop work. When that dead block was deleted,
the var declaration was lost. Fix: add var to the loop. This was also causing
process_html to crash early, leaving all subsequent show/hide logic unexecuted
and making the PID tab appear to show elements that should be hidden.
When no loaded_font_file matches a preset option (e.g. 'clarity' for
fontver=1/EmuFlight 0.1.0), the fallback previously called
.find('option').first() which returns the disabled <option value="-1">
placeholder as the first child. Selecting value "-1" picked a non-data
option, causing $font.data('font-file') to return undefined and the font
URL to become 'resources/osd/1/undefined.mcm'.
Fix: use 'option:not([disabled])' to skip the placeholder and land on
the first real preset option ("default" for fontver=1).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/js/tabs/configuration.js (1)
62-64: 💤 Low valuePassthrough functions are remnants of removed conditionals.
load_misc()andsave_misc()now only call the next function in their respective chains without performing any work. Consider inlining these to simplify the call chains.♻️ Optional: Inline passthrough functions
For the load chain, update
load_acc_trimto callload_arming_configdirectly:function load_acc_trim() { - MSP.send_message(MSPCodes.MSP_ACC_TRIM, false, false, load_misc); + MSP.send_message(MSPCodes.MSP_ACC_TRIM, false, false, load_arming_config); } -function load_misc() { - load_arming_config(); -}For the save chain, update
save_beeper_configto callsave_mixer_configdirectly:function save_beeper_config() { - MSP.send_message(MSPCodes.MSP_SET_BEEPER_CONFIG, mspHelper.crunch(MSPCodes.MSP_SET_BEEPER_CONFIG), false, save_misc); + MSP.send_message(MSPCodes.MSP_SET_BEEPER_CONFIG, mspHelper.crunch(MSPCodes.MSP_SET_BEEPER_CONFIG), false, save_mixer_config); } -function save_misc() { - save_mixer_config(); -}Also applies to: 924-926
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/tabs/configuration.js` around lines 62 - 64, The functions load_misc() and save_misc() are passthrough functions that only delegate to load_arming_config() and save_mixer_config() respectively without performing any additional work. Inline these passthrough functions by locating all calls to load_misc() and replacing them with direct calls to load_arming_config(), and all calls to save_misc() with direct calls to save_mixer_config(). Then remove the now-unnecessary load_misc() and save_misc() function definitions entirely to simplify the call chains.src/js/tabs/onboard_logging.js (1)
38-43: 💤 Low valueDead code:
gcdfunction is no longer used.The
gcdfunction was previously used for fraction-based rate calculations in the oldpopulateLoggingRatesimplementation. After the refactor to useblackboxPDenomdirectly, this function is unreferenced.Suggested removal
- function gcd(a, b) { - if (b === 0) - return a; - - return gcd(b, a % b); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/tabs/onboard_logging.js` around lines 38 - 43, The gcd function in onboard_logging.js is dead code that is no longer referenced anywhere after the refactor to use blackboxPDenom directly. Remove the entire gcd function definition (lines 38-43) including the function declaration, parameters, conditional check, and return statements, as it serves no purpose in the current codebase.src/js/tabs/power.js (1)
199-199: ⚡ Quick winUse strict equality for number comparison.
Static analysis flagged this line. Using
===instead of==prevents unintended type coercion and is the safer pattern for comparing numbers.Suggested fix
-var haveFc = (CONFIG.boardType == 0 || CONFIG.boardType == 2); +var haveFc = (CONFIG.boardType === 0 || CONFIG.boardType === 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/tabs/power.js` at line 199, The variable assignment for haveFc is using loose equality operators (==) when comparing CONFIG.boardType against numbers, which can lead to unintended type coercion. Replace both instances of == with === (strict equality) in the haveFc assignment to ensure the comparison is both type-safe and follows JavaScript best practices. This means changing both CONFIG.boardType == 0 and CONFIG.boardType == 2 to use === instead.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/tabs/osd.js`:
- Around line 1945-1958: The warning parsing code sets warningCount to
OSD.constants.WARNINGS.length at line 1946, which makes the loop condition at
line 1948 identical to the inner check at line 1951, causing the unknown
warnings else branch at lines 1954-1957 to be unreachable. Instead of using the
constant as the loop bound, read the actual warning count from the firmware data
stream (similar to how expectedStatsCount is read for statistics parsing at
lines 1900-1927). Replace the warningCount assignment with a read from the view
to obtain the count sent by the firmware, then keep the inner condition to
differentiate between known and unknown warnings.
In `@src/js/tabs/pid_tuning.js`:
- Around line 184-189: When apiVersion is 1.46.0 or higher, the
vbatpidcompensation control is hidden at line 188 without first syncing its
value from ADVANCED_TUNING, and then at line 1155 during save, the hidden
checkbox still overwrites the MSP value that was originally read from the FC. To
fix this, apply the same semver.lt(CONFIG.apiVersion, "1.46.0") gate condition
around the save assignment at line 1155 that saves the vbatPidCompensation
value, or alternatively, sync ADVANCED_TUNING.vbatPidCompensation from the input
checkbox before hiding it. This ensures the FC-read value is preserved and not
cleared when saving for apiVersion >= 1.46.0.
- Around line 241-245: Remove all dead code paths related to
dtermSetpointTransition in pid_tuning.js since EmuFlight only supports API
versions 1.40.0 and above, which use feedforwardTransition instead. Delete the
unconditional load of dtermSetpointTransitionNumberElement (lines 241-245
showing the input element setup and value assignment), remove the unconditional
save operation at line 1157 that writes dtermSetpointTransition back to
ADVANCED_TUNING, delete the warning handler block for dtermSetpointTransition
(lines 2203-2218), and remove any unconditional UI population of
dtermSetpointTransition at line 2853 that creates stale data never used by the
MSP encoder for supported firmware versions.
- Around line 2187-2201: In the rate profile change handler for the select
element with name "rate_profile", replace the string literals passed to the
`.prop()` method for the disabled property with actual boolean values. Change
the first `.prop('disabled', 'true')` to use the boolean true, and the second
`.prop('disabled', 'false')` to use the boolean false. This ensures jQuery 3.5
compatibility for setting boolean DOM properties.
---
Nitpick comments:
In `@src/js/tabs/configuration.js`:
- Around line 62-64: The functions load_misc() and save_misc() are passthrough
functions that only delegate to load_arming_config() and save_mixer_config()
respectively without performing any additional work. Inline these passthrough
functions by locating all calls to load_misc() and replacing them with direct
calls to load_arming_config(), and all calls to save_misc() with direct calls to
save_mixer_config(). Then remove the now-unnecessary load_misc() and save_misc()
function definitions entirely to simplify the call chains.
In `@src/js/tabs/onboard_logging.js`:
- Around line 38-43: The gcd function in onboard_logging.js is dead code that is
no longer referenced anywhere after the refactor to use blackboxPDenom directly.
Remove the entire gcd function definition (lines 38-43) including the function
declaration, parameters, conditional check, and return statements, as it serves
no purpose in the current codebase.
In `@src/js/tabs/power.js`:
- Line 199: The variable assignment for haveFc is using loose equality operators
(==) when comparing CONFIG.boardType against numbers, which can lead to
unintended type coercion. Replace both instances of == with === (strict
equality) in the haveFc assignment to ensure the comparison is both type-safe
and follows JavaScript best practices. This means changing both CONFIG.boardType
== 0 and CONFIG.boardType == 2 to use === instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a906fbd2-bc30-4644-88b9-17c18d885b3b
📒 Files selected for processing (11)
src/js/data_storage.jssrc/js/tabs/adjustments.jssrc/js/tabs/configuration.jssrc/js/tabs/gps.jssrc/js/tabs/motors.jssrc/js/tabs/onboard_logging.jssrc/js/tabs/osd.jssrc/js/tabs/pid_tuning.jssrc/js/tabs/power.jssrc/js/tabs/servos.jssrc/js/tabs/transponder.js
💤 Files with no reviewable changes (1)
- src/js/tabs/gps.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-09T14:03:37.978Z
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 544
File: src/js/tabs/pid_tuning.js:1712-1714
Timestamp: 2026-03-09T14:03:37.978Z
Learning: In EmuConfigurator for emuflight, API versions below 1.40.0 do not exist. The legacy semver check using semver.gte(CONFIG.apiVersion, "1.20.0") in src/js/tabs/pid_tuning.js is dead code and should be removed or refactored. Emuflight firmware always includes 3 PID profiles throughout its lifetime; any code paths assuming a different number of PID profiles should be updated accordingly. Validate and remove dead/broken API-version guards in pid_tuning.js, and ensure code paths reflect a minimum API version of 1.40.0 and the three PID profiles.
Applied to files:
src/js/tabs/pid_tuning.js
🪛 GitHub Check: Codacy Static Code Analysis
src/js/tabs/power.js
[warning] 199-199: src/js/tabs/power.js#L199
Use ===/!== to compare with true/false or Numbers
🔇 Additional comments (32)
src/js/data_storage.js (1)
6-6: LGTM!src/js/tabs/servos.js (1)
34-34: LGTM!src/js/tabs/transponder.js (1)
117-117: LGTM!src/js/tabs/motors.js (1)
60-60: LGTM!Also applies to: 190-190
src/js/tabs/adjustments.js (1)
275-291: LGTM!src/js/tabs/configuration.js (9)
16-24: LGTM!The MSP load chain simplification correctly removes version gates for API versions ≤ 1.40.0 while maintaining proper sequential callback flow.
Also applies to: 46-97, 118-120
142-145: LGTM!UI rendering de-versioning is correct: mixer preview reverse suffix, DShot beeper options, and sensor alignment population now execute unconditionally.
Also applies to: 182-207, 236-244
254-264: LGTM!ESC protocols correctly add base protocols unconditionally while preserving the 1.48.0+ gate for DSHOT2400/4800.
342-367: LGTM!Gyro/PID denominator generation correctly expands ranges and simplifies gyroUse32kHz handling while preserving the 1.54.0 check for
gyroSampleRateHz.Also applies to: 382-401
458-467: LGTM!GPS baudrate select is intentionally disabled and hidden as part of the de-versioning.
496-579: LGTM!Serial RX and SPI RX protocol lists correctly preserve version gates for 1.44.0+ (SFHSS), 1.50.0+ (IRC GHOST), and 1.51.0+ (REDPINE) while adding base protocols unconditionally.
599-614: LGTM!Arming/disarm UI visibility correctly switches from firmware version checks to
isExpertModeEnabled()dependency, and theaccHardwareSwitchhandler is appropriately simplified.Also applies to: 804-811
718-720: LGTM!3D deadband inputs and save handler assignments are now unconditional, correctly removing version gates for API ≤ 1.40.0.
Also applies to: 857-892
920-951: LGTM!Save chain correctly simplified: beeper/misc/GPS/compass saves are unconditional, and
save_rx_configalways routes tosave_to_eeprom.Also applies to: 1019-1021
src/js/tabs/power.js (3)
53-55: LGTM!
222-227: LGTM!
436-442: LGTM!src/js/tabs/onboard_logging.js (5)
72-76: LGTM!
107-107: LGTM!
145-158: LGTM!
160-192: LGTM!
347-353:VCP_BLOCK_SIZEis properly defined as 4096 in the tab initialization (line 12) and is correctly referenced at line 350 for VCP boards.src/js/tabs/osd.js (9)
766-766: LGTM!Also applies to: 789-789, 831-831
866-867: LGTM!Also applies to: 877-878, 1067-1068
1518-1608: LGTM!
1622-1666: LGTM!
1726-1808: LGTM!
1815-1833: LGTM!
2123-2124: LGTM!Also applies to: 2273-2310
2809-2814: LGTM!
1929-1943: Undefined variablecpassed tounpack.timer()(harmless).At line 1936,
cis referenced but undefined at that scope. However, the practical impact is zero: theunpack.timerfunction (line 1768) declares a second parametercbut never uses it—it only extracts bits from thebitsparameter. Passingundefinedcauses no runtime error or behavior change.src/js/tabs/pid_tuning.js (1)
47-58: LGTM!Also applies to: 110-110, 191-192, 231-240, 246-303, 321-321, 347-348, 362-364, 420-424, 1165-1214, 1457-1484, 1547-1547, 2493-2494, 2562-2564, 2579-2583, 2794-2794, 2810-2814
| // Parse enabled warnings | ||
| var warningCount = OSD.constants.WARNINGS.length; | ||
| var warningFlags = view.readU16(); | ||
| for (var i = 0; i < warningCount; i++) { | ||
|
|
||
| // Known warning field | ||
| if (i < OSD.constants.WARNINGS.length) { | ||
| d.warnings.push($.extend(OSD.constants.WARNINGS[i], { enabled: (warningFlags & (1 << i)) != 0 })); | ||
| // Known warning field | ||
| if (i < OSD.constants.WARNINGS.length) { | ||
| d.warnings.push($.extend(OSD.constants.WARNINGS[i], { enabled: (warningFlags & (1 << i)) != 0 })); | ||
|
|
||
| // Push Unknown Warning field | ||
| } else { | ||
| var warningNumber = i - OSD.constants.WARNINGS.length + 1; | ||
| d.warnings.push({name: 'UNKNOWN', text: ['osdWarningTextUnknown', warningNumber], desc: 'osdWarningUnknown', enabled: (warningFlags & (1 << i)) != 0 }); | ||
| } | ||
| // Push Unknown Warning field | ||
| } else { | ||
| var warningNumber = i - OSD.constants.WARNINGS.length + 1; | ||
| d.warnings.push({name: 'UNKNOWN', text: ['osdWarningTextUnknown', warningNumber], desc: 'osdWarningUnknown', enabled: (warningFlags & (1 << i)) != 0 }); | ||
| } |
There was a problem hiding this comment.
Unreachable code: unknown warnings handling can never execute.
The loop bound at line 1948 is i < warningCount where warningCount = OSD.constants.WARNINGS.length (line 1946). The inner condition at line 1951 checks i < OSD.constants.WARNINGS.length, which is the same constraint. Since the loop already guarantees i < warningCount, the else branch (lines 1954-1957) for unknown warnings is unreachable.
This differs from the statistics handling (lines 1900-1927) which correctly reads expectedStatsCount from the firmware and can handle unknown fields. For consistency and to support future firmware versions with additional warnings, the warnings parsing should similarly read an expected count from the stream rather than using its own constant as the loop bound.
💡 Suggested approach
If the MSP protocol sends a warning count, use it similar to statistics:
// Parse enabled warnings
-var warningCount = OSD.constants.WARNINGS.length;
+var expectedWarningCount = view.readU8(); // If protocol supports this
var warningFlags = view.readU16();
-for (var i = 0; i < warningCount; i++) {
+for (var i = 0; i < expectedWarningCount; i++) {Otherwise, if warning count is implicit in the 16-bit flags, consider iterating through all 16 bits or documenting that unknown warnings are intentionally ignored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/tabs/osd.js` around lines 1945 - 1958, The warning parsing code sets
warningCount to OSD.constants.WARNINGS.length at line 1946, which makes the loop
condition at line 1948 identical to the inner check at line 1951, causing the
unknown warnings else branch at lines 1954-1957 to be unreachable. Instead of
using the constant as the loop bound, read the actual warning count from the
firmware data stream (similar to how expectedStatsCount is read for statistics
parsing at lines 1900-1927). Replace the warningCount assignment with a read
from the view to obtain the count sent by the firmware, then keep the inner
condition to differentiate between known and unknown warnings.
| if (semver.lt(CONFIG.apiVersion, "1.46.0")) { | ||
| $('input[id="vbatpidcompensation"]').prop('checked', ADVANCED_TUNING.vbatPidCompensation !== 0); | ||
| }else{ | ||
| } else { | ||
| console.log('vbat'); | ||
| $('.vbatpidcompensation').hide(); | ||
| } |
There was a problem hiding this comment.
Preserve hidden vbatPidCompensation when saving.
For apiVersion >= 1.46.0, Line 188 hides the control without syncing it from ADVANCED_TUNING, but Line 1155 still overwrites the MSP value from that hidden checkbox. Gate the save assignment the same way, or sync before hiding, so Save does not clear the FC-read value.
Proposed fix
- ADVANCED_TUNING.vbatPidCompensation = $('input[id="vbatpidcompensation"]').is(':checked') ? 1 : 0;
+ if (semver.lt(CONFIG.apiVersion, "1.46.0")) {
+ ADVANCED_TUNING.vbatPidCompensation = $('input[id="vbatpidcompensation"]').is(':checked') ? 1 : 0;
+ }Also applies to: 1155-1155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/tabs/pid_tuning.js` around lines 184 - 189, When apiVersion is 1.46.0
or higher, the vbatpidcompensation control is hidden at line 188 without first
syncing its value from ADVANCED_TUNING, and then at line 1155 during save, the
hidden checkbox still overwrites the MSP value that was originally read from the
FC. To fix this, apply the same semver.lt(CONFIG.apiVersion, "1.46.0") gate
condition around the save assignment at line 1155 that saves the
vbatPidCompensation value, or alternatively, sync
ADVANCED_TUNING.vbatPidCompensation from the input checkbox before hiding it.
This ensures the FC-read value is preserved and not cleared when saving for
apiVersion >= 1.46.0.
| var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]'); | ||
| dtermSetpointTransitionNumberElement.attr('min', 0.00); | ||
|
|
||
| if (semver.gte(CONFIG.apiVersion, "1.20.0")) { | ||
| $('.pid_filter input[name="gyroNotch1Frequency"]').val(FILTER_CONFIG.gyro_notch_hz); | ||
| $('.pid_filter input[name="gyroNotch1Cutoff"]').val(FILTER_CONFIG.gyro_notch_cutoff); | ||
| if (semver.lt(CONFIG.apiVersion, "1.44.0")) { | ||
| $('.pid_filter input[name="dTermNotchFrequency"]').val(FILTER_CONFIG.dterm_notch_hz); | ||
| $('.pid_filter input[name="dTermNotchCutoff"]').val(FILTER_CONFIG.dterm_notch_cutoff); | ||
| } else { | ||
| $('.dtermNotch').hide(); | ||
| } | ||
| var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]'); | ||
| if (semver.gte(CONFIG.apiVersion, "1.38.0")) { | ||
| dtermSetpointTransitionNumberElement.attr('min', 0.00); | ||
| } else { | ||
| dtermSetpointTransitionNumberElement.attr('min', 0.01); | ||
| } | ||
|
|
||
| dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100); | ||
| dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect all dtermSetpointTransition/feedforwardTransition MSP encode/decode paths.
# Expectation: Supported apiVersion >= 1.40.0 paths should use feedforwardTransition, and pid_tuning.js should not save dtermSetpointTransition unconditionally.
rg -n -C4 'dtermSetpointTransition|feedforwardTransition' src/js/msp src/js/tabs/pid_tuning.jsRepository: emuflight/EmuConfigurator
Length of output: 9222
Remove the dtermSetpointTransition dead code path.
MSPHelper.js encode/decode (lines 1164–1168, 2168–2172) uses feedforwardTransition for API ≥ 1.40.0 and dtermSetpointTransition only for older APIs. EmuFlight does not support API versions below 1.40.0, making lines 241–245, 1157, and 2203–2218 in pid_tuning.js dead code. Currently, the hidden UI (line 2853) still populates and saves dtermSetpointTransition unconditionally, creating stale data in ADVANCED_TUNING that is never read by MSP for any supported firmware version.
Cleanup locations
Lines 241–245: Remove unconditional load
- var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]');
- dtermSetpointTransitionNumberElement.attr('min', 0.00);
-
- dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100);Line 1157: Remove unconditional save
- ADVANCED_TUNING.dtermSetpointTransition = parseInt($('input[name="dtermSetpointTransition-number"]').val() * 100);Lines 2203–2218: Remove warning handler
- var dtermTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]');
- var dtermTransitionWarningElement = $('`#pid-tuning` .dtermSetpointTransitionWarning');
-
- function checkUpdateDtermTransitionWarning(value) {
- if (value > 0 && value < 0.1) {
- dtermTransitionWarningElement.show();
- } else {
- dtermTransitionWarningElement.hide();
- }
- }
- checkUpdateDtermTransitionWarning(dtermTransitionNumberElement.val());
-
- //Use 'input' event for coupled controls to allow synchronized update
- dtermTransitionNumberElement.on('input', function() {
- checkUpdateDtermTransitionWarning($(this).val());
- });📝 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.
| var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]'); | |
| dtermSetpointTransitionNumberElement.attr('min', 0.00); | |
| if (semver.gte(CONFIG.apiVersion, "1.20.0")) { | |
| $('.pid_filter input[name="gyroNotch1Frequency"]').val(FILTER_CONFIG.gyro_notch_hz); | |
| $('.pid_filter input[name="gyroNotch1Cutoff"]').val(FILTER_CONFIG.gyro_notch_cutoff); | |
| if (semver.lt(CONFIG.apiVersion, "1.44.0")) { | |
| $('.pid_filter input[name="dTermNotchFrequency"]').val(FILTER_CONFIG.dterm_notch_hz); | |
| $('.pid_filter input[name="dTermNotchCutoff"]').val(FILTER_CONFIG.dterm_notch_cutoff); | |
| } else { | |
| $('.dtermNotch').hide(); | |
| } | |
| var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]'); | |
| if (semver.gte(CONFIG.apiVersion, "1.38.0")) { | |
| dtermSetpointTransitionNumberElement.attr('min', 0.00); | |
| } else { | |
| dtermSetpointTransitionNumberElement.attr('min', 0.01); | |
| } | |
| dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100); | |
| dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/tabs/pid_tuning.js` around lines 241 - 245, Remove all dead code paths
related to dtermSetpointTransition in pid_tuning.js since EmuFlight only
supports API versions 1.40.0 and above, which use feedforwardTransition instead.
Delete the unconditional load of dtermSetpointTransitionNumberElement (lines
241-245 showing the input element setup and value assignment), remove the
unconditional save operation at line 1157 that writes dtermSetpointTransition
back to ADVANCED_TUNING, delete the warning handler block for
dtermSetpointTransition (lines 2203-2218), and remove any unconditional UI
population of dtermSetpointTransition at line 2853 that creates stale data never
used by the MSP encoder for supported firmware versions.
Source: Learnings
There was a problem hiding this comment.
@coderabbitai, then associated, `src/js/msp/MSPHelper.js:1163 and beyond should be:
if (semver.gte(CONFIG.apiVersion, "1.40.0")) {
ADVANCED_TUNING.feedforwardTransition = data.readU8();
ADVANCED_TUNING.errorBoostLimit = data.readU8();
ADVANCED_TUNING.iDecay = data.readU8();
correct?
we have all of these to cleanup:
$ grep -Iirn dtermSetpointTransition src/
src/js/msp/MSPHelper.js:1167: ADVANCED_TUNING.dtermSetpointTransition = data.readU8();
src/js/msp/MSPHelper.js:2171: buffer.push8(ADVANCED_TUNING.dtermSetpointTransition);
src/js/fc.js:439: dtermSetpointTransition: 0,
src/js/tabs/pid_tuning.js:241: var dtermSetpointTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]');
src/js/tabs/pid_tuning.js:242: dtermSetpointTransitionNumberElement.attr('min', 0.00);
src/js/tabs/pid_tuning.js:244: dtermSetpointTransitionNumberElement.val(ADVANCED_TUNING.dtermSetpointTransition / 100);
src/js/tabs/pid_tuning.js:1157: ADVANCED_TUNING.dtermSetpointTransition = parseInt($('input[name="dtermSetpointTransition-number"]').val() * 100);
src/js/tabs/pid_tuning.js:2203: var dtermTransitionNumberElement = $('input[name="dtermSetpointTransition-number"]');
src/js/tabs/pid_tuning.js:2204: var dtermTransitionWarningElement = $('#pid-tuning .dtermSetpointTransitionWarning');
src/js/tabs/pid_tuning.js:2853: $('#pid-tuning .dtermSetpointTransition').hide();
src/tabs/pid_tuning.html:219: <tr class="dtermSetpointTransition">
src/tabs/pid_tuning.html:220: <td><input type="number" name="dtermSetpointTransition-number" step="0.01" min="0.01" max="1.00"></td>
src/tabs/pid_tuning.html:224: <span i18n="pidTuningDtermSetpointTransition"></span>
src/tabs/pid_tuning.html:226: <div class="helpicon cf_tip" i18n_title="pidTuningDtermSetpointTransitionHelp"></div>
src/tabs/pid_tuning.html:230: <tr class="dtermSetpointTransitionWarning">
src/tabs/pid_tuning.html:232: <span i18n="pidTuningDtermSetpointTransitionWarning"></span>
does this branch fail to account for al the HTML changes (code and/or .show/./hide) required too?
There was a problem hiding this comment.
@nerdCopter Yes — good catch. My earlier comment was too narrow.
Given the existing repo invariant you’d already established (EmuFlight minimum MSP API is 1.40.0), the cleanup should be end-to-end, not just the pid_tuning.js load/save lines.
That means this branch should also cover:
-
src/js/msp/MSPHelper.js- decode
feedforwardTransitionunconditionally for supported firmware - encode
feedforwardTransitionunconditionally - remove the legacy
dtermSetpointTransitionbranch entirely
- decode
-
src/js/fc.js- remove the unused
dtermSetpointTransitiondefault field if nothing supported reads it anymore
- remove the unused
-
src/js/tabs/pid_tuning.js- remove the unconditional populate/save of
dtermSetpointTransition - remove the warning handler
- remove the
.hide()for.dtermSetpointTransition, since that should no longer exist in the template
- remove the unconditional populate/save of
-
src/tabs/pid_tuning.html- remove the dead
tr.dtermSetpointTransition - remove the dead
tr.dtermSetpointTransitionWarning
- remove the dead
So to your MSPHelper.js snippet: yes, conceptually that’s the right direction — for supported APIs this should be feedforwardTransition, not the legacy dtermSetpointTransition path.
In other words, this branch currently looks incomplete because it removes part of the dead path in src/js/tabs/pid_tuning.js, but leaves behind:
- dead MSP serialization/deserialization,
- dead model state,
- and dead HTML/UI scaffolding.
I used your earlier repo-specific note about the 1.40.0 minimum API here, and it does change the review conclusion: this should be treated as a full dead-code removal, not just a partial JS simplification.
(•ㅅ•)
✏️ Learnings added
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 608
File: src/js/tabs/pid_tuning.js:241-245
Timestamp: 2026-06-26T16:35:02.674Z
Learning: In emuflight/EmuConfigurator, `dtermSetpointTransition` is legacy dead code for unsupported API versions below 1.40.0. For supported firmware, `src/js/msp/MSPHelper.js` should use `ADVANCED_TUNING.feedforwardTransition` instead, and cleanup should remove remaining `dtermSetpointTransition` references across `src/js/msp/MSPHelper.js`, `src/js/fc.js`, `src/js/tabs/pid_tuning.js`, and `src/tabs/pid_tuning.html` rather than merely hiding the old UI.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 544
File: src/js/tabs/pid_tuning.js:1712-1714
Timestamp: 2026-03-09T14:03:37.978Z
Learning: In EmuConfigurator for emuflight, API versions below 1.40.0 do not exist. The legacy semver check using semver.gte(CONFIG.apiVersion, "1.20.0") in src/js/tabs/pid_tuning.js is dead code and should be removed or refactored. Emuflight firmware always includes 3 PID profiles throughout its lifetime; any code paths assuming a different number of PID profiles should be updated accordingly. Validate and remove dead/broken API-version guards in pid_tuning.js, and ensure code paths reflect a minimum API version of 1.40.0 and the three PID profiles.
There was a problem hiding this comment.
@coderabbitai, my AI agent did not squash everything as instructed, i still see ./src/js/msp/MSPHelper.js
if (semver.gte(CONFIG.apiVersion, "1.24.0")) {
ADVANCED_TUNING.levelAngleLimit = data.readU8();
ADVANCED_TUNING.levelSensitivity = data.readU8();
if (semver.gte(CONFIG.apiVersion, "1.36.0")) {
ADVANCED_TUNING.itermThrottleThreshold = data.readU16();
ADVANCED_TUNING.itermAcceleratorGain = data.readU16();
if (semver.gte(CONFIG.apiVersion, "1.39.0")) {
and many other 1.2x, 1.3x, 1.1x, 1.6., 1.7., 1.8.
There was a problem hiding this comment.
@nerdCopter You're right — I was still too narrow there.
Using your earlier repo invariant that EmuFlight only supports MSP API >= 1.40.0, those src/js/msp/MSPHelper.js branches for 1.24.0, 1.36.0, 1.39.0, and the other 1.1x/1.2x/1.3x checks are also dead and should be audited in this PR. So yes: if the goal of #608 is to remove sub-1.40.0 semver gating, this branch is still incomplete while those conditionals remain in MSPHelper.js.
The practical bar here should be:
- remove legacy sub-
1.40.0API branches insrc/js/msp/MSPHelper.js - keep only gates for versions actually supported/relevant above that floor
- make the model/UI/MSP codec consistent so no legacy fields survive just because the tab code was cleaned first
So I agree with your read: this wasn’t fully squashed “as instructed,” and src/js/msp/MSPHelper.js still needs a pass for the remaining dead semver gates.
(╥﹏╥)🐇
✏️ Learnings added
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 608
File: src/js/tabs/pid_tuning.js:241-245
Timestamp: 2026-06-26T16:42:09.237Z
Learning: In emuflight/EmuConfigurator, semver cleanup for the minimum supported MSP API version `1.40.0` must include `src/js/msp/MSPHelper.js`, not just UI/tab files. During reviews of dead semver removal PRs, treat `semver.gte/lt` branches for API versions below `1.40.0` in `src/js/msp/MSPHelper.js` (for example `1.1x`, `1.2x`, and `1.3x` gates) as dead code that should be audited and removed unless a newer supported-version boundary still matters.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 544
File: src/js/tabs/pid_tuning.js:1712-1714
Timestamp: 2026-03-09T14:03:37.978Z
Learning: In EmuConfigurator for emuflight, API versions below 1.40.0 do not exist. The legacy semver check using semver.gte(CONFIG.apiVersion, "1.20.0") in src/js/tabs/pid_tuning.js is dead code and should be removed or refactored. Emuflight firmware always includes 3 PID profiles throughout its lifetime; any code paths assuming a different number of PID profiles should be updated accordingly. Validate and remove dead/broken API-version guards in pid_tuning.js, and ensure code paths reflect a minimum API version of 1.40.0 and the three PID profiles.
Learnt from: nerdCopter
Repo: emuflight/EmuConfigurator PR: 608
File: src/js/tabs/pid_tuning.js:241-245
Timestamp: 2026-06-26T16:35:02.674Z
Learning: In emuflight/EmuConfigurator, treat `dtermSetpointTransition` as legacy dead code for unsupported firmware API versions below 1.40.0. For supported firmware (API >= 1.40.0), ensure `src/js/msp/MSPHelper.js` uses `ADVANCED_TUNING.feedforwardTransition` instead of `dtermSetpointTransition`. During cleanup, remove all remaining `dtermSetpointTransition` references across the pid tuning implementation/UI (`src/js/msp/MSPHelper.js`, `src/js/fc.js`, `src/js/tabs/pid_tuning.js`, `src/tabs/pid_tuning.html`) rather than hiding the old UI/behavior.
| $('.tab-pid_tuning select[name="rate_profile"]').change(function() { | ||
| self.currentRateProfile = parseInt($(this).val()); | ||
| self.updating = true; | ||
| $(this).prop('disabled', 'true'); | ||
| MSP.promise(MSPCodes.MSP_SELECT_SETTING, [self.currentRateProfile + self.RATE_PROFILE_MASK]).then(function() { | ||
| self.refresh(function() { | ||
| self.updating = false; | ||
|
|
||
| $('.tab-pid_tuning select[name="rate_profile"]').prop('disabled', 'false'); | ||
| CONFIG.rateProfile = self.currentRateProfile; | ||
| $('.tab-pid_tuning select[name="rate_profile"]').prop('disabled', 'false'); | ||
| CONFIG.rateProfile = self.currentRateProfile; | ||
|
|
||
| GUI.log(i18n.getMessage('pidTuningLoadedRateProfile', [self.currentRateProfile + 1])); | ||
| }); | ||
| GUI.log(i18n.getMessage('pidTuningLoadedRateProfile', [self.currentRateProfile + 1])); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jQuery 3.5 .prop disabled false boolean property behavior
💡 Result:
In jQuery 3.5, the.prop('disabled', false) method is the standard and recommended way to enable a form element [1][2][3]. The behavior of.prop('disabled', false) correctly updates the underlying DOM property to false, which effectively enables the element [4]. Because disabled is a boolean property in the DOM, jQuery handles this state explicitly using boolean values [1][5][4]. Key technical points regarding this behavior: 1. Recommended Method: The.prop method should always be used for boolean properties like disabled, checked, or selected [1][5]. It directly modifies the element's property state rather than its HTML attribute [6][4]. 2. Avoiding Incorrect Practices: You should not use.removeProp('disabled') to enable an element [7][2]. As documented by the jQuery team,.removeProp can remove the property completely from the element, leading to permanent, unexpected behavior where the property can no longer be toggled [7][2]. 3. Attribute vs. Property: While the disabled attribute in HTML is a boolean attribute (meaning its presence alone signifies "disabled"), the.prop('disabled', false) call ensures the DOM element's programmatic state is set to false [5][4]. This is distinct from using.attr, which manipulates the HTML attribute directly and is not intended for managing dynamic state [5][4]. In summary,.prop('disabled', false) is the correct, reliable, and intended approach for enabling elements in jQuery 3.5 [2][4][3].
Citations:
- 1: https://api.jquery.com/prop/
- 2: https://stackoverflow.com/questions/1414365/disable-enable-an-input-with-jquery
- 3: https://learn.jquery.com/using-jquery-core/faq/how-do-i-disable-enable-a-form-element/
- 4: https://www.xjavascript.com/blog/disable-enable-an-input-with-jquery/
- 5: https://api.jquery.com/attr/
- 6: https://learnjavascript.co.uk/jq/reference/ap/prop.html
- 7: jQuery.removeProp("disabled") does not enable input after disabling with jQuery.prop("disabled", true) jquery/jquery#2913
🏁 Script executed:
#!/bin/bash
# Description: Find string-valued disabled property assignments in this file.
# Expectation: changed rate-profile code should use true/false booleans, not 'true'/'false' strings.
rg -n -C2 "\\.prop\\(\\s*['\"]disabled['\"]\\s*,\\s*['\"](?:true|false)['\"]\\s*\\)" src/js/tabs/pid_tuning.jsRepository: emuflight/EmuConfigurator
Length of output: 1156
Use booleans for the disabled property.
Lines 2190 and 2195 pass strings 'true' and 'false' to the .prop() method for a boolean DOM property. jQuery 3.5 requires boolean values, not strings, for this operation. Replace the string literals with actual booleans to ensure the selector is correctly enabled after the async refresh completes.
Proposed fix
- $(this).prop('disabled', 'true');
+ $(this).prop('disabled', true);
...
- $('.tab-pid_tuning select[name="rate_profile"]').prop('disabled', 'false');
+ $('.tab-pid_tuning select[name="rate_profile"]').prop('disabled', false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/tabs/pid_tuning.js` around lines 2187 - 2201, In the rate profile
change handler for the select element with name "rate_profile", replace the
string literals passed to the `.prop()` method for the disabled property with
actual boolean values. Change the first `.prop('disabled', 'true')` to use the
boolean true, and the second `.prop('disabled', 'false')` to use the boolean
false. This ensures jQuery 3.5 compatibility for setting boolean DOM properties.
EmuFlight's minimum MSP API version is always 1.40.0. Remove all dead code paths across 18 files: - semver.gte(CONFIG.apiVersion, X) where X <= 1.40.0: always-true, remove wrapper and any dead else branch - semver.lt(CONFIG.apiVersion, X) where X <= 1.40.0: always-false, remove entire block and keep any else branch Files changed: - MSPHelper.js: flatten process_data() and crunch() — removes ~50 nested if-wrappers; simplifies dataflashRead, sendServoConfigurations (removes dead lt(1.12.0)/lt(1.33.0) branches + send_channel_forwarding), sendLedStripConfig, setArmingEnabled compound condition - configuration.js: remove dead load_misc/save_misc passthroughs - osd.js, pid_tuning.js, pid_tuning.html: remove dead gates; remove dtermSetpointTransition field and HTML rows - onboard_logging.js: remove dead gates including 1.40.0 boundaries - fc.js: simplify boardHasVcp() to direct flag read; remove dtermSetpointTransition default field - Features.js: remove 9 dead gte-wrappers for 1.16.0/1.36.0 - RateCurve.js: keep 1.20.0+ path (expoPower=3, rcRateConstant=200) - backup_restore.js: remove 14 dead gte-wrappers for 1.8.0–1.33.0 - serial_backend.js: remove 7 dead gates for 1.20.0–1.37.0 - failsafe.js: remove 7 dead gates; power/VTX-rescue always active - led_strip.js: remove 15 dead gates for 1.19.0/1.20.0 - receiver.js: remove 10 dead gates for 1.15.0/1.20.0 - vtx.js: remove 2 dead 1.40.0 boundary gates - ports.js: remove 9 dead gates for 1.6.0/1.31.0/1.40.0 - main.js: power/VTX tabs always shown (remove 1.36.0/1.40.0 wrappers) - setup.js: remove 4 dead gates for 1.36.0–1.40.0 Net: -864 lines. No behavior change for firmware >= 1.40.0. Closes emuflight#586. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dead variable/function cleanup after semver gate removal: - data_storage.js: remove backupRestoreMinApiVersionAccepted and pidControllerChangeMinApiVersion constants (both '1.5.0', always triggered with floor 1.40.0) - setup.js: remove backup/restore button-disable block guarded by lt(backupRestoreMinApiVersionAccepted) — always false - pid_tuning.js: unwrap gte(pidControllerChangeMinApiVersion) — always true; MSP_PID_CONTROLLER now always requested unconditionally. Remove dead var pidController_e (assigned but never read). - MSPHelper.js process_data: remove dead case MSP_SET_CHANNEL_FORWARDING (MSPCodes.MSP_SET_CHANNEL_FORWARDING is undefined; no-op case) - MSPHelper.js crunch: remove dead case MSP_SET_CHANNEL_FORWARDING (same — undefined code, no callers after send_channel_forwarding removed) - MSPHelper.js: remove duplicate ledFunctionLetters declarations in MSP_LED_STRIP_CONFIG receive handler and send_next_led_strip_config (variable declared but never referenced in either block) - MSPHelper.js: fix pre-existing bug — gyro_32khz_hardware_lpf was read from the FC into a discarded local variable; crunch() was then sending FILTER_CONFIG.gyro_32khz_hardware_lpf (always 0) back, silently clobbering the FC value on every save. Now stored to FILTER_CONFIG. Crunch side simplified to read directly from FILTER_CONFIG field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the outer semver.gte('1.20.0') wrapper was removed (always-true
with floor 1.40.0), the inner if/else save block was incorrectly
collapsed to if-only, dropping the else branch that writes
ADVANCED_TUNING.dtermSetpointWeight from the dtermSetpoint-number
input. The crunch in MSPHelper.js correctly sends dtermSetpointWeight
via its else branch for API < 1.49.0, but the value was always the
stale loaded value since the UI change was never captured.
Affected firmware: EmuFlight 0.1.0 (API 1.40.0) and 0.3.1 (API 1.47.0).
Found by self-review in Phase 5 of issue emuflight#586 cleanup.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Addressing findings from the initial review and follow-up discussion. All four original findings have been resolved in subsequent commits on this branch: [osd.js — unreachable warnings else branch] — Superseded [pid_tuning.js:187 — vbatPidCompensation cleared on save for API >= 1.46.0] — Superseded [pid_tuning.js — dtermSetpointTransition dead code end-to-end] — Superseded
[pid_tuning.js:2196 — string-valued disabled prop] — Superseded [MSPHelper.js — remaining dead gates for 1.24.0, 1.36.0, 1.39.0] (from follow-up discussion) Self-review (Phase 5) found one additional regression:
Please trigger a fresh review of the current state. |
|
Tip For best results, initiate chat on the files or code changes.
(full_review) |
1 participant
AI Generated pull-request
Closes #586
Summary
EmuFlight's minimum MSP API version is always 1.40.0. Every
`semver.gte(CONFIG.apiVersion, X)` where X ≤ 1.40.0 is always-true, and every
`semver.lt(CONFIG.apiVersion, X)` where X ≤ 1.40.0 is always-false. This PR
removes all such dead code across 11 source files.
Changes per file:
Preserved: all checks against versions > 1.40.0, all `CONFIG.flightControllerVersion`
checks, `semver.eq` exact-version checks, and onboard_logging.js lines 125/255
(CodeRabbit "boundary cases").
11 files changed, 551 insertions(+), 1287 deletions(-)
Runtime bugs found and fixed during hardware testing
Four regressions introduced by the dead-code removal were caught and fixed in this branch:
Test plan