Skip to content

remove customrotations parameter conversion - #34124

Merged
peterbarker merged 6 commits into
ArduPilot:masterfrom
peterbarker:pr/remove-customrotations-parameter-conversion
Aug 25, 2026
Merged

remove customrotations parameter conversion#34124
peterbarker merged 6 commits into
ArduPilot:masterfrom
peterbarker:pr/remove-customrotations-parameter-conversion

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

Removes code which converted from 4.1-style custom rotations to modern style

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request
Board                    AP_Periph  antennatracker  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *                                 *                                                   
Durandal                            -336            -336   *           -344    -344              -336   -344   -344
Hitec-Airspeed           *                                 *                                                   
KakuteH7-bdshot                     -336            -344   *           -336    -336              -344   -336   -344
MatekF405                           -344            -344   *           -344    -336              -336   -344   -336
Pixhawk1-1M-bdshot                  -344            -344               -344    -344              -344   -344   -344
SITL_x86_64_linux_gnu               4024            -72                -72     4024              -72    4032   -72
YJUAV_A6SE                          -344            -344   *           -344    -352              -344   -352   -344
f103-QiotekPeriph        *                                 *                                                   
f303-MatekGPS            *                                 *                                                   
f303-Universal           *                                 *                                                   
iomcu                                                                                *                         
revo-mini                           -344            -336   *           -336    -344              -344   -336   -336
skyviper-v2450                                                         -344                                    
speedybeef4                         -336            -336   *           -344    -344              -344   -344   -344

Description

Removes some ancient conversion code.

Past this, someone moving from ArduPilot 4.1 to 4.8 who was using custom rotations will not get their new values transferred over to the new running firmware.

Also see ArduPilot/ardupilot_wiki#7981

@peterbarker peterbarker changed the title Pr/remove customrotations parameter conversion remove customrotations parameter conversion Aug 20, 2026
@peterbarker peterbarker moved this to ReadyForDevCall in Peter's ArduPilot 4.8 Queue Aug 24, 2026
@peterbarker
peterbarker force-pushed the pr/remove-customrotations-parameter-conversion branch from fd41308 to ddf2e21 Compare August 24, 2026 01:30
@tridge

tridge commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html#pr34124

Reviewed at head ddf2e21d0a. Verdict: REQUEST CHANGES — the deletion itself is clean (no remaining users of convert(), includes correctly dropped, one commit per subsystem, helper removed last), but it strands a maintained frame preset and the recovery path it leaves behind points at parameters that don't exist. All small fixes.

  • BUGTools/Frame_params/iflight-chimera7-4.2.param:36-39 still ships COMPASS_CUS_PIT,-15 / COMPASS_CUS_ROLL,0 / COMPASS_CUS_YAW,180 / COMPASS_ORIENT,100. That file was updated on 2025-12-10 and 2026-02-14, so it's maintained, not frozen. Today loading it converts on the next boot; with this PR it leaves COMPASS_ORIENT=100, which hits INTERNAL_ERROR(bad_rotation) on every sample (AP_Math/vector3.cpp:268-273) and refuses arming with "Internal errors 0x400000". Please update it in the same PR to COMPASS_ORIENT,102 + CUST_ROT2_ROLL/PIT/YAW and drop the COMPASS_CUS_* lines. (Found by the Codex validation pass, verified against the file and its history.)
  • ISSUElibraries/AP_Compass/AP_Compass.cpp:182 (and :347, :370) tell users to set "CUST_1_ROLL/PIT/YAW or CUST_2_ROLL/PIT/YAW"; the registered names are CUST_ROT1_*/CUST_ROT2_* (AP_Vehicle.cpp:96-98). Pre-existing, but manual recovery is now the only path, so worth fixing in the Compass commit.
  • ISSUE — this is a "4.2 or earlier" removal, not 4.1: Copter-4.2.0/4.2.3 still have AP_GROUPINFO("CUSTOM_ROLL", 15, …) and AP_GROUPINFO("CUS_ROLL", 49, …), and the conversion commit 480cc3fa46 is not an ancestor of any 4.2.x tag — it first shipped in Copter/Plane 4.3.0 (Oct 2022). The wiki firmware-update page the PR cites (ardupilot_wiki#7981) says 4.8 migrates from 4.2, which this contradicts for custom rotations — either that row becomes 4.3 or this waits a cycle. Policy call for the dev call (it has shipped in 4.3–4.7, ~4 years, well past the usual two releases); the description should at least say "4.2 or earlier".
  • ISSUE — the six doc-only @Param stubs (AP_AHRS.cpp:177-205, AP_Compass.cpp:668-699) still generate metadata: param_parse.py:63 terminates on a blank line, not on AP_GROUPINFO, so AHRS_CUSTOM_*/COMPASS_CUS_* stay in apm.pdef.xml with descriptions that can never be true. Drop the blocks, keep the // index N reservation comments.
  • ISSUE100:Custom 4.1 and older is still in @Values (AP_AHRS.cpp:145, AP_Compass.cpp:183/348/371; rotations.h:93 is the reference comment), so a GCS keeps offering a value that now triggers bad_rotation. ROTATION_CUSTOM_OLD must stay in the enum (AP_Math users and tests), but the @Values entry and the "Firmware versions 4.2 and prior…" sentence should go.
  • NOTE — a vehicle arriving with 100 stored fails safe (can't arm) but with no hint to set 101/102 + CUST_ROT*. Worth a release-note line.

Also checked: AP_CustomRotations::set() still used by AP_Math tests/example (not dead); find_old_parameter/ConversionInfo have ~15 other users; no autotest references the old names or value 100; flash −336…−352 B per the description. CI at this head: 46 pass / 52 pending / 0 fail.

@peterbarker

Copy link
Copy Markdown
Contributor Author
* **BUG** — `Tools/Frame_params/iflight-chimera7-4.2.param:36-39` still ships `COMPASS_CUS_PIT,-15` / 

COMPASS_CUS_ROLL,0 / COMPASS_CUS_YAW,180 / COMPASS_ORIENT,100. That file was updated on 2025-12-10 and 2026-02-14, so it's maintained, not frozen. Today loading it converts on the next boot; with this PR it leaves COMPASS_ORIENT=100, which hits INTERNAL_ERROR(bad_rotation) on every sample (AP_Math/vector3.cpp:268-273) and refuses arming with "Internal errors 0x400000". Please update it in the same PR to COMPASS_ORIENT,102 + CUST_ROT2_ROLL/PIT/YAW and drop the COMPASS_CUS_* lines. (Found by the Codex validation pass, verified against the file and its history.)

The version the parameter file is applicable to is right there in the damned filename.

* **ISSUE** — `libraries/AP_Compass/AP_Compass.cpp:182` (and `:347`, `:370`) tell users to set "CUST_1_ROLL/PIT/YAW or CUST_2_ROLL/PIT/YAW"; the registered names are `CUST_ROT1_*`/`CUST_ROT2_*` (`AP_Vehicle.cpp:96-98`). Pre-existing, but manual recovery is now the only path, so worth fixing in the Compass commit.

Pre-existing. Don't scope-creep.

* **ISSUE** — this is a "4.2 or earlier" removal, not 4.1: `Copter-4.2.0`/`4.2.3` still have `AP_GROUPINFO("CUSTOM_ROLL", 15, …)` and `AP_GROUPINFO("CUS_ROLL", 49, …)`, and the conversion commit `480cc3fa46` is not an ancestor of any 4.2.x tag — it first shipped in Copter/Plane 4.3.0 (Oct 2022). The wiki firmware-update page the PR cites (ardupilot_wiki#7981) says 4.8 migrates from 4.2, which this contradicts for custom rotations — either that row becomes 4.3 or this waits a cycle. Policy call for the dev call (it has shipped in 4.3–4.7, ~4 years, well past the usual two releases); the description should at least say "4.2 or earlier".

The Wiki issue PR clearly says those versions are up for debate.

* **ISSUE** — the six doc-only `@Param` stubs (`AP_AHRS.cpp:177-205`, `AP_Compass.cpp:668-699`) still generate metadata: `param_parse.py:63` terminates on a blank line, not on `AP_GROUPINFO`, so `AHRS_CUSTOM_*`/`COMPASS_CUS_*` stay in `apm.pdef.xml` with descriptions that can never be true. Drop the blocks, keep the `// index N` reservation comments.

Fixed.

* **ISSUE** — `100:Custom 4.1 and older` is still in `@Values` (`AP_AHRS.cpp:145`, `AP_Compass.cpp:183/348/371`; `rotations.h:93` is the reference comment), so a GCS keeps offering a value that now triggers `bad_rotation`. `ROTATION_CUSTOM_OLD` must stay in the enum (AP_Math users and tests), but the `@Values` entry and the "Firmware versions 4.2 and prior…" sentence should go.

Nothing makes the old comment untrue. If a user does have the value this is a strong hint they should change.

* **NOTE** — a vehicle arriving with 100 stored fails safe (can't arm) but with no hint to set 101/102 + `CUST_ROT*`. Worth a release-note line.

What? It's right there in the value comment description you just asked me to remove.

@tridge

tridge commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html#pr34124

Re-reviewed at head 8a7500e849; my earlier comment above is superseded. The stub removal is done exactly as asked, and your replies are taken — most of my previous findings are conceded and dropped below. One blocker remains, and it is now CI-enforced. Verdict: REQUEST CHANGES.

  • BUG (still open, now red in CI)build (param-file-validation) fails at this head: job 97320735758 ends with COMPASS_CUS_PIT not found in metadata (and ROLL/YAW), "Parameter checks failed!". On the filename argument — the gate doesn't read filenames: param_check_all.py validates every Frame_params file not in its skip list against metadata generated from the tree, and this PR's own stub removal is what orphaned the file. Two valid fixes, either in this PR: migrate the preset (CUST_ROT2_PITCH,-15 / CUST_ROT2_ROLL,0 / CUST_ROT2_YAW,180 / COMPASS_ORIENT,102, dropping the COMPASS_CUS_* lines — note the parameter is _PITCH, not _PIT as my previous comment said) — or, since the file is explicitly a 4.2-era preset and migrating it makes it wrong for 4.2, add it to frame_params_to_skip in param_check_all.py with a comment. One correction to my previous comment: param_check_all.py:179-183 disables value checks, so COMPASS_ORIENT,100 was never being value-checked against @Values.
  • RESOLVED — the six @Param stubs are gone, // index N was X reservation comments kept, one commit per subsystem; param_parse.py emits nothing for them.
  • Conceded and dropped (your replies hold): the CUST_1_*/CUST_2_* naming is pre-existing and out of scope (kept only as a pointer: the registered names are CUST_ROT1_*/CUST_ROT2_*); the 4.1-vs-4.2 wording belongs with wiki Copter: Added an optional pre-arm check for a GPS fix in all modes #7981 and the dev call; the migration hint already lives in the value-100 description; and keeping 100 in @Values is your call (it's also what keeps the preset value-check green).
  • NOTE — both new commit messages say CUSTOM_ROLL/CUSTOM_PIT_CUSTOM_YAW (underscore for the second slash) and the AP_Compass one says CUSTOM_* for params named CUS_* — worth fixing if rebasing anyway for the preset.

Also checked: the preset is the only file under Tools/ still referencing the removed names at this head; the earlier three commits are unchanged. CI otherwise: 69 pass / 28 pending; the sole failure is param-file-validation.

@tridge

tridge commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Follow-up report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_25_0855/devcall_pr_reviews.html

Re-reviewed at head 6c5accd795; my earlier comment above is superseded. The one blocker from the previous round is resolved. Verdict: COMMENT — down from REQUEST CHANGES.

You took a third option on the orphaned-preset blocker — deleting Tools/Frame_params/iflight-chimera7-4.2.param rather than migrating or skip-listing it. That fixes param-file-validation by construction: param_check_all.py enumerates presets by glob, so a deleted file isn't validated, and git grep COMPASS_CUS over Tools/Frame_params now returns nothing. I've left this at COMMENT rather than APPROVE for two reasons, neither blocking:

  • CI hasn't confirmed it yet — the push kicked a fresh run and param-file-validation is still pending. Expected to go green; worth a glance before merge.
  • One thing worth a word at the call: deleting vs migrating is a curation call. It's defensible (on current firmware the file's COMPASS_ORIENT=100+COMPASS_CUS_* is non-functional once the conversion is gone, and migrating a -4.2-named file to CUST_ROT2_* would make it wrong for 4.2). But it wasn't a relic — git log --follow shows it's the only iFlight Chimera7 preset in the tree and was kept current as recently as 2026-03-04 (ATC rename) and 2025-12-17 (NEU→NED). So dropping it outright rather than re-basing it to a current-firmware Chimera7 preset is a deliberate decision worth surfacing — your call, just flagging it.

Also checked: the delta is only the one file in its own Tools: commit; the earlier five are unchanged. An independent Codex pass reviewed the head and returned APPROVE with nothing new. (Cosmetic carry-over: the two doc commit subjects still have the CUSTOM_PIT_CUSTOM_YAW typo.)

@rmackay9 rmackay9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy with losing the old conversion code.

@andyp1per are you happy removing the old chimera parameter file? We probably need to either update it or remove it

was used to convert from 4.1-style rotations to the modern form
removes as the conversion code is also going, and we have versioned parameters now-adays
removes as the conversion code is also going, and we have versioned parameters now-adays
…arams

these are notionally 4.2 parameters - it's there in the name
@peterbarker
peterbarker force-pushed the pr/remove-customrotations-parameter-conversion branch from 6c5accd to 609122f Compare August 25, 2026 02:03
@peterbarker

Copy link
Copy Markdown
Contributor Author

I'm very happy with losing the old conversion code.

@andyp1per are you happy removing the old chimera parameter file? We probably need to either update it or remove it

I've revived the 4.2 parameter file but added it to the list of things that should not be checked by CI

@tridge

tridge commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Follow-up report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_25_1328/devcall_pr_reviews.html

Re-reviewed at head 609122f0e4; my earlier comment above is superseded. You took the second option I offered — restore the preset and skip-list it — which resolves both the blocker and the curation concern. Verdict: APPROVE — up from COMMENT.

  • RESOLVED (blocker) — the skip-list is correct: check_frame_params() globs the presets then if os.path.relpath(file, frame_params_dir) in frame_params_to_skip: continue (param_check_all.py:155), and the preset's relpath is exactly iflight-chimera7-4.2.param, so its COMPASS_ORIENT=100/COMPASS_CUS_* are never checked against metadata. (CI: the push kicked a fresh run and param-file-validation is still pending — expected green.)
  • RESOLVED (curation) — the preset is kept: present at the head and byte-identical to master.
  • NOTE — skip-listing silences CI but doesn't make the preset loadable on current firmware (once this PR removes the conversion, COMPASS_ORIENT=100+COMPASS_CUS_*bad_rotation, arming refused). That's a deliberate, defensible call — it's an explicitly-named 4.2 preset, and 4.2 firmware supported rotation 100 + COMPASS_CUS_* directly (the migration conversion came later, in 4.3, and this PR removes it), so the preset works on the firmware it names; and you already surfaced this with andyp1per in-thread. Worth a sentence at the call, nothing to change.

Cosmetic carry-over: the two doc commits still have the CUSTOM_PIT_CUSTOM_YAW subject typo. An independent Codex pass reviewed the head and returned APPROVE with nothing new.

@peterbarker
peterbarker merged commit 958dafe into ArduPilot:master Aug 25, 2026
99 of 100 checks passed
@github-project-automation github-project-automation Bot moved this from ReadyForDevCall to Done in Peter's ArduPilot 4.8 Queue Aug 25, 2026
@peterbarker
peterbarker deleted the pr/remove-customrotations-parameter-conversion branch August 26, 2026 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants