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(motion_velocity_smoother): improve backward trajectory handling #6079

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

VRichardJP
Copy link
Contributor

@VRichardJP VRichardJP commented Jan 15, 2024

Description

Fixes #5976

Current support for backward trajectories is fairly limited and sometimes just happen to work "by accident" (e.g. some calculated data is wrong but mostly unused). In particular, I have found many backward driving issues with the motion_velocity_smoother node while implementing a new freespace planner.

Most functions in motion_velocity_smoother have been implemented with forward trajectories in mind and don't apply to backward trajectories. The "trick" currently used to handle backward trajectories is to "flip" the trajectory points velocity before processing it, to do as if the vehicle was driving forward and not backward, and then flip the output back. I think the idea is valid, however it is not properly implemented: for example it causes steering values to be incorrect on backward trajectories.

The "proper" way to implement the flipping trick is not just to flip the velocity but to flip everything: trajectory pose orientation, velocity, steering... but also the ego pose and velocity when we need to place ego vehicle on the flipped trajectory.

Note that the way the logic is implemented at the moment (local trajectory but global is_reverse_ parameter) is error prone and difficult to maintain. It would be worth packing trajectory data and ego state inside a single ("flippable") struct so that sub functions could just assume everything goes forward.

Related links

#5976

Tests performed

With the freespace planner in the planning simulator:

  • curved forward/backward trajectories values for longitudinal_velocity_mps, acceleration_mps2, front_wheel_angle_rad have correct signs.
  • same for single point turn trajectories
  • external speed limit on above trajectories works fine.
  • lanelet based driving still looks ok

Notes for reviewers

Interface changes

None.

Effects on system behavior

On all trajectories:

  • motion_velocity_smoother now ignores trajectories mixing forward/backward driving. AFAIK this is not supposed to be supported and would most likely give strange result. But now it is checked.

On backward trajectory:

  • motion_velocity_smoother output trajectory points values are now signed properly

Except for these, there should be no other visible behavior change.

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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

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.
  • The PR is ready for merge.

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

@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Jan 15, 2024
Copy link
Contributor

@rej55 rej55 left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you for your contribution.

@VRichardJP VRichardJP force-pushed the fix_motion_velocity_smoother_backward_driving branch 2 times, most recently from d3153f8 to 462f7f9 Compare January 22, 2024 05:21
@VRichardJP VRichardJP force-pushed the fix_motion_velocity_smoother_backward_driving branch from 462f7f9 to 36e5d8e Compare January 29, 2024 01:19
Copy link

stale bot commented Mar 30, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Mar 30, 2024
@xmfcx xmfcx marked this pull request as draft December 10, 2024 15:45
@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Dec 10, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Dec 10, 2024

@takayuki5168 -san, could you assign someone from TIER IV for this issue? @VRichardJP is probably busy and this seems like an important feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:planning Route planning, decision-making, and navigation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misc backward driving issues in motion_velocity_smoother
3 participants