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 : DCO fix #1427

Merged

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Apr 5, 2022

Signed-off-by: Aditya [email protected]

🎉 New feature

Ported from : #1320 to make the DCO bot happy

Summary

This PR, along with gazebosim/gz-sensors#186, enables the user to specify named orientation frames like ENU and NED as 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.

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Apr 5, 2022

The changes here were originally approved here : #1320

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.

LGTM pending green CI. Thanks @adityapande-1995!

@scpeters any other comments?

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Apr 5, 2022

Homebrew failures seem unrelated, and INTEGRATION_imu_system is passing. Windows has segafaults in other tests. The scene broadcaster and network tests are flaky and unrelated.

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #1427 (60ced56) into ign-gazebo6 (fa2d7cc) will increase coverage by 0.10%.
The diff coverage is 98.27%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1427      +/-   ##
===============================================
+ Coverage        63.36%   63.46%   +0.10%     
===============================================
  Files              306      307       +1     
  Lines            24715    24783      +68     
===============================================
+ Hits             15660    15729      +69     
+ Misses            9055     9054       -1     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/detail/BaseView.hh 100.00% <ø> (ø)
src/BaseView.cc 100.00% <ø> (ø)
...rc/systems/odometry_publisher/OdometryPublisher.hh 100.00% <ø> (ø)
...rc/systems/odometry_publisher/OdometryPublisher.cc 88.37% <95.16%> (+2.31%) ⬆️
...e/ignition/gazebo/detail/EntityComponentManager.hh 93.86% <100.00%> (+0.44%) ⬆️
include/ignition/gazebo/detail/View.hh 100.00% <100.00%> (ø)
src/View.cc 100.00% <100.00%> (ø)
src/systems/imu/Imu.cc 74.25% <100.00%> (+2.51%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 031e334...60ced56. Read the comment docs.

@adityapande-1995 adityapande-1995 merged commit 2e8892a into ign-gazebo6 Apr 6, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/named_frames_imu_system_dco_fix branch April 6, 2022 17:32
@hidmic
Copy link
Contributor

hidmic commented Apr 7, 2022

@adityapande-1995 @scpeters when will this be forwarded to main ?

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Apr 7, 2022

I'm not sure about the forward porting strategy, maybe @chapulina or @scpeters can comment about that ? We would also need to port gazebosim/gz-sensors#186 and gazebosim/gz-sensors#212 to main in ign-sensors

@scpeters
Copy link
Member

scpeters commented Apr 7, 2022

@adityapande-1995 @scpeters when will this be forwarded to main ?

it can be forward-ported at any time. I'll make an attempt

@scpeters
Copy link
Member

scpeters commented Apr 7, 2022

@adityapande-1995 @scpeters when will this be forwarded to main ?

it can be forward-ported at any time. I'll make an attempt

gazebosim/gz-sensors#214

@hidmic
Copy link
Contributor

hidmic commented Apr 11, 2022

Thanks @scpeters! Now we just have to do the same here, right?

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1

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.

4 participants