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

Supply world frame orientation and heading to IMU sensor #1320

Closed
wants to merge 40 commits into from

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Feb 3, 2022

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Summary

This PR, along with gazebosim/gz-sensors#186, enables the user to specify named orientation frames like ENU and NED as <localization> reference tags for the IMU sensor. The sensor should output the orientation based on the frame specified in the localization tag.

This should also take into account the orientation of the world and the heading_dec offset in the <spherical_coordiantes> tag. Currently, spherical coordinates tag nly supports the ENU world fame.

Test it

2 integration tests have been added that use a combination of heading_deg and localization tags to get different orientations for IMUs.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Feb 3, 2022
@adityapande-1995 adityapande-1995 self-assigned this Feb 3, 2022
@adityapande-1995 adityapande-1995 marked this pull request as draft February 3, 2022 05:13
@adityapande-1995 adityapande-1995 changed the title Aditya/named frames imu system Supply world frame orientation and heading to IMU sensor Feb 3, 2022
@adityapande-1995 adityapande-1995 marked this pull request as ready for review February 4, 2022 22:35
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Feb 4, 2022

Adding expected behavior of the "CUSTOM" tag.

@adityapande-1995 adityapande-1995 marked this pull request as ready for review February 4, 2022 23:42
src/systems/imu/Imu.cc Outdated Show resolved Hide resolved
heading = sphericalCoordinates.HeadingOffset().Radian();
}

sensor->SetWorldFrameOrientation(math::Quaterniond(0, 0, heading),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this function only be executed if the World has SphericalCoordinates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sensor->SetWorldFrameOrientation should be run irrespective of whether the World has SphericalCoordinates because that is where the <localization> tag in IMU is parsed. Added here : https://github.com/ignitionrobotics/ign-sensors/blob/33a79c6524e69dbf4006815458cddef1fec3f785/src/ImuSensor.cc#L315

For the heading = .... line, that was just to make sure it defaults to 0, and is then overridden if the world has spherical coordinates. Maybe I can remove that if sphericalCoordinates has a default with 0 heading.

Co-authored-by: Nate Koenig <[email protected]>
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 12, 2022
src/systems/imu/Imu.cc Outdated Show resolved Hide resolved
test/integration/imu_system.cc Outdated Show resolved Hide resolved
test/integration/imu_system.cc Show resolved Hide resolved
test/integration/imu_system.cc Outdated Show resolved Hide resolved
test/worlds/imu_named_frame.sdf Outdated Show resolved Hide resolved
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Tests are great @adityapande-1995 !

test/integration/imu_system.cc Show resolved Hide resolved
test/worlds/imu_rotating_demo.sdf Show resolved Hide resolved
test/integration/imu_system.cc Outdated Show resolved Hide resolved
chapulina and others added 5 commits April 1, 2022 13:05
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@adityapande-1995 adityapande-1995 added the needs upstream release Blocked by a release of an upstream library label Apr 4, 2022
@adityapande-1995 adityapande-1995 removed the needs upstream release Blocked by a release of an upstream library label Apr 5, 2022
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Patch looks good, but some of the tests are failing CI

@adityapande-1995
Copy link
Contributor Author

Yeah, I don't think the new release has been applied to homebrew yet : osrf/homebrew-simulation#1863

@adityapande-1995
Copy link
Contributor Author

@hidmic, ported all changes here to make the DCO bot happy. Signing that one commit was turning out to be a pain. : #1427

@adityapande-1995 adityapande-1995 deleted the aditya/named_frames_imu_system branch April 6, 2022 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants