Skip to content
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

Open
wants to merge 5 commits into
base: ros2
Choose a base branch
from

Conversation

ugol-1
Copy link

@ugol-1 ugol-1 commented Jun 16, 2024

  • Fixed compiler warnings
  • Removed circular dependencies between UAS node and its plugins => clean node shutdown. The UAS and plugins destructors were never called before.
  • Timely creation and destruction of UAS executor thread => clean node shutdown. Before it was:
    [WARN] [1718579669.490040973] [mavros.uas]: UAS Executor terminated
    terminate called after throwing an instance of 'rclcpp::exceptions::RCLInvalidArgument'
       what():  guard condition implementation is invalid, at ./src/rcl/guard_condition.c:172
    
  • No more "delayed initialization" in UAS
  • Linking to yaml-cpp in a correct way.

@ugol-1 ugol-1 force-pushed the clean_uas branch 2 times, most recently from ac225e1 to 8a0e968 Compare June 17, 2024 00:23
Copy link
Member

@vooon vooon left a 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For not copying

Copy link
Member

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.

Copy link
Author

@ugol-1 ugol-1 Jun 25, 2024

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());
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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...

Copy link
Author

@ugol-1 ugol-1 Jun 25, 2024

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?

mavros/src/plugins/setpoint_trajectory.cpp Show resolved Hide resolved
mavros_extras/CMakeLists.txt Show resolved Hide resolved
@ugol-1
Copy link
Author

ugol-1 commented Jun 22, 2024

UAS test fails. Jazzy's uncrustify also unhappy.

On the ros2 branch, the UAS test run result looks like following:

$ build/mavros/mavros-uas-test
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from TestUAS
[ RUN      ] TestUAS.is_plugin_allowed
[       OK ] TestUAS.is_plugin_allowed (37 ms)
[ RUN      ] TestUAS.add_plugin__route_message__filter
[INFO] [1719053548.105424681] [test_mavros_uas]: Plugin test1 created
[INFO] [1719053548.105817054] [test_mavros_uas]: Plugin test1 initialized
[INFO] [1719053548.105848344] [test_mavros_uas]: Plugin test2 created
[INFO] [1719053548.105885739] [test_mavros_uas]: Plugin test2 initialized
[       OK ] TestUAS.add_plugin__route_message__filter (15 ms)
[----------] 2 tests from TestUAS (52 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (52 ms total)
[  PASSED  ] 2 tests.
cannot publish data, at ./src/rmw_publish.cpp:62 during '__function__'
Fail in delete datareader, at ./src/rmw_service.cpp:100 during '__function__'
Segmentation fault (core dumped)

The test executable crashes before most of the tests have a chance to fail. Could you maybe make the tests in the ros2 branch work first #1962 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants