API review

Proposer: Jack O'Quin

Present at review:

Patrick Mihelich
Eric Perko
Ken Conley
William Morris

Question / concerns / comments

(Jack) Since the beginning, this package needed a way to handle multiple calibrations. A simple, yet compatible, extension is adding shell-like substitution variables to the URL:
- ${NAME} camera name
- ${RESOLUTION} current image size (width x height)
- others? How would we encode focus or zoom?
- (Bill) The most important part is an ID to differentiate two of the same camera, where you have one attached to the left side and one attached to the right side of a robot and each one has a different calibration. This is probably also the most common use case. The ID will be hardware dependent, but we can probably limit it to hex or something. For USB devices the vendor id, product id, and serial number can form a unique id. Firewire cameras can use the guid. Devices that cannot be disambiguated could have an ID of NULL. example: camera with a serial # of 1234 looks for cam-1234.yaml, if it can't find that it looks for cam.yaml
(Jack) It probably makes sense to move the default URL from /tmp to $ROS_HOME. That is incompatible with C-turtle and Diamondback, but not likely to cause much trouble.
- (Bill) I think this makes sense and should cause few problems. We could output a reminder warning message about the change. ROS_WARN_ONCE
(Patrick) I think ${NAME} is sufficient for now. I'm not sold on ${RESOLUTION}; whenever possible, REP 104 recommends calibrating the camera at its max resolution, and describing all lower resolutions through binning/ROI. And for variable focus/zoom, I don't think we have enough experience to mandate a standard for those yet. Instead we can recommend that camera drivers include the relevant parameters in ${NAME}.
- (Jack) Good point about REP 104. But, with IIDC cameras the only way to support binning and ROI is with Format 7. Many devices do not support those video modes, and some that do are more reliable in Format 0, 1 or 2 modes. So, for many of those devices a way to resolve the URL depending on video resolution would be helpful.
  - (Patrick) The perfect solution would still be to express the various Format 0/1/2 modes in terms of the max resolution, but I understand that's hard to do in practice. What if camera1394 made the resolution part of ${NAME}, or even resolved ${WIDTH} and ${HEIGHT} itself. I'm somewhat against supporting ${WIDTH} and ${HEIGHT} at the level of CameraInfoManager, because I don't want to encourage multiple resolution-dependent calibrations as a general practice.
    - (Jack) Camera1394 could work that way. During the original driver review we discussed making the camera name a user parameter, but ended up using the GUID of the device instead, a good choice. The driver could legally append an _ and the current video mode to the 16 hex digits of the GUID, yielding names like 08144361026320a0_640x480_yuv411 and 08144361026320a0_640x480_mono8. All those characters are valid for a camera name. I don't know whether the calibration actually depends on the color mode, but the video_mode includes it. (I have no plan to make this change to the driver, but it could be done later if desired.)
- (Jack) Note that setCameraName() insists that the name be non-empty and contain only alphabetic, numeric, and _ characters. So, a / would be rejected. If / seems necessary, we can extend the syntax.
  - (Patrick) I don't see a need for /.
    - (Jack) Me, either. I prefer not to allow it unless there's a need, since it can alter the URL syntax.
- (Bill) I have been a really big proponent of verbose naming based on extensive testing and calibration of many usb cameras. Some cameras support 4:3 and 16:9 AND 16:10 modes. I know of at least one camera that has a different FOV at different framerates. The Logitech C910 finally supports autofocus properly in linux, so that is on the way. All of these issues yield different calibrations.
  - (Bill) One solution is to redesign the yaml to support multiple calibrations per file. Then the file can be driver-id.yaml and all of the resolutions, framerates and etc can be in the yaml instead of encoded into the filename. I think ID needs to be in the file name to associate the calibrations with a physical device.
    - (Eric) I don't really like the multiple calibrations per .yaml file. I like the idea that the calibration .yaml corresponds to 1 CameraInfo message, otherwise we may end up with huge .yaml files for complicated cameras.
