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

Ahcorde/add/joint limit interface #181

Merged

Conversation

guru-florida
Copy link
Contributor

@guru-florida guru-florida commented Oct 4, 2020

As suggested by @destogl, this is a follow-on to a PR by ahcorde, PR135.

This PR has joint_limit_interface updated with #134 changes which remote state and command handles and replace with just JointHandle. It has been synced with the latest ros2_control/master branch.

Also all tests (previously commented out) have been fixed up and pass on my local system.

Suggesting review by @Karsten1987 @bmagyar as well.

This PR:
Originally coded by: @ddengster [email protected]
Signed-off-by: @ahcorde [email protected]
#134, refactor + unit tests by myself: guru-florida

I believe ahcorde and I are both working to the same goal of getting gazeobo+ros+ros-control working together! :) Kudos to all, we are so close!

FYI I also have transmission_interface similarly updated with master in PR182. This PR is also included in PR182 so you can just approve and do that one, or both, PR182 is based from 181 and both synced to master so no worries. It's just a matter of scope.

@guru-florida
Copy link
Contributor Author

LICENSING FYI @v-lopez @destogl @bmagyar I changed the file header licenses to Apache 2 as discussed here.

@bmagyar bmagyar requested review from bmagyar and Karsten1987 October 5, 2020 17:05
@bmagyar
Copy link
Member

bmagyar commented Oct 7, 2020

@ahcorde we'd appreciate your feedback on this one

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, @bmagyar should review the maintainers in the package.xml

@guru-florida
Copy link
Contributor Author

@bmagyar @Karsten1987 @destogl In this refactor for #134 I went with JointHandle arguments wrapped in std::shared_ptr<>, should these have been without shared_ptr? This was due to a commit I was using as a reference that had shared_ptr, but after reviewing more code such as robot_hardware.cpp I dont see any reason to use shared_ptr here, the JontHandle by value would work fine and probably more compatible. Thoughts?

@bmagyar bmagyar merged commit 6ae1976 into ros-controls:master Oct 10, 2020
guru-florida added a commit to guru-florida/ros2_control that referenced this pull request Oct 23, 2020
* Added joint_limits_interface - Direct port from ROS 1
* Follow ros2 styles and added tests
* Added missing dependencies in package.xml
* updated joint limits interface exception to nothrow and match base override
* converted joint limit interfaces to use new JointHandles (no more JointStateHandle or JointCommandHandle). Refactored common functionality into base JointSaturationLimitHandle and JointSoftLimitsHandle classes.
* updated joint limits tests to use new JointHandles
* updated package version from 2 to 3
* added memory include as suggested by ros_industrial test
* uncrustify fixes
* removing changelog from ROS1
* Reset version and fix links
* Update maintainers

Co-authored-by: ddengster <[email protected]>
Co-authored-by: ahcorde <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Colin MacKenzie guru-florida
destogl pushed a commit to StoglRobotics-forks/ros2_control that referenced this pull request Aug 11, 2022
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