Skip to content

Conversation

@my-rice
Copy link
Contributor

@my-rice my-rice commented Jun 26, 2025

This PR introduces joint trajectory messages with task space wrench as described in issue #284. In particular, the following message definitions are added:

  1. JointWrenchTrajectory.msg
  2. JointWrenchTrajectoryPoint.msg

In addition to these messages, the corresponding action definition, FollowJointWrenchTrajectory.action, has also been created.

To avoid repeating code, I reused trajectory_msgs/JointTrajectoryPoint where possible. I am not completely sure if this was the best approach, so I am open to feedback on that.

Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Thanks the addition. I have some minor comments

@my-rice
Copy link
Contributor Author

my-rice commented Jun 26, 2025

@christophfroehlich I implemented all the requested changes.

Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 6 to 7
string wrench_frame
geometry_msgs/Wrench wrench
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the WrenchStamped here instead?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string wrench_frame
geometry_msgs/Wrench wrench
geometry_msgs/WrenchStamped wrench

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use WrenchStamped here instead of Wrench + wrench_frame, the message would end up with two headers, each containing its own timestamp (one inside the JointTrajectoryPoint and another inside the WrenchStamped field). Generally, I’d expect users of this message to assume that the wrench and the JointTrajectoryPoint are synchronized, meaning they share the same timestamp.

Copy link
Member

@saikishor saikishor Jul 14, 2025

Choose a reason for hiding this comment

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

I agree with you, but I'm not sure about separating it as a wrench_frame_id. It should belong within the same context in my opinion. I understand your point on using stamped, but I'm not convinced with the current variable naming and usage.

Any other proposals? Or thoughts? @my-rice @christophfroehlich

Copy link
Member

Choose a reason for hiding this comment

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

I understand your point, but would be happy with the current approach as well.

To avoid duplicate header, we could add another layer with a WrenchFramed message?

Copy link
Member

Choose a reason for hiding this comment

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

Yes!
Let's discuss in the PMC Meeting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I wrote an item in the 'New Business' section of the agenda. Feel free to revise it or move it to a different section.

Copy link
Member

Choose a reason for hiding this comment

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

WrenchFramed

Copy link
Contributor Author

@my-rice my-rice Jul 17, 2025

Choose a reason for hiding this comment

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

@saikishor as we discussed in yesterday’s PMC meeting, the proposed solution is to introduce the WrenchFramed message. This would allow us to encapsulate both the wrench_frame_id and the wrench itself in a structured layer.
I have two questions about this:

  • Should I introduce the WrenchFramed message as part of this PR, or submit it in a separate one?

  • Should WrenchFramed be added to the control_msgs package or another one?

Copy link
Member

Choose a reason for hiding this comment

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

Please add it in the same PR in this repository.

@my-rice
Copy link
Contributor Author

my-rice commented Jul 14, 2025

Hey @saikishor, have you had a chance to review the updates on this PR?

@saikishor
Copy link
Member

Hey @saikishor, have you had a chance to review the updates on this PR?

Hey @my-rice
I'm extremely sorry, this got out of my inbox last week. Thanks for the ping.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

I left some comments. Please take a look

@my-rice
Copy link
Contributor Author

my-rice commented Jul 15, 2025

I'm extremely sorry, this got out of my inbox last week. Thanks for the ping.

@saikishor No worries at all. Thanks for getting back to me.

I left some comments. Please take a look

I believe I have responded to all of your comments on the PR.

saikishor
saikishor previously approved these changes Jul 22, 2025
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Rest looks good to me :)

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

@christophfroehlich christophfroehlich merged commit 8780034 into ros-controls:master Jul 22, 2025
10 checks passed
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.

4 participants