-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add standalone executable for Servo server node #621
Conversation
There was a problem hiding this 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.
rclcpp::NodeOptions options; | ||
options.automatically_declare_parameters_from_overrides(true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ 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.
|
@schornakj do you have a launch file to test with? My simple modification of |
I'll add a standalone launch file for this equivalent to the one we provide for the Servo component node. |
There was a problem hiding this 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.
Agree that the naming is starting to be confusing. |
I created a PR because I feel the same way: #649 |
7a65d7e
to
1791baf
Compare
There was a problem hiding this 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
@schornakj I updated the example launch file to:
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. |
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