Update Rep 145 (P II)

[SteveMacenski]

Continuation of #186 reflecting comments in the discussion:

• Addition of consolidated message type for IMU data alongside the raw sources
• Policies established for publication rate of IMU message
• Policies established for namespaced data source types for gravity compensation, raw, and bias status
• Adding in mag message again
• Added vendor SDK callout
• No need for imu/data vs imu/data_raw since the raw data will be namespaced and/or represented in the raw channels

Should I add myself and Tully as coauthors?

[peci1]

I understand that unbiasing angular velocity measurements is much more common than unbiasing acceleration, but the sole names of the topics do not actually indicate which one of these was unbiased... One possible interpretation is that e.g. if you only compensate gyros, then you would have topics imu_unbiased/angular_velocity and imu_biased/linear_acceleration. That would make sense, but it would make it harder to use the topics further. Also, it's not obvious what parts of the aggregate message are unbiased. Apart from this confusion, I like the approach this PR settled on.

I would also add a sketch of the relationship between the 4 defined namespaces. As I understand it, each IMU driver should publish imu_raw, but the subscribers cannot do any kind of assumptions about the data (unbiasing, attitude, grav comp). Then the role of the driver is to republish each of these data streams to the correct namespaces. Do I get it right? It might also help to add a few notes about how to add the missing links - i.e. using a bias removal node, using IMU filter etc.

I think the unbiasing tools could be a part of standard ROS packages (possibly in imu_tools). If you'd like to get a reference implementation, just ping me, we have a nice node for that. It is not yet public, but I aim at publishing it to Noetic.

[SteveMacenski]

I understand that unbiasing angular velocity measurements is much more common than unbiasing acceleration, but the sole names of the topics do not actually indicate which one of these was unbiased...

Maybe what isn't clear is that not every topic needs to exist under every namespace if it isn't possible or completed. Just as an IMU need not publish all of the messages in the section above, depending on what it has and what is relevant. Since we've established already the concept that not all those topics may exist to begin with, the indirection of namespaces is just applying that same rule but to sorted data.

If you buy into that view of it, I can add a clarifying note about how not every topic needs to exist.

Then the role of the driver is to republish each of these data streams to the correct namespaces. Do I get it right? i.e. using a bias removal node, using IMU filter etc.

Maybe, maybe not. There could be external tools (like the filters) that are unbiasing the data and they publish the imu_unbiased or imu_compensated topics, not the drivers. I don't think specifying who in particular must publish this data is a good restriction, but I agree that having some examples or illustrations of what we mean could help.

I have only a cursory understanding of IMU calibration (e.g. working next to people in grad school that focused on it, but not myself 😆 ) but this is a topic that you appear to know quite alot about. Would you be open to providing some language for added descriptions to add to the biased/unbiased namespaces? I can fill in more detail on grav comp and raw.

[peci1]

I have only a cursory understanding of IMU calibration but this is a topic that you appear to know quite alot about. Would you be open to providing some language for added descriptions to add to the biased/unbiased namespaces?

Well, I wouldn't call myself an IMU calibration expert, but I read something about it when implementing state estimation for our robots. I'll have a look at it in a week or two.

[SteveMacenski]

@peci1 hi, just pinging you back here 😄

[peci1]

Thanks, I must admit I forgot about this. I won't have time for it today, but tomorrow I'll have a look at it ;)

[peci1]

I don't quite get this. Should I imagine for example a sensor that can measure only in x-y, but not in z? The last sentence says I should set _covariance[0] to NaN, but the sensor could actually have a covariance for the x axis, which corresponds to _covariance[0].

[SteveMacenski]

This was language from the original one - I completely agree on closer examination I don't think this makes sense either.

I think we should change this to be:

