-
Notifications
You must be signed in to change notification settings - Fork 272
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 spawner and unspawner scripts #310
Add spawner and unspawner scripts #310
Conversation
This works better if you have #309 to detect when a command failed. And also ros2/ros2cli#590 for the param load functionality. |
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.
Request for 2 small changes.
Were spawner and unspawner nodes in ros1. Does it make sense to have them as nodes or just as scripts as you implemented?
That's up for debate. I started as a node, but I would need to duplicate much code from ros2controlcli and from the ros2param load. I opened ros2/rclpy#671 and went to work as a script to above duplication. If we want to add more features like listening to a topic as in ROS1, it makes more sense as a node, hopefully there's a way of avoiding duplicating everything. |
I'd say go with the current version so we have something, and later when we can avoid code duplication, we rewrite it to use services directly. One thing we could do now as a middleground solution, is implement it as a Node that calls the subprocess. If we want launch files to remain unchanged, we should use a Node right now, even if it's implemented as a script. |
edf3a1e
to
9fb19de
Compare
OK, if you correct flake8 I agree :) |
Damn it, yeah I will. Let me change it to node first (albeit using subprocess), I'll do it Friday morning. |
I will, sorry about that. Do you know why my colcon test and ament_flake8 won't detect that stuff?
|
b45c374
to
ddb3809
Compare
I'm happy to merge this if you pinky promise a follow-up PR with some tests? |
I will add the tests, but we may want to hold the merge until ros2/ros2cli#590 is passed. Otherwise the part about loading parameters is going to fail because |
Is that going into Foxy or only Galactic? |
I'm gonna request for a cherry-pick to foxy since it's 100% backwards compatible and useful. |
0f35b92
to
fdbc23e
Compare
I added the tests but they seem to fail on the buildfarm, I'll check them next week. |
bcf2240
to
6564241
Compare
Could you add an example of using spawners in launch files? Maybe on |
4b4ea95
to
57007d6
Compare
I'll add the examples. Do you have any idea why is it failing? Also I expect that they will fail until ros2/ros2cli#596 is accepted, but they should fail differently complaining with:
|
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.
I like this! Very nice work! Here few comments/proposals.
@Karsten1987 This is the PR we were talking about. Waiting on ros2cli to be released. |
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.
I have two requests for changes here:
- Replace the python subprocess calls with actual ROS2 python API. Most of the calls can easily be replaced by a service call. That saves error prone command line output parsing.
- I'd love to see this functionality refactored into some sort of spawner API. The reason for this is that I believe we should leverage this quite a bit within the ros2 control CLI, i.e. loading a controller or unloading it.
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
fixes ros-controls#320 Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
e0d00a9
to
32d8933
Compare
I have rebased the branch to resolve the conflict. We are still blocked waiting for a release of ros2/ros2cli#596 , Asked for a release here: ros2/ros2cli#612 |
Fixes #294