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
Odometry should be published in robot coordinates #774
Conversation
Hi, If I understand correctly, you are using ROS to get information. However, you modified the odometry sensor instead of ROS overlay (middleware/ros), if you modification is related to ROS package as it seems to me you have to modify ROS overlay and not internal sensor. If you modify directly the sensor you are changing MORSE behavior what I think it is incorrect, in addition this changes affects other middlewares that MORSE provides. |
No, morse publishes "wrong" information. ROS just reads it and the nav stack tries to handle that. Edit: I now see what you mean by ros overlay. I am new to morse. But I don't see how the change could be made in the ros overlay since the odom computation takes arguments not available there. Additionally I just don't think the way odom is published right now makes sense for any middleware. |
I think @johaq is right. Also, this "bug" was introduced in the the MORSE 1.4 release. Before that, odom was derived correctly. See: 43cd5a9 Lastly, check the comments of this commit. Someone already suggested that the change made by @adegroote (no offense Arnaud :) ) breaks the odom sensor. |
OK, @warp1337, I understand, I did not know this as I was not involved previously. And thanks for explaining it to me. In fact, before accepting this merge I wanted to review history of the odometry sensor to know why a so common sensor as this one was broken, unfortunately I had not time. @johaq, now I have two comments/questions:
Thanks, and sorry due to my late answer but I am busy with other things. |
@adegroote @pierriko maybe you can chip in? You know very well this corner of MORSE, don't you? |
Yes I looked at the old code. In fact i got my changes from previous versions. |
The test does not seem to check velocity returned by odometer sensor, so the changes is completely untested on the odometry side. Through, I'm not sure about my comment on this commit, a break in linear_velocity should appear everywhere. |
OK, @johaq, thanks for confirming. |
@dgerod @johaq @adegroote @severin-lemaignan Thx for the merge! |
I noticed that the odometry sensor publishes its twist in inverted world coordinates. Below you see the robot standing aligned to the world coordinate system. /cmd_vel in x direction results in negative x value published as odometry.
Here the robot's x-axis is aligned with the world's y-axis. So /cmd_vel in x direction results in negative y value published as odometry.
I changed the odometry sensor to publish in robot coordinates so the regular ros navstack can be used. Results:
Remark: I also noticed spikes in the angular odometry. These were results of euler angles being used instead of say quaternions. This is kind of fixed since a robot usually does not complete a full rotation in one time tick.