Parse URDF joint hard limits into the HardwareInfo structure #1472

saikishor · 2024-04-02T21:13:43Z

This PR allows to parse the joint_limits into the HardwareInfo structure, so that this can be later reused by the ResourceManager to enforce the limits on the JointCommandInterfaces.

The joint limits that are available in the HardwareInfo structure are basically parsed from the URDF joint limits, and these joint limits range can be reduced with the <min> and <max> tag of each command interface, the idea is to be able to always be able to conservative.

There will be a following PR on parsing the SoftLimits as well

…ruct

…description

codecov · 2024-04-17T16:34:04Z

Codecov Report

Attention: Patch coverage is 94.19355% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 88.53%. Comparing base (7fdf472) to head (8e0d86e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1472      +/-   ##
==========================================
+ Coverage   88.42%   88.53%   +0.10%     
==========================================
  Files         101      101              
  Lines        8349     8502     +153     
  Branches      728      759      +31     
==========================================
+ Hits         7383     7527     +144     
- Misses        703      706       +3     
- Partials      263      269       +6

Flag	Coverage Δ
unittests	`88.53% <94.19%> (+0.10%)`	⬆️

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

Files	Coverage Δ
hardware_interface/test/test_component_parser.cpp	`100.00% <100.00%> (ø)`
hardware_interface/src/component_parser.cpp	`92.74% <91.34%> (-0.60%)`	⬇️

christophfroehlich

The idea is clear and the implementation looks fine. However, a couple of lines miss coverage, especially in the update_interface_limits lambda

hardware_interface/src/component_parser.cpp

Co-authored-by: Christoph Fröhlich <[email protected]>

saikishor · 2024-04-22T21:01:01Z

10 lines are still not covered, and total coverage will decrease. should I try to add tests to run into this failure cases?

Sure. if you can, it would be really helpful. Thanks @christophfroehlich

olivier-stasse · 2024-04-23T13:17:59Z

hardware_interface/src/component_parser.cpp

+            max_vel = std::min(std::abs(min_vel), max_vel);
+            limits.max_velocity = std::min(max_vel, limits.max_velocity);
+          }
+        }


Out of curiosity why you do not do a default initialization of the max_velocity ?

what do you mean by default initialization? limits will be filled from the URDF tag, which is mandatory for REVOLUTE and PRISMATIC joints -> max_velocity is already populated

Yes, moreover, there might be a joint without velocity limits, so it doesn't make sense

olivier-stasse · 2024-04-23T13:18:39Z

hardware_interface/src/component_parser.cpp

+            max_eff = std::min(std::abs(min_eff), max_eff);
+            limits.max_effort = std::min(max_eff, limits.max_effort);
+          }
+        }


Same here wrt to max_effort

Well same as above

olivier-stasse · 2024-04-23T13:19:34Z

hardware_interface/src/component_parser.cpp

+        {
+          limits.max_jerk = std::abs(max_jerk);
+          limits.has_jerk_limits = true && itr.enable_limits;
+        }


Same here for max_jerk

Jerk limits are very familiar as the support in ROS is little, so I think it is better to leave it up to the user

it does not throw on continuous

saikishor · 2024-04-24T20:42:02Z

@christophfroehlich Thanks for helping out and adding very nice tests

christophfroehlich

Just let us add some notes about this once #1477 is merged.

saikishor · 2024-04-25T14:09:51Z

Just let us add some notes about this once #1477 is merged.

Sure. I will do that :)

bmagyar · 2024-05-03T11:50:01Z

hardware_interface/src/component_parser.cpp

+          {
+            limits.max_acceleration = std::fabs(limits.max_deceleration);
+          }
+          limits.has_deceleration_limits = true && itr.enable_limits;


I noticed this pattern of true && in a couple of places. Why?

You are right, it can be simply itr.enable_limits. I will make the modifications soon and let you know

You are right. This is fixed now. 👍

hardware_interface/src/component_parser.cpp

…imits works

saikishor added 8 commits April 2, 2024 19:05

Add joint_limits dependency and add limits map to the HardwareInfo st…

f37ae78

…ruct

Retrieve limits from the URDF as well as ros2_control tag from robot_…

7d1077a

…description

print error and continue instead of throwing exception

f43124d

Fix the logic of the retrieval of min and max elements

b39f1cf

update tests adding the limits part

f4586f3

remove limits enforcing when defined under state interface

aec2e6f

add more testing for other cases

3c7a4d7

added comments to the limits map and removed extra include

ce26ef2

github-actions bot requested review from aprotyas, bmagyar, destogl, livanov93, moriarty, olivier-stasse and peterdavidfagan April 2, 2024 21:13

saikishor mentioned this pull request Apr 2, 2024

Joint Limiters: Saturation (current PR971 - will be renamed) and Soft Limiter #1466

Open

saikishor added 4 commits April 4, 2024 13:57

Added a way to enable and disable the joint interface limits

8e0faba

Added tests for the newly added disabling of joint limits

1779bc0

Merge branch 'master' into parse_limits_from_urdf_to_hw_info

d058b8a

Add the missing limits to the diffbot urdf continuous joints

e3847f9

Fix formatting changes

68874f5

adriaroig mentioned this pull request Apr 18, 2024

Parse URDF soft_limits into the HardwareInfo structure #1488

Merged

saikishor added this to the Pre-Jazzy (Feb.'24) milestone Apr 18, 2024

christophfroehlich requested changes Apr 22, 2024

View reviewed changes

saikishor and others added 5 commits April 22, 2024 17:42

Change kEnableTag to kEnableAttribute and other minor typos

7725dd7

Co-authored-by: Christoph Fröhlich <[email protected]>

Merge branch 'master' into parse_limits_from_urdf_to_hw_info

4fd605a

Add tests for the acceleration and jerk limits parsing

2016885

Fix minor bug in the acceleration and deceleration signs

78c80e8

Add tests for specific use conditions

98c95fe

christophfroehlich mentioned this pull request Apr 22, 2024

Improve test coverage pal-robotics-forks/ros2_control#6

Merged

christophfroehlich and others added 5 commits April 23, 2024 00:18

Format error strings

18fae53

Add comment to remove duplicate code

6ac3140

Add another test with different min/max values

a4d98b1

Rename tests

0784152

Fix the missing GPIOs part in the tests

5e8f3f0

olivier-stasse approved these changes Apr 23, 2024

View reviewed changes

christophfroehlich and others added 7 commits April 23, 2024 19:03

Remove unnecessary check and add test if it still throws

2f6cf66

Test if it throws on revolute+prismatic without limits

611e60a

it does not throw on continuous

Fix continuous joint limits

3cfce6b

Successfully parse continuous joint limits if set in URDF

9b21ba4

Throw if trying to configure a fixed joint

74e22b9

Remove duplicate code

b414970

Merge branch 'master' into parse_limits_from_urdf_to_hw_info

75731af

christophfroehlich previously approved these changes Apr 25, 2024

View reviewed changes

bmagyar reviewed May 3, 2024

View reviewed changes

hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved

saikishor added 2 commits May 3, 2024 15:35

separate the lambdas into isolated helper methods

48bbef3

fix the has_limits logic for different interfaces

9d08858

saikishor dismissed christophfroehlich’s stale review via 9d08858 May 3, 2024 13:38

saikishor added 4 commits May 3, 2024 15:41

Merge branch 'master' into parse_limits_from_urdf_to_hw_info

fbe02b4

make update_interface_limits more generic and fix minor bug

616b491

Remove the copy_interface_limits method as generic update_interface_l…

dc0d414

…imits works

Merge branch 'master' into parse_limits_from_urdf_to_hw_info

8e0d86e

bmagyar approved these changes May 7, 2024

View reviewed changes

bmagyar merged commit 607755e into ros-controls:master May 7, 2024
10 of 12 checks passed

saikishor deleted the parse_limits_from_urdf_to_hw_info branch August 17, 2024 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse URDF joint hard limits into the HardwareInfo structure #1472

Parse URDF joint hard limits into the HardwareInfo structure #1472

saikishor commented Apr 2, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

christophfroehlich left a comment

saikishor commented Apr 22, 2024 •

edited

Loading

olivier-stasse Apr 23, 2024

christophfroehlich Apr 24, 2024

saikishor Apr 24, 2024

olivier-stasse Apr 23, 2024

saikishor Apr 24, 2024

olivier-stasse Apr 23, 2024

saikishor Apr 24, 2024

saikishor commented Apr 24, 2024

christophfroehlich left a comment

saikishor commented Apr 25, 2024

bmagyar May 3, 2024

saikishor May 3, 2024

saikishor May 3, 2024

Parse URDF joint hard limits into the HardwareInfo structure #1472

Parse URDF joint hard limits into the HardwareInfo structure #1472

Conversation

saikishor commented Apr 2, 2024

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Apr 22, 2024 • 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

Choose a reason for hiding this comment

saikishor commented Apr 24, 2024

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2024 •

edited

Loading

saikishor commented Apr 22, 2024 •

edited

Loading