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

Add RosPositionCommandExecutor #173

Merged
merged 50 commits into from
Apr 22, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
8feb924
Renaming, adding an abstract BarretHandPositionCommandExecutor class
gilwoolee Apr 12, 2017
811a63b
Adding -override
gilwoolee Apr 12, 2017
dd96c10
Merge branch 'master' into SimBarrettHandCommandExecutor
gilwoolee Apr 12, 2017
83f69dd
Exposing step
gilwoolee Apr 13, 2017
cf8f104
Merge branch 'SimBarrettHandCommandExecutor' of https://github.com/pe…
gilwoolee Apr 13, 2017
9d9fb3d
Renaiming PositionCommandExecutor
gilwoolee Apr 13, 2017
648c2bf
Changing header files
gilwoolee Apr 13, 2017
a38c65a
Changing names and step function.
gilwoolee Apr 14, 2017
06dad13
Merge branch 'master' into SimBarrettHandCommandExecutor
gilwoolee Apr 14, 2017
c6462db
Update simulation speed constants and matrix->vector methods.
brianhou Apr 14, 2017
e16880e
Add collisionDetector argument. Make collideWith an optional argument.
brianhou Apr 15, 2017
ce86f93
Don't reset mCollideWith every time.
brianhou Apr 15, 2017
c90cd8d
Simplify logic of optional collision detector and collision groups.
brianhou Apr 15, 2017
2d3a5bc
Add ShapeFrames of original collision group to new collision group.
brianhou Apr 15, 2017
c7c2040
Adding RosBarrettHandPositionCommandExecutor to CMakeLists of aikido/…
Apr 15, 2017
ea130a6
Adding conversion from goal positions and joint names to sensor_msgs/…
Apr 15, 2017
eaff64c
Ros executor for hand commands
Apr 15, 2017
d9d36fc
Make collisionDetector, collideWith optional.
brianhou Apr 15, 2017
3af8671
Instantiate FingerPositionCommandExecutors and FingerSpreadCommandExe…
brianhou Apr 15, 2017
2174930
Clean up some documentation.
brianhou Apr 15, 2017
6ecb30e
Remove parameter from step.
brianhou Apr 15, 2017
c3eb6f3
Set mCollisionDetector when setting mCollideWith.
brianhou Apr 15, 2017
47a9852
Make FingerCommandExecutors implement PositionCommandExecutor.
brianhou Apr 15, 2017
bebb761
Swap argument order in HandPositionExecutor.
brianhou Apr 16, 2017
f1d805f
Partial test fixes.
brianhou Apr 16, 2017
fc7cd1c
Adding time-reset when a new execute command is given.
gilwoolee Apr 16, 2017
1eba53b
Update expected values in test to make them pass.
brianhou Apr 16, 2017
fa23b68
Merge branch 'master' into SimBarrettHandCommandExecutor
brianhou Apr 16, 2017
cc709a2
Basing this branch off SimBarrettHandCommandExecutor as it depends on it
Apr 17, 2017
90bb1c9
Renamed RosBarrettHandPositionCommandExecutor to RosPositionCommandEx…
Apr 17, 2017
db9bd59
Changed methods to match header file changes, addressed Mike's other …
Apr 17, 2017
4fc36d6
Edited CMakeLists of control/ros to reflect new filename
Apr 17, 2017
5a1059a
Edited the toJointState method
Apr 17, 2017
dc53d76
Created waitForActionServer helper function
Apr 17, 2017
d3b1647
Deleted renamed files
Apr 17, 2017
1bd2834
No longer blocked by #166
Apr 17, 2017
5e18e73
Adding detail/util-impl in the necessary convention
Apr 18, 2017
971bb6a
Taking joint names by non-const value
Apr 18, 2017
35d8144
Addressed rest of Mike
Apr 18, 2017
571bc22
Merge branch 'master' into rosBarrettHandExecutor
gilwoolee Apr 19, 2017
b89672b
Better docstring for class
Apr 21, 2017
7973137
Fixed incorrect currentTime
Apr 21, 2017
45d632b
Taking joint names by non-const value in constructor
Apr 21, 2017
5e38013
Merge branch 'rosBarrettHandExecutor' of https://github.com/personalr…
Apr 21, 2017
83a41fd
Renaming toJointState to positionsToJointState
Apr 21, 2017
e18ab71
Fixing signed with unsigned check
Apr 21, 2017
0ecb2b9
Resolve compilation issues:
jslee02 Apr 21, 2017
cd516a2
Remove debug code
jslee02 Apr 21, 2017
061e676
Add pr_control_msgs to package.xml
jslee02 Apr 21, 2017
7474bea
Clean up ros/CMakeLists.txt
jslee02 Apr 21, 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
9 changes: 9 additions & 0 deletions include/aikido/control/ros/Conversions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <memory>
#include <trajectory_msgs/JointTrajectory.h>
#include <sensor_msgs/JointState.h>
#include <aikido/statespace/dart/MetaSkeletonStateSpace.hpp>
#include <aikido/trajectory/Spline.hpp>

