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

Ignition ros2 control #1

Merged
merged 50 commits into from
Dec 3, 2021
Merged

Ignition ros2 control #1

merged 50 commits into from
Dec 3, 2021

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Mar 13, 2021

🎉 New feature

Summary

This PR includes two packages:

  • ignition_ros2_control - A system plugin that allows to use ros2_control with Ignition Robotics
  • ignition_ros2_control_demos - Some demos

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@chapulina chapulina added this to Inbox in Core development via automation Mar 15, 2021
@chapulina chapulina moved this from Inbox to In progress in Core development Mar 15, 2021
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde marked this pull request as ready for review March 21, 2021 22:42
@ahcorde
Copy link
Collaborator Author

ahcorde commented Mar 21, 2021

I opened this PR for review.

ignition_ros2_control

There is on issue with the color. I checked the sdformat library ( version 11 ) is not parsing the color properly for Ignition. According the migration guide.

  • URDF defined the color as rgba but in ignition the color is defined as ambient, specular and diffuse.
  • The other way to use a color in the URDF format is with OGRE materials, but according with the migration guide there is no support for this.

None of this two methods are working.

With this patch in sdformat plain color are working:

diff --git a/src/parser_urdf.cc b/src/parser_urdf.cc
index 85eebbaa..02c1139d 100644
--- a/src/parser_urdf.cc
+++ b/src/parser_urdf.cc
@@ -3197,6 +3197,19 @@ void CreateVisual(tinyxml2::XMLElement *_elem, urdf::LinkConstSharedPtr _link,
     CreateGeometry(sdfVisual, _visual->geometry);
   }
 
+  if (_visual->material){
+    double color[4];
+    color[0] = _visual->material->color.r;
+    color[1] = _visual->material->color.g;
+    color[2] = _visual->material->color.b;
+    color[3] = _visual->material->color.a;
+    AddKeyValue(sdfVisual, "material", "");
+    auto materialTag = sdfVisual->FirstChildElement("material");
+    AddKeyValue(materialTag, "ambient", Values2str(4, color));
+    AddKeyValue(materialTag, "diffuse", Values2str(4, color));
+    AddKeyValue(materialTag, "specular", Values2str(4, color));
+  }
+
   // set additional data from extensions
   InsertSDFExtensionVisual(sdfVisual, _link->name);

ign_ros2_control

@chapulina chapulina self-requested a review March 22, 2021 17:01
@chapulina chapulina moved this from In progress to In review in Core development Mar 22, 2021
@chapulina chapulina requested a review from adlarkin April 12, 2021 18:58
Signed-off-by: ahcorde <ahcorde@gmail.com>
@adlarkin
Copy link

GitHub actions is failing at the Setup colcon workspace step, does this need to be addressed?

Reading package lists...
E: Unable to correct problems, you have held broken packages.
Error: Process completed with exit code 100.

Copy link

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

For now, I went through the README, so I'll have to review the code and other files next. My main question about the README is that there appear to be times where Gazebo classic is referenced (for example, using gzclient to run the Gazebo client), but I thought that this package should target ign-gazebo - so, should Gazebo classic be referenced at all here?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +188 to +190
ros2 launch ignition_ros2_control_demos cart_example_position.launch.py
ros2 launch ignition_ros2_control_demos cart_example_velocity.launch.py
ros2 launch ignition_ros2_control_demos cart_example_effort.launch.py

Choose a reason for hiding this comment

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

Should we add instructions for building a workspace with the demos first (including installing dependencies) before users try these commands?

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Vatan Aksoy Tezer and others added 3 commits November 26, 2021 09:48
Signed-off-by: Vatan Aksoy Tezer <vatan@picknik.ai>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

This looks great for the first implementation. I added few comments which could also be addressed int the future. We should merge this soon to continue development for Galactic and Rolling.

P.S. Sorry that took me so long...

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ignition_ros2_control/LICENSE Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ignition_ros2_control/package.xml Outdated Show resolved Hide resolved
ignition_ros2_control/src/ignition_ros2_control_plugin.cpp Outdated Show resolved Hide resolved
ignition_ros2_control/src/ignition_ros2_control_plugin.cpp Outdated Show resolved Hide resolved
std::dynamic_pointer_cast<ignition_ros2_control::IgnitionSystemInterface>(
this->dataPtr->controller_manager_);
this->dataPtr->controller_manager_->read();
this->dataPtr->controller_manager_->update();
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that the following line is missing. Can this be? Or I am getting something wrong?

Suggested change
this->dataPtr->controller_manager_->update();
this->dataPtr->controller_manager_->update();
this->dataPtr->controller_manager_->write();

We should execute updates something like ros2_control_node is doing. (check here)

ignition_ros2_control/src/ignition_system.cpp Show resolved Hide resolved
@adlarkin adlarkin removed their request for review November 29, 2021 17:07
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Nov 30, 2021

thank you @destogl for your feedback! Just waiting the last review from @chapulina

Signed-off-by: ahcorde <ahcorde@gmail.com>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ignition_ros2_control/src/ignition_ros2_control_plugin.cpp Outdated Show resolved Hide resolved
README.md Outdated

```xml
<gazebo>
<plugin name="ignition_ros2_control" filename="libignition_ros2_control.so">
<parameters>$(find ignition_ros2_control_demos)/config/cartpole_controller.yaml</parameters>
<controller_manager_node_name>controller_manager</controller_manager_node_name>
Copy link
Member

Choose a reason for hiding this comment

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

Could we rather set a “prefix” which will be then used before both node names, plugin node and CM node? This is then also compatible with most robot descriptions, which have an option to set robot's prefix in multi robot systems.

The idea here is to enable to run multiple Plugins/CMs in Ignition and enable complex multi-robot simulations. What do you think?


include_directories(include)

add_library(${PROJECT_NAME}-system SHARED
Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, this plugin is loading controller manager and “whole” ros2_control framework. If this is correct, I propose to rename the library to simply "${PROJECT_NAME}". The name would be something similar to Gazebo library, i.e., "libgazebo_ros2_control.so"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now it's creating libignition_ros2_control-system.so. I added the postfix -system beacause it's the standard way to name them.

As you can see for example in the IMU: ignition-gazebo-imu-system

Signed-off-by: ahcorde <ahcorde@gmail.com>
Copy link
Collaborator

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I think it's about time this gets merged and we start reviewing some smaller PRs 😄 Things are looking good!

README.md Outdated
Tested on:

- [ROS 2 Foxy](https://docs.ros.org/en/foxy/Installation.html)
- [Ignition Edifice](https://ignitionrobotics.org/docs/edifice)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the code supports more combinations now. Can we document them here?

ignition_ros2_control/package.xml Show resolved Hide resolved
ignition_ros2_control_demos/examples/example_effort.cpp Outdated Show resolved Hide resolved
ignition_ros2_control_demos/package.xml Outdated Show resolved Hide resolved
ignition_ros2_control/src/ignition_ros2_control_plugin.cpp Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Dockerfile/Dockerfile Show resolved Hide resolved
Core development automation moved this from In progress to In review Dec 3, 2021
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Dec 3, 2021

Thank you all for the reviews!

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.

None yet

6 participants