Proposer: Eric Perko
Present at review:
- Jack O'Quin
- Chad Rockey
Question / concerns / comments
(Eric) Right now, I'm using the useRMC parameter to indicate the sentences the user wants us to read. Most NMEA GPS devices output all of their supported sentences, some of which contain redundant information (e.g. RMC and GGA both contain position fixes). Is there a good way to allow the user to pick which sentences we should use to generate which information, or should we just always prefer certain ones for certain info (e.g. if both RMC and GGA are available, always use GGA for position)?
(Jack) I don't really have any useful opinion about this: useRMC seems reasonable.
(Jack) What is the meaning of the frame_id parameter? It should not be used in sensor_msgs/TimeReference, it is unclear what header.frame_id means in sensor_msgs/NavSatFix. For geometry_msgs/TwistStamped it presumably indicates the robot frame to which those velocities apply. For that, /gps seems an odd default, perhaps base_link would be better. If there is no leading /, the tf_prefix should be applied.
(Eric) The frame_id is not used for the TimeReference, except as the default if you don't set a time_ref_source string. The frame_id in both the TwistStamped and NavSatFix should correspond to the physical location of your GPS unit on the robot. For example, if you are on a car, it's unlikely that your GPS antenna is exactly at base_link, hence the need for a frame_id for both the TwistStamped and NavSatFix output to describe the offset between base_link and the location that the GPS is calculating a fix for.
(Jack) I see what you mean about TwistStamped. I still don't understand what the NavSatFix frame ID should be. For that message, WGS-84 is implied.
(Eric) Perhaps I've just worded the documentation poorly. The frame_id isn't the reference frame of the absolute coordinate system (that would be WGS-84), the frame_id is only needed so that we know the transformation between the GPS antenna location and some other part of the TF tree. Otherwise, how would we know how to reference say, 1 meter in front of base_link in absolute GPS coordinates? Would it make more sense if the default frame_id were gps_link to emphasize that this is for describing how the GPS is attached to the robot?
(Jack) Now I get it. My questions were naive, but I suspect others will share them. I recommend keeping the parameter name frame_id, while describing its meaning in more detail. In fact, the NavSatFix message should spell it out explicitly in a comment.
(Chad) Looks like we put a misleading comment in the NavSatFix message. We should modify that message to make it more clear.
(Jack) +1; I plan to go ahead and change it.
(Jack) I posted a diff for this change in the Conclusion section for your comments before I commit it.
(Eric) Isn't the "location reported by the satellite receiver" technically the GPS coordinates. I think something like "location of the satellite receiver's antenna on the robot" would be slightly clearer. We should remove the "frame of reference" verbiage on the existing head description line - technically "frame of reference" for the GPS device is WGS84.
(Jack) It may not always be the location of the antenna. Our Applanix POS-LV reports its position already transformed into the vehicle frame of reference. It has two GPS antennae. (That is probably why I was confused by this issue in the first place.)
(Jack) How about this rewording of the entire Header comment?
- header.stamp specifies ROS time, which may require a phase-locked loop translating GPS time into system time.
- header.frame_id is the frame of reference reported by the satellite receiver (usually the location of the antenna on the vehicle).
(Eric) I suggest we just say something like the following.
header.stamp specifies the ROS time when this measurement was taken. NavSat timestamps may be reported using the sensor_msgs/TimeReference message.
- header.frame_id is the frame of reference that the satellite receiver reports position fixes relative to. This is usually the location of the antenna on the robot or the location of the receiver if your satellite receiver accounts for antenna offset. This is a metric coordinate frame id, not a GPS reference ellipsoid.
(Jack) frame_id is really a frame of reference denoting the actual fix location, right? Not relative, always [0, 0, 0]. Our receiver reports fixes in the vehicle frame, not its own location. That is why I phrased it the way I did. Mentioning that this frame is Euclidean, not relative to an ellipsoid is a good idea.
(Eric) What I meant by relative to is that it is reporting fixes relative to it's own location or relative to the vehicle frame or some other reference frame. Maybe it we just explicitly mention that its a Euclidean frame and give the example of the location of the antenna on the robot that's clear enough.
(Eric) How about something like the following:
- header.frame_id is the frame of reference reported by the satellite receiver (usually the location of the antenna on the vehicle). This is a Euclidean frame of reference relative to the vehicle, not a reference ellipsoid.
(Jack) +1; added as a conclusion below.
(Chad) Yeah, don't forget tf_prefix.
(Eric) I had forgotten tf_prefix support. I've added support for it and will update the documentation at the end of the review period.
(Jack) While working on these NavSatFix message comment changes, I remembered another we had discussed: publishing altitude as a quiet NaN when none is available. Does the NMEA driver do it that way?
(Eric) Yes, I output a NaN for altitude when using the NMEA sentence that doesn't include altitude information.
Meeting held via email with reviewers.
The review conclusions were approved by the reviewers. This review is now closed.
Update sensor_msgs/NavSatFix comments as follows:
header.stamp specifies the ROS time for this measurement. (The corresponding satellite time may be reported using the sensor_msgs/TimeReference message.)
header.frame_id is the frame of reference reported by the satellite receiver, usually the location of the antenna on the vehicle. This is a Euclidean frame relative to the vehicle, not a reference ellipsoid.
- Altitude [m]. Positive is above the WGS 84 ellipsoid (quiet NaN if no altitude is available).
Update documentation to reflect support for tf_prefix