Update the REP 145 with respect to discussed new messages

[tfoote]

This is a followon to #95

homogenizing with: ros/common_msgs#101

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/request-for-feedback-pr-186-on-rep-145-conventions-for-imu-sensor-drivers/27272/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/request-for-feedback-pr-186-on-rep-145-conventions-for-imu-sensor-drivers/27272/2

[SteveMacenski]

• Otherwise, all data should be published by the driver as it is reported by the device. Any subsequent modifications to the data (e.g. filtering, transformations) should be delegated to a downstream consumer of the data [3].

I think that needs some clarification. Many devices will either on-board or through a provided SDK give some optional filtering tools to help improve estimates or provide some results from raw data. For the case of on-board, that cannot be delegated later in the pipeline. For provided SDKs, its not entirely sensible to have a separate node handle that when a driver could expose functions from the SDK as optional functions / parameterizations for pre-processing results. For any other type of sensor driver, I would expect if the sensor's SDK provides some function that I would be able to enact those behaviors on the data in the driver itself.

I think this is really meaning non-vendor provided utilities / tools / filters. I think that's a simple clarification to make.

When the device is at rest, the vector will represent the specific force solely due to gravity. I.e. if the body z axis points upwards, its z axis should indicate +g. This data must be in m/s^2.

Some IMUs give acceleration information with gravity compensated. Is the intent of this statement to explicitly say that this is undesired? There are several applications where this is actually a huge plus of a particular IMU vendor. Maybe instead we add a boolean field in the IMU message for whether or not the gravity vector is included or not?

Edit: its a actually a good point that we in the ROS community don't have any explicit conventions around this, we probably should. Saying that IMUs shouldn't gravity compensate isn't a great solution since that's a really nice thing for many systems.

imu/data_raw ... imu/data

The only difference here is adding in the orientation vector, if applicable. It seems odd to me to have the 2 separate topics for that separation given the rest of the data is the same and its known that an orientation vector is a derived result. There's no mistaking that for a "raw" result. I think data_raw should be removed, unless there's some rationale for this that I'm not thinking of.

---

For this PR: I would highly not recommend that action. It seems awfully inconvenient to have to synchronize 3 topics that are usually (and historically) associated with IMUs to get a full result from an IMU. I would, however, support breaking apart those as different messages so they can be individually used by other systems, but retaining an IMU message that is the aggregation of those 3 message types:

IMU.msg

sensor_msgs/Attitude orientation
sensor_msgs/AngularVelocity angular_velocities
sensor_msgs/LinearAccleration angular_accelerations
... covariances as needed

[SteveMacenski]

Suggested change

	* `imu/linear_acceleration` (sensor_msgs/LinearAccleration)
	* `imu/linear_acceleration` (sensor_msgs/LinearAcceleration)

[mintar]

Maybe move linear_acceleration up one bullet point, so it's right behind angular_velocity? Makes more sense to group these two together, because those are usually the raw sensor data, and attitude is computed from those, so it shouldn't be between them. We could also move attitude right to the end (after angular_velocity, linear_acceleration and mag).

[SteveMacenski]

Suggested change

	- The sensor output of the measured linear accleration.
	- The sensor output of the measured linear acceleration.

[omichel]

Suggested change

	- Sensor output containing information about the attitude
	- Sensor output containing information about the attitude estimation (roll, pitch and yaw).

[peci1]

I'm partly seconding Steve's wish to be able to subscribe to a single message for all 3/4 IMU output data types. However, I don't see a way how to properly implement it, so in the end I agree with splitting all the topics. Please note I'm looking at it from ROS 1 perspective as I haven't worked with 2 yet.

I definitely support creating specific data types for angular velocities and linear accelerations instead of plain Vector3s. Similarly for globally referenced attitude.

One argument against aggregate message is I don't know about a way to properly specify optional fields in ROS messages. Therefore, there is no nice way to tell the driver does not output e.g. the orientation estimate. For orientation specifically, you could set an all-zero quaternion to signify it. For the other fields, would nans serve in the same fashion? I guess this could be a solution, although it would be harder for newcomers to figure out why e.g. their odometry estimate is full of nans.

What about defining both the aggregate type and the individual types, letting drivers and downstream code choose which one they want, and provide a nodelet that does the (de)aggregation? This could also nicely handle the backwards compatibility problem (I'd say better than the proposed transport library, as that would require changes in old code).

A strong argument (IMO) against the aggregate type are publishing rates for the various measurements. Desired behavior of the above-suggested aggregation node is hard to define. E.g. Xsens can output gyro/accels at 2 kHz, while orientation tops at 400 Hz, magnetometer at 100 Hz and barometer at 50 Hz. How should all this data be aggregated? Obviously, choosing the slowest frequency is not the best way. I think tooling is the right way to solve this complexity.

Is it actually often the case that you subscribe all of the IMU outputs? I understand that IMU filters and localization nodes need to do that, but other code? I remember subscribing acceleration to determine hard hits (possibly with attitude). Or magnetometer+attitude for compass. Or angular rate alone for enhancing kinematic odometry. But never all 3/4 together.

A "nice" example of the weirdness of the current aggregate message is navsat_transform_node from robot_localization. It subscribes Imu messages just to parse out the global attitude. When first reading the docs, I was really surprised why does it need Imu when it already receives odometry. Subscribing attitude would seem much clearer. I think this can be one of the reasons why there isn't any node working as compass - because there's no suitable data type for its output.

One thing not mentioned here and only slightly mentioned on the 8y old discussions - should this REP also handle dynamic calibration/bias estimation? Gyros and magnetometers are usually useless without startup calibration, accels can also make use of it. And there's no way to tell just from data whether they are calibrated or not. In our code, I solved it in the following way: I publish topics imu/gyro_bias etc. and have a nodelet that takes imu/data and imu/gyro_bias and outputs imu_unbiased/data. Could a similar naming convention work? This way, the IMU filters could become a bit more complicated, but it would be obvious what do the data mean. With the separate topics per each modality, it would be even better. Each of them could be calibrated/uncalibrated separately. Good thing is that the bias topics can use the same data type as the raw data (and it makes "physics-based" sense). Regarding calibration service/action, I'd leave it unspecified as these procedures might require physical motion of the robot. The only problem I haven't solved yet is how to represent that magnetometer has only been calibrated in x-y (by doing a 360 rotation), but not in z. So far, I specify 0 bias with nan or infinite variance.

An idea regarding gravity-compensating IMUs - what about publishing the standardized linear acceleration message with gravity included, and publishing a separate synchronized message with the estimated gravity vector? This way, nodes could keep their assumptions about g being present, while other nodes being able to make use of the gravity-compensated accelerations without losing precision. But I haven't worked with this kind of IMUs, so I don't know whether this would be possible. The compensated/uncompensated differentiation could also be made by a naming convention. I would even second adding a bool field to the LinearAcceleration message that would tell whether gravity is there or not - then the data would be self-explaining. But it's not a hard requirement from me.

Regarding tooling, everything needs to be published as c++/python libraries or as nodelets. Nodes (not even speaking of python nodes) have too much overhead for kHz-fast topics.

One thing I haven't understood in the REP update is the sensor_frame field of Attitude. Wouldn't that be already written in the header?

So my ideal set of topics defined by this REP would be (up to the naming):

• imu/raw/angular_velocity
• imu/raw/linear_acceleration (including g)
• imu/raw/magnetic_field
• imu/raw/barometer
• imu/bias/angular_velocity (latched)
• imu/bias/linear_acceleration (latched)
• imu/bias/magnetic_field (latched)
• not sure if barometers also need online calibration
• imu/unbiased/angular_velocity
• imu/unbiased/linear_acceleration (including g)
• imu/unbiased/magnetic_field
• imu/gravity
• imu/attitude

I think this naming provides a clear idea of what kind of data each topic should contain. Gravity and attitude are not namespaced raw or unbiased to show they are derived values and not something measured, while still relating their origin to the IMU device.

[peci1]

This becomes obsolete and requires new wording.

[peci1]

This comment is to the above section Transformation, but github doesn't allow to place a comment there. I never understood the idea of this paragraph. If I transform both the sensor frame and the reference frame, isn't the resulting data transform always identity? I imagine applying a transform to IMU data is like taking the IMU and rotating its body. I.e. only changing the sensor frame transform regarding the same reference frame. How would I rotate ENU by 90 degrees yaw?

[mintar]

I agree this could be phrased better.

Here's a relevant email by @paulbovbel (the original author of REP 145) that probably explains his thinking when he wrote that paragraph: https://groups.google.com/g/ros-sig-drivers/c/Fb4cxdRqjlU/m/MwS3uZDyfkoJ

I suspect this paragraph is mixing two things, but I'm not 100% certain:

• NED -> ENU conversion: You have to change body and world frames and the orientation.
• transformation into a different body frame (e.g., from imu_link to base_link): You only have to change body frame and orientation; the world frame stays the same.

[peci1]

This comment is to Reference implementation section, but github doesn't allow adding a comment there. The reference implementation has long had a serious bug in transformation of attitude (ros-perception/imu_pipeline#8). Wouldn't it be good to fix the reference before accepting this PR? A fix is provided, but not merged.

[peci1]

Fixed by ros-perception/imu_pipeline#15 .

[mintar]

Hey, where did imu/mag go? We still need that one!

[mintar]

A general question: This REP 145 has been the de facto standard for correctly using the current sensor_msgs/Imu message for over 7 years now, even though it has always been in "draft" status.

1. Should we really remove any mentions of the old messages from it and replace with the new messages which haven't even been finalized yet? (this is what the current PR proposes)
2. Should we keep the existing sections and add a new one on the new messages?
3. Should we leave REP-145 alone (maybe make it non-draft) and create a new REP that supersedes it?

These are open questions, I haven't formed a definite opinion yet. I tend towards option 2 though, as the changes here are quite small.

[peci1]

2. Should we keep the existing sections and add a new one on the new messages?

This solution sounds best to me. The old messages can be moved to some Deprecated/Historical section, but users will keep bouncing to these messages for long, so it'd be weird to remove any mention about them...

[SteveMacenski]

Good idea @mintar

I would just like to bring up again that it would be extremely disruptive to remove a single IMU message. For users that wish to have separate messages published at their native rates, I think this makes sense, but that should not preclude the general solution being a central IMU message comprising those messages put together.

There are a number of quality of life changes in ROS 2 I frequently hear new to ROS or ROS 1 users lambasting about. I can already hear the complaining from mobile robotics users if a centralized IMU message doesn't continue to exist / be supported. The idea of having a multi-message-sychronizer setup will really make people think twice about the maturity of ROS, I think, from conversations I've had internal to Samsung + external in Nav2-land. I totally understand the motivation for splitting them apart since often they operate at different frequencies (which can be important for more advanced fusion / drones -- though arguably most of those techniques require hardware sychronization so something like ROS messages aren't really appropriate) and its sensible to offer that option.

Could we do something like having the standard publish an IMU message, comprising the sub-fields, in addition to the individual streams at their natural frequencies? Its not like there aren't other sensor streams with similar etiquette. For example, RGB-D sensors have image_raw, image, image_rect_raw, and pointclouds for depth information. All slightly different variations of the same information for different types of consumers.

I think that's a good, practical, middle ground 😄

IMU.msg

sensor_msgs/Attitude orientation
sensor_msgs/AngularVelocity angular_velocities
sensor_msgs/LinearAccleration angular_accelerations
... covariances as needed

[peci1]

@SteveMacenski Specifying both the aggregate and the single messages in the REP might seem redundant, but it is probably the best way forward without making a lot of people angry (or the REP being ignored in practice). I agree with your proposal. The aggregate message could be used by nodes that do not require the best achievable precision, while the individual messages could be used either to increase precision (by being allowed to subscribe to higher rate data) or they could be used by nodes that require just the one modality.

It would be nice to specify in the REP what should the frequency of the aggregate message be regarding the other frequencies (or if it should be e.g. synchronized with either the gyros or accels).

The REP should also specify how to signify that attitude is not provided in the aggregate message. Invalid (all zeros) quaternion is one way, but maybe not the best one?

What do you (and others) think about adding a bool to LinearAcceleration saying whether gravity has been compensated for or not?

[SteveMacenski]

I agree on all points.

[SteveMacenski]

Suggested change

	The timestamp for these messages sould reflect the best estimate of when the observation was made.
	The timestamp for these messages should reflect the best estimate of when the observation was made.

[SteveMacenski]

Discussion:

Discussion
Steve: Agrees with comments he made in there
Should split the messages
Should have consolidated message that exists (Imu.msg) - maybe just keep it as the slowest rate of the device - approximately the same thing as the synchronizer - which clock of which sensor we are going to synchronize against?  Need a policy enforced.  Adds more complexity to implementation - can’t just use ExactTimeSynchronizer
Looking at precedent - RGB-D have multiple things like this, largely redundant information
Michael - high-rate, high precision IMU - don’t use Imu.msg
If you are doing your own thing
Not necessarily using it for long-term dead-reckoning
High-end gyros - just getting rates out, rather than filtered
Steve:
REP says that filtering should happen downstream
But this isn’t always true, particularly on high-end IMUs
REP should be updated to allow for this
Gravity Compensation
Not covered
Worth having conventions
Have to know where you are to do this
Standard output that many IMUs give you
Either have a flag gravcomp or not, or have convention
Could use separate topic for gravcomp - this is similar to how Android does it
William
Also there are GPS/IMUs that can do gravity compensation properly

I'll work on this next week and submit a followup PR

[SteveMacenski]

#372 to supersede

[clalancette]

Closing in favor of #372