-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor RMW to use DDS API [10557] #3
Conversation
Comment for reviewers. |
Please, do not forget to open also a related PR in
|
ROS2 foxy will fail in some test cases using the command
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
Please, in file rmw_fastrtps_cpp/src/rmw_compare_gids_equal.cpp
remove fastrtps/rtps/common/Guid.h
header which is not being used. Also, in rmw_fastrtps_dynamic_cpp/src/rmw_compare_gids_equal.cpp
.
This same header file should be updated to fastdds/rtps/common/Guid.h
in rmw_fastrtps_shared_cpp/src/create_rmw_gid.cpp
.
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/TypeSupport.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
In rmw_fastrtps_shared_cpp/src/rmw_security_logging.cpp
there is a mention in one comment to Fast RTPS that I suggest we upgrade to Fast DDS. I would also include the following header files which are being used in this file:
fastdds/rtps/common/Property.h
fastdds/rtps/attributes/PropertyPolicy.h
Finally, this same file also includes a error message using Fast-RTPS
which I suggest upgrading to Fast DDS
.
File rmw_fastrtps_shared_cpp/src/rmw_wait.cpp
includes unused header file fastrtps/subscriber/Subscriber.h
. Please, remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
rmw_fastrtps_shared_cpp
test files (rmw_fastrtps_shared_cpp/test/
):
test_dds_attributes_to_rmw_qos.cpp
: this test depends on how to proceed with the old QoS functions. A deprecated warning should be add or they can be directly removed if the ROS 2 maintainers agree. Also, these tests should be implemented using the new DDS API.test_guid_utils.cpp
: upgradeGuid.h
header file to the one included in thefastdds
folder. Also, following the rule to include the header files that are used, please includefastdds/rtps/common/GuidPrefix_t.hpp
andfastdds/rtps/common/EntityId_t.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review:
File rmw_fastrtps_shared_cpp/test/test_security_logging.cpp
:
- Include Fast DDS header files being used:
fastdds/rtps/common/Property.h
andfastdds/rtps/attributes/PropertyPolicy.h
- Also, modify the
Fast-RTPS
reference (L269) forFast DDS
. This will ensure that the test keeps passing if the corresponding suggestion inrmw_fastrtps_shared_cpp/src/rmw_security_logging.cpp
has been applied.
rmw_fastrtps_shared_cpp/test/test_rmw_qos_to_dds_attributes.cpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/test/test_rmw_qos_to_dds_attributes.cpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review:
- CustomParticipantInfo
- CustomPublisherInfo
- CustomSubscriberInfo
- rmw_take
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_client_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_client_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review: rmw_fastrtps_shared_cpp
review is finished.
if (ret != ReturnCode_t::RETCODE_OK) { | ||
RMW_SET_ERROR_MSG("Fail in delete datareader"); | ||
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ret != ReturnCode_t::RETCODE_OK) { | |
RMW_SET_ERROR_MSG("Fail in delete datareader"); | |
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); | |
if (ret != ReturnCode_t::RETCODE_OK && final_ret != RMW_RET_OK) { | |
RMW_SAFE_FWRITE_TO_STDERR( | |
RCUTILS_STRINGIFY(__function__) ":" RCUTILS_STRINGIFY(__line__) ": " | |
"couldn't publish writer disassociation from graph_cache when destroying client"); | |
RMW_SET_ERROR_MSG("Fail in delete datareader"); | |
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); |
if (ret != ReturnCode_t::RETCODE_OK) { | ||
RMW_SET_ERROR_MSG("Fail in delete datareader"); | ||
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ret != ReturnCode_t::RETCODE_OK) { | |
RMW_SET_ERROR_MSG("Fail in delete datareader"); | |
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); | |
if (ret != ReturnCode_t::RETCODE_OK && final_ret != RMW_RET_OK) { | |
RMW_SAFE_FWRITE_TO_STDERR( | |
RCUTILS_STRINGIFY(__function__) ":" RCUTILS_STRINGIFY(__line__) ": " | |
"couldn't publish writer disassociation from graph_cache when destroying client"); | |
RMW_SET_ERROR_MSG("Fail in delete datareader"); | |
return rmw_fastrtps_shared_cpp::cast_error_dds_to_rmw(ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a check in the CmakeLists.txt
of the three packages (rmw_fastrtps_cpp
, rmw_fastrtps_dynamic_cpp
and rmw_fastrtps_shared_cpp
checking that the Fast DDS version is 2.3 or higher. Without the changes implemented within Fast DDS 2.3.x, this new implementation of the RMW layer will not build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final review! Enjoy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments:
- Change all
sample_info.instance_state == ALIVE_INSTANCE_STATE
comparisons intosample_info.valid_data
- When creating an entity, try to replicate the old behavior regarding the retrieval of the type-support. Method
DomainParticipant::find_type
can be used for that. Consider the possibility of changing the type of thetype_support_
fields toeprosima::fastdds::dds::TypeSupport
- Restore the behavior of deleting the topic and unregistering the type when deleting the endpoints. The return code can be ignored, as it will be either
OK
orPRECONDITION_NOT_MET
if the topic / type is still in use.
Take sequence:
Using vectors will allocate on the heap, so I would try to avoid them. One possible strategy is to use StackAllocatedSequence<T, max_elems>
and loop if the requested count
is greater than max_elems
. As the maximum should be known at compile-time, there is a performance-allocations compromise here. I propose to use a maximum of 32, which is the default value of ReaderResourceLimitsQos::max_samples_per_read
dc80c49
to
65a1ef6
Compare
cf24cf5
to
ba31af5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly typos.
I will finish resolving conversations and checking the tests tomorrow.
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp
Show resolved
Hide resolved
5c78569
to
34e8225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other changes:
- First of all, my suggestion of removing
rmw_fastrtps_shared_cpp/TypeSupport
fromcustom_service_info.hpp
andcustom_client_info.hpp
breaks the building because in that same header file is definedrmw_fastrtps_shared_cpp::SerializedData
. -
rmw_fastrtps_shared_cpp/src/participant.cpp
: L319-320 there is an error message referring to Fast-RTPS (twice) that should be changed to Fast DDS. -
rmw_fastrtps_shared_cpp/src/rmw_client.cpp
: if there is an error updating the graph and the function returns before due to failures deleting the DataReader or the DataWriter, this error is lost. It should be logged in these cases. - The previous comment also affects
rmw_fastrtps_shared_cpp/src/rmw_service.cpp
. -
rmw_fastrtps_shared_cpp/test/test_dds_attributes_to_rmw_qos.cpp
should also checked QoS using the new DDS templates (besides the RTPS Attributes already checked). - Besides adding the requirement for Fast DDS version being at least 2.3 to the CMake files, I suggest stating this new requirement in the README.
-
ament_cpplint rmw_fastrtps_dynamic_cpp
returns 10 errors -
ament_uncrustify rmw_fastrtps_shared_cpp
returns 2 files with code style divergence (custom_subscriber_info.hpp
andcustom_publisher_info.hpp
) -
ament_uncrustify rmw_fastrtps_dynamic_cpp
returns 5 files with code style divergence (subscription.cpp
,rmw_service.cpp
,rmw_client.cpp
,publisher.cpp
andtest_logging.cpp
.
Still analyzing test failures from running locally ROS 2 tests.
void | ||
rtps_qos_to_rmw_qos( | ||
const DDSQoSPolicyT & dds_qos, | ||
rmw_qos_profile_t * qos) | ||
{ | ||
switch (dds_qos.m_reliability.kind) { | ||
case eprosima::fastrtps::BEST_EFFORT_RELIABILITY_QOS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the fastrtps
namespace, I am not sure if we should also include fastrtps/qos/QosPolicies.h
file where these constants are defined within this namespace.
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp
Outdated
Show resolved
Hide resolved
Running locally ROS 2 CI tests I have consistent failures in the following packages and tests:
|
Just in case, before running the tests, I usually remove the contents of the shared memory folder (i.e.
Running
Running
These two fail consistently, though not always on the same tests. I will investigate them.
Running |
Running the tests on Ubuntu 20.04, after a clean installation of ROS 2 Rolling and after removing the content on
As soon as test 31 (
Similarly, as soon as test 73 (
In this case both tests fail always in the first run: test 7 ( |
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Commit fe90baa solves CI failures. CI looks good now (running on Ubuntu 20.04) |
f860e06
to
8e03b3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please uncrustify rmw_fastrtps_shared_cpp/src/rmw_client.cpp
and rmw_fastrtps_shared_cpp/src/rmw_service.cpp
, and fix copyright notice on the new test file.
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
* Changes required to incorporate the network flows feature Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Update API names to match rmw updated ones Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Update to new options interface. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Solve windows linkage issues. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Remove unique flows support on publishers. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Update subscription creation to new Fast DDS interface Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Update to new locator getters interface. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Update to endpoints interface. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Fix linting Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Make linters happy Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Use updated rmw interface Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com> * Uncrustify Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com> * Include C++ header before others Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com> * Use updated rmw interface to populate flow endpoint Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com> * Fixed unreferenced local variable warnings Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Miguel Barro <miguelbarro@eprosima.com> Co-authored-by: EduPonz <eduardoponz@eprosima.com> Co-authored-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
2bc658a
to
e0b0b01
Compare
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
It requires Fast DDS PRs to be merged to pass tests successfully:
Depends on
Loans are not implemented.