-
Notifications
You must be signed in to change notification settings - Fork 122
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
Arc 78 trajectory follower integration #2419
Arc 78 trajectory follower integration #2419
Conversation
trajectory_follower_wrapper/src/trajectory_follower_wrapper_node.cpp
Outdated
Show resolved
Hide resolved
...ry_follower_wrapper/include/trajectory_follower_wrapper/trajectory_follower_wrapper_node.hpp
Outdated
Show resolved
Hide resolved
...ry_follower_wrapper/include/trajectory_follower_wrapper/trajectory_follower_wrapper_node.hpp
Outdated
Show resolved
Hide resolved
..._follower_wrapper/include/trajectory_follower_wrapper/trajectory_follower_wrapper_config.hpp
Outdated
Show resolved
Hide resolved
carma/launch/guidance.launch.py
Outdated
trajectory_follower_container = ComposableNodeContainer( | ||
package='carma_ros2_utils', | ||
name='trajectory_follower_container', | ||
executable='carma_component_container_mt', | ||
namespace=GetCurrentNamespace(), | ||
composable_node_descriptions=[ | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LatLonMuxer', | ||
name='latlon_muxer_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': False}, | ||
{'--log-level' : GetLogLevel('latlon_muxer', env_log_levels) } | ||
], | ||
parameters=[ | ||
{'timeout_thr_sec':0.5} | ||
] | ||
), | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LateralController', | ||
name='lateral_controller_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': True}, | ||
{'--log-level' : GetLogLevel('lateral_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/lateral/control_cmd", "input/lateral/control_cmd") | ||
], | ||
parameters = [ | ||
[vehicle_calibration_dir, "/mpc_follower/lateral_controller_defaults.yaml"] | ||
] | ||
), | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LongitudinalController', | ||
name='longitudinal_controller_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': False}, | ||
{'--log-level' : GetLogLevel('longitudinal_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/longitudinal/control_cmd", "input/longitudinal/control_cmd"), | ||
("input/current_trajectory", "input/reference_trajectory"), | ||
("input/current_state", "input/current_kinematic_state") | ||
], | ||
parameters = [ | ||
[vehicle_calibration_dir, "/mpc_follower/longitudinal_controller_defaults.yaml"] | ||
] | ||
) | ||
] | ||
) | ||
|
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 think this might be better to be in the plugins.launch.py close to its "main" node trajectory_follower_wrapper
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 dont have a problem with that, but it seemed to me that the pluginns.launch.py has all the carma plugins that we developed. While external autoware nodes such as twist filter are in guidance.launch.py
What do you think?
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.
It just seems easier if all the related nodes were in one file. Not a blocker though.
carma/launch/guidance.launch.py
Outdated
{'--log-level' : GetLogLevel('lateral_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/lateral/control_cmd", "input/lateral/control_cmd") |
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 think it might be better if we drop the input
output
naming convention for the final name resulting from the mapping. for example, consider revising to:
("output/lateral/control_cmd", "lateral/control_cmd")
(above suggestion is assuming the final topic name includes the node's name if I remember correctly. I am imagining it should look like /guidance/lateral_controller_node/lateral/control_cmd
)
This is mainly because the name "input" would be confusing if we inspect this specific node (instead of the main trajectory_follower) because it is still output for this one.
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.
These are not topic names that I came up with. They are Autoware's nodes expected topics that are published by one, and consumed by another one. And I tried to minimize changes to Autoware nodes in case we want to update the package. Let me know what you think?
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 agree, the input/output naming here is a little confusing. As long as we only remapping the topic names in the launch, it should still be the same effort to update the package?
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.
@saina-ramyar yeah we don't need to change names in the autoware package itself. Just topic remapping for general naming here in the launch file would be good. Since we are already topic remapping from "output" to "input", which means every topic would end up being "input", why not drop it and rename it to something general.
"input/lateral/control_cmd" would be just sth like "lateral/control_cmd"
carma/launch/guidance.launch.py
Outdated
("output/longitudinal/control_cmd", "input/longitudinal/control_cmd"), | ||
("input/current_trajectory", "input/reference_trajectory"), | ||
("input/current_state", "input/current_kinematic_state") |
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.
Same with above comment:
("output/longitudinal/control_cmd", "longitudinal/control_cmd"),
("input/current_trajectory", "reference_trajectory"),
("input/current_state", "current_kinematic_state")
carma/launch/plugins.launch.py
Outdated
("input/current_kinematic_state", [ EnvironmentVariable('CARMA_GUIDE_NS', default_value=''),"/input/current_kinematic_state"]), | ||
("input/reference_trajectory", [ EnvironmentVariable('CARMA_GUIDE_NS', default_value=''),"/input/reference_trajectory"]), | ||
("output/control_cmd" , [ EnvironmentVariable('CARMA_GUIDE_NS', default_value=''),"/output/control_cmd"]), |
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.
If we follow comments on the guidance.launch.py, this would change to match as well
carma/launch/guidance.launch.py
Outdated
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LatLonMuxer', | ||
name='latlon_muxer_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': False}, | ||
{'--log-level' : GetLogLevel('latlon_muxer', env_log_levels) } | ||
], | ||
parameters=[ | ||
{'timeout_thr_sec':0.5} | ||
] |
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.
If this node also has some output, input topic in the name. it should probably be remappaed to drop those as mentioned in my later comments
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.
The output of this node are subscribed to by trajectory follower wrapper, and the remapping is implemented there
carma/launch/guidance.launch.py
Outdated
{'--log-level' : GetLogLevel('lateral_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/lateral/control_cmd", "input/lateral/control_cmd") |
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.
@saina-ramyar yeah we don't need to change names in the autoware package itself. Just topic remapping for general naming here in the launch file would be good. Since we are already topic remapping from "output" to "input", which means every topic would end up being "input", why not drop it and rename it to something general.
"input/lateral/control_cmd" would be just sth like "lateral/control_cmd"
carma/launch/guidance.launch.py
Outdated
trajectory_follower_container = ComposableNodeContainer( | ||
package='carma_ros2_utils', | ||
name='trajectory_follower_container', | ||
executable='carma_component_container_mt', | ||
namespace=GetCurrentNamespace(), | ||
composable_node_descriptions=[ | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LatLonMuxer', | ||
name='latlon_muxer_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': False}, | ||
{'--log-level' : GetLogLevel('latlon_muxer', env_log_levels) } | ||
], | ||
parameters=[ | ||
{'timeout_thr_sec':0.5} | ||
] | ||
), | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LateralController', | ||
name='lateral_controller_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': True}, | ||
{'--log-level' : GetLogLevel('lateral_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/lateral/control_cmd", "input/lateral/control_cmd") | ||
], | ||
parameters = [ | ||
[vehicle_calibration_dir, "/mpc_follower/lateral_controller_defaults.yaml"] | ||
] | ||
), | ||
ComposableNode( | ||
package='trajectory_follower_nodes', | ||
plugin='autoware::motion::control::trajectory_follower_nodes::LongitudinalController', | ||
name='longitudinal_controller_node', | ||
extra_arguments=[ | ||
{'use_intra_process_comms': False}, | ||
{'--log-level' : GetLogLevel('longitudinal_controller', env_log_levels) } | ||
], | ||
remappings = [ | ||
("output/longitudinal/control_cmd", "input/longitudinal/control_cmd"), | ||
("input/current_trajectory", "input/reference_trajectory"), | ||
("input/current_state", "input/current_kinematic_state") | ||
], | ||
parameters = [ | ||
[vehicle_calibration_dir, "/mpc_follower/longitudinal_controller_defaults.yaml"] | ||
] | ||
) | ||
] | ||
) | ||
|
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.
It just seems easier if all the related nodes were in one file. Not a blocker though.
|
Looks good to me. Good stuff. |
<!-- Thanks for the contribution, this is awesome. --> # PR Details ## Description This PR is part of the integration testing of the Trajectory Follower Wrapper on a physical vehicle. Related PR: usdot-fhwa-stol/carma-platform#2419 ## Related GitHub Issue <!--- This project only accepts pull requests related to open GitHub issues or Jira Keys --> <!--- If suggesting a new feature or change, please discuss it in an issue first --> <!--- If fixing a bug, there should be an issue describing it with steps to reproduce --> <!--- Please DO NOT name partially fixed issues, instead open an issue specific to this fix --> <!--- Please link to the issue here: --> ## Related Jira Key https://usdot-carma.atlassian.net/browse/ARC-78 ## Motivation and Context Integrate the Trajectory Follower controller of Autoware, that has a MPC based lateral controller, to improve vehicle's performance. ## How Has This Been Tested? Integration tested on Blue Lexus <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Defect fix (non-breaking change that fixes an issue) - [x] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that cause existing functionality to change) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] I have added any new packages to the sonar-scanner.properties file - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [ ] I have read the [**CONTRIBUTING**](https://github.com/usdot-fhwa-stol/carma-platform/blob/develop/Contributing.md) document. - [ ] I have added tests to cover my changes. - [x] All new and existing tests passed. --------- Co-authored-by: Misheel Bayartsengel <misheel.bayart@gmail.com>
PR Details
Description
The Trajectory Follower Wrapper is being tested on a physical vehicle.
related PRs:
usdot-fhwa-stol/autoware.auto#26
https://github.com/usdot-fhwa-stol/carma-vehicle-calibration/pull/128
Related GitHub Issue
Related Jira Key
(https://usdot-carma.atlassian.net/browse/ARC-78)
Motivation and Context
Integrate the Trajectory Follower controller of Autoware, that has a MPC based lateral controller, to improve vehicle's performance.
How Has This Been Tested?
This controller is tested on Blue Lexus.
Notes for testing:
Types of changes
Checklist: