Skip to content

Conversation

@christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Dec 10, 2023

I introduced a bug with #567: If an empty trajectory (no points) is received with allow_nonzero_velocity_at_trajectory_end=true then trajectory.points.back() is undefined behavior and may result in a segfault.

I added now a check to skip empty trajectories. Or is there a point for accepting them? Trajectory::sample() will return false with an empty trajectory.

if (trajectory_msg_->points.empty())
{
start_segment_itr = end();
end_segment_itr = end();
return false;
}

@christophfroehlich christophfroehlich added bug backport-humble Triggers PR backport to ROS 2 humble. backport-iron labels Dec 10, 2023
@christophfroehlich christophfroehlich changed the title Invalidate empty trajectory messages [JTC] Invalidate empty trajectory messages Dec 10, 2023
@codecov
Copy link

codecov bot commented Dec 10, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9f7e9e9) 47.71% compared to head (fba33dd) 47.47%.
Report is 5 commits behind head on master.

❗ Current head fba33dd differs from pull request most recent head ac4205d. Consider uploading reports for the commit ac4205d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
- Coverage   47.71%   47.47%   -0.24%     
==========================================
  Files          41       41              
  Lines        3871     3865       -6     
  Branches     1833     1818      -15     
==========================================
- Hits         1847     1835      -12     
- Misses        751      768      +17     
+ Partials     1273     1262      -11     
Flag Coverage Δ
unittests 47.47% <66.66%> (-0.24%) ⬇️

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

Files Coverage Δ
...ory_controller/src/joint_trajectory_controller.cpp 48.62% <66.66%> (+1.50%) ⬆️

... and 5 files with indirect coverage changes

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Looking good to me, just a small proposal to rename test for clarity.

Maybe I tend to have tooooooooo long test names lately, but this brings clarity what is tested.

/**
* @brief invalid_message_nonzero_vel Test invalid velocity at trajectory end
*/
TEST_P(TrajectoryControllerTestParameterized, invalid_message_nonzero_vel)
Copy link
Member

Choose a reason for hiding this comment

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

Just a bit more clarity.

Suggested change
TEST_P(TrajectoryControllerTestParameterized, invalid_message_nonzero_vel)
TEST_P(
TrajectoryControllerTestParameterized,
expect_invalid_when_message_with_nonzero_end_velocity_and_when_param_false)

@destogl destogl disabled auto-merge January 30, 2024 16:43
@destogl destogl merged commit 99fadce into ros-controls:master Jan 30, 2024
mergify bot pushed a commit that referenced this pull request Jan 30, 2024
mergify bot pushed a commit that referenced this pull request Jan 30, 2024
@christophfroehlich christophfroehlich deleted the jtc/fix_segfault branch January 30, 2024 21:01
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
Bumps [docker/login-action](https://github.com/docker/login-action) from 1 to 2.
- [Release notes](https://github.com/docker/login-action/releases)
- [Commits](docker/login-action@v1...v2)

---
updated-dependencies:
- dependency-name: docker/login-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-humble Triggers PR backport to ROS 2 humble. bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants