open_mower_ros icon indicating copy to clipboard operation
open_mower_ros copied to clipboard

Fix deprecation warning

Open rovo89 opened this issue 1 year ago • 6 comments

Compiling mower_logic showed a warning about that constructor being deprecated. It would either call setEuler() or setRPY(). Testing in the simulator, setRPY(0.0, 0.0, yaw) seems to record the docking orientation fine, while setEuler(yaw, 0.0, 0.0) produced crap. That fits to the fact that I saw various getRPY() calls in the code base. But another pair of eyes would definitely help.

rovo89 avatar Aug 18 '24 21:08 rovo89

Hi @rovo89 👋

~~The use of setEuler() is done only if TF2_EULER_DEFAULT_ZYX is set.~~

I searched for this définition in open_mower_ros compilation configuration with no results.

I searched on github "#define TF2_EULER_DEFAULT_ZYX" and i found only 3 repo using this definition.

To me, it confirms that you choose to use the good fonction setRPY().

I would understand that you want someone to check "on the ground"... 😁

quelqundev avatar Aug 19 '24 07:08 quelqundev

Good point I think that's used in multiple places though, since during compilation it comes up a lot

ClemensElflein avatar Aug 19 '24 23:08 ClemensElflein

The use of setEuler() is done only if TF2_EULER_DEFAULT_ZYX is set.

Hm, really? It's #ifndef TF2_EULER_DEFAULT_ZYX, so setEuler() is used if not defined: https://github.com/ros/geometry2/blob/6df9e94363061a24ba890a6ff29771a96b0d756b/tf2/include/tf2/LinearMath/Quaternion.h#L54-L58

That confused me a lot, because I didn't find references to TF2_EULER_DEFAULT_ZYX either... but the reality check worked only for setRPY(). That's why I hope "someone" who is more familiar with tf2 can judge what's the correct function mathematically.

rovo89 avatar Aug 19 '24 23:08 rovo89

Well i'm sorry to have done this misreading...😅

Yeah then i understand that there is a need to check more in detail. 👌

quelqundev avatar Aug 21 '24 14:08 quelqundev

I did some emperical tests, the strongest one being that I modified Quaternion.h and changed the #else branch to call setRPYxxx. It still compiled, while doing the same thing in the #ifndef branch complained about the unknown function. So setEuler it is... but that raises more questions.

The constructor is defined as:

ROS_DEPRECATED Quaternion(const tf2Scalar& yaw, const tf2Scalar& pitch, const tf2Scalar& roll)

And it passes on the arguments to setEuler() in the same order. But the call in OM is with (0.0, 0.0, yaw). Looks mixed up, because whatever is in the yaw variable is actually used as roll. Maybe it's used wrong in all places, especially those which read back the values, and two times wrong gives a right?

rovo89 avatar Sep 02 '24 21:09 rovo89

Finally got confirmation that setRPY(0.0, 0.0, yaw) is the right thing to do: https://discord.com/channels/958476543846412329/958481126882680902/1280476699661701162

Nevertheless, I'm converting this PR to draft so I can check for other instances of the deprecated call.

rovo89 avatar Sep 03 '24 15:09 rovo89