Feature/revise sonar image comment #13
Conversation
acoustic_msgs/msg/SonarImage.msg
Outdated
| # acquisition cycle is defined by the sonar driver. It is taken to be | ||
| # a best estimate of the time when the sonar data is valid, which will | ||
| # the timing information available from the sonar hardware. | ||
| # The relationship between the header timestamp and the sonar data |
There was a problem hiding this comment.
I think this makes it less clear.
There is no other place to "specify otherwise" -- this timestamp is what all downstream processing should use, and it will all assume that it is the time that the data is valid.
There was a problem hiding this comment.
OK, I'll take another crack it...
|
I think that these suggested edits reveal a significant philosophical difference in how we think of this message =) The re-written comment suggests that the message should support both 2D and 3D imaging sonars, which I disagree with. I think that the data from those instruments will need different processing approaches and thus aren't interchangeable. e.g. what are you going to do with the sonar viewer that subscribes to data that turns out to be 3D? I think some of the confusion came in the elevation angles variable -- that wasn't meant to describe the elevation of multple fans in an array, but rather to describe the elevation of individual beams in the fan. This is necessary for some of the fancier multi-beam sonars that also include water column data. |
|
Maybe not a philosophical difference as much as mis-remembering the agreed on scope. I'll dial out the discussion of 2D sonars ... |
Removed language on 2D sonars Revised language on azimuth and elevation angles Revised lanauge on header timestamp.
|
Updated based on above comments... |
acoustic_msgs/msg/SonarImage.msg
Outdated
| # | ||
| # len(azimuth_angles) must equal len(elevation_angles) | ||
| # | ||
| # For each beam, the sonar collects a time sequence of acoustic returns. |
There was a problem hiding this comment.
This seems to imply that the data is in TWTT, when we're assuming that the conversion has already been done. Instead, the SoS field is more of a FYI ...
acoustic_msgs/msg/SonarImage.msg
Outdated
| # acquisition cycle is defined by the sonar driver. It is taken to be | ||
| # a best estimate of the time when the sonar data is valid, which will | ||
| # the timing information available from the sonar hardware. | ||
| # The relationship between the header timestamp is the best |
There was a problem hiding this comment.
I'm still not thrilled with this -- though neither the old nor the new is grammatically correct =)
How about:
"""
The header timestamp gives the best estimate of the time for which the data is valid.
The exact relationship between timestamp and the sonar's data acquisition cycle is determined by the sonar driver.
"""
acoustic_msgs/msg/SonarImage.msg
Outdated
| # ahead with positive to starboard or up, depending on the orientation | ||
| # of the sensor | ||
| # Azimuth for center of each beam in radians | ||
| # Unless otherwise specified, zero azimuth is assumed to |
There was a problem hiding this comment.
I don't like "unless otherwise specified" comments, because there's nowhere to programmatically specify additional info.
Zero azimuth will be along the sensor's X-axis, by definition, which is usually straight ahead from the sensor.
acoustic_msgs/msg/SonarImage.msg
Outdated
| float32[] azimuth_angles | ||
|
|
||
| # Elevation for each "row" of beams in radians. If left empty, the | ||
| # source is assumed to be a 1D sonar with an elevation of { 0.0 }. | ||
| # Elevation for each "row" of beams in radians. |
There was a problem hiding this comment.
You still have embedded 1D/2D assumptions here.
|
@amarburg -- OK, done with the next pass =) |
|
@lindzey -- I gave it another pass... |
Rewrote the SonarImage.msg comment for clarity.
I believe it's much clearer, though it's also much more verbose.