diff_drive_controller: set parameters as read_only#2293
diff_drive_controller: set parameters as read_only#2293christophfroehlich merged 7 commits intoros-controls:masterfrom
Conversation
Add read_only: true to all 36 parameters in diff_drive_controller for consistency with other drive controllers. Fixes ros-controls#1696
christophfroehlich
left a comment
There was a problem hiding this comment.
Please evaluate which parameters make sense to be reconfigured at runtime, see my comment here
#2074 (review)
|
I evaluated which parameters make sense to be reconfigured at runtime and identified 19 parameters that can be made dynamic:
Direct from params_ (auto-updates):
SpeedLimiter set_params() updates:
Implementation PlanI will handle the runtime updates in
|
|
Sounds good. Only, I'm not sure about |
…arameters Add set_params() method to SpeedLimiter class for updating parameters at runtime. Add try_update_params() in RT loop to detect and apply parameter changes on-the-fly. Dynamic parameters: - enable_odom_tf, cmd_vel_timeout, publish_limited_velocity - linear.x.* (8 params) - angular.z.* (8 params) Fixes ros-controls#1696
|
@christophfroehlich Added parameter handling for runtime updates. Could you review? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2293 +/- ##
==========================================
+ Coverage 84.99% 85.01% +0.02%
==========================================
Files 145 145
Lines 15186 15234 +48
Branches 1331 1336 +5
==========================================
+ Hits 12907 12951 +44
- Misses 1789 1791 +2
- Partials 490 492 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christophfroehlich
left a comment
There was a problem hiding this comment.
looks fine for me, but could you please add a test for the parameter updates at runtime? have a look at existing tests with specified limits, copy it, and extend it with another update call with changed limits.
|
Sure, even I was thinking the same 😅 |
|
@christophfroehlich Added test for runtime parameter updates of speed limiter. Could you please review? |
christophfroehlich
left a comment
There was a problem hiding this comment.
Thank you! LGTM, I only have 2 nitpicks about readability.
| publish(linear, 0.0); | ||
| // wait for msg is be published to the system | ||
| controller_->wait_for_twist(executor); | ||
| const double time_dec = 1.0 / 4.0; |
There was a problem hiding this comment.
| const double time_dec = 1.0 / 4.0; | |
| const double time_dec = 1.0 / std::abs(max_deceleration); |
|
Hello @christophfroehlich , I’ve addressed the requested changes. Ready for review! |
christophfroehlich
left a comment
There was a problem hiding this comment.
perfect, thank you!
ede5b33
into
ros-controls:master
Add read_only: true to all 36 parameters in diff_drive_controller for consistency with other drive controllers.
Fixes #1696
Description
Right now I added read_only: true for all params because currently, the controller does not support runtime parameter updates for SpeedLimiter-related parameters. This would require adding try_update_params() to the RT loop and implementing a set_params() method in the SpeedLimiter class. If needed, I will address this in a follow-up PR.
Fixes #1696
Is this user-facing behavior change?
NO
Did you use Generative AI?
No
Additional Information
TODOs
To send us a pull request, please:
colcon testandpre-commit run(requires you to install pre-commit bypip3 install pre-commit)