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.

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

Measure IMU orientation with respect to world (ros2) #1064

Merged
merged 3 commits into from
Jun 2, 2020

Conversation

scpeters
Copy link
Member

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

This ports #1058 from eloquent forward to ros2 (foxy) and changes the default from retaining the legacy behavior (initial_orientation_as_reference == true) to complying with REP 145 by default (initial_orientation_as_reference == false). It also prints a deprecation warning if the user explicitly sets initial_orientation_as_reference to true. This PR is similar to #1057 which ported from melodic -> noetic.

Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM. I've triggered CI:

Build Status

@chapulina chapulina added the ros2 label Mar 25, 2020
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@jacobperron
Copy link
Collaborator

I guess we need to update the CI branch for Ubuntu Focal. Maybe we can add a CI branch gazebo_ros_pkgs_focal, @chapulina what do you think?

@scpeters
Copy link
Member Author

scpeters commented Apr 8, 2020

I guess we need to update the CI branch for Ubuntu Focal. Maybe we can add a CI branch gazebo_ros_pkgs_focal, @chapulina what do you think?

ping @chapulina

@chapulina
Copy link
Contributor

Sorry, this comment fell through the cracks. A new CI branch sounds good to me. It would be good to document it here

@jacobperron
Copy link
Collaborator

I branched from ros2/ci@gazebo_ros_pkgs and rebased on master. It looks like we don't need a separate branch for Focal since the master branch supports Bionic and Focal. @chapulina We could instead just rebase the gazebo_ros_pkgs branch on master (there's one small conflict). What do you think?

Besides that, we're blocked by cv_bridge compiling with OpenCV 4 on Focal. Here's a proposed fix we can wait on: ros-perception/vision_opencv#324

@chapulina
Copy link
Contributor

just rebase the gazebo_ros_pkgs branch on master

SGTM

@jacobperron
Copy link
Collaborator

I've updated the gazebo_ros_pkgs branch on ros2/ci.

@scpeters
Copy link
Member Author

scpeters commented May 7, 2020

does this just need to be rebased to get working CI?

@chapulina
Copy link
Contributor

does this just need to be rebased to get working CI?

No, you'd need to follow the Next-turtle procedure here: https://github.com/ros-simulation/gazebo_ros_pkgs/wiki/ROS-2-CI#next-turtle

scpeters and others added 2 commits June 1, 2020 10:58
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.

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Change default value of initial_orientation_as_reference to false
and print deprecation warning if user explicitly sets it to true.
@scpeters
Copy link
Member Author

scpeters commented Jun 1, 2020

I just rebased on ros2 to resolve conflicts

@jacobperron
Copy link
Collaborator

Linux: Build Status

Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

There's a couple linter errors that should be fixed. Otherwise, it looks like the remaining failures already exist on the ros2 branch:

Linux (ros2): Build Status

gazebo_plugins/src/gazebo_ros_imu_sensor.cpp Outdated Show resolved Hide resolved
gazebo_plugins/src/gazebo_ros_imu_sensor.cpp Outdated Show resolved Hide resolved
@scpeters
Copy link
Member Author

scpeters commented Jun 2, 2020

@jacobperron can you restart the builds if the changes in 02aa68b look right to you?

Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM Build Status

@scpeters
Copy link
Member Author

scpeters commented Jun 2, 2020

@jacobperron I'll let you interpret the results of that CI job

@jacobperron
Copy link
Collaborator

Looks like the result matches the ros2 branch.

@jacobperron jacobperron merged commit 57732fe into ros-simulation:ros2 Jun 2, 2020
@scpeters scpeters deleted the imu_orientation_ros2 branch June 2, 2020 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants