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

Add an IMU sensor plugin that inherits from SensorPlugin #363

Merged
merged 13 commits into from Apr 25, 2017

Conversation

Projects
None yet
@alessandrosettimi
Copy link
Contributor

commented Oct 23, 2015

We added a new imu plugin: gazebo_ros_imu_sensor.
Main differences from gazebo_ros_imu:

  • inheritance from SensorPlugin,
  • measurements are given from gazebo imu sensor instead of being computed by the ros plugin,
  • gravity is included in inertial measurements.
@l0g1x

This comment has been minimized.

Copy link

commented Nov 18, 2015

Has anyone had time to look at this?

@hsu

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

This looks good. Could you please add your overview (3 bullet points in PR description) on differences between gazebo_ros_imu and gazebo_ros_imu_sensor somewhere? I don't have a good place for it in mind (maybe here). I am looking into doxygen generation as well.

@alessandrosettimi

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2016

I uptaded the documentation of the plugin and I also created this pull request in the tutorials documentation: https://bitbucket.org/osrf/gazebo_tutorials/pull-requests/335

@alessandrosettimi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2016

The aforementioned pull request has been merged.

@nicolaerosia

This comment has been minimized.

Copy link

commented Apr 3, 2016

@scpeters scpeters added this to the untargeted milestone Apr 25, 2016

@j-rivero

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

@osrf-jenkins run test please

void gazebo::GazeboRosImuSensor::Load(gazebo::sensors::SensorPtr sensor_, sdf::ElementPtr sdf_)
{
sdf=sdf_;
sensor=dynamic_cast<gazebo::sensors::ImuSensor*>(sensor_.get());

This comment has been minimized.

Copy link
@meyerj

meyerj Jul 26, 2016

Contributor

You should check against null pointer after the dynamic_cast here. Maybe the plugin was loaded for a sensor which is not an ImuSensor.

This comment has been minimized.

Copy link
@alessandrosettimi

alessandrosettimi Jul 27, 2016

Author Contributor

I added the test you suggested.

@meyerj

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

Is there a reason why you took over the scalar gaussianNoise parameter from gazebo_ros_imu and add a white noise term to all fields individually instead of relying on the more powerful noise model already present in Gazebo's ImuSensor? Using the same noise variance for linear acceleration, angular velocity and even quaternion components has nothing do to with reality. However, the current ImuSensor (https://bitbucket.org/osrf/gazebo/src/2e153b94b73a5f3e6301cc65640920cdb311f08c/gazebo/sensors/ImuSensor.cc?at=default&fileviewer=file-view-default) does not apply any noise or bias to the orientation part, which is also not very realistic.

+1 for providing an alternative ROS plugin for the broken gazebo_ros_imu plugin (unrealistic noise and no gravity component in acceleration).

@130s

This comment has been minimized.

Copy link

commented Jul 27, 2016

#363 (comment)

This is present in tutorial but not merged: http://gazebosim.org/tutorials?tut=ros_gzplugins&cat=connect_ros#IMUsensor(GazeboRosImuSensor)

Yeah. Just as fyi, the tutorial is confusing (2 IMU plugins. One is not existent yet (GazeboRosImuSensor. The one this PR suggests). The info for the other GazeboRosImu isn't really helpful). Since we still have only GazeboRosImu, I opened a pr to update the doc.

@alessandrosettimi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

I added a check on Gazebo version since some functions have changed since the version 6.0

@alessandrosettimi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2016

I also changed some function that are now deprecated for gazebo version greater than 7.0.

@jbrookshire

This comment has been minimized.

Copy link

commented Sep 8, 2016

Yes, my thanks too for adding this sensor plugin. I agree, though: not clear why gaussianNoise parameter is needed. It seems to just add Gaussian noise on top of the sensor's Gaussian noise.

//NAMESPACE
if (sdf->HasElement("robotNamespace"))
{
robot_namespace = "/" + sdf->Get<std::string>("robotNamespace") +"/";

This comment has been minimized.

Copy link
@Skydes

Skydes Dec 14, 2016

Why not including the root '/' in the namespace ? As already used in the gazebo_ros_control plugin (see http://gazebosim.org/tutorials?tut=ros_control&cat=connect_ros)

This comment has been minimized.

Copy link
@alessandrosettimi

alessandrosettimi Mar 20, 2017

Author Contributor

done in 3b4735a

}

//BODY NAME
if (sdf->HasElement("bodyName"))

This comment has been minimized.

Copy link
@Skydes

Skydes Dec 14, 2016

"bodyName" is referenced as "frameName" in other plugins (see here), maybe better keeping the same convention ?

This comment has been minimized.

Copy link
@alessandrosettimi

alessandrosettimi Mar 20, 2017

Author Contributor

done in 0cb04ca

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

@j-rivero think this can be merged? I'm not super familiar with IMU sensors. This PR also targets indigo, maybe it should just be targeted to Kinetic since the original PR is so old now?

@iluetkeb

This comment has been minimized.

Copy link

commented Mar 30, 2017

+1 for merging. This can only be better than what's currently available.

Lets merge first to Indigo and then pull up to Kinetic.

@jliviero

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2017

We would also find this plugin useful, +1 to merging.

@j-rivero

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2017

Thanks for the feedback. Before merging, it would be nice to include a test or at least an example world that includes the IMU so black box testing can be run without having to implement one.

@iluetkeb we try to merge and release new additions or modifications first into Jade/Kinetic and threat Indigo as a more stable code where changes landed after a period of testing in the other distributions.

@davetcoleman davetcoleman changed the base branch from indigo-devel to kinetic-devel Apr 10, 2017

@davetcoleman davetcoleman changed the title added a IMU sensor plugin that inherits from SensorPlugin Add an IMU sensor plugin that inherits from SensorPlugin Apr 10, 2017

@j-rivero j-rivero merged commit 4a8821f into ros-simulation:kinetic-devel Apr 25, 2017

4 of 5 checks passed

ros_gazebo_pkgs-ci-pr_regression_any-xenial-amd64 Build finished.
Details
Kpr__gazebo_ros_pkgs__ubuntu_xenial_amd64 Build finished.
Details
ros_gazebo7_pkgs-ci-pr_any-xenial-amd64 Build finished.
Details
ros_gazebo_pkgs-ci-pr_any-xenial-amd64 Build finished.
Details
ros_gazebo_pkgs-ci-pr_any_kinetic-xenial-amd64 Build finished.
Details
@j-rivero

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Thanks very much @alessandrosettimi for the work and the rest of you for the comments.

@meyerj meyerj referenced this pull request May 2, 2018

Open

IMU plugin #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.