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

  • Unclear to me why stopping() needs to return a bool

  • Kind of clunky interface to link controllers together, but I think we're ok with that for 1.0.
  • Is stopping() called before a controller gets killed? This should be made clear

Gunter

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

Meeting agenda

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

Conclusion

Package status change mark change manifest)

  • /!\ Action items that need to be taken.

  • {X} Major issues that need to be resolved


Wiki: pr2_controller_interface/Reviews/2009-10-07_API_Review (last edited 2009-10-23 17:46:47 by wim)