controller state should include information about the integrator state

[goretkin]

...otherwise it's not truly the state of the controller.

Here is what I mean: PR2/pr2_controllers#375

[adolfo-rt]

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.

[goretkin]

For example,

ros_controllers/effort_controllers/src/joint_position_controller.cpp

Line 216 in 48a224e

if(controller_state_publisher_ && controller_state_publisher_->trylock())

, 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.

[nstiurca]

I am also having some difficulties due to not having direct access to the integrator state. +1 from me

[bmagyar]

At the moment we don't have resources to implement this. Please feel free to propose changes on a pull request.

[nstiurca]

@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).

[bmagyar]

How about publishing a PidState message directly?

[goretkin]

@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.

[nstiurca]

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