ros_control/Executor #151
ros_control/Executor #151
Conversation
There was a problem hiding this comment.
Like @gilwoolee said, this is missing documentation and unit tests. Otherwise, it generally looks reasonable.
src/util/ExecutorThread.cpp
Outdated
| catch (const std::exception& e) | ||
| { | ||
| std::cerr << "Exception thrown by callback: " << e.what() << std::endl; | ||
| mIsRunning.store(false); |
There was a problem hiding this comment.
Nit: add a break here so we don't sleep_until after encountering an error.
There was a problem hiding this comment.
I don't have much experience in multithreaded programming so this might be a silly question: Shouldn't we throw e here instead so that the caller can handle the exception?
| } | ||
| catch (const std::exception& e) | ||
| { | ||
| std::cerr << "Exception thrown by callback: " << e.what() << std::endl; |
There was a problem hiding this comment.
Nit: We should find another way to handle this error, so we don't print directly to std::cerr. Unfortunately, I don't have an immediately better solution since Aikido doesn't use any particular logging framework.
There was a problem hiding this comment.
Yeah. I think we'll have to keep it as a TODO for now. #155
|
Added documentation and tests |
|
Changes Unknown when pulling 0ec48ea on ros_Executor into ** on master**. |
| } | ||
| catch (const std::exception& e) | ||
| { | ||
| std::cerr << "Exception thrown by callback: " << e.what() << std::endl; |
There was a problem hiding this comment.
Yeah. I think we'll have to keep it as a TODO for now. #155
|
Looks good! Thanks @jslee02 @mkoval @ClintLiddick for all your previous and current work on this. I'll merge it once Travis passes. :D |
| namespace aikido { | ||
| namespace util { | ||
|
|
||
| /// ExecutorMultiplexer stores callbacks and calls them in order of they added. |
There was a problem hiding this comment.
The fact that callbacks are called in the order they are added isn't important. My original intent for implementing this class was to allow one ExecutorThread to call multiple executors. The purpose of ExecutorThread is to periodically call "spin" functions, like those in RosJointStateClient and RosTrajectoryExecutor.
I'd like to capture this "big picture" idea here. Here's my attempt at a new docstring:
Combine multiple executors (i.e. no argument callbacks) into one executor.
This helper class allows one `ExecutorThread to call multiple executors by sequentially calling the callbacks added to this class.
It is not critical, but I would slightly prefer to remove all of the comments that specify the order the callbacks will be called in. The fact that they are called in order is an implementation detail that is subject to change - and may very well need to change if we add a removeCallback helper function to this class.
| /// | ||
| /// \code | ||
| /// ExecutorThread exec( | ||
| /// []() { std::cout << "running\n";}, std::chrono::nanoseconds(1)); |
There was a problem hiding this comment.
This is a scary example because printing running will likely take longer than 1 ns. Can you change the example to something more realistic, e.g. 10 ms?
| /// | ||
| /// // thread is running | ||
| /// | ||
| /// exec.stop(); |
There was a problem hiding this comment.
I suggest removing stop() from this example code, since ExecutorThread is intended to be used primarily as an RAII class. In fact, I'd like to mention the fact that the destructor stops the thread in this comment.
tests/util/test_Executor.cpp
Outdated
|
|
||
| std::this_thread::sleep_for(std::chrono::seconds(3)); | ||
|
|
||
| EXPECT_TRUE(!exec.isRunning()); |
There was a problem hiding this comment.
This is technically a race condition, but I don't see a better way of implementing it. I suppose the 3 second sleep is essentially a timeout for the test.
|
Thanks for the comments @mkoval . Now I have a better understanding of the classes' purpose. Let's merge this once Travis CI becomes happy. |
Needs documentation, review, and unit-tests.