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.

[bmagyar]

Started a "tracking issue" for this ;)

https://github.com/PickNikRobotics/generate_parameter_library/issues/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[bot]]

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 :warning:
...ory_controller/src/joint_trajectory_controller.cpp	0.00%	0 Missing and 1 partial :warning:
tricycle_controller/src/tricycle_controller.cpp	0.00%	0 Missing and 1 partial :warning:

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%)	:arrow_up:

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%)	:arrow_up:
...roadcaster/src/force_torque_sensor_broadcaster.cpp	92.50% <100.00%> (+1.59%)	:arrow_up:
pid_controller/src/pid_controller.cpp	69.91% <100.00%> (-0.51%)	:arrow_down:
...iff_drive_controller/src/diff_drive_controller.cpp	83.11% <0.00%> (+0.26%)	:arrow_up:
...ory_controller/src/joint_trajectory_controller.cpp	86.80% <0.00%> (-0.02%)	:arrow_down:
tricycle_controller/src/tricycle_controller.cpp	67.08% <0.00%> (+0.28%)	:arrow_up:

:rocket: New features to boost your workflow:

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

[github-actions[bot]]

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.

https://github.com/PickNikRobotics/generate_parameter_library/pull/260

This should fix it

[github-actions[bot]]

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.