ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.47k stars 1.25k forks source link

Issue with Bidirectional Acceleration and Deceleration Settings in MPPI Controller #4601

Open osama-z-salah opened 1 month ago

osama-z-salah commented 1 month ago

I am using an MPPI controller to drive a robot capable of traveling bidirectionally with equal efficiency. I want to have different values for acceleration and deceleration. To achieve this, I set the acceleration by setting ax_max to 0.5 and the deceleration by setting ax_min to -1.5. These values are respected when the robot is traveling forward. However, when the robot is traveling backward, the acceleration value becomes the deceleration value, and vice versa.

SteveMacenski commented 1 month ago

This is the block of code of interest https://github.com/ros-navigation/navigation2/blob/main/nav2_mppi_controller/include/nav2_mppi_controller/motion_models.hpp#L85-L113

What you mention here is very sensible and I think that should be enable-able. I'd be open to your suggestions, but I think a potential solution would be to have a new parameter option for bidirectionality - similar to mode in PathAngleCritic - which in this block can be checked and flip the signs as needed. The parameter should be stored in control_constraints_.

Optionally, we could make that default behavior and just have the logic in that block for checking if we're slowing our total speed (apply deceleration) or increasing our total speed (apply acceleration), regardless of signed direction when applying limits.

Something to take care about is that this block is very performance sensitive, so when prototyping solutions, we need to benchmark before and after compute times of the controller to make sure we're not manifestly dropping performance. This should be possible without any performance dings I think.

I imagine that's what @doisyg would also want. @tonynajjar do you think that's reasonable to have it default have that behavior as a general case for your non-bidirectional case? I'd always prefer less parameters if it can be avoided, but it does seem like it should be applied in this way for all cases I think since its more about 'speed' than direction.

doisyg commented 1 month ago

I imagine that's what @doisyg would also want.

Yes! Maybe some logic to reuse from this: https://github.com/ros-navigation/navigation2/pull/3529

SteveMacenski commented 1 month ago

I think that is precisely code to reuse here! @osama-z-salah can you create a PR for this? Happy to merge!