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

Add indices to FollowJointTrajectory to store trajectory index being executed #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Levi-Armstrong
Copy link

@Levi-Armstrong Levi-Armstrong commented Jul 27, 2022

This is to address issue #65

Open Question: Should it be storing an index for each joint or just a single index for all joints? I believe it is the former.

@destogl
Copy link
Member

destogl commented Sep 15, 2022

Open Question: Should it be storing an index for each joint or just a single index for all joints? I believe it is the former.

I think it is sufficient to have one index for all joints because the controllers usually take cases about individual points (all joints) they are passing through.

What is the exact purpose of these indices? Do we need a more descriptive field name?

@Levi-Armstrong
Copy link
Author

Currently we use this for dynamic planning. So based on the environment if we are approaching a collision object we use this index to then look forward some time in the trajectory and replan a new segment and splice it in on the controller side. Another use case is if the tolerance violation is triggered during execution this index is useful for restarting the trajectory.

@bmagyar bmagyar changed the base branch from galactic-devel to master April 1, 2023 14:04
@bmagyar
Copy link
Member

bmagyar commented Apr 1, 2023

@Levi-Armstrong is this still required? Could you please document the new field please? The current name gives 0 clues to the user as to what they should be expecting those indices to correspond to and why.

@bmagyar bmagyar self-requested a review April 1, 2023 14:07
@Levi-Armstrong
Copy link
Author

@bmagyar

is this still required?

I would say yes. This is the index from the provided trajectory that the controller is executing toward. Though I don't think this needs to be a vector; do you agree?

Could you please document the new field please?

I will update the documentation.

Copy link
Contributor

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale label Mar 31, 2025
@christophfroehlich
Copy link
Contributor

@Levi-Armstrong is this still required? 🙈 I also think so, and we discussed this some time ago in a WG meeting. but I don't know if anyone reached out to you or anyone else at Southwest Research Institute

@bmagyar

is this still required?

I would say yes. This is the index from the provided trajectory that the controller is executing toward. Though I don't think this needs to be a vector; do you agree?

A scalar index should be enough right?

Could you please document the new field please?

I will update the documentation.

@github-actions github-actions bot removed the stale label Apr 4, 2025
@Levi-Armstrong
Copy link
Author

I believe it is still needed. I may have some time over the next month to make the requested changes.

@christophfroehlich
Copy link
Contributor

The changes here in the control_msgs are quick, but we have to include that in the JTC then. Do you use ROS 2, or ROS 1 as the linked PR to ros_controllers?

@Levi-Armstrong
Copy link
Author

Currently using both.

@saikishor
Copy link
Member

@Levi-Armstrong why do you need a vector of indices? Isn't an index enough?

@Levi-Armstrong
Copy link
Author

A don't think a vector is needed and only the index would be enough. I had asked a question related to this to see if anyone could think of any reason to keep the vector but not sure if I got any feedback.

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.

5 participants