-
Notifications
You must be signed in to change notification settings - Fork 399
Refactoring to motion_primitives_base_controller to support PR #1858 #1857
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
Refactoring to motion_primitives_base_controller to support PR #1858 #1857
Conversation
…time --> added gitignore, to not push it to github --> remove it after everything is done
… the hardware interface can receive a new motion primitive — replaces the previous, more complex handling via execution_status
…ls according to other controllers in the repo
… rtw template to fit the controller implementation
…tation from rtw template to fit the controller implementation
…ontroller namespace
…o_moprim_base_controller
…o_moprim_base_controller
…o_moprim_base_controller
This pull request is in conflict. Could you fix it @mathias31415? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1857 +/- ##
==========================================
- Coverage 85.12% 85.10% -0.03%
==========================================
Files 138 140 +2
Lines 13547 13559 +12
Branches 1188 1187 -1
==========================================
+ Hits 11532 11539 +7
- Misses 1620 1626 +6
+ Partials 395 394 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but I have some minor comments
@@ -159,7 +98,7 @@ controller_interface::return_type MotionPrimitivesForwardController::update( | |||
{ | |||
case ExecutionState::IDLE: | |||
print_error_once_ = true; | |||
was_executing_ = false; | |||
// was_executing_ = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action was sometimes not properly finished with SUCCESSFUL, because was_executing_ was reset too early.
When transitioning from EXECUTING → SUCCESS, the action should end with SUCCESSFUL, which is why the flag was_executing_ is checked here.
The flag should only be reset if the action is canceled (STOPPED) or in case of an error (ERROR).
However, I should remove this completely and not just comment it out.
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
I merge this without another review as it does not change the behavior but only refactors some stuff. |
dc2ee70
into
ros-controls:master
This pull request supports PR #1858 . Shared functionality of the motion_primitives forward and from_trajectory controllers has been moved to a common base controller. To make reviewing easier, this first PR only extracts the functionality of the forward controller. In a subsequent PR, the from_trajectory controller will then be added.
This PR is marked as a draft because it depends on PR #1636 , which has not yet been merged.