Filling index field in feedback message of the action interface

[GiuseppeMonetti95]

Hi everyone!

I took this task from the good-first-issue here: #1742

According to the new field description in the .action file, this looks like working for me.
The only doubt I have is about testing, I don't know if this change is worth a test case.

Looking forward to have reviews 😄

[christophfroehlich]

Thanks a lot. Except for the conversion error below, this looks very good. Testing in existing testcases is fine.

   /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/joint_trajectory_controller/src/joint_trajectory_controller.cpp:239:37: error: implicit conversion changes signedness: 'typename iterator_traits<__normal_iterator<const JointTrajectoryPoint_<allocator<void>> *, vector<JointTrajectoryPoint_<allocator<void>>>>>::difference_type' (aka 'long') to 'const size_t' (aka 'const unsigned long') [-Werror,-Wsign-conversion]
    239 |     const size_t next_point_index = std::distance(current_trajectory_->begin(), end_segment_itr);
        |                  ~~~~~~~~~~~~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[GiuseppeMonetti95]

Implicit conversion problem has been fixed. However, I had a test failure that I was not capable to reproduce in the little time I had today: Even if the time_from_start was less then the one of the next via point in the trajectory, in the feedback message there was already the next point.
I have a feeling that it may be due to path and/or goal tolerances. I will investigate more.
Does the problem description sound familiar? Do you have a guess such that I can find faster the problem?

[christophfroehlich]

I have a feeling that it may be due to path and/or goal tolerances. I will investigate more.

I don't think that this relates, I can't imagine now why this should happen.

but the failing jazzy test now shows the same?

   <<< failure message
    /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_actions.cpp:395
    Expected equality of these values:
      static_cast<int32_t>(expected_index)
        Which is: 0
      feedback_msg->index
        Which is: 1
  >>>

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.93%. Comparing base (fcad0a3) to head (687fb33).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1850      +/-   ##
==========================================
+ Coverage   85.92%   85.93%   +0.01%     
==========================================
  Files         133      133              
  Lines       13148    13165      +17     
  Branches     1145     1145              
==========================================
+ Hits        11297    11314      +17     
  Misses       1480     1480              
  Partials      371      371              

Flag	Coverage Δ
unittests	85.93% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ory_controller/src/joint_trajectory_controller.cpp	83.95% <100.00%> (+0.03%)	⬆️
...ectory_controller/test/test_trajectory_actions.cpp	97.39% <100.00%> (+0.06%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[christophfroehlich]

  /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_actions.cpp:211:12: error: implicit conversion changes signedness: 'typename iterator_traits<__normal_iterator<const double *, vector<double>>>::difference_type' (aka 'long') to 'size_t' (aka 'unsigned long') [-Werror,-Wsign-conversion]
    211 |     return std::distance(
        |     ~~~~~~ ^~~~~~~~~~~~~~
    212 |       times_vector.begin(),
        |       ~~~~~~~~~~~~~~~~~~~~~
    213 |       std::lower_bound(times_vector.begin(), times_vector.end(), time_in_seconds));
        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[GiuseppeMonetti95]

There was a logic error in my implementation, now it works!
I also made the conversion explicit.

[christophfroehlich]

Thanks, now LGTM

[GiuseppeMonetti95]

Thanks, now LGTM

Thanks! Do you understand why the pipelines are failing? It looks like the failure is in the apt install ... command. I don't see why the failure should be caused by my changes

[christophfroehlich]

not related at all, moveit packages are not available

[GiuseppeMonetti95]

Should I do something extra to get this merged?