Yaskawa-Global / motoros2

ROS 2 (rcl, rclc & micro-ROS) node for MotoPlus-compatible Yaskawa Motoman robot controllers
96 stars 18 forks source link

Improve JTA goal validation to also check for duplicate joint names #28

Closed gavanderhoorn closed 11 months ago

gavanderhoorn commented 1 year ago

As per subject.

The current implementation of the FollowJointTrajectory action goal validation does not check whether incoming JointTrajectorys have duplicated joint names in the joint_names field.

This could allow malformed trajectories to be considered OK to execute, while they should be rejected.

gavanderhoorn commented 1 year ago

In order to not delay releasing 0.1.1, I propose we move this to the 0.1.2 milestone.

@ted-miller: thoughts?

ted-miller commented 1 year ago

Yeah, I'm fine with bumping this out.

Additionally, this type of feature would add more overhead to the message processing. I've been getting a lot of complaints lately about latency. So we should decide if this is really important. Personally, I don't think it's super needed. I don't envision a motion-planner outputting a joint that contradicts itself.

gavanderhoorn commented 1 year ago

I've been getting a lot of complaints lately about latency.

in MotoROS2?

I don't envision a motion-planner outputting a joint that contradicts itself.

it's really not that difficult to misconfigure things.

If one additional loop over 6 const char*s is where things become too slow, we probably have bigger problems.

ted-miller commented 1 year ago

in MotoROS2?

No. Just in general. But we already have an issue with the publish rates in MotoROS2. We don't know where that bottleneck is yet.

If one additional loop over 6 const char*s is where things become too slow, we probably have bigger problems.

Well it'd be more than one loop... but your point is fair.

it's really not that difficult to misconfigure things.

Ok. We can keep it. It shouldn't be hard to implement. I'm just pretty bogged down until FabTech, otherwise I'd have done it already.

gavanderhoorn commented 11 months ago

@ted-miller: would this be something we include in 0.1.2? I'm thinking we should not delay releasing it too long, as it includes fixes for things like #144 and #55 which would be nice to release.

ted-miller commented 11 months ago

Fix is in #162