Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset external trajectory message upon activation #185

Merged
merged 3 commits into from
May 22, 2021

Conversation

nbbrooks
Copy link
Contributor

Reset external trajectory message to prevent preserving the old goal on systems with hardware offsets

  • Fix has_trajectory_msg() function: two wrongs were making a right so functionally things were fine

…on systems with hardware offsets

- Fix has_trajectory_msg() function: two wrongs were making a right so functionally things were fine
Copy link
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

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

to prevent preserving the old goal on systems with hardware offsets

What do you mean by "hardware offsets"? This seems like a good change regardless, I'm just not following this point.

@bmagyar
Copy link
Member

bmagyar commented May 19, 2021

Could be related to #161 ?

Co-authored-by: Matt Reynolds <mtreynolds@uwaterloo.ca>
@nbbrooks nbbrooks changed the title Reset external trajectory message Reset external trajectory message upon activation May 20, 2021
@nbbrooks
Copy link
Contributor Author

nbbrooks commented May 20, 2021

Yes, by hardware offsets I am referring to the hardware_state_has_offset param introduced in #161. I was in a rush getting this pushed up and did not write a good description. This PR makes sure any existing trajectory is cleared out when the controller is activated.

Consider this workflow:

  • Command movement through JTC topic
  • // movement begins
  • Deactivate JTC
  • // movement stops
  • An arm offset is introduced (see motivations in 161)
  • Reactivate JTC
  • // movement resumes, trying to jump to previous point in pre-offset trajectory

Currently, the above workflow would see the previous trajectory being preserved and resumed when the JTC was reactivated. I do not think this is intended behavior, or at the very least, should not be if operating with hardware_state_has_offset.

@destogl
Copy link
Member

destogl commented May 20, 2021

Consider this workflow:

* Command movement through JTC topic

* // movement begins

* Deactivate JTC

* // movement stops

* An arm offset is introduced (see motivations in 161)

* Reactivate JTC

* // movement resumes, trying to jump to previous point in pre-offset trajectory

This is actually also needed always when JTC is used along other controllers. So the issues exist always when robot if moved with another controller and switched back to JTC.

@bmagyar
Copy link
Member

bmagyar commented May 21, 2021

@nbbrooks I have a fix ready for you but can't push as you disabled Allow edits from maintainers.

Please run ament_uncrustify --reformat and push or enable me pushing

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

I'd even consider this a bugfix, thanks!

@codecov-commenter
Copy link

Codecov Report

Merging #185 (0eb9494) into master (7187083) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   35.27%   35.29%   +0.01%     
==========================================
  Files         286       26     -260     
  Lines       24816     2258   -22558     
  Branches    16082     1463   -14619     
==========================================
- Hits         8755      797    -7958     
+ Misses       1452      132    -1320     
+ Partials    14609     1329   -13280     
Flag Coverage Δ
unittests 35.29% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...int_trajectory_controller/test/test_trajectory.cpp
...ontroller/test/test_forward_command_controller.cpp
...dcaster/test/test_load_joint_state_broadcaster.cpp
...dcaster/test/test_load_joint_state_broadcaster.cpp
...test/test_load_joint_group_position_controller.cpp
...dcaster/test/test_load_joint_state_broadcaster.cpp
...test/test_load_joint_group_position_controller.cpp
...ory_controller/test/test_trajectory_controller.cpp
...ntroller/test/test_load_joint_state_controller.cpp
...include/joint_trajectory_controller/tolerances.hpp
... and 302 more

@bmagyar bmagyar merged commit 32f089b into ros-controls:master May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants