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

Fix incorrect coordinate system in imu publisher #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rendellc
Copy link

Addresses the bug discussed in #9.

Changes the coordinate transformation done for one of the CoordinateSystemEnum.
Not sure if the changes are inline with the intended behaviour, but this PR serves as a place to discuss it.

@micahpearlman
Copy link
Contributor

@rendellc are you using and testing that particular .RightHanded_XBackward_YLeft_ZDown coordinate system?

It seems like every IMU we run across has a different coordinate system and it is no surprise this is a bit of a mess. I know internally with different projects we end up just hacking at this just to get it to work for a particular IMU that a client uses.

@rendellc
Copy link
Author

rendellc commented Aug 12, 2021

Hey, thanks for the clarification. I was a bit confused about where the different coordinate systems came from, so that makes sense.
As mentioned in #9, we do not use RightHanded_XBackward_YLeft_ZUp nor the proposed bug fix RightHanded_XBackward_YLeft_ZDown, so we are not able to test this properly and compare it to a real IMU. Do you know if there is a particular IMU that this coordinate system is supposed to represent? If it is hacked together for a very specific use-case then it may be risky to change its behaviour. Maybe it's more appropriate to add a comment with an explanation?

@micahpearlman
Copy link
Contributor

@rendellc I'm thinking of deleting or commenting out all the untested coordinate systems as I'm not clear on the state of things without creating some sort of test harness -- which is a TODO. Which coordinate system are you using?

@rendellc
Copy link
Author

We are using RightHanded_XRight_YDown_ZForward.
An option is to let the users specify a (possibly improper) rotation matrix to convert from unity coordinates to IMU coordinates. This would allow conversion from Unitys left-handed coordinate system to any left- or right-handed coordinate system, and we could remove the enums. Not sure how much work this is to implement, and a disadvantage is that it is slightly less intuitive than the current setup.

@micahpearlman
Copy link
Contributor

@rendellc that is a very good idea!

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.

None yet

2 participants