Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions joint_limits/include/joint_limits/joint_saturation_limiter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ class JointSaturationLimiter : public JointLimiterInterface<JointLimitsStateData
bool on_configure(const JointLimitsStateDataType & current_joint_states) override
{
prev_command_ = current_joint_states;
const auto num_joints = this->number_of_joints_;
prev_dt_seconds_ = 0.0;
effective_max_acc_.assign(num_joints, 0.0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we really need here additional variables? Why? If this is just calculated, we don't need to keep the values. And if we are just storing them locally within a method, this should be on a heap and should not initialize any memory.

This makes sense as we need to later adjust all joint togeather.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I decided to remove the dt recalculation logic as in RT system dt should be constant. Now will remove these additional variables and update the on_enforce to update the max acc/dec when implicit vel exceeds the max vel.

Pls LMK if is approach is good?

effective_max_dec_.assign(num_joints, 0.0);

return true;
}

Expand Down Expand Up @@ -91,6 +96,11 @@ class JointSaturationLimiter : public JointLimiterInterface<JointLimitsStateData
rclcpp::Clock::SharedPtr clock_;
JointLimitsStateDataType prev_command_;
std::mutex mutex_;

private:
std::vector<double> effective_max_acc_;
std::vector<double> effective_max_dec_;
double prev_dt_seconds_;
};

template <typename JointLimitsStateDataType>
Expand Down
62 changes: 53 additions & 9 deletions joint_limits/src/joint_saturation_limiter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,53 @@ bool JointSaturationLimiter<trajectory_msgs::msg::JointTrajectoryPoint>::on_enfo
return false;
}

// TODO(gwalck) compute if the max are not implicitly violated with the given dt
// e.g. for max vel 2.0 and max acc 5.0, with dt >0.4
// velocity max is implicitly already violated due to max_acc * dt > 2.0
// if the dt is changed then recalculate the max_acceleration/max_deceleration if implicit vel

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what does it mean that dt is changend?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this also a problem for the velocity too?

// exceeds max vel
if (std::fabs(dt_seconds - prev_dt_seconds_) > 1e-9)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a chance that the dt won't be changed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Earlier, I was thinking as the enforce fn has dt argument, then the user can change it. But now I think for the real-time control system, dt should be constant. I will remove this comparison of dt and only update the params max_acceleration and max_deceleration to effective acc/dec when impl vel exceeds the max acc/dec.
Pls LMK if this approach is good?

{
for (size_t i = 0; i < number_of_joints_; ++i)
{
if (joint_limits_[i].has_velocity_limits && joint_limits_[i].has_acceleration_limits)
{
const double implicit_vel = joint_limits_[i].max_acceleration * dt_seconds;
if (implicit_vel > joint_limits_[i].max_velocity)
{
RCLCPP_WARN_STREAM_THROTTLE(
node_logging_itf_->get_logger(), *clock_, ROS_LOG_THROTTLE_PERIOD,
"Joint '" << joint_names_[i] << "': dt (" << dt_seconds
<< ") is too large for max_acceleration ("
<< joint_limits_[i].max_acceleration << ") and max_velocity ("
<< joint_limits_[i].max_velocity << "); max_acc * dt = " << implicit_vel
<< " exceeds max_velocity");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please change this to non STREAM version please. STREAM versions are not realtime safe

effective_max_acc_[i] = joint_limits_[i].max_velocity / dt_seconds;
}
else
{
effective_max_acc_[i] = joint_limits_[i].max_acceleration;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this might cause a problem if done only for one joint. We should scale down then all other joints appropriately...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we are for now not scaling down also in other cases, then we don't have to do now, but in the followup PR, as this is important to not change robot trajectories.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I am thinking of calculating the max_reduction_factor = max_vel/implicit_vel, for all the joints and then use the min max_reduction_factor to scale down all the joints acc/dec.

Pls LMK if this approach is good?

}
}
if (joint_limits_[i].has_velocity_limits && joint_limits_[i].has_deceleration_limits)
{
const double implicit_vel = joint_limits_[i].max_deceleration * dt_seconds;
if (implicit_vel > joint_limits_[i].max_velocity)
{
RCLCPP_WARN_STREAM_THROTTLE(
node_logging_itf_->get_logger(), *clock_, ROS_LOG_THROTTLE_PERIOD,
"Joint '" << joint_names_[i] << "': dt (" << dt_seconds
<< ") is too large for max_deceleration ("
<< joint_limits_[i].max_deceleration << ") and max_velocity ("
<< joint_limits_[i].max_velocity << "); max_dec * dt = " << implicit_vel
<< " exceeds max_velocity");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here non STREAM version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the code for non stream version

effective_max_dec_[i] = joint_limits_[i].max_velocity / dt_seconds;
}
else
{
effective_max_dec_[i] = joint_limits_[i].max_deceleration;
}
}
}
prev_dt_seconds_ = dt_seconds;
}

// check for required inputs combination
const bool has_desired_position = (desired_joint_states.positions.size() == number_of_joints_);
Expand Down Expand Up @@ -185,17 +229,17 @@ bool JointSaturationLimiter<trajectory_msgs::msg::JointTrajectoryPoint>::on_enfo
// limit deceleration
if (joint_limits_[index].has_deceleration_limits)
{
limit_applied = apply_acc_or_dec_limit(
joint_limits_[index].max_deceleration, desired_acc, limited_jnts_dec);
limit_applied =
apply_acc_or_dec_limit(effective_max_dec_[index], desired_acc, limited_jnts_dec);
deceleration_limit_applied = true;
}
}

// limit acceleration (fallback to acceleration if no deceleration limits)
if (joint_limits_[index].has_acceleration_limits && !deceleration_limit_applied)
{
limit_applied = apply_acc_or_dec_limit(
joint_limits_[index].max_acceleration, desired_acc, limited_jnts_acc);
limit_applied =
apply_acc_or_dec_limit(effective_max_acc_[index], desired_acc, limited_jnts_acc);
}

if (limit_applied)
Expand Down Expand Up @@ -253,11 +297,11 @@ bool JointSaturationLimiter<trajectory_msgs::msg::JointTrajectoryPoint>::on_enfo
double stopping_deccel = std::fabs(desired_vel[index] / dt_seconds);
if (joint_limits_[index].has_deceleration_limits)
{
stopping_deccel = joint_limits_[index].max_deceleration;
stopping_deccel = effective_max_dec_[index];
}
else if (joint_limits_[index].has_acceleration_limits)
{
stopping_deccel = joint_limits_[index].max_acceleration;
stopping_deccel = effective_max_acc_[index];
}

double stopping_distance =
Expand Down
33 changes: 31 additions & 2 deletions joint_limits/test/test_joint_saturation_limiter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ TEST_F(JointSaturationLimiterTest, when_within_limits_expect_no_limits_applied_w
// within limits
desired_joint_states_.positions[0] = 1.0;
desired_joint_states_.velocities[0] = 1.5; // valid pos derivative as well
desired_joint_states_.accelerations[0] = 2.9; // valid pos derivative as well
desired_joint_states_.accelerations[0] = 2.0; // valid pos derivative as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @destogl, when recalculating the max acc/dec if impl vel exceed max vel, this test fails. So I have updated the test. I want to ask if it is fine or should I do something else?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pleaes create a new test for this and call it "when_derivative_too_high_expect_lower_effective_limits".

Also make the exact values checks.

ASSERT_FALSE(joint_limiter_->enforce(current_joint_states_, desired_joint_states_, period));

// check if no limits applied
CHECK_STATE_SINGLE_JOINT(
desired_joint_states_, 0,
1.0, // pos unchanged
1.5, // vel unchanged
2.9 // acc = vel / 1.0
2.0 // acc = vel / 1.0
);
}
}
Expand Down Expand Up @@ -517,6 +517,35 @@ TEST_F(JointSaturationLimiterTest, when_deceleration_exceeded_with_no_maxdec_exp
}
}

TEST_F(JointSaturationLimiterTest, when_implicit_vel_exceeds_max_vel_expect_vel_enforced)
{
SetupNode("joint_saturation_limiter_nodeclimit");
Load();

if (joint_limiter_)
{
Init();
Configure();

rclcpp::Duration period(0, 500000000); // 0.5 seconds
// Joint limits: max_vel = 2.0, max_acc = 5.0, no decel limits
// implicit_vel = max_acc * dt = 5.0 * 0.5 = 2.5 > max_vel (2.0)
// effective_max_acc = max_vel / dt = 2.0 / 0.5 = 4.0

current_joint_states_.positions[0] = 0.0;
current_joint_states_.velocities[0] = -0.5;
desired_joint_states_.positions.clear();
desired_joint_states_.velocities[0] = 3.0; // exceeds max_vel -> clamped to 2.0
// After vel clamping: desired_acc = (2.0 - (-0.5)) / 0.5 = 5.0 > effective_max_acc (4.0)
ASSERT_TRUE(joint_limiter_->enforce(current_joint_states_, desired_joint_states_, period));

// Acc limited to effective_max_acc = 4.0 (not raw max_acc = 5.0)
// Vel recomputed: -0.5 + 4.0 * 0.5 = 1.5
ASSERT_NEAR(desired_joint_states_.velocities[0], 1.5, COMMON_THRESHOLD);
ASSERT_NEAR(desired_joint_states_.accelerations[0], 4.0, COMMON_THRESHOLD);
}
}

int main(int argc, char ** argv)
{
::testing::InitGoogleMock(&argc, argv);
Expand Down
Loading