More autoimports - #2047
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A missing comma in configuration_steps_ArduCopter.json makes the file invalid JSON, which would break loading/validation at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands ArduCopter configuration-step metadata to automatically import additional non-default parameters (via autoimport_nondefault_regexp) and adds/updates a few user guidance messages (instruction popups and step notes) to better reflect the workflow.
Changes:
- Extend
autoimport_nondefault_regexplists across multiple ArduCopter steps to capture more relevant parameters automatically. - Add
instructions_popupblocks to additional steps and refine some popup guidance text. - Update an
auto_changed_bymessage to instruct the user about required external action.
File summaries
| File | Description |
|---|---|
| ardupilot_methodic_configurator/configuration_steps_ArduCopter.json | Adds more auto-import regex rules and additional instructional UI metadata for ArduCopter steps. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
42d879b to
58da5ab
Compare
amilcarlucas
added
the
AIReview
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Previous review (2026-09-09)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Verdict: REQUEST CHANGES — one blocker in the new FC-import workflow, plus a batch of parameter-name fixes that are cheap and mechanical. Blocker: the FC-import path uses the wrong "what is non-default" baseline
Reproduced end-to-end with a real Proved by mutation. Inserting The exact count is template-dependent — a second reviewer measured 65 using The same root cause also loses genuine settings, which is the worse direction. With Why the tests miss it: Suggested fix: snapshot Ten new autoimport patterns use 4.6 names renamed in 4.7
This is inconsistent with the file's own convention rather than a deliberate choice — Two smaller issues
Notes
What was checked and came out cleanAutoimport cannot clobber user data — this was the first thing I looked for and it holds: CI is effectively green — 33 checks pass including all four pytest matrix jobs (4 956 tests, 0 failures). The two red checks are unrelated infrastructure: |
… values that are relevant for a config step Add support for multiple batteries
Introduce ParameterForcedOrDerivedError for attempts to modify forced or derived parameters. Log this expected FC-copy condition as a warning while retaining traceback logging for unexpected value errors. Update affected tests to verify the dedicated exception and logging behavior.
00bff79 to
f237851
Compare
Add an opener action and minimal project creator for already configured flight controllers. Automatically select the matching empty firmware template, apply FC parameter values, and infer component specifications and connections. Refactor creator widget setup, add BDD coverage for FC workflows, and update screenshot automation for the new UI.
Refresh the application screenshots used by the documentation. Stabilize the file 24 capture workflow by waiting for the FC-copy dialog to finish initializing before selecting “No”.
f237851 to
f80d0c4
Compare
When creating a project from a template with FC parameter values enabled, snapshot the connected controller's defaults before reinitializing the new project and write them to 00_default.param. Add a real-filesystem regression test covering template creation and ensuring FC-specific default values are preserved.
|
Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting. Re-reviewed at head Verdict: REQUEST CHANGES — but for a different reason than last time. Everything I raised last round is resolved, and the blocker is proven fixed in both directions. An independent cold pass then found a new defect in the autoimport feature itself, and I have reproduced it: an autoimported parameter is never recorded as a change, so with the shipped default settings it is displayed in the editor and never written to disk. Resolved since
|
scenario (real LocalFilesystem, factory-settings FC) |
at head | ordering reverted |
|---|---|---|
FC defaults = Holybro_X500_V2 (1086 params, 138 differ from empty_4.6.x) |
0 imported; 00_default.param == FC defaults (1086) |
73 imported; 00_default.param = template's (1019) |
SERIAL5_BAUD=57, FC default 115, template default 57 |
00_default.param = 115.0, exported as 57.0 — retained |
00_default.param = 57.0, SERIAL5_BAUD in no file — dropped |
Both directions confirmed, and both fixes are pinned: reverting 013ee1986a's ordering fails tests/test_data_model_vehicle_project.py:460 (assert 57.0 == 115.0) plus test_configured_fc_defaults_survive_destination_reinitialization; reverting 0fba8236b4's three lines fails :503. The MagicMock(spec=LocalFilesystem) gap is closed with real-filesystem tests, and no tests were lost in the refactor (that file went 61 → 70).
All ten 4.6→4.7 renamed patterns → broadened exactly as suggested. Re-derived independently against your own empty_4.6.x (1019 params) and empty_4.7.x (1090): :369 (WPNAV_RADIUS|WP_RADIUS_M) 1/1, :398 ATC_ACC(EL)?_[PRY]_MAX 3/3, :468 (ATC_)?ANGLE_MAX 1/1, :485 ATC_(SLEW_YAW|RATE_WPY_MAX) 1/1, :663 PSC_(ACCZ|D_ACC)_[IP] 4/4, :986/:1017/:1051 1 each, :1332 1 and 12.
The second harmonic notch → :600 and :699 are now INS_HNTC[H2]_.*, matching 17 of the 17 parameters in Big_Owl/25_motor_notch_filter_results.param, up from 8.
Button wording and the stray space → frontend_tkinter_project_opener.py:92 and frontend_tkinter_project_creator.py:144 now agree, all four TUNING_GUIDE_*.md quote that wording, and "connected , you can" is gone.
BUG — an autoimported parameter is never written to disk with default settings
data_model_configuration_step.py:189-203 injects the imported parameter straight into current_step_parameters. The two sibling injection paths both record it as a change — forced parameters at data_model_parameter_editor.py:1954 and add_parameters at :1971 each do self._added_parameters.add(param_name) — but _apply_auto_imports records nothing.
It also cannot be is_dirty: it is built as Par(float(live_value), ""), so ArduPilotParameter.__init__ sets _value_on_file and _new_value to the same value (data_model_ardupilot_parameter.py:86, :103) and leaves the change reason equal to its on-file counterpart.
So _has_unsaved_changes() (:2284-2305) returns False, and the save gate at :462 reads has_param_changes = self._has_unsaved_changes() or annotate_params_into_files. That second term is the only thing that would save the value — and ProgramSettings ships "annotate_docs_into_param_files": False (backend_filesystem_program_settings.py:177), plumbed through frontend_tkinter_parameter_editor.py:481-482. Measured by driving your real _has_unsaved_changes and handle_write_changes_workflow:
| case | _has_unsaved_changes |
prompted | written |
|---|---|---|---|
| autoimported, annotation OFF (the default) | False | 0 | 0 |
| autoimported, annotation ON | False | 1 | 1 |
| user-edited, annotation OFF | True | 1 | 1 |
The cold pass reached the same conclusion from the other end, with a real LocalFilesystem and empty_4.6.x: a non-default ATC_RAT_RLL_PDMX=0.5 appeared in the editor, the save workflow reported no changes, no prompt was shown, and the value was absent from disk.
Suggested fix: have _apply_auto_imports return the names it injected and add them to _added_parameters, exactly as the add_parameters path already does at :1971.
Why the tests miss it: every autoimport test exercises _apply_auto_imports in isolation and asserts only on the contents of current_step_parameters; none continues into the save workflow. Deleting the new quick-tune rule outright still leaves 548 tests passing.
Also worth fixing
- I/O failures in the new configured-FC workflow escape as untranslated tracebacks.
create_new_vehicle_from_flight_controller(data_model_vehicle_project.py:202-250) raises a translatedVehicleProjectCreationErrorfrom every guard clause, but calls_complete_imported_vehicle_project_creationwith notry, and neitherwrite_param_default_values_to_file(:323) norexport_to_param(:329) translates anything. Making the destination read-only reproduces a realPermissionErrorescaping the window callback withmessagebox.showerrornever called. Related, same handler:frontend_tkinter_project_creator.py:255-259catches onlyVehicleProjectCreationError, butopen_vehicle_directory(:336,:408-425) raisesVehicleProjectOpenError— an unrelatedExceptionsubclass — after the project has been written. The template path at:245-249has the identical exposure so that half is not a regression, but this is new code and could catch both. This corrects my clean note from last round, where I said the FC-creation failure paths all raise a translated error: true of the guard clauses, not of the I/O. - (re-raised) The two referenced screenshots are still not committed.
USERMANUAL.md:183and:226referenceimages/App_screenshot_Vehicle_directory_create_from_flight_controller.pngand…_creator.png; neither exists at head, and they are the only two broken image references in the file.f80d0c464b chore: regenerate application screenshotsadds no image files at all — it touches onlyscripts/regenerate_app_screenshots_fully_automated.pyandfrontend_tkinter_project_opener.py. (The…_create_from_configured_{options,name,create}.pngfiles thatUSECASES.md:155-159uses do exist.) Notemarkdown-link-checkdid not run for this head — it is path-filtered and your last commit touches only.py— so CI will not catch these. - One more instance of the rename class you just fixed:
configuration_steps_ArduCopter.json:748carriesPSC_ACCZ_N[ET]F, which matches 2 names in 4.6 and 0 in 4.7 — they arePSC_D_ACC_NEF/PSC_D_ACC_NTFnow, and your ownempty_4.7.x/28_pid_notch_filter_results.paramstores exactly those, so on 4.7 the vertical-accel notch values never autoimport into step 28. The line is pre-existing rather than added here.PSC_(ACCZ|D_ACC)_N[ET]Fmatches 2 on both. - The notch fix stops one group short:
INS_HNTC[H2]_.*missesINS_HNTC3_*andINS_HNTC4_*. ArduPilot registers all four groups (libraries/AP_InertialSensor/AP_InertialSensor.cpp:574,:579,:585,:591) and your ownempty_4.7.x/00_default.paramalready containsINS_HNTC3_ENABLE. Measured:INS_HNTC[H2]_.*→ 2 names in 4.7,INS_HNTC[H2-4]_.*→ 3.
Notes
- (re-raised)
re.matchis start-anchored but not end-anchored, so several patterns stay broader than they look, re-derived at this head::649ATC_RAT_(PIT|RLL|YAW)_[DIP]→ 18 in both 4.6 and 4.7 (not 9 — it also pulls_IMAX,_PDMX,_D_FF),:663→ 4,:485RTL_ALT→ 3. Also broad:RC[0-9]+_.+96,EK3_.*83,SERVO[0-9]+_.+80. Appending$gives exact matching where that is what you want. Note only — cross-step overlap is clearly normal in this file. - The forced/derived warning drops the parameter name.
data_model_parameter_editor.py:352logslogging_warning("%s", exc), so the message names no parameter, unlike its siblings at:349and:355. And if every relevant parameter is forced or derived,params_copiedstays 0,_update_parameters_from_fc_valuesreturnsFalse, and the user gets no feedback at all after answering YES to the copy dialog (:377-388). - Coverage gaps, partially addressed — suggestion only. Now covered:
ATC_ANG_YAW_P(:398),SCR_ENABLE(:250),BRD_HEAT_TARG(viaBRD_HEAT_.*at:14). Still matched by nothing, over your 21 ArduCopter template dirs:ATC_ANG_{RLL,PIT}_P20/21,MOT_SPIN_MAX20/21,TKOFF_RPM_MIN20/21,SCHED_LOOP_RATEandINS_POS{1,2}_X20/21,LAND_ALT_LOWandPSC_ACCZ_SMAX19/21,PSC_VELXY_FF19/21,AUTO_OPTIONS13/21. - Docs:
USERMANUAL.md:232-236does not mention that the FC's defaults are written to00_default.param, nor that anxx_imported_flight_controller_parameters.paramfile may be created — both of which the analogous bin-log section (USERMANUAL.md:248-251,USECASES.md:119) documents.
Checked and clean
Autoimport still cannot clobber user data or defeat a forced/derived rule — re-checked against the new regexes with an adversarial FC set (empty_4.7.x defaults with every value perturbed so nothing looks default), driving the real _apply_auto_imports over every step file: 688 parameters autoimported across 25 step files, 0 existing step parameters modified, 0 forced or derived parameters added or overridden. The bug above is a missing change-record, not a broken precedence rule. 3404535d22's exception ordering is correct and pinned — ParameterForcedOrDerivedError is caught at :351-353, before except (ValueError, TypeError) at :354-356, and swapping them fails test_system_warns_without_a_traceback_when_a_parameter_is_forced_or_derived. _get_fc_template_dir_for_project_creation raises a translated error on every guard branch; the opener button is disabled unless an FC is connected and has parameters; data-model/view separation respected. I chased a hypothesis that :186 could snapshot stale defaults from a previously-open project and disproved it — the creator is only reachable when the vehicle dir has no parameter files, so param_default_dict is either the FC's defaults or empty, and empty is falsy so :191 skips. Local tests: 510 passed across the five relevant data-model suites, 36 passed / 2 skipped across the two frontend project suites.
Copilot's invalid-JSON claim is still wrong — the file parses at this head (63 steps) and the comma it says is missing is present at :671. Flagging again so you do not go looking.
CI: 30 pass / 1 skip / 1 fail. All four pytest matrix jobs green (5 174 tests, 0 failures). The failure is Publish Tests Results → Check coverage (coverage report --fail-under=89) against TOTAL 20156 2105 6116 607 88%; master at your base cf063929b6 gives 20091 … 88% and fails identically, as does master at the merge base — not yours. Separately worth raising with the maintainer: the repo's two coverage reporters disagree — the PR comment bot reports 90% (statement-only, 18051/20156 = 89.6%) while the gate uses coverage report, which includes branch coverage and yields 88%.
Description
More autoimports
Checklist
git commit --signoff)Testing
Describe how you tested these changes: