Fix for: Add an OdometryWithAcceleration message

[m-naumann]

Fix acceleration message

Keep the name twist for velocities

Add documentation for treatment of incomplete information

[tfoote]

Thanks for looking at these messages. Overall changes here cannot be taken lightly. There are a lot of lines of code using these messages. The cost of changing these messages is very high and as such we strongly refrain from making stylistic changes without strong justification.

Also I'd like to see more references for the documented standards for the covariance matrices. Also since these messages do not have covariance matrices in them this is not the right place for the documentation of the covariance matrices.

[tfoote]

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.

[m-naumann]

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

[tfoote]

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

[m-naumann]

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

[tfoote]

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.

[m-naumann]

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.

[tfoote]

Sorry I edited my comments as I didn't realize that this was modifying the proposal not the core messages. But the semantic meaning and clarity comments still hold as well as the location of the covariance documentation.

[m-naumann]

No worries. I tried to make my point in the code comments, but it's probably a matter of taste whether to start improving the naming but therefore mixing old and new, or whether to only improve naming for entirely new messages.

About the covariance: If there is another convention/documentation anywhere, let me know. That was the only one I found, and I changed it to separate treatment of single entries as that's at least in the automated driving context very useful.

EDIT:

as well as the location of the covariance documentation.

I agree, that's probably worth a PR to update the documentation of the three messages itself. Maybe you can shortly tell me whether I should prepare a PR for those 3.