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

Merged

Conversation

Juliaj
Copy link
Contributor

@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
  • doc/index.rst is updated.

@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
Contributor 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?

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.

The demo (and the chaining itself) is working great now, thanks for the effort! I already have added the doc page to the TOC, some other comments below.

I have tuned the PIDs a bit to have a faster step response. what do you think about adding the plotjuggler config and a screenshot to the docs? Feel free to record any useful time series.
pj.xml.txt
plotjuggler

@Juliaj
Copy link
Contributor Author

Juliaj commented Feb 18, 2025

I have tuned the PIDs a bit to have a faster step response.

This is truly awesome. I must admit that while I’ve known about PID controllers from textbooks, this is the first time I’ve actually had the chance to play with one.

what do you think about adding the plotjuggler config and a screenshot to the docs? Feel free to record any useful time series.

Thank you so much for pointing me to the plotjuggler tool, really love it ❤️. I added a few python scripts with hope to simplify some of the steps. Let me know your thoughts on these.

I also has an issue with plotjuggler which I'm not able to get data for pid_controller command interfaces, i.e. there is no plot for command_interface.pid_controller_left_wheel_joint/left_wheel_joint/velocity. Is there additional step that I need to do to accomplish this ? Once I get a proper diagram, will add to the documentation.

Screenshot_2025-02-17_23-33-03

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, this looks great now.
With ros-controls/ros2_control#2050 the command reference interfaces are reported here, can you confirm this?

@Juliaj
Copy link
Contributor Author

Juliaj commented Feb 19, 2025

Thanks, this looks great now. With ros-controls/ros2_control#2050 the command reference interfaces are reported here, can you confirm this?

That's it. Added the diagram!

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.

Two new comments, and still some older open conversations, can you have a look please?

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.

LGTM. Just let's wait for the next release of ros2_control and ros2_controllers before we merge this.

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.

The changes on ros2_controllers got synced to ros2-testing on rolling today, so we can merge that now

@christophfroehlich christophfroehlich merged commit c12d5b2 into ros-controls:master Mar 11, 2025
12 of 13 checks passed
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