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_transformer: Fix transformation of the orientation of IMU #15

Merged

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Aug 15, 2022

Solves #8.

The previous computation was wrong. According to REP-145, IMU orientation should express attitude of the sensor frame in a world frame. imu_transformer changes the sensor frame, so it should just recompute the new attitude by transforming the old sensor frame into the new one.

I added an example launch file and rviz config to the test folder which you can launch to visually inspect the situation.

I also added a unit test that fails with the old implementation and succeeds with the new one.

I also made one other fix not mentioned in #8 - I do not transform orientation covariance. If I get it correctly, the covariance is relative to the axes of the fixed world frame, so it should not change when using a different sensor frame - at least not in the case of a gravity-aligned IMU. I'm not sure how that would be in completely floating IMUs. But please, check my lines of thought around here. I haven't found a good definition of the exact meaning of orientation covariance, so this is my best guess. In the gravity-aligned case, I understand it so that estimating roll/pitch of an IMU is the same task with the same errors as estimating roll/pitch of base_link. Yaw would be similar even when not georeferenced - in the worst case, there would be some static offset, but that should not affect the covariance in world yaw.

The previous computation was wrong. According to REP-145, IMU orientation should express attitude of the sensor frame in a world frame. imu_transformer changes the sensor frame, so it should just recompute the new attitude by transforming the old sensor frame into the new one.
@peci1
Copy link
Contributor Author

peci1 commented Nov 8, 2022

@paulbovbel @tonybaltovski friendly ping

@tonybaltovski
Copy link
Member

LGTM, thanks!

@tonybaltovski tonybaltovski merged commit 775b00f into ros-perception:noetic-devel Nov 9, 2022
@peci1
Copy link
Contributor Author

peci1 commented Nov 9, 2022

Can I also ask you for melodic backport?

@tonybaltovski
Copy link
Member

Done: f426878

@peci1
Copy link
Contributor Author

peci1 commented Nov 18, 2022

Thanks! Can I ask you for a release in both Melodic and Noetic? Given the pace of changes in this repo, I think waiting for others to accumulate would mean years =)

@tonybaltovski
Copy link
Member

Done: ros/rosdistro#35391 and ros/rosdistro#35390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants