jazzy-dev #20

LitheshSari · 2024-10-22T02:22:39Z

modify CMakelists.txt to support source-build Mujoco, in this case, ENV_PATH is not necessarily added to bash file manually.
In ros2-control Jazzy, the function load_urdf is removed from resource manager, so robot_desciprtion callback is used to get robot urdf.
support imu and test it in the diff_drive demo.

1. add the MJResourceManager class. 2. the robot_description is now read from topic. 3. fix the controller configuration failure. 4. modify CMakeLists to support the source-build mujoco.

1. modify the demo launch.py

1. IMU support 2. modify MJCF and URDF for demos 3. fix a typo in CMakeLists.txt

CihatAltiparmak

Hello, firstly thank you for your effort. i actually am not maintainer of this repository and i have no knowledge about controller_interface. But i have wanted to review your PR. If my review does'nt make sense to you (and @sangteak601 ), just ignore this. Btw i think it's better to divide this PR into sub PRs (for instance: Imu added, upgrated to jazzy etc.) but it's definitely not problem at the moment. The last word definitely belongs to @sangteak601

Edit: pre-commit would fit well for this PR.

.gitignore

mujoco_ros2_control/include/mujoco_ros2_control/mujoco_ros2_control.hpp

mujoco_ros2_control/include/mujoco_ros2_control/mujoco_system.hpp

mujoco_ros2_control/src/mujoco_system.cpp

mujoco_ros2_control/src/mujoco_ros2_control_node.cpp

1. return value in main function; 2. format in .gitignore; 3. modify the construction function of `MujocoRos2Control`.

1. use destructor to handle the release of the rendering resource; 2. allocate the mj_data and mj_model in RM member method(not in constructor) and deallocate them in destructor.

mujoco_ros2_control/src/mujoco_ros2_control.cpp

mujoco_ros2_control/src/mujoco_rendering.cpp

1. rewrite the sensors, they are now inherited from SensorBase class; 2. fix the bugs of FTSensor example.

CihatAltiparmak · 2024-11-06T22:39:18Z

mujoco_ros2_control/src/mujoco_ros2_control.cpp

+  stop_cm_thread_ = true;
+  cm_executor_->remove_node(controller_manager_);
+  cm_executor_->cancel();


I'm not sure whether we need this variable. with cancel method, you can finalize spin method's execution. Corresponding Documentation

CihatAltiparmak · 2024-11-06T22:42:09Z

mujoco_ros2_control/src/mujoco_ros2_control_node.cpp

-  mj_deleteModel(mujoco_model);
-
-  return 1;
+  RCLCPP_INFO(node->get_logger(), "Mujoco Sim Stop ...");


rclcpp::shutdown may be good practice here.

CihatAltiparmak · 2024-11-06T22:57:57Z

mujoco_ros2_control/include/mujoco_ros2_control/mujoco_ros2_control.hpp

+  mjModel* mj_model_ = nullptr;
+  mjData* mj_data_ = nullptr;


To move this initializations to constructor's initialization section may be good. Because in case you try to copy this object, mj_model will not be copied [link]. In the future, this may turn back with trouble in debugging phase. What do you think?

CihatAltiparmak · 2024-11-06T23:00:26Z

mujoco_ros2_control/src/mujoco_ros2_control.cpp

+    mj_deleteModel(mj_model_);
+    mj_deleteData(mj_data_);


It would be better to check if mj_model is null or not.

