Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

param save: Add a blocking API for param saves to be used from shell. #21737

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

ThomasDebrunner
Copy link
Member

@ThomasDebrunner ThomasDebrunner commented Jun 19, 2023

Solved Problem

After the latest param rework, the file lock in param_save_default was changed to a trylock, to prevent any blocking in the param system. This makes param_save_default occasionally fail, if it is called from multiple threads at the same time. This is correct, but unexpected especially when used from the shell.

Solution

This changes the param_save_default function to have be either blocking or non-blocking.

From the shell and from modules that expect a synchronous API, the blocking version is called. From the work queue, the non blocking version is called.

This also modifies the autosave worker, to re-schedule writing up to three times, in case writes fail.

Changelog Entry

For release notes:

  • Bugfix: Blocking API for param save

Alternatives

As an alternative, we could disable the autosave worker in the CI tests. This would solve the error in the CI tests. But still, a user typing in param save could still hit it then.

Test coverage

  • All unit tests still pass
  • Bench test on v5x

@mrpollo
Copy link
Contributor

mrpollo commented Jun 19, 2023

Hey @ThomasDebrunner thanks for the PR. Would you mind fixing the style check test?

@ThomasDebrunner
Copy link
Member Author

Fixed style

src/lib/parameters/param.h Show resolved Hide resolved
src/lib/parameters/parameters.cpp Outdated Show resolved Hide resolved
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

CI failure is unrelated, something with the Micro-XRCE-DDS-Client update.

 FAILED: src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-stamp/libmicroxrceddsclient_project-configure 
cd /__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-build && /usr/bin/cmake -P /__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-stamp/libmicroxrceddsclient_project-configure-RelWithDebInfo.cmake && /usr/bin/cmake -E touch /__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-stamp/libmicroxrceddsclient_project-configure
CMake Error at /__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-stamp/libmicroxrceddsclient_project-configure-RelWithDebInfo.cmake:16 (message):
  Command failed: 1

   '/usr/bin/cmake' '-C/__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/tmp/libmicroxrceddsclient_project-cache-RelWithDebInfo.cmake' '-GNinja' '/__w/PX4-Autopilot/PX4-Autopilot/src/modules/uxrce_dds_client/Micro-XRCE-DDS-Client'

  See also

    /__w/PX4-Autopilot/PX4-Autopilot/build/emlid_navio2_default/src/modules/uxrce_dds_client/src/libmicroxrceddsclient_project-stamp/libmicroxrceddsclient_project-configure-*.log

EDIT: the cmake version in the container px4io/px4-dev-armhf is too old:

CMake Error at CMakeLists.txt:18 (cmake_minimum_required):
  CMake 3.16 or higher is required.  You are running version 3.13.4

@bkueng bkueng merged commit fd267fb into main Jun 21, 2023
78 of 84 checks passed
@bkueng bkueng deleted the param-autosave-lock branch June 21, 2023 06:19
@junwoo091400 junwoo091400 added the Parameter System Parameter system of PX4 label Jun 26, 2023
MaEtUgR added a commit that referenced this pull request Jun 27, 2023
This solves the CI problem related to the required cmake version
described here:
#21737 (review)

See PX4/PX4-containers#332
for the container changes.
MaEtUgR added a commit that referenced this pull request Jun 27, 2023
This solves the CI problem related to the required cmake version
described here:
#21737 (review)

See PX4/PX4-containers#332
for the container changes.
harrisondragoon pushed a commit to harrisondragoon/PX4-Autopilot that referenced this pull request Jun 30, 2023
This solves the CI problem related to the required cmake version
described here:
PX4#21737 (review)

See PX4/PX4-containers#332
for the container changes.
antbre pushed a commit to BioMorphic-Intelligence-Lab/PX4-Autopilot that referenced this pull request Sep 14, 2023
This solves the CI problem related to the required cmake version
described here:
PX4#21737 (review)

See PX4/PX4-containers#332
for the container changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Parameter System Parameter system of PX4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants