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

Implemented a SetLocalPosition Service #259

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

Conversation

MarvinKlemp
Copy link

No description provided.

@MarvinKlemp
Copy link
Author

Looks like I didn't use the latest version locally.
I'll fix merge conflicts later

@SijmenHuizenga
Copy link
Member

This looks great! Only thing left to do is add some documentation. After you resolve the conflicts I will test it 🚀

…verless-Simulator into feature/set_local_position_handler
@MarvinKlemp
Copy link
Author

MarvinKlemp commented Jan 14, 2021

Should all services be moved to fsds_msgs?

as Reset.srv was moved while the SetGPSPosition.srv and SetLocalPosition.srv definitions are still in fsds_ros_bridge

@SijmenHuizenga
Copy link
Member

Should all services be moved to fsds_msgs?

Yes! I think that would be the right way forward.

@SijmenHuizenga
Copy link
Member

Tested it, works great!

I added a commit that removes the vehicle_name from SetLocalPosition.srv as FSDS only supports a single vehicle anyway. I also added the service file to the CMakeLists.txt because without it compilation failed.

Last thing to do is add a note in the docs. Let's also mention explicitly that setting the local position does not reset the velocity and also does not reset the command_controll.

@MarvinKlemp
Copy link
Author

Sorry, I didn't really have a lot of time in the past days.

Should all services be moved to fsds_msgs?

Yes! I think that would be the right way forward.

Do you want this to be done in this PR? Or in another PR after this one.

@SijmenHuizenga
Copy link
Member

@MarvinKlemp We can do it in a separate pr 😄

After you add some docs I we can merge 🚀

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.

2 participants