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.

Sign up for GitHub

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

Jump to bottom

waitset -> wait_set #152

Merged
merged 2 commits into from
Nov 27, 2017
Merged

waitset -> wait_set #152

merged 2 commits into from
Nov 27, 2017

Conversation

mikaelarguedas
Copy link
Member

connects to ros2/rmw#131

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Nov 22, 2017
sloretz
sloretz reviewed Nov 27, 2017
View reviewed changes
rclpy/src/rclpy/_rclpy.c
@@ -1668,11 +1668,11 @@ rclpy_get_zero_initialized_wait_set(PyObject * Py_UNUSED(self), PyObject * Py_UN
return pywait_set;
}

/// Initialize a waitset
/// Initialize a wait set
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all of this code is removed in #140. Would you mind reviewing that instead? This is assuming #140 gets in before feature freeze of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that this PR touches only documentation don't feel strongly about merging it or holding it in favor of #140, I'll go ahead and merge all the other PRs as they are affecting rmw API.

I'll try to review #140 today, but as its a >1k lines it'll take a while

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should merge this PR.

How does this sound? With the number of people on vacation this week it seems unlikely the wait_for_service feature will make it in. Since most of #140 is refactoring for that, it won't need to be in the dec 8th release. Separately I'll split out the executor bug fixes from #140 to make sure those get in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll merge this soon, wrapping up third party rmw PRs.

With the number of people on vacation this week it seems unlikely the wait_for_service feature will make it in.

We'll see, I think there is a lot of value in getting some version of wait_for_service in before the release. We also have several PRs blocked until we can use that feature in tests.

Separately I'll split out the executor bug fixes from #140 to make sure those get in.

Yeah thats always a good idea to ease review. thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@sloretz you would know best if this is a reasonable proposal, but it seems there was a state at which #127 was approved, and then work continued in order to address some executor bugs. Perhaps what has been approved already can be merged and reviewers can focus on what's on top?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhood I've made the state more complicated than it needs to be. The version of #127 that was approved has a couple problems:

  • It adds overhead that makes the 1kHz timer test fail 100% of the time on aarch64
  • A separate GraphListener wouldn't make sense once a wait_for_service_async is added

#135 was split from #127 to implement wait_for_service in a way that makes sense with wait_for_service_async. #140 was split from #135 to make the PR smaller. #140 has bug fixes found while reviewing #127 plus the performance improvements needed to get the 1kHz timer test to pass.

@mikaelarguedas mikaelarguedas merged commit 74b5f18 into master Nov 27, 2017
@mikaelarguedas mikaelarguedas deleted the wait_set_two_words branch November 27, 2017 21:30
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhood dhood dhood approved these changes

@sloretz sloretz sloretz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikaelarguedas @sloretz @dhood