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 trajectory cmd #13

Closed

Conversation

sktometometo
Copy link
Contributor

  • add /spot/trajectory service to issue a trajectory command which enables to move spot to a desired position.

@sktometometo sktometometo marked this pull request as draft November 21, 2020 06:51
@EricVoll
Copy link
Contributor

EricVoll commented Nov 24, 2020

Hi @sktometometo,
I have two questions:

  1. Wouldn't it make more sense to have the trajectory cmd be a subscribed topic rather than a service? That would be more in line with the velocity and pose commands, which are also subscribed topics. (continuity)
    2. Why did you rename '_last_motion_command ' to ' _last_velocity_command'? [Understood - Motion would not be clear since the velocity commands are also motions]

I'll test this branch on our spot in the next few days 😊

@sktometometo
Copy link
Contributor Author

sktometometo commented Nov 25, 2020

Hi @EricVoll.

Your idea sounds good. but I think there are 3 candidates of API for trajectory cmd including your suggestion and my implementation.

There are patterns for ROS Communication API.
I implemented trajectory cmd as a service because I thought it is a kind of remote procedure which issue a robot command with Boston Dynamics API.
But it is also good to make trajectory cmd as a topic or an action if treating trajectory cmd as a 'continuous data stream' like /cmd_vel or a 'discrete behavior' like move_base.
That is why I leave this PR as draft.

I think it is good to discuss this issue more.

@dniewinski
Copy link
Contributor

I think actions are the most correct here. This is similar functionality to move_base

@heuristicus
Copy link
Owner

Hi, thanks for this PR - it works well but as @dniewinski suggested I think it would work better as an action since then it is possible to call it from elsewhere and know that the action has completed.

I've done a bit of work at ori-drs@ee594aa to convert the service into an actionserver. I could make a PR into @sktometometo's branch if it would be useful to add into this PR. If you have any suggestions for improvements they are most welcome.

@EricVoll
Copy link
Contributor

I'm happy to test your branch this week and will give you some feedback if I notice anything :)

@sktometometo
Copy link
Contributor Author

hi @heuristicus , thank you for your work of action server implementation!

I also think action server is better than service for this case, and I am planning to use your version.
So could you make a PR with your patch, or can I make a PR with your patch? (I have already make a branch with this branch, your patch, + suttle changes and rebased on origin/master sktometometo#2 ).

@heuristicus
Copy link
Owner

Hi @sktometometo, thanks for your initial efforts on this, you added a lot of useful things. I think since you've already made a branch it would make sense for you to make the PR with that branch.

@sktometometo
Copy link
Contributor Author

@heuristicus thank you for your kind reply!. I have made PR with you patch

@sktometometo sktometometo deleted the add-trajectory-cmd branch September 17, 2021 10:20
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.

None yet

4 participants