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.

Sign up for GitHub

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

Removes locking from step() #182

Merged
merged 6 commits into from
Apr 22, 2017
Merged

Removes locking from step() #182

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78a86de
Removes locking from step()
gilwoolee Apr 19, 2017
1d734b2
Addressing Mike and JS's comments
gilwoolee Apr 20, 2017
4e84775
Removing empty line
gilwoolee Apr 20, 2017
38682d6
Fixing float to double
gilwoolee Apr 20, 2017
81ec3c7
Addressing Mike's comments
gilwoolee Apr 22, 2017
c3b212e
Merge branch 'master' into bugfix/KinematicSimulationTrajectoryExecutor
gilwoolee Apr 22, 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: 2 additions & 0 deletions include/aikido/control/BarrettHandKinematicSimulationPositionCommandExecutor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class BarrettHandKinematicSimulationPositionCommandExecutor
/// \return Future which becomes available when the execution completes.
std::future<void> execute(const Eigen::VectorXd& goalPositions) override;

/// \copydoc PositionCommandExecutor::step()
///
/// If multiple threads are accessing this function or the skeleton associated
/// with this executor, it is necessary to lock the skeleton before
/// calling this method.
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 believe this comment is a note to be added to the original docstring of PositionCommandExecutor::step(), but this will just overwrite the original docstring. It would be nice to copy the original docstring and add this node as:

  /// \copydoc PositionCommandExecutor::step()
  ///
  /// If multiple threads are accessing this function or the skeleton associated
  /// with this executor, it is necessary to lock the skeleton before
  /// calling this method.

Also, it would be great to actually add a docstring to PositionCommandExecutor::step(). Currently, it's // Step once. If it's enough then make it doxygen comment by prepending one more /.

Expand Down
2 changes: 2 additions & 0 deletions include/aikido/control/KinematicSimulationTrajectoryExecutor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class KinematicSimulationTrajectoryExecutor : public TrajectoryExecutor
std::future<void> execute(
trajectory::TrajectoryPtr traj) override;

/// \copydoc PositionCommandExecutor::step()
///
/// If multiple threads are accessing this function or the skeleton associated
/// with this executor, it is necessary to lock the skeleton before
/// calling this method.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Expand Down
4 changes: 2 additions & 2 deletions include/aikido/trajectory/Trajectory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ class Trajectory
/// function is implementation-defined if \c _t is not between
/// \c getStartTime() and \c getEndTime().
///
/// \param _t time parameter
/// \param[out] _state output state of the trajectory at time \c _t
/// \param _t time parameter (in seconds)
/// \param[out] _state output state of the trajectory at time \c _t (seconds)
virtual void evaluate(double _t,
statespace::StateSpace::State *_state) const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really "in seconds" -it's an arbitrary path parameter.


Expand Down
7 changes: 4 additions & 3 deletions src/control/KinematicSimulationTrajectoryExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,9 @@ void KinematicSimulationTrajectoryExecutor::step()
mInExecution = false;
}

using seconds = std::chrono::duration<float>;
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 believe the type of time should be double as Trajectory takes it as double.

auto timeSinceBeginning = system_clock::now() - mExecutionStartTime;
auto t = duration<double>(timeSinceBeginning).count();
auto tsec = duration_cast<seconds>(timeSinceBeginning).count();

// Can't do static here because MetaSkeletonStateSpace inherits
// CartesianProduct which inherits virtual StateSpace
Expand All @@ -115,12 +116,12 @@ void KinematicSimulationTrajectoryExecutor::step()
auto metaSkeleton = space->getMetaSkeleton();
auto state = space->createState();

mTraj->evaluate(t, state);
mTraj->evaluate(tsec, state);

space->setState(state);

// Check if trajectory has completed.
bool const is_done = (t >= mTraj->getEndTime());
bool const is_done = (tsec >= mTraj->getEndTime());
if (is_done) {
mTraj.reset();
mPromise->set_value();
Expand Down