Measure IMU orientation with respect to world

[jacobperron]

Report the IMU orientation from the sensor plugin with respect to the world frame.
This complies with convention documented in REP 145: https://www.ros.org/reps/rep-0145.html

In order to not break existing behavior, users should opt-in by adding a new SDF tag.

---

I'm open to naming suggestions for the new SDF tag.

Pending approval of this change, I will follow up with ports to Noetic and ROS 2 branches. @scpeters and I discussed this offline, specifically we propose to change the behavior as follows:

• Noetic and Foxy: Change the default behavior so that orientation is wrt the world. Log a warning if the new SDF tag is not set. Log a deprecation warning if the new SDF tag has value false.
• Dashing and Eloquent: Identical to the change proposed in this PR.

[jacobperron]

cc/ @mogumbo @moriarty

[scpeters]

I made a comment about this on gazebo issue 1959:

• https://bitbucket.org/osrf/gazebo/issues/1959

People have been complaining about this for a while, so I would like to get this into current releases so we can add warnings in noetic / foxy

[scpeters]

I approve, but I want to run this past @chapulina

[chapulina]

I just have some questions. Can this be documented somewhere? Maybe here? It would also be nice to add some migration info to the changelog (i.e. commit message).

Dashing and Eloquent: Identical to the change proposed in this PR.

Just note that for ROS 2 we're using camel_case for the XML tags, as well as documenting the tags on the header file, and the migration from ROS 1 here.

About deprecation:

Log a warning if the new SDF tag is not set.

What warning would that be? Is it wrong to not set the tag? Should it just be an informational message? It would be interesting to see the Noetic PR before this one is merged.

Log a deprecation warning if the new SDF tag has value false.

I'm not sure I understand, false initialOrientationAsReference will be the new default, correct? Why would a warning be printed if the user reinforced the default behaviour?

---

Going forward, should we consider adding this option to SDF?

[jacobperron]

Can this be documented somewhere?

Sure. If you point me to the right place, I can open a PR.

for ROS 2 we're using camel_case for the XML tags, as well as documenting the tags on the header file

Should I make this change for Melodic / Noetic so that they're consistent with ROS 2, or change the case only in ROS 2?

Regarding warnings, we think that the new behavior is the correct behavior. But, we don't want to break existing user code if they are relying on the old behavior. Furthermore, we'd like to avoid breaking the behavior silently between releases. Therefore, we will log a warning if the tag isn't set so that the user is aware of the change (informational warning). Furthermore, since we want to encourage people to use the new behavior we log another warning if they are choosing the old (default) behavior (deprecation warning). The idea is that in a future release (G-turtle) we will remove the new tag completely and default to the new behavior.

[chapulina]

If you point me to the right place, I can open a PR.

The tutorial has an Edit button up top 😉

Should I make this change for Melodic / Noetic so that they're consistent with ROS 2, or change the case only in ROS 2?

I'd keep the camelCase convention for ROS 1.

we want to encourage people to use the new behavior we log another warning if they are choosing the old (default) behavior (deprecation warning)

I'm confused, the default behaviour on Melodic / Dashing / Eloquent is equivalent to initialOrientationAsReference == true, right?

On Noetic / Foxy, the default will be equivalent to initialOrientationAsReference == false, and that's what we want to encourage. So:

• If !sdf->HasElement("initialOrientationAsReference"), use false and print ROS_INFO that the world reference is being used.
• If sdf->Get<bool>("initialOrientationAsReference") == true, use old default behavior and print deprecation warning (or not, see below).
• If sdf->Get<bool>("initialOrientationAsReference") == false, use new behaviour and print no warning - right?

in a future release (G-turtle) we will remove the new tag completely

Is there a downside to keeping the tag forever? I can imagine users are already accounting for the current frame in their code, and it would be convenient if they could just keep their code just setting an extra flag. I'd vote for supporting the legacy behaviour without warnings if the user explicitly sets the tag to true.

[jacobperron]

I'm confused, the default behaviour on Melodic / Dashing / Eloquent is equivalent to initialOrientationAsReference == true, right?

Correct. Your code snippets look like what I described.

Is there a downside to keeping the tag forever?

The only downside I can think of is people choosing to set the new tag to true without understanding the consequences.

It just seems like an odd option to choose initialOrientationAsReference == true. If the robot starts on a slope then the IMU readings are dependent on that initial angle. Measurements become difficult to account for by consuming code if the angle of the slope changes between simulation scenarios. This can lead to bugs if the user is not aware of this behaviour, e.g. we did hit a bug when integrating the measurements into an EKF.

[chapulina]

It just seems like an odd option to choose

Yup, I don't think anyone would choose that because they desire that behaviour. The current behaviour has been around for several releases. Most likely, many downstream users hit that problem and are doing something on their end to compensate for that, like rotating the data. Once we change the default behaviour, their code will be doing the wrong thing. Rather than telling them to change their code, we can tell them to set initialOrientationAsReference == true to retain the legacy behaviour.

[scpeters]

Most likely, many downstream users hit that problem and are doing something on their end to compensate for that, like rotating the data. Once we change the default behaviour, their code will be doing the wrong thing. Rather than telling them to change their code, we can tell them to set initialOrientationAsReference == true to retain the legacy behaviour.

Once we change the default behavior, their code will be doing the wrong thing, but only if they skip from a very old version past the versions with deprecation warnings. I think we can change behavior in a new release if we document it after providing adequate warnings, no?

[jacobperron]

I'm okay not deprecating the old behavior if that is the preference, i.e. not warning users on Noetic and Foxy if they set initialOrientationAsReference == true. But, IMO, it would be nice to be able to completely remove this patch eventually (post Foxy) enforcing consistency with REP 145.

[chapulina]

Sure, we can remove the patch. I was just thinking that there was no downside to keeping it.

[scpeters]

I've updated the ros2 migration wiki:

• https://github.com/ros-simulation/gazebo_ros_pkgs/wiki/ROS-2-Migration:-IMU-Sensors/_compare/da0144f9f0efdae956861f71724adf05fef95388...2ae1cd09a76da057defa2dfb2aae7d8a1b8820c2

[scpeters]

I opened a pull request to document the new parameter in the gazebo tutorials:

• https://bitbucket.org/osrf/gazebo_tutorials/pull-requests/569/ros_gzplugins-add/diff

[scpeters]

@chapulina I've updated those tutorials that you mentioned. Do you have any other feedback?

[chapulina]

Oh sorry, I didn't mean to hold this. LGTM.