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

Fixed playing if unknown message types exist #592

Merged
merged 1 commit into from
Dec 29, 2020

Conversation

iuhilnehc-ynos
Copy link

Fixes #591

  1. play a specific known message type even if some unknown types exist.
  2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui [email protected]

1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
@iuhilnehc-ynos iuhilnehc-ynos requested a review from a team as a code owner December 29, 2020 10:52
@iuhilnehc-ynos iuhilnehc-ynos requested review from emersonknapp and dabonnie and removed request for a team December 29, 2020 10:52
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This LGTM - i am convinced by the argument that it is simpler just to warn and play anyways. Couple of minor nitpicks unrelated to the functionality - address if you like.

@@ -44,7 +44,7 @@ class MockSequentialReader : public rosbag2_cpp::reader_interfaces::BaseReaderIn

while (num_read_ < messages_.size()) {
for (const auto & filter_topic : filter_.topics) {
if (!messages_[num_read_ + 1]->topic_name.compare(filter_topic)) {
if (!messages_[num_read_]->topic_name.compare(filter_topic)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change actually necessary?

topic.name, rosbag2_transport_->create_generic_publisher(
topic.name, topic.type, topic_qos)));
} catch (const std::runtime_error & e) {
// using a warning log seems better than adding a new option
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment doesn't make much sense without the context of the issue conversation - i think that the text in the warning explains it well enough.

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/14502914c97ff87953246acb31fd7622/raw/5a1b1739bf80b50809975253103a1e68a56ef542/ros2.repos
BUILD args: --packages-up-to rosbag2_transport
TEST args: --packages-select rosbag2_transport
Job: ci_launcher

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

@emersonknapp
Copy link
Collaborator

The windows build:

15:27:06 40:  - C:/ci/ws/install/Scripts/ament_uncrustify.exe --xunit-file C:/ci/ws/build/rosbag2_transport/test_results/rosbag2_transport/uncrustify.xunit.xml
15:27:06 40: Could not find 'uncrustify' executable

Considering the action CI here ran the linters, let's not be blocked on that.

@emersonknapp emersonknapp merged commit f53435f into ros2:master Dec 29, 2020
adamdbrw pushed a commit that referenced this pull request Jan 14, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
@Karsten1987
Copy link
Collaborator

@emersonknapp I believe we should backport this to Foxy.

emersonknapp pushed a commit that referenced this pull request Mar 19, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
emersonknapp pushed a commit that referenced this pull request Mar 19, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
emersonknapp pushed a commit that referenced this pull request Jul 9, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
emersonknapp pushed a commit that referenced this pull request Jul 10, 2021
1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
emersonknapp added a commit that referenced this pull request Jul 15, 2021
#592) (#686)

1. play a specific known message type even if some unknown types exist.
2. add a warning message while a message type library not exist.

Signed-off-by: Chen Lihui <[email protected]>
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.

play a topic with a known type could be failure
3 participants