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
4 changes: 3 additions & 1 deletion include/aikido/control/BarrettHandKinematicSimulationPositionCommandExecutor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ class BarrettHandKinematicSimulationPositionCommandExecutor
/// \return Future which becomes available when the execution completes.
std::future<void> execute(const Eigen::VectorXd& goalPositions) override;

// Documentation inherited.
/// 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 /.

void step() override;

/// Resets CollisionGroup to check collision with fingers.
Expand Down
4 changes: 3 additions & 1 deletion include/aikido/control/KinematicSimulationTrajectoryExecutor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ class KinematicSimulationTrajectoryExecutor : public TrajectoryExecutor
std::future<void> execute(
trajectory::TrajectoryPtr traj) override;

// Documentation inherited.
/// 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

void step() override;

private:
Expand Down
6 changes: 2 additions & 4 deletions src/control/KinematicSimulationTrajectoryExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ void KinematicSimulationTrajectoryExecutor::step()
}

auto timeSinceBeginning = system_clock::now() - mExecutionStartTime;
double t = duration_cast<seconds>(timeSinceBeginning).count();
auto t = duration<double>(timeSinceBeginning).count();
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 unit of time here is seconds. It would be nice to either (1) change the variable name to encode the unit or (2) add trailing comment something like // sec.

Additionally, it would be much better to comment the unit of the time parameter to the docstring of Trajectory::evaluate().

Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed to duration instead of duration_cast? I believe timeSinceBeginning is a duration, so duration_cast was appropriate here. 🤔


// Can't do static here because MetaSkeletonStateSpace inherits
// CartesianProduct which inherits virtual StateSpace
Expand All @@ -117,10 +117,7 @@ void KinematicSimulationTrajectoryExecutor::step()

mTraj->evaluate(t, state);

// Lock the skeleton, set state.
std::unique_lock<std::mutex> skeleton_lock(mSkeleton->getMutex());
space->setState(state);
skeleton_lock.unlock();

// Check if trajectory has completed.
bool const is_done = (t >= mTraj->getEndTime());
Expand All @@ -129,6 +126,7 @@ void KinematicSimulationTrajectoryExecutor::step()
mPromise->set_value();
mInExecution = false;
}

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 remove this empty line?

}

}
Expand Down