API review

Proposer: Jack O'Quin

Reviewers:

• Bill Morris
• Rosen Diankov
• Blaise Gassend
• Eric Perko

API documentation

The only API exported by this package is the ROS interface for camera1394_node.

Ignore the old camera1394 interface, it is deprecated and only provided for backwards compatibility with alpha testing on Box Turtle.

Road Map

This package originated as a port to ROS of the Player camera1394 driver. Patrick Beeson did the original ROS port. Jack O'Quin is now maintaining it, and has made many API changes, mostly aimed at making it fit cleanly into the ROS image_pipeline. There is probably not much Player code left at this point, but since it is a "derived work", the Player GPL license still applies.

The code has passed through so many hands that it has become somewhat awkward to maintain. But, it does seem to work reliably with a reasonably wide range of IIDC cameras, and it plays nicely with the ROS image pipeline, including calibration.

For those reasons, we want to include it in the C-turtle release in more or less its current form.

I will probably rewrite the code in D-turtle for easier maintenance and a BSD license. That means we need to focus on developing an API that will be extensible for the future.

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.

• (Jack) The Bayer filtering parameters are not strictly necessary for use with ROS, since image_proc provides similar color decoding features. But, there seems to be a performance advantage (not measured) in some cases to doing Bayer filtering in the driver avoiding image pipeline stages. In the D-turtle release, we plan to upgrade camera1394 to include nodelet support, which may solve any performance issues. So, should we:

  • delete the bayer_pattern and bayer_method parameters?

  • retain them just for this release, but mark them deprecated?

  • commit to long-term support for them?

  • (Blaise) I'm not stressed about removing support if it adds no dependencies. I would manly base my decision on how much effort the long-term support requires. If we do decide to remove these, we should deprecate for one distribution, and then remove in the following one to give folks time to migrate.

• (Bill) There are missing video modes.

• (Jack) I recently updated the code to add many of the missing libdc1394 video modes. Due to limitations of the current implementation, some formats and modes are still not supported. That can probably be fixed, but it takes time, effort, and testing resources. These are the IIDC video modes still not supported:

  • All the mono16 modes (probably easy to add, but not implemented yet).

  • The 160x120_yuv444 and 320x240_yuv411 modes (missing color conversions, looks do-able but not certain).

  • All the Format 7 modes (driver lacks needed infrastructure).

• (Bill) Why is whitebalance a string? Why not two ints and set them to -1 to make it auto..

  • (Jack) I agree that whitebalance should be changed. I am waiting until after the review to make incompatible changes that might affect existing users, so we can batch them all together.

• (Bill) I'm showing higher frame rates as well...

   enum FrameRate
   {
       FRAMERATE_1_875, /**< 1.875 fps. */
       FRAMERATE_3_75, /**< 3.75 fps. */
       FRAMERATE_7_5, /**< 7.5 fps. */
       FRAMERATE_15, /**< 15 fps. */
       FRAMERATE_30, /**< 30 fps. */
       FRAMERATE_60, /**< 60 fps. */
       FRAMERATE_120, /**< 120 fps. */
       FRAMERATE_240, /**< 240 fps. */
       FRAMERATE_FORMAT7, /**< Custom frame rate for Format7 functionality. */
       NUM_FRAMERATES, /**< Number of possible camera frame rates. */
       FRAMERATE_FORCE_32BITS = FULL_32BIT_VALUE
   };

• (Jack) Now fixed in SVN, except for the Format 7 setting. Is there a future compatibility issue with the fps parameter being a double? The code currently rounds up to the next higher value in the sequence 1.875 * 2^n. That makes it tricky to add Format 7 support in the future.

• (Jack) In a future release, I'd like the driver to restrict its parameter settings to modes and frame rates actually supported by the device. Libdc1394 provides most of the relevant camera capabilities. Getting that to work nicely with dynamic_reconfigure will be tricky. (Not do-able on the C-turtle schedule.)

• (Jack) Verify that the encoding parameter is one of the sensor_msgs::image_encodings.

  • Which of those encodings are valid for IIDC data?

  • Should this parameter be a dynamic reconfigure enum?

• (Jack) Bad things happen downstream in the image_pipeline if the CameraInfo width and height do not match the Image. This can happen if the user changes the video_mode or camera_info_url on the fly. The driver should detect this situation and log a warning.

  • What should it do for recovery?

  • Publish an uncalibrated CameraInfo with appropriate dimensions?

    • (Blaise) I like this option.

