Skip to content

Added BoundedPlainSequences messages#14

Merged
hidmic merged 4 commits intoros2:masterfrom
eProsima:sequences-no-strings
Aug 4, 2021
Merged

Added BoundedPlainSequences messages#14
hidmic merged 4 commits intoros2:masterfrom
eProsima:sequences-no-strings

Conversation

@MiguelCompany
Copy link
Copy Markdown
Contributor

@MiguelCompany MiguelCompany commented Jun 8, 2021

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
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown

@hidmic hidmic Jun 15, 2021

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
Copy Markdown

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
Copy Markdown

hidmic commented Jul 30, 2021

MacOS CI, one more time:

  • macOS Build Status

@hidmic
Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor Author

MiguelCompany commented Aug 6, 2021

@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.

3 participants

@MiguelCompany @hidmic @scpeters