Limit 4.8 migration to be only from 4.3 and younger - #34194
Conversation
|
Let's do it! |
|
This is great but means we still need #33878 for 4.7 - do you agree? |
d8c1d43 to
77eccc7
Compare
Oh yes. I hope nothing I do here would affect the 4.7 branch and fixes to it. |
|
Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head
Non-blocking notes:
CI at this head: 39 pending, 60 passing. |
Acknowledged. Explicit decision. If we don't do this then we can never git rid of the conversion code, we have to assume these parameters are unused. |
| @@ -373,7 +373,6 @@ class AP_Mount | |||
| void handle_gimbal_device_attitude_status(const mavlink_message_t &msg); | |||
|
|
|||
| // perform any required parameter conversion | |||
There was a problem hiding this comment.
we can remove this comment it seems
| @@ -805,7 +805,6 @@ bool NavEKF3::InitialiseFilter(void) | |||
|
|
|||
| #if !APM_BUILD_TYPE(APM_BUILD_AP_DAL_Standalone) | |||
There was a problem hiding this comment.
I guess this whole section can be removed now
| @@ -367,7 +367,6 @@ class NavEKF3 { | |||
| void writeDefaultAirSpeed(float airspeed, float uncertainty); | |||
|
|
|||
| // parameter conversion | |||
There was a problem hiding this comment.
this comment can be removed
| @@ -62,7 +62,6 @@ void AP_Mount::init() | |||
| } | |||
|
|
|||
| // perform any required parameter conversion | |||
rmackay9
left a comment
There was a problem hiding this comment.
I'm happy with removing these old parameter conversions
|
Can be merged once Randy's stuff is looked at |
Sub has no 4.2, 4.3, 4.4 or 4.6 release, so the oldest version a user can be moving from once the migration floor is 4.3 is Sub-4.5. Both of these conversions are present in ArduSub-4.5.0, so anybody running that or later has already had them applied: - the airspeed object out of g2 (Jan-2022) - FENCE_ parameters into the AC_Fence object (Mar-2022) Also drops the call to SRV_Channels::upgrade_parameters(), which is being removed.
Rover has no 4.3 release, so the oldest version a user can be moving from once the migration floor is 4.3 is Rover-4.4. All of these conversions are present in Rover-4.4.0, so anybody running that or later has already had them applied: - the airspeed object out of g2 (Jan-2022) - the AIS object out of g2 (Mar-2022) - FENCE_ parameters into the AC_Fence object (Mar-2022) Also drops the call to SRV_Channels::upgrade_parameters(), which is being removed.
This converted SERVOn_FUNCTION from int8 to int16, and is present in the 4.3.0 release (in fact all the way back to 4.0.0). Anybody running 4.3.0 or later has already had it applied.
This converted BRD_SERIAL_NUM from int16 to int32 and is present in the 4.3.0 release, so anybody running 4.3.0 or later has already had it applied.
This was meant to convert VTX_OPTIONS from int8 to int16, and is present in the 4.3.0 release, so anybody running 4.3.0 or later has already had it applied. It should be noted that it never actually did anything. 0658f06 widened _options from AP_Int8 to AP_Int16 and added the conversion in the same commit, but passed the new width rather than the old one: _options.convert_parameter_width(AP_PARAM_INT16); The argument to convert_parameter_width() is the type the value was stored as *before* the widening, so this should have been AP_PARAM_INT8. AP_Param::scan() matches on the type in the parameter header, so a value written as an int8 is never found by a scan for an int16 and the conversion always returned false without touching anything. A user upgrading across the widening lost their VTX_OPTIONS regardless. So this removes dead code, and the migration floor is not really what makes it safe to do so.
This moved ATC_RAT_RLL/PIT/YAW_FILT to ATC_RAT_*_FLTE and dates from 2019 - it is present in ArduSub-4.1.2 and ArduSub-4.5.0. Sub has no 4.2, 4.3, 4.4 or 4.6 release, so the oldest version a user can be moving from once the migration floor is 4.3 is ArduSub-4.5.0, which has already had this applied. Sub::convert_old_parameters() is left empty by this, so it goes too.
Added in 2019 for the Rover-3.4 to 3.5 upgrade, and present in Rover-4.4.0 - the oldest release a Rover user can be moving from once the migration floor is 4.3, as Rover has no 4.3.
- the conversion_table, which converted the old altitude fence parameters (Mar-2021) - the loop converting the old chan params to RCx_OPTION, along with rc_option_conversion and the RCConversionInfo struct (Mar-2021) Both are present in the 4.3.0 release, so anybody running 4.3.0 or later has already had them applied.
Converted the old battery failsafe parameters (Mar-2018) and COMPASS_ENABLE (Apr-2019). Sub has no 4.2, 4.3, 4.4 or 4.6 release, so the oldest version a user can be moving from once the migration floor is 4.3 is ArduSub-4.5.0, which has already had these applied.
Trims conversion_table down to just the TRQ1_ block, which was added May-2024 and so is still required. Everything above it - battery (Oct-2013), serial baud (Jan-2015), MOT_THR (Aug-2017), COMPASS_ENABLE (Apr-2019), WP_RADIUS (May-2019), sailboat (May-2019), ATC_TURN_MAX_G (May-2021), WP_PIVOT (Dec-2021) and PRX1_ (Aug-2022) - is present in Rover-4.4.0, the oldest release a Rover user can be moving from once the migration floor is 4.3, as Rover has no 4.3. Also removes the WP_SPEED/CRUISE_SPEED conversion (May-2019) and the attitude control FF/FILT conversion (Jul-2019) on the same grounds.
AP_RollController::convert_pid() and AP_PitchController::convert_pid() converted the old RLL2SRV_/PTCH2SRV_ gains into the AC_PID form. Added Apr-2021 and described in the code as "a temporary conversion function during development"; present in the 4.3.0 release, so anybody running 4.3.0 or later has already had it applied.
Converted the old COMPASS_PRIMARY into the device-id priority list.
Added Feb-2020 and present in the 4.3.0 release, so anybody running
4.3.0 or later has already had it applied.
Note that the conversion sat inside Compass::init(), below its
if (!_enabled) {
return;
}
early return, so "present in the release" is not quite the same as "has
run". A user who has had COMPASS_ENABLE=0 for every release from 4.3
onwards never reached the conversion, and if they enable the compass
after moving to 4.8 their old COMPASS_PRIMARY selection is not honoured.
That is judged too unlikely - and the parameter too old - to keep the
conversion for.
Widened LOG_FILE_BUFSIZE from 8 to 16 bits. Added Nov-2020 and present in the 4.3.0 release, so anybody running 4.3.0 or later has already had it applied.
Converted MNT_ to MNT1_, mount angle limits from centi-degrees to degrees, and the mount RC input channels to RCx_OPTION. Added Sep-2022 and first shipped in the 4.3.0 release, so anybody running 4.3.0 or later has already had it applied.
Converted EK3_GPS_TYPE, EK3_ALT_SOURCE and EK3_MAG_CAL into the
EK3_SRC1_ parameters. Added Nov-2020 and present in the 4.3.0 release,
so anybody running 4.3.0 or later has already had it applied.
Note that the only call to convert_parameters() was from
InitialiseFilter(), below its
if (_enable == 0 || _imuMask == 0) {
return false;
}
early return, so "present in the release" is not quite the same as "has
run". A user who has had EK3_ENABLE=0 for every release from 4.3
onwards never reached the conversion, and if they enable EKF3 after
moving to 4.8 they get EK3_SRC1_ defaults rather than their old source
selections. That is judged too unlikely - and the parameters too old -
to keep the conversion for.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM (153). Added Sep-2021 and present in Tracker-4.5.0, which is the earliest tracker release there is, so every tracker release a user can be moving from already applied it. A stored 41 which survives becomes an unhandled auxiliary function and does nothing, which is the same outcome as any other option number this firmware does not know.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM_AIRMODE (154). Added Sep-2021 and present in Copter-4.2.0, so anybody moving from 4.3.0 or later has already had it applied. A stored 41 which survives becomes an unhandled auxiliary function and does nothing, which is the same outcome as any other option number this firmware does not know.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM (153), or to ARMDISARM_AIRMODE (154) on a quadplane which had the airmode option set and no AIRMODE switch. Added Sep-2021 and present in Plane-4.2.0, so anybody moving from 4.3.0 or later has already had it applied. A stored 41 which survives becomes an unhandled auxiliary function and does nothing, which is the same outcome as any other option number this firmware does not know.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM (153). Added Sep-2021, but Sub has no 4.2 to 4.4 releases, so the first release carrying it is ArduSub-4.5.0. That is also the oldest release a Sub user can be moving from once the migration floor is 4.3, so everybody in scope has already had it applied. A stored 41 which survives becomes an unhandled auxiliary function and does nothing, which is the same outcome as any other option number this firmware does not know.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM (153). Added Sep-2021. Blimp has no release tags, so unlike the other vehicles there is no release which can be shown to contain the conversion, and a 4.3 migration floor says nothing about a build made from master. This removal takes the floor to mean that a Blimp older than the whole 4.3 cycle is out of scope for migration, the same as it is for every other vehicle. A stored 41 which survives becomes an unhandled auxiliary function and does nothing, which is the same outcome as any other option number this firmware does not know.
Rewrote a stored RCn_OPTION of ARMDISARM_UNUSED (41) to ARMDISARM (153), and Rover's SAVE_TRIM (5) to TRIM_TO_CURRENT_SERVO_RC (155). Both were added Sep-2021 and are present in Rover-4.2.0, so anybody moving from Rover-4.4.0 - the oldest release a Rover user can be moving from once the migration floor is 4.3, as Rover has no 4.3 - has already had them applied. A stored 41 or 5 which survives becomes an unhandled auxiliary function on Rover and does nothing, which is the same outcome as any other option number this firmware does not know.
The last caller went away with the per-vehicle RCn_OPTION conversions. AUX_FUNC::ARMDISARM_UNUSED and AUX_FUNC::SAVE_TRIM stay in the enum: those numbers are still spoken for and must not be handed out again.
GPS_AUTO_SWITCH used to have a value 3, "use second GPS". That was replaced by GPS_PRIMARY, and this converted a stored 3 into GPS_PRIMARY=1 with GPS_AUTO_SWITCH=0. Added Nov-2020 and present in the 4.1.0 release, so anybody moving from 4.3.0 or later has already had it applied. The conversion only ran while GPS_PRIMARY was unconfigured, but GPS_PRIMARY arrived in the very same commit as the conversion, so on the first 4.1-or-later boot of any vehicle carrying a 3 it was unconfigured and the conversion ran. A stored 3 which survives is no longer any of NONE, BLEND or USE_PRIMARY_IF_3D_FIX, so update_primary() falls through to the UseBest path - the same as any other value this firmware does not know, and the same as it already does today for anybody who sets 3 by hand after having configured GPS_PRIMARY. 3 is not offered in the parameter's @values and GPSAutoSwitch::USE_SECOND stays commented out in the enum, so the number is not handed out again.
77eccc7 to
6365262
Compare
Summary
Moves the oldest firmware version we support migrating parameters from up to
4.3, and deletes the parameter conversion code that predates it.
Classification & Testing (check all that apply and add your own)
Builds clean for SITL across Copter, Plane, Rover, Sub, Blimp and Tracker.
Every removal was justified by reading release tags rather than dates: for each
conversion, that vehicle's release tags were walked in version order to find the
first release whose file actually contains the conversion.
git tag --containswas deliberately not used - the 4.3 release branch carries some conversions as
separate cherry-picks, so ancestry reports a later release than the truth. The
legacy tag prefixes were taken into account (
ArduPlane-4.0.xbecomesPlane-4.1.2,APMrover2-becomesRover-,ArduSub-becomesSub-), as wasthe fact that Rover has no 4.3 and Sub has no 4.2 to 4.4 at all.
No autotest depends on any of the removed code: the surviving references to
LOG_FILE_BUFSIZE,VTX_OPTIONSandGPS_AUTO_SWITCHinTools/autotest/alluse current parameter names and in-range values, and
MNT_TYPEappears only incomments.
Description
Built on top of #34188
What and why
ArduPilot carries parameter conversion code so that a user upgrading from an old
release keeps their settings. That code is never free - it costs flash on every
board, it runs on every boot, and it is a long tail of rarely-exercised paths
that still has to be read and maintained. It is only worth carrying back as far
as the oldest release we actually promise to migrate from.
The wiki has not stated what that floor is (see ArduPilot/ardupilot_wiki#7981,
where the proposed support table had invented version numbers). This PR takes
the floor to be 4.3 and removes everything below it: 26 commits, 837 lines
deleted across 30 files, retiring conversions covering roughly 119 parameter
names.
A conversion is only removed here if it is present in the oldest release a user
of that vehicle can be moving from once the floor is 4.3 - which means such a
user has already had it applied and saved, and so arrives at 4.8 with the new
parameters already written.
Relationship to the annotation PR
This is the second of two PRs. The first is comment-only: it normalises every
PARAMETER_CONVERSION - Added: <Mon>-<YYYY> for <Release>annotation and addsthe ones that were missing, so that the vintage of each conversion is greppable.
Those 28 commits are included at the base of this branch and should be reviewed
and merged first; this PR is the removal step that the annotations make
checkable.
Three things worth a reviewer's attention
These are the only places where a user can notice something beyond "my old
parameter is no longer migrated".
ArduPlane: remove magic repair of Q_M_PWM_MIN/Q_M_PWM_MAXis adeliberate behaviour change, in a commit of its own. The old code ran the
conversion a second time with
CONVERT_FLAG_FORCEon every boot wheneverthe current values were invalid, so a vehicle with bad
Q_M_PWM_MIN/MAXwassilently overwritten from the legacy quadplane slots over and over. That is
gone: such a vehicle now fails the existing arming check, "Check Q_M_PWM_MIN
and Q_M_PWM_MAX", and the user fixes it themselves.
AP_Compass: remove primary compass parameter conversionhas a caveatspelled out in its commit message. The conversion sat below Compass::init()'s
if (!_enabled) return;, so "present in the release" is not quite "has run".A user who has had
COMPASS_ENABLE=0for every release since 4.3 and thenenables the compass on 4.8 will not have their old
COMPASS_PRIMARYhonoured.Judged too unlikely, and the parameter too old, to keep the conversion for.
The
RCn_OPTIONandGPS_AUTO_SWITCHremovals leave stale stored valuesbehind rather than rejecting them. A surviving
RCn_OPTIONof 41(
ARMDISARM_UNUSED), or 5 on Rover, becomes an unhandled auxiliary functionand does nothing; a surviving
GPS_AUTO_SWITCHof 3 is no longer any of thehandled cases and falls through to the "UseBest" path. In both cases that is
the same outcome as any other option number the firmware does not know, and
the retired enum values stay reserved in their enums so the numbers are not
handed out again.
Parameter and migration considerations
old name is still translated into its current name at boot.
ran on the release they are coming from and the new values are in storage.
parameters by hand. That is the intended meaning of the migration floor, and
is the thing the wiki change documents.
through a supported release first.
Not included
Two things that look like candidates but are deliberately left alone, because
neither is purely a migration - both still run for new vehicles, so they do not
retire when the floor moves:
AP_MotorsMulticopter::convert_pwm_min_max_param(), which stampsMOT_PWM_MIN/MOT_PWM_MAXfrom the throttle RC calibration on any vehiclethat has never had them configured.
Q_TAILSIT_ENABLEandQ_TILT_ENABLEheuristics inTailsitter::setup()and
Tiltrotor::setup().AI disclosure
This contribution was AI-assisted. Claude (Claude Code) did the audit of the
conversion code, the release-by-content verification of every conversion's
vintage, and wrote the commits and this description; a second model (OpenAI
Codex) was used as an independent reviewer over both branches and its findings
were checked and acted on. The work was directed, reviewed and is submitted by
a human, who bears full responsibility for it.