• (Jack) The feature set looks incomplete (brightness, exposure, gain, shutter, whitebalance):

  • What features do we need?

    • (Blaise) It is easy to add more later. These actually sound like the staples. What is the difference between shutter and exposure?

  • What is the valid range of each feature? (-1 to 9999 was an arbitrary choice)

    • (Blaise) Wherever possible, SI units should be used. For example, exposure should be in seconds. If there is no bound on how high the value can be then +infinity is a valid option. Some day dynamic_reconfigure will allow this to be adjusted on the fly, at leas for the GUI...

  • (Eric) Included in the valid range specs we should also note what units the driver expects the value to be in. (For example, is shutter in ms, us, ns? I have no idea just from looking at the API)

  • (Jack) The units are defined in the IIDC spec (Appendix B). They should probably be included in the documentation, if that is really correct. The libdc1394 interfaces allow the driver to read the min and max range for each feature from the camera (once connected). But, I don't see any reasonable way to reflect that information in the dynamic reconfigure GUI at this point. Maybe some day.

• (Jack) Are the parameter names consistent with other camera drivers where that makes sense?

  • (Blaise) I would usually go for the wordier frame_rate rather than fps.

  • (Blaise) We generally have auto_gain as a boolean instead of using a magic value of gain. I don't feel strongly about it, but in my experience it ends up being a bit cleaner.

  • (Jack) I prefer that, too. For one thing it allows the user to toggle between manual and auto settings for A/B comparisons. Also, it's more like the wge100 driver.

• (Eric) Why does the Camera1394Node have public methods such as read or spin? Do you anticipate the node being created by another C++ node or something? I'm a tad confused as to why those are not private methods... being public implies I might want an instance of this class in my own stuff and use those methods to get data from it; that seems like incorrect usage of the class to me.

  • (Jack) That's just crappy code. Since I plan to rewrite it in the next release and since there are no C++ interfaces exported from the package API, I chose to ignore those problems for the moment.

  • (Blaise) I don't believe that this package is publishing a C++ API anyhow (that should be made explicit like here: http://www.ros.org/doc/api/wge100_camera/html/ (And also briefly on the main page for the driver)

• (Blaise) Take out references to "primary register set". They are probably an artefact of copying from the wge100 driver.

• (Blaise) What is the behavior if the guid is not specified?

  • (Jack) it uses the first camera found by libdc1394.

• (Blaise) Is the camera_name parameter actually useful? Could the guid be used instead? (No strong opinion, but it really is a useless parameter...)

  • (Jack) I've been wondering about that, too. There are several options. The guid could be used, but it's not very human-friendly (16 hex digits). At one point I was using the "basename" of the frame ID. I've considered using the camera_name as the default frame ID instead.

• (Blaise) Would it make sense to decouple resolution from encoding in the parameters? (Again one I don't have a strong opinion on.)

  • (Jack) I could be done that way. I don't have enough experience with IIDC to know what's customary. The standard is written in terms of a list of formats and modes that correspond to the video_mode parameter. But, the list gets a bit long for reconfigure_gui when all the possible modes are listed. It would be better if we could restrict the list to the ones supported by that specific camera. In that case, the interaction between resolution and encoding gets complicated due to bandwidth limitations.

Meeting agenda

The review meeting will be held on-line via IRC using #meeting.ros.org at freenode.net.

To give reviewers more time to respond, this meeting has been rescheduled to 2010-06-01 at 3:00PM Central Time (1:00PM Pacific Time).

Conclusion

Review meeting conclusions.

Actions for this release:

• Add int enum type for most feature settings: {Off, Manual, Auto, One-Push}.

• Split whitebalance into two parameters, with feature settings enum.

• Mention that there is no C++ API.

• Add missing frame rates.

• Document feature parameter units where defined in IIDC spec.

• Eliminate encoding parameter, use bayer_pattern to construct Bayer encoding strings, when needed. Do that when bayer_method specifies image_proc as an option.

• Add gamma feature parameter.

• Eliminate camera_name parameter, use guid for camera name.

Additional actions:

• Support additional features: hue, iris, saturation, sharpness

• Set control state to None for features not supported by the device.

• Add a feature Query option that reads the current value and state (if read-out supported by the device).

Questions to answer:

• Can the driver detect the Bayer pattern automatically? (No, it cannot.)

For future releases:

• limit modes, frame rates, etc. to those actually supported by the device
• add format 7 support
• add trigger support