API review

Proposer: Wim Meeussen

Present at review:

• List reviewers

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Vijay

• Should we also be discussing how controllers get parameters, or is this out of the scope of this review?
• publish_rate_joint_state sounds a little odd. joint_state_publish_rate might make more sense. This is simply my personal preference.

• Can we come up with a more descriptive name than mech.py?

  • mech_config.py, controller_config.py, mechanism_control_config.py

• We have a JointState message in both pr2_mechanism_msgs and in sensor_msgs. Does this node publish both?

  • Seems a little odd that the joint_states topic is of type JointState. Seems like an artifact of API changes.

Sachin

• One frequent request from users was to be able to set joint gains on the fly without bringing down controllers.
• We need a method to list all the joints/chains a controller is operating on.
• I don't agree that people will never use the C++ API, that's what is needed to allow controllers to point to other controllers. I think that should be part of the review too.
• Can we implement a better way to sequence controllers by creating some kind of dependency graph?
• Can all the parameters that pr2_mechanism_control uses be exposed in a default launch file - there might be an issue where the params are namespaced differently in simulation and on the robot.
• The diagnostics information from pr2_mechanism_control should still contain the running numbers instead of just averages

Gunter

• Why do realtime functions return anything (bools)? Allowing the start() function (and stop()) to return an error leads down a dangerous road. Error handling in realtime is very tricky. By definition realtime implies a guaranteed behavior, so just like update() I would strongly argue that start() should guarantee proper execution. After all, init() was called to set up the controller. If init() claims all is ready, the start() function should be guaranteed. I.e. have start() and stop() return void just like update().
• The danger of that road starts showing up in the switch-controller services. Defining "strictness" leads to uncertain outcomes - inconsistent with realtime guarantees.... it just seems to make things more complicated for no real gain. More comments in the messages.

• SwitchController Service: As mentioned above, I would strongly argue that starting/stopping controllers should be a guaranteed behavior (no fail possible), which completely eliminates the need for strictness. As is, there is still the question of whether on failure some controllers have been stopped? Will they be restarted? So you can be left with no controllers running? This entire error handling process (which needs to occur in realtime..) is tricky and inconsistent with "realtime". So I think it's both better and easier to avoid entirely.

• SwitchControllerResult: Why is it empty? As well as Feedback? I guess that really brings up the bigger question: What is the action and why is there an action? I thought actions were intended for things that take a finite amount of time, i.e. that are interruptible, etc. Switching controllers happens instantaneously. Either yes or no. So a service seems very appropriate. I guess I don't understand the need for an action (admittedly neither the details).

• JointState: What is the publishing cycle for the min/max values? It wasn't immediately clear to me that these are the "statistics" on the actual values versus the imposed limits.

• Consistent with ActuatorState (and other discussions) should JointState and ControllerState have a time field?

• Missing timing discussion. Not sure whether this is in-scope, but a critical feature of any realtime system is how it handles timing problems. Beyond a discussion of how well timing is guaranteed (if feasible), what happens if a controller takes too long or crashes in some way? Is there any checking? How does the system find out? Is a controller told if it is violating timing? If an update was skipped? So at a minimum there should be documentation about what happens (under timing problems), what checking is in place. And hopefully the interface would provide some response/notification/action? Can one controller bring down the house?
• I'm confused by the get_controller() on two levels: First, why is it part of this class - my controller? It does seem to be associated with this controller, but with the mechanism control in general?
• Also, I am really confused by the ordering aspect. The whole question of ordering is non-ideal (creates dependencies that show up in funny places?) - I would even argue that every controller should be self-contained! They can certainly share code, but I think it's again dangerous to have situations where one controller requires another to run...
• Besides whether to allow ordering, I would think that ordering must be understood when initializing/starting/stopping controllers. Obviously if there is a dependency, then things have to be started correctly. So doesn't the ordering appear there? Why implement the ordering here? Does that lead to two places that both need to understand / repeat the constraints?
• Oh, if there is an init() function, why not an exit() function also?

Meeting agenda

• rename pr2 mechanism control to pr2 controller manager
• rename pr2 ethercat to realtime loop
• make spawner take in yaml file
• how to change gains etc on the fly? Should MC provide support for this or is this controller specific.
• change publish_rate_... to .._publish_rate

• change mech.py to realtime_loop / controller / ? --> depending on package name

• change JointState to JointStatistics. Same for ActuatorState and ControllerState

• change topic name joint_states to joint_state
• resource management (joint usage) for roadmap.
  • Would be nice to add method to get access to which controller is using which joints
• controller communication on roadmap
• create example launch file
• Comments of Gunter are pushed off to the next meeting. See the notes of that review meeting for details.

Roadmap

• Realtime infrastructure (realtime_tools and mechanism_control/controller loading) aren't fundamentally PR2 specific. We will plan to put them into something like a realtime_loop stack. In structure, we will take inspiration from other packages such as Orocos RTT and figure out how to combine what exists into a good solution. The minimum change would

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

pr2_mechanism_control -> pr2_controller_manager

---