Repository navigation
Add a v2 migrated version of the X500 template - #2133
amilcarlucas wants to merge 2 commits into
Conversation
amilcarlucas
added
the
AIReview
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (4)
VEHICLE_COMPONENTS_FORMAT_VERSIONis still set to 1, but this PR adds a v1→v2 migration. With the… · NewVEHICLE_COMPONENTS_FORMAT_VERSIONis still set to 1, but this PR adds a v1→v2 migration. With the… · Newexc.messageis not a standard Python exception attribute and can raiseAttributeError,… · New
What changed in this PR
Adds a format-v2 “migrated” Holybro X500 ArduCopter template and introduces v1→v2 filesystem migration logic to support splitting parameters into new step files and restoring missing step files from templates.
Changes:
- Add Holybro_X500_mig vehicle template (vehicle_components + numbered intermediate
.paramsteps + one.pdef.xml). - Implement backend migration v1→v2 (parameter extraction/splitting, obsolete deletions, and missing-step restoration from the matching empty firmware template).
- Add tests that exercise v1→v2 parameter extraction and missing-step file restoration.
| File | Description |
|---|---|
| tests/test_backend_filesystem_migration.py | Adds tests for v1→v2 parameter splitting and for restoring missing configuration-step files from an empty template. |
| ardupilot_methodic_configurator/backend_filesystem_migration.py | Implements v1→v2 migration logic and adds restoration of missing step files based on firmware template. |
| .github/skills/configuration-steps-reorder/scripts/param_reorder.py | Updates renaming map for reordered configuration-step numbering. |
| TUNING_GUIDE_ArduCopter_mig.md | Adds a migration-focused ArduCopter methodic tuning guide aligned with the new step files. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/vehicle_components.json | Adds format-v2 components definition for the migrated X500 template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/00_default.param | Adds baseline default parameter set for the migrated X500 template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/02_imu_temperature_calibration_setup.param | Adds IMU temperature calibration setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/03_imu_temperature_calibration_results.param | Adds IMU temperature calibration results step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/04_imu_temperature_calibration_finish.param | Adds IMU temperature calibration finish step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/05_board_orientation.param | Adds board orientation step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/06_remote_controller_receiver.param | Adds RC receiver setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/07_remote_controller_controller.param | Adds RC controller/channel mapping step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/08_telemetry.param | Adds telemetry setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/09_esc_telemetry.param | Adds ESC telemetry protocol step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/10_battery_monitor.param | Adds battery monitor hardware/calibration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/11_battery.param | Adds battery/failsafe configuration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/12_gnss.param | Adds GNSS configuration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/13_initial_atc.param | Adds initial attitude controller tuning step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/14_accelerometer_calibration.param | Adds dedicated accelerometer calibration step (new v2 split) for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/14_mp_setup_mandatory_hardware.param | Adds legacy “mandatory hardware” step file content used for migration/training. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/15_accelerometer_level.param | Adds dedicated accel-level step (new v2 split) for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/16_compass_calibration.param | Adds dedicated compass calibration step (new v2 split) for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/17_flight_modes.param | Adds dedicated flight modes step (new v2 split) for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/18_servo_outputs.param | Adds dedicated servo output functions step (new v2 split) for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/19_general_configuration.param | Adds general configuration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/20_safety_setup.param | Adds safety/failsafe/geofence setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/21_remote_id.param | Adds Remote ID step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/22_osd.param | Adds OSD setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/23_motor.param | Adds motor poles configuration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/24_esc.param | Adds ESC/pwm/servo range setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/25_motor_notch_filter_setup.param | Adds harmonic notch initial setup step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/26_motor_notch_logging.param | Adds logging configuration for notch tuning step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/27_optional_pid_adjustment.param | Adds optional PID adjustment step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/28_throttle_controller.param | Adds throttle controller tuning step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/29_motor_notch_filter_results.param | Adds notch filter results step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/30_ekf_config.param | Adds EKF configuration step for the migrated template. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/31_inflight_magnetometer_fit_setup.pdef.xml | Adds parameter definition XML for MagFit setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/31_pid_notch_filter_logging.param | Adds PID notch filter logging configuration step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/32_pid_notch_filter_results.param | Adds PID notch filter results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/33_quick_tune_setup.param | Adds QuickTune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/34_quick_tune_results.param | Adds QuickTune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/35_inflight_magnetometer_fit_setup.param | Adds inflight MagFit setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/36_inflight_magnetometer_fit_results.param | Adds inflight MagFit results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/37_evaluate_the_aircraft_tune_ff_disable.param | Adds evaluate-tune (FF disabled) step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/38_evaluate_the_aircraft_tune_ff_enable.param | Adds evaluate-tune (FF enabled) step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/39_autotune_roll_setup.param | Adds roll autotune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/40_autotune_roll_results.param | Adds roll autotune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/41_autotune_pitch_setup.param | Adds pitch autotune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/42_autotune_pitch_results.param | Adds pitch autotune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/43_autotune_yaw_setup.param | Adds yaw autotune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/44_autotune_yaw_results.param | Adds yaw autotune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/45_autotune_yawd_setup.param | Adds yaw-D autotune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/46_autotune_yawd_results.param | Adds yaw-D autotune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/47_autotune_roll_pitch_retune_setup.param | Adds roll/pitch retune setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/48_autotune_roll_pitch_retune_results.param | Adds roll/pitch retune results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/49_autotune_finish.param | Adds post-autotune finishing step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/50_pid_d_ff.param | Adds D feed-forward step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/51_windspeed_estimation.param | Adds windspeed estimation step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/52_barometer_compensation.param | Adds barometer compensation step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/53_windspeed_estimation_finish.param | Adds windspeed estimation finish step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/54_system_id_input_roll.param | Adds sysid input roll step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/55_system_id_input_pitch.param | Adds sysid input pitch step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/56_system_id_input_yaw.param | Adds sysid input yaw step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/57_system_id_mixer_roll.param | Adds sysid mixer roll step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/58_system_id_mixer_pitch.param | Adds sysid mixer pitch step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/59_system_id_mixer_yaw.param | Adds sysid mixer yaw step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/60_system_id_mixer_thrust.param | Adds sysid mixer thrust step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/61_analytical_pid_optimization.param | Adds analytical PID optimization step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/64_position_controller.param | Adds position controller tuning step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/65_guided_operation.param | Adds guided operation tuning step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/66_precision_land.param | Adds precision landing configuration step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/67_optical_flow_setup.param | Adds optical flow setup step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/68_optical_flow_results.param | Adds optical flow results step. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/69_use_optical_flow_instead_of_gnss.param | Adds step to switch EKF sources to optical flow. |
| ardupilot_methodic_configurator/vehicle_templates/ArduCopter/Holybro_X500_mig/70_everyday_use.param | Adds everyday-operation configuration step. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| VehicleProjectCreator, | ||
| ) | ||
|
|
||
| VEHICLE_COMPONENTS_FORMAT_VERSION = 1 |
| if format_version >= VEHICLE_COMPONENTS_FORMAT_VERSION: | ||
| return False |
| file_renames["52_use_optical_flow_instead_of_gnss.param"] = "65_use_optical_flow_instead_of_gnss.param" | ||
| file_renames["53_everyday_use.param"] = "66_everyday_use.param" | ||
|
|
||
| # Explicit renames for the current files after inserting steps 14-17. |
| try: | ||
| template_dir = Path(VehicleProjectCreator.template_dir_for_bin_import(vehicle_type, major, minor)) | ||
| except VehicleProjectCreationError as exc: | ||
| logging.warning(_("Migration template directory not found, skipping missing files: %s"), exc.message) |
Previous review (2026-09-30)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks for the migrated X500 template. A few problems need fixing before this can go in; each was reproduced offline at f8f52ed. The v1->v2 migration (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R747) runs for format-0 projects while the package configuration steps and empty templates are still v1. The following restore (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R749) copies the empty template's 14_mp_setup_mandatory_hardware.param back in after the split, which silently replaces calibration and frame values with defaults. For example, on a format-0 Holybro_X500 copy, INS_ACCSCAL_X goes from 0.998941 to 1 and FRAME_CLASS from 1 to 0. Please ship the v2 step definitions and templates together with the migration, or gate it. The restore after v0->v1 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R746) runs before rename_parameter_files(). An old project's 04_board_orientation.param is then blocked from renaming by a freshly copied template 05_board_orientation.param, and AHRS_ORIENTATION ends up as 0. Restore after renaming, or treat old_filenames as present. VEHICLE_COMPONENTS_FORMAT_VERSION is still 1 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R33), so v1 projects never migrate, and the new restore test fails. Only bump it once the first point is fixed. _PARAM_DELETES_V1_TO_V2 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R378) drops FENCE_ACTION, FENCE_RADIUS, FRAME_TYPE and the learned MOT_THST_HOVER instead of moving them to their owning steps. Holybro_X500_mig still contains 14_mp_setup_mandatory_hardware.param (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-db615890f93903a738b18f6c753d278b6c15bddd6c3c77a0cefbf046ddf18d30R37). Its stale INS_ACCSCAL_X/INS_ACC1_CALTEMP override the new accelerometer file, and the template-prefix tests fail. The template-local configuration steps omit the SERIALx_PROTOCOL derived_parameters for the receiver (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R195) and telemetry (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R236). Smaller items: rename 31_inflight_magnetometer_fit_setup.pdef.xml to the 35_ prefix so its metadata loads (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-1e209a71555d86ae3ccd908ae5d573d703c5547d5ca34ae04e83e2dfeba25143R1); set FRAME_CLASS in 18_servo_outputs.param to 1 to match the Quad frame (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-0a5ab9e05ec490df194f8d8f56ac08b1818bebc3ea750e506ebb684bd69992aeR1); and align the param_reorder.py rename table with the committed numbering (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-1f5a26403437ee1c6df1af88f28e52cc5716cc4c2f9ef504c6b375701b113ad1R41). Copilot's exc.message concern does not apply, since VehicleProjectCreationError defines .message. |
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Test Results 3 files 3 suites 36m 48s ⏱️ Results for commit 5fbfa38. ♻️ This comment has been updated with latest results. |
f8f52ed to
5a04563
Compare
|
The v1 version configuration is not changed yet, because this is a self contained template, used for testing. So there is no need to increase VEHICLE_COMPONENTS_FORMAT_VERSION yet. It will be incremented later once this template is fully tested. |
Previous review (2026-09-30)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks for the fixes. The SERIALx_PROTOCOL derivations, the 35_ MagFit sidecar, FRAME_CLASS in 18_servo_outputs.param and the reorder table all check out now at 5a04563. On keeping VEHICLE_COMPONENTS_FORMAT_VERSION at 1: agreed, no need to bump it yet. The catch is that the v1->v2 branch at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R747 still runs for any format-0 project, because 0 < 2. The package steps and empty templates are still v1, so the restore at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R749 then puts the empty template's 14_mp_setup_mandatory_hardware.param back, and that copy wins. On a format-0 Holybro_X500 copy this changes FRAME_CLASS from 1 to 0, INS_ACCSCAL_X from 0.998941 to 1 and MOT_THST_HOVER from 0.301157 to 0.35; master keeps them. Could the v1->v2 call be gated on the target version until the v2 steps ship? The restore at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R746 still runs before rename_parameter_files(). An old 04_board_orientation.param is then blocked by a fresh template 05_board_orientation.param and left orphaned. Restoring after the rename, or skipping steps whose old_filenames exist, would fix it. With the constant at 1, the new test at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-46abaa490dbf486888b50033a988929f605ce8f695930b2d3c5d3a0dc874a1f3R857 fails because it writes format 1. Using format 0 or monkeypatching the constant would fix it. Holybro_X500_mig still ships 14_mp_setup_mandatory_hardware.param (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-db615890f93903a738b18f6c753d278b6c15bddd6c3c77a0cefbf046ddf18d30R37), and its older INS_ACCSCAL_X and INS_ACC1_CALTEMP override 14_accelerometer_calibration.param. It also trips the duplicate and ordering template tests. The range and 58/59 reserved-prefix tests probably just need their tables updated for the new numbering. Worth fixing before the migration is switched on: _PARAM_DELETES_V1_TO_V2 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R378) drops FENCE_ACTION, FENCE_RADIUS, FRAME_TYPE and MOT_THST_HOVER instead of moving them. The existing-name filter at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R660 lets an older value in the destination override the later calibration from step 14. And the source is trimmed at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R651 before any destination is written, so a failed write loses the value. I haven't checked the GUI, uploads to a real flight controller, the full test suite or CI (still pending). |
fb20079 to
54ac5f6
Compare
Previous review (2026-09-30)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at pinned head fb20079. The head has moved since the previous AI review at 5a04563, and only the migration module and its test changed. Thanks, the version gate works. With the target at 1, a format-0 Holybro_X500 copy migrates exactly as on master, and the old_filenames check stops the restore from orphaning an old 04_board_orientation.param. Keeping the version at 1 until the template is tested is fine. Two things still need fixing before merge. Holybro_X500_mig still ships 14_mp_setup_mandatory_hardware.param (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-db615890f93903a738b18f6c753d278b6c15bddd6c3c77a0cefbf046ddf18d30R37). It is not one of the template's steps, but it is still loaded after 14_accelerometer_calibration.param, so its older INS_ACCSCAL_X 0.998941 and INS_ACC1_CALTEMP 43.062431 override the new calibration. It also fails the duplicate- and ordering-prefix tests. Separately, the template tests reject 67_..70_ (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-ce0d98b91169e438e72e4299ab5347d00ca764b2df4b6b730fb957ba0ccc1a58R1) and 58/59 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-cc785ca12db778535114db7dface2219a0a753665272579240057cf04d6b455eR1), so their tables need to recognise the v2 numbering. Locally, tests/test_vehicle_template_param_files.py has 4 failures at this head. Worth doing before the target goes to 2: running the restore after every stage (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R781) would let v2 template defaults overwrite a format-0 project's calibration during the v1->v2 split. In a simulation, INS_ACCSCAL_X became 1 and COMPASS_OFS_X became 0. Running the restore once after the final stage avoids this. The earlier v1->v2 points still reproduce: dropped FENCE_/FRAME_TYPE/MOT_THST_HOVER (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R392), destination-wins precedence (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R684) and trim-before-write (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R675). Also, the 31_/32_ PID-notch steps point at the motor-notch/logging sections and FilterReview rather than guide sections 8.4.1/8.4.2 and PIDReview (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R875). I haven't checked the GUI, uploads to a real flight controller, the full test suite or CI (still pending). Head moved during review: observed |
54ac5f6 to
008c8a8
Compare
Previous review (2026-09-30)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head This follow-up covers 54ac5f6. The PR has since moved to 008c8a8, which I have not reviewed, so some of this may already be fixed. Since fb20079 the only change is pylint-disable comments. Keeping the format version at 1 is still fine. Format-0 projects migrate exactly as on master. Two things still block, and both show up as the four failing tests in the Windows CI job. First, Holybro_X500_mig still ships 14_mp_setup_mandatory_hardware.param (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-db615890f93903a738b18f6c753d278b6c15bddd6c3c77a0cefbf046ddf18d30R37). No step lists it, but it loads after 14_accelerometer_calibration.param, so its older INS_ACCSCAL_X 0.998941 and INS_ACC1_CALTEMP 43.062431 win. Deleting it, or folding any remaining parameters into the v2 steps, fixes both of the prefix tests it breaks. Second, tests/test_vehicle_template_param_files.py still rejects 67..70 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-ce0d98b91169e438e72e4299ab5347d00ca764b2df4b6b730fb957ba0ccc1a58R1) and 58/59 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-cc785ca12db778535114db7dface2219a0a753665272579240057cf04d6b455eR1), so it needs to accept the v2 layout for this template. Before the target goes to 2, a few things are still worth fixing. Run the restore once after the final stage (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R781). Move rather than delete the FENCE_/FRAME_/MOT_THST_HOVER values (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R392). Let the effective step-14 value win (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R684). Write destinations before trimming the source (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R675). The 31_/32_ PID-notch links (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R875) should point at sections 8.4.1/8.4.2 and PIDReview. One follow-up in existing plugin code: after a successful full accelerometer calibration, _end_full_calibration does not pass parameter_names_to_copy (ardupilot_methodic_configurator/plugins/frontend_tkinter_accelerometer_calibration.py:332). The staged values therefore keep the old calibration, and an upload could revert it. The simple-calibration path already handles this. This template is the first to bind that plugin (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R473), but the fix belongs in the plugin and does not need to block this PR. Not checked: the GUI, real flight-controller calibration or uploads, SITL, and the full suite. Head moved during review: observed |
008c8a8 to
815cdba
Compare
Previous review (2026-09-30)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 815cdba, the current head. The migration fixes all check out. The restore now runs once after the final stage, source values win, destinations are written before the source is trimmed, and the PID-notch links are correct. Keeping the format version at 1 is still fine. Three things still block, and I reproduced all three at this head:
Things to fix before the target goes to 2, none blocking now:
Separately, and not blocking this PR: after a full accelerometer calibration, _end_full_calibration (plugins/frontend_tkinter_accelerometer_calibration.py:332) does not copy the new values to the staged parameters, so a later upload could revert them. Not checked: the GUI, real flight-controller calibration or uploads, SITL, and the full test suite. |
815cdba to
3d626ae
Compare
Previous review (2026-10-01)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 3d626ae, the current head. It is a straight rebase of 815cdba with an identical patch, so the earlier findings carry over unchanged. The migration fixes still check out, and keeping the format version at 1 is still fine. Three things still block, all reproduced at this head:
Before the target goes to 2 (not blocking now):
Separately: this template is the first to use the accelerometer plugin (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R473). After a full calibration, _end_full_calibration (plugins/frontend_tkinter_accelerometer_calibration.py:332) does not copy the new values to the staged parameters, so a later upload could revert them. That is a plugin fix, not a blocker here. Not checked: the GUI, real calibration or uploads, SITL, the full test suite, and the 7 CI errors, which I could not reproduce or explain from the non-GUI tests. |
|
FENCE_/FRAME_TYPE/MOT_THST_HOVER should be dropped. That change is intentional. |
075fc9a to
843629d
Compare
Previous review (2026-10-01)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at fc49500. The head has moved since my last review at 3d626ae: the feat commit was reworked, and a WIP commit plus a pre-commit.ci commit were added. Nothing newer is covered. Thanks for the fixes. The stray mandatory-hardware file is gone, the template prefix tests accept the v2 layout, the 11_/12_ split and regression tests are in, the guide is corrected and pylint is clean. Dropping FENCE_/FRAME_TYPE/MOT_THST_HOVER is fine since you've confirmed it's intentional, and keeping the format target at 1 is still fine. Three things still block, all verified at this head:
Before the format target goes to 2 (not blocking now):
Separately, _end_full_calibration (plugins/frontend_tkinter_accelerometer_calibration.py:332) still does not stage the full-calibration results, and this template is the first to use that plugin (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R473). That is a plugin follow-up, not a blocker here. Not checked: the GUI, real calibration or uploads, SITL, the full suite, and the root cause of the 7 ubuntu-3.10 acceptance errors (an offline run shows a matching download timeout, but the CI traceback wasn't read). |
8c125d3 to
c77db2b
Compare
Previous review (2026-10-01)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at c77db2b. The head has moved since my last review at fc49500: the WIP and pre-commit.ci commits were replaced by "fix(migration): WIP". Nothing newer is covered. Thanks for the fixes. The test file is now executable, both commit subjects pass the Conventional Commits check, INITIAL_MODE now moves to 17_, and the CALTEMP, 18_ order and 08_/35_ comment mismatches are gone. One thing still blocks, verified at this head: test_existing_holybro_x500_migrates_to_x500_mig_parameter_files still fails (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR87), now on 2 files instead of 7, and the CI summary for c77db2b shows 1 failure.
Minor: the test makes 17_ match by appending INITIAL_MODE,0 to the copied source (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR45). None of the templates, now or in history, store INITIAL_MODE in a mandatory-hardware file, so the comment "Older projects stored the startup mode with the mandatory hardware values" should say this is a synthetic legacy case. Adding a case with the unmodified source would also work. Before the format target goes to 2 (not blocking now):
Separately, the accelerometer plugin's _end_full_calibration (plugins/frontend_tkinter_accelerometer_calibration.py:332) still doesn't stage the calibration results. That's a plugin follow-up. Not checked: the GUI, real calibration or uploads, SITL, the full suite, and which test failed in CI at this head. |
c77db2b to
c65bf9c
Compare
Previous review (2026-10-04)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at c65bf9c. The head has moved since my last review at c77db2b: the WIP commit was rewritten and rebased onto c099f29. Nothing newer is covered. Thanks, the X500 migration test now passes. 07_ is reordered and 14_ carries the source calibration. Blocking, verified at this head: the new empty_4.6.x_mig template fails 3 template tests.
Should fix, not blocking: the restore pass runs at the current target 1 and treats an empty legacy file as missing (st_size > 0 at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R594). A format-0 project with an empty 04_board_orientation.param gets template 05_ defaults, and 04_ is then left behind because the rename refuses. Please treat an existing empty alias as present. Before the format target goes to 2 (not blocking now): the accelerometer move (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R325) still leaves INS_ACC*OFFS behind, although step 14 owns them (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R478). The moves under "all" (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R321) still give Rover an undeclared 18_servo_outputs.param. Minor: neither commit has a Signed-off-by, which CONTRIBUTING.md asks for. I'm dropping my earlier request about the INITIAL_MODE comment (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR43): your move rule handles that case, and I can't show older projects didn't store it there. Separately, the accelerometer plugin's _end_full_calibration still stages nothing; that's a plugin follow-up. Not checked: the GUI, real calibration and uploads, SITL, the full suite, and CI at this head. |
c65bf9c to
9796a9e
Compare
61e075f to
85483fc
Compare
Previous review (2026-10-05)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 85483fc. The head has moved since my last review at c65bf9c: the PR is now one signed-off commit rebased onto d1786e2. Nothing newer is covered. Thanks, the earlier problems are fixed. empty_4.6.x_mig is format 2 and its 13_/67_ files parse. An empty legacy alias now blocks the template restore. INS_ACC*OFFS move into 14_, and Rover no longer gets 18_servo_outputs.param. Blocking, verified at this head:
Should fix, not blocking: pylint still reports too-many-locals (18/15) at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR30. A Follow-up only: step 14 in the _mig template uses the accelerometer plugin (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R473), whose full-calibration path still stages nothing. That's existing plugin code and is worth fixing before v2 goes live. Not checked: the GUI, real calibration and uploads, SITL, the full suite, and a native Windows rerun. |
e4cc81f to
99853ec
Compare
Previous review (2026-10-06)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 67ed4a0. The head has moved since my last review at 85483fc: there are now four commits on 1579de4. Nothing newer is covered. Thanks, both previous blockers are fixed. The script test now loads by file path and passes under plain Should fix, not blocking: the X500 migration test compares byte-for-byte with the committed Holybro_X500_mig files (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR157). update_vehicle_templates.py re-saves every template directory, mig included, after each template push to master. When I applied its output, the test failed on 03, 07_, 14_, 18_ and 35_. 35_ changes because the _mig steps force 'wp-advance' (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-99c9bf1eb190ad1fd97fb5a2d0764016b60efd49cc35263a3b1c52d5e5d43e58R966) but the committed file says 'advance-wp' (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-70bb1c4218f92e0db54630a01d3de53bf30542befaed8721c39cf24eb5264b60R9). Skipping *_mig in find_template_directories is probably the simplest fix; otherwise commit the files in the app's own format with that comment. As it stands, the first template-update bot PR after merge would fail CI. Minor: the tip commit 67ed4a0 has no Signed-off-by. CONTRIBUTING.md asks for it on the last commit; Notes, not change requests:
Not checked: the GUI, real calibration and uploads, SITL, the full suite, native Windows/macOS reruns and frozen builds. |
8a42f5a to
b0ea0e4
Compare
Previous review (2026-10-06)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at fa12542. The PR head has since moved to b0ea0e4, which this review does not cover. Thanks: the restore race and the in-place overwrite are both fixed, and the 35_ comment mismatch is gone. None of the following blocks while the format target stays at 1, but each should be fixed before v2 is switched on:
Minor: fa12542 and 72191ac are missing Signed-off-by. Not checked: the GUI, real calibration and uploads, SITL, the full suite, native Windows/macOS, frozen builds and real FAT/SMB filesystems. Head moved during review: observed |
b0ea0e4 to
b734a90
Compare
Previous review (2026-10-08)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head This covers 61a9104, the current head. Thanks for the fixes: the general-configuration collision, the hard-link fallback, the MagFit sidecar and the v2 filenames in the tuning guide are all VERIFIED fixed. Blocking: the PR's own test_existing_holybro_x500_migrates_to_x500_mig_parameter_files[lf]/[crlf] fails at https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-e759f73f1d32b0977f1abee91334ddebcf106109c572384d3e22527bb8d1e74eR215, locally and in CI on Linux, macOS and Docker. The migration appends the moved INS_ACC1/2_CALTEMP lines at the end of 03_ (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R1001), while the fixture has them first (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-c539332f72dedc514dad5750552e236ffc394172f4003b5fa06b4758a7e2e2d7R1). The values are the same; only the order differs. update_vehicle_templates.py also still re-sorts Holybro_X500_mig 06_/11_/15_, so it would break the test again. Writing in missionplanner_sort order would fix both, as would committing the fixture in migration order and skipping *_mig in the updater. Before v2 is enabled (not blocking while the target is 1):
Minor: 56c4f60, 408eb9c, 30851d8, fa12542 and 72191ac are missing Signed-off-by. Not checked: the GUI, real calibration and uploads, SITL, the full local suite, native Windows/macOS, frozen builds, real FAT/SMB filesystems and the network-backed template updater. |
ede2150 to
f626791
Compare
Previous review (2026-10-08)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head This covers pinned head ede2150. The PR has since moved to f626791, which I have not reviewed. Thanks for the fixes: the X500 migration test now passes for LF and CRLF, the updater skips *_mig, and the empty_4.6.x_mig guide anchors all resolve (VERIFIED). Blocking: 2ef5eb3 writes OSD_TYPE,0 into 18_osd.param of live v1 templates. Chimera7, Marmotte5v2 and TarotFY680Hexacopter default to OSD_TYPE,1, so uploading step 18 now turns off the onboard OSD (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-9491d7fb1c20f4fded3797850f7e63a7e17cf86d7c353b91838ee40019615bdcR1). This is not behind the format gate. Use OSD_TYPE,1 there, or handle empty aliases in the migration instead. Before v2 is enabled (not blocking while the target is 1):
Reachable today, low: without hard links, a crash or concurrent open can leave a partial restored file that still counts as done (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R679), and restored steps do not get their .pdef.xml (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R786). Minor: 45cb231, 4024edf, 2ef5eb3, 69e5c3f, d83d569, 488c35a and ede2150 have no Signed-off-by. Not checked: the GUI, real uploads, SITL, the full suite, native Windows/macOS, real FAT/SMB filesystems, the cause of the CI failures (they match master), and f626791. Head moved during review: observed |
Previous review (2026-10-09)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head This review covers f626791, which is a rebase of the previously reviewed ede2150 with all 13 patches unchanged; the PR is still at that head. The earlier fixes still hold: the X500 LF/CRLF migration test passes, the updater skips *_mig, and the guide anchors resolve. Locally, 587 PR-related tests pass (VERIFIED). Blocking: ebbf972 writes OSD_TYPE,0 into 18_osd.param of live v1 templates. Chimera7, Marmotte5v2 and TarotFY680Hexacopter default to OSD_TYPE,1, so uploading step 18 turns off their onboard OSD (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-9491d7fb1c20f4fded3797850f7e63a7e17cf86d7c353b91838ee40019615bdcR1). This is not behind the format gate. Use OSD_TYPE,1 there, or handle empty aliases in the migration instead. Before v2 is enabled (not blocking while the target is 1):
Reachable today, low:
FYI, no change required: 072c3c6 also changes the live Holybro_X500 template, which its message doesn't mention: BATT_FS_VOLTSRC 1 to 0 (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-fffa94ad06ed7f13f64f2a0d24752b2db6fd7f9f59c0b46cbdb318d1b751f08dR7), TCAL TMAX 70 to 50, and BRD_SAFETY_DEFLT,0 added. Please confirm these are intended. Minor: f626791, 0d7663a, 33e7d1d, 072c3c6, ebbf972, b40351d and 4c459d0 have no Signed-off-by. Not checked: the GUI, real uploads, SITL, the full suite, native Windows/macOS, and real FAT/SMB filesystems. The macOS CI failure also fails on base, but its cause was not re-checked. |
|
Flight modes come from 14_ and ignore later overrides; <- that is fine. Without hard links, a crash or concurrent open can leave a partial restored file that still counts as done <- that is fine for now. changes the live Holybro_X500 template: BATT_FS_VOLTSRC 1 to 0, TCAL TMAX 70 to 50, and BRD_SAFETY_DEFLT,0. These are intended |
d3a5078 to
8495b2b
Compare
Previous review (2026-10-10)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head This review covers 8495b2b, the current head. Since the last review at f626791 the 13 earlier patches are unchanged and three commits were added (829e711, 0eb3f0b, 8495b2b). Thanks for the replies of 2026-10-09; they are taken as settled below. Blocking: 8495b2b renamed 15_accelerometer_calibration.param to 16_, but the schema test still looks up the old name (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-cba1762653fc48e7e620da511b693f917163e219625a6505494aa4e34c2cd26eR58). test_optional_fourth_and_fifth_imu_calibration_uses_real_subgroup_names fails with KeyError here and on CI (ubuntu 3.10, ubuntu 3.14, windows 3.14; 6530 pass, 1 fail). Changing the key to 16_accelerometer_calibration.param makes it pass. Fixed and verified: OSD_TYPE in the three OSD templates, the empty 18_osd.param rename, initial_atc now before the ESC and general steps, the IMU-temperature skip target, and .pdef.xml restore. Before v2 is enabled (not blocking while the target is 1):
Reachable today, low: the new empty-file handling in rename_parameter_files() (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-cf28409654b00ab8706721d7366feaf693978b04612204ba7b945adac7563716R295) and the sidecar restore (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R787) have no test; reverting both still passes 548 tests. A non-empty old-named file next to an empty new-named one is now left in place with an error, where it used to be moved over the empty file. Accepted as replied, nothing requested: flight modes from 14_, the fallback copy without hard links, and the Holybro_X500 value changes. Minor: 9 of 16 commits have no Signed-off-by (0eb3f0b, 829e711, 272fd18, 4a8ad2f, 182bbed, 2699e20, a84fbe7, 0a1af3b, bce4b21); the tip is signed. Not checked: the GUI, real uploads, SITL, the full suite locally, native Windows/macOS, frozen builds and real FAT/SMB filesystems. The macOS and Docker CI failures at this head were not diagnosed. |
db7fd21 to
8ac32eb
Compare
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head This review covers 8ac32eb. The head has moved since the last review (8495b2b): the branch is now two commits on e1880a6. There are no new replies since 2026-10-09. Thanks for the fixes. The schema test, the filter values (now moved to 29_motor_notch_filter_results.param, with no template changing value), the MagFit sidecar in empty_4.6.x_mig, the empty-placeholder rename and the sign-offs are all verified. Needs fixing before merge:
Before v2 is enabled (not blocking while the target is 1):
Tests, low: the rename of a 0-byte old-named file (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-cf28409654b00ab8706721d7366feaf693978b04612204ba7b945adac7563716R329) and the .pdef.xml restore from the template (https://github.com/ArduPilot/MethodicConfigurator/pull/2133/files#diff-6f9d3a1b7af838fb1881b7dd63ff45af36aaa81c128f1487b7e1834acd561935R863) still have no test; removing either leaves the test result unchanged. Optional: the tip commit is a 'chore(templates)' about tests but carries the migration engine and the git-based rename, so the generated release notes would list it under Maintenance. Accepted as replied, nothing requested: flight modes from the hardware step, the copy fallback without hard links, and the Holybro_X500 value changes. Not checked: the GUI, real uploads, SITL, native Windows and macOS, frozen builds and real FAT/SMB filesystems. Docker Build and Test and markdown-link-check also fail at this head and pass at the base; I did not diagnose them. macOS pytest fails at the base as well. |
This will be used to test and train the v1->v2 migration logic and to help develop the plugins that will remove the need to use mission planner when configuring a vehicle. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>
…ints Normalize CRLF to LF when comparing migrated parameter files, preserving all other bytes so values, comments, and spacing remain significant. Exercise X500 migration with isolated LF and CRLF template copies. Copy numbered parameter files and JSON inputs while excluding local download artifacts such as complete.param. Add regressions verifying that: - Equivalent LF and CRLF files compare equally - Comparison does not modify source files - Value, comment, and spacing changes remain detectable Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>
8ac32eb to
5fbfa38
Compare
Description
This will be used to test and train the v1->v2 migration logic and to help develop the plugins that will remove the need to use mission planner when configuring a vehicle.
Checklist
git commit --signoff)Testing
Describe how you tested these changes: