API review

Proposer: Blaise Gassend

Present at review:

• Kevin
• Tully
• Jeremy
• James

Scope

The self_test API includes:

• The Dispatcher class (see doxygen for C++ API, ROS API is just a ~self_test service)
• The run_selftest program that calls self_test on a node and pretty-prints the output (no documentation yet)
• The selftest_rostest node that makes a rostest by running a node's self-test (no documentation yet, an example can be found in wge100_camera/tests).

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

• The node handle parameter to the constructor seems to be unused, or made to default to "~".
• Should the owner parameter to the constructor be deprecated and removed, or is it okay?
• Need a way to use this in single-threaded context. Currently prevented by the fact that you need the service callback and checkTest to be called concurrently.
  • Tully: If the single threaded model cannot be achieved, this should spin it's own callback queue for the service call.

Tully

• in Dispacher, it should be possible to pass fully qualified functions, not require them to be a member function of "owner" I would recommend removing owner completely.

• Why is DiagnosticTaskVector inherited publicly? It seems like it could be a private member variable.

• the selftest_rostest delay seems a little odd. Could you replace it with a waitForService call?

Meeting agenda

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

Notes

• boost::get_system_time is being used in the timed_wait. This might be ok.
  • Check if there's a more ROS-friendly way of doing a condition timedwait
    • BG: Jeremy (self_test) and I (timestamp_tools) both seem to have concluded that boost::get_system_time was necessary when implementing condition timedwait. In any case, the conditions went away when making this single-thread compatible.
• id_ gets reset every time the rest is called because we always want to make sure we get the real id from the device.
  • BG: No action item here.
• Defaulting nodehandle to "~" makes sense
  • BG: No action item here.
• self_test shouldn't take a owner in the constructor
  • This can get rid of Templating, which is a win

    • BG: Deprecated Dispatcher<>, and replaced it with Sequencer, which does the same thing without pretest, posttest, owner or templating. Much cleaner API now.

• not currently single thread-compatible
  • Appropriate solution is probably to only service the service callback queue inside of the check test function
  • Potential problem: this means your service call may hang
  • Could spin internal thread to deal with this
  • Hanging on service call if it can't get to it is probably fine
    • BG: Done. Simplifies the code a lot too.
• Looks like selftest_rostest is using delay to wait for service, just using wait for service is probably the correct implementation
  • BG: Replaced with waitForService. Took out the delay parameter, added in a max_delay parameter.

Conclusion

Package status change mark change manifest)

• Blaise will change code/API from notes as appropriate

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

• Reviewers will check off at that point if appropriate