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

fix: motive auto generated names are not valid ROS identifiers: "Rigidy Body n" #8

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

cjue
Copy link
Contributor

@cjue cjue commented Aug 4, 2017

Removes spaces and other invalid characters from rigid body names streamed from Motive.

Old behavior: No TF is published if the rigid body name is invalid.

@paulbovbel
Copy link
Member

Thanks for the contribution. It's actually a little trickier than that:

First character is an alpha character ([a-z|A-Z]), tilde (~) or forward slash (/)
Subsequent characters can be alphanumeric ([0-9|a-z|A-Z]), underscores (_), or forward slashes (/)

So the behaviour is different for the first and subsequent characters, as well as underscores and slashes being legal.

@cjue
Copy link
Contributor Author

cjue commented Sep 8, 2017

I added a commit that applies the name rules for first and subsequent characters.

@paulbovbel paulbovbel closed this Sep 10, 2017
@paulbovbel paulbovbel reopened this Sep 10, 2017
@cjue
Copy link
Contributor Author

cjue commented Sep 11, 2017

The last commit fixes a small logic error.

Are you satisfied with this solution? The most pressing use-case in my mind is just the removal of spaces in the default names generated by Motive.

@paulbovbel
Copy link
Member

Thanks, I was looking at getting PR builds working before I merged this. LGTM otherwise.

@paulbovbel paulbovbel closed this Sep 26, 2017
@paulbovbel paulbovbel reopened this Sep 26, 2017
@paulbovbel paulbovbel merged commit 895989d into ros-drivers:indigo-devel Sep 26, 2017
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