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

Max steer angle parameter is defined in each package, not in vehicle_info package #535

Closed
takayuki5168 opened this issue Mar 17, 2022 · 14 comments · Fixed by #740
Closed
Assignees
Labels
type:new-feature New functionalities or additions, feature requests.

Comments

@takayuki5168
Copy link
Contributor

Since max steer angle parameter is not in the vehicle_info package, this parameter is defined in each packages' yaml file.

@takayuki5168
Copy link
Contributor Author

cc @satoshi-ota

@takayuki5168
Copy link
Contributor Author

takayuki5168 commented Mar 17, 2022

Max jerk, acceleration, velocity are the same. (should be in vehicle_info package) in the common yaml, mentioned as follows.

@takayuki5168
Copy link
Contributor Author

takayuki5168 commented Mar 17, 2022

@satoshi-ota @kenji-miyake @TakaHoribe
Do you agree, and if you do, do you have any other parameters to include in the vehicle_info package.

@kenji-miyake
Copy link
Contributor

cc @yukkysaito @mitsudome-r @xmfcx
I agree that we should manage max_steer_angle as a global parameter.
For what else, I have no concrete ideas, but I feel they should be categorized like shape, hard_limit, soft_limit, etc.

Also, as a long-term issue, we should write a design document for global parameters and re-design some packages for Autoware Core.

