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

Fix handles for following controllers of diff_drive_controller #1513

Closed
wants to merge 1 commit into from

Conversation

christophfroehlich
Copy link
Contributor

As reported in ros-controls/ros2_control_demos#710, the pid_controller activation failed because it checked for prefix_name and interface_name separately, expecting prefix_name pid_controller_left_wheel_joint/left_wheel_joint with interface velocity, but actually it is prefix_name pid_controller_left_wheel_joint with interface_name left_wheel_joint/velocity.

Let's just check the full name.

@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.16%. Comparing base (50ff026) to head (0db2dec).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...iff_drive_controller/src/diff_drive_controller.cpp 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1513      +/-   ##
==========================================
- Coverage   84.19%   84.16%   -0.03%     
==========================================
  Files         123      123              
  Lines       11323    11327       +4     
  Branches      957      959       +2     
==========================================
  Hits         9533     9533              
- Misses       1473     1477       +4     
  Partials      317      317              
Flag Coverage Δ
unittests 84.16% <50.00%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
...iff_drive_controller/src/diff_drive_controller.cpp 72.09% <50.00%> (-0.85%) ⬇️

RCLCPP_ERROR(
logger, " - %s: %s", interface.get_prefix_name().c_str(),
interface.get_interface_name().c_str());
}
Copy link
Contributor

@Juliaj Juliaj Feb 1, 2025

Choose a reason for hiding this comment

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

I tested the fix above and it works with example_16. However, I wonder these debug message could lead to confusion if the developer is unaware about the chain created under the hood. For example_16, prefix: interface pair is pid_controller_left_wheel_joint : left_wheel_joint/velocity.

A naive alternative to the current fix could be:

  • preserve the prefix and interface as in the current definition
  • introduce a mode (e.g. chained_right) with additional params and logic to handle the use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought about this a bit more. Please ignore my comments on alternative fix above because I believe it is supported by default. This is illustrated by demo example_12 where the forward_position_controller controls "joints" exposed by the second level controller. These (virtual ?) joints are configured in a hierarchical pattern.

# Third-level controllers
forward_position_controller:
  ros__parameters:
    type: forward_command_controller/ForwardCommandController
    joints:
      - position_controller/joint1_position_controller/joint1
      - position_controller/joint2_position_controller/joint2
    interface_name: position

Copy link
Contributor

@Juliaj Juliaj left a comment

Choose a reason for hiding this comment

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

LGTM. This was tested as part of demo example_16.

@christophfroehlich
Copy link
Contributor Author

I fixed the wrong part in the chain, superseded by #1528

@christophfroehlich christophfroehlich deleted the diff_drive/chain branch February 5, 2025 16:24
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.

2 participants