Skip to content

Lifecycle support 2#67

Merged
mjcarroll merged 10 commits intoros:ros2from
SteveMacenski:lifecycle_support_2
Sep 1, 2020
Merged

Lifecycle support 2#67
mjcarroll merged 10 commits intoros:ros2from
SteveMacenski:lifecycle_support_2

Conversation

@SteveMacenski
Copy link
Copy Markdown
Member

@SteveMacenski SteveMacenski commented Jul 29, 2020 •
edited
Loading

Supersedes #64 to:

  • Enable lifecycle nodes in ROS2
  • Fix ROS2 porting issues
  • Fix ROS2 WaitUntilXYZ() functions with spins on nodes already spinning (requirement for this to work)
  • Tested to be actualy working now & integrated into Nav2
  • No reduced capability
  • Removes a number of crashing problems introduced in porting

This was referenced Jul 29, 2020
Closed
Merged
naiveHobo
naiveHobo reviewed Jul 30, 2020
View reviewed changes
@SteveMacenski
Copy link
Copy Markdown
Member Author

SteveMacenski commented Jul 30, 2020 •
edited
Loading

Whoops, see the commit f72af40, I didn't push it to remote, I had already done that :-)

@SteveMacenski
Copy link
Copy Markdown
Member Author

SteveMacenski commented Aug 6, 2020

Pinging @mjcarroll as listed maintainer, can we get a review?

This work has been merged in and used on nav2 for about a week now without any issues. I think this is safe for a review for inclusion

@SteveMacenski SteveMacenski mentioned this pull request Aug 17, 2020
Closed
@SteveMacenski
Copy link
Copy Markdown
Member Author

SteveMacenski commented Aug 20, 2020

Pinging @mjcarroll

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Aug 20, 2020

Pong. I'm reviewing.

mjcarroll
mjcarroll requested changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Overall, looks good, one comment about DRY and a question about a previous resolution.

@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Aug 23, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-08-20/16063/1

@SteveMacenski
Copy link
Copy Markdown
Member Author

SteveMacenski commented Sep 1, 2020

@mjcarroll pinging again

@mjcarroll mjcarroll merged commit 99e8e63 into ros:ros2 Sep 1, 2020
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Sep 1, 2020

@SteveMacenski would you like this to be released into rolling?

@SteveMacenski
Copy link
Copy Markdown
Member Author

SteveMacenski commented Sep 2, 2020 •
edited
Loading

I don't require it, but might be a good idea. If nothing else, having it in Foxy would be our immediate needs. Right now, we don't use Rolling in Nav2 (frankly, the emails from occasional failures in just image_pipeline is prohibitive for me to support rolling releases.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

+1 more reviewer

@naiveHobo naiveHobo naiveHobo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SteveMacenski @mjcarroll @ros-discourse @naiveHobo