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 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.54%. Comparing base (36068e1) to head (8a9cc3a).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
- Coverage   83.56%   83.54%   -0.02%     
==========================================
  Files         122      122              
  Lines       10992    10992              
  Branches      937      938       +1     
==========================================
- Hits         9185     9183       -2     
  Misses       1493     1493              
- Partials      314      316       +2     

Flag	Coverage Δ
unittests	83.54% <50.00%> (-0.02%)	⬇️

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

Files with missing lines	Coverage Δ
pid_controller/src/pid_controller.cpp	67.82% <100.00%> (-0.87%)	⬇️
...ory_controller/src/joint_trajectory_controller.cpp	83.88% <0.00%> (-0.03%)	⬇️

... and 1 file with indirect coverage changes

[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.

[christophfroehlich]

We can remove update_parameters() method and use just get_params() in the on_configure() method (not in the RT loop).