-
Notifications
You must be signed in to change notification settings - Fork 773
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
Reverse order of rotation offset (rpyOffset) calculation in p3d and imu plugins #1241
base: melodic-devel
Are you sure you want to change the base?
Conversation
Signed-off-by: Ian Chen <[email protected]>
@@ -244,7 +244,7 @@ void GazeboRosIMU::UpdateChild() | |||
rot = pose.Rot(); | |||
|
|||
// apply rpy offsets | |||
rot = this->offset_.Rot()*rot; | |||
rot = rot*this->offset_.Rot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to condition this based on what version of libsdformat is being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added version check in fca594d. It assumes that the release will be made in sdformat 6.3.0 and forward ported though. So we'll need to merge and release the sdformat pull request first
Signed-off-by: Ian Chen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's another IMU plugin (GazeboRosImuSensor) that also uses rpyOffset
orientation = offset.Rot()*sensor->Orientation(); //applying offsets to the orientation measurement |
but from what I can tell, the rpyOffset
that would be used in that sensor is manually set, not automatically generated by the fixed joint reduction code in libsdformat. So it seems okay to leave it as is.
@@ -244,7 +246,13 @@ void GazeboRosIMU::UpdateChild() | |||
rot = pose.Rot(); | |||
|
|||
// apply rpy offsets | |||
// rotation calculation needs to be reversed for sdformat versions > 6.2.0, | |||
// see https://github.com/osrf/sdformat/pull/500 | |||
#if SDF_MAJOR_VERSION < 6 || (SDF_MAJOR_VERSION == 6 && SDF_MINOR_VERSION <= 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, thinking on this some more, if this runs on currently released versions of libsdformat that are greater than 6.2 (eg. v9.0), it'll do the wrong thing. It might be an ugly hack, but maybe we should have a flag in libsdformat that we check instead of the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think injecting an SDFormat element into the plugin block would be much simpler than trying to track which versions have the fix, especially once we start forward-porting:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to use the injected corrected_offsets element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scpeters are you ok with the latest changes?
I think for p3d and the imu plugin, the hmm does that mean we should also not be touching these equations? |
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
as of ef708a3, the equation will only be flipped if it detects that the |
See this comment in sdformat. Ran into an issue with calculation of
rpyOffset
in urdf to sdf conversion, which seems to suggest that therpyOffset
should be post multiplied in the gazebo ros plugins.There isn't really much documentation on the
xyzOffset
andrpyOffset
params and what frames they are in. All the example I found online have been using 0s forrpyOffset
. The changes here will however affect user's code if they are manually specifyingrpyOffset
.Signed-off-by: Ian Chen [email protected]