-
Notifications
You must be signed in to change notification settings - Fork 494
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
Fix the launching of Servo as a node component #2194
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #2194 +/- ##
==========================================
+ Coverage 50.52% 50.53% +0.02%
==========================================
Files 386 386
Lines 31736 31736
==========================================
+ Hits 16032 16036 +4
+ Misses 15704 15700 -4 ☔ View full report in Codecov by Sentry. |
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.
Instead of commenting things in/out, could we maybe introduce an actual launch parameter + some IfCondition
/ UnlessCondition
logic that toggles this more cleanly?
b1d4a17
to
1cd78ef
Compare
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
2dbe8d6
to
9be50e9
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.
Looks good. When are we ready to use component nodes by default? ;)
If it works, I'd be totally fine with the default param being true. @AndyZe up to you if you wanna tweak that before merging. |
I believe it is the default with this PR:
|
Woops, I got my logicals mixed up. Yes, you're right :) |
Unfortunately this change actually has a bug, my apologies for not actually testing it. Turns out a component node doesn't have the ability to accept a launch condition parameter... fun From a user on Discord, I was following this tutorial (https://moveit.picknik.ai/main/doc/examples/realtime_servo/realtime_servo_tutorial.html#launching-a-servo-node) and after running the command: I got an error:
Commenting out the condition argument in launch file at
|
Update: This actually works on Rolling, but not Humble, so I think we shouldn't do much. See ros2/launch_ros#294 for more details. |
Ok. I thought I had tested it before pushing |
Description
Now that this seems to work reliably, make it the default way. It should decrease latency.
I tested with the tutorial, like normal:
https://moveit.picknik.ai/main/doc/examples/realtime_servo/realtime_servo_tutorial.html