Skip to content
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

chore(e2e_launch): add launch_sensing_driver arg #1095

Merged

Conversation

TakaHoribe
Copy link
Contributor

@TakaHoribe TakaHoribe commented Jul 29, 2024

Description

To launch a sensing driver (e.g. Nebula) via argument control, I added launch_sensing_driver arg in the launch file.

Tests performed

Added a log message in the autoware.launch.xml, and confirmed the argument control works fine.
image

I also tested with AWSIM to ensure the sensor driver launched correctly. Since the test was conducted locally, I will omit the details here.

Effects on system behavior

None since the default parameter is not changed.

Interface changes

launch_sensing_driver variable can be controlled via CLI like below:

ros2 launch autoware_launch e2e_simulator.launch.xml launch_sensing_driver:=true 

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:simulation Virtual environment setups and simulations. (auto-assigned) label Jul 29, 2024
@TakaHoribe TakaHoribe requested review from knzo25 and drwnz July 29, 2024 02:39
@knzo25
Copy link
Contributor

knzo25 commented Jul 31, 2024

@TakaHoribe
nit:
In the launcher I see two conventions. I think we are migrating to launch_.... right?

<arg name="launch_sensing" value="$(var sensing)"/>
...
<arg name="launch_sensing_driver" value="$(var launch_sensing_driver)"/>

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
(I left a nit, but I am not sure if it should be addressed or not)

@xmfcx
Copy link
Contributor

xmfcx commented Jul 31, 2024

brave_LJah5PAW5D

I think we should rename these variables to have launch_ prefix later on to express the intent more directly. I think @TakaHoribe 's way is alright.

@TakaHoribe
Copy link
Contributor Author

@knzo25 @xmfcx Thank you for your reviews! As you guys say, sensing, perception... should be renamed by launch_sensing.

@TakaHoribe TakaHoribe merged commit 80cc472 into autowarefoundation:main Aug 22, 2024
17 checks passed
@TakaHoribe TakaHoribe deleted the add-launch-sensor-driver-arg branch August 22, 2024 05:03
kyoichi-sugahara pushed a commit to tier4/autoware_launch that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:simulation Virtual environment setups and simulations. (auto-assigned) tag:run-build-and-test-differential
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants