ros-navigation / navigation2

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

Nav2 Velocity Smoother #2633

Closed vinnnyr closed 2 years ago

vinnnyr commented 3 years ago

https://github.com/ros-planning/navigation2/pull/2631 will remove acceleration / deceleration limits from RPP due to some issues. With the idea that these kind of things should be handled by a downstream velocity smoother anyways.

Is there a home for a velocity smoother in Nav2, either as a tutorial, or a full implementation?

This proposed smoother could set (this list is just an initial set of ideas):

vinnnyr commented 3 years ago

On another note, I am not sure why this issue did not pre-populate with the issue template -- perhaps bc I am using the iOS app... Apologies, I will edit to fit the template soon as possible.

SteveMacenski commented 3 years ago

https://github.com/yujinrobot/yujin_ocs/tree/devel/yocs_velocity_smoother this is what I've used in previous projects in ROS 1, but not sure if this or another (better) version is available in ROS 2. I think its a fabulous idea for us to have a reference on in Nav2. Do you have any suggestions of options or techniques? Applying kinematic constraints isn't rocket science (see code I linked above) but not sure if there's some additional ideas.

Also, adding in a "if no command after X time, send 0" in case there's poorly implemented robot base controllers without timeout sequences.

No worries on the template

SteveMacenski commented 3 years ago

Options to look at for porting / listing features to get the best of all worlds

They all appear to be derivative though, so not much difference between them (at first glance).

SteveMacenski commented 3 years ago

Some requirements

dpm-seasony commented 3 years ago

I've been testing https://github.com/kobuki-base/velocity_smoother with good results.

The code is mostly ready to be used in galactic, I only had to do some minor modifications to have it working.

SteveMacenski commented 3 years ago

I wouldn't be opposed to adopting that in Nav2 (with obvious code quality / styling changes). I think the big thing would be to look over the other methods and make sure this is the "best" of them or if there are features the others have, we adopt those into the port (e.g. support omni).

Is jerk limiting important to you? I know this is a usual need for manipulation and high-speed driving. I certainly wouldn't mind adding it, but I would suspect the data in most mobile robot motors is too noisy at the speeds we run at to be meaningfully smoothed to a 3rd derivative. But I could be totally wrong and definitely not opposed if there's a need.

The steps here

This is honestly a nice, compact project for a student or company that had a need for such a thing and wanted to help make the contribution. This is also something I could commit to working on at some point next year (or over the holidays as a toy project on the plane rides home) if there wasn't external contributor interest. I think this is a good add for Nav2 to bring more basic navigation capacity 'in house'.

vinnnyr commented 3 years ago

Yeah Jerk Limiting is important for us, although not as important as the deadband issue.

I would suspect the data in most mobile robot motors is too noisy at the speeds we run at to be meaningfully smoothed to a 3rd derivative

I don't think this is an issue because the velocity smoother is being applied to the commanded velocities, not the measured ones from the robot. In theory you won't have to take 3rd derivatives of noisy mobile base motor data, or am I missing something?

SteveMacenski commented 3 years ago

Aren't most of those taking odometry messages https://github.com/kobuki-base/velocity_smoother/blob/devel/src/velocity_smoother.cpp#L161 to check and limit the next velocity command by? That would make most sense to me.

wilcobonestroo commented 2 years ago

Aren't most of those taking odometry messages https://github.com/kobuki-base/velocity_smoother/blob/devel/src/velocity_smoother.cpp#L161 to check and limit the next velocity command by? That would make most sense to me.

I see that the Kobuko package has 3 options for input: none, odometry and commanded velocities. Odometry makes sure that the new cmd_vel value is within bounds compared to the currently estimated odometry. The commanded velocities looks at the previous cmd_vel that was send and assumes that the robot follows these commands. I don't understand the none option yet, but I will have a look at their code to see what it actually does. I think it makes sense to at least have the options for odom and for commanded velocities as input.

In the three implementations mentioned above I only see speed limits and acceleration limits. I think we can add jerk limits as an option.

@vinnnyr Can you describe some use cases or scenarios where you use the deadband issue? E.g. what are typical values? Is there one band or multiple? What should happen if the orginal cmd_vel is in the deadband?

I can pick up this issue and work on a nav2 velocity smoother. Should I put it in its own package or add it to an existing one? What would be a good name for the package? Simply nav2_velocity_smoother? There is another folder called nav2_smoother

vinnnyr commented 2 years ago

Jerk Limits

Deadband

The velocity smoother in the version of Regulated Pure Pursuit (RPP) was in odometry mode. The deadband issue was inspired by issues we were fighting with RPP on a robot. We would give RPP a reasonable acceleration limit, but since that robot did not have a lot of control regime in the low end of velocity inputs, getting the robot to start would be tough.

If given a reasonable acceleration limit ( that we would have wanted enforced when the robot was at speed), we would enter this situation.

delta_t = 0.1
max_acc = 1.0

# loop 0
current_velocity = 0.0
# max_allowed_velocity = current_velocity + delta_t * max_acc
max_allowed_velocity = 0.1
cmd_velocity = 0.1

# loop 1
current_velocity = 0.01
max_allowed_velocity = 0.11
cmd_velocity = 0.11

# loop 2
current_velocity = 0.01
max_allowed_velocity = 0.11
cmd_velocity = 0.11 #and so on

Where loop 2 onwards, we were stuck at commanding a velocity the robot would have never been able to move.

I think in our use case, a deadband from (0.0, 0.5] for cmd_vel would have been useful (linear.x). In my use case, I do not think multiple deadbands would have been needed.

What should happen if the orginal cmd_vel is in the deadband?

Hm , this is a tough issue to generalize. In essence, I want the smoother to either command 0.0 or 0.5. Maybe this would just be something of this sorts: "if my current velocity is in the deadband range, do not apply the acceleration limit based on odom, but rather apply the acceleration limit based only on cmd, or maybe do not apply the acceleration limit at all"

wilcobonestroo commented 2 years ago

That helps understanding the deadband issue :smile: So there should probably be several options for the behavior in the band.

Do you think we should include jerk for the commanded velocity mode? Or can we completely drop the jerk issue?

Ps. I started working on the boilerplate code and reading the parameters: https://github.com/wilcobonestroo/navigation2/tree/add-velocity-smoother

SteveMacenski commented 2 years ago

In the three implementations mentioned above I only see speed limits and acceleration limits.

Some only handle X, one of the things to make sure is that it supports omni robots in the Y velocity / acceleration direction. Its more about the implementation details that some have bells and whistles others are lacking in.

Should I put it in its own package or add it to an existing one?

nav2_velocity_smoother sounds good to me!

Do you think we should include jerk for the commanded velocity mode? Or can we completely drop the jerk issue?

I think for a first-release we can not include jerk, but if @vinnnyr wanted to come in after and include it, that PR would be easily merged. After the bulk is in place, adding a couple new parameters and a new constraint would be a trivial PR to review and merge.

wilcobonestroo commented 2 years ago

Hmmm... I had never thought about omni robots and the y-velocity. In the implementation from Care-O-bot (cob) they treat the x and y as independent things to control. I.e. they simply apply the rules for linear speed and acceleration to both components. So, in theory you can have a total combined linear speed larger than the max linear speed (e.g. when both x and y are at their max). Is this what you also had in mind?

Another question: what is the logical way to have this node running? I was thinking to listen to the original cmd_vel topic and in the callback immediately publish the smoothed version. There can be a timer-based check to see if there are messages coming in or not. In the other implementations, I also see an approach where the smoother has his own timer (and thus his own publish rate). Velocity message that arrive are cached and processed at the speed of the internal timer. I don't see the benefits of this. I think it only adds latency in the messages and more complexity in the code. So, I was thinking in the direction of reacting to incoming Twist messages on cmd_vel. In the meanwhile, odom messages can be cached to determine the best odom estimate once a Twist message arrives. What do you think?

wilcobonestroo commented 2 years ago

I have some initial smoothing attempts going on :smile:

Screenshot from 2022-02-06 16-52-50

SteveMacenski commented 2 years ago

they treat the x and y as independent things to control

That would be fine to do, so you could make some functions that take in the min/max values (or probably a struct containing them) and then pass in each axis into them separately to smooth independently. Would be nice actually to have that abstracted architecturally.

I'm not sure the best way off hand to deal with a maximum velocity that is higher than that of the sum of the components. I think that's also the role of the local trajectory planner. This is smoothing out the values, which I agree would be helpful to enforce the constraints. Though enforcing each direction's constraints seems good enough to me unless there's a clear way of doing that demonstrated by another method linked above.

I was thinking to listen to the original cmd_vel topic and in the callback immediately publish the smoothed version. There can be a timer-based check to see if there are messages coming in or not.

It looks like they all do that in one way or another (timer, while loop, etc). I figure there must be a reason for that? Regularity of the intervals? So that it can be run at a faster rate than local trajectory planner is executing at in order to have a smooth interpolation to "ramp" commands by the regular interval samples? <-- that sounds about right

wilcobonestroo commented 2 years ago

I have also been playing around some more. I assumed that I would always get a continuous stream of cmd_vel messages. However, they immediately stop once the goal has been reached. Using the deceleration constraints you need to make it stop slowly. So, I need to have the node running on its own timer anyway. Moreover, smoothly interpolating by having a higher rate is the best reason for having it that way.

wilcobonestroo commented 2 years ago

Screenshot from 2022-02-11 11-46-40

The behavior is now like this. The ramp-up and ramp-down seem ok to me. What should be the behavior at the top? Currently, it is almost following the noisy behavior of the input, because this is within the acceleration limits. Is this behavior ok?

wilcobonestroo commented 2 years ago

Screenshot from 2022-02-11 12-32-21

I can also do something like this with a buffer for the cmd_vel messages. As with all smoothing you get some delay in the signal.

SteveMacenski commented 2 years ago

What should be the behavior at the top? Currently, it is almost following the noisy behavior of the input, because this is within the acceleration limits. Is this behavior ok?

Yeah, but I'd test with setting the max accels to something lower to see what happens if those little variations are invalid (so it should be smoothed out). Actually, that should be more or less what you show with the buffer.

The buffer is an interesting idea. I don't think that should be the default behavior but I'd be fine with that a parameterized option.

SteveMacenski commented 2 years ago

How's this going?

SteveMacenski commented 2 years ago

@wilcobonestroo any update?

wilcobonestroo commented 2 years ago

Most functionality is in place. I have to remove some buffering code. Moreover, I did not include the deadband yet and I don’t know how (or where) to write the documentation. I will try to finish it this weekend and make a PR. Code is at: https://github.com/wilcobonestroo/navigation2/tree/add-velocity-smoother

SteveMacenski commented 2 years ago

Awesome! If you need any help, let us know. Between me, @AlexeyMerzlyakov and @padhupradheep, you've got resources

wilcobonestroo commented 2 years ago

Could someone give me some feedback on this code for this issue? @SteveMacenski , @AlexeyMerzlyakov or @padhupradheep?

jwallace42 commented 2 years ago

@wilcobonestroo can you put in a pr? Then I can take a look :)!

SteveMacenski commented 2 years ago

Hi,

I'm back from PTO now and will take a look tomorrow or the next day. I haven't taken a look at this yet, but I did want to point you to ruckig that's being used in https://github.com/ros-planning/navigation2/pull/2816. I think there's some natural synergies here to use that for this work potentially. I know it would be a near total rewrite regardless of what you've done so far, so I just want to bring it up, but it is not a requirement to use it. Its being used in MoveIt2 and likely to be added to RPP so if we do other kinematics "stuff" it might be good to use to be consistent with other ecosystem projects.

Steve

SteveMacenski commented 2 years ago

@wilcobonestroo in terms of feedback

@vinnnyr how do you feel about jerk ( see last comment)

AlexeyMerzlyakov commented 2 years ago

My 5 cents into review:

SteveMacenski commented 2 years ago

I started to play with this using ruckig just to see how / if it would work. See branch: https://github.com/ros-planning/navigation2/tree/vel_smoother. Its still in development and incomplete. It brings up a few questions I'm still working through

So it might be that a more manual approach is more appropriate for us, like what @wilcobonestroo is working on? Though in doing so, I don't think its reasonable to try to limit jerk if our own inputs and outputs are just Twist and Odometry, we don't have any acceleration information, let alone jerk. We can certainly set limits on acceleration manually (e.g. v1 = v0 + a_limits * t) and threshold within limits, but that's not as smooth as generating a trajectory if new discontinuous commands come in. Jerk is another derivative on top of that which would not be numerically stable to compute based on velocities.

@AndyZe over at PickNik is using Ruckig for MoveIt2, maybe he has some thoughts to share? Our aim is to add a velocity smoother to Nav2 to take in cmd_vel from the stack and smooth them before throwing into the robot hardware controllers for execution. I like the idea of off-boarding as much as I can to external libraries, especially if they're used elsewhere in the ecosystem. Do you think we should stick to ruckig?

AndyZe commented 2 years ago

I don't have any magic answers but it sounds like you're asking the right questions. Tough problem.

We don't have access to that information reliably unless we numerically differentiate the odometry which is unstable for closed-loop feedback

If you end up numerically differentiating, here's a decent way to do it:

it does work if we do open loop feedback (e.g. we assume the last timestep we met the required state and so we can store the last iteration's velocity/acceleration/jerk to use as the "current" in the next timestep).

I tried this 2 ways:

For setting the current acceleration / jerk, I suppose we could set them all to 0 across the board and then the trajectory generated would assume the extremes for starting/stopping the trajectory. So for closed-loop set them all to 0-s since we can't estimate them reliably?

My intuition says that won't work very well. I think the motion will be slower than you expect. I could be wrong, though

SteveMacenski commented 2 years ago

I don't actually understand the difference between the 2 methods you mentioned, can you elaborate on the second point? (1) would be setting the new_{velocity, acceleration, pose} from the output as the input of the next iteration. I'm not sure what (2) entails. There's also pass_to_input method that appears in the demos but never in the documentation that you could see in my branch I have open questions around precisely its nature (which might be what you're referring to).

My intuition says that won't work very well. I think the motion will be slower than you expect. I could be wrong, though

As well. Though if we do a trivial v min / max = v0 +/- a * t calculation for thresholding, it's not really taking the current acceleration into account either. I need to do some thinking on that, but that feels analogous to the ruckig setting all current accelerations to 0.

I feel like this shouldn't be a technology mismatch, but might end up being. Traditionally in the ROS navigation ecosystem, we've just taken some $vi$ and used basic kinematics to find the guard rails of $v{min}$ and $v_{max}$ based on the set acceleration min/max and threshold it. Nothing fancy.

The 2 major uses cases of it are:

For the second case, generating a true trajectory sounds awesome so that we can get an optimal profile to work with. But for the first, I don't think generating a trajectory would be any worse than just thresholding.

Since we have a constant stream of these cmd_vel's coming out of the trajectory planner at a relatively consistent rate (and with algorithms that pre-apply varying levels of feasibility constraints) the target setpoint for the velocity is constantly changing but we don't know what the future holds to be able to meaningfully set target acceleration setpoints. But having a target acceleration of 0 seems rational to me, since that means that we achieved the goal velocity and are using it as steady state.

I'm also now thinking if, for illustration purposes, we had a trajectory planner giving us a cmd_vel at 1hz and we have a smoother at 100hz. Would we rather smoothly and slowly, below the kinematic limits of the robot's acceleration, wait the full 1s to get to the target speed, or get there as quickly as possible and maintain state.

--> I'm thinking the latter, which might then argue against using any kind of trajectory libraries, since we would want to use the full limits available to us vs moving below it to use the full time available (is that an option in ruckig? I don't think so). Navigation assumes instantaneous response, so the closer to that we can give, the better performance of tracking would be. The 1hz / 100hz is a drastic example, a more typical set up would be 20-50hz / 20-100hz, so the results would be significantly less noticable.

@wilcobonestroo what do you think about that?


@vinnnyr what are your thoughts on jerk limiting, how are you measuring acceleration so that you wouldn't be double differentiating noisy velocity data? What is/was your game plan there? It seems to me like that would probably need to be handled at the trajectory planner or hardware controller level so that either (1) you can make trajectories that are themselves already jerk limited or (2) you have access to raw data in the low levels that might be able to better estimate acceleration / jerk.

SteveMacenski commented 2 years ago

Well another thing to point out is that these all assume independent X, Y, and Theta velocity channels. Something Yocs does is constraining other channels when one is too high so that the velocity follows the vector previously created, just scaled down https://github.com/kobuki-base/velocity_smoother/blob/devel/src/velocity_smoother.cpp#L263-L296. I think this is a good idea. Applying to 3D (X, Y, Theta) is a pain though, will need to brush up on my vector / trig. I will say though the few examples of 3D velocity smoothers have removed that feature which is telling. Nice to know other smart people than me also had to think twice if it was worth going into that level of detail :wink:

AndyZe commented 2 years ago

so that you wouldn't be double differentiating noisy velocity data?

Small correction here: Ruckig doesn't care what the current jerk of the robot is. It's not an input to Ruckig. So you only have to differentiate once to calculate acceleration.

I don't actually understand the difference between the 2 methods you mentioned, can you elaborate on the second point?

The second point means:

It's really similar to Point 1 except you're not using the Ruckig output, you're using the target state that you provided originally. It should be an almost perfect match, which is why I was confused to see different behavior on our hardware.

SteveMacenski commented 2 years ago

Thanks for the clarification!

AndyZe commented 2 years ago

If it would help, I can write some of this. Maybe the velocity differentiation part and/or the Ruckig part itself. My problem is, I have no idea where things go in the Nav2 codebase.

SteveMacenski commented 2 years ago

I think from my current looking, ruckig is not the best choice for us unfortunately for this project :frowning: which I'm disappointed by since it looks like it could really streamline some things. However we want to (1) use the maximum kinematic limits possible to achieve velocities ASAP and maintain them vs using the full time allotted and (2) be able to proportionately bound the velocities of the axes so that we maintain the same (or as similar as possible) commanded direction.

From that, ruckig doesn't quite make sense for our application. However, I think it would be really interesting to see if we could use ruckig in the trajectory planners or smoothers to work with theoretical trajectories being generated versus involvement of real data. If we're using it for trajectories generated via other methods, then we should have acceleration and other information we can meaningfully use versus live / noisy data.

An area I could see ruckig being really nice for is if we had a post-trajectory planning step to take trajectories from local planners and smoothed them out using ruckig. Although, for the frequency that the local planners are updated at, this might be overkill, but I think its the most appropriate place for it in the mobile robot stack (which would be relatively analog to MoveIt's use of it as well). I'll keep this tool in my back pocket though. If we added some machine learning or heavy sampling based trajectory planners, I think ruckig would really shine

Thanks @AndyZe for the information and help! It's really helped. Feel free to poke me if I can be helpful on the moveit side.

SteveMacenski commented 2 years ago

2964 is ready for testing if folks want to kick the tires!

vinnnyr commented 2 years ago

I just noticed I missed the tags here when my input was requested for Jerk. So Sorry about that!!! Now I feel like a jerk ;) I think this question is no longer relevant, but indeed I was hoping Jerk could be handled based on cmd_vel alone rather than trying to measure acceleration based off of sensors / localization estimates.

(1) you can make trajectories that are themselves already jerk limited

SteveMacenski commented 2 years ago

I think it makes sense to add jerk limiting to the trajectory planners perhaps to get around this situation, so that way the computed trajectories generating the commands are limited by it so that the velocity smoother doesn't require to do it -- and then its based on theoretical models and not actual current sensor data so you can differentiate it to your heart's desire.

SteveMacenski commented 2 years ago

Merging imminent, thanks @vinnnyr for bringing up this gap