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

ros_control/RosTrajectoryExecutor #149

Merged
merged 31 commits into from
Apr 14, 2017
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
43ea8bf
ros/Conversions
gilwoolee Apr 2, 2017
e3f783b
Cleaning up CMakeLists.
gilwoolee Apr 2, 2017
4391bf9
CMakeLists cleanup
gilwoolee Apr 2, 2017
338f1b7
Style change to match AIKIDO style guide.
gilwoolee Apr 2, 2017
aafa1cf
Checking joint-name matches
gilwoolee Apr 2, 2017
b013bfd
Initial commit for RosTrajectoryExecutor
gilwoolee Apr 3, 2017
793a23a
Temporarily removing tests in CMakeLists.
gilwoolee Apr 3, 2017
a2e0ed8
Merge branch 'master' into convertJointTrajectory
gilwoolee Apr 4, 2017
22ba7b4
Addressing some of Mike's comments
gilwoolee Apr 5, 2017
9e09781
Addressing Miks and JS's comments.
gilwoolee Apr 7, 2017
6714011
Merge branch 'master' into convertJointTrajectory
gilwoolee Apr 7, 2017
785871b
Minor bug fix, adding one test for joint mapping.
gilwoolee Apr 7, 2017
c98e8ea
Merge branch 'master' into convertJointTrajectory
gilwoolee Apr 7, 2017
5a655e5
Adding conversion from trajectory to ros trajectory.
gilwoolee Apr 9, 2017
d4bdaa3
Addressing Mike's comments.
gilwoolee Apr 9, 2017
300e050
Merge master and latest Conversions.cpp
gilwoolee Apr 9, 2017
d0d29dc
Line wrapping
gilwoolee Apr 9, 2017
f750b32
Removing a line.
gilwoolee Apr 9, 2017
3a10630
Merge branch 'master' into convertJointTrajectory
jslee02 Apr 11, 2017
9b9ba1e
Merge branch 'master' into RosTrajectoryExecutor
jslee02 Apr 11, 2017
064aa8a
Addressing Mike's comments
gilwoolee Apr 11, 2017
8f1a94b
Merge branch 'convertJointTrajectory' of https://github.com/personalr…
gilwoolee Apr 11, 2017
fc70347
Changing method names
gilwoolee Apr 12, 2017
4fd63b7
Merge branch 'convertJointTrajectory' of https://github.com/personalr…
gilwoolee Apr 12, 2017
65292c0
Addressing Mike and JS's comments.
gilwoolee Apr 12, 2017
6ad1d9c
Changing constructor to templated, removing reorderMap.
gilwoolee Apr 13, 2017
5db6d4e
Suppress warning
jslee02 Apr 13, 2017
6b71cb1
Merge remote-tracking branch 'origin/master' into RosTrajectoryExecutor
jslee02 Apr 13, 2017
98e8440
Fix Conversions
jslee02 Apr 13, 2017
5f0f1ae
Add missing dependencies to package.xml
jslee02 Apr 14, 2017
2537bfd
Suppress warnings
jslee02 Apr 14, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/aikido/control/TrajectoryExecutor.hpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#ifndef AIKIDO_CONTROL_TRAJECTORYEXECUTOR_HPP_
#define AIKIDO_CONTROL_TRAJECTORYEXECUTOR_HPP_
#include "TrajectoryResult.hpp"
#include "../trajectory/Trajectory.hpp"
#include <aikido/trajectory/Trajectory.hpp>
#include <future>

namespace aikido {
Expand Down
38 changes: 38 additions & 0 deletions include/aikido/control/ros/Conversions.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#ifndef AIKIDO_CONTROL_ROS_CONVERSIONS_HPP_
#define AIKIDO_CONTROL_ROS_CONVERSIONS_HPP_
#include <memory>
#include <trajectory_msgs/JointTrajectory.h>
#include <aikido/statespace/dart/MetaSkeletonStateSpace.hpp>
#include <aikido/trajectory/Spline.hpp>
#include <map>

namespace aikido {
namespace control {
namespace ros {

/// Converts a ROS JointTrajectory into an aikido's Spline trajectory.
/// This method only handles single-DOF joints.
/// \param[in] _space MetaSkeletonStateSpace for Spline trajectory.
// Subspaces must be either 1D RnJoint or SO2Joint.
/// \param[in] _jointTrajectory ROS JointTrajectory to be converted.
/// \return Spline trajectory.
std::unique_ptr<aikido::trajectory::Spline> convertJointTrajectory(
const std::shared_ptr<
aikido::statespace::dart::MetaSkeletonStateSpace>& space,
const trajectory_msgs::JointTrajectory& jointTrajectory);

/// Converts Aikido Trajectory to ROS JointTrajectory.
/// Supports only 1D RnJoints and SO2Joints.
/// \_param[in] trajectory Aikido trajectory to be converted.
/// \_param[in] indexMap Mapping between trajectory's joints and ros joints.
/// \_param[in] timestep Timestep between two consecutive waypoints.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please remove the leading underscores.

const trajectory_msgs::JointTrajectory convertTrajectoryToRosTrajectory(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Returning const qualifier for non-reference rvalue is meaningless. Related SO posts: 1, 2

Copy link
Member

Choose a reason for hiding this comment

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

I would like to suggest patterning the two convert function names. convertJointTrajectory() doesn't convey the original and target type in the function name unlike to the latter function name, and convertTrajectoryToRosTrajectory() uses only Trajectory to refer JointTrajectory unlike to the former function name.

We could put both of original and target type name to the function name like convertJointTrajectoryToRosJointTrajectory() but this might be too long. So my preference is including only the goal type to the function name so that we can call the correct conversion functions depending on the target type. We don't need to worry about the original type since functions can overload for the argument types, which is the original type, but not for the return type.

TL;DR: I suggest renaming the two function names to something like:

convertToSplineJointTrajectory()
convertToRosJointTrajectory()

// or
toSplineJointTrajectory()
toRosJointTrajectory()

const aikido::trajectory::TrajectoryPtr& trajectory,
const std::map<std::string, size_t>& indexMap, double timestep);

} // namespace ros
} // namespace control
} // namespace aikido

#endif // ifndef AIKIDO_CONTROL_ROS_CONVERSIONS_HPP_

33 changes: 33 additions & 0 deletions include/aikido/control/ros/RosTrajectoryExecutionException.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#ifndef AIKIDO_CONTROL_ROS_ROSTRAJECTORYEXECUTIONEXCEPTION_HPP_
#define AIKIDO_CONTROL_ROS_ROSTRAJECTORYEXECUTIONEXCEPTION_HPP_
#include <exception>
#include <actionlib/client/action_client.h>
#include <control_msgs/FollowJointTrajectoryAction.h>

namespace aikido {
namespace control {
namespace ros {

/// This class wraps various exceptions that may arise during trajectory
/// execution over ROS.
class RosTrajectoryExecutionException: public std::runtime_error
{
public:

RosTrajectoryExecutionException(
const std::string& what,
actionlib::TerminalState terminalState);
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this to take const reference to avoid unnecessary copy?

Copy link
Member

Choose a reason for hiding this comment

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

TerminalState is an enum, so there is no point in avoiding a copy here.


RosTrajectoryExecutionException(
const std::string& what,
int result);

virtual ~RosTrajectoryExecutionException() = default;

};

} // ros
} // control
} // aikido

#endif
95 changes: 95 additions & 0 deletions include/aikido/control/ros/RosTrajectoryExecutor.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#ifndef AIKIDO_CONTROL_ROS_ROSTRAJECTORYEXECUTOR_HPP_
#define AIKIDO_CONTROL_ROS_ROSTRAJECTORYEXECUTOR_HPP_
#include <chrono>
#include <future>
#include <mutex>
#include <ros/ros.h>
#include <ros/callback_queue.h>
#include <control_msgs/FollowJointTrajectoryAction.h>
#include <aikido/control/TrajectoryExecutor.hpp>
#include <aikido/statespace/dart/MetaSkeletonStateSpace.hpp>
#include <aikido/trajectory/Trajectory.hpp>
#include <actionlib/client/action_client.h>

namespace aikido {
namespace control {
namespace ros {

/// This class sends trajectory commands to ROS server.
class RosTrajectoryExecutor : public aikido::control::TrajectoryExecutor
{
public:
/// Constructor.
/// \param[in] space Space in which trajectries operate.
/// \param[in] node ROS node handle for action client.
/// \param[in] serverName Name of the server to send traejctory to.
/// \param[in] timestep Step size for interpolating trajectories.
/// \param[in] goalTimeTolerance
/// \param[in] indexMap Joint index mapping from statespace to ros trajectory.
/// \param[in] connectionTimeout Timeout for server connection.
/// \param[in] connectionPollingPeriod Polling period for server connection.
RosTrajectoryExecutor(
statespace::dart::MetaSkeletonStateSpacePtr space,
::ros::NodeHandle node,
const std::string& serverName,
double timestep,
double goalTimeTolerance,
const std::map<std::string, size_t>& jointIndexMap,
std::chrono::milliseconds connectionTimeout
= std::chrono::milliseconds{1000},
std::chrono::milliseconds connectionPollingPeriod
= std::chrono::milliseconds{20}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Rename this to connectionPollingPeriod.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to suggest templatizing this constructor for the each time-related types so that we could pass any of time units. Here is a similar example. In this case, however, we would like to templatize for two possibly different duration types like:

template <typename DurationA, typename DurationB>
RosTrajectoryExecutor(...,
  const DurationA& connectionTimeout = std::chrono::milliseconds{1000},
  const DurationB& connectionPollingPeriod = std::chrono::milliseconds{20});

Copy link
Member

Choose a reason for hiding this comment

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

I am generally supportive of this, but I am not sure how to implement it. We store these durations in member variables, so this would require templateing the entire class on DurationA and DurationB. It's possible we could do some type erasure shenaniganz to avoid this, but I don't think it's worth the effort at this point.

Copy link
Member

Choose a reason for hiding this comment

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

We actually don't need to templatize the whole class but only this constructor. The member variable can be any specific duration type. Here is an example what I did: member variable, templated constructor.

);

virtual ~RosTrajectoryExecutor();

/// Sends trajectory to ROS server for execution.
/// \param[in] traj Trajecotry to be executed.
std::future<void> execute(trajectory::TrajectoryPtr traj) override;

/// Sends trajectory to ROS server for execution.
/// \param[in] traj Trajectrory to be executed.
/// \param[in] startTime Start time for the trajectory.
std::future<void> execute(
trajectory::TrajectoryPtr traj, const ::ros::Time& startTime);

/// To be executed on a separate thread.
/// Regularly checks for the completion of a sent trajectory.
void spin();
Copy link
Member

Choose a reason for hiding this comment

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

We should refactor this function into a spinOnce-style callback that we can call from the Executor implemented in #151. That is potentially important because it lets us consolidate all of our various callbacks, e.g. multiple RosTrajectoryExecutors and JointStateSubscribers, into a single thread.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind! It turns out that spin() is already implemented that way. 😅

Maybe we should rename this to something more obvious? Maybe step() or tick()?


private:
using TrajectoryActionClient
= actionlib::ActionClient<control_msgs::FollowJointTrajectoryAction>;
using GoalHandle = TrajectoryActionClient::GoalHandle;

bool waitForServer();

void transitionCallback(GoalHandle handle);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we pass const GoalHandle&?

Copy link
Member

Choose a reason for hiding this comment

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

Nope. This function is passed as a callback to ActionClient::sendGoal, which expects this signature.


statespace::dart::MetaSkeletonStateSpacePtr mSpace;
::ros::NodeHandle mNode;
::ros::CallbackQueue mCallbackQueue;
TrajectoryActionClient mClient;
TrajectoryActionClient::GoalHandle mGoalHandle;

std::map<std::string, size_t> mJointIndexMap;

double mTimestep;
double mGoalTimeTolerance;

std::chrono::milliseconds mConnectionTimeout;
std::chrono::milliseconds mConnectionPollingPeriod;

bool mInProgress;
std::promise<void> mPromise;
trajectory::TrajectoryPtr mTrajectory;

std::mutex mMutex;
};

} // namespace ros
} // namespace control
} // namespace aikido

#endif // ifndef AIKIDO_CONTROL_ROS_ROSTRAJECTORYEXECUTOR_HPP_

2 changes: 2 additions & 0 deletions src/control/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ add_component_targets(${PROJECT_NAME} control "${PROJECT_NAME}_control")
add_component_dependencies(${PROJECT_NAME} control statespace trajectory)

coveralls_add_sources(${sources})

add_subdirectory("ros") # Dependencies: control, statespace, trajectory
57 changes: 57 additions & 0 deletions src/control/ros/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#==============================================================================
# Dependencies
#
find_package(trajectory_msgs QUIET)
aikido_check_package(trajectory_msgs "aikido::control::ros" "trajectory_msgs")

# TODO: Workaround because find_package(actionlib) imports "tests" and "run_tests"
# targets that conflict with the targets defined by Aikido.
find_path(actionlib_INCLUDE_DIRS actionlib PATHS "/opt/ros/indigo/include")
find_library(actionlib_LIBRARIES actionlib PATHS "/opt/ros/indigo/lib")
evaluate_condition(actionlib_FOUND actionlib_INCLUDE_DIRS AND actionlib_LIBRARIES)
#find_package(actionlib QUIET)
aikido_check_package(actionlib "aikido::control::ros" "actionlib")

find_path(control_msgs_INCLUDE_DIRS control_msgs PATHS "/opt/ros/indigo/include")
#find_library(control_msgs_LIBRARIES control_msgs PATHS "/opt/ros/indigo/lib")
evaluate_condition(control_msgs_FOUND control_msgs_INCLUDE_DIRS)
aikido_check_package(control_msgs "aikido::control::ros" "control_msgs")

find_package(roscpp QUIET)
aikido_check_package(roscpp "aikido::control::ros" "roscpp")

#==============================================================================
# Libraries
#
set(sources
RosTrajectoryExecutor.cpp
RosTrajectoryExecutionException.cpp
Conversions.cpp
)

add_library("${PROJECT_NAME}_control_ros" SHARED ${sources})
target_include_directories("${PROJECT_NAME}_control_ros" SYSTEM
PUBLIC
${trajectory_msgs_INCLUDE_DIRS}
${actionlib_INCLUDE_DIRS}
${control_msgs_INCLUDE_DIRS}
${roscpp_INCLUDE_DIRS}
)

target_link_libraries("${PROJECT_NAME}_control_ros"
"${PROJECT_NAME}_control"
"${PROJECT_NAME}_statespace"
"${PROJECT_NAME}_trajectory"
${DART_LIBRARIES}
${trajectory_msgs_LIBRARIES}
${actionlib_LIBRARIES}
${control_msgs_LIBRARIES}
${roscpp_LIBRARIES}
)

add_component(${PROJECT_NAME} control_ros)
add_component_targets(${PROJECT_NAME} control_ros "${PROJECT_NAME}_control_ros")
add_component_dependencies(${PROJECT_NAME} control_ros control statespace trajectory)

coveralls_add_sources(${sources})

Loading