Check if constants should be replaced by user-configurable parameters

[rokusottervanger]

In a few places, there's magic numbers in the code. We may want to get rid of those. Some examples:

path_tracking_pid/include/path_tracking_pid/controller.hpp

Lines 22 to 24 in 1495813

	#define RADIUS_EPS 0.001 // Smallest relevant radius [m]
	#define VELOCITY_EPS 1e-3 // Neglegible velocity
	#define LONG_DURATION 31556926 // A year (ros::Duration cannot be inf)

path_tracking_pid/include/path_tracking_pid/path_tracking_pid_local_planner.hpp

Lines 24 to 25 in 1495813

	#define MAP_PARALLEL_THRESH 0.2
	constexpr double DT_MAX=1.5;

path_tracking_pid/include/path_tracking_pid/controller.hpp

Lines 330 to 338 in 1495813

	// Upper and lower saturation limits
	double upper_limit_ = 100.0;
	double lower_limit_ = -100.0;
	double ang_upper_limit_ = 100.0;
	double ang_lower_limit_ = -100.0;
	// Anti-windup term. Limits the absolute value of the integral term.
	double windup_limit_ = 1000.0;

[Timple]

#define RADIUS_EPS 0.001 // Smallest relevant radius [m]
#define VELOCITY_EPS 1e-3 // Neglegible velocity

These are meant for numerical comparisons. Like in c++ you cannot compare floats directly. But good practice is not to use EPS itself but choose a domain-specific smallest number. So I think these can stay without being parameterised.

#define LONG_DURATION 31556926 // A year (ros::Duration cannot be inf)

This is a workaround for INF missing in the message. For Robot domain I think we're good here.

I agree that all others can be parameterised.

---

Is it worth doing though? Without someone actually asking for this, it just makes the package more complex.

[lewie-donckers]

Just to be 100% sure we're talking about the same thing:

A magic value is an unnamed literal value which is directly used in the code. The examples you listed above are constants and that's fine. Maybe they can be replaced with variables, removed or given more descriptive names but they are not magic values.

Not a magic value:

double upper_limit_ = 100.0; 

Magic value:

      if (abs(new_nominal_x_vel) < 0.01)

[rokusottervanger]

I stand corrected. Nonetheless, at least some of these values should be made configurable for the user. Do you have a suggestion for an alternative issue title?

[lewie-donckers]

I stand corrected. Nonetheless, at least some of these values should be made configurable for the user. Do you have a suggestion for an alternative issue title?

"Replace constants with user configurable settings" of
"Check if constants should be replaced with user configurable settings"?