Expand All @@ -28,6 +29,14 @@ std::unique_ptr<aikido::trajectory::Spline> toSplineJointTrajectory(
trajectory_msgs::JointTrajectory toRosJointTrajectory(
const aikido::trajectory::TrajectoryPtr& trajectory, double timestep);

/// Converts Eigen VectorXd and joint names to JointState
/// \param[in] goalPositions The required positions for the fingers
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This is not specific to finger joints! 😉

Also, we should add "position" to the name of this function, since we may very well need equivalent functions for velocities and/or efforts. Perhaps positionsToJointState?

/// \param[in] jointNames The corresponding names of the joints
/// \return The JointState message object
sensor_msgs::JointState toJointState(
const Eigen::VectorXd& goalPositions, const std::vector<std::string> jointNames);


} // namespace ros
} // namespace control
} // namespace aikido
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Assume it has 4 positions
#ifndef AIKIDO_CONTROL_ROS_ROSBARRETTHANDPOSITIONCOMMANDEXECUTOR_HPP_
#define AIKIDO_CONTROL_ROS_ROSBARRETTHANDPOSITIONCOMMANDEXECUTOR_HPP_
#include <chrono>
#include <future>
#include <mutex>
#include <ros/ros.h>
#include <ros/callback_queue.h>
#include <sensor_msgs/JointState.h>
#include <pr_control_msgs/SetPositionAction.h>
#include <actionlib/client/action_client.h>
#include <Eigen/Dense>

namespace aikido{
namespace control {
namespace ros {

/// This class sends commands for the hand position to the ROS server
class RosBarrettHandPositionCommandExecutor
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed name and is now inheriting

{
public:
/// Constructor
/// \param[in] node ROS node handle for action client.
/// \param[in] serverName Name of the server to send traejctory to.
/// \param[in] connectionTimeout Timeout for server connection.
/// \param[in] connectionPollingPeriod Polling period for server connection.
template <typename DurationA, typename DurationB>
RosBarrettHandPositionCommandExecutor(
::ros::NodeHandle node,
const std::string& serverName,
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.

Nit: I don't think there is any value in templateing DurationA and DurationB here, since we immediately store these values in std::chrono::milliseconds member variables. To make this work, we would have to template the entire class on these two types - which seems like it is distinctly not worth it.

I believe @jslee02, @gilwoolee, and I discussed this in a previous PR. Tagging them to see if they have anything to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed template params and changed to std::chrono::milliseconds


~RosBarrettHandPositionCommandExecutor();
Copy link
Member

Choose a reason for hiding this comment

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

Either make this virtual or mark the class as final. In this case, I'd prefer the former.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made virtual


/// Sends positions to ROS server for execution.
/// \param[in] goalPositions Vector of 3 finger positions and 1 spread position
/// \param[in] jointNames Vector of 4 joint names for fingers
std::future<void> execute(Eigen::VectorXd& goalPositions,std::vector<std::string>& jointNames);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done both


/// Sends positions to ROS server for execution.
/// \param[in] goalPositions Vector of 3 finger positions and 1 spread position
/// \param[in] jointNames Vector of 4 joint names for fingers
/// \param[in] startTime Start time for the trajectory.
std::future<void> execute(
Eigen::VectorXd& goalPositions,
std::vector<std::string>& jointNames,
const ::ros::Time& startTime);
Copy link
Member

Choose a reason for hiding this comment

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

The pr_control_msgs/SetPosition action doesn't have a time parameter, so this argument is not necessary. We should remove this function and roll everything into the execute(const Eigen::VectorXd&) overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed - only a single execute method now


void step();

private:
using HandPositionActionClient
= actionlib::ActionClient<pr_control_msgs::SetPositionAction>;
using GoalHandle = HandPositionActionClient::GoalHandle;

bool waitForServer();

void transitionCallback(GoalHandle handle);

::ros::NodeHandle mNode;
::ros::CallbackQueue mCallbackQueue;
HandPositionActionClient mClient;
HandPositionActionClient::GoalHandle mGoalHandle;

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

bool mInProgress;
std::promise<void> mPromise;
Eigen::VectorXd mGoalPositions;

std::mutex mMutex;
};

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

#endif // ifndef AIKIDO_CONTROL_ROS_ROSBARRETTHANDPOSITIONCOMMANDEXECUTOR_HPP_
1 change: 1 addition & 0 deletions src/control/ros/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ set(sources
RosTrajectoryExecutionException.cpp
Conversions.cpp
RosJointStateClient.cpp
RosBarrettHandPositionCommandExecutor.cpp
)

add_library("${PROJECT_NAME}_control_ros" SHARED ${sources})
Expand Down
30 changes: 30 additions & 0 deletions src/control/ros/Conversions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,36 @@ trajectory_msgs::JointTrajectory toRosJointTrajectory(
return jointTrajectory;
}

//=============================================================================
sensor_msgs::JointState toJointState(
const Eigen::VectorXd& goalPositions, const std::vector<std::string> jointNames)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Take both arguments by const &.

{
if (goalPositions.size() != jointNames.size())
{
std::stringstream message;
message << "The size of goalPositions must be the same as jointNames!" ;
throw std::invalid_argument(message.str());
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Include goalPositions.size() and jointNames.size() in the message.

}

sensor_msgs::JointState jointState;

const auto numJoints = jointNames.size();

jointState.name.reserve(numJoints);
jointState.position.reserve(numJoints);

for (size_t i = 0; i < numJoints ; ++i)
{
jointState.name.emplace_back(jointNames[i]);
jointState.position.emplace_back(goalPositions[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: All of this code can be replaced by:

sensor_msgs::JointState jointState;
jointState.name = std::move(jointNames); // assuming jointNames is taken by value
jointState.position.assign(goalPositions.data(), goalPositions.data() + goalPositions.size());
return jointState;


return jointState;

}



} // namespace ros
} // namespace control
} // namespace aikido
181 changes: 181 additions & 0 deletions src/control/ros/RosBarrettHandPositionCommandExecutor.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
#include <aikido/control/ros/RosBarrettHandPositionCommandExecutor.hpp>
#include <aikido/control/ros/Conversions.hpp>

namespace aikido {
namespace control {
namespace ros {

using std::chrono::milliseconds;

//=============================================================================
template <typename DurationA, typename DurationB>
RosBarrettHandPositionCommandExecutor::RosBarrettHandPositionCommandExecutor(
::ros::NodeHandle node,
const std::string& serverName,
const DurationA& connectionTimeout,
const DurationB& connectionPollingPeriod)
: mNode(std::move(node))
, mCallbackQueue{}
, mClient{mNode, serverName, &mCallbackQueue}
, mConnectionTimeout{
std::chrono::duration_cast<milliseconds>(connectionTimeout)}
, mConnectionPollingPeriod{
std::chrono::duration_cast<milliseconds>(connectionPollingPeriod)}
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above about this cast. I'd prefer to eliminate the template parameters entirely and force the user to perform this conversion, if necessary.

If we do decide to keep the template parameters, then this implementation needs to be moved into a header file. By convention, it would go into a file named src/control/ros/detail/RosBarrettHandPositionCommandExecutor-impl.cpp.

, mInProgress{false}
{
}

//=============================================================================
RosBarrettHandPositionCommandExecutor::~RosBarrettHandPositionCommandExecutor()
{
// Do nothing.
// TODO: Should we wait for the current trajectory to finish executing?
}

//=============================================================================
std::future<void> RosBarrettHandPositionCommandExecutor::execute(
Eigen::VectorXd& goalPositions,
std::vector<std::string>& jointNames)
{
static const ::ros::Time invalidTime;
return execute(goalPositions, jointNames, invalidTime);
}

//=============================================================================
std::future<void> RosBarrettHandPositionCommandExecutor::execute(
Eigen::VectorXd& goalPositions,
std::vector<std::string>& jointNames,
const ::ros::Time& startTime)
{
if (goalPositions.size() != 4)
Copy link
Member

Choose a reason for hiding this comment

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

Replace the hard-coded value 4 with jointNames.size().

Copy link
Contributor Author

@Shushman Shushman Apr 17, 2017

Choose a reason for hiding this comment

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

Actually delegated the check to the toJointState message in conversions

{
std::stringstream message;
message << "GoalPositions must have 4 elements, but ["
<< goalPositions.size() << "] given.";
throw std::invalid_argument(message.str());
}

pr_control_msgs::SetPositionGoal goal;

// Convert goal positions and joint names to jointstate
goal.command = toJointState(goalPositions,jointNames);

goal.command.header.stamp = startTime;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think SetPositionGoal has a header. Are you sure this compiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as we no longer need startTime. But in general the command which is JointState does have a header.


if (!waitForServer())
throw std::runtime_error("Unable to connect to action server.");

{
std::lock_guard<std::mutex> lock(mMutex);
DART_UNUSED(lock); // Suppress unused variable warning

if (mInProgress)
throw std::runtime_error("Another trajectory is in progress.");

mPromise = std::promise<void>();
mInProgress = true;
mGoalHandle = mClient.sendGoal(goal,
boost::bind(&RosBarrettHandPositionCommandExecutor::transitionCallback, this, _1));

return mPromise.get_future();
}

}

//=============================================================================
// TODO : This is directly reused from RosTrajectoryExecutor.cpp
// Anyway to reuse this? (assuming we want this here)
Copy link
Member

Choose a reason for hiding this comment

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

I agree! Code duplication sucks. 😁

It should be straightforward to move this to a helper function:

template <class ActionSpec, class TimeoutDuration, class PeriodDuration>
bool waitForActionServer(
  actionlib::ActionClient<ActionSpec>& actionClient,
  ros::CallbackQueue* callbackQueue,
  TimeoutDuration timeoutDuration = std::chrono::seconds{ 1 },
  PeriodDuration periodDuration = std::chronos::milliseconds{ 10 }
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved this to aikido/include/control/ros/util.hpp and am calling this directly with templates in execute.

bool RosBarrettHandPositionCommandExecutor::waitForServer()
{
using Clock = std::chrono::steady_clock;

const auto startTime = Clock::now();
const auto endTime = startTime + mConnectionTimeout;
auto currentTime = startTime + mConnectionPollingPeriod;

while (currentTime < endTime)
{
mCallbackQueue.callAvailable();

// TODO: Is this thread safe?
if (mClient.isServerConnected())
return true;

currentTime += mConnectionPollingPeriod;
std::this_thread::sleep_until(currentTime);
}

return false;
}

//=============================================================================
void RosBarrettHandPositionCommandExecutor::transitionCallback(GoalHandle handle)
{

using actionlib::TerminalState;
using Result = pr_control_msgs::SetPositionResult;

if (handle.getCommState() == actionlib::CommState::DONE)
{
std::stringstream message;
bool isSuccessful = true;

const auto terminalState = handle.getTerminalState();
if (terminalState != TerminalState::SUCCEEDED)
{
message << "Action " << terminalState.toString();

const auto terminalMessage = terminalState.getText();
if (!terminalMessage.empty())
message << " (" << terminalMessage << ")";

mPromise.set_exception(std::make_exception_ptr(
std::runtime_error(message.str())));

isSuccessful = false;
}
else{
message << "Execution failed.";
}

const auto result = handle.getResult();
if (result && result->success == false)
{
if (!result->message.empty())
message << " (" << result->message << ")";

mPromise.set_exception(std::make_exception_ptr(
std::runtime_error(message.str())));

isSuccessful = false;
}

if (isSuccessful)
mPromise.set_value();

mInProgress = false;

}
}

//=============================================================================
// TODO : This is also basically copied from RosTrajectoryExecutor
void RosBarrettHandPositionCommandExecutor::step()
Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine in for now. We may want to write a base class for actionnlib-based executors in the future, but there really isn't all that much duplicated code once we move waitForServer to a helper function.

{
std::lock_guard<std::mutex> lock(mMutex);
DART_UNUSED(lock); // Suppress unused variable warning.

mCallbackQueue.callAvailable();

if (!::ros::ok() && mInProgress)
{
mPromise.set_exception(
std::make_exception_ptr(std::runtime_error("Detected ROS shutdown.")));
mInProgress = false;
}
}


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