Check for i_min <= i_max at initialization #139
Conversation
test/pid_tests.cpp
Outdated
| @@ -43,23 +43,11 @@ TEST(ParameterTest, ITermBadIBoundsTest) | |||
| { | |||
| RecordProperty( | |||
| "description", | |||
| "This test checks that the integral contribution is robust to bad i_bounds specification (i.e. " | |||
| "This test checks that the constructor throws an error for bad i_bounds specification (i.e. " | |||
There was a problem hiding this comment.
Well technically it's "setGains" that throws. Can we trigger a throw with dyn reconf?
There was a problem hiding this comment.
never used dynamic reconfigure up to now, I'll see if I can write a test for that.
There was a problem hiding this comment.
@bmagyar I checked now how dynamic reconf works with the ROS-PID. So you mean if it is a good idea to throw an error in this case? I'm not sure how this is used in projects.. we could just give a RCLCPP_WARN message and forget the new parameters, but throw an error only if the constructor is called with i_min>i_max?
There was a problem hiding this comment.
@bmagyar I think we have to this in the controller. here we can only document the behavior
src/pid.cpp
Outdated
| @@ -126,6 +126,9 @@ void Pid::setGains(double p, double i, double d, double i_max, double i_min, boo | |||
|
|
|||
| void Pid::setGains(const Gains & gains) | |||
| { | |||
| if (gains.i_min_ > gains.i_max_) { | |||
| throw std::invalid_argument("received i_min > i_max"); | |||
There was a problem hiding this comment.
IMHO, it would be better to add ERROR here and disable PID then throw exception. What if we have running system and start new controller with bad parameter – then the whole drive crashes. If we disable movement of something would be better, wouldn't be?
Or should we catch this in controller manager, to prevent HW drivers and other controllers to crash?
There was a problem hiding this comment.
with ERROR, you mean an output on std::cerr?
"Disable movement" is not straight forward, isn't it? It could be in some configuration where zero torque would cause the robot to crash -> we cannot solve all failure cases from this generic controller.
I think for the initial setup of the controllers it is safe to "just" throw an exception (nothing is moving, we are in a safe configuration)? In the case of online reconfiguration, we also could just drop the new gains, give an error message, and work with the previous ones.
There was a problem hiding this comment.
That's true. I mean RCLCPP_ERROR, but we don't need to change anything. You are right, this is a good solution.
Can you also add to docs in the header file information when this error is thrown.
There was a problem hiding this comment.
I changed it now, so that the exception is only thrown if the base class is constructed with bad parameters. Otherwise, the new gains are skipped and nothing more happens.
…mater * add checks for all init/setGain functions * user RCLCPP_ERROR for ROS-node * update documentation in header files
Closes #102