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

Added support for LifecycleNode to TransformBroadcaster and `Stat… #79

Closed
wants to merge 1 commit into from

Conversation

Myzhar
Copy link

@Myzhar Myzhar commented Nov 5, 2018

…icTransformBroadcaster`

Tried to replicate the "latch" behaviour for /tf_static in StaticTransformBroadcaster

…icTransformBroadcaster`

Tried to replicate the "latch" behaviour for `/tf_static` in `StaticTransformBroadcaster`
@tfoote tfoote added the in review Waiting for review (Kanban column) label Nov 5, 2018
@Myzhar
Copy link
Author

Myzhar commented Nov 5, 2018

A possible solution for #70

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please separate your pull request into separate logical items for review. The lifecycle support and latching behavior are completely separate issues and would be easier to review separately. Best would be separate pull requests, at minimum they should be two different commits in this PR.

Also please provide more documentation of what you're doing and how. "Tried to replicate the "latch" behaviour for /tf_static in StaticTransformBroadcaster" is pretty vague and offhand. Do you think that this is good enough to merge or is it a prototype that needs iteration? I would also like to see some unit tests that validate the new code paths that are being added so we can make sure that there's not a regression in the future and they actually do what we want it to do.

There's also a lot of unnecessary whitespace changes clutter up the diff. And you appear to have reindented the code. The combination of these makes the diff much harder to review and will break any other outstanding branches or pull requests unnecessarily. To that end please roll those changes back.

@tfoote tfoote added in progress Actively being worked on (Kanban column) more-information-needed Further information is required and removed in review Waiting for review (Kanban column) labels Nov 22, 2018
@cottsay
Copy link
Member

cottsay commented Feb 14, 2019

@Myzhar, it looks like there hasn't been any activity on this PR for a while now. Are you able to iterate based on the provided feedback?

@Myzhar
Copy link
Author

Myzhar commented Feb 14, 2019

@cottsay I have not too much time now. If there is not anyone that can work on this, I can try, but I cannot guarantee... let me know

@Karsten1987
Copy link
Contributor

@Myzhar Can this be closed? I believe with the latest change in #108 this should be resolved.

@Myzhar
Copy link
Author

Myzhar commented May 21, 2019

Amazing. I can't wait to test the new ROS2 release.
I'm sorry I have no time to test the preview release.

@Myzhar Myzhar closed this May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Actively being worked on (Kanban column) more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants