Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change servo collision checking parameters to dynamically update #2183

Merged
merged 5 commits into from
May 22, 2023

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented May 17, 2023

In this PR, we change the MoveIt servo parameter listener to be a shared pointer so we can listen to servo parameters in the main servo class, as well as the servo calculations and collision checking classes.

This, in turn, allows for collision checking parameters to dynamically be updated!

To test, you can directly change parameters in the servo node as follows, for example:

ros2 param set /servo_server moveit_servo.self_collision_proximity_threshold 0.1
ros2 param set /servo_server moveit_servo.check_collisions False

Closes #2178

@sea-bass sea-bass requested a review from AndyZe May 17, 2023 13:37
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Patch coverage: 88.10% and no project coverage change.

Comparison is base (243b0b2) 50.54% compared to head (cff200b) 50.53%.

❗ Current head cff200b differs from pull request most recent head 4b9bf50. Consider uploading reports for the commit 4b9bf50 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2183      +/-   ##
==========================================
- Coverage   50.54%   50.53%   -0.00%     
==========================================
  Files         387      387              
  Lines       31719    31725       +6     
==========================================
+ Hits        16028    16029       +1     
- Misses      15691    15696       +5     
Impacted Files Coverage Δ
...oveit_servo/include/moveit_servo/collision_check.h 100.00% <ø> (ø)
moveit_ros/moveit_servo/src/collision_check.cpp 85.08% <85.30%> (-0.63%) ⬇️
moveit_ros/moveit_servo/src/servo.cpp 57.15% <100.00%> (+3.30%) ⬆️
moveit_ros/moveit_servo/src/servo_calcs.cpp 70.27% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went and looked at parameter updating and found this variable in the config that controls if it ever checks for updated parameters in the main loop: enable_parameter_update. Why was this done? Shouldn't we allow users to set any parameter? If we want to have this behavior, should the collision checking code also pay attention to this too?

@sea-bass
Copy link
Contributor Author

I went and looked at parameter updating and found this variable in the config that controls if it ever checks for updated parameters in the main loop: enable_parameter_update. Why was this done? Shouldn't we allow users to set any parameter? If we want to have this behavior, should the collision checking code also pay attention to this too?

The collision checking code should pay attention to it -- tagged you in a separate comment.

The parameter itself was done to copy how the admittance controller was set up: if this param is false, the servo loop will never update its internal parameters as a way of "locking" the system configuration. But if true, it will allow runtime changes.

In the big PR that switched servo to generate_parameter_library, the dynamic updates only happened in ServoCalcs, but not in CollisionChecking

@tylerjw tylerjw force-pushed the dynamic-servo-collision-params branch from d5bfdf0 to a5b1057 Compare May 19, 2023 16:35
@AndyZe
Copy link
Member

AndyZe commented May 21, 2023

I'd be happy to get rid of enable_parameter_update. It's not like somebody accidentally updates a ROS parameter. They usually know what they're doing ;)

@sea-bass sea-bass force-pushed the dynamic-servo-collision-params branch from a5b1057 to c04492a Compare May 22, 2023 13:23
@sea-bass sea-bass requested a review from AndyZe May 22, 2023 13:23
@sea-bass sea-bass force-pushed the dynamic-servo-collision-params branch from 0f17d59 to cff200b Compare May 22, 2023 15:13
@tylerjw tylerjw merged commit 2a5a80d into moveit:main May 22, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dynamically toggling collision checking in MoveIt Servo without stopping and restarting.
3 participants