Additionaly, if we are freeing some memory, nullify address to avoid dangling reference (if mj_deleteModel itself doesn't do it)

CihatAltiparmak

Firstly, thank you for your patient.
I made some nitpick comments. However, the important review here is that your PR throws out segmentation fault when i close the rendering window. I have tried to find it yesterday, unfortunately i am yet to find the bug. If i find it, i will write.

I have tried to change the definition order of some variables inside MujocoRos2Control like below. (In your PR, executor crashes)

rclcpp::executors::MultiThreadedExecutor::SharedPtr cm_executor_;
std::shared_ptr<controller_manager::ControllerManager> controller_manager_;

But IMO, in destructor of unique ptr, std::default_delete cannot find MujocoSystemInterface's destructor. This is my backtrace.

Additionaly, you should also modify dockerfile to upgrade jazzy. I use this dockerfile Don't hesitate to use it in your PR: ) Finally would you add xacro with exec_depend tag? Thus it can be installed automatically.

EDIT: My hardware_interface version is 4.18.0. which version do you use? If your PR doesn't throw out segfault, it may be related to version difference.

LitheshSari · 2024-11-07T00:40:38Z

Oops, I use hardware_interface=4.17.0(with ros2-control=4.17.0). I will update to version 4.18.0 and test again.

EDIT:
I write a simplified code which can reproduce the segfault (but I didn't find the bug), the code is here.
The backtrace is listed below

#0  0x00007ffff79af284 in hardware_interface::ResourceManager::~ResourceManager() () from /opt/ros/jazzy/lib/libhardware_interface.so
#1  0x00005555555ac697 in mujoco_ros2_control::MJResourceManager::~MJResourceManager (this=0x5555559a6b90, __in_chrg=<optimized out>) at /home/colcon_ws/src/mujoco_ros2_control/mujoco_ros2_control/src/mujoco_ros2_control.cpp:58
#2  mujoco_ros2_control::MJResourceManager::~MJResourceManager (this=0x5555559a6b90, __in_chrg=<optimized out>) at /home/colcon_ws/src/mujoco_ros2_control/mujoco_ros2_control/src/mujoco_ros2_control.cpp:58
#3  0x00007ffff7f637a1 in ?? () from /opt/ros/jazzy/lib/libcontroller_manager.so
#4  0x00007ffff7f63925 in ?? () from /opt/ros/jazzy/lib/libcontroller_manager.so
#5  0x000055555557dc5e in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use (this=0x555555b31aa0) at /usr/include/c++/13/bits/shared_ptr_base.h:175
#6  std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use_cold (this=0x555555b31aa0) at /usr/include/c++/13/bits/shared_ptr_base.h:199
#7  0x0000555555582f81 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x55555575b900, __in_chrg=<optimized out>) at /usr/include/c++/13/bits/shared_ptr_base.h:1071
#8  std::__shared_ptr<controller_manager::ControllerManager, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x55555575b8f8, __in_chrg=<optimized out>) at /usr/include/c++/13/bits/shared_ptr_base.h:1524
#9  std::shared_ptr<controller_manager::ControllerManager>::~shared_ptr (this=0x55555575b8f8, __in_chrg=<optimized out>) at /usr/include/c++/13/bits/shared_ptr.h:175
#10 mujoco_ros2_control::MujocoSimROS2ControlPluginPrivate::~MujocoSimROS2ControlPluginPrivate (this=0x55555575b8c0, __in_chrg=<optimized out>) at /home/pan/colcon_ws/src/mujoco_ros2_control/mujoco_ros2_control/src/mujoco_ros2_control.cpp:9
#11 std::default_delete<mujoco_ros2_control::MujocoSimROS2ControlPluginPrivate>::operator() (this=<optimized out>, __ptr=0x55555575b8c0) at /usr/include/c++/13/bits/unique_ptr.h:99
#12 std::default_delete<mujoco_ros2_control::MujocoSimROS2ControlPluginPrivate>::operator() (__ptr=0x55555575b8c0, this=<optimized out>) at /usr/include/c++/13/bits/unique_ptr.h:93
#13 std::unique_ptr<mujoco_ros2_control::MujocoSimROS2ControlPluginPrivate, std::default_delete<mujoco_ros2_control::MujocoSimROS2ControlPluginPrivate> >::~unique_ptr (this=0x7fffffffa5a8, __in_chrg=<optimized out>) at /usr/include/c++/13/bits/unique_ptr.h:404
#14 mujoco_ros2_control::MujocoRos2Control::~MujocoRos2Control (this=0x7fffffffa270, __in_chrg=<optimized out>) at /home/colcon_ws/src/mujoco_ros2_control/mujoco_ros2_control/src/mujoco_ros2_control.cpp:141
#15 0x000055555557d4a6 in main (argc=<optimized out>, argv=<optimized out>) at /home/colcon_ws/src/mujoco_ros2_control/mujoco_ros2_control/src/mujoco_ros2_control_node.cpp:50

Looks like the code of part System may cause the fault, because after commenting the import_component function the program can exit normally.

CihatAltiparmak · 2024-11-20T18:37:03Z

Hello @LitheshSari , sorry for turning back to you lately, i have solved bug, now we can end process without segfault but i have no idea about how it crashes and my solution is not proper solution. Just to debug better. I've removed plugin loader from code and all segfaults are fixed. Here is my changes. Just to inform you. Unfortunately i have not so much time to go dive into it at the moment(but i may go dive into it in my free times), but please don't hesitate to tag me in any updates

Sincerely, C.

EDIT: I found something here. But i have still no solution for this. (e.g. i cannot create unique_ptr with custom_deleter function)

Managed vs Unmanaged Objects

A class_loader::ClassLoader allows one to create objects in the form of boost::shared_ptr values so as to provide automatic cleanup of objects. Another one of the important reasons this is implemented is so that the user cannot unload the library prematurely. The user is free to create unmanaged objects via the alternate class_loader::ClassLoader::createUnmanagedInstance method, but this will prevent the [ClassLoader](https://wiki.ros.org/ClassLoader) from stopping the user on unloading the library when there are still objects in memory. This is because the [ClassLoader](https://wiki.ros.org/ClassLoader) cannot be aware of the state of unmanaged instances.

It is encourage that the shared_ptr version class_loader::ClassLoader::createInstance() be used instead to allow for safe usage of the class.

WARNING

    IT IS VERY DANGEROUS TO PUT THE CLASS_LOADER INTO LAZY MODE AND ALSO CREATE UNMANAGED INSTANCES IF YOU ALSO CREATE MANAGED INSTANCES!

sangteak601 · 2024-12-21T11:23:21Z

@LitheshSari Thank you for your contribution, and I apologize for the delayed response. Overall, it looks good, but I think it will be better to divide this into multiple smaller PRs. I will work on this and request your review.

LitheshSari added 5 commits October 18, 2024 23:47

[BUG] update for 22.04 jazzy

b179442

1. add the MJResourceManager class. 2. the robot_description is now read from topic. 3. fix the controller configuration failure. 4. modify CMakeLists to support the source-build mujoco.

[BUG] update for 22.04 jazzy

3936178

1. modify the demo launch.py

[BUG] modify the demo launch.py

786c817

Merge remote-tracking branch 'origin/main'

2d498d0

[Feature] IMU support

0eb9b9e

1. IMU support 2. modify MJCF and URDF for demos 3. fix a typo in CMakeLists.txt

CihatAltiparmak reviewed Nov 2, 2024

View reviewed changes

LitheshSari added 2 commits November 4, 2024 15:35

[Format]: suggestions

3c6abbd

1. return value in main function; 2. format in .gitignore; 3. modify the construction function of `MujocoRos2Control`.

[Feature]: about scope_exit and RAII

d6334d2

1. use destructor to handle the release of the rendering resource; 2. allocate the mj_data and mj_model in RM member method(not in constructor) and deallocate them in destructor.

LitheshSari commented Nov 5, 2024

View reviewed changes

mujoco_ros2_control/src/mujoco_ros2_control.cpp Show resolved Hide resolved

mujoco_ros2_control/src/mujoco_ros2_control.cpp Show resolved Hide resolved

mujoco_ros2_control/src/mujoco_rendering.cpp Show resolved Hide resolved

[Feature]: rewrite sensors

e030760

1. rewrite the sensors, they are now inherited from SensorBase class; 2. fix the bugs of FTSensor example.

CihatAltiparmak reviewed Nov 6, 2024

View reviewed changes

CihatAltiparmak mentioned this pull request Dec 7, 2024

Unmanaged instances throws out segfault after deleting plugin loader ros/class_loader#218

Open

sangteak601 mentioned this pull request Dec 21, 2024

add support for source build mujoco #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jazzy-dev #20

jazzy-dev #20

LitheshSari commented Oct 22, 2024

CihatAltiparmak left a comment •

edited

Loading

CihatAltiparmak Nov 6, 2024

CihatAltiparmak Nov 6, 2024

CihatAltiparmak Nov 6, 2024

CihatAltiparmak Nov 6, 2024

CihatAltiparmak Nov 20, 2024

CihatAltiparmak left a comment •

edited

Loading

LitheshSari commented Nov 7, 2024 •

edited

Loading

CihatAltiparmak commented Nov 20, 2024 •

edited

Loading

sangteak601 commented Dec 21, 2024

jazzy-dev #20

Are you sure you want to change the base?

jazzy-dev #20

Conversation

LitheshSari commented Oct 22, 2024

CihatAltiparmak left a comment • edited Loading

Choose a reason for hiding this comment

CihatAltiparmak Nov 6, 2024

Choose a reason for hiding this comment

CihatAltiparmak Nov 6, 2024

Choose a reason for hiding this comment

CihatAltiparmak Nov 6, 2024

Choose a reason for hiding this comment

CihatAltiparmak Nov 6, 2024

Choose a reason for hiding this comment

CihatAltiparmak Nov 20, 2024

Choose a reason for hiding this comment

CihatAltiparmak left a comment • edited Loading

Choose a reason for hiding this comment

LitheshSari commented Nov 7, 2024 • edited Loading

CihatAltiparmak commented Nov 20, 2024 • edited Loading

sangteak601 commented Dec 21, 2024

CihatAltiparmak left a comment •

edited

Loading

CihatAltiparmak left a comment •

edited

Loading

LitheshSari commented Nov 7, 2024 •

edited

Loading

CihatAltiparmak commented Nov 20, 2024 •

edited

Loading