Removes locking from step() #182
Conversation
| // 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. |
There was a problem hiding this comment.
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 /.
| // 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. |
| @@ -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(); | |||
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Why was this changed to duration instead of duration_cast? I believe timeSinceBeginning is a duration, so duration_cast was appropriate here. 🤔
| @@ -129,6 +126,7 @@ void KinematicSimulationTrajectoryExecutor::step() | |||
| mPromise->set_value(); | |||
| mInExecution = false; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nit: Could we remove this empty line?
| @@ -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(); | |||
There was a problem hiding this comment.
Why was this changed to duration instead of duration_cast? I believe timeSinceBeginning is a duration, so duration_cast was appropriate here. 🤔
| @@ -105,8 +105,9 @@ void KinematicSimulationTrajectoryExecutor::step() | |||
| mInExecution = false; | |||
| } | |||
|
|
|||
| using seconds = std::chrono::duration<float>; | |||
There was a problem hiding this comment.
Nit: I believe the type of time should be double as Trajectory takes it as double.
| /// \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; |
There was a problem hiding this comment.
This isn't really "in seconds" -it's an arbitrary path parameter.
| @@ -105,8 +105,9 @@ void KinematicSimulationTrajectoryExecutor::step() | |||
| mInExecution = false; | |||
| } | |||
|
|
|||
| using seconds = std::chrono::duration<double>; | |||
There was a problem hiding this comment.
This is a potentially confusing alias, because std::chrono::seconds stores integers and this alias stores doubles.
KinematicSimulationTrajectoryExecutor::step()should no longer lock the skeleton and let the caller be responsible for locking if it's running under multi-threaded setting. This is consistent with other executors.