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

PID controller and diff drive controller chaining #710

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Juliaj
Copy link

@Juliaj Juliaj commented Jan 30, 2025

Address #568. This is to add a new example_16 based on the use case outlined in the controller manager doc section Motivation, Purpose and Use. It uses example_2 as a base and adds a chained 'diff_drive_controller' and two PID controllers with following flow.

diff_drive_controller     → pid_controllers      → hardware
     (velocities)           (error control)       (motors) 

This requires following changes from ros2_controllers:

New Examples

If you propose adding a new example, make sure that your new example has:

  • The correct folder structure (described in the main README.md)
  • Example has doc/README.rst with description
  • Detailed commands how to run an example
  • Output examples of CLI commands
  • Screenshots with expected visualizations (if applicable)

@christophfroehlich
Copy link
Contributor

I think you found a bug, I proposed a fix in ros-controls/ros2_controllers#1513
I also changed your configuration a bit, let met know what you think.

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 your submission! Some thoughts:

  • Please cleanup the docs to focus on the controller chain (e.g., remove the MockHardware part)
  • Update the test_diffbot_launch.py to check for all controllers
  • I suggest that you add some dynamics to the hardware interface, a first-order or second-order low pass, so that the PID controller actually makes sense.

When the following stuff is fixed/merged we should include to the docs (maybe in follow-up PRs)

@Juliaj
Copy link
Author

Juliaj commented Feb 1, 2025

I think you found a bug, I proposed a fix in ros-controls/ros2_controllers#1513 I also changed your configuration a bit, let met know what you think.

Thank you so much for responding so quickly. The configuration changes are really nice, love them. I tested the fix you made, the activation is working now. I'll continue the rest of work and include the suggestions that you made. Left some comments on your PR ros-controls/ros2_controllers#1513.

@Juliaj Juliaj marked this pull request as ready for review February 4, 2025 06:16
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.

see #1528 for a proper fix of the interfaces. could you have a look there and review please?

],
)

# start the base controller in inactive mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# start the base controller in inactive mode

Comment on lines 136 to 139
# "--inactive",
"--ros-args",
"--log-level",
"debug",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# "--inactive",
"--ros-args",
"--log-level",
"debug",

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.

2 participants