• Numbers reported if they got'm (current behavior)
• If no information is known about anything covariance related, the first element (or all elements) set to NaN (new behavior - in case we have nothing, so we don't even try)
• If a particular piece of information is not known, but others are known, set to 0 (current behavior)

Thoughts?

[peci1]

Suggested change

All message types provide a covariance matrix (see REP 103 [1]_) alongside the data field (`*_covariance`). If the data's covariance is unknown, all elements of the covariance matrix should be set to 0, unless overridden by a parameter. If a data field is unreported, the first element (`0`) of the covariance matrix should be set to `NaN`.

All message types provide a 9-element (3x3 row-major) covariance matrix (see REP 103 [1]_) alongside the data field (`*_covariance`). If the covariance of some dimension is unknown, the corresponding diagonal element of the covariance matrix should be set to 0 (unless overridden by a parameter). If a dimension is unreported (i.e. the sensor only measures in one axis), the corresponding diagonal element of the covariance matrix should be set to `NaN`. Thus any algorithm processing the covariance should first check if the 3 diagonal terms are different from 0 and are not `NaN`.

What about this wording? Does it sound clearer?

[peci1]

#186 (comment) still applies here and hasn't been resolved:

Applying a transformation to IMU data requires transforming both the body and the world frames.

I still don't think transforming the world frame is correct here.

[SteveMacenski]

I don't see that sentence in this document or in the previous discussion. I'm not sure what change or issue you have is 😦

[peci1]

It is a few lines above, chapter Transformation: https://github.com/ros-infrastructure/rep/blob/a7e5e6a337a859f8ca789e5aa65067b508ef466d/rep-0145.rst#transformation

[peci1]

The previous discussion:

@peci1

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]

My attempt at improving the bias and gravity compensation sections is in SteveMacenski#1 . The text seems a bit heavy to me, but I was not able to make it more concise.

I have also ditched the imu_biased namespace in favor of just imu. As it is not a problem to try to remove bias from an already unbiased topic, it does not matter whether data on the imu topic are unbiased or not. It seems to me that imu and imu_biased were very similar in many regards, in getting rid of one of them helps to simplify the REP.

[SteveMacenski]

Huh, somehow I wasn't notified about that. I wonder what else I haven't seen.... scary thoughts

I have also ditched the imu_biased namespace in favor of just imu.

I like that. I think what you did there makes sense - IMU is the base, everything else should be specified relative. It does make me wonder if we need both imu and imu_raw, they appear to be the same thing functionally. Do we really need both?

Note a comment I made in your PR: SteveMacenski#1 (comment)

[ros-discourse]

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-03-16/30432/1

[peci1]

It does make me wonder if we need both imu and imu_raw, they appear to be the same thing functionally.

The difference is that imu_raw might have gravity compensated or not. imu should always include gravity. Maybe it needs better wording? The idea is that the driver can just relay data from a serial link/Ethernet to imu_raw, and then a postprocessing step turns it into imu.

Let's have a look at the two cases:

1. The IMU publishes gravity-compensated data. In this case, imu_raw has gravity compensated data and you need a node that adds back gravity and creates topic imu (angular velocities and magnetometer can be copied directly to imu). You can directly republish the accelerations to imu_compensated.

2. If the IMU publishes data with gravity, imu and imu_raw can be the same, so you just create a republisher. Or you don't even need to expose the imu_raw topic and can directly publish to imu. Creating imu_compensated would require estimating the gravity vector and subtracting it from imu.

No generic processing node should access imu_raw. It doesn't know what it finds there. imu_raw should only be used by the low-level processing node in a particular IMU's driver.

[SteveMacenski]

Apologies, this is on a hiatus for me given the recent changes in employment. Need to baton down the hatches a bit and revisit things like this once I know what's going on

[ros-discourse]

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

https://discourse.ros.org/t/driver-for-witmotion-imu-sensors-released/30602/2

[ros-discourse]

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-4-20-2023/31087/1

[ros-discourse]

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

https://discourse.ros.org/t/future-of-ros-2-gps-support/33297/53

[clalancette]

We did a review of this in the ROS 2 weekly meeting, and here are some of the notes from that:

• In the Data Reporting, we are not sure what “vendor-specific SDK functionality” is; that probably needs to be defined or further clarified
• In the Topics section, the Attitude isn’t discussed prior to this section. It probably should be defined prior to this.
• In the Topics, there is mention of some new messages like AngularVelocity, LinearAcceleration, etc. It would help a lot to know what these messages are going to look like.
• In general, these changes are not backwards compatible at all. That means that if we were to apply this, it would be a huge effort to convert both the producers (the IMU drivers) and the downstream consumers of that data to conform to this. Given that this is a long-standing set of conventions, we think that this change really needs to think about backwards compatibility a lot more. This applies to the messages themselves, the topic names, the parameters, and the topic namespaces.

[ros-discourse]

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

https://discourse.ros.org/t/ros-2-meeting-minutes-2024-02-15/36221/1