-
Notifications
You must be signed in to change notification settings - Fork 89
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
Conversation
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: ahcorde <[email protected]>
I opened this PR for review. 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.
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); |
Signed-off-by: ahcorde <[email protected]>
GitHub actions is failing at the
|
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 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?
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 |
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.
Should we add instructions for building a workspace with the demos first (including installing dependencies) before users try these commands?
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Vatan Aksoy Tezer <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
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.
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...
std::dynamic_pointer_cast<ignition_ros2_control::IgnitionSystemInterface>( | ||
this->dataPtr->controller_manager_); | ||
this->dataPtr->controller_manager_->read(); | ||
this->dataPtr->controller_manager_->update(); |
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 seems to me that the following line is missing. Can this be? Or I am getting something wrong?
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_demos/launch/cart_example_effort.launch.py
Outdated
Show resolved
Hide resolved
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
thank you @destogl for your feedback! Just waiting the last review from @chapulina |
Signed-off-by: ahcorde <[email protected]>
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> |
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.
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 |
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.
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"
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.
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 <[email protected]>
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.
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) |
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.
I see the code supports more combinations now. Can we document them here?
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Thank you all for the reviews! |
🎉 New feature
Summary
This PR includes two packages:
ros2_control
with Ignition RoboticsTest it
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge