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

Various bug fixes #114

Merged
merged 5 commits into from
Feb 3, 2023
Merged

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Jan 31, 2023

Fixes #113. To quote from that issue:

    if (this->dataPtr->joints_[i].joint_control_method & VELOCITY) {
      ...
        *jointVelCmd = ignition::gazebo::components::JointVelocityCmd(
          {this->dataPtr->joints_[i].joint_velocity_cmd});
      }
    }

    if (this->dataPtr->joints_[i].joint_control_method & POSITION) {
      ...
        vel->Data()[0] = targetVel;
      }
    }

    if (this->dataPtr->joints_[i].joint_control_method & EFFORT) {
      ...
        *jointEffortCmd = ignition::gazebo::components::JointForceCmd(
          {this->dataPtr->joints_[i].joint_effort_cmd});
      }
    }
  }

Here are some of the bugs in that code:

  • VELOCITY can set a target velocity, then POSITION can set a different velocity target. In other words, POSITION overrides the effect of VELOCITY.
  • All of POSITION, VELOCITY, and EFFORT can be active at any time. This does not seem right -- only 1 should be active.
  • There's no PID gain specified in calculating the POSITION command. The gain is 1.
  • When I send a position command to just one joint (for example), the other joints tend to sag downward with gravity.

Here's a "before" video. It's hard to see but note that the "upper_arm_link" of the robot is always drooping down with gravity although only the elbow joint is actuated.

slow_droop.online-video-cutter.com.mp4

Here's an "after" video showing no droop of other joints besides the actuated elbow.

no_droop

@AndyZe AndyZe requested a review from ahcorde as a code owner January 31, 2023 18:41
@AndyZe AndyZe force-pushed the saggy_controller_back_to_main branch from 14d358f to 6716415 Compare January 31, 2023 18:49
@@ -173,8 +175,14 @@ bool IgnitionSystem::initSim(

this->dataPtr->joints_.resize(this->dataPtr->n_dof_);

constexpr double default_gain = 0.1;
if (!this->nh_->get_parameter_or("position_proportional_gain", this->dataPtr->position_proportional_gain_, default_gain))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An optional gain parameter. Previously it was hard-coded to 1.0.

@@ -532,13 +540,13 @@ IgnitionSystem::perform_command_mode_switch(
this->dataPtr->joints_[j].joint_control_method &=
static_cast<ControlMethod_>(VELOCITY & EFFORT);
}
if (interface_name == (this->dataPtr->joints_[j].name + "/" +
else if (interface_name == (this->dataPtr->joints_[j].name + "/" +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small efficiency improvement

@@ -627,6 +637,25 @@ hardware_interface::return_type IgnitionSystem::write(
{this->dataPtr->joints_[i].joint_effort_cmd});
}
}

// Fallback case is a velocity command of zero
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is what prevented the unactuated joints from drooping with gravity.

@@ -590,14 +599,14 @@ hardware_interface::return_type IgnitionSystem::write(
}
}

if (this->dataPtr->joints_[i].joint_control_method & POSITION) {
else if (this->dataPtr->joints_[i].joint_control_method & POSITION) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

else-if so the velocity command from this interface doesn't conflict with the one at L597

@AndyZe AndyZe changed the title Saggy controller back to main Various bug fixes Jan 31, 2023
@AndyZe AndyZe force-pushed the saggy_controller_back_to_main branch from c7bd1e6 to d0f114f Compare February 2, 2023 15:02
@AndyZe AndyZe force-pushed the saggy_controller_back_to_main branch from d0f114f to 61d51ce Compare February 2, 2023 15:19
@AndyZe
Copy link
Contributor Author

AndyZe commented Feb 2, 2023

ament_cpp_lint conflicts with one of the other linters:

/home/ros2_ws/src/gz_ros2_control/ign_ros2_control/src/ign_system.cpp:547: If an else has a brace on one side, it should have it on both [readability/braces] [5]

@AndyZe AndyZe force-pushed the saggy_controller_back_to_main branch from be428ec to c3b4d4e Compare February 2, 2023 20:22
@ahcorde ahcorde merged commit 7954683 into ros-controls:master Feb 3, 2023
@ahcorde
Copy link
Collaborator

ahcorde commented Feb 3, 2023

@AndyZe do you have any plans to include these fixes in gazebo_ros2_control ? Let me know if you don't have time and I will do it

@AndyZe AndyZe deleted the saggy_controller_back_to_main branch February 3, 2023 13:24
@AndyZe
Copy link
Contributor Author

AndyZe commented Feb 3, 2023

I don't mind making a quick PR to gazebo_ros2_control 👍

I actually wasn't aware of gazebo_ros2_control prior to this. Is that the recommended way?

@ahcorde
Copy link
Collaborator

ahcorde commented Feb 3, 2023

We need to support Gazebo classic too, the code is pretty similar, when something is added here or there I try to keep both repositories up to day. If the contributor is open to do it it's fantastic otherwise I will do it.

By the way, thank you for your contribution.

livanov93 pushed a commit to livanov93/ign_ros2_control that referenced this pull request Feb 22, 2023
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.

In ign_system.cpp, VELOCITY and POSITION command modes are conflicting
2 participants