## page was renamed from diagnostic updater/Reviews/11-1-2009 API Review
= 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

----
## PackageReviewCategory