API review

Proposer: Blaise

Present at review:

  • Kevin
  • Tully
  • Jeremy
  • James

Scope

C++ API. Should be fully documented in doxygen (if it has updated itself).

ROS API. Not properly documented yet:

  • Updater has a parameter and publishes to /diagnostics.

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.

Blaise

  • Should the node handle in the Updater constructor default to "~"?
  • Should the updater and the DiagnosticTaskVector be split into separate files?

  • Would be nice to resurrect ~Updater, but not currently possible.
  • split_run could take just one argument. The summary could be merged into another location, and then copied back before publishing. This would make the API cleaner. If I do that then there is no longer any need to have a ComposableDiagnosticTask. Any tasks could be merged.

Kevin

  • Should "node starting up" be an error?
  • Tabs and indenting are completely messed up all over
  • StatusWrapper should warn somehow when called with bad input? We've seen bad input with it before

  • Looking at the prosilica node, it looks like Patrick didn't use the FrequencyStatus. Why not? Do we need a tutorial for this? He also used the deprecated stuff, even though he just added it.

  • The deprecated stuff should be removed ASAP
  • Needs HW ID field support. This should be part of every driver (#3221)

Self Test

  • Should we switch to ros::Time instead of boost::get_system_time
  • Why does it reset the id_ every time the test is called?
  • How will the single threaded version work?
  • Indents also bad

Meeting agenda

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

Notes

  • Why this package?
    • One thing to help fill out diagnostics
    • Periodic dissemination of information
    • Common diagnostics
  • Node Starting up should be info and not an error.
    • BG: Done.
  • Need good tutorials on non-deprecated API.
    • You will now find example.cpp that gives usage examples of most of the API.
  • HW ID Support:
    • The updater should force people to set hardware id or WARN. Things without hardware IDs can set "None"
      • BG: Added a warning if all the diagnostics are OK but the hardware id is not set.
  • Passed in nodehandle can become optional and default to "~"
    • Waiting to see what Wim concludes in the name harmonization process to decide which optional NodeHandles to pass in here.


/!\ Edit conflict - other version:


  • BG: After discussion with Jeremy, just took the NodeHandle parameter out for now. We can add it as a non-API-breaking change later if a use case springs up. Jeremy says that the way in which we might want to push /diagnostics into a namespace is currently under discussion so it doesn't make sense to settle here yet.


/!\ Edit conflict - your version:


  • BG: After discussion with Jeremy, just took the NodeHandle parameter out for now. We can add it as a non-API-breaking change later if there is a use case.


/!\ End of edit conflict


  • Check how frequency parameter can be changed. Make it check every time update is called.
    • BG: Done.
  • Possibly pull out common dependencies of diagnostic_updater and self_test.
    • Probably not since this requires touching a lot of code
      • BG: No action item here.
  • Would be nice to send message at destruction, but ROS is already down at this point.
    • At the moment when diagnostics stop being broadcast, we get "stale" messages
    • Indicating a "clean shutdown" would be nice
    • Ticket this as a feature request, and talk to Josh about how to make this possible with ROS
      • Maybe request shutdown_callback from ROS
        • BG: Ticketed, no further action item for now.
  • Would be nice to make it so all DiagnosticTasks are Composable

    • BG: Done.
  • Action Item:
    • Document intended API usage.
    • API Review could benefit from an example
  • This can't be run in the real-time loop. This is not something we should support. People doing diagnostics in realtime are responsible for calling update outside of the realtime loop.
    • BG: No action item.

Conclusion

Package status change mark change manifest)

  • /!\ Blaise will change code/API from notes as appropriate

  • {X} Blaise will tie examples to some extra documentation on intended usage

  • /!\ Reviewers will check off at that point if appropriate


Wiki: diagnostic_updater/Reviews/11-1-2009 API Review (last edited 2009-12-10 08:31:08 by BlaiseGassend)