Skip to content

Fix#1189 ask confirmation before saving derived parameter changes to file#1252

Open
OmkarSarkar204 wants to merge 2 commits intoArduPilot:masterfrom
OmkarSarkar204:fix/derived-param-confirmation
Open

Fix#1189 ask confirmation before saving derived parameter changes to file#1252
OmkarSarkar204 wants to merge 2 commits intoArduPilot:masterfrom
OmkarSarkar204:fix/derived-param-confirmation

Conversation

@OmkarSarkar204
Copy link
Contributor

@OmkarSarkar204 OmkarSarkar204 commented Feb 4, 2026 •
edited
Loading

Fix #1189

Earlier, derived parameters were recalculated and saved immediately, which could overwrite user parameter files without any confirmation. I tried to add a small safety step so the backend can detect derived changes first and let the frontend decide whether to commit them.

Screenshot (7)

I am not fully sure if this is the best approach or if its necessary. This is mainly an attempt to make the behavior explicit and safe rather than silent.

@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 5, 2026

Hi @amilcarlucas ,
I tried a simple fix here. It now shows a popup when derived parameters changes. The UI shows a popup listing the affected files and asks the user whether they want to save them.

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Feb 5, 2026

Thanks. I'll take a look at it soon.

@amilcarlucas amilcarlucas mentioned this pull request Feb 6, 2026
Open
@amilcarlucas amilcarlucas force-pushed the fix/derived-param-confirmation branch 2 times, most recently from 58ea5eb to 70ce9c9 Compare February 9, 2026 17:40
@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Feb 9, 2026

I rebased, squashed and force pushed this. I plan to merge it soon.

@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 9, 2026

Thank you so much, I'll make sure to clean up my commit history next time to save you the effort.

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Feb 9, 2026

I did a clean up commit on top of your changes. This way you can clearly see what I changed.

@OmkarSarkar204 OmkarSarkar204 force-pushed the fix/derived-param-confirmation branch 2 times, most recently from e09b0da to aa6b4e3 Compare February 16, 2026 08:15
@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 16, 2026

I fixed the pylint issue, passing now.

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Feb 16, 2026 •
edited
Loading

Thanks.
Can you refactor this to be a single step approach?

  1. The parameters are calculated and stored in a dict[str, ParDict] datastructure with both values and comments.
  2. Then the user gets prompt, and if he answers yes, the data stored in the dict gets saved to file(s). If not, it gets discarded.

This will avoid the duplication in calculating it all twice and make the code cleaner.

@OmkarSarkar204 OmkarSarkar204 force-pushed the fix/derived-param-confirmation branch from aa6b4e3 to 1d30dbb Compare February 23, 2026 09:27
@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 23, 2026

I've refactored the code to use the single-step approach you suggested:

Separated Computation from Saving: update_and_export_vehicle_params_from_fc now only handles the math and updates the dict[str, ParDict] in memory. It returns the list of pending changes but no longer touches the disk.

If the user clicks 'Yes' on the prompt (or if there are no derived changes), the code calls a new save_vehicle_params_to_files function to write that in-memory dictionary to disk.

@OmkarSarkar204 OmkarSarkar204 force-pushed the fix/derived-param-confirmation branch 2 times, most recently from 2f55b2a to e7bbb69 Compare February 23, 2026 12:47
@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 24, 2026 •
edited
Loading

The macOS (Python 3.14) CI job failed with an exit code 139 (segfault) during tests/unit_frontend_tkinter_battery_monitor.py.
I'm not sure why this specific CI runner is crashing here. Could you please advise on how to handle this, and re-trigger the job when you have a moment?
Its passing locally.

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Feb 24, 2026

It's re-triggered.

@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Feb 25, 2026

It's re-triggered.

Thank You! Its passing now.

@amilcarlucas amilcarlucas force-pushed the fix/derived-param-confirmation branch from 5c38fff to c0fe055 Compare March 4, 2026 00:48
OmkarSarkar204 and others added 2 commits March 4, 2026 14:42
@OmkarSarkar204 @amilcarlucas
fix(parameter-editor): require confirmation for derived parameter cha…
c9fede5 
…nges

Derived parameters are no longer saved automatically. The system now
computes changes without mutating the in-memory model and presents them
to the user for confirmation before applying.

Changes are applied to the in-memory model after user approval, but are
only persisted to disk when the user executes the corresponding configuration
step in the parameter editor.

Implements two-phase approach:
- Phase 1: Compute derived values without side effects
- Phase 2: Ask user confirmation before applying changes

Fixes ArduPilot#1189

Signed-off-by: Omkar Sarkar <omkarsarkar24@gmail.com>
Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>
@amilcarlucas
refactor(project creator): refactor the template files mutation to th…
f1110ab 
…e copy_template_files_to_new_vehicle_dir method

This makes it orthogonal from the component derived parameter changes at a later stage.
It also makes it easier to understand and test.
@amilcarlucas amilcarlucas force-pushed the fix/derived-param-confirmation branch from c0fe055 to f1110ab Compare March 4, 2026 13:43
@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Mar 4, 2026

I need to do some more work on this one. But it's almost there.

@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Mar 4, 2026

I need to do some more work on this one. But it's almost there.

Ok! Please let me know if there’s anything else needed from my side

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Mar 4, 2026

I added a couple of new issues. Can you help implementing them?

@OmkarSarkar204
Copy link
Contributor Author

OmkarSarkar204 commented Mar 4, 2026

I added a couple of new issues. Can you help implementing them?

Sure, I'll check those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amilcarlucas amilcarlucas Awaiting requested review from amilcarlucas amilcarlucas is a code owner

At least 0 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Always ask confirmation before saving derived parameter changes to file

2 participants

@OmkarSarkar204 @amilcarlucas