Added joint_limits_interface - Direct port from ROS 1 #135

ahcorde · 2020-08-27T08:32:31Z

As discussed yesterday in the ros2_control WG, I'm adding this package originally ported by @ddengster

I still need to copy from @ddengster repository the transmmision_interface package.

@Karsten1987 @bmagyar

Co-authored-by: ddengster [email protected]

Signed-off-by: ahcorde [email protected]

Co-authored-by: ddengster <[email protected]> Signed-off-by: ahcorde <[email protected]>

codecov-commenter · 2020-08-27T09:02:51Z

Codecov Report

Merging #135 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #135   +/-   ##
=======================================
  Coverage   43.54%   43.54%           
=======================================
  Files          42       42           
  Lines        1410     1410           
  Branches      761      761           
=======================================
  Hits          614      614           
  Misses        112      112           
  Partials      684      684

Flag	Coverage Δ
#unittests	`43.54% <ø> (ø)`

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9ea092...e05d0da. Read the comment docs.

bmagyar · 2020-08-28T06:58:50Z

@v-lopez @jordan-palacios could you guys confirm whether the license on this could also be updated?

v-lopez · 2020-08-28T07:07:02Z

Yes, no problem.

v-lopez

I'd cleanup the test part before merging. Also made some minor suggestions.

joint_limits_interface/CMakeLists.txt

joint_limits_interface/include/joint_limits_interface/joint_limits_interface.hpp

joint_limits_interface/include/joint_limits_interface/joint_limits_rosparam.hpp

Signed-off-by: ahcorde <[email protected]>

ahcorde · 2020-09-03T10:30:40Z

Sorry for the bad conditions of the package. I was just copying a version from other repo that it was just functional.

I added

common ROS 2 linters
Fixed test

Signed-off-by: ahcorde <[email protected]>

…imit_interface

destogl

First part of the review.

joint_limits_interface/CMakeLists.txt

joint_limits_interface/README.md

destogl · 2020-09-09T07:31:05Z

joint_limits_interface/include/joint_limits_interface/joint_limits_interface.hpp

+  double max_vel_limit_;
+};
+
+#if 0


It seems this code is never used. Do we still need it?

added a TODO

destogl

Also remove .pyc file.

I have to do it one more time

destogl · 2020-09-09T15:29:18Z

joint_limits_interface/package.xml

@@ -0,0 +1,39 @@
+<package format="2">


Why not use format=3

The other packages are version 2, I can update this one.

destogl · 2020-09-09T15:29:54Z

joint_limits_interface/package.xml

+
+  <maintainer email="[email protected]">Bence Magyar</maintainer>
+  <maintainer email="[email protected]">Enrique Fernandez</maintainer>
+  <maintainer email="[email protected]">Mathias Lüdtke</maintainer>


Add not-active maintainers as authors

can you suggest me which are the authors and maintainer of this package?

I think Enrique and Mathias are not active so much here, so I would move them in authors. @bmagyar is clearly maintainer :)

joint_limits_interface/test/joint_limits_interface_test.cpp

destogl · 2020-09-09T20:05:48Z

This looks good to me. I believe using the new concept from #121 this code is only temporarily. If not so there are few more issues. I list them here just for the "documentation":

headers alpha sort
There are some TODO comments in the code
use of new JointHandle from Remove old joint state and joint command handles #134

Signed-off-by: ahcorde <[email protected]>

destogl

It looks good!

destogl · 2020-09-20T12:36:21Z

joint_limits_interface/test/joint_limits_interface_test.cpp

+// Copyright 2013, PAL Robotics S.L. All rights reserved.
+// All rights reserved.
+//
+// Software License Agreement (BSD License 2.0)


Is this potential issue in the ros2_control repository? Currently, we have everything Apache2.0 licensed. If so, can we discuss this with PAL to change the license?

@bmagyar can you please take a look?

If it is a problem we have no objections to change it to Apache2.0

guru-florida · 2020-09-30T03:27:22Z

Unfortunately it looks like this merge didn't make it in time before the use of the new JointHandle from #134 took place (see also #155). I will see if I can resolve.

guru-florida · 2020-10-01T15:27:27Z

