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

Update documentation to correct default publishing mode #3778

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

grizzi
Copy link
Contributor

@grizzi grizzi commented Jul 12, 2023

I checked both the documentation and the source code. The documentation shall be updated to reference the default publishing mode.

@ros-discourse
Copy link

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

https://discourse.ros.org/t/rmw-fast-dds-publication-mode-sync-vs-async-and-how-to-change-it/17153/3

Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

This LGTM, @audrow you changed the behaviour in ros2/rmw_fastrtps#571

Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

This change would need to be forward ported to Iron & Rolling

@@ -43,7 +43,6 @@ Mixing synchronous and asynchronous publications in the same node

In this first example, a node with two publishers, one of them with synchronous publication mode and the other one with asynchronous publication mode, will be created.

``rmw_fastrtps`` uses asynchronous publication mode by default.
When the publisher invokes the write operation, the data is copied into a queue,
Copy link

Choose a reason for hiding this comment

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

Suggested change
When the publisher invokes the write operation, the data is copied into a queue,
One the one hand, on the asynchronous publication mode, when the publisher invokes the write operation, the data is copied into a queue,

Copy link
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 with @EduPonz 's comment.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for the fixup. You are right that we switched to synchronous mode by default in Humble and since.

I can't make this suggestion inline due to GitHub limitations, but I think we should restructure this a little more. In particular, I think we should swap the order of the two paragraphs, so that the paragraph talking about synchronous mode comes first. I think that makes the most sense, since that is the default. The text will then need some light editing to have it make more sense, but I think that will flow better overall.

@grizzi
Copy link
Contributor Author

grizzi commented Jul 12, 2023

I agree with @clalancette. I have refactored the paragraph and changed a bit the phrasing.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thank you.

@clalancette clalancette merged commit 87b1737 into ros2:humble Jul 12, 2023
@clalancette
Copy link
Contributor

@Mergifyio backport rolling iron

@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

backport rolling iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 12, 2023
* Update documentation to correct default publishing mode

(cherry picked from commit 87b1737)
mergify bot pushed a commit that referenced this pull request Jul 12, 2023
* Update documentation to correct default publishing mode

(cherry picked from commit 87b1737)
clalancette pushed a commit that referenced this pull request Jul 12, 2023
* Update documentation to correct default publishing mode

(cherry picked from commit 87b1737)

Co-authored-by: Giuseppe Rizzi <[email protected]>
clalancette pushed a commit that referenced this pull request Jul 12, 2023
* Update documentation to correct default publishing mode

(cherry picked from commit 87b1737)

Co-authored-by: Giuseppe Rizzi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants