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 tf2::TimePoint <-> builtin_interfaces::msg::Time conversions #2

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

dhood
Copy link
Member

@dhood dhood commented Mar 22, 2017

Connects to ros2/navigation#5
replacement for ros/geometry2#213

@dhood dhood added the in progress Actively being worked on (Kanban column) label Mar 22, 2017
@clalancette
Copy link
Contributor

This looks good to me, and should help out my other PR. I'm going to merge it, thanks!

@clalancette clalancette merged commit 1c4d964 into ros2 Mar 23, 2017
@clalancette clalancette removed the in progress Actively being worked on (Kanban column) label Mar 23, 2017
@clalancette clalancette deleted the builtin_interfaces_conv branch March 23, 2017 13:33
@dhood
Copy link
Member Author

dhood commented Mar 23, 2017

This was mostly opened for visibility so we didn't end up duplicating work, it wasn't necessarily ready for merge. Specifically I hadn't run CI on this to check that it works on windows (as @mikaelarguedas reminded me, this package is in the "core" ros2 repos file so it's important that it works on all platforms even if the changes were made for turtlebot demo purposes).

I'll leave it as is for now and revert if I find any issues when testing, but in the future could you please not merge PRs without CI? We also typically leave it to the opener to decide when a PR is to be merged as they usually have the most context on its state (but that's not a hard rule). Thanks in advance 😄

@clalancette
Copy link
Contributor

Oops, sorry about that. I wasn't quite sure about that. I'll definitely do that in the future.

@mikaelarguedas
Copy link
Member

We also usually squash and merge the PRs. Review them only when they're marked as ready for review and don't merge them without at least the agreement of the opener.

Thanks

@dhood
Copy link
Member Author

dhood commented Mar 23, 2017

update: this builds fine on windows. the "dummy robot demo" ros2/demos#118 seems to be publishing static transforms fine on windows (from tf echo inspection; I have issues with rviz on my machine). so this PR seems fine to stay merged

@mikaelarguedas
Copy link
Member

Thanks for testing! I guess ros2/ros2#324 should be merged then so that ci uses this repo

@dhood
Copy link
Member Author

dhood commented Mar 23, 2017

windows CI job was Build Status and then tested manually.

@mikaelarguedas
Copy link
Member

I think that this job doesn't include these commits does it ?

@dhood
Copy link
Member Author

dhood commented Mar 23, 2017

yeah, you're right. as you can see I thought passing geometry2-fork as the branch would be sufficient but it doesn't apply to ros2/ros2. I'll keep testing then!

@dhood
Copy link
Member Author

dhood commented Mar 24, 2017

Real CI now:
linux Build Status
OSX Build Status
windows Build Status

and I have tested that the dummy robot demo works fine on windows. so I again conclude (for real this time) that this PR is good to stay merged

@clalancette
Copy link
Contributor

Thanks Deanna, I appreciate it. Sorry for the breach in protocol; I'll do better for next time.

@dhood
Copy link
Member Author

dhood commented Mar 24, 2017

no worries! 😄

@dhood dhood mentioned this pull request Apr 11, 2017
clalancette pushed a commit that referenced this pull request Oct 26, 2018
* cherry pick over to ros2

* adding new clock logic
JacobJBeck pushed a commit to JacobJBeck/geometry2 that referenced this pull request Dec 8, 2018
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

3 participants