API review

Proposer: your name here

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.

Caroline:

• wide_stereo_cfg --> wide_stereo_config

  • Blaise: Going for wide_stereo as per #ros-pkg3598

• "To avoid having texture in images taken by the Prosilica camera, the prosilica_projector_disable option can be turned on. In this case, an exposure signal from the Prosilica camera directly inhibits firing of the projector. When the projector is inhibited the WGE100 cameras will continue to be triggered as if the projector was active, so some frames that should be textured may be partially textured or not textured at all."
  • So the narrow cameras will still publish on the /narrow_stereo_textured topic? Could that become misleading? Could the Prosilica exposure trigger + prosilica_projector_disable stop the narrow cameras from publishing erroneously textured images?

    • Blaise: I would like to, but this is a really difficult one to do well given the number of asynchronously communicating nodes that are involved, and given that you might have a half-textured image. It would be much simpler for now to detect downstream that image_proc did really badly on a particular image.

• "It is not recommended that the user adjust parameters that are not listed below as they are likely to leave the system in a non functioning state."
  • Are other (dangerous) parameters visible in reconfigure_gui? If so, can they be made invisible?

    • Blaise: Currently reconfigure_gui is just like rosparam. It shows you everything. There are plans to add support for limited reconfiguration sets to reconfigure_gui, but that is past M3, and is a dynamic_reconfigure problem.

Radu:

• This parameter seems too "custom" to our current setup.
  • ~prosilica_projector_disable (bool, default: True)

How can we make the projector + stereo work with the prosilica too? It looks like we have to get a setup where stereo + all the other cameras in the system work fine.

• Blaise: The whole node is totally custom to our current setup. The classes involved could be relatively easily be reconfigured for a different hardware topology, but that would really be wasted effort. The prosilica_projector_disable selection needs to be somewhere, and this is by far the most natural place to put it.

• The description of the ~projector_pulse_length parameter (double, default: 0.005) is wrong, because the range is given between 0.001->0.002 but the default is set to 0.005

  • Blaise: Good point. And by the beauty of autogenerated documentation, this allowed me to fix the bug in the .cfg file from which it originated...

• The ~stereo_rate parameter can only be set up to 60Hz, but that seems a hardware limitation of the current cameras that we have. Ideally, the code could be general and the camera would report what is the max it can do.

  • Blaise: We can expand this if and when we have better cameras. Given that this node is totally hardware-specific. I think it is reasonable to leave that limitation in for now.

• ~projector_tweak should be removed for a release.

  • Blaise: I disagree. It would make debugging much more difficult. And debugging is an ongoing process with a system this complex. If you take out the knobs, then the day somebody needs help, you can't figure out what is going on without taking their system down and putting the knobs back in. If the system happened to be in a difficult to reproduce bad state, you're out of luck. When we get simplified reconfigure_gui capibility, that one is definitely not going to be in the simplified interface.

Matei:

• I guess some of the comments below are rather for a doc review than an API review, but might as well throw them in.

  • Blaise: Excellent, it's never too early to improve documentation.

• Section 5: I had written a couple of comments asking for a high-level overview as I was going through the page, until I got here. I suggest moving this section all the way to the top, right after the Package Summary - without it, many concepts in the previous sections are hard to understand for the novice user. It is a very good overview, clear and detailed. Section 6 could stay towards the end, as it looks like it's oriented more towards the advanced user.

  • Blaise: Done.

• Section 2.2: "For the stereo pairs, the wge100_multi_configurator node should be configured" - what does this mean? The rest of the section goes on to talk about other nodes (wide_stereo_cfg, narrow_stereo_cfg and forearm_camera_node)

  • Blaise: Made it clear that they are one and the same.

• Section 4: what does a hard reset of the cameras mean? When might it be helpful? What does it do different than a soft reset? Is there a soft reset?

  • Blaise: Added details.

• ROS API
  • double check the stereo rates constraints as being multiples / divisors of the projector rate, there are some inconsistencies.

    • Blaise: Fixed.

  • the graph showing the pulses is really good, but it is inconsistent with the explanations just below it. E.g., the text says that "with projector" cameras expose during pulses 1 and 3, but the image shows them during pulses 2 and 4.

    • Blaise: Fixed.

  • for all the stereo parameters that get rounded because of the projector rate, is this visible to the user? If I set some value using the reconfigure GUI, which then gets rounded to something else, does the value shown in the GUI also get updated?

    • Blaise: Added a section mentioning this explicitely.

• what is the default behavior? From my experience with prg, the projector_mode comes on as Auto and both stereo cameras trigger modes are set to "without projector". Is this going to be the default for the betas?

  • Blaise: Yes, this will be the default. I have hinted at it being the default for the projector mode. I won't say what the default is for the camera mode to encourage people to set it explicitly each time.

Meeting agenda

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

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

---