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

fix(raw_vehicle_cmd_converter): fix parameter files to parse path to csv files #6136

Merged

Conversation

rej55
Copy link
Contributor

@rej55 rej55 commented Jan 22, 2024

Description

According to #5134, some parameters are moved into yaml files.
However, if we launch raw_vehicle_cmd_converter from autoware_launch, some parameters defined in launch files does not appear.

In this change, I fixed above problems.

accel/brake/steer map path should change at launch, so I removed csv_path_**_map from yaml file and add them into xml file.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

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 component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) component:launch Launch files, scripts and initialization tools. (auto-assigned) labels Jan 22, 2024
@rej55 rej55 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 22, 2024
@rej55 rej55 requested a review from PhoebeWu21 January 22, 2024 08:16
@rej55
Copy link
Contributor Author

rej55 commented Jan 22, 2024

@PhoebeWu21
Hello,

I changed the parameter file in raw_vehicle_cmd_converter to change path to csv files dynamically.
Do you have any comments for this change?

Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

@PhoebeWu21
Copy link
Member

@PhoebeWu21 Hello,

I changed the parameter file in raw_vehicle_cmd_converter to change path to csv files dynamically. Do you have any comments for this change?

Looks good to me.
But, @ambroise-arm I'm not sure if this is fine in ROS Node Configuration layout rule.

@ambroise-arm
Copy link
Contributor

@PhoebeWu21 All parameters defined in the C++ code should be listed in the schema and parameter files according to the ROS Node Configuration page. If we want both that and being able to pass the parameters from external launch file, I think this leads to the same discussion about parameter override as in #5773. I don't think there was a clear conclusion on what should be done.

@rej55
Copy link
Contributor Author

rej55 commented Jan 22, 2024

@PhoebeWu21 @ambroise-arm Thank you for your comments!
We want to fix the problem for parameter overriding for now, is it okay to merge this PR?
Let us discuss the problem again separately.

Copy link
Contributor

@TakaHoribe TakaHoribe left a comment

Choose a reason for hiding this comment

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

This PR looks good to me to fix the current problem. However, we need to discuss how to support both the json schema and passing arguments from launch as in the comment from @ambroise-arm .

@rej55 Would you make an issue for this?

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (56dcff5) 14.60% compared to head (bfadc87) 14.60%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6136   +/-   ##
=======================================
  Coverage   14.60%   14.60%           
=======================================
  Files        1860     1860           
  Lines      127418   127418           
  Branches    37286    37286           
=======================================
  Hits        18606    18606           
  Misses      87903    87903           
  Partials    20909    20909           
Flag Coverage Δ *Carryforward flag
differential 10.52% <ø> (?)
total 14.60% <ø> (ø) Carriedforward from 56dcff5

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rej55
Copy link
Contributor Author

rej55 commented Jan 23, 2024

It seems the same issue is discussed in autowarefoundation/autoware-github-actions#232

@rej55 rej55 merged commit 9cda3b3 into autowarefoundation:main Jan 23, 2024
34 of 36 checks passed
@rej55 rej55 deleted the fix/raw_vehicle_cmd_converter_param branch January 23, 2024 02:51
rej55 added a commit to tier4/autoware.universe that referenced this pull request Jan 23, 2024
kyoichi-sugahara pushed a commit to kyoichi-sugahara/autoware.universe that referenced this pull request Jan 23, 2024
h-ohta pushed a commit to tier4/autoware.universe that referenced this pull request Feb 9, 2024
0x126 pushed a commit to tier4/autoware.universe that referenced this pull request Feb 13, 2024
mergify bot pushed a commit to tier4/autoware.universe that referenced this pull request Apr 18, 2024
…csv files (autowarefoundation#6136) (#1139)

Signed-off-by: Fumiya Watanabe <[email protected]>
Co-authored-by: Fumiya Watanabe <[email protected]>
(cherry picked from commit 604848f)
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants