Skip to content

Commit 4d83cd3

Browse files
committed
fix(upload): replace recursive retry with iterative loop in upload_selected_params_workflow
The recursive retry path had three bugs: 1. Missing get_connection_progress_callback argument — the download callback factory was passed in position 7 (where connection callback belongs), causing wrong progress text on retry and no download progress at all. 2. Unbounded recursion — each retry added a stack frame, risking RecursionError on persistent FC communication issues. 3. Redundant side effects — _write_current_file() and the _at_least_one_changed reset executed once per recursion depth on unwind, not just once. Replace with a while loop that re-runs the entire upload/validate cycle on retry. All callback factories are now correctly re-invoked each iteration, _write_current_file() runs exactly once after the loop exits. Closes #1427 Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
1 parent 2308180 commit 4d83cd3

File tree

1 file changed

+46
-50
lines changed

1 file changed

+46
-50
lines changed

ardupilot_methodic_configurator/data_model_parameter_editor.py

Lines changed: 46 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,65 +1001,61 @@ def upload_selected_params_workflow( # pylint: disable=too-many-arguments, too-
10011001
get_download_progress_callback: Optional factory function that creates and returns a download progress callback.
10021002
10031003
"""
1004-
logging_info(
1005-
_("Uploading %d selected %s parameters to flight controller..."),
1006-
len(selected_params),
1007-
self.current_file,
1008-
)
1004+
should_retry = True
1005+
while should_retry:
1006+
should_retry = False
10091007

1010-
# Get progress callbacks from factories if provided
1011-
progress_callback_for_upload = get_upload_progress_callback() if get_upload_progress_callback else None
1012-
progress_callback_for_reset = get_reset_progress_callback() if get_reset_progress_callback else None
1013-
progress_callback_for_connection = get_connection_progress_callback() if get_connection_progress_callback else None
1014-
progress_callback_for_download = get_download_progress_callback() if get_download_progress_callback else None
1015-
# Upload parameters that require reset
1016-
reset_happened, already_uploaded_params = self.upload_parameters_that_require_reset_workflow(
1017-
selected_params, ask_confirmation, show_error, progress_callback_for_reset, progress_callback_for_connection
1018-
)
1008+
logging_info(
1009+
_("Uploading %d selected %s parameters to flight controller..."),
1010+
len(selected_params),
1011+
self.current_file,
1012+
)
1013+
1014+
# Get progress callbacks from factories if provided
1015+
progress_callback_for_upload = get_upload_progress_callback() if get_upload_progress_callback else None
1016+
progress_callback_for_reset = get_reset_progress_callback() if get_reset_progress_callback else None
1017+
progress_callback_for_connection = get_connection_progress_callback() if get_connection_progress_callback else None
1018+
progress_callback_for_download = get_download_progress_callback() if get_download_progress_callback else None
1019+
# Upload parameters that require reset
1020+
reset_happened, already_uploaded_params = self.upload_parameters_that_require_reset_workflow(
1021+
selected_params, ask_confirmation, show_error, progress_callback_for_reset, progress_callback_for_connection
1022+
)
10191023

1020-
# If reset happened, fc_parameters cache was cleared during disconnect/reconnect
1021-
# Re-download parameters now so _upload_parameters_to_fc has valid cache for comparison
1022-
if reset_happened:
1023-
self.download_flight_controller_parameters(lambda: progress_callback_for_download)
1024+
# If reset happened, fc_parameters cache was cleared during disconnect/reconnect
1025+
# Re-download parameters now so _upload_parameters_to_fc has valid cache for comparison
1026+
if reset_happened:
1027+
self.download_flight_controller_parameters(lambda: progress_callback_for_download)
10241028

1025-
# Upload remaining parameters (excluding those already uploaded in reset workflow)
1026-
remaining_params = {k: v for k, v in selected_params.items() if k not in already_uploaded_params}
1027-
nr_changed = self._upload_parameters_to_fc(remaining_params, show_error, progress_callback_for_upload)
1029+
# Upload remaining parameters (excluding those already uploaded in reset workflow)
1030+
remaining_params = {k: v for k, v in selected_params.items() if k not in already_uploaded_params}
1031+
nr_changed = self._upload_parameters_to_fc(remaining_params, show_error, progress_callback_for_upload)
10281032

1029-
# Add count of already uploaded params to total changed count
1030-
nr_changed += len(already_uploaded_params)
1033+
# Add count of already uploaded params to total changed count
1034+
nr_changed += len(already_uploaded_params)
10311035

1032-
if reset_happened or nr_changed > 0:
1033-
self._at_least_one_changed = True
1036+
if reset_happened or nr_changed > 0:
1037+
self._at_least_one_changed = True
10341038

1035-
if self._at_least_one_changed:
1036-
# Re-download all parameters to validate
1037-
# Note: Passing the callback directly, not the factory, since we already got it
1038-
self.download_flight_controller_parameters(lambda: progress_callback_for_download)
1039-
param_upload_error = self._validate_uploaded_parameters(selected_params)
1039+
if self._at_least_one_changed:
1040+
# Re-download all parameters to validate
1041+
# Note: Passing the callback directly, not the factory, since we already got it
1042+
self.download_flight_controller_parameters(lambda: progress_callback_for_download)
1043+
param_upload_error = self._validate_uploaded_parameters(selected_params)
10401044

1041-
if param_upload_error:
1042-
if ask_retry_cancel(
1043-
_("Parameter upload error"),
1044-
_("Failed to upload the following parameters to the flight controller:\n")
1045-
+ f"{(', ').join(param_upload_error)}",
1046-
):
1047-
# Retry the entire workflow - pass the factories again, not the callbacks
1048-
self.upload_selected_params_workflow(
1049-
selected_params,
1050-
ask_confirmation,
1051-
ask_retry_cancel,
1052-
show_error,
1053-
get_upload_progress_callback,
1054-
get_reset_progress_callback,
1055-
get_download_progress_callback,
1045+
if param_upload_error:
1046+
should_retry = ask_retry_cancel(
1047+
_("Parameter upload error"),
1048+
_("Failed to upload the following parameters to the flight controller:\n")
1049+
+ f"{(', ').join(param_upload_error)}",
10561050
)
1057-
# If not retrying, continue without success message
1058-
else:
1059-
logging_info(_("All parameters uploaded to the flight controller successfully"))
1051+
if should_retry:
1052+
continue
1053+
# If not retrying, continue without success message
1054+
else:
1055+
logging_info(_("All parameters uploaded to the flight controller successfully"))
10601056

1061-
if self._should_export_fc_params_diff:
1062-
self._export_fc_params_missing_or_different()
1057+
if self._should_export_fc_params_diff:
1058+
self._export_fc_params_missing_or_different()
10631059

10641060
self._write_current_file()
10651061
self._at_least_one_changed = False

0 commit comments

Comments
 (0)