Codecov Report

Merging #1456 into master will decrease coverage by 2.35%.
The diff coverage is 2.43%.

@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
- Coverage   39.92%   37.57%   -2.36%     
==========================================
  Files         229      229              
  Lines       11729    11771      +42     
  Branches     5057     5073      +16     
==========================================
- Hits         4683     4423     -260     
- Misses       3689     4146     +457     
+ Partials     3357     3202     -155

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea42a7d...3ff7705. Read the comment docs.

This needs some thread synchronization. The kinematic parameters are used on a different thread than the reconfigure call back.

The simple fix would be to add a mutex to the KinematicParameters class that we take in on_parameter_event_callback and every function that accesses these parameters. However, that's not going to be very efficient; every parameter has an individual accessor method.

Instead this class should be refactored so clients can grab the set of parameters instead of individual parameters.

However, that's not going to be very efficient; every parameter has an individual accessor method.

Aren't the callbacks triggered by a ros thread spinning? If we're using single threaded executors (which we are), this is going to be already be thread safe, right?

The callbacks run in the context of the ros spin function, but the actions runs on their own thread. So, in this case, all the FollowPath logic is on it's own thread, independent of the callbacks.

Here's where the thread is kicked off:
https://github.com/ros-planning/navigation2/blob/124663a8b7def5cad403a97223c0483bb6173877/nav2_util/include/nav2_util/simple_action_server.hpp#L134

All our action servers run this way, so it is something we need to watch out for mostly everywhere.

Got it. Another option is to just make all the variables atomic. I know atomic doubles don't "technically" exist, but the template will work fine for our purposes because we're not using the operators on the values, they're just being stored and retrieved.

Yeah. You're right. Atomics would be a better option than taking the mutex everywhere.

I'm not sure where the cutoff lies between the cost of accessing many atomics and the cost of taking one mutex. I suspect we're approaching that in this class though, so I think refactoring the interface makes sense in the long run.

Just making them all atomic though, would be a good short term fix and may be good enough that we never need to come back to the long run fix :-)

https://developernote.com/2017/06/comparison-stdmutex-and-stdatomic-performance/

42x, it appears from that dummy example. I'm actually a little surprised to see much overhead at all from just reading, I thought atomics were programmed to check for a condition that would make it near-zero overhead when something wasn't actively trying to write.

I believe they do memory fence instructions to force writes all the way out to cache and reads to refresh from cache, at least on x86

This is going to impose some overhead and it might vary depending on how much data is in the write buffers and, ... well, I'm a bit fuzzy on exactly what a read fence does. :-)

Hi! I really like this feature, it was exactly what I needed for my project :-)

The functionality works as expected, the parameters are changed in runtime and the velocity commands from the controller are changed. What I am having trouble with is that the navigation seems to fail a few seconds (or shorter) after I have changed the parameters. I have been testing this by simulating the Turtlebot multiple times, and it happens every time. A very short time after I reduce the maximum velocity, the recovery service hits and the robot is not able to recover. When running without recovery service I get the error: [controller_server-4] [ERROR] [controller_server]: Failed to make progress.

By looking at the code I get the impression that this error is printed if the robot has not moved long enough in a given time (default: 0.5m in 10 seconds). The time between changing the parameters and the robot stops moving is not 10 seconds, so I'm not sure if this is the correct source of the problem.

This does not happen if the robot is standing still when I am changing the parameters (only during navigation).

My default parameters are: [0.26, 0.0, 1.0, 0.26] (max_vel_x, max_vel_y, max_vel_theta, max_vel_xy). During runtime I am changing them to: [0.1, 0.0, 0.5, 0.1]

Have you experienced any similar behaviour while testing?