Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ bool computeLinkFK(moveit::core::RobotState& robot_state, const std::string& lin
* @param joint_limits: joint limits
* @return
*/
bool verifySampleJointLimits(const std::map<std::string, double>& position_last,
double verifySampleJointLimits(const std::map<std::string, double>& position_last,
const std::map<std::string, double>& velocity_last,
const std::map<std::string, double>& position_current, double duration_last,
double duration_current, const JointLimitsContainer& joint_limits);
Comment thread
sangteak601 marked this conversation as resolved.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ bool pilz_industrial_motion_planner::computeLinkFK(moveit::core::RobotState& rob
return true;
}

bool pilz_industrial_motion_planner::verifySampleJointLimits(
double pilz_industrial_motion_planner::verifySampleJointLimits(
const std::map<std::string, double>& position_last, const std::map<std::string, double>& velocity_last,
const std::map<std::string, double>& position_current, double duration_last, double duration_current,
const pilz_industrial_motion_planner::JointLimitsContainer& joint_limits)
Expand All @@ -151,55 +151,57 @@ bool pilz_industrial_motion_planner::verifySampleJointLimits(
if (duration_current <= epsilon)
{
RCLCPP_ERROR(getLogger(), "Sample duration too small, cannot compute the velocity");
return false;
return -1.0;
}

double velocity_current, acceleration_current;

double max_scaling_factor = 0.0;

Copilot AI Mar 5, 2026

Copy link

Choose a reason for hiding this comment

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

verifySampleJointLimits() initializes max_scaling_factor to 0.0, which means it can return 0 for valid samples (e.g., no motion). Downstream code is now treating the return value as a scaling factor where <= 1.0 should mean “no rescaling needed”, so returning 0 is ambiguous and (in the other generateJointTrajectory overload) can be interpreted as failure when used in a boolean context. Consider making the function’s contract explicit (e.g., return 1.0 when within limits, >1.0 when scaling is required, and <0 on verification error) and initialize max_scaling_factor accordingly.

Suggested change
double max_scaling_factor = 0.0;
double max_scaling_factor = 1.0;

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor - Would returning an optional rather than a negative value be more explicit about what can be used or not?


for (const auto& pos : position_current)
{
velocity_current = (pos.second - position_last.at(pos.first)) / duration_current;

if (!joint_limits.verifyVelocityLimit(pos.first, velocity_current))
{
RCLCPP_ERROR_STREAM(getLogger(), "Joint velocity limit of "
RCLCPP_DEBUG_STREAM(getLogger(), "Joint velocity limit of "
<< pos.first << " violated. Set the velocity scaling factor lower!"
<< " Actual joint velocity is " << velocity_current
<< ", while the limit is " << joint_limits.getLimit(pos.first).max_velocity
<< ". ");
return false;
}
max_scaling_factor = std::max(max_scaling_factor, std::fabs(velocity_current / joint_limits.getLimit(pos.first).max_velocity));

acceleration_current = (velocity_current - velocity_last.at(pos.first)) / (duration_last + duration_current) * 2;
// acceleration case
if (fabs(velocity_last.at(pos.first)) <= fabs(velocity_current))
{
if (!joint_limits.verifyAccelerationLimit(pos.first, acceleration_current))
{
RCLCPP_ERROR_STREAM(getLogger(), "Joint acceleration limit of "
RCLCPP_DEBUG_STREAM(getLogger(), "Joint acceleration limit of "
<< pos.first << " violated. Set the acceleration scaling factor lower!"
<< " Actual joint acceleration is " << acceleration_current
<< ", while the limit is "
<< joint_limits.getLimit(pos.first).max_acceleration << ". ");
return false;
}
}
// deceleration case
else
{
if (!joint_limits.verifyDecelerationLimit(pos.first, acceleration_current))
{
RCLCPP_ERROR_STREAM(getLogger(), "Joint deceleration limit of "
RCLCPP_DEBUG_STREAM(getLogger(), "Joint deceleration limit of "
<< pos.first << " violated. Set the acceleration scaling factor lower!"
<< " Actual joint deceleration is " << acceleration_current
<< ", while the limit is "
<< joint_limits.getLimit(pos.first).max_deceleration << ". ");
return false;
Comment thread
sangteak601 marked this conversation as resolved.
}
}
max_scaling_factor = std::max(max_scaling_factor, std::sqrt(std::fabs(acceleration_current / joint_limits.getLimit(pos.first).max_acceleration)));
max_scaling_factor = std::max(max_scaling_factor, std::sqrt(std::fabs(acceleration_current / joint_limits.getLimit(pos.first).max_deceleration)));
}

return true;
return max_scaling_factor;

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.

[minor]: not actionable, but "max_scaling_factor" may be a bit misleading for people used to ros/moveit terminologies, because typically "vel_scaling_factor" and "acc_scaling_factor" is used to "speed up" the trajectory, while here it is a measure of how much "slow down" the trajectory.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Any suggestions for a different name?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe required_vel_slowdown_factor? I'm assuming it's specifically a scaling on the velocity to bring both vel and acc into compliance given we sqrt the acc above and square it when reapplying below?

}

bool pilz_industrial_motion_planner::generateJointTrajectory(
Expand Down Expand Up @@ -234,6 +236,8 @@ bool pilz_industrial_motion_planner::generateJointTrajectory(
joint_velocity_last[item.first] = 0.0;
}

double max_scaling_factor = 1.0;

for (std::vector<double>::const_iterator time_iter = time_samples.begin(); time_iter != time_samples.end();
++time_iter)
{
Expand Down Expand Up @@ -261,18 +265,21 @@ bool pilz_industrial_motion_planner::generateJointTrajectory(
}

// skip the first sample with zero time from start for limits checking
if (time_iter != time_samples.begin() &&
!verifySampleJointLimits(ik_solution_last, joint_velocity_last, ik_solution, sampling_time,
duration_current_sample, joint_limits))
auto max_scaling_factor_sample = verifySampleJointLimits(ik_solution_last, joint_velocity_last, ik_solution,
sampling_time, duration_current_sample, joint_limits);
if (time_iter != time_samples.begin() && max_scaling_factor_sample < 0.0)
{
Comment thread
sangteak601 marked this conversation as resolved.
RCLCPP_ERROR_STREAM(getLogger(), "Inverse kinematics solution at "
<< *time_iter
<< "s violates the joint velocity/acceleration/deceleration limits.");
RCLCPP_ERROR_STREAM(getLogger(), "Can't verify the joint limits for the sample at " << *time_iter);
error_code.val = moveit_msgs::msg::MoveItErrorCodes::PLANNING_FAILED;
joint_trajectory.points.clear();
return false;
}

RCLCPP_INFO_STREAM(getLogger(), "The sample at " << *time_iter << " has a scaling factor of "
<< max_scaling_factor_sample << ".");
Comment thread
sangteak601 marked this conversation as resolved.

max_scaling_factor = std::max(max_scaling_factor, max_scaling_factor_sample);

// fill the point with joint values
trajectory_msgs::msg::JointTrajectoryPoint point;

Expand Down Expand Up @@ -310,6 +317,30 @@ bool pilz_industrial_motion_planner::generateJointTrajectory(
ik_solution_last = ik_solution;
}

if (max_scaling_factor > 1.5)

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.

Why 1.5?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The number is just my guess. During the test, in most cases violation is small (less than 1.2), if cartesian limits are reasonable. If it is too high, I think it would make sense to adjust limits rather than re-scale.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor - Maybe best to throw this in a constexpr at the top of the file to make it easy to spot as a "tuneable" value instead of having to dig in?

{
RCLCPP_ERROR_STREAM(getLogger(), "Velocity/acceleration limits are severely violated. The trajectory is not safe to execute.");
error_code.val = moveit_msgs::msg::MoveItErrorCodes::PLANNING_FAILED;
joint_trajectory.points.clear();
return false;
}
Comment thread
sangteak601 marked this conversation as resolved.
else if (max_scaling_factor > 1.0)
{
RCLCPP_WARN_STREAM(getLogger(), "Velocity/acceleration limits are violated. Re-scaling the trajectory with a factor of " << max_scaling_factor);
for (auto& point : joint_trajectory.points)
{
point.time_from_start = rclcpp::Duration::from_seconds(rclcpp::Duration(point.time_from_start).seconds() * max_scaling_factor);
for (auto& velocity : point.velocities)
{
velocity /= max_scaling_factor;
}
for (auto& acceleration : point.accelerations)
{
acceleration /= (max_scaling_factor * max_scaling_factor);
}
}
}

error_code.val = moveit_msgs::msg::MoveItErrorCodes::SUCCESS;
double duration_ms = (clock.now() - generation_begin).seconds() * 1000;
RCLCPP_DEBUG_STREAM(getLogger(), "Generate trajectory (N-Points: "
Expand Down Expand Up @@ -376,6 +407,8 @@ bool pilz_industrial_motion_planner::generateJointTrajectory(
// overload generateJointTrajectory(...,
// KDL::Trajectory, ...)
// TODO: refactor to avoid code duplication.
// TODO(): Re-scale velocity and acceleration instead of failing.
// It seems this overload is not used at all, so it is not critical to fix this in the current state.
Comment thread
sangteak601 marked this conversation as resolved.
RCLCPP_ERROR_STREAM(getLogger(), "Inverse kinematics solution of the "
<< i
<< "th sample violates the joint "
Expand Down