Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for: Add an OdometryWithAcceleration message #146

Open
wants to merge 3 commits into
base: rep_147
Choose a base branch
from
Open

Fix for: Add an OdometryWithAcceleration message #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
558cb17
Fix acceleration message
m-naumann May 22, 2019
7016864
Keep the name twist for velocities
m-naumann May 22, 2019
9c7cec6
Add documentation for treatment of incomplete information
m-naumann May 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions nav_msgs/msg/Odometry.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
# This represents an estimate of a position and velocity in free space.
# The pose in this message should be specified in the coordinate frame given by header.frame_id.
# The velocity in this message should be specified in the coordinate frame given by the child_frame_id
# The velocity in this message should be specified in the coordinate frame given by the child_frame_id.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that there is already several standards for this. Please point to existing documentation or implementations to justify these statements. I'm not entirely familiar with the current state but I think that there might be a difference between this documentation and current practices relating to the use of zeros vs negative ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this from here: http://docs.ros.org/api/sensor_msgs/html/msg/Imu.html

except that I changed

If you have no estimate for one of the data elements (e.g. your IMU doesn't produce an orientation
estimate), please set element 0 of the associated covariance matrix to -1

into

If you have no estimate for one of the data elements, please set _the respective diagonal_ element
of the associated covariance matrix to -1.

because you might know x and y of a vehicle on a road for example, but not z.

#
# If the covariance of the measurement is known, it should be filled in (if all you know is the
# variance of each measurement, e.g. from a datasheet, just put those along the diagonal).
# A covariance matrix of all zeros will be interpreted as "covariance unknown", and to use the
# data a covariance will have to be assumed or gotten from some other source.
#
# If you have no estimate for one of the data elements, please set _the respective diagonal_ element
# of the associated covariance matrix to -1.
# If you are interpreting this message, please check for a value of -1 in the _diagonal_ elements
# of each covariance matrix, and disregard the associated estimates.

Header header
string child_frame_id
geometry_msgs/PoseWithCovariance pose
geometry_msgs/TwistWithCovariance twist # velocity but keeping name for backwards compatibility
geometry_msgs/TwistWithCovariance twist
15 changes: 13 additions & 2 deletions nav_msgs/msg/OdometryWithAcceleration.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,19 @@
# The pose in this message should be specified in the coordinate frame given by header.frame_id.
# The velocity in this message should be specified in the coordinate frame given by the child_frame_id.
# The acceleration in this message should be specified in the coordinate frame given by the child_frame_id.
#
# If the covariance of the measurement is known, it should be filled in (if all you know is the
# variance of each measurement, e.g. from a datasheet, just put those along the diagonal).
# A covariance matrix of all zeros will be interpreted as "covariance unknown", and to use the
# data a covariance will have to be assumed or gotten from some other source.
#
# If you have no estimate for one of the data elements, please set _the respective diagonal_ element
# of the associated covariance matrix to -1.
# If you are interpreting this message, please check for a value of -1 in the _diagonal_ elements
# of each covariance matrix, and disregard the associated estimates.

Header header
string child_frame_id
geometry_msgs/PoseWithCovariance pose
geometry_msgs/TwistWithCovariance velocity
geometry_msgs/TwistWithCovariance acceleration
geometry_msgs/TwistWithCovariance twist
Copy link
Member

@tfoote tfoote May 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression backwards. Twist is the datatype not the meaning. This came up in the review of the Odometry message but was not changed there to velocity to keep backwards compatibility even though velocity was more semantically accurate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that velocity would be better, however if you actually want to access the velocity, you'd have to access msg.velocity.twist and there is the covariance msg.velocity.covariance

As I totally agree with your upper statement that

cost of changing these messages is very high

but I'd also prefer to keep this msg.twist.twist and msg.twist.covariance to avoid twist and velocity alongside each other, because we cannot have msg.velocity.velocity anyway

geometry_msgs/AccelWithCovariance accel
Copy link
Member

@tfoote tfoote May 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truncating it a step backwards from our goals of having clear and obviously named variables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, here it's also to have msg.accel.accel instead of msg.acceleration.accel, because we cannot change to msg.acceleration.acceleration