add read_only parameters to diff_drive_controller #1781
Conversation
github-actions
bot
requested review from
bijoua29,
bmagyar,
erickisos,
mhubii,
olivier-stasse,
rosterloh and
saikishor
There was a problem hiding this comment.
The parameters itself look good. But we don't update the parameters post on_configure. I propose changing this to a simple get_params() as on_configure is not in the RT thread
ros2_controllers/diff_drive_controller/src/diff_drive_controller.cpp
Lines 313 to 317 in 922e49b
but add this
try_update_params to the update_reference_from_subscribers or update_and_write_commands method.
Please review then also if we need local copies of the params_-struct members and either remove it or update them after try_update_params. (cmd_vel_timeout_, publish_limited_velocity_,..)
|
@adhithiyan110 do you need any input for going on with this PR? |
|
@christophfroehlich can you please review changes |
| // limit the publication on the topics /odom and /tf | ||
| publish_rate_ = params_.publish_rate; | ||
| publish_period_ = rclcpp::Duration::from_seconds(1.0 / publish_rate_); | ||
| const double wheel_separation = params_.wheel_separation_multiplier * params_.wheel_separation; | ||
| const double left_wheel_radius = params_.left_wheel_radius_multiplier * params_.wheel_radius; | ||
| const double right_wheel_radius = params_.right_wheel_radius_multiplier * params_.wheel_radius; | ||
|
|
||
| odometry_.setWheelParams(wheel_separation, left_wheel_radius, right_wheel_radius); | ||
| odometry_.setVelocityRollingWindowSize(static_cast<size_t>(params_.velocity_rolling_window_size)); |
There was a problem hiding this comment.
these parameters are read-only, better leave them in on_configure
| limiter_linear_ = std::make_unique<SpeedLimiter>( | ||
| params_.linear.x.min_velocity, params_.linear.x.max_velocity, | ||
| params_.linear.x.max_acceleration_reverse, params_.linear.x.max_acceleration, | ||
| params_.linear.x.max_deceleration, params_.linear.x.max_deceleration_reverse, | ||
| params_.linear.x.min_jerk, params_.linear.x.max_jerk); | ||
|
|
||
| limiter_angular_ = std::make_unique<SpeedLimiter>( | ||
| params_.angular.z.min_velocity, params_.angular.z.max_velocity, | ||
| params_.angular.z.max_acceleration_reverse, params_.angular.z.max_acceleration, | ||
| params_.angular.z.max_deceleration, params_.angular.z.max_deceleration_reverse, | ||
| params_.angular.z.min_jerk, params_.angular.z.max_jerk); |
There was a problem hiding this comment.
This method is part of the RT loop, we must not allocate memory here. Better leave the initial construction of the objects in the on_configure, but update the parameters (only if they changed, i.e., inside the if clause above).
To achieve this, we have to expose the control_toolbox::RateLimiter::set_params method in the diff_drive_controller::SpeedLimiter class.
https://github.com/ros-controls/control_toolbox/blob/c46e3c5b56d62c30e30e42d3658821fbf909dd3e/control_toolbox/include/control_toolbox/rate_limiter.hpp#L107-L132
Furthermore, we should change the rate_limiter class to throw the errors of the input checks before the member variables are set. So we can catch the errors in the diff_drive_controller::SpeedLimiter::set_params method, otherwise we have to reimplement the checks to avoid crashing the controller: ros-controls/control_toolbox#437
This PR adds the read_only attribute to key parameters of diff_drive_controller.Related to #1696