-
Notifications
You must be signed in to change notification settings - Fork 266
[Fix] Cleanup Launch Files #982
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
base: master
Are you sure you want to change the base?
[Fix] Cleanup Launch Files #982
Conversation
|
I will be away for the next week, so I apologize if I do not immediately respond to comments. |
christophfroehlich
left a comment
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.
Jobs are failing, can you have a look please?
|
This pull request is in conflict. Could you fix it @philipchurchley? |
christophfroehlich
left a comment
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.
Thanks a lot for the big changes.
My first review round below. Please also fix the merge conflict with example_5 now.
example_10/bringup/launch/test_forward_position_controller.launch.py
Outdated
Show resolved
Hide resolved
…nch.py Co-authored-by: Christoph Fröhlich <[email protected]>
|
I'm not sure what's happening with the wrench transformer errors because that seems to have been added with the merge commit. I resolved the merge conflicts by just accepting my branches version and then I added the new launch argument to the launch file. However, looking at the diff for commit 74a8f6c, the merge commit also seems to have added other stuff that may be the cause for these failing tests. |
@philipchurchley, the test failures were due to the new wrentch_transformer wasn't launched. Added my suggestion. |
…wrench transformer node Co-authored-by: Julia Jia <[email protected]>
christophfroehlich
left a comment
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.
Except for failing pre-commit jobs this is ready for review?
Yes, although I'm not sure what the failing jobs are for. Could it have to do with the missing EventHandlers? |
@philipchurchley, for the failed pre-commit jobs, run following commnd in your dev env. If you don't have fix any issues reported and commit the changes. |
binary builds are failing because this wrench transformer node was released but not synced yet. |
|
just curious, is there a way for the controller manager to load controllers in the correct order other than event handlers? (for chainable controllers) |
|
what do you mean by correct order? If they are part of a chain, simply load them from the same spawner. The system will figure out the hierarchy. |
|
oh cool, didn't know that was possible! :) thx |
Addresses #950 and follows up on #959 by cleaning the remaining launch files. The launch files have been improved to use a declarative programming style, as described by @emersonknapp at ROSCon, which can be viewed in the livestream at 6:10.
Summary
LaunchDescription([...]), replacedPathJoinSubstitutionwithPathSubstitution+/,used literal
"xacro"inCommand, and removedRegisterEventHandler/OnProcessExit).Validation performed
generate_launch_description()for each launch) — all passed.master; it flagged 18 files but most differences are cosmetic or expected.Potential confusions for reviewer attention
example_15: addedpublisher_configlaunch argument in test launches. Please confirm the default file and behavior.RegisterEventHandlerentries may affect startup/shutdown ordering for some bringup launches. Please check that this removal doesn't break anything.Recommendation