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

Add standalone executable for Servo server node #621

Merged
merged 9 commits into from
Sep 14, 2021

Conversation

schornakj
Copy link
Contributor

@schornakj schornakj commented Aug 18, 2021

Description

Previously the servo_server node was only available as a component node. This adds a new executable to run the servo_server as a standalone node.

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

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.

Looks really good and useful. Thank you. Only thing I'd add is an example launch file for it but it is so similar to the component node I'm not sure that's worth it. I hope this is helpful in debugging the issues you've seen with component nodes.

Comment on lines +46 to +47
rclcpp::NodeOptions options;
options.automatically_declare_parameters_from_overrides(true);
Copy link
Member

Choose a reason for hiding this comment

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

In the future I hope we can get away from this by declaring all the parameters in a better way with nicer validation. I'm working on that.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I'm still fuzzy on the finer differences between component nodes and standalone nodes.

The PR looks good. I'll prob give it a thumbs up after testing.

@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #621 (eab5e3a) into main (09406c1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #621   +/-   ##
=======================================
  Coverage   54.38%   54.38%           
=======================================
  Files         191      191           
  Lines       20100    20100           
=======================================
  Hits        10929    10929           
  Misses       9171     9171           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09406c1...eab5e3a. Read the comment docs.

@AndyZe
Copy link
Member

AndyZe commented Aug 23, 2021

@schornakj do you have a launch file to test with? My simple modification of moveit2_tutorials/docs/realtime_servo/launch/servo_teleop.launch.py did not work. I'm guessing the parameters were loaded in the wrong namespace.

servo_teleop.launch.py.txt

@schornakj
Copy link
Contributor Author

@schornakj do you have a launch file to test with? My simple modification of moveit2_tutorials/docs/realtime_servo/launch/servo_teleop.launch.py did not work. I'm guessing the parameters were loaded in the wrong namespace.

servo_teleop.launch.py.txt

I'll add a standalone launch file for this equivalent to the one we provide for the Servo component node.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Looks good to me. In general, would it be bad practice to add a main function to source files of component nodes directly? Say, add it to servo_server.cpp below registering the component node, or alternatively add the RCLCPP_COMPONENTS_REGISTER_NODE macro to servo_server_node.cpp? Also, the source file naming is really confusing, we have servo.cpp (main servo class), servo_server.cpp (node implementation) and servo_server_node.cpp which is the now added main function to run ServoServer as node executable. Out of scope of this PR, but we should clear that up.

@AndyZe
Copy link
Member

AndyZe commented Aug 24, 2021

Agree that the naming is starting to be confusing.

@tylerjw
Copy link
Member

tylerjw commented Aug 30, 2021

Agree that the naming is starting to be confusing.

I created a PR because I feel the same way: #649

@AndyZe AndyZe mentioned this pull request Aug 31, 2021
4 tasks
@schornakj schornakj force-pushed the feature/servo-server-standalone-node branch from 7a65d7e to 1791baf Compare September 7, 2021 20:56
Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

blocking merge until I can test it

Galactic/Rolling - 2.3.0 - September 10 automation moved this from In Progress to Review in progress Sep 14, 2021
@AndyZe
Copy link
Member

AndyZe commented Sep 14, 2021

@schornakj I updated the example launch file to:

  • launch RViz
  • include a commented example of launching Servo as a component, so that documentation is all in one place

I think the tutorial can now use this launch file rather than a completely separate one 👍

Let me know if that bugs you for some reason and we can revert the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants