Full review #2

Timple · 2021-08-16T08:28:46Z

PR for commenting on the complete codebase.

Robustify against negative dt values

…pplication

Make init-vel threshold tunable so users can pick a value for their application

ros-path_tracking_pid 2.16.0 81b78b9 ros-roslint 0.12.0

2.16.0

Industrial ci

Fix backwards cancel bug and add test for it

…ion values

rokusottervanger

See the comments. Additionally, I'm curious as to what the rationale is behind the separation between the controller.cpp file and the path_tracking_pid.cpp file. Could you elaborate?

cfg/Pid.cfg

include/path_tracking_pid/controller.hpp

rokusottervanger · 2022-01-25T10:57:45Z

launch/path_tracking_pid_mbf.launch

+        <rosparam file="$(find path_tracking_pid)/test/local_costmap_params.yaml" command="load" />
+    </node>
+
+</launch>


This file tries to launch mbf_costmap_nav from move_base_flex, but it is not a dependency. You may not want to add a dependency on move_base_flex to this package. But instead, you could make a separate package in this repo containing this applied example and its configuration, with dependencies on path_tracking_pid and mbf_costmap_nav. But I'm open to alternatives.

Or perhaps add it as a test dependency?

This should have been tackled by the roslaunch checker.
Should be an exec_depend

This type of errors will be caught by a proper CI. Reason to set this up?

src/path_tracking_pid_local_planner.cpp

rokusottervanger · 2022-01-25T11:06:25Z

src/path_tracking_pid_local_planner.cpp

+      // 'Project' plan by removing z-component
+      pose_stamped.pose.position.z = 0.0;
+    }
+  }


The function above checks for compatibility of the plan's frame and the frame the controller is running in. This makes assumptions about the application. Does the user want you to control in the plan's frame, or do they want the controller to convert the plan to the controller's frame? There's no way of knowing.

Keep the the controller (and the entire ROS system) clean by assuming you're running in the standardized frames (map, odom, or maybe utm or something). In this case you're using the frame specified for move_base_flex. If the user wants to be flexible with respect to the reference frames of navigation plans, they should transform any incoming plans to the move_base_flex global frame in a separate rosnode with the isolated functionality of providing this flexible interface.

There seems to be a discussion about this topic on ROS2. We will check this and decide later on how to proceed.

src/path_tracking_pid_local_planner.cpp

rokusottervanger · 2022-01-25T12:14:31Z

src/controller.cpp

+  controller_state_.current_x_vel = new_x_vel;
+  controller_state_.current_yaw_vel = new_yaw_vel;
+  return output_combined;
+}


This single function is nearly 470 lines of code. Split this up.

Sounds good. We should probably enable sonarcloud on this repository as well. It gives indications like cyclomatic-complexity of the code. And maximum number of seperate statements in functions.

This refactor is in progress. Should I raise an issue about CI missing?

Timple · 2022-01-25T12:58:15Z

As I'm not the original (nor the main) author here, I didn't address all comments 🙂

rokusottervanger

~~I'm losing confidence in the viability of this controller for the long term... It takes a complete reimplementation to fix this.~~

src/controller.cpp

rokusottervanger · 2022-01-27T16:22:55Z

src/controller.cpp

+  // When velocity error is too big reset current_x_vel
+  if (fabs(odom_twist.linear.x - controller_state_.current_x_vel) > max_error_x_vel_)
+  {
+    // TODO(clopez/mcfurry/nobleo): Give feedback to higher level software here
+    ROS_WARN("Large control error. Current_x_vel %f / odometry %f",
+            controller_state_.current_x_vel, odom_twist.linear.x);
+  }


"reset current_x_vel"? I don't see this happening...

There was a reset at some point, but then it introduced a undesired behavior in one of the applications, we should remove or rewrite the comment.

We can get rid of this condition and warning, because it is doesn't do anything functionally, and a similar check is performed in the update method here:

path_tracking_pid/src/controller.cpp

Lines 642 to 649 in 61c6cc8

// When velocity error is too big reset current_x_vel

if (

fabs(odom_twist.linear.x) < fabs(current_target_x_vel_) &&

fabs(odom_twist.linear.x - new_x_vel) > config_.max_error_x_vel) {

// TODO(clopez/mcfurry/nobleo): Give feedback to higher level software here

ROS_WARN_THROTTLE(

1.0, "Large tracking error. Current_x_vel %f / odometry %f", new_x_vel, odom_twist.linear.x);

}

Rayman · 2022-01-28T10:43:31Z

include/path_tracking_pid/controller.hpp

+
+  // Cutoff frequency for the derivative calculation in Hz.
+  // Negative -> Has not been set by the user yet, so use a default.
+  double cutoff_frequency_long_ = -1.0;


