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

IMU sensor API to get world ref frame and heading offset #186

Merged
merged 24 commits into from
Mar 4, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Jan 11, 2022

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

🎉 New feature

Summary

As per the discussion here : #178 (comment), this PR aims to expose SetWorldFrameOrientation API which accepts heading offset and world frame orientation (ENU by default). If the <localization> tag for IMU sensor is set to any named frame (https://github.com/ignitionrobotics/sdformat/blob/6404afdf959982f759526dea8700037b50fa7758/sdf/1.9/imu.sdf#L7-L30), the sensor should output its orientation with respect to the frame specified in the <localization> tag.

There would be another follow up PR in IMU sensor system to use this API

Test it

Added some test cases to ImuSensor_TEST.cc

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 Jan 11, 2022
@adityapande-1995 adityapande-1995 marked this pull request as draft January 11, 2022 17:27
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
@adityapande-1995 adityapande-1995 self-assigned this Jan 14, 2022
@adityapande-1995 adityapande-1995 marked this pull request as ready for review January 14, 2022 15:26
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ign-sensors6@2614d1d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             ign-sensors6     #186   +/-   ##
===============================================
  Coverage                ?   76.44%           
===============================================
  Files                   ?       29           
  Lines                   ?     2908           
  Branches                ?        0           
===============================================
  Hits                    ?     2223           
  Misses                  ?      685           
  Partials                ?        0           

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 2614d1d...744d0d1. Read the comment docs.

@adityapande-1995 adityapande-1995 marked this pull request as draft January 14, 2022 17:56
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Jan 14, 2022

TODO

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Jan 19, 2022

Moved localization tag parsing logic to a separate method. This way the code in IMU system can be changed as follows :

void ImuPrivate::addIMU( ---- )
{
  // Get the world acceleration (defined in world frame)
  auto gravity = _ecm.Component<components::Gravity>(worldEntity);
  -------

  // create sensor
  std::string sensorScopedName =
      removeParentScope(scopedName(_entity, _ecm, "::", false), "::");
  sdf::Sensor data = _imu->Data();
  data.SetName(sensorScopedName);
  // check topic
  -----

  std::unique_ptr<sensors::ImuSensor> sensor =
      this->sensorFactory.CreateSensor<
      sensors::ImuSensor>(data);
  if (nullptr == sensor)
  { ---- }

  // set sensor parent
 -----

  // set gravity
  ------

  // Get initial pose of sensor and set the reference z pos
  // The WorldPose component was just created and so it's empty
  // We'll compute the world pose manually here
  math::Pose3d p = worldPose(_entity, _ecm);
  sensor->SetOrientationReference(p.Rot());

  // Set topic
 ----

  // Set whether orientation is enabled
  if (data.ImuSensor())
  {
    sensor->SetOrientationEnabled(
        data.ImuSensor()->OrientationEnabled());
  }

  this->entitySensorMap.insert(
      std::make_pair(_entity, std::move(sensor)));
 
 // ------- ADDITION --------
 sensor->ReadLocalizationTag(data);
 sensor->SetWorldFrameOrientation(Quaterniond(0,0,0), WorldFrameType::ENU);
}

@adityapande-1995 adityapande-1995 marked this pull request as ready for review January 19, 2022 17:00
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I have some quick comments about the variable names which I think will clarify what the variables mean

src/ImuSensor.cc Outdated Show resolved Hide resolved
src/ImuSensor.cc Outdated Show resolved Hide resolved
src/ImuSensor.cc Outdated Show resolved Hide resolved
src/ImuSensor.cc Outdated Show resolved Hide resolved
src/ImuSensor.cc Outdated Show resolved Hide resolved
@adityapande-1995 adityapande-1995 marked this pull request as ready for review February 3, 2022 04:35
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Copy link
Contributor Author

Relevant PR in IMU system : gazebosim/gz-sim#1320

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I'm still looking through the tests, but I have a few comments for now

include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Outdated Show resolved Hide resolved
@@ -436,6 +436,8 @@ TEST(ImuSensor_TEST, OrientationReference)
// Create an ImuSensor
auto sensor = mgr.CreateSensor<ignition::sensors::ImuSensor>(
imuSDF);
sensor->SetWorldFrameOrientation(math::Quaterniond(0, 0, 0),
sensors::WorldFrameEnumType::ENU);
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, SetWorldFrameOrientation is also responsible for applying the "CUSTOM" tag. I could separate this logic in a separate method though.

@@ -496,6 +498,9 @@ TEST(ImuSensor_TEST, CustomRpyParentFrame)
auto sensor = mgr.CreateSensor<ignition::sensors::ImuSensor>(
imuSDF);

sensor->SetWorldFrameOrientation(math::Quaterniond(0, 0, 0),
sensors::WorldFrameEnumType::ENU);
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason above.

src/ImuSensor_TEST.cc Outdated Show resolved Hide resolved
src/ImuSensor_TEST.cc Outdated Show resolved Hide resolved
include/ignition/sensors/ImuSensor.hh Show resolved Hide resolved
src/ImuSensor.cc Show resolved Hide resolved
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

ok, two more tiny nits, and then I'll be ready to approve

src/ImuSensor.cc Outdated Show resolved Hide resolved
src/ImuSensor.cc Outdated Show resolved Hide resolved
@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-ignition-releases-2022-04-13-fortress-edifice/1367/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.

3 participants