add Queue handling in solver_trajectory_subscriber#20
add Queue handling in solver_trajectory_subscriber#20Sergim96 wants to merge 5 commits intoRobotMotorIntelligence:develfrom
Conversation
cmastalli
left a comment
There was a problem hiding this comment.
The PR is quite complete and well-developed. However, there are a few aspects to improve:
- Allocate date for merged objects,
- Support unit tests for ROS2.
This latter point is the most relevant in my opinon. Also, because we need to kill our ROS1 CI. Can you also handle this in this PR?
| std::deque<double> ts_queue_; | ||
| std::deque<double> dts_queue_; | ||
| std::deque<Eigen::VectorXd> xs_queue_; | ||
| std::deque<Eigen::VectorXd> dxs_queue_; | ||
| std::deque<Eigen::VectorXd> us_queue_; | ||
| std::deque<Eigen::MatrixXd> Ks_queue_; | ||
| std::deque<crocoddyl_msgs::ControlType> types_queue_; | ||
| std::deque<crocoddyl_msgs::ControlParametrization> params_queue_; |
There was a problem hiding this comment.
Let's allocate data for merged objects as well.
| // std::cout << "Adding new message with initial time: " << | ||
| // msg->intervals[0].time << std::endl; |
There was a problem hiding this comment.
yes sorry! I forgot to remove that
| if (is_reduced_model_) | ||
| updateBodyInertialParameters(reduced_model_, body_name, psi); |
There was a problem hiding this comment.
Could we add some unit tests for all these cases?
There was a problem hiding this comment.
they already are see here
crocoddyl_msgs/unittest/test_whole_body_state.py
Lines 406 to 523 in 523aa2d
| else() # ROS2 | ||
| # ----------------------- | ||
| # Python Tests (ROS2) | ||
| # ----------------------- |
There was a problem hiding this comment.
We are not running test_solver_queue on ROS2, which should be our first support.
|
|
||
| typedef std::chrono::milliseconds ms; | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
Weird code. No name for this namespace.
There was a problem hiding this comment.
Your namespace doesn't have a name.
| ASSERT_NEAR(t, ts[index], 1e-9); | ||
| ASSERT_NEAR(dt, dts[index], 1e-9); | ||
| ASSERT_TRUE(x.isApprox(xs[index], 1e-9)); | ||
| ASSERT_TRUE(dx.isApprox(dxs[index], 1e-9)); | ||
| ASSERT_TRUE(u.isApprox(us[index], 1e-9)); | ||
| ASSERT_TRUE(K.isApprox(Ks[index], 1e-9)); |
There was a problem hiding this comment.
Do we need to use 1e-9 as a tolerance? I believe we can remove the tolerance for each test case.
|
I have pushed some minor changes that I preferred not to comment on. |
|
@Sergim96 -- please ping me when you're done with my comments. |
|
Hi @cmastalli I don't know what happened but the test is getting stuck in some kind of endless loop now, are you able to run it? |
|
Hey @Sergim96! I am swamped in this period. Could you be more specific? Is this happening on your PC? |
Hi @cmastalli this PR brings the queue concept to crocoddyl_msgs. As you know, this feature is needed for our MPC and bringing it to crocoddyl_msgs lets us:
The behaviour of the queue is well-documented and should be self-explanatory, I believe it is extensively unittested so we are confident it is working correctly, I had to write the unittest in c++ as
ros.timefrom python is inconsistent with the c++ one, making the unittest unreliable.in addition it fixes a minor bug in the updateBodyInertialParameters of the reduced model.
Let me known your thoughts!