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

Add Fast DDS Waitsets to Vulcanexus Humble branch #13

Merged
merged 22 commits into from
Jul 13, 2022

Conversation

jsan-rt
Copy link

@jsan-rt jsan-rt commented Jul 12, 2022

No description provided.

MiguelCompany and others added 17 commits July 12, 2022 09:53
…#616)

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
…ith get_first_untaken_info

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
…READ

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
rmw_fastrtps_cpp/src/subscription.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_cpp/src/publisher.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_cpp/src/rmw_client.cpp Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/rmw_service.cpp Outdated Show resolved Hide resolved
@@ -102,12 +103,6 @@ __rmw_destroy_service(
show_previous_error();
RMW_SET_ERROR_MSG("Fail in delete datareader");
final_ret = RMW_RET_ERROR;
info->request_reader_->set_listener(nullptr);

Choose a reason for hiding this comment

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

Do not remove this line. In case that the DataReader deletion fails, the listener is detached from the DataReader so it can be deleted without having any conflict. Same comment applies below to the response_writer.

Choose a reason for hiding this comment

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

response_writer listener is not set to nullptr in case that the deletion of the DataWriter fails. This could cause a core dump if the deleted listener is still attached to the DataWriter.

@JLBuenoLopez
Copy link

I suggest applying these suggestions to the original PR too.

@jsan-rt jsan-rt force-pushed the feature/fastdds-waitsets-humble branch from 3aec19d to 966891b Compare July 13, 2022 07:40
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
@jsan-rt jsan-rt force-pushed the feature/fastdds-waitsets-humble branch from 966891b to c31f2eb Compare July 13, 2022 08:01
richiware and others added 4 commits July 13, 2022 10:41
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
…ents.

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
@jsan-rt jsan-rt force-pushed the feature/fastdds-waitsets-humble branch from c31f2eb to aca2fae Compare July 13, 2022 08:41
Copy link

@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.

Changes LGTM!

Disclaimer: currently the following tests in rclcpp are failing. This is caused because ROS 2 assumes that every sample is notified when this is not necessarily required.

  • test_client
  • test_service
  • test_subscription

Due to the current implementation, notifying more samples than expected can lead to some overhead performance. Nevertheless, changes included in this PR are required to solve several other more important issues.

@jsan-rt jsan-rt merged commit 6811420 into vulcanexus-humble Jul 13, 2022
@jsan-rt jsan-rt deleted the feature/fastdds-waitsets-humble branch July 13, 2022 10:25
@jsan-rt jsan-rt restored the feature/fastdds-waitsets-humble branch July 15, 2022 06:46
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.

4 participants