Odometry should be published in robot coordinates

[johaq]

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.

[dgerod]

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.

[johaq]

No, morse publishes "wrong" information. ROS just reads it and the nav stack tries to handle that. If you want morse to publish odom in inverted world coordinates then you should make that clear in your ros tutorials and maybe provide an odom converter node. But as is the basic ROS nav stack does not work properly with the odom provided by morse.

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.

[warp1337]

I think @johaq is right. Also, this "bug" was introduced in the the MORSE 1.4 release. Before that, odom was derived correctly.

See: https://github.com/morse-simulator/morse/commit/43cd5a9cc9d5a265a317a68b2ec86be84a9e5a9a

Lastly, check the comments of this commit. Someone already suggested that the change made by @adegroote (no offense Arnaud :) ) breaks the odom sensor.

[dgerod]

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:

• Did you reviewed the old code before doing this modification? I think this review is important to be sure that we are fixing completely the situation, maybe other things must be fixed, too.
• Have you checked why unit tests were passing with broken code and they are still passing with new one? Could you review unit tests to understand why and write better ones? If we don't fix them someone could break the sensor again in future.

Thanks, and sorry due to my late answer but I am busy with other things.

[severin-lemaignan]

@adegroote @pierriko maybe you can chip in? You know very well this corner of MORSE, don't you?

[johaq]

Yes I looked at the old code. In fact i got my changes from previous versions. From what I can tell some helper functions were introduced and there were a lot of changes beautifying old code with new helper functions. But in the case here the code did something different than the helper functions but it was replaced anyway.

[adegroote]

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.

[dgerod]

OK, @johaq, thanks for confirming.

[warp1337]

@dgerod @johaq @adegroote @severin-lemaignan Thx for the merge!