ok, I have a build working with the use of the new JointHandle (#134). I have also fixed up the tests and they are all passing. You can see my changes in:
https://github.com/guru-florida/ros2_control/tree/ahcorde/add/joint_limit_interface/joint_limits_interface

Should I create a new PR?

Here is my review of the code. Most of the code is in the following joint limit policies:

PositionJointSaturationHandle
PositionJointSoftLimitsHandle
EffortJointSaturationHandle
EffortJointSoftLimitsHandle
VelocityJointSaturationHandle
VelocityJointSoftLimitsHandle

There is also JointLimitsInterface<> that deals with creating these handles via ResourceManager that still needs porting to ROS2. I dont see any ResourceManager class so I think this is blocked on #164 and/or #141.

Refactoring: Much of the LimitHandle classes are identical except for the enforceLimits() method. I refactored that common code out into JointSaturationLimitHandle. It's constructor takes joint position state and command handle and a JointLimits object, same as all the saturation limit policies did before. I then derived JointSoftLimitsHandle from the JointSaturationLimitHandle which just adds the SoftJointLimits argument to the constructor. All the policies then derive from one of these base classes and override the pure virtual enforceLimits(...) method. There is some common handling of state and derived velocity as well. This simplified the code a lot and made fixing up easier. These 2 bases classes are at the top of the file.

Velocity Handle: The new generalized JointHandle no longer has a get_velocity() method, instead the user should construct a position and velocity handle separately. Therefor another constructor prototype was added that supports passing in position, velocity and command handles. If a velocity handle is not provided then velocity will be computed by position/time (saved previous state). User has a choice of estimating velocity by position deltas or using actual velocity via (ex) a speedometer.

Open/Closed Loop: @bmagyar Since state and command handles now have the same interface (under change #134), in fact the new state/command handle arguments are of identical type now....so do we have an opportunity to convert these limit interfaces to support open or closed loop? Since the state handle could be either the position state or a position command handle. Currently the enforceLimits() initially takes from the state handle, but then there-after it compares to previous command and generates a new constrained command. I am not sure I agree with "start with a state value then switch to command value" logic but I am hesitant to change any logic since I am a ros_controls newbie. Plus this is limits, not PID/control logic or something so I may be way off base....thoughts? Did #134 present an opportunity for new behavior? If so, I can probably code it easily into the 2 common base classes above.

destogl · 2020-10-02T21:06:26Z

@ahcorde can we close this PR an open the new one wir changes from @guru-florida?

@guru-florida the changes in #134 are also kind of temporary since we will move everything to components in the future (when ResourceManager is finished - #164 and #147). Nevertheless, this restructuring you did is anyway needed since the logic behind is similar.

Maybe you have some idea how to integrate the limits into components. The example implementation of the PositionJoint is in #140

guru-florida · 2020-10-04T03:58:06Z

Thanks @destogl. I have created a new PR181.

bmagyar · 2020-10-05T17:07:01Z

Closing in favour of #181

Added joint_limits_interface - Direct port from ROS 1

21da0a2

Co-authored-by: ddengster <[email protected]> Signed-off-by: ahcorde <[email protected]>

bmagyar mentioned this pull request Aug 31, 2020

As a robotics developer, I want to specify the limits of my robot's joints so I can ensure safe operation of my robot #26

Closed

2 tasks

bmagyar requested review from v-lopez and bmagyar August 31, 2020 09:01

v-lopez suggested changes Aug 31, 2020

View reviewed changes

Follow ros2 styles and added tests

5c1bd59

Signed-off-by: ahcorde <[email protected]>

Added missing dependencies in package.xml

1cab86b

Signed-off-by: ahcorde <[email protected]>

ahcorde requested a review from v-lopez September 3, 2020 12:31

Merge remote-tracking branch 'origin/master' into ahcorde/add/joint_l…

dff8ae6

…imit_interface

destogl reviewed Sep 9, 2020

View reviewed changes

destogl requested changes Sep 9, 2020

View reviewed changes

ahcorde added 2 commits September 14, 2020 10:06

Added feedback

4354690

Signed-off-by: ahcorde <[email protected]>

removed pycache

e05d0da

Signed-off-by: ahcorde <[email protected]>

ahcorde requested a review from destogl September 15, 2020 13:46

destogl approved these changes Sep 20, 2020

View reviewed changes

guru-florida mentioned this pull request Oct 4, 2020

Ahcorde/add/joint limit interface #181

Merged

ahcorde closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added joint_limits_interface - Direct port from ROS 1 #135

Added joint_limits_interface - Direct port from ROS 1 #135

ahcorde commented Aug 27, 2020

codecov-commenter commented Aug 27, 2020 •

edited

Loading

bmagyar commented Aug 28, 2020

v-lopez commented Aug 28, 2020

v-lopez left a comment

ahcorde commented Sep 3, 2020

destogl left a comment

destogl Sep 9, 2020

ahcorde Sep 14, 2020

destogl left a comment •

edited

Loading

destogl Sep 9, 2020

ahcorde Sep 14, 2020

destogl Sep 9, 2020

ahcorde Sep 14, 2020

destogl Sep 20, 2020

destogl commented Sep 9, 2020

destogl left a comment

destogl Sep 20, 2020 •

edited

Loading

v-lopez Sep 21, 2020

destogl Sep 23, 2020

guru-florida commented Sep 30, 2020

guru-florida commented Oct 1, 2020

destogl commented Oct 2, 2020

guru-florida commented Oct 4, 2020

bmagyar commented Oct 5, 2020

Added joint_limits_interface - Direct port from ROS 1 #135

Added joint_limits_interface - Direct port from ROS 1 #135

Conversation

ahcorde commented Aug 27, 2020

codecov-commenter commented Aug 27, 2020 • edited Loading

Codecov Report

bmagyar commented Aug 28, 2020

v-lopez commented Aug 28, 2020

v-lopez left a comment

Choose a reason for hiding this comment

ahcorde commented Sep 3, 2020

destogl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

destogl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

destogl commented Sep 9, 2020

destogl left a comment

Choose a reason for hiding this comment

destogl Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guru-florida commented Sep 30, 2020

guru-florida commented Oct 1, 2020

destogl commented Oct 2, 2020

guru-florida commented Oct 4, 2020

bmagyar commented Oct 5, 2020

codecov-commenter commented Aug 27, 2020 •

edited

Loading

destogl left a comment •

edited

Loading

destogl Sep 20, 2020 •

edited

Loading