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

Servo Node - pause service: check if request is different than current state. #3265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jelmerdw
Copy link
Contributor

Description

When using the servo_node from moveit_servo, the service call to /servo_node/pause_servo hangs for a while (sometimes even up to a minute on my machine) when requesting pause = false while the pause status was already false. This behavior can be experienced by running the demo:

ros2 launch moveit_servo demo_ros_api.launch.py

And calling the service:

ros2 service call /servo_node/pause_servo std_srvs/srv/SetBool '{data: false}'

After a while, the following error is logged:

[moveit_servo_demo_container.moveit.ros.move_group.collision_monitor]: Collision monitor could not be started

To avoid this behavior, I propose this change to first check whether the requested pause state is different compared to the current pause state. If not, the response will be returned with the message that nothing is changed since the requested pause stated was already active.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@Jelmerdw Jelmerdw changed the title Return if requested pause state is already active. Servo Node - pause service: check if request is different than current state. Jan 24, 2025
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 45.95%. Comparing base (b91e529) to head (38c2b66).

Files with missing lines Patch % Lines
moveit_ros/moveit_servo/src/servo_node.cpp 0.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3265      +/-   ##
==========================================
- Coverage   45.97%   45.95%   -0.01%     
==========================================
  Files         716      716              
  Lines       62394    62398       +4     
  Branches     7545     7543       -2     
==========================================
- Hits        28677    28668       -9     
- Misses      33551    33563      +12     
- Partials      166      167       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

This is a good change, but also noting that #3259 was recently merged which may help with this. Checking if you have this commit locally?

Besides that, just one small comment to reduce duplication.

{
RCLCPP_INFO(node_->get_logger(), "Requested pause state is already active.");
response->success = true;
response->message = "Nothing changed since requested pause state was already active.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would either leave this out, or reuse the logger message with a shared string variable.

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.

3 participants