API review

Proposer: Patrick

Present at review:

• List reviewers

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.

Patrick

This API review is for the stack as a whole, as each plugin package has the same API structure and there isn't that much of it.

Plugin parameters

I'm making a breaking change to how publisher plugins retrieve parameters (for example, the compressed publisher has parameters for JPEG quality, etc.). The old behavior is documented in detail here and described briefly below. Possible options for retrieving parameters (happens for every image published/received):

1. Do a searchParam starting from the namespace of the image topic. This is how publisher plugins work in boxturtle/cturtle. This provides a lot of flexibility, but my impression is that it's little used and that a searchParam on every publication hammers the master.

   • Blaise: I am not certain that this hammers the master. getParam should not go to the master more than once AFAIK, at least for roscpp Josh hasn't done anything to dispell that impression that I have. I would be unpleasantly surprised if searchParam was bypassing that optimization. This is one that should definitely get checked with Josh (and Ken for rospy).

   • Blaise: I'm worried that using searchParam may make it really easy to accidentally pull in unintended parameters, unless the plugin parameters have names that makes it very clear that they apply to a particular plugin. For instance, compression_level, is sure to cause collisions at some point, whereas theora_transport_compression_level would probably be okay.

   • Blaise: Is there ever a need to check these parameters at more than 1Hz or so?

   • Radu: I agree with Blaise about the parameter search. I ran into some nasty similar problems lately. One workaround is to make the search really explicit by making sure the parameter names are somewhat unique, but that's not guaranteed to work on the long run I think.

2. Do a getParamCached in the node private namespace (by default, can be changed through image_transport::TransportHints). This is how subscriber plugins are already defined to work. It should (I think?) be easier on the master, it's simpler to have all plugins work this way, and it's forward-compatible with option 3.

   • Blaise What is the advantage of this over the current method? This means that you need to know the node that is publishing to configure the transport, and it means that a given node can only have one set of transport parameters for all its image publications.

3. Use dynamic_reconfigure. I like this the best, but dynamic_reconfigure wasn't designed with this use case in mind. Ideally dynamic_reconfigure would coalesce each plugin's parameters with those of the node, but I don't think this is currently possible.

   • Blaise Even if dynamic_reconfigure could coalesce all the node parameters, I would find it preferable to group each plugin's parameters into a separate dynamic_reconfigure namespace. Otherwise you increase the number of parameters that the user is confronted with when seeing the node, and they already tend to find that there are too many.

So for Diamondback I'm choosing option 2 for all plugins, possibly switching to option 3 if dynamic_reconfigure is up to it.

• Blaise I'm not sure you mentioned why you want to make the change from the old behavior. The proposed solutions all seem to solve different problems, and the old behavior seems to be just as valid as the new. In fact, I personally like the old behavior much much better than option 2.

• Radu: I am not sure if this is relevant, but with PCL nodelets, we often group the nodelets into different namespaces to make it easier to configure their parameters with dynamic_reconfigure. Is that an option for you too here?

ogg_saver in theora_image_transport

I'm changing the topic remap on the command line to be more consistent with other image subscriber nodes, docs. Probably I can fix the problem of not knowing the frame rate.

libtheora

I'm planning to replace this 3rd-party package with a rosdep before the Diamondback release, so ignore it. It only exists due to Ubuntu's libtheora-dev package being broken up through Jaunty, and Jaunty gets end-of-lifed in October.

• Radu: though Jaunty is no longer supported by Canonical, we still need to support it in ROS, until diamondback when we hopefully upgrade our robots too. I had to jump through hoops to get a bunch of packages play nice with Jaunty just because we couldn't drop support for it in ROS yet.

• Patrick: to be clear, libtheora will still exist in cturtle, but I believe I can replace it with a rosdep in diamondback.

Meeting agenda

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

Conclusion

• Use dynamic_reconfigure for parameters.

  • Publisher plugins configurable per-transport, per-topic. E.g. compressed transport params might go in namespace /stereo/left/image_rect/compressed.

  • Subscriber plugins configurable per-transport, per-node. E.g. /my_node_name/compressed.

  • reconfigure_gui probably needs to go to a tree view of parameter groups.

  • Changing multiple configurations (as in searchParam) if needed can be implemented on top of this with something similar to the wge100_multi_configurator.

• Check with the powers-that-be as to whether Diamondback needs to support Jaunty before I remove libtheora.

Stack status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

---