-
Notifications
You must be signed in to change notification settings - Fork 142
Add Altitude.msg #304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rolling
Are you sure you want to change the base?
Add Altitude.msg #304
Conversation
tfoote
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few inline comments.
And from my previous issue comments
If you can prototype of the converter which can convert all the different datatypes into the common one. And provide a demonstration of one of the state estimators or other potential downstream user that can demonstrate that the proposed message covers the needs on both sides.
Before we land something in common_interfaces we want to see implementations and real use cases that are working to know that we have coverage of the needs. When a proposal hits realitiy there's often an iteration on the design required. And once it's here changes are very expensive to make due to our stability goals.
| float64 variance # 0 is interpreted as variance unknown | ||
|
|
||
| # Optional name of the datum/surface | ||
| string datum # Name of vertical datum or surface if applicable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each datum supported should be in the enum above, not a string here.
| # (e.g., base_link, imu_link, map, earth). | ||
| # | ||
| # Reference: | ||
| # The 'reference' field indicates what this measurement is relative to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field doesn't appear to exist.
| # Semantics (REP-105): | ||
| # - 'altitude' is the vertical position along the +Z axis of header.frame_id. | ||
| # - header.frame_id is the frame in which the measurement is expressed | ||
| # (e.g., base_link, imu_link, map, earth). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful on this. base_link and imu_link are rarely actually vertical in the world. Isn't this about being vertical in the datum?
Description
Add a new scalar message to sensor_msgs named Altitude.msg, containing a timestamped vertical position z in meters (positive up), with an optional uncertainty. This fills the gap between existing messages that either (a) embed altitude together with latitude/longitude (NavSatFix) or (b) represent barometric pressure or generic ranges rather than altitude itself. The proposed definition mirrors existing scalar messages (e.g., Temperature, Illuminance) that carry a variance field.
Fixes #294
Is this user-facing behavior change?
No
Did you use Generative AI?
No
Additional Information
Intended publishers (examples)
reference=REFERENCE_DATUM,datum="MSL"or a specified geoid/ellipsoid (e.g.,"EGM96","WGS84").reference=REFERENCE_SURFACE,datum="SEA_SURFACE"; report depth as negative altitude.Before approving or merging, @tfoote requested additional files. I’ll address this as soon as possible, but I won’t have time over the next few weeks. You can see the request here