k-obitsu pushed a commit to k-obitsu/autoware.universe that referenced this issue Mar 19, 2022
* Add autoware api (autowarefoundation#1979)

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Use EmergencyState instead of deprecated EmergencyMode (autowarefoundation#2030)

* Use EmergencyState instead of deprecated EmergencyMode

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Use stamped type

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* add sort-package-xml hook in pre-commit (autowarefoundation#1881)

* add sort xml hook in pre-commit

* change retval to exit_status

* rename

* add prettier plugin-xml

* use early return

* add license note

* add tier4 license

* restore prettier

* change license order

* move local hooks to public repo

* move prettier-xml to pre-commit-hooks-ros

* update version for bug-fix

* apply pre-commit

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Add selected external command API (autowarefoundation#2053)

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Feature/vehicle interface improvements (autowarefoundation#1361) (autowarefoundation#1688)

* Feature/vehicle interface improvements (autowarefoundation#1361)

* add vehicle msg

* add pacmod interface

* add eps controller

* use each control commands instead of vehicle command

* fixed details

* fixed speell check

* const

* fixed brake status

* publish cmd when recieving ctrl cmd

* fix shift cmd ptr

* remove unused function and set proper license

* fix names

* fix typo for pacmod

* remove unnecessary waiting

* use flags, limit, multiarray

* remove accel brake dependency

* fix retrun value

* replace eps to steer

* cosmetic change for namespace

* fix segfo and retval

* Use Enum instead of int

* remove unused var

* add const

* rename to calcFFMap

* prev time steer calc

* add sample csv

* add new line

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* Apply lint

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* Fix build failure for remote cmd converter

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* Fix lint

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* replace to vehicle cmd emergency (autowarefoundation#1710) (autowarefoundation#1717)

* Fix subscriber topic type

Co-authored-by: tkimura4 <tomoya.kimura@tier4.jp>

* Fix rclcpp::Time initialization

Co-authored-by: tkimura4 <tomoya.kimura@tier4.jp>

Co-authored-by: tkimura4 <tomoya.kimura@tier4.jp>
Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Change formatter to clang-format and black (autowarefoundation#2332)

* Revert "Temporarily comment out pre-commit hooks"

This reverts commit 748e9cdb145ce12f8b520bcbd97f5ff899fc28a3.

* Replace ament_lint_common with autoware_lint_common

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Remove ament_cmake_uncrustify and ament_clang_format

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Apply Black

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Apply clang-format

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Fix build errors

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Fix for cpplint

* Fix include double quotes to angle brackets

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Apply clang-format

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* Fix build errors

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Add COLCON_IGNORE (autowarefoundation#500)

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* [external_cmd_converter] apply autoware auto msgs (autowarefoundation#535)

* remove ignore

* apply auto msgs

* modify launch

* topic to shift command

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* move cmd converters to control pkg (autowarefoundation#642)

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* auto/revert cmd converter (autowarefoundation#680)

* Revert "move cmd converters to control pkg (autowarefoundation#642)"

This reverts commit aed827c4fdfa01cc7bc29e70307d5e2cbc8aa8d3.

* fix topic

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* fix shift to gear (autowarefoundation#683)

Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

* Auto/readme cmd converter (autowarefoundation#692)

* fix format

* add readme external cmd converter

* fix lint

* fiix sentence

Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>

* fix format

Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>

* fix sentence

Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>

* better expression

Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>

Co-authored-by: taikitanaka3 <taiki.tanaka@tier4.jp>
Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>
Signed-off-by: tanaka3 <ttatcoder@outlook.jp>

Co-authored-by: Takagi, Isamu <43976882+isamu-takagi@users.noreply.github.com>
Co-authored-by: Kenji Miyake <31987104+kenji-miyake@users.noreply.github.com>
Co-authored-by: Keisuke Shima <19993104+KeisukeShima@users.noreply.github.com>
Co-authored-by: Daisuke Nishimatsu <42202095+wep21@users.noreply.github.com>
Co-authored-by: tkimura4 <tomoya.kimura@tier4.jp>
Co-authored-by: taikitanaka3 <taiki.tanaka@tier4.jp>
Co-authored-by: Kazuki Miyahara <kmiya@outlook.com>
Co-authored-by: Takeshi Miura <57553950+1222-takeshi@users.noreply.github.com>
@takayuki5168 takayuki5168 self-assigned this Apr 17, 2022
@takayuki5168
Copy link
Contributor Author

@kenji-miyake
We've already commonized max and min acceleration and jerk. It may be better to write max and min velocity as well here though.
https://github.com/autowarefoundation/autoware.universe/blob/main/launch/tier4_planning_launch/config/scenario_planning/common/common.param.yaml

For the vehicle_info package, I don't have any parameters other than max_steer_angle so far.

@TakaHoribe
Could you give us your opinion.

@TakaHoribe
Copy link
Contributor

I agree to have max_steer_angle in the vehicle_info data. There already are some packages defining max_steer value in their local and it's getting messy.

About the velocity, it may be ok to have the max_velocity in the planning common param. In my opinion, we could have two max_velocity: normal.max_velocity and limit.max_velocity. They are like, "follow the limit if possible" and "vehicle must follow the limit". However the definition of the normal and limit is a little vague, so it'd be nice to add more detailed description for them when we add a max_velocty.

@BonoloAWF BonoloAWF added the type:new-feature New functionalities or additions, feature requests. label Apr 19, 2022
@takayuki5168
Copy link
Contributor Author

I'm thinking about the name of the parameter, max_steer_angle or max_tire_angle. I'm not sure if max_steer_angle is confusing for non-Japanese people.

@xmfcx Could you tell me your opinion.

@xmfcx
Copy link
Contributor

xmfcx commented Apr 20, 2022

@takayuki5168 I think max_steer_angle is ok.

image

Reference: http://street.umn.edu/VehControl/javahelp/HTML/Definition_of_Vehicle_Heading_and_Steeing_Angle.htm

@xmfcx
Copy link
Contributor

xmfcx commented Apr 20, 2022

@TakaHoribe

About the velocity, it may be ok to have the max_velocity in the planning common param. In my opinion, we could have two max_velocity: normal.max_velocity and limit.max_velocity. They are like, "follow the limit if possible" and "vehicle must follow the limit".

What is the difference between these 2?

Why is a single speed limit not enough, like vehicle is never allowed to go over 50kmph?

The other speed limits are road related, not vehicle.

@takayuki5168
Copy link
Contributor Author

@TakaHoribe @kenji-miyake @xmfcx cc @tkimura4 @h-ohta
We found that max_steer_angle is not required for the differential wheeled robot.

So how about the following vehicle_info.param.yaml?
Any major/minor comment would be welcome!

/**:
  ros__parameters:
    wheel_radius: 0.39
    wheel_width: 0.42
    ...
    vehicle_height: 2.5

    vehicle_model: "differential_model" # select the vehicle model from differential_model or bicycle_model

    differential_model:
        # if there is any parameter for differential model.
    bicycle_model:
        max_steer_angle: 0.70 # [rad]

@takayuki5168
Copy link
Contributor Author

@TakaHoribe What do you think.

@TakaHoribe
Copy link
Contributor

What is the difference between these 2?

@xmfcx In my mind, just one velocity limit is enough.
However, the concept of 2 limits (e.g normal.max_acc and limit.max_acc) is that different accelerations are use for emergency situations (like, a very large acceleration is required to avoid a risk) and non-emergency situations. And I think it is reasonable.

So we can apply the same idea to the velocity limit as well, that is, the two velocity limits for regular and emergency situations. For example, in the case of merging, ego-vehicle may sometimes need to accelerate beyond the velocity limit to avoid a collision to the behind vehicle.

But as you say, the velocity limit should be defined by the road, so just having two velocity limits as I wrote is not a good way.
(I don't think we have to care about this right now though.)

@TakaHoribe
Copy link
Contributor

@takayuki5168
The current policy for the adaptation to the differential drive robot is to consider the robot as a virtual Ackerman vehicle.

In order to support the differential drive robot officially, we need to replace many parts of planning/control/vehicle modules.

Whereas, the drawbacks of the virtual Ackerman vehicle is handling of the non-essential parameters like steering angle and the lack of the pure angular-velocity rotation. This is not that burden compared to the modification of all Ackerman-specific modules.

So I suggest to have max_steer parameter for all types of vehicles (at least for now).

@takayuki5168
Copy link
Contributor Author

takayuki5168 commented Apr 27, 2022

@TakaHoribe

In order to support the differential drive robot officially, we need to replace many parts of planning/control/vehicle modules.

Right. I've been thinking about using this yaml file with the comment "the differential model is not officially supported. xxx have to be fixed.".

But I found this almost meaningless for now. Now I totally agree with your opinion. Thank you so much.

kyoichi-sugahara pushed a commit that referenced this issue Sep 16, 2023
Signed-off-by: GitHub <noreply@github.com>

Signed-off-by: GitHub <noreply@github.com>
Co-authored-by: kenji-miyake <kenji-miyake@users.noreply.github.com>
YamatoAndo pushed a commit to YamatoAndo/autoware.universe that referenced this issue Aug 8, 2024
…dation#535)

* fix(probablistic_occupancy_grid_map): fix build warning

* ci(pre-commit): autofix

* delete unused

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:new-feature New functionalities or additions, feature requests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants