-
Notifications
You must be signed in to change notification settings - Fork 999
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
Fix resource management in UAS node #1961
base: ros2
Are you sure you want to change the base?
Conversation
ac225e1
to
8a0e968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UAS test fails. Jazzy's uncrustify also unhappy.
Eigen::Vector3d af, | ||
Eigen::Vector3d const & p, | ||
Eigen::Vector3d const & v, | ||
Eigen::Vector3d const & af, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For not copying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually questionable thing, what's faster - just copy 4 float64's or access trough a pointer all fields.
To me that looks as an unneeded preliminary optimisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"just copy 4 float64's" -- 9 doubles, to be precise; which is 72 bytes.
"or access trough a pointer all fields" -- how do you think the fields are accessed if the object is passed by value?
Eigen::Vector3d
is a type that is bigger than just a couple of bytes and it has a copy constructor. It is a good practice to pass such types by const reference.
std::dynamic_pointer_cast<rclcpp::Node>(uas_)->get_fully_qualified_name(), options)) | ||
{} | ||
UASPtr uas, const std::string & subnode_name, | ||
const rclcpp::NodeOptions & options = rclcpp::NodeOptions()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to move that into cpp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S.t. all plugins don't have to recompile when something changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's rarely changed, and if so, I'd prefer to recompile everything...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you prefer to recompile everything?
On the
The test executable crashes before most of the tests have a chance to fail. Could you maybe make the tests in the |
- Removed unnecessary dynamic_pointer_cast<>'s Removed unused Plugin ctor
Prohibit copying and moving of UAS Removed startup_delay_timer
UAS
yaml-cpp
in a correct way.