Cancel in-progress trajectories. #400

brianhou · 2018-05-07T23:16:12Z

There was a comment that suggested that this isn't supported by rewd_controllers yet, but it seems like it might be? We should test this out on the real hardware.

Before creating a pull request

Document new methods and classes
Format code with make format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md
Add unit test(s) for this change

codecov · 2018-05-08T12:36:49Z

Codecov Report

Merging #400 into master will decrease coverage by 0.08%.
The diff coverage is 38.88%.

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
- Coverage   78.98%   78.89%   -0.09%     
==========================================
  Files         258      258              
  Lines        6296     6303       +7     
==========================================
  Hits         4973     4973              
- Misses       1323     1330       +7

Impacted Files	Coverage Δ
include/aikido/control/TrajectoryExecutor.hpp	`100% <ø> (ø)`	⬆️
src/control/InstantaneousTrajectoryExecutor.cpp	`92.85% <0%> (ø)`	⬆️
src/control/ros/RosTrajectoryExecutor.cpp	`0% <0%> (ø)`	⬆️
.../control/KinematicSimulationTrajectoryExecutor.cpp	`84.5% <40%> (-2.45%)`	⬇️
src/control/QueuedTrajectoryExecutor.cpp	`93.61% <83.33%> (ø)`	⬆️

jslee02 · 2018-05-17T05:37:19Z

src/control/ros/RosTrajectoryExecutor.cpp

+  DART_UNUSED(lock); // Suppress unused variable warning.
+
+  if (mInProgress)
+    mGoalHandle.cancel();


Would it be useful to print a warning something like "Attempting to abort trajectory execution but no trajectory in progress."?

That sounds like a good idea. We should do this to the other TrajectoryExecutor::abort implementations as well.

gilwoolee · 2018-07-02T22:06:53Z

src/control/ros/RosTrajectoryExecutor.cpp

@@ -234,7 +234,11 @@ void RosTrajectoryExecutor::step(
 //==============================================================================
 void RosTrajectoryExecutor::abort()


I'd prefer "cancel" instead of "abort". It seems like "cancel" and "abort" are different terms in ros_control; "cancel" is where the client requested to cancel the trajectory, and "abort" is where the server had to stop it for some other reason (e.g. force threshold has been violated).

http://wiki.ros.org/actionlib/DetailedDescription

gilwoolee · 2018-07-02T22:08:38Z

Shouldn't we add the method to TrajectoryExecutor and support the same functionality in KinematicSimulationTrajectoryExecutor?

brianhou · 2018-07-02T22:10:41Z

I think KinematicSimulationTrajectoryExecutor already has this abort implemented (although as you said, it should probably be renamed to cancel).

My understanding is that we're not using actionlib in simulation, so this particular implementation is not helpful there.

gilwoolee · 2018-07-02T23:02:05Z

Oops- sorry I wasn't aware that the base class / KinematicSimulationTrajectoryExecutor had this method. I think it's all good as long as we rename it then!

28a7221

brianhou · 2018-08-02T22:22:16Z

@Tonkel said that they're using this in the feeding demo already, so it looks like this works! @gilwoolee or @jslee02 would you mind taking one last look before merging?

jslee02 · 2018-08-02T22:25:31Z

include/aikido/control/InstantaneousTrajectoryExecutor.hpp

@@ -43,7 +43,7 @@ class InstantaneousTrajectoryExecutor : public TrajectoryExecutor
      const std::chrono::system_clock::time_point& /*timepoint*/) override;

  // Do nothing.


Nit: /// Does nothing? Note three slashes.

jslee02 · 2018-08-02T22:26:17Z

include/aikido/control/TrajectoryExecutor.hpp

@@ -4,6 +4,7 @@
 #include <chrono>
 #include <future>
 #include <set>
+#include <dart/common/Console.hpp>


Nit: This can be moved to the cpp file.

jslee02 · 2018-08-02T22:26:38Z

include/aikido/control/ros/RosTrajectoryExecutor.hpp

@@ -58,8 +58,8 @@ class RosTrajectoryExecutor : public aikido::control::TrajectoryExecutor
  /// Regularly checks for the completion of a sent trajectory.
  void step(const std::chrono::system_clock::time_point& timepoint) override;

-  // Do nothing.
-  void abort() override;
+  // \copydoc TrajectoryExecutor::cancel()


jslee02 · 2018-08-02T22:27:10Z

src/control/ros/RosTrajectoryExecutor.cpp

+    mGoalHandle.cancel();
+  else
+    dtwarn << "[RosTrajectoryExecutor::cancel] Attempting to "
+           << "cancel trajectory, but no trajectory in progress.\n";


Nit: Please wrap multiple lines with curly brackets.

* Cancel actionlib goal. * Rename abort to cancel to match actionlib. * Add warning when no trajectory is in progress. * Update CHANGELOG.md. * Address PR comments.

Cancel actionlib goal.

df30f1c

brianhou requested review from gilwoolee and aditya-vk May 7, 2018 23:16

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

bba5d44

jslee02 added this to the Aikido 0.3.0 milestone May 9, 2018

brianhou and others added 3 commits May 9, 2018 11:23

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

4ff1bf4

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

d18d869

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

2ff827e

jslee02 reviewed May 17, 2018

View reviewed changes

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

0805f10

gilwoolee previously requested changes Jul 2, 2018

View reviewed changes

brianhou and others added 3 commits August 2, 2018 14:40

Merge branch 'master' into enhancement/brianhou/ros-traj-abort

8b5e378

Rename abort to cancel to match actionlib.

28a7221

Add warning when no trajectory is in progress.

1d1b281

Update CHANGELOG.md.

2b43da7

jslee02 previously approved these changes Aug 2, 2018

View reviewed changes

brianhou dismissed jslee02’s stale review via 722ecc8 August 3, 2018 22:28

brianhou force-pushed the enhancement/brianhou/ros-traj-abort branch from 722ecc8 to 0dbbe07 Compare August 3, 2018 22:30

Address PR comments.

9e360d2

brianhou force-pushed the enhancement/brianhou/ros-traj-abort branch from 0dbbe07 to 9e360d2 Compare August 3, 2018 22:30

jslee02 approved these changes Aug 3, 2018

View reviewed changes

brianhou merged commit de4ea8b into master Aug 4, 2018

brianhou deleted the enhancement/brianhou/ros-traj-abort branch August 4, 2018 06:30

gilwoolee pushed a commit that referenced this pull request Jan 21, 2019

Cancel in-progress trajectories. (#400)

b0a1c9a

* Cancel actionlib goal. * Rename abort to cancel to match actionlib. * Add warning when no trajectory is in progress. * Update CHANGELOG.md. * Address PR comments.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel in-progress trajectories. #400

Cancel in-progress trajectories. #400

brianhou commented May 7, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading

jslee02 May 17, 2018

brianhou May 17, 2018

gilwoolee Jul 2, 2018

gilwoolee commented Jul 2, 2018

brianhou commented Jul 2, 2018

gilwoolee commented Jul 2, 2018

brianhou commented Aug 2, 2018

jslee02 Aug 2, 2018 •

edited

Loading

jslee02 Aug 2, 2018

jslee02 Aug 2, 2018

jslee02 Aug 2, 2018

		@@ -234,7 +234,11 @@ void RosTrajectoryExecutor::step(
		//==============================================================================
		void RosTrajectoryExecutor::abort()

		@@ -43,7 +43,7 @@ class InstantaneousTrajectoryExecutor : public TrajectoryExecutor
		const std::chrono::system_clock::time_point& /timepoint/) override;

		// Do nothing.

Cancel in-progress trajectories. #400

Cancel in-progress trajectories. #400

Conversation

brianhou commented May 7, 2018 • edited Loading

codecov bot commented May 8, 2018 • edited Loading

Codecov Report

jslee02 May 17, 2018

Choose a reason for hiding this comment

brianhou May 17, 2018

Choose a reason for hiding this comment

gilwoolee Jul 2, 2018

Choose a reason for hiding this comment

gilwoolee commented Jul 2, 2018

brianhou commented Jul 2, 2018

gilwoolee commented Jul 2, 2018

brianhou commented Aug 2, 2018

jslee02 Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

jslee02 Aug 2, 2018

Choose a reason for hiding this comment

jslee02 Aug 2, 2018

Choose a reason for hiding this comment

jslee02 Aug 2, 2018

Choose a reason for hiding this comment

brianhou commented May 7, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading

jslee02 Aug 2, 2018 •

edited

Loading