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

Jump to bottom

Added BoundedPlainSequences messages #14

Merged
merged 4 commits into from
Aug 4, 2021
Merged

Added BoundedPlainSequences messages #14

merged 4 commits into from
Aug 4, 2021

Conversation

MiguelCompany
Copy link
Contributor

Adding two message definitions with sequences of POD types

@MiguelCompany
Added BoundedSequencesNoStrings and UnboundedSequencesNoStrings messages
e17e99b 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
This was referenced Jun 8, 2021
Merged
Merged
hidmic
hidmic reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Circling back, sorry for the delay!

msg/BoundedSequencesNoStrings.msg Outdated
@@ -0,0 +1,32 @@
# Bounded sequences of different types
Copy link
Contributor

@hidmic hidmic Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@MiguelCompany if the core idea behind adding messages is to have bounded sequence of plain data types, consider making that explicit in the name e.g. BoundedPlainSequences, BoundedPODSequences or any meaningful name you like.

msg/UnboundedSequencesNoStrings.msg Outdated
@@ -0,0 +1,32 @@
# Unbounded sequences of different types
Copy link
Contributor

Choose a reason for hiding this comment

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

@MiguelCompany if you have no use case for this message type, please don't add it.

@MiguelCompany MiguelCompany changed the title Added BoundedSequencesNoStrings and UnboundedSequencesNoStrings messages Added BoundedPlainSequences messages Jun 16, 2021
@MiguelCompany
Removed UnboundedSequencesNoStrings
861840e 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Renamed BoundedSequencesNoStrings -> BoundedPlainSequences
1da1995 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Improve comment on BoundedPlainSequences.msg
4cf5487 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@hidmic
Copy link
Contributor

hidmic commented Jun 16, 2021 •
edited
Loading

CI up to rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, rcl_interfaces and test_rmw_implementation:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented Jul 30, 2021

MacOS CI, one more time:

  • macOS Build Status

@hidmic
Copy link
Contributor

hidmic commented Aug 4, 2021

CI again as it's been a while:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented Aug 4, 2021

Alright, finally green. Going in!

@hidmic hidmic merged commit d092944 into ros2:master Aug 4, 2021
@MiguelCompany MiguelCompany deleted the sequences-no-strings branch August 5, 2021 06:58
@scpeters
Copy link

scpeters commented Aug 5, 2021

This is Steve your friendly build farmer here checking in. I don't know if it was this or ros2/rcl_interfaces#123, but the last nightly build shows quite a few failures in the test_communication package related to BoundedPlainSequences

I'm not sure if this PR's CI builds included the test_communication package or not, but it seems to be unhappy at the moment

@sloretz sloretz mentioned this pull request Aug 5, 2021
Merged
This was referenced Aug 6, 2021
Closed
Closed
@MiguelCompany
Copy link
Contributor Author

@scpeters @hidmic It seems test_communication was not included in the PR's CI builds.

I see two different issues here:

  1. ros2/rcl_interfaces#123 did not update function get_test_msg. Adding BoundedPlainSequences to get_test_msg rcl_interfaces#126 should take care of it
  2. CPP executables on test_communication package should handle BoundedPlainSequences as an argument. Will try to do a PR today for that

@sloretz sloretz mentioned this pull request Sep 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MiguelCompany @hidmic @scpeters