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

[JSB] added fixes to mantain the joint names order #1572

Merged
merged 22 commits into from
Apr 9, 2025

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Mar 4, 2025

I've add some fixes to the codes to retain the proper naming order. it will maintain the same naming as the state_interfaces. Most of the time. it is same as requested.

I confirm that it is working as expected. The following are the results after testing on the TIAGo robot

With master branch:

header:
  stamp:
    sec: 27
    nanosec: 634000000
  frame_id: ''
name:
- wheel_right_joint
- torso_lift_joint
- head_2_joint
- arm_1_joint
- head_1_joint
- gripper_right_finger_joint
- arm_2_joint
- arm_3_joint
- gripper_left_finger_joint
- arm_6_joint
- wheel_left_joint
- arm_7_joint
- arm_5_joint
- arm_4_joint
position:
- -0.01883440381531276
- 0.14992120022237515
- -2.1771094615985476e-07
- 0.5000004071831876
- 2.679102557578972e-07
- 1.498072885788662e-09
- -1.3399972501658528
- -0.47999627762595054
- 1.6320817259907867e-07
- 1.3700221974953228
- 0.00558756172246877
- -5.676095425677374e-06
- -1.4900038801677784
- 1.9399743393071844
velocity:
- .nan
- -0.009820570088929573
- 0.00010381144165565768
- -0.0004520477620429137
- -0.0005884276721260586
- 0.0003904782235743167
- 0.0026640572724227402
- 0.004136899003124899
- -0.0002584158488683724
- 0.02222271007923489
- .nan
- -0.0019689225065199385
- -0.004196435823050384
- -0.025497848941894285
effort:
- .nan
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- .nan
- 0.0
- 0.0
- 0.0
---

With the fix proposed in this PR:

header:
  stamp:
    sec: 3
    nanosec: 7000000
  frame_id: ''
name:
- wheel_right_joint
- wheel_left_joint
- torso_lift_joint
- arm_1_joint
- arm_2_joint
- arm_3_joint
- arm_4_joint
- arm_5_joint
- arm_6_joint
- arm_7_joint
- gripper_left_finger_joint
- gripper_right_finger_joint
- head_1_joint
- head_2_joint
position:
- -0.02799593211424245
- 0.003465789616476478
- -1.7199045996895583e-08
- -0.0005503061596039416
- -0.0003136661241898153
- -2.6876812497356184e-05
- -5.9248231679021046e-05
- 0.000129607204049087
- 0.0012243190113494506
- 0.0002718504386338694
- 2.561374795404651e-06
- 5.5824897165386553e-08
- -5.915678524814894e-07
- -0.0003185975886408343
velocity:
- .nan
- .nan
- -1.5386795985256052e-05
- 0.028547943684709375
- -0.05535369149489766
- 0.007545245983102342
- -0.0006098150419372589
- -0.031006116599605057
- 0.005376069634232324
- -0.015109590818687155
- -0.009228197094119775
- 0.012866053358847787
- 0.0035212422027661536
- -0.008785733629921097
effort:
- .nan
- .nan
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
- 0.0
---

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.10%. Comparing base (d1844e6) to head (7a5b2c8).

Files with missing lines Patch % Lines
..._state_broadcaster/src/joint_state_broadcaster.cpp 81.25% 0 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1572      +/-   ##
==========================================
+ Coverage   85.06%   85.10%   +0.03%     
==========================================
  Files         123      123              
  Lines       11712    11719       +7     
  Branches      998      996       -2     
==========================================
+ Hits         9963     9973      +10     
+ Misses       1434     1433       -1     
+ Partials      315      313       -2     
Flag Coverage Δ
unittests 85.10% <81.25%> (+0.03%) ⬆️

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

Files with missing lines Coverage Δ
..._state_broadcaster/src/joint_state_broadcaster.cpp 88.88% <81.25%> (-0.66%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Shouldn't it now just use the order of the joints defined in the URDF, if we use the auto-export of the state interfaces? But with the example_1 one gets

 $ ros2 topic echo /joint_states --once
header:
  stamp:
    sec: 1741554154
    nanosec: 735820900
  frame_id: ''
name:
- joint2
- joint1
position:
- 0.0
- 0.0
velocity:
- .nan
- .nan
effort:
- .nan
- .nan

@saikishor
Copy link
Member Author

Shouldn't it now just use the order of the joints defined in the URDF, if we use the auto-export of the state interfaces? But with the example_1 one gets

If the joints parameter is not defined, then it publishes in the original order of the state interfaces

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-ups and the docs, just minor suggestions there

Co-authored-by: Christoph Fröhlich <[email protected]>
@saikishor saikishor added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Mar 12, 2025
Copy link

@mhubii mhubii left a comment

Choose a reason for hiding this comment

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

Thank you for the effort and also considering URDF order!

I have tested this on mock hardware for lbr_fri_ros2_stack inside an Ubuntu 24.04 Docker container using option 1), 2), and 3).

ros2_control master branch source build.

  • Option 1: ✅
  • Option 2: ✅
  • Option 3: ✅

However, this error occurred frequently:

[ros2_control_node-2] [ERROR] [1743008902.700347616] [lbr.controller_manager]: Caught exception of type : St18bad_variant_access while updating controller 'joint_state_broadcaster': std::get: wrong index for variant

Not sure this is related.

@saikishor
Copy link
Member Author

saikishor commented Mar 26, 2025

However, this error occurred frequently:

[ros2_control_node-2] [ERROR] [1743008902.700347616] [lbr.controller_manager]: Caught exception of type : St18bad_variant_access while updating controller 'joint_state_broadcaster': std::get: wrong index for variant

Not sure this is related.

I'm not sure if this has something to do with the joint_state_broadcaster. IMO for now, we can discard this

@bmagyar bmagyar merged commit 364462a into ros-controls:master Apr 9, 2025
21 of 26 checks passed
@bmagyar bmagyar deleted the fix/jsb/names_order branch April 9, 2025 19:01
mergify bot pushed a commit that referenced this pull request Apr 9, 2025
(cherry picked from commit 364462a)

# Conflicts:
#	joint_state_broadcaster/src/joint_state_broadcaster.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants