TrajectoryPoint.acceleration_mps2 is not really an acceleration

[VRichardJP]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I'm convinced that this is not my fault but a bug.

Description

Pretty much everywhere I see acceleration_mps2 used, the value is implicitely interpreted as follow:

• if acceleration_mps2 > 0 then the vehicle is accelerating.
• if acceleration_mps2 < 0 then the vehicle is braking.

This is correct as long as the vehicle drives forward. If you drive backward (v < 0), then:

• the vehicle should be braking when acceleration_mps2 > 0
• the vehicle should be accelerating when acceleration_mps2 < 0

In practice, I observe the opposite: when using the freespace planner to go backward, the vehicle goes faster (backward) when acceleration_mps2 > 0 and slower when acceleration_mps2 < 0.

In other words, TrajectoryPoint.acceleration_mps2 is rather the "directed acceleration" (i.e. a(t) = sign(v(t)) * (v(t+dt) - v(t)) / dt) than the mathematical acceleration (i.e. a(t) = (v(t+dt) - v(t)) / dt)

I understand this design choice, as it is consistent with the way cars behave: pushing the acceleration pedal will always accelerate the vehicle, and pressing the brakes strongly while stopped will not make the vehicle go reverse. But then, I would suggest to change the name acceleration_mps2 to something more appropriate (such as directed_acceleration_mps2 or w/e is the appropriate term) to avoid confusion.

TL;DR
grep -ril acceleration_mps2 src | xargs sed -i 's/acceleration_mps2/directed_acceleration_mps2/g' ?

Expected behavior

N/A

Actual behavior

N/A

Steps to reproduce

N/A

Versions

No response

Possible causes

No response

Additional context

No response

[takayuki5168]

@VRichardJP

In practice, I observe the opposite: when using the freespace planner to go backward, the vehicle goes faster (backward) when acceleration_mps2 > 0 and slower when acceleration_mps2 < 0.

Currently, we define the sign of the acceleration in the planning/control/vehicle like this.
This weird definition comes from the following issue when using the "correct" sign of the acceleration.

The vehicle layer receives the longitudinal control command and the shift separately from the control layer. When we change the shift from the DRIVE to REAR for example, and if there is synchronization mismatch between the control command and the shift, the direction of acceleration may not be correct, causing unexpected acceleration.

I feel that rather than renaming to directed_acceleration_mps2, we can put the longitudinal controller and shift decider into one node so that there is no synchronization mismatch. What do you think about this idea?

[VRichardJP]

Avoiding synchronization mismatch cannot be bad, but my issue is rather that using the name "acceleration" is confusing at code level without proper documentation. "Directed acceleration" is not necessarily very clear, but at least if I read that I immediately understand it might not be the "normal" acceleration I am used to and so I will double check its definition somewhere. If renaming is too annoying, I think at least adding documentation or a few comments here and there would greatly help. For example, in the TrajectoryPoint.msg or TrajectoryPoint.idl files.

[takayuki5168]

I think at least adding documentation or a few comments here

I agree with this.

[stale]

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

[ahmeddesokyebrahim]

I have created these two PRs adding comments and a note in a readme file trying to clarify the usage of acceleration_mps2 :

• docs(TrajectoryPoint): add comment for acceleration_mps2 to clarify its usage autoware_msgs#92
• docs(TrajectoryPoint): add comment for acceleration_mps2 to clarify its usage tier4/autoware_auto_msgs#47

Please let me know if any further change is needed.

[takayuki5168]

@yukkysaito @mitsudome-r
Can you review the above PR?

[ahmeddesokyebrahim]

As the above PRs are merged, we can close this issue as completed.