ros_controllers
ros_controllers copied to clipboard
controller state should include information about the integrator state
...otherwise it's not truly the state of the controller.
Here is what I mean: https://github.com/PR2/pr2_controllers/pull/375
What specific controller plugin(s) and controller state message are you referring to?. Many controllers report state, and not all use the same message type.
For example, https://github.com/ros-controls/ros_controllers/blob/48a224ecf60473f99a9ccc9d10bf387c60012b0a/effort_controllers/src/joint_position_controller.cpp#L216, which uses the message defined here: https://github.com/ros-controls/control_msgs/blob/3f383c46ac0a25254c6e5c09614970e38dcd1b13/control_msgs/msg/JointControllerState.msg
So at least all the controllers that use the current version of JointControllerState
message: https://github.com/ros-controls/ros_controllers/search?utf8=%E2%9C%93&q=JointControllerState
Probably it should be the responsibility of the Pid
class to package its own state message in some new PID message type. If a controller uses a PID controller and has other state, it can define a new message that includes a PID message. This will reduce code duplication across the controllers currently in the ros_controllers package.
It also reduces code duplication across all the controllers that use Pid
For example, there are 6 PID controllers here (from the robot_mechanism_controllers repo, which I don't know where it fits into the new ros-controls ecosystem) https://github.com/PR2/pr2_controllers/blob/02376d1a1899b71f561dc0e669786cfbc4d3d512/robot_mechanism_controllers/src/cartesian_pose_controller.cpp and in general, you'd have to write message-packing code for each controller plugin that uses the Pid class if you'd like to report the state of the controller.
I am also having some difficulties due to not having direct access to the integrator state. +1 from me
At the moment we don't have resources to implement this. Please feel free to propose changes on a pull request.
@bmagyar Fair enough.
@goretkin If you are still interested in this, can we discuss the new API/messages a bit more? Otherwise I will probably do something similar to what you but try to retain backwards compatibility. So probably leave the current state message alone, and add a JointControllerState2
leveraging the PidState message.
While I'm at it, I might also take a crack at #41 (the making state publishers lazy part).
How about publishing a PidState message directly?
@nstiurca I never got around to using ros-controls. The only ROS robot I use is the PR2, and I don't think ros-controls works on it. That's just to say I might not be able to test it.
PidState
message appears to be a "recent" development. I think it's a good idea for the control_toolbox::Pid
class to publish the PidState
message directly, under the namespace of the PID controller, which is currently maybe fixed https://github.com/ros-controls/control_toolbox/blob/ba1100638a48a461e4e642150f4ae666303e3ecc/src/pid.cpp#L48 (what happens if you instantiate two Pid
classes under one controller?). I think it would make more sense for PidState
to likewise be defined in the control_toolbox package.
@bmagyar @goretkin It looks like publishing PidState
per PID controller already exists. I just tested it and it works, at least in Kinetic.I think this more-or-less serves my purposes for now, even if it's a little cumbersome to synchronize the JointControllerState
with the PidState
messages.
Perhaps this is a documentation issue at this point?