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

[14447] Added ROS 2 Topic Support to Shapes Demo #51

Merged
merged 9 commits into from
May 18, 2022
Merged

Conversation

jsan-rt
Copy link
Contributor

@jsan-rt jsan-rt commented May 9, 2022

Topics used by ShapesDemo are not seen by ROS 2. This PR adds a feature to switch between the standard Topics and ROS 2 Topics.

ROS 2 support needs to be enabled on compile time.

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>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
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.

When enabling Content Based checkbox the following error messages are output:

[DDSSQLFILTER Error] No TypeObject found for type shapes_demo_typesupport::idl::dds_::KeylessShapeType_ -> Function create_content_filter
[PARTICIPANT Error] Could not create filter of class DDSSQL for expression "x > %0 AND x < %1 AND y > %2 AND y < %3 -> Function create_contentfilteredtopic

Shapes Demo is filtering but using the builtin ShapesDemo content filter and not the one in Fast DDS. I think that if possible this issue should be solved before merging this PR.

shapes_demo_typesupport/CMakeLists.txt Outdated Show resolved Hide resolved
shapes_demo_typesupport/idl/KeylessShapeType.idl Outdated Show resolved Hide resolved
shapes_demo_typesupport/package.xml Outdated Show resolved Hide resolved
shapes_demo_typesupport/package.xml Outdated Show resolved Hide resolved
idl/KeylessShape.idl Show resolved Hide resolved
src/shapesdemo/ShapePublisher.cpp Outdated Show resolved Hide resolved
src/shapesdemo/ShapesDemo.cpp Outdated Show resolved Hide resolved
src/shapesdemo/ShapesDemo.cpp Outdated Show resolved Hide resolved
src/qt/subscribedialog.cpp Show resolved Hide resolved
Comment on lines +331 to +334
$ ros2 topic list -t
/Square [shapes_demo_typesupport/idl/KeylessShapeType]
/parameter_events [rcl_interfaces/msg/ParameterEvent]
/rosout [rcl_interfaces/msg/Log]
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 able to see always the topic. In fact I have just seen it once and I have not been able to see it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually interesting because I can see the topic consistently. Can you give me any more info?

@jsan-rt
Copy link
Contributor Author

jsan-rt commented May 10, 2022

When enabling Content Based checkbox the following error messages are output:

[DDSSQLFILTER Error] No TypeObject found for type shapes_demo_typesupport::idl::dds_::KeylessShapeType_ -> Function create_content_filter
[PARTICIPANT Error] Could not create filter of class DDSSQL for expression "x > %0 AND x < %1 AND y > %2 AND y < %3 -> Function create_contentfilteredtopic

Shapes Demo is filtering but using the builtin ShapesDemo content filter and not the one in Fast DDS. I think that if possible this issue should be solved before merging this PR.

Good catch. This appears to actually be an issue with how Fast DDS-Gen generates the TypeObject when ROS name mangling is requested. The name for the TypeSupport is not the same it is registered with.

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
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.

Some small things, pending also to see why sometimes it is not working with ROS 2 CLI

CMakeLists.txt Outdated
if (NOT fastrtps_FOUND OR fastrtps_VERSION LESS 2.5.1)
message(FATAL_ERROR "Trying to use a version of Fast-DDS lower than 2.5.1. Please update your Fast-DDS installation.")
endif()

if(BUILD_ROS_COMPONENTS)
cmake_policy(SET CMP0057 NEW)
# cmake_policy(SET CMP0057 NEW)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the policy is not used, I suggest removing it instead of leaving phantom code.

README.md Outdated
---
**NOTE**

ROS 2 Topics enablement will disable some QOS that are not supported by ROS 2 at the moment, namely Ownership and Partitions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use QoS instead of QOS

@@ -41,9 +41,9 @@ using namespace eprosima::fastrtps::rtps;
void registerKeylessShapeTypes()
{
TypeObjectFactory *factory = TypeObjectFactory::get_instance();
factory->add_type_object("shapes_demo_typesupport::idl::KeylessShapeType", shapes_demo_typesupport::idl::GetKeylessShapeTypeIdentifier(true),
factory->add_type_object("shapes_demo_typesupport::idl::dds_::KeylessShapeType_", shapes_demo_typesupport::idl::GetKeylessShapeTypeIdentifier(true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you opened an internal task (Redmine) to track this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been created

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
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!

@jsan-rt jsan-rt merged commit dc1ad80 into master May 18, 2022
@jsan-rt jsan-rt deleted the feature/ros_topics branch May 18, 2022 05:25
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.

2 participants