Skip to content

Support publishing loaned messages in LifecyclePublisher#2159

Merged
clalancette merged 5 commits intoros2:rollingfrom
michaelbabenko:lifecycle_loanded_publish
Apr 14, 2023
Merged

Support publishing loaned messages in LifecyclePublisher#2159
clalancette merged 5 commits intoros2:rollingfrom
michaelbabenko:lifecycle_loanded_publish

Conversation

@michaelbabenko
Copy link
Copy Markdown
Contributor

@michaelbabenko michaelbabenko commented Apr 5, 2023

Problem

Right now LifecyclePublisher doesn't support publishing LoanedMessages.

Solution

Override publish for loaned message, implementation is the same as in other overloads, check if publisher is activated and if so, pass message to base publisher

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 5, 2023

thanks for opening PR! there is build failure https://build.ros2.org/job/Rpr__rclcpp__ubuntu_jammy_amd64/746/console, can you address it?

fujitatomoya
fujitatomoya reviewed Apr 5, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

it would be better to add unit test in test_lifecycle_publisher.cpp to make sure if loaned messae can be published with lifecycle states.

@michaelbabenko
Support loaned messages in LifecyclePublisher
6a8f96d 
Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@michaelbabenko
add tests. fix cpplint
e9fbfc6 
Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@michaelbabenko
spaces instead of tabs
723e147 
Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@michaelbabenko
uncrustify test fix
a2b28be 
Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@michaelbabenko michaelbabenko force-pushed the lifecycle_loanded_publish branch from bb98e3f to a2b28be Compare April 9, 2023 17:42
@michaelbabenko
Copy link
Copy Markdown
Contributor Author

michaelbabenko commented Apr 9, 2023

Hey, @fujitatomoya, thanks for taking a look at this!

I don't think the build failure is related to my changes, since last merged PR has the same issue.

Added tests.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 10, 2023

I don't think the build failure is related to my changes, since last merged PR has the same issue.

Yeah, right now the Rpr jobs aren't going to work (we expect to have that fixed this week). We have to rely on the regular CI jobs from https://ci.ros2.org .

fujitatomoya
fujitatomoya approved these changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 13, 2023

CI:

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

@michaelbabenko
Copy link
Copy Markdown
Contributor Author

michaelbabenko commented Apr 13, 2023

@fujitatomoya can you help me figure out the CI failure, please?
I only see some java exceptions, which don't look relevant to the PR. I couldn't find any errors related to lifecycle publishers/loaned messages.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 13, 2023

something goes wrong during building cyclonedds, https://ci.ros2.org/job/ci_linux/18545/console, could be temporary. I will restart the CI.

CI:

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 14, 2023

CI unstable failures are not related to this PR.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 14, 2023

@iuhilnehc-ynos @Barry-Xu-2018 can i request another review on this?

@Barry-Xu-2018
Copy link
Copy Markdown
Collaborator

Barry-Xu-2018 commented Apr 14, 2023

LGTM

@michaelbabenko
Copy link
Copy Markdown
Contributor Author

michaelbabenko commented Apr 14, 2023

@fujitatomoya Assuming I'm good to merge with 3 approves, how do I merge/proceed if there's a build failure?

x Rpr__rclcpp__ubuntu_jammy_amd64 — Build finished.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

CI unstable failures are not related to this PR.

Hm, but we do not see these failures on the nightlies: https://ci.ros2.org/view/nightly/job/nightly_linux_release/2616/

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

@ros-pull-request-builder retest this please

@michaelbabenko
Copy link
Copy Markdown
Contributor Author

michaelbabenko commented Apr 14, 2023

@clalancette yay, it passes now, thanks!

This PR is ready to be merged, then.

Only those with write access to this repository can merge pull requests.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

This PR is ready to be merged, then.

Sorry, I don't think so. There are failures in the jobs in #2159 (comment) that we don't see elsewhere.

@michaelbabenko
Copy link
Copy Markdown
Contributor Author

michaelbabenko commented Apr 14, 2023

ah, ok, I though those were unrelated:

CI unstable failures are not related to this PR.

I don't see anything related to my changes in the CI logs, is there any way I can replicate the issue locally?

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

I don't see anything related to my changes in the CI logs, is there any way I can replicate the issue locally?

Actually, it might be enough for you to rebase this PR onto the latest rclcpp code. Then we can run CI again and see if the issues are fixed.

@michaelbabenko
Merge branch 'rolling' into lifecycle_loanded_publish
a908e0f 
Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Apr 14, 2023

CI:

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Apr 14, 2023

@clalancette thanks for restarting CI!

@clalancette clalancette merged commit 6ffd54d into ros2:rolling Apr 14, 2023
michaelbabenko added a commit to michaelbabenko/rclcpp that referenced this pull request Apr 15, 2023
@michaelbabenko
Support publishing loaned messages in LifecyclePublisher (ros2#2159)
1f8dc48 
* Support loaned messages in LifecyclePublisher

Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
@michaelbabenko michaelbabenko mentioned this pull request Apr 15, 2023
Closed
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rclcpp that referenced this pull request Jan 12, 2024
@michaelbabenko @Barry-Xu-2018
Support publishing loaned messages in LifecyclePublisher (ros2#2159)
dfdb503 
* Support loaned messages in LifecyclePublisher

Signed-off-by: Michael Babenko <mbabenko@wayve.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@michaelbabenko @fujitatomoya @clalancette @Barry-Xu-2018 @iuhilnehc-ynos