(Jack) If we do support resolution, I would prefer to define ${WIDTH} and ${HEIGHT} separately. Specifying those values might simplify usage of getCameraInfo(), allowing camera_info_manager to check the resolution and automatically return a null CameraInfo value if the current resolution is not handled. (But, with binning and ROI that can get complicated, so it might be worth postponing until a future release.)
- (Bill) USB cameras need height and width since 4:3, 16:9 16:10 aspect ratios are commonly supported and the calibrations can't be reused.
(Jack) I propose a single pass for resolving variables in URLs.
(Jack) I suppose the URL syntax needs to allow $$ as a way to specify a single actual $ like the shell does. Sigh. That sort of logic gets tiresome.
- (Jack) Dumb idea. The shell resolves $$ to the current process ID. To escape a $, one must use \$, which is even worse.
(Jack) Since we're adding all this string processing to the URL, we should consider resolving the values of environment variables like ${ROS_HOME} and maybe ${HOME}.
- (Eric) I'm not sure where this would be all that useful. I don't see why the camera_info_manager needs to use these - these can be set by roslaunch. We need to add things to the URL that roslaunch doesn't know about, like current device settings. When would we recommend doing something like $(env HOME)/calibration/camera.yaml vs ${HOME}/calibration/camera.yaml as the parameter string for the URL?
  - (Jack) We probably should not recommend using ${HOME} in either case. ${ROS_HOME} might make sense. I'd like some convenient way to specify paths relative to there. Not all URLs are specified in roslaunch files, rosparam YAML files are also convenient places to store them. Users can provide one for each camera.
    - (Eric) That's a good point. I didn't think about the yaml files being directly loaded to the parameter server and not going through the roslaunch substitution stuff. I don't really like the idea that the camera_info_manager substitutions could depend on the running environment. I'd much rather them just depend on the current camera driver state. What happens if I don't set $ROS_HOME? Would we have a default and would that default have to be updated to keep pace with any changes in say, roslaunch (or whoever sets ROS_HOME if it's not setup right now)?
  - (Ken) roslaunch doesn't have explicit knowledge of ROS_HOME in the event that the env variable isn't set, but it makes sense to add this knowledge. It won't take long to implement, but I'll need some heads up and guidance, e.g. should $(env ROS_HOME) auto-derive ROS_HOME (i.e. have a different contract than env), or should there be an explicit $(ROS_HOME)?
    - (Eric) So, if ROS_HOME isn't set, who defaults it to ~/.ros?
      - (Jack) I plan to do that explicitly in camera_info_manager, using whatever syntax seems best to us. (Maybe making it look like an environment variable isn't so good.)
      - (Ken): Eric -- the default value is ~/.ros, the environment variable overrides the value. Thus, any env-central syntax is actually specified against the override, not the actual value. Patrick's comment below shows the alternative.
  - (Patrick) I guess you can already do $(optenv ROS_HOME ~/.ros), but I like $(ROS_HOME) as a less error-prone shorthand.
- (Bill) I was thinking more along the lines that by default camera_info_url set in the parameter server would work the same as it does now, but if camera_info_url was set to null or "auto" it would load the calibration from $ROS_HOME/calibrations/drive_name-id.yaml
(Jack) Instead of making ${ROS_HOME} look like an environment variable, file:///${ROS_HOME}/calibrations/${NAME}.yaml, we could define a new URL prefix, roshome:calibrations/${NAME}.yaml, with equivalent semantics. Should there be some extra slashes, like roshome://calibrations/${NAME}.yaml? If so, how many?
- (Bill) Is this useful for other applications? Would a mapserver store maps in roshome://maps/0123.map? I think this is a more general problem with ROS that needs a more general resolution.
  - (Eric) +1 for adding this to the same package that supports "package://", "file://" etc. I forget the name of that package.
    - (Jack) I don't know of any such package. There probably should be one. The URLs in camera_info_manager are handled internally, likely a bad idea.
      - (Ken): resource_retriever
        (Jack) That looks like a good place to add a roshome: tag that would be generally useful. It does not currently provide all the interfaces needed by camera_info_manager, but could probably be extended as needed for Fuerte. For Electric I think we should just implement the ${ROS_HOME} syntax.
(Jack) With good ${ROS_HOME} support, we would not necessarily need to change the default for an empty URL, file:///tmp/calibration_${NAME}.yaml. That would retain strict compatibility with previous releases. Users could specify their own generic ${ROS_HOME} location, if desired. (I could go either way with this, let me know what you think.)
- (Bill) If you want to maintain compatibility we could require that the url is set to "auto" for it to automatically locate the correct calibration.
  - (Jack) That would make auto a new URL type (I guess it should be auto://). I'd really rather not add that. We can redefine what "" means easily enough.

Meeting agenda

Time is short to resolve this in time for Electric Emys. I would like to collect all comments on this review page by 2011-06-13. Then, we can resolve any remaining issues by e-mail.

Conclusion

$/!\$ Resolve ${NAME} in a URL to the currently-defined camera name. In documentation, encourage camera drivers to use device-specific unique names plus resolution or other information, when appropriate.
$/!\$ Resolve ${ROS_HOME} in a URL, using the environment variable if defined, ${HOME}/.ros otherwise.
$/!\$ Only substitute when a $ is immediately followed by {. If ${ is found but no valid substitution variable follows, treat the string literally and log an error message.
$/!\$ Change the default path of a "" URL from file:///tmp/calibration_${NAME}.yaml to file:///${ROS_HOME}/camera_info/${NAME}.yaml. Load data from the same place if available, which was previously not done.
$/!\$ Add a new CameraInfoManager::resolveURL() public method that substitutes URL variables, mainly for unit test purposes. Use that to resolve names when accessing CameraInfo files.
$/!\$ When saving CameraInfo data to a file, create missing parent directories when needed and if possible. If that fails, log an error.