Use ParamListener::try_get_params to Avoid Blocking in Real-Time Contexts

[KentaKato]

Due to the potential blocking risk associated with ParamListener::get_params, it is advisable to avoid its usage in real-time contexts. The method, defined here, can lead to delays that are unsuitable for time-sensitive operations.

As an alternative, I have switched to using ParamListener::try_get_params, which does not carry the risk of blocking. This non-blocking approach ensures better performance in real-time scenarios. You can find the implementation here.

[saikishor]

@KentaKato Thank you for propagating these nice changes into the ros2_controllers.

We would need to wait for the generate_parameter_library to be released in the distros before merging this.

Thank you

[bmagyar]

Started a "tracking issue" for this ;)

PickNikRobotics/generate_parameter_library#209

[KentaKato]

@saikishor @bmagyar Thank you!

[christophfroehlich]

GPL got released, but something is very strange now with the JTC tests in the CI. Severals jobs fail with a segfault, and others have errors like test_success_multi_point_with_velocity_sendgoal in semi-binary (rolling, main). I ran the tests locally, here

5: [  FAILED  ] OnlyEffortTrajectoryControllers/TestTrajectoryActionsTestParameterized.test_success_single_point_with_velocity_sendgoal/0, where GetParam() = ({ "effort" }, { "position", "velocity" }) (2026 ms)

@KentaKato could you have a look please?

[KentaKato]

@christophfroehlich

Although I haven't been able to reproduce this in my environment and haven't had time to thoroughly investigate it yet, I think the following logs are abnormal:

2024-10-29T21:07:04.7515029Z 5: [ERROR] [1730235957.984823612] [tolerances]: State tolerances failed for joint 0:
2024-10-29T21:07:04.7515356Z 5: [ERROR] [1730235957.984917837] [tolerances]: Position Error: 0.000000, Position Tolerance: 0.000000
2024-10-29T21:07:04.7515679Z 5: [ERROR] [1730235957.984965445] [tolerances]: Velocity Error: 0.015223, Velocity Tolerance: 0.000000
2024-10-29T21:07:04.7516030Z 5: [WARN] [1730235957.984997345] [test_joint_trajectory_controller]: Aborted due to state tolerance violation

This is because, according to tolerances.hpp (link), tolerance errors should only be evaluated when state_tolerance > 0.0.

I'll look into this further from tomorrow onward.

[bmagyar]

There was one set of tests passing on the previous run of the CI so I'm suspecting flakiness, I poked the CI again, let's see the results...

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.43%. Comparing base (ae12984) to head (893e493).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...iff_drive_controller/src/diff_drive_controller.cpp	0.00%	0 Missing and 1 partial ⚠️
...ory_controller/src/joint_trajectory_controller.cpp	0.00%	0 Missing and 1 partial ⚠️
tricycle_controller/src/tricycle_controller.cpp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
+ Coverage   86.39%   86.43%   +0.03%     
==========================================
  Files         123      123              
  Lines       12239    12230       -9     
  Branches     1023     1020       -3     
==========================================
- Hits        10574    10571       -3     
+ Misses       1348     1344       -4     
+ Partials      317      315       -2     

Flag	Coverage Δ
unittests	86.43% <57.14%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ude/admittance_controller/admittance_rule_impl.hpp	93.51% <100.00%> (+0.89%)	⬆️
...roadcaster/src/force_torque_sensor_broadcaster.cpp	92.50% <100.00%> (+1.59%)	⬆️
pid_controller/src/pid_controller.cpp	69.91% <100.00%> (-0.51%)	⬇️
...iff_drive_controller/src/diff_drive_controller.cpp	83.11% <0.00%> (+0.26%)	⬆️
...ory_controller/src/joint_trajectory_controller.cpp	86.80% <0.00%> (-0.02%)	⬇️
tricycle_controller/src/tricycle_controller.cpp	67.08% <0.00%> (+0.28%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[christophfroehlich]

I had another quick look: For me it seems that the API of try_get_params is not suitable here because it also returns true even if the parameters haven't changed -> stuff is updated at every update call.

    bool try_get_params(Params & params_in) const {
      if (mutex_.try_lock()) {
        if (const bool is_old = params_in.__stamp != params_.__stamp; is_old) {
          params_in = params_;
        }
        mutex_.unlock();
        return true;
      }
      return false;
    }

This results in errors when I build this locally, and it might be that this is also the reason for the strange CI errors.

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[bmagyar]

@christophfroehlich do we have a verdict on this?

[christophfroehlich]

there was no update anymore, I'm not sure if this is needed at all

[saikishor]

I had another quick look: For me it seems that the API of try_get_params is not suitable here because it also returns true even if the parameters haven't changed -> stuff is updated at every update call.

    bool try_get_params(Params & params_in) const {
      if (mutex_.try_lock()) {
        if (const bool is_old = params_in.__stamp != params_.__stamp; is_old) {
          params_in = params_;
        }
        mutex_.unlock();
        return true;
      }
      return false;
    }

This results in errors when I build this locally, and it might be that this is also the reason for the strange CI errors.

PickNikRobotics/generate_parameter_library#260

This should fix it

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[christophfroehlich]

As the upstream changes to GPL got released recently, we can go on with this PR. I updated all other occurrences as well, but kept the try_get_params without evaluating its return value where it is not necessary.

kilted-testing is fine, but something broke on rolling-testing triggering the build to abort.