API review

Proposer: Vijay Pradeep

Present at review:

• List e-Reviewers
  • Vijay
  • Jeremy
  • Matei
  • Radu
  • Eric

Please refer to http://www.ros.org/wiki/laser_assembler

Note that only the ROS API is being released for this package.

This review will conducted via email and this page. All comments/concerns received before Friday, Sept 18th @ 5pm will be addressed.

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.

Eric - collection of things to think about, not necessarily all changes.

• I would remove _srv from the end of the node names
  • [Vijay] - Sounds completely reasonable
• grab_cloud_data should be marked as a test program somewhere other than in the source-code
  • [Vijay] - I can add this to the wiki docs
• There are no tutorials or sample configurations for either package
  • [Vijay] - I can add this to the wiki docs
• max_scans should possibly be max_clouds for the cloud_assembler?
  • [Vijay] - Sounds like a reasonable change
• to match the nav_stack, scan_in should be scan_topic and cloud_topic for the two nodes
  • [Vijay] - Sounds reasonable
• Behavior question - does it keep a rolling buffer, or does it only start to record once a scan is requested?
  • [Vijay] - It keeps a rolling buffer, and the service call is non blocking. I can be more clear about this in the wiki docs. I'll save any added functionality for a 2.0 release.
• Error reporting - what happens for failed requests (e.g. start_time after end_time, both time far in the past, etc.). Is an empty message returned? Is a warning sent to rosconsole?
  • [Vijay] - It seems ok leave this as an undefined part of the ROS API. But, I can definitely add ROS_WARNS to the code.
• What frame is the output cloud published in (I assume fixed-frame, but that's not in the docs and is a part of the API)
  • [Vijay] - The cloud is published in the fixed frame. I'll update the wiki docs to explain this

Jeremy:

• Eric caught most of my suggestions already...
• Removing "_srv" from node names would be good.
• The code API mainpage seems out of date. This should probably be removed since it is now in wiki. There should probably just be page that says the laser_assembler does not have a release code API.
• I would suggest that any request which fails should produce a ROS_ERROR, and the servicecall should return false.

• More complete listing/documentation of the AssembleScans service would be good:

  • e.g., the interfaces to it are: request.begin/request.end and response.cloud
• Should clarify that the cloud always be produced in the fixed_frame.
  • There is a question of if completeness you should offer a target_frame as well. Note that because the assembled scan is across time, transforming the assembled cloud to another (non-fixed) frame after the fact will not produce correct results.
  • [Vijay] Because of all the confusion and bugs that arise from a user requesting a 'non-fixed' frame to transform to, it seemed to make much more sense to just have the user do this frame transform themselves. We would also have to specify a bunch of error semantics regarding this transform. Does the service call fail if the transform isn't available? Or maybe the service callback should block until it is possible? Or, maybe it should block until a timeout? Or, since we're transforming to a fixed frame, the timestamp doesn't even matter, so we don't even let the user specify one. It's almost like we're putting a bunch of TF configuration into a service call, when in fact the user can do the transforms themselves. It just seems like there's a huge can of worms here that we could easily ignore, at least for releasing 1.0.
• I noticed that the input topics right now are in a private namespace: "~scan_in".
  • I would recommend moving this out into the global namespace unless anyone has a good reason for it being private.
• Also, I don't know if there is a large value to calling the topic "scan_in" a topic does not have a direction. Nobody will ever be publishing "scan_in" because that doesn't really make semantic sense.
  • I would just use the topic "scan" for the laser_assembler, and "cloud" for the cloud assembler.
  • This means at the command line, you can run a hokuyo_driver, and then run a laser_assembler, and have the default topics connect up to one

another nicely. I think this is a compelling scenario for new users coming into the system.

Matei:

• Caveat: this is the first review I've done so I'm probably not yet familiar with some things that other people here are...
• What is the purpose of the tf_tolerance_secs? Documentation says that it's the time to wait after the transform becomes available, but why would one want to do that?

  • [Vijay] - Normally, the tf::MessageFilter waits until scan_in is transformable at the time scan_in.header.stamp. However, the last ray of a scan occurs after header.stamp. Thus, we might want to wait until header.stamp + tf_tolerance_secs is transformable.

  • [Vijay] - Wow, that's not a straightforward explanation at all. I guess it might be nice to somehow hide this from the user. I can calculate this tolerance on a per scan basis, but there's currently no way to feed this time into the tf::MessageFilter. Maybe the tf::MessageFilter should support this?

• All my other questions are about the snapshotter
  • it might be unclear to somebody outside of Willow what we mean by a snapshot. Maybe an explanation about the tilting Hokuyo, and how a "complete scan" means one sweep from top to bottom?
  • in general, it might be good to make it clear upfront that in our context "scan" means a single batch of data from the scanner, which for a line scanner is just one stripe.
  • does the snapshotter for the Hokuyo exist already? The documentation says "This can be done by..."
  • does the snapshotter set the parameters for the Assembler? In particular, I am thinking about the ring buffer. It would seem that we don't want the Assembler to have a buffer larger than would be needed for a single sweep, as that would waste memory.
  • [Vijay] - I'm beginning to think that the laser_assembler main wiki page isn't the right place to be talking about snapshotters. Maybe this could all live as a "writing a snapshotter" tutorial inside this package? That might clear things up a little bit.

Radu - (a bit late at the party)

• In principle the API looks fine to me, with the exception of a few small changes (suggested above)
• I would take care in explaining that a (laser)_scan is one vector of X individual measurements/hits (usually) in spherical coordinates expressed as distances from an acquisition point to objects in the world, while a (point)_cloud is referred to a collection of 3D {x,y,z} points (usually) in Cartesian coordinates
  • [Vijay] Seems very reasonable to add this background info somewhere near the package overview part of the wiki docs.

• Regarding Eric's comments about max_scans -> max_clouds... I am not sure if that is "correct". As far as the laser_scan assembler goes, the input is "scans", and the output is "cloud"(s). However both cloud_assembler and scan_assembler inherit from base_assembler_srv.h, which is where max_scans is defined. A tricky one!

  • [Vijay] There might have to be some [hopefully very short] discussion about this one
• Umm, so what happens if I give you a laser sensor that outputs data at 500Hz (LMS400)? Can the assembler keep up? What about 1000Hz? etc... This is more a general question on what happens when we have internal (rolling or not) buffers, and the queue fills up. Depending on the user, we might want to have two different behaviors: 1) stop accepting new scans/clouds in the input queue, until a few slots are processed; 2) delete the oldest scans/clouds and accept new incoming data. PS. You don't need a laser sensor to test this, just make a different node that sends data fast.
  • [Vijay] - Yes, there definitely are more specialized behaviors that we could implement. But, for now, we've gotten by just fine with just a rolling buffer implementation. I'm willing to say that for a 1.0 release, only a rolling buffer is sufficient.

• Is there any hope to get "PCD" as an longer term acronym for referring to Point Cloud Data? It seems more unique than "cloud", and it's being used in the 3D depth processing community. We already use it as a file extension for saving/dumping individual datasets to disk when we want to bypass/circumvent the ROS bag mechanism. I'm not pushing it - just asking whether we should start using it at the root (which is the assembler it self!)

  • [Vijay] - I guess I haven't been tracking the perception folks' PCD discussions. We could definitely add a "Dump to PCD" service call, but that's definitely outside the scope of the 1.0 release.

Meeting agenda

Conclusion

API Conditionally Cleared

ROS API Changes

• Remove _srv from node names

• Change "max_scans" topic to "max_clouds" for the point_cloud_assembler

• Remove "tf_tolerance_secs" param. Compute tolerance on a per scan basis before feeding each into tf::MessageFilter

• Pull topics and services out of private namespaces

• Input topic for laser_scan_assembler should be 'scan'

• Input topic for point_cloud_assembler should be 'cloud'

Documentation

• Be more descriptive in overview as to what scans and clouds are, and what this node is actually doing.

• Document grab_cloud_data

  • (removed app instead. It didn't belong here anyways)

• Provide tutorials or sample configurations

• Explain behavior better (stores rolling buffer)

• Document what the frame of the assembled cloud is

• Remove the out-of-date doxygen mainpage

• Move snapshotter description somewhere else. Maybe a tutorial page.

  • Ticketed ros-pkg #2870