-
Notifications
You must be signed in to change notification settings - Fork 92
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
PID effort control of joints (+mimic) #122
base: humble
Are you sure you want to change the base?
Conversation
55f6364
to
0104d7a
Compare
5e6d70c
to
cb75106
Compare
de3e194
to
ea36469
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.
I've been hearing good things from testing. For example, this can actually handle contact with the environment. That's a big win 👍
Can you rename the PR, "PID effort control of a mimic joint" or something like that? |
@livanov93 it would help reviewers a lot in the future if you put a nice, long description in the PR. Especially on a 1,000+ line PR like this. Why did you make the design decisions you have made? Why did you choose to have 2 modes, ABS and PID? Then your reviewers won't have to ask ;) |
@@ -25,6 +25,10 @@ | |||
#include "rclcpp_lifecycle/state.hpp" | |||
#include "rclcpp_lifecycle/node_interfaces/lifecycle_node_interface.hpp" | |||
|
|||
#include "ign_ros2_control_parameters.hpp" |
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.
TODO still (nitpick)
I was writing it at the moment you wrote this. See here. |
cf8b3b5
to
b00a6bf
Compare
b00a6bf
to
a100537
Compare
06c7536
to
496dde5
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.
@livanov93 Thank you for addressing my feedback. This looks good to me!
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 won't block this any more. I think it's leaps and bounds ahead of what we had before.
@AndyZe @livanov93 This seems like a great change. I'm curious what you're seeing re: joint position accuracy. Sounds from some other issues (#113, for example) there are/were kinematic accuracy issues and sag with the prior position controllers anyway. I'd imagine the cascaded controller coming in here could be better at achieving low joint position error than the ros-industrial/universal_robot#521 (comment) It'll be really nice to have a simulated robot that can make contact with a rigid environment. I was getting back to working on some ideas about elastic mounting, etc. in Gazebo Classic but maybe I should shift focus and try this out in new Gazebo first. |
@ahcorde friendly ping for a review. |
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.
Sorry for the late response I was out some weeks, I made a initial pass and I will test it right today
@@ -39,10 +44,30 @@ elseif("$ENV{IGNITION_VERSION}" STREQUAL "fortress") | |||
set(IGN_GAZEBO_VER ${ignition-gazebo6_VERSION_MAJOR}) | |||
message(STATUS "Compiling against Ignition Fortress") | |||
|
|||
find_package(ignition-cmake2 REQUIRED) |
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.
duplicated code in the if
and else
#include "ignition/gazebo/components/Name.hh" | ||
#include "ignition/gazebo/components/JointType.hh" |
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.
alphabetize
ignerr << "MimicJointSystem Failed to initialize because [" << | ||
this->dataPtr->model.Name(_ecm) | ||
<< "] (Entity=" << _entity << ")] is not a model.. "; |
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.
ignerr << "MimicJointSystem Failed to initialize because [" << | |
this->dataPtr->model.Name(_ecm) | |
<< "] (Entity=" << _entity << ")] is not a model.. "; | |
ignerr << "MimicJointSystem Failed to initialize because [" | |
<< this->dataPtr->model.Name(_ecm) | |
<< "] (Entity=" << _entity << ")] is not a model.. "; |
ignwarn << "Detected jump back in time [" << std::chrono::duration_cast < | ||
std::chrono::seconds > (_info.dt).count() | ||
<< "s]. System may not work properly." << std::endl; |
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.
ignwarn << "Detected jump back in time [" << std::chrono::duration_cast < | |
std::chrono::seconds > (_info.dt).count() | |
<< "s]. System may not work properly." << std::endl; | |
ignwarn << "Detected jump back in time [" | |
<< std::chrono::duration_cast <std::chrono::seconds > (_info.dt).count() | |
<< "s]. System may not work properly." << std::endl; |
@@ -16,6 +16,8 @@ | |||
|
|||
#include <ignition/msgs/imu.pb.h> | |||
|
|||
#include <iostream> | |||
|
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.
#include <ignition/gazebo/components/JointVelocityReset.hh> | ||
|
||
#include <ignition/gazebo/components/JointAxis.hh> | ||
#include <ignition/gazebo/components/JointType.hh> | ||
#include <ignition/gazebo/components/Joint.hh> |
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.
alphabetize
#include <ignition/gazebo/components/JointVelocityReset.hh> | |
#include <ignition/gazebo/components/JointAxis.hh> | |
#include <ignition/gazebo/components/JointType.hh> | |
#include <ignition/gazebo/components/Joint.hh> | |
#include <ignition/gazebo/components/JointVelocityReset.hh> | |
#include <ignition/gazebo/components/JointAxis.hh> | |
#include <ignition/gazebo/components/JointType.hh> | |
#include <ignition/gazebo/components/Joint.hh> |
@@ -417,15 +623,9 @@ void IgnitionSystem::registerSensors( | |||
} | |||
|
|||
static const std::map<std::string, size_t> interface_name_map = { | |||
{"orientation.x", 0}, |
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.
to reduce the changes, can you restore these changes? Same with other similar changes
{ | ||
RCLCPP_WARN(this->nh_->get_logger(), "On init..."); | ||
|
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.
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.
add gripper_controllers
to ign_ros2_control
package.xml
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.
add gripper_controllers
to ign_ros2_control
package.xml
Depends on #121
Description
PR introduces mapping all types of command interfaces (position, velocity, effort) to
ForceJointCmd
component.:Using the
MimicJointSystem
that can be found here which is pure ignition/gz system that is disconnected from ROS ecosystem.Using the same approach as for regular joints within
ign_ros2_control::IgnitionSystemInterface
here