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

Publish PID integrator state for joint velocity and position controllers #375

Closed
wants to merge 6 commits into from

Conversation

goretkin
Copy link

the value of the integrator is an important part of the state of a PID controller. I only tested this on groovy, but I don't know whether it's appropriate to add any new features to groovy at this point. Either way, this commit should merge cleanly with Hydro too.

This was useful is discovering #374

@UltronDestroyer
Copy link
Contributor

Sorry I'm not very familiar with controls. What would change visually on the robot if this is implemented?

Also, the .msg file will have a different checksum which will cause big errors for users of this node. (Causing it to be non-backward compatible)

@goretkin
Copy link
Author

This change, apart from breaking the "binary"-ish compatibility you mentioned, doesn't affect any behavior on the robot.

The joint/velocity controllers use the Pid class in the control_toolbox package, which is a PID controller. One of the things in the Pid class is a field which accumulates (integrates) the error, which is an important quantity for the PID controller. And to tune or debug a PID controller, it's nice to have access to its full state, including the value of the integrator.

As far as I know, the purpose of the JointControllerState is for debugging. This PR just makes available some extra debugging information. It is unnatural that the JointControllerState publishes every other single piece of state of the PID controller (all the gains, the derivative) but not the integral.

I don't know enough to know how bad it is to change .msg files the change-the-hash way. It's not the most backwards incompatible since no fields were removed or renamed. But it does require all distributions of ROS to have the same message file.

I just did a search and found some code using it for what appears to be something other than debugging
http://ua-ros-pkg.googlecode.com/svn%27:%27ua-ros-pkg%27,-/trunk/arrg/ua_controllers/wubble_actions/nodes/smart_arm_action.py

https://gitlab.mech.kuleuven.be/rob-itasc/itasc_robots_objects/raw/e3b3824fb2109db8247d2243066bf05e7c03a456/itasc_pr2/src/pr2connect.hpp

@v4hn
Copy link
Member

v4hn commented Aug 10, 2022

replaced by #405

Thank you for working on the controllers 7 years ago @goretkin 🐕

@v4hn v4hn closed this Aug 10, 2022
@goretkin
Copy link
Author

goretkin commented Aug 10, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants