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

DDS refactor #21

Merged
merged 5 commits into from
Aug 5, 2021
Merged

DDS refactor #21

merged 5 commits into from
Aug 5, 2021

Conversation

jparisu
Copy link
Contributor

@jparisu jparisu commented Jul 30, 2021

Signed-off-by: jparisu javierparis@eprosima.com

Signed-off-by: jparisu <javierparis@eprosima.com>
@richiprosima
Copy link

Build status: Build Status

Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LAN_settings_options.png image does not have the TCP option selected. This image is shown both in the First steps section as in the TCP one. I think that being used in the TCP section the TCP checkbox should appear selected.

Also, I think we should clarify that if no transport is selected, the builtin_transports are enabled (that corresponds to UDP).

docs/examples/tcp_LAN_WAN_transport.rst Show resolved Hide resolved
@@ -66,7 +66,10 @@ The following image shows server and client settings:
* On the server side, the *WAN IP* field contains the server's router IP address, i.e. i.e. the Router S IP address
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* On the server side, the *WAN IP* field contains the server's router IP address, i.e. i.e. the Router S IP address
* On the server side, the *WAN IP* field contains the server's router IP address, i.e. the Router S IP address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Comment on lines 69 to 75
.. figure:: /01-figures/tcp_wan_server.png
:alt: WAN Server settings options
:align: center

.. figure:: /01-figures/tcp_client.png
:alt: WAN Client settings options
:align: center
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest changing the order this figures are shown. In the text first the client side is described and then the server side, but the images follow the opposite order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


- **Intraprocess:** Allow Intraprocess delivery when two Participants run in the same process.

- **Data SHaring:** Allow Data Sharing delivery when two Participants run in the same process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **Data SHaring:** Allow Data Sharing delivery when two Participants run in the same process.
- **Data Sharing:** Allow Data Sharing delivery when two Participants run in the same process.

I suggest adding a link to the Fast DDS documentation in all these new options so the user can have easy access to a more in-depth explanation of the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -160,6 +160,12 @@ The user can customize several aspects of Shapes Demo operation:

+ Push the *Start* button in order to resume Shapes Demo operation.

- **Shared Memory Transport:** Fast DDS transport using Shared Memory Segment for Participants running in the same
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why but when generating the documentation locally, this option is shown in a blue box. Can you check in your side if it is something happening only on my side?

jparisu added 2 commits August 4, 2021 08:41
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
@richiprosima
Copy link

Build status: Build Status

Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

The Deadline example should be updated too because the update interval is modified.

Please, remove also all the figures that are not longer used.

docs/examples/tcp_LAN_WAN_transport.rst Show resolved Hide resolved
docs/first_steps/first_steps.rst Outdated Show resolved Hide resolved
docs/first_steps/first_steps.rst Outdated Show resolved Hide resolved
docs/first_steps/first_steps.rst Outdated Show resolved Hide resolved
Signed-off-by: jparisu <javierparis@eprosima.com>
@richiprosima
Copy link

Build status: Build Status

Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

There is still a figure in the Deadline example that should be updated...

Signed-off-by: jparisu <javierparis@eprosima.com>
@richiprosima
Copy link

Build status: Build Status

Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM!

@jparisu jparisu merged commit 5122a1f into master Aug 5, 2021
@jparisu jparisu deleted the feature/dds-refactor branch August 5, 2021 11:53
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.

3 participants