-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: acceleration and transport layer #348
base: tier4/universe
Are you sure you want to change the base?
feat: acceleration and transport layer #348
Conversation
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
…g and transport layer Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Note: this PR will be kept as a draft until #337 it merged, at which point this PR will be reabsed |
…cceleration_and_transport_layer Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…rt_layer Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
The concatenated PR was merged, so after solving the conflicts and some recent changes, I am opening this PR 🙏 |
…es implementation Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…rt_layer Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
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.
Looking good! I left a few comments on readability/maintainability but that's all I think.
Is there a reason that x2(_gen2) is not covered?
if LaunchConfiguration("use_cuda_preprocessor").perform(context): | ||
concat_component = ComposableNode( | ||
package="autoware_cuda_pointcloud_preprocessor", | ||
plugin="autoware::cuda_pointcloud_preprocessor::CudaPointCloudConcatenateDataSynchronizerComponent", | ||
name="concatenate_data", | ||
remappings=[ | ||
("~/input/twist", "/sensing/vehicle_velocity_converter/twist_with_covariance"), | ||
("output", "concatenated/pointcloud"), | ||
("output/cuda", "concatenated/pointcloud/cuda"), | ||
], | ||
parameters=[concatenate_and_time_sync_node_param], | ||
# NOTE(knzo25): when using the cuda blackboard, this setting can not be made global | ||
# extra_arguments=[{"use_intra_process_comms": LaunchConfiguration("use_intra_process")}], | ||
) | ||
else: | ||
concat_component = ComposableNode( | ||
package="autoware_pointcloud_preprocessor", | ||
plugin="autoware::pointcloud_preprocessor::PointCloudConcatenateDataSynchronizerComponent", | ||
name="concatenate_data", | ||
remappings=[ | ||
("~/input/twist", "/sensing/vehicle_velocity_converter/twist_with_covariance"), | ||
("output", "concatenated/pointcloud"), | ||
], | ||
parameters=[concatenate_and_time_sync_node_param], | ||
extra_arguments=[{"use_intra_process_comms": LaunchConfiguration("use_intra_process")}], | ||
) |
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 two branches differ only in their package, plugin and extra_params. The rest of the code could be merged into one to prevent future contributors from only modifying the one they're concerned about
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.
This was already the case before this PR, but this launch file is spaghetti code, making it very hard to understand at a glance what is going on.
Especially with the kind of multiplexed code of CUDA/non-CUDA, I'd suggest factoring out into a few functions like these:
def make_cuda_pointcloud_preprocessor(distortion_corrector_params, ring_outlier_params, vehicle_info, mirror_info) -> ComposableNode:
...
def make_vehicle_cropbox_filter(vehicle_info) ->ComposableNode:
...
add_launch_arg("use_intra_process", "False", "use ROS 2 component container communication") | ||
add_launch_arg("lidar_container_name", "nebula_node_container") | ||
add_launch_arg("pointcloud_container_name", "pointcloud_container") | ||
add_launch_arg("use_pointcloud_container", "False") | ||
add_launch_arg("use_cuda_preprocessor", "False") | ||
add_launch_arg("ptp_profile", "1588v2") |
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.
lidar_container_name
, pointcloud_container_name
and use_pointcloud_container
could be confusing for users, how about for example container_name
and use_shared_container
, with the appropriate description=
?
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.
Also, are users always expected to use a shared container when using CUDA?
Description
This PR is part of a series of PRs that aim to accelerate the Sensing/Perception pipeline through an appropriate use of CUDA.
List of PRs:
To use these branches, the following additions to the
autoware.repos
are necessary:Depending on your machine and how many nodes are in a container, the following branch may also be required:
https://github.com/knzo25/launch_ros/tree/fix/load_composable_node
There seems to be a but in ROS where if you send too many services at once some will be lost and
ros_launch
can not handle that.Related links
Parent Issue:
How was this PR tested?
The sensing/perception pipeline was tested until centerpoint for TIER IV's taxi using the logging simulator.
Notes for reviewers
The main branch that I used for development is
feat/cuda_acceleration_and_transport_layer
.However, the changes were too big so I split the PRs. That being said, development, if any will still be on that branch (and then cherrypicked to the respective PRs), and the review changes will be cherrypicked into the development branch.
Interface changes
An additional topic is added to perform type negotiation:
Example:
input/pointcloud
->input/pointcloud
andinput/pointcloud/cuda
Effects on system behavior
Enabling this preprocessing in the launchers should provide a much reduced latency and cpu usage (at the cost of a higher GPU usage)