diagnostic_aggregator/Reviews/Jan_12_2010_Doc_Review

Reviewer:

• Blaise
• Josh

Instructions for doing a doc review

See DocReviewProcess for more instructions

1. Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?
2. Are all of these APIs documented?
3. Do relevant usages have associated tutorials? (you can ignore this if a Stack-level tutorial covers the relevant usage), and are the indexed in the right places?
4. If there are hardware dependencies of the Package, are these documented?
5. Is it clear to an outside user what the roadmap is for the Package?
6. Is it clear to an outside user what the stability is for the Package?
7. Are concepts introduced by the Package well illustrated?
8. Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers?
9. Are any mathematical formulas in the Package not covered by papers properly documented?

For each launch file in a Package

1. Is it clear how to run that launch file?
2. Does the launch file start up with no errors when run correctly?
3. Do the Nodes in that launch file correctly use ROS_ERROR/ROS_WARN/ROS_INFO logging levels?

Concerns / issues

Blaise

That was easy to read and follow. I have to look at the missing tutorials as many of the details live in there, but the content looks pretty good to me. You have clearly put a lot of time into this.

As usual I have lots of comments. Feel free to push back where you disagree.

• I was a bit confused at first by the analyzer group. If I understand it correctly, you use AnalyzerGroups as nodes in the output tree. The advantage is twofold. First, it avoids typing the name of the tree node for each analyzer (pretty minor advantage). Second, it allows you to load the same .yaml file at multiple places on the parameter server. I suspect that the second advantage is the key one, but that wasn't at all clear in the documentation. So I think that the motivation of the AnalyzerGroup could be beefed up. The example highlights the first advantage, and the second advantage is completely missed. The "move as a group" bit needs to be clarified. (Or perhaps I completely misunderstood what was going on.) By the way, what happens if I put an Analyzer in the same subtree as the AnalyzerGroup? For example, I have an analyzer at path foo/bar and an AnalyzerGroup at foo?

• The links to tutorials seem to be broken on the main page, and there are no tutorials listed. Are they still work in progress, or did I miss something?
• The CreatingADiagnosticsAnalyzer tutorial should be in the diagnostic_analyzers package, and should show up in the Tutorials page for both the diagnostic and the stack.
• I created a blank Troubleshooting page.
• I fixed minor typos. (Note, AFAIK the plural of Analyzer is Analyzers, not Analyzer's. Used mixed typography if you want Analyser to be the class name with an s appended.)
• I ignored formatting, but it is looking pretty good generally.
• If it isn't too painful to produce, a screen shot of the robot monitor with your example tree would be nice. (Not having it doesn't hurt comprehension, so I wouldn't prioritize this.)
• The ROS API section is missing. It should have the ROS API of the node formatted using the standard macros (see microstrain_3dmgx2_imu for an example).
• I usually put an explicit section on API stability to make it really clear what is and isn't stable. (If it's all stable, I make that explicit too.)
• I diagree with writing "look in the demo/ directory". I find that the / is unconventional. This may be subjective so I didn't make the tweak.
• There is a lot of replication between doxygen and the wiki. I generally frown on that as it is difficult to maintain. Redundancy is good, but having the same example with the same text at both places is a waste of your time. Generally, I put the terser nitty-gritty code API documentation in doxygen (that's what doxygen does well), and put the more general stuff in the wiki (easier for folks to tweak, findable by the wiki search).
• I would recommend having a brief Code API section on the wiki with links to the key class pages in doxygen. Currently the information is there but more diluted. (This recommendation is totally arguable. It is just my way of doing things.)

• What does "generic_analyzer holds the GenericAnalyzer class" mean?

• The tutorials seem to be more explanatory than others I have seen. Most tutorials I have seen take you by the hand, and march you down the nitty-gritty of making something work. These are great examples, but they don't seem to be doing the same thing as a typical tutorial. With typical tutorials, I could be pasting commands into a shell, to get something working. (This comment applies to the GenericAnalyzer tutorial. The Creating a Diagnostic Analyzer tutorial is awesome.)

• When you take a screen shot of the robot monitor, make it as small as possible. No need to have tons of blank space as in the one you have in CreatingADiagnosticAnalyzer. It doesn't even fit on my laptop's screen. No need to fix this, but keep in mind for next time. (Or you could cut out slices of the screen shot you have to fake a smaller window for this time.)

Response by Kevin

• I think the AnalyzerGroup stuff must not be clear. I find it super useful, but it's not a huge deal. I'll talk with you about it in person, and we can decide how to describe it.

• Added ROS API section
• Fixed "demo/", "generic_analyzer holds ...", etc

• Using the GenericAnalyzer is a little different. I didn't think the dox was clear on how and why to use everything. It's just a collection of examples, but I think it'd be useful. I saw Brian try to configure and use the GenericAnalyzer, and I think the extra help is necessary. A power system and HK driver are a little different.

• robot_monitor screen shot should be smaller, I'll try to fix that.

• Fixed broken links
• Added content to troubleshooting page

Josh

• Analyzer/Aggregator information is too intermixed. Someone configuring an aggregator (section 4) does not need to know anything about the internals of the analyzers (section 3). I would either break these into separate sections (first all the aggregator information, then information about creating your own analyzer) or move the analyzer stuff to a separate page.
• The documentation makes it seem like the aggregator is tied to the robot monitor, which is not true. Should probably mention that the aggregator exists to make creating something like the robot monitor easy.
• Documentation should not reference PR2 at all, other than perhaps to link to examples of analyzers. This includes tutorials.
• "Configuring Diagnostic Aggregators" tutorial
  • Testing involves "pull[ing] a diagnostics logfile from your robot". A bag file or test app needs to be provided in the tutorial.
  • References to the pr2 should be removed. section 7 should not exist
• "Creating a Diagnostic Analyzer" tutorial
  • Using an example from the PR2 here really hurts, since it makes things confusing and requires knowledge of the PR2 to really understand what is going on (powerboard, ethercat, runstop semantics, etc.).
  • Section 5.5 (Making Regression Test) doesn't belong
• Code API
  • Virtually the entire main page should be removed, since it's duplicated in the wiki and is likely to become out of date (and already is... references to namespaces like generic_analyzer that do not exist). Code API should really just point at the analyzer API
  • Uses of diagnostic_aggregator that are not meant to be links to the diagnostic_aggregator namespace should be prefixed by % so doxygen won't auto-link them
  • Is the Aggregator class meant to be used anywhere external to the aggregator node? If not it should be mentioned somewhere in the description.

  • Not sure how this didn't get caught in the API review -- all the "const std::string" arguments to methods should really be "const std::string&". I know you can't change this now, but it should get put onto the list for future versions (current version causes a copy of the string to be created for every call)

Conclusion