Skip to content

Conversation

@fgallegosalido
Copy link
Contributor

This adds support for the messages added by ros2/rcl_interfaces#173 to the test_communication package.

Part of ros2/ros2#1538.

@MiguelCompany
Copy link
Contributor

@fgallegosalido Would you mind rebasing this on top of #542 with:

  1. A commit that adds support for KeyedLong
  2. A commit with the changes in CMakeLists.txt regarding the rmw_implementation validation

@fgallegosalido
Copy link
Contributor Author

fgallegosalido commented Apr 2, 2025

@MiguelCompany I do not have permission to modify the branch feature/rolling/keys from your fork. I'm not sure what you want me to do. Do you want to merge the pull request #542 instead of this one? In that case, it makes sense for you to add the KeyedLong type to your fork.

@MiguelCompany
Copy link
Contributor

@fgallegosalido The idea is that your branch (which you can modify) stems from the head of my branch, and contains additional commits with your work. This way, your PR will have the commits from both, and we could close my PR in favor of this one, and when this one is merged, we will both count as contributors.

…RTPS and Connext RMWs

Signed-off-by: Francisco Gallego Salido <[email protected]>
@fgallegosalido
Copy link
Contributor Author

@MiguelCompany done. I've added tests for the KeyedLong message and also generate tests for Connext RMW too.

@MiguelCompany
Copy link
Contributor

MiguelCompany commented Apr 3, 2025

CI with this repos file:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

string_ends_with("${interface_file}" ".msg" is_message)
if(is_message AND interface_ns STREQUAL "msg")
string_ends_with("${interface_file}" ".idl" is_idl)
if(is_idl AND interface_ns STREQUAL "msg")
Copy link
Contributor

Choose a reason for hiding this comment

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

is the same change required for srv to avoid the duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this probably related to #549, although it doesn't seem to be necessary for now and can be backported later

fgallegosalido and others added 2 commits April 6, 2025 16:07
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Francisco Gallego Salido <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Francisco Gallego Salido <[email protected]>
@fgallegosalido
Copy link
Contributor Author

@fujitatomoya changes applied. Let me know of anything else in order to be able to merge.

@MiguelCompany
Copy link
Contributor

New CI with this repos file:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@mjcarroll mjcarroll merged commit e5c9a0d into ros2:rolling Apr 7, 2025
2 of 3 checks passed
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.

5 participants