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

QoS - Expose the assert_liveliness API for Publishers and Nodes #313

Merged
merged 2 commits into from
May 3, 2019
Merged

QoS - Expose the assert_liveliness API for Publishers and Nodes #313

merged 2 commits into from
May 3, 2019

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Apr 16, 2019 •
edited
Loading

Depends on ros2/rmw#171

Expose the rcl assert_liveliness APIs for Nodes and Publishers

Signed-off-by: Emerson Knapp eknapp@amazon.com

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Apr 16, 2019
@emersonknapp emersonknapp changed the title Expose the assert_liveliness API for Publishers and Nodes QoS - Expose the assert_liveliness API for Publishers and Nodes Apr 16, 2019
@nburek
Copy link

nburek commented Apr 19, 2019

@wjwwood This is ready for review. Can you please update the label? Thank you.

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 24, 2019
jacobperron
jacobperron reviewed Apr 24, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending passing CI, just a few nitpicks.

rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
@emersonknapp
Copy link
Contributor Author

Thanks for the feedback! I've addressed the comments in a pushed commit. If we're happy with how this looks - we can leave it on deck for CI for when the bundle of dependencies ros2/rmw#171 is merged.

jacobperron
jacobperron approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

we can leave it on deck for CI for when the bundle of dependencies ros2/rmw#171 is merged.

SGTM

@emersonknapp emersonknapp force-pushed the qos-5-assert-liveliness branch 2 times, most recently from fe1ba01 to dc6f263 Compare May 3, 2019 17:26
Expose the assert_liveliness API for Publishers and Nodes
32e93b0 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Contributor Author

@thomas-moulard - please run the following CI job:

@thomas-moulard
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp mentioned this pull request May 3, 2019
Merged
@emersonknapp
Copy link
Contributor Author

emersonknapp commented May 3, 2019 •
edited
Loading

@thomas-moulard I messed up and pointed that CI against the other Python PR - I have updated the existing Gist to be correct - here it is. So sorry about that

https://gist.githubusercontent.com/emersonknapp/02889df53fbb6e7b06ebda807d930d15/raw/789129bd6b869b0bbc16a5f1f2b890da87e711c3/ros2.repos

You can cancel any remaining running builds, they'll fail too.

@tfoote
Copy link
Contributor

tfoote commented May 3, 2019 •
edited
Loading

Retriggered with the updated repos file:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

sloretz
sloretz reviewed May 3, 2019
View reviewed changes
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
Use rclpy_publisher_t
fcc33d3 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@sloretz
Copy link
Contributor

sloretz commented May 3, 2019

CI (Testing just rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

@sloretz hurray!

sloretz
sloretz approved these changes May 3, 2019
View reviewed changes
rclpy/src/rclpy/_rclpy.c
return NULL;
}
} else if (PyCapsule_IsValid(pyentity, "rclpy_publisher_t")) {
rcl_publisher_t * publisher = (rcl_publisher_t *)PyCapsule_GetPointer(
Copy link
Contributor

Choose a reason for hiding this comment

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

This works because rcl_publisher_t is the first member of rclpy_publisher_t, but it would be nice to be more explicit about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't disagree - do we want to block merging for that? Or can we follow up afterwards?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll merge this one.

@sloretz
Copy link
Contributor

sloretz commented May 3, 2019

Thanks for the PR!

@sloretz sloretz merged commit 4ba8c1b into ros2:master May 3, 2019
@sloretz sloretz removed the in review Waiting for review (Kanban column) label May 3, 2019
@sloretz sloretz mentioned this pull request May 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@jacobperron jacobperron jacobperron 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.
6 participants
@emersonknapp @nburek @thomas-moulard @tfoote @sloretz @jacobperron