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

[JointTrajectoryController] Enable position, velocity and acceleration interfaces #140

Merged
merged 10 commits into from
May 20, 2021

Conversation

destogl
Copy link
Member

@destogl destogl commented Jan 13, 2021

this addresses #135 and it should replace #36.

Currently the only version with commented-out code to work with KUKA robots.

Proposal: we should choose between different versions using parameters.

@bmagyar
Copy link
Member

bmagyar commented Jan 13, 2021

We could have a similar approach to forward_command_controller, with joint_trajectory_controller having a parameter to pick the "flavour" and some derivatives that hard-code this parameter.

@destogl
Copy link
Member Author

destogl commented Mar 2, 2021

We could have a similar approach to forward_command_controller, with joint_trajectory_controller having a parameter to pick the "flavour" and some derivatives that hard-code this parameter.

To keep this PR shorter I will add those into a separate one.

Before merging we need to:

  • add test for not allowed interface combinations
  • add test for velocity and acceleration
  • check what is happening when having all three state and command interfaces and why are those braking tests

Copy link
Contributor

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Reviewed a couple hundred lines... I'll come back for more later

@destogl
Copy link
Member Author

destogl commented Mar 14, 2021

Addresses: #4, #49, #118

@bmagyar bmagyar self-requested a review March 22, 2021 22:00
@destogl destogl changed the title Revive JointTrajectoryController versions [JointTrajectoryController] Enable position, velocity and acceleration interfaces Mar 28, 2021
@destogl destogl force-pushed the jtc_versions branch 2 times, most recently from 040f077 to 325bdb6 Compare March 29, 2021 06:53
@destogl
Copy link
Member Author

destogl commented Mar 29, 2021

OK, now I only have one two more test issues left:

  1. uncrustify and cpplint and fighting over braces
  2. the new test throws some exception and I cannot find from where
  3. Update documentation to proper files.

Please help.

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.

the actual controller implementation looks good but I think we should reorganize things a bit to make the core more maintainable

Copy link
Member

@jordan-palacios jordan-palacios left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

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 like what's being done here with the shared understanding that we need to refactor it.
There are several places in JTC already that are not ideal for realtime-safety already, those should be ironed out in one go probably.

@bmagyar bmagyar merged commit 612f610 into ros-controls:master May 20, 2021
@destogl destogl deleted the jtc_versions branch May 28, 2021 07:07
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.

4 participants