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.

Sign up for GitHub

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

Allow test_communication to use idl messages #549

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Allow test_communication to use idl messages #549

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc44c3a
Make test_communication use idl messages instead of msg ones.
MiguelCompany Apr 2, 2024
0eddfa9
Only process idl interfaces
MiguelCompany Sep 26, 2024
a8edc10
Additional refactor leveraging `elseif`
MiguelCompany Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions test_communication/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ if(BUILD_TESTING)
foreach(interface_file ${interface_files})
get_filename_component(interface_ns "${interface_file}" DIRECTORY)
get_filename_component(interface_ns "${interface_ns}" NAME)
string_ends_with("${interface_file}" ".msg" is_message)
if(is_message AND interface_ns STREQUAL "msg")
string_ends_with("${interface_file}" ".idl" is_idl)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see current change works with test_communication pacakge, but more generally what we can do is,

  • if .idl, we can categorize them based on namespace.
  • and other files .msg, .srv and .action can be categorized with file extensions and namespace.

what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

The official definition language is currently idl.
When .msg, .srv and .action files are processed, they are converted into .idl, and both files are installed and returned by ament_index_get_resource(interface_files "rosidl_interfaces" "<pkg_name>")

For instance, the contents of the file in install/share/ament_index/resource_index/rosidl_interfaces/test_msgs are the following:

action/Fibonacci.action
action/Fibonacci.idl
action/NestedMessage.action
action/NestedMessage.idl
msg/Arrays.idl
msg/Arrays.msg
msg/BasicTypes.idl
msg/BasicTypes.msg
msg/BoundedPlainSequences.idl
msg/BoundedPlainSequences.msg
msg/BoundedSequences.idl
msg/BoundedSequences.msg
msg/Builtins.idl
msg/Builtins.msg
msg/Constants.idl
msg/Constants.msg
msg/Defaults.idl
msg/Defaults.msg
msg/Empty.idl
msg/Empty.msg
msg/MultiNested.idl
msg/MultiNested.msg
msg/Nested.idl
msg/Nested.msg
msg/Strings.idl
msg/Strings.msg
msg/UnboundedSequences.idl
msg/UnboundedSequences.msg
msg/WStrings.idl
msg/WStrings.msg
srv/Arrays.idl
srv/Arrays.srv
srv/BasicTypes.idl
srv/BasicTypes.srv
srv/Empty.idl
srv/Empty.srv

So we should filter the list to remove duplicates. I think we should stick to .idl files.
We could take this a step further, and have a specific CMake function in either ament_index or rosidl_cmake to return the list of .idl in the ament index.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In that case, to keep the consistent check for all endpoints, we also want to change the line 49 and 50 for service files to use is_idl and namespace check?

Copy link
Author

@MiguelCompany MiguelCompany Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Refactored to only process idl interfaces in 0eddfa9 and a8edc10

if(is_idl AND interface_ns STREQUAL "msg")
list(APPEND message_files "${interface_file}")
continue()
endif()
Expand All @@ -51,8 +51,7 @@ if(BUILD_TESTING)
list(APPEND service_files "${interface_file}")
continue()
endif()
string_ends_with("${interface_file}" ".idl" is_action)
if(is_action AND interface_ns STREQUAL "action")
if(is_idl AND interface_ns STREQUAL "action")
list(APPEND action_files "${interface_file}")
continue()
endif()
Expand Down