Unused private field

Rayman · 2022-01-28T10:44:08Z

include/path_tracking_pid/controller.hpp

+  double feedforward_ang_ = 0.0;
+  double xvel_ = 0.0;
+  double yvel_ = 0.0;
+  double thvel_ = 0.0;


Unused private fields xvel_, yvel_ and thvel_

Rayman · 2022-01-28T10:59:49Z

include/path_tracking_pid/path_tracking_pid_local_planner.hpp

+  {
+    std::vector<polygon_t> output_vec;
+    boost::geometry::union_(polygon1, polygon2, output_vec);
+    return output_vec.at(0);  // Only first vector element is filled


The union of two polygons can be two polygons if there is no overlap. So this funciton always throws away the second

Ah, good point. We expect them to overlap though, so we should probably warn on size > 1

Rayman · 2022-01-28T11:04:33Z

src/path_tracking_pid_local_planner.cpp

+  nh.param<std::string>("base_link_frame", base_link_frame_, "base_link");
+
+  nh.param<bool>("use_tricycle_model", use_tricycle_model_, false);
+  nh.param<std::string>("steered_wheel_frame", steered_wheel_frame_, "steer");


Why are these parameters not part of the dynamic reconfigure set?

Vehicles rarely get a fourth wheel on runtime 🙂

And frame-names are assumed to be static in ROS mostly?

For me the cfg file is very useful to get a quick look at which parameters are available. It would be nice if all parameters would be described in it. Even if some parameters won't change on runtime

Can be done, but then the code should cope with changing them.

Cleanup Controller::update().

Replaced path pose parameter with return value.

…onst Made Controller::findPositionOnPlan() const.

…-data-member Replaced distance_to_goal_ data member with local.

…ndex Moved last visited pose index to find result.

…-parameter Replaced controller_state parameter of findPositionOnPlan().

Renamed position to pose for findPositionOnPlan.

Replaced Controller::getControllerState().

…g-backwards fix: Use absolute stopping time to calculate end phase distance

Move & simplify getControlPointPose

ros-dynamic_reconfigure 1.7.2 ros-path_tracking_pid 2.19.0 ff653a6 ros-roslint 0.12.0

[AStapler] converter node

The old gpl license might make us vulnerable to oudside contributers, as discussed in the tech share meeting. Let's relicense under Apache-2.0 to also address some potential patent related issues. Relicensing requires the consent of all copyright holders. I’ve tracked down all contributions, and they were all done by people working at Nobleo. This means Nobleo is the sole copyright holder.

Relicense the code under Apache-2.0

Timple added 2 commits August 3, 2021 13:47

Robustify against negative dt values

f0ea20f

Merge pull request #1 from nobleo/bugfix/HARVEY-812-negative-dt

a8f45bd

Robustify against negative dt values

Timple requested a review from cesar-lopez-mar August 16, 2021 08:28

Ferry Schoenmakers and others added 14 commits August 16, 2021 17:03

Make init-vel threshold tunable so users can pick a value for their a…

727ad5f

…pplication

Merge pull request #3 from nobleo/fix/init_vel_threshold_tunable

81b78b9

Make init-vel threshold tunable so users can pick a value for their application

2.16.0

86e2dcf

ros-path_tracking_pid 2.16.0 81b78b9 ros-roslint 0.12.0

Merge pull request #4 from nobleo/release/2.16.0

c6edd9c

2.16.0

Enable industrial CI

503603d

See if noetic fails as well

5bb319c

Relax the criterium a bit

388bc15

Merge pull request #5 from nobleo/industrial-ci

709a2d2

Industrial ci

Fix backwards cancel bug and add test for it

aed0080

Fix test!?

ada695d

Merge pull request #7 from nobleo/fix/HARVEY-855-cancel-on-backward-turn

00202b4

Fix backwards cancel bug and add test for it

Fix acceleration/deceleration switching in reverse driving

18146c1

Add ugly test for checking forward and reverse acceleration/decelerat…

fa60109

…ion values

Slight test improvements

8876ba8

Timple mentioned this pull request Jan 24, 2022

Code review #12

Closed

rokusottervanger requested changes Jan 25, 2022

View reviewed changes

Ferry Schoenmakers added 5 commits January 25, 2022 14:09

Change accel/decel check in test

c26e5cf

Try fix test?

39c0a10

This seems to fix the tests?

bdf0057

using sign() function here is not necessary...

6bb3f8b

sign() function not needed here

e6f4fcb

rokusottervanger reviewed Jan 27, 2022

View reviewed changes

Rayman reviewed Jan 28, 2022

View reviewed changes

lewie-donckers and others added 26 commits March 15, 2022 11:51

Cleanup Controller::update().

b084218

Replaced path pose parameter with return value.

be0176d

Made Controller::findPositionOnPlan() const.

e9701f3

Replaced distance_to_goal_ data member with local.

9d4b602

Moved last visited pose index to find result.

b7e143c

Replaced controller_state parameter of findPositionOnPlan().

3ac6955

Replaced Controller::getControllerState().

cd11f02

Renamed position to pose for findPositionOnPlan.

a9db24d

Merge pull request #135 from nobleo/fix/123-cleanup-update

c0e7d15

Cleanup Controller::update().

Merge pull request #136 from nobleo/fix/123-replace-path-pose-parameter

13aad0a

Replaced path pose parameter with return value.

Merge pull request #137 from nobleo/fix/123-made-findPositionOnPlan-c…

536a557

…onst Made Controller::findPositionOnPlan() const.

Merge pull request #138 from nobleo/fix/123-replaced-distance-to-goal…

0e68580

…-data-member Replaced distance_to_goal_ data member with local.

Merge pull request #139 from nobleo/fix/123-moved-last-visited-pose-i…

010f296

…ndex Moved last visited pose index to find result.

Merge pull request #140 from nobleo/fix/123-replaced-controller-state…

746ccbe

…-parameter Replaced controller_state parameter of findPositionOnPlan().

Merge pull request #144 from nobleo/fix/123-renamed-position-to-pose

497e790

Renamed position to pose for findPositionOnPlan.

Merge pull request #141 from nobleo/fix/124-replace-getControllerState

dbe631d

Replaced Controller::getControllerState().

Move getControlPointPose to a separate function & test

e10e6b0

Simplify closestPoseOnSegment math

e3bd43c

fix: Use absolute target velocity to calculate end phase distance

d5198be

fix: Enforce positive end phase time and simplify end phase distance

ec82e90

Merge pull request #150 from nobleo/fix/end-phase-distance-for-drivin…

8f26afc

…g-backwards fix: Use absolute stopping time to calculate end phase distance

Merge pull request #148 from nobleo/refactor/closest-pose-on-segment

1cd31ea

Move & simplify getControlPointPose

Refactor projectedCollisionCost to input-output function

4d51759

Make projectedCollisionCost static

1512fbc

Factored out creation of projected footprint

28d90cc

2.19.0

826bd16

ros-dynamic_reconfigure 1.7.2 ros-path_tracking_pid 2.19.0 ff653a6 ros-roslint 0.12.0

fmessmer added a commit to fmessmer/path_tracking_pid that referenced this pull request May 5, 2022

Merge pull request nobleo#2 from LoyVanBeek/feature/standalone_node

9f88a35

[AStapler] converter node

Rayman added 2 commits January 9, 2023 16:03

Merge pull request #157 from nobleo/relicense-apache-v2

750f8c1

Relicense the code under Apache-2.0

Timple closed this Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full review #2

Full review #2

Timple commented Aug 16, 2021

rokusottervanger left a comment

rokusottervanger Jan 25, 2022

cesar-lopez-mar Feb 3, 2022

Timple Feb 15, 2022

rokusottervanger Feb 23, 2022

rokusottervanger Jan 25, 2022

cesar-lopez-mar Feb 15, 2022

rokusottervanger Jan 25, 2022

Timple Jan 25, 2022

rokusottervanger Feb 23, 2022

Timple commented Jan 25, 2022

rokusottervanger left a comment •

edited

Loading

rokusottervanger Jan 27, 2022

cesar-lopez-mar Feb 3, 2022

rokusottervanger Feb 23, 2022

Rayman Jan 28, 2022

Rayman Jan 28, 2022

Rayman Jan 28, 2022

Timple Jan 28, 2022

Rayman Jan 28, 2022

Timple Jan 28, 2022

Rayman Jan 28, 2022

Timple Jan 28, 2022

	// When velocity error is too big reset current_x_vel
	if (
	fabs(odom_twist.linear.x) < fabs(current_target_x_vel_) &&
	fabs(odom_twist.linear.x - new_x_vel) > config_.max_error_x_vel) {
	// TODO(clopez/mcfurry/nobleo): Give feedback to higher level software here
	ROS_WARN_THROTTLE(
	1.0, "Large tracking error. Current_x_vel %f / odometry %f", new_x_vel, odom_twist.linear.x);
	}

Full review #2

Full review #2

Conversation

Timple commented Aug 16, 2021

rokusottervanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timple commented Jan 25, 2022

rokusottervanger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokusottervanger left a comment •

edited

Loading