API review

Proposer: Wim Meeussen

Present at review:

• List reviewers

Question / concerns / comments

Gunter

Controller Interface:

• 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.
• 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?

Mechanism Control:

• 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?
• List of controllers: can that also show timing information (percent of 1ms cycle?)?
• Can I set/change the 1 KHz cycle? How?
• mech.py spawn/kill = load/unload?
• mech.py shutdown - what is it? Does it still exist?
• How are the safety limited initialized? This is part of pr2_ethercat?
• Is the velocity filter part of ethercat too? Probably outside this scope...

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

• BufferedData: What is this? I don't understand...

• The only messages that I understand are Joint/Actuator/Controller State....

  • ControllerState: How are times defined? Why float64? What is their unit? And over what time frame are mean/variance calculated?

  • ActuatorState: Why is time a float64? Why include encoder_velocity? I don't understand the calibration_reading and other calibration data? requested and commanded current/effort - naming doesn't make it very clear which is which (or what their difference is).

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

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Add percent duty cycle on list controllers

• A timing histogram would be nice

• Make sure that realtime loop warns if it drops out of realtime

• Make sure mech.py shutdown is gone

• Add load/unload to spawner and service w/ clean semantics keep spawn for backwards compatibility

  • mech.py(methods: load, unload(fails if running), start, stop, spawn(load and start), kill(stop and unload) )
  • services implement only (load, unload, switch)

• Move SwitchControllerAction into another package like pr2_actions (remove actionlib dependency)

• Move BufferedData for Recorder into realtime_tools and open ticket for ROS to add this aggregated send capability.

• add ros time to jnt/act/contr messages in mech state

• Document calibration_reading 3 bits (value, ever seen high, ever seen low) (displayed in float)

• Requested, commanded, measured is ambiguous Torque and Current

Parking Lot

• Currently no realtime overrun management/enforcement.
  • Consider a complete redesign for the future.
• Look at allowing different update rates.