search

robotology / idyntree

Multibody Dynamics Library designed for Free Floating Robots
BSD 3-Clause "New" or "Revised" License
155 stars 65 forks source link

Fix bug in DynamicsLinearization due to use of ; in place of + #1161

Closed traversaro closed 3 months ago

traversaro commented 3 months ago

Back in January 2016, in a rush to avoid that the work I did during my secondment in Eindhoven diverged too much w.r.t. to iDynTree master, I merged https://github.com/robotology/idyntree/pull/129 . In it, I included my work on Dynamics linearization that eventually got published in https://www.aimsciences.org/article/doi/10.3934/jgm.2022009 / https://arxiv.org/pdf/2204.05092.pdf (Equations 22 and 23, if anyone is curious).

Unfortunately, I remember that a test was not passing, so I disabled it as I was not able to fix it. Then, time passed and for linearization we started using almost uniquely autodiff system, so I did not turned back my attention to hand-written DynamicsLinearization functions in iDynTree. However, after some years (but still a few years ago) I started noticing a warning that only occured in Visual Studio/Windows-builds:

[105/308] Building CXX object src\model\CMakeFiles\idyntree-model.dir\src\DynamicsLinearization.cpp.obj
D:\a\idyntree\idyntree\src\model\src\DynamicsLinearization.cpp(311): warning C4552: '*': result of expression not used

I remember thinking that "result of expression not used" probably indicated a real bug, and that could be the reason why my test in Eindhoven were not passing! Anyhow, I never actually looked into it. However, today @S-Dafarra noticed again the warning and pinged me about it, so I finally looked into it. The bug itself was quite trivial (a ; instead of a + typo), but I was not able to find it back in 2015/2016. And after fixing it, ~indeed the test finally passed!~ the test have less failures, but unfortunately still fails. So, let's just fix the obvious error, and leave the test disabled.

traversaro commented 3 months ago

Having said that, given their limited use we could eventually remove those functions. But let's at give them a release as fully working functions with nice working tests.

traversaro commented 3 months ago

Cool, valgrind tests still fail.

traversaro commented 3 months ago

Cool, valgrind tests still fail.

Ok, I was a bit optimistic, test in general still fail. However, let's at least remove the warning and improve the situation a bit.