search

KumarRobotics / kr_mav_control

Code for quadrotor control
BSD 3-Clause "New" or "Revised" License
106 stars 42 forks source link

Potential bugs with usage of Eigen #119

Closed versatran01 closed 4 years ago

versatran01 commented 4 years ago

I noticed there are some issues with how Eigen is used in some parts of this repo. For example

https://github.com/KumarRobotics/quadrotor_control/blob/master/mav_manager/include/mav_manager/manager.h

https://github.com/KumarRobotics/quadrotor_control/blob/fd9812b1d563de07c3248cb6c5ebe0934c51deab/mav_manager/include/mav_manager/manager.h#L76

Passing Eigen objects by value is discouraged. https://eigen.tuxfamily.org/dox/group__TopicPassingByValue.html

This will not have a problem if the user doesn't compile with -march=native, but I suggest we follow eigen's guidelines on this.

kartikmohta commented 4 years ago

@versatran01 Inspecting the quadrotor_control code in your free time? :thinking:

versatran01 commented 4 years ago

@kartikmohta Not really, we were having a meeting and people were discussing issues with two versions of quad control.

Also, do you agree this is a problem? or am I being stupid?

versatran01 commented 4 years ago

Also there's no need to use Eigen's aligned allocator for types that do not require alignment, as in

https://github.com/KumarRobotics/quadrotor_control/blob/fd9812b1d563de07c3248cb6c5ebe0934c51deab/trackers/std_trackers/include/std_trackers/traj_gen.h#L49

kartikmohta commented 4 years ago

Maybe you can call me lazy but I just try to use the rule of thumb

Use aligned_allocator for Eigen types

rather than remember when to use it and when to not, though I agree that for any dynamic sized ones you never need to bother.

versatran01 commented 4 years ago

yep, that's lazy. :laughing: