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

[RPP] Jerk limited trajectory generation #2816

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

angstrem98
Copy link


Basic Info

Info Please fill out this column
Ticket(s) this addresses (#2815)
Primary OS tested on (Ubuntu)
Robotic platform tested on (Custom differential drive robot in Webots, TurtleBot 3 Gazebo simulation)

Description of contribution in a few bullet points

  • Generate jerk limited trajectories using ruckig package
  • Provide accurate goal angle control using proportional controller
  • Smooth stopping near goal while respecting kinematic limits due to motion profiling

Description of documentation updates required from your changes

  • Add new parameters to documentation, remove unused

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2022

@angstrem98, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2022

@angstrem98, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2022

@angstrem98, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2022

@angstrem98, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@doisyg
Copy link
Contributor

doisyg commented Mar 12, 2022

Thanks for the PR, it seems to generate nice smoothed cmd vel.

However I believe there is a bug with the rotating_ globlal variable. When rotating at the end of a path, it can be left in the true state because the goal checker stops executing computeVelocityCommands and then this code path is never executed (i.e. it is never set back to false): https://github.com/memristor/navigation2/blob/7f1e4e5584bed038cb660ac10166cc9a27b65bf7/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L395-L398

This creates an inconsistent/unrequired rotation at the beginning of the next path.

The rotating_ globlal variable needs to be reseted before each new execution of a path, for instance by adding rotating_ = false; in setPlan

@mergify
Copy link
Contributor

mergify bot commented Mar 13, 2022

@angstrem98, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@angstrem98
Copy link
Author

Thanks for the PR, it seems to generate nice smoothed cmd vel.

However I believe there is a bug with the rotating_ globlal variable. When rotating at the end of a path, it can be left in the true state because the goal checker stops executing computeVelocityCommands and then this code path is never executed (i.e. it is never set back to false): https://github.com/memristor/navigation2/blob/7f1e4e5584bed038cb660ac10166cc9a27b65bf7/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L395-L398

This creates an inconsistent/unrequired rotation at the beginning of the next path.

The rotating_ globlal variable needs to be reseted before each new execution of a path, for instance by adding rotating_ = false; in setPlan

Thanks for trying it out and pointing an issue. I've pushed a commit that should fix the problem. Besides resetting it in setPlan as you proposed, rotating_ is being reset here too:
https://github.com/memristor/navigation2/blob/4eb8c3920d6e3f772e59f20b67556d4c22a0690d/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L333

I'm looking forward to hearing how it works out!

@doisyg
Copy link
Contributor

doisyg commented Mar 13, 2022

Your last PR fixes the rotation bug, thx.

@doisyg
Copy link
Contributor

doisyg commented Mar 13, 2022

You should probably make your new parameters dynamic similarly to the others

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

I need to in more detail look into the ruckig stuff, but here are the project / software level changes required.

@@ -295,6 +370,11 @@ geometry_msgs::msg::TwistStamped RegulatedPurePursuitController::computeVelocity
curvature = 2.0 * carrot_pose.pose.position.y / carrot_dist2;
}

double carrot_dist = sqrt(carrot_dist2);
Copy link
Member

Choose a reason for hiding this comment

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

We use the square intentionally to above the sqrt, can't the remaining_path_length be remaining_path_length*remaining_path_length, that's much cheaper than a sqrt operation

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough - replace the call on that line then with this variable so we don't do it twice

double kp_angle_;
double max_linear_jerk_;
double max_angular_jerk_;
rclcpp::Time system_time_;
Copy link
Member

Choose a reason for hiding this comment

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

This is reliably run at the controller_frequency rate, may that be substituted instead of keeping timers?

Copy link
Author

Choose a reason for hiding this comment

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

True, it is run at mentioned frequency. I am not using timers, just tracking system time. The problem is, I need to reset some internal states in case goal has been cancelled or goal checker stopped calling computeVelocityCommands. There is no such callback that allows me to do that, so my solution is to check if computeVelocityCommands has not been called for 4 control loop durations. If that's true, we have to reset internal states since controller has been idle for a while. You can look at the logic here:
https://github.com/memristor/navigation2/blob/03cf93ce88499222ed5e599f1a5dad8e8572c50d/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L324-L344

Copy link
Member

Choose a reason for hiding this comment

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

You should break that logic out into a separate function

Copy link
Member

Choose a reason for hiding this comment

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

Would setPlan do that? E.g. its a new plan given. The 2 cases is that there's that delay you mention or its a replan during robot execution.

The state reset if the time is too long seems like a better fit to be executed in setPlan than in this function

@@ -303,13 +383,21 @@ geometry_msgs::msg::TwistStamped RegulatedPurePursuitController::computeVelocity

linear_vel = desired_linear_vel_;

distance_profile_input_.max_velocity = {desired_linear_vel_};
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already set?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but logic is as follows:

  1. Set maximum velocity to desired_linear_vel_
  2. RPP algorithm modifies maximum velocity in order to folow path

Because maximum velocity limit in motion profile is changed, we must reload it in step 1.

Copy link
Member

Choose a reason for hiding this comment

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

This is another place where semantically meaningful functions and comments are important

@@ -568,6 +679,9 @@ void RegulatedPurePursuitController::applyConstraints(
void RegulatedPurePursuitController::setPlan(const nav_msgs::msg::Path & path)
{
global_plan_ = path;

angle_profile_input_.control_interface = ruckig::ControlInterface::Velocity;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already set?

Copy link
Author

Choose a reason for hiding this comment

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

When new path is passed to controller it could be rotating to some heading at the moment. This allows the main control loop to break that rotation if needed.

@SteveMacenski
Copy link
Member

Is there another proxy you can use rather than keeping some rotating_ state? For instance, in rotation, check some value which would indicate as such or be able to reset another variable to indicate that?

@@ -318,19 +406,51 @@ geometry_msgs::msg::TwistStamped RegulatedPurePursuitController::computeVelocity

// Apply curvature to angular velocity after constraining linear velocity
angular_vel = linear_vel * curvature;

distance_profile_input_.control_interface = ruckig::ControlInterface::Position;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already set in activate?

Copy link
Author

Choose a reason for hiding this comment

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

Sometimes distance profile goes in velocity mode. This is when we rotate to heading - we set distance profile to velocity mode and command zero velocity which brings us to a smooth stop. So, switching to position mode is needed later.

distance_profile_input_.current_position[0] + sign * remaining_path_length};
distance_profile_input_.max_velocity = {fabs(linear_vel)};

angle_profile_input_.control_interface = ruckig::ControlInterface::Velocity;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already set in activate?

Copy link
Author

Choose a reason for hiding this comment

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

It has to be set again since we switch position and velocity modes around and modify velocity limits. Similar to previous answers.

angular_vel_command = angle_profile_output_.new_velocity[0];
} else {
// proportional controller for robot angle when we are in position mode
angular_vel_command = kp_angle_ * angleNormalize(
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely following this kp_angle_ thing - can you elaborate on it?

Copy link
Author

Choose a reason for hiding this comment

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

In case a robot is able to rotate in place I wanted to make it able to rotate to goal heading (or path heading) accurately. Current implementation on main branch doesn't allow fine positioning at high speeds.
We have a goal heading (angle) setpoint which we want to achieve (make out robot angle become equal to setpoint). Difference between setpoint and current angle is error. We can pass this error to a controller such as PID to make the error zero and thus achieve our goal heading (with gains kp, ki, kd). Position tracking controllers are generally designed as P or PD. I made a simple control law that basically does this: command = proportional_gain * error.
So, kp_angle_ is proportional gain.

We could simply pass it a setpoint and it will eventually reach it, but high gain could cause oscillations and low gain would prevent oscilaltions but it would take pretty long to reach the setpoint. Setting the angle motion profile to position mode makes it generate series of position setpoints which will make us reach the setpoint while respecting kinematic limits. This profile allows us to have a higher kp gain.

This approach allowed me to have yaw goal checker at 1 degree and less precision (on a custom robot).

Copy link
Member

Choose a reason for hiding this comment

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

This should be being handled in the rotateToHeading method, not separately implemented. We can't have multiple rotation implementations using different methods.


distance_profile_input_.control_interface = ruckig::ControlInterface::Position;
distance_profile_input_.target_position = {
distance_profile_input_.current_position[0] + sign * remaining_path_length};
Copy link
Member

Choose a reason for hiding this comment

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

Does this strictly need to be the path integrated distance, or can this be the L2 norm? This should only be happening on starting or ending a goal, so either the goal is far away or very close

Copy link
Author

Choose a reason for hiding this comment

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

It needs to be the path integrated distance, this is the core idea when it comes to stopping on goal point. By passing remaining path length (in meters) to distance profile it can calculate when to start braking.

Copy link
Member

Choose a reason for hiding this comment

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

If its on approach though, which is when this matters, wouldn't it be effectively a straight line path?

This breaks encapsulation of software functions by having the getLookAheadPoint return this value and does so in a pretty not-great way as an implicit return type when the main return value is explicit. Architecturally, this is really not great and I think and L2 norm would do effectively the same job since we should only be slowing down when in close proximity to the goal.

If integrated path distance is required, it should not be being returned from the getLookAheadPoint method.

Copy link
Author

@angstrem98 angstrem98 Apr 5, 2022

Choose a reason for hiding this comment

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

Well, the remaining path length for sure must me in meters (because inside ruckig velocity, acceleration and jerk are in default units, giving it a setpoint in m^2 would be problematic). Also, we could be approaching the goal on a circular path, like robot driving on a circle?

I was thinking about one solution to lower computational cost. When we are "far away" from the goal we just make the robot speed up. But when we are "near enough" the goal we can start calculating remaining integrated path distance and come to a nice stop at goal. "Near enough" goal could be defined as a parameter (safe_braking_distance maybe?), let's say 5 meters default.
In order to detect if we are near enough goal we could simply use L2 distance to goal (L2 dist < safe_braking_distance^2).
Other approach is to use your already tried method - find a lookahead point (5 meters ahead) and at the end of path optimal lookahead distance and actual lookahead distance will differ - we can start calculating the remaining path and robot comes to a smooth stop. (Please note this isn't lookahead point used for RPP, it's just to detect if we are at safe_braking_distance or closer to the goal )

@SteveMacenski
Copy link
Member

SteveMacenski commented Mar 15, 2022

Another question is whether its strictly required to have the angular velocity be set with profiles. I think it depends on the aims and goals of this. If the angular velocity is smoothed out too much or parameters not set properly, it could deviate from the path and that will cause substantial oscillatory behaviors. Linear velocity its more essentially clear the value and need for

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2022

This pull request is in conflict. Could you fix it @angstrem98?

@doisyg
Copy link
Contributor

doisyg commented Mar 28, 2022

@angstrem98, this PR is promising, any update ?

@angstrem98
Copy link
Author

angstrem98 commented Mar 28, 2022

@angstrem98, this PR is promising, any update ?

@doisyg Sorry for the delay, I've been busy with other projects. I think I will be able to finish this PR in the next few days and answer the comments above.

@Aposhian
Copy link
Contributor

I haven't done rigorous testing, but I have tested this in simulation with our robot and found good results!

@Aposhian
Copy link
Contributor

Another question is whether its strictly required to have the angular velocity be set with profiles. I think it depends on the aims and goals of this. If the angular velocity is smoothed out too much or parameters not set properly, it could deviate from the path and that will cause substantial oscillatory behaviors. Linear velocity its more essentially clear the value and need for

As a side note, the recently added RPP lookahead point interpolation(#2872) greatly helps to smooth out angular velocity, as well as linear velocity when scaling is applied around turns. In my experimentation with this PR, I have observed some oscillations when the max_angular_jerk is aggressively small. However, I still think it is a good knob to provide, to complement interpolation.

Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
Signed-off-by: angstrem98 <angstrem98@gmail.com>
@angstrem98
Copy link
Author

Another question is whether its strictly required to have the angular velocity be set with profiles. I think it depends on the aims and goals of this. If the angular velocity is smoothed out too much or parameters not set properly, it could deviate from the path and that will cause substantial oscillatory behaviors. Linear velocity its more essentially clear the value and need for

I would say that angular velocity still needs profiles (especially when rotating towards heading in position mode.)
I don't think about it only as a smoother, but also as a limiter. Imagine a robot without payload heading to pick up some lab samples. While empty it can go fast towards the goal. After picking up the samples, user sets limits that need to be respected in order to bring them safely.

@Aposhian Yes, making jerk limits too low will make robot oscillate for sure, that way you are making it really "lazy". Also concerning @SteveMacenski question: My stance is that if kinematic limits make robot oscillate or deviate from path, RPP regulated parameters are simply not properly set (or limits are unreasonable). User has entered the limits that need to be respected, and they are being respected, but regulated parameters need some tuning for proper performance.

@angstrem98
Copy link
Author

Is there another proxy you can use rather than keeping some rotating_ state? For instance, in rotation, check some value which would indicate as such or be able to reset another variable to indicate that?

I got rid of it in the latest push. I am able to check if angle profile is in position or velocity mode.

@doisyg Dynamic parameters should be working now.

@angstrem98 angstrem98 requested a review from SteveMacenski April 3, 2022 04:00
@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 4, 2022

Architecturally, this PR needs work, there are large chunks of code added to existing functions that should be broken out into semantically meaningful chunks and put into well named functions so the code is better self documenting and easier for users to understand.

There are implicit and then explicit return types from modifications of the functions which is really not great. This overall needs alot of work to redesign these changes to be more easily understood and function's not being modified outside the scope of their specific role

@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 4, 2022

I would say that angular velocity still needs profiles (especially when rotating towards heading in position mode.)
I don't think about it only as a smoother, but also as a limiter. Imagine a robot without payload heading to pick up some lab samples. While empty it can go fast towards the goal. After picking up the samples, user sets limits that need to be respected in order to bring them safely.

I agree in pure rotations having some control for smooth behavior is nice. However, for the path tracking PP algorithm part, it should be the one dictating the angular velocity based on the curvature of the path. The 'Regulation' part is only on the linear velocities. If you deviate from the angular velocities, this is no longer a PP and it will not be tracking the path accurately.

We actually used to have kinematic limits in place until #2621 which brought up an issue there and we removed it in #2631. @dpm-seasony @vinnnyr can you comment on this PR / thoughts?

Perhaps some of this work doesn't need to be done in RPP itself but in the velocity smoother #2633 that interacts at the lower-levels that can be tuned or used as appropriate for a given user. Certain elements of this PR I think could stay in that situation, but it would simplify the constraints that we needed to apply.

Yes, making jerk limits too low will make robot oscillate for sure, that way you are making it really "lazy". Also concerning @SteveMacenski question: My stance is that if kinematic limits make robot oscillate or deviate from path, RPP regulated parameters are simply not properly set (or limits are unreasonable). User has entered the limits that need to be respected, and they are being respected, but regulated parameters need some tuning for proper performance.

This needs to be optional to turn off kinematic limiting so that users that don't wish to deeply model their platform can use this without issue.

@Aposhian
Copy link
Contributor

Aposhian commented Apr 5, 2022

Perhaps some of this work doesn't need to be done in RPP itself but in the velocity smoother #2633 that interacts at the lower-levels that can be tuned or used as appropriate for a given user.

While I think there is a place for a velocity smoother, a velocity smoother cannot intelligently produce planned velocities to stop at the end of the path.

@doisyg
Copy link
Contributor

doisyg commented Apr 5, 2022

Perhaps some of this work doesn't need to be done in RPP itself but in the velocity smoother #2633 that interacts at the lower-levels that can be tuned or used as appropriate for a given user.

While I think there is a place for a velocity smoother, a velocity smoother cannot intelligently produce planned velocities to stop at the end of the path.

I agree. This all comes down to not overshooting a goal.
Safe velocities and accelerations/decelerations can be enforced by a velocity smoother (whereas it should be part of nav2 or a lower level node is another debate), BUT the controller needs to be aware of these kinematic constraints otherwise it will generate infeasible cmd_vel which will lead to sub-optimal or impossible control.
The example of a robot with a very slow deceleration capability illustrate the problem well: there will be a larger goal overshoot if the controller doesn't start to deccelerate sufficiently before the goal.


distance_profile_input_.control_interface = ruckig::ControlInterface::Position;
distance_profile_input_.target_position = {
distance_profile_input_.current_position[0] + sign * remaining_path_length};
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I might just be missing this, but how is distance_profile_input_.current_position populated? As far as I can tell, we are not actually populating this based on any localization estimate from the controller server

Copy link
Author

@angstrem98 angstrem98 Apr 5, 2022

Choose a reason for hiding this comment

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

Update function generates state which includes new position, velocity, acceleration and puts everything in distance_profile_result_ as you can see here:
https://github.com/memristor/navigation2/blob/03cf93ce88499222ed5e599f1a5dad8e8572c50d/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L440-L441

This line basically copies these values to distance_profile_input_ which includes new value for current_position:
https://github.com/memristor/navigation2/blob/03cf93ce88499222ed5e599f1a5dad8e8572c50d/nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp#L448

Previous steps are standard way to use ruckig to generate motion.

Please note that position inside distance profile currently does not have any real interpretation. We calculate how much we need to move towards goal = remaining_path_length. Distance motion profile "thinks" that we are currently at current_position, then we tell it: "move to position equal to current_position + remaining_path_length" and it fits the motion profile.

So, yes, we are not populating it based on some localization. It starts from position 0.0 and we just use relative moves based on how much we want to move.

@vinnnyr
Copy link
Contributor

vinnnyr commented Apr 5, 2022

I don't think this change will reintroduce the problems we encountered in #2621 since as far as I can tell, this change does not account for the robots actual speed, it just provides limits on the commanded speed.

Problems we ran into in the earlier iteration of kinematic limiting in RPP were related to our robot not having great velocity control when the velocities were relatively small. This is a problem I describe in the velocity smoothing ticket here (see Deadband section): #2633 (comment)

We actually used to have kinematic limits in place until #2621 which brought up an issue there and we removed it in #2631. @dpm-seasony @vinnnyr can you comment on this PR / thoughts?

As far as the "where should this live" I don't really have a strong preference as it relates to our use case. If there is a desire to put it all in the separate smoother, I can imagine a version of the proposed smoother where the controller calls a smoother service to prevent overshoot.

Not sure if that obfuscates the meaning of a smoother though, so perhaps the "prevent overshoot" portion should remain in RPP / controller and the jerk limit + ang vel limits could remain in the vel smoother?

@angstrem98
Copy link
Author

angstrem98 commented Apr 5, 2022

This needs to be optional to turn off kinematic limiting so that users that don't wish to deeply model their platform can use this without issue.

This is a very good idea.

Not sure if that obfuscates the meaning of a smoother though, so perhaps the "prevent overshoot" portion should remain in RPP / controller and the jerk limit + ang vel limits could remain in the vel smoother?

I'd definitely keep "prevent overshoot" behavior inside the controller, also the motion profile when turning to heading. When it comes to angular velocity / accel / jerk limits, I guess limiting them inside the controller or inside the velocity smoother leads to same behavior?
I'm leaning a bit towards keeping all the limits inside the controller since user can set them in one location. As far as I know, DWB controller also has all the limits inside it?

If you want to move angular limit to velocity smoother and keep it only inside rotate to heading, sure we can do it if it fits user needs the most.

@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 5, 2022

I'd definitely keep "prevent overshoot" behavior inside the controller

Agreed, the slowing on the approach is the "meat" of this PR in my eyes .

Maybe it all can / should stay, but I think maybe that's better to reassess after this has a little more design work done on it so its easier to read / understand / broken out into functional chunks.

On that front:

  • Functions with explicit returns should not also have implicit reference returns as well from these modifications
  • I think each of the "chunks" added doing the profiling need to be broken out into functions of a meaningful name so they're self documenting and not simply inline
  • The pure rotation stuff should only be done in 1 place rotateToHeading but I'm perfectly happy if you updated that implementation to use the kp stuff for a smoother motion

@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2022

This pull request is in conflict. Could you fix it @angstrem98?

3 similar comments
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

This pull request is in conflict. Could you fix it @angstrem98?

@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2023

This pull request is in conflict. Could you fix it @angstrem98?

@mergify
Copy link
Contributor

mergify bot commented Sep 18, 2023

This pull request is in conflict. Could you fix it @angstrem98?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants