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

Duplicate messages in bidirectional_bridge fix #113

Merged
merged 7 commits into from
Apr 12, 2018

Conversation

ArkadiuszNiemiec
Copy link
Contributor

Issue mentioned in #43 and #111, solution based on discussion in #43. Branched from ardent as I am not able to build master on my system.

Tested by using parameter_bridge and multiple ROS1 and ROS2 publishers and subscribers on the same topic.

Thank you @wjwwood for your help!

This PR is copy of broken PR #112. Sorry for the inconvenience.

@ArkadiuszNiemiec
Copy link
Contributor Author

@wjwwood It now passes all tests.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

The changes make sense to me, I've just asked that you catch and report one of the error cases.

I still haven't reproduced it myself, but I assume you have seen that it fixes your original issue?

if (result) { // message GID equals to bridge's ROS2 publisher GID
return; // do not publish messages from bridge itself
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The else case should be handled here. If rmw_compare_gids_equal fails, then we should probably raise an exception or at least log an error message to the console using RCLCPP_ERROR logging macro.

Copy link
Member

Choose a reason for hiding this comment

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

You can get the underlying error message with rmw_get_error_string_safe(), and after getting it you should clear the error state with rmw_reset_error().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have found similar code on different repo. Added.

@ArkadiuszNiemiec
Copy link
Contributor Author

I still haven't reproduced it myself, but I assume you have seen that it fixes your original issue?

Yes, I have tested it by starting two nodes publishing on one topic. After including the fix the duplicates went away. I have started ROS2 node on the same topic and it also bridged perfectly.

@wjwwood
Copy link
Member

wjwwood commented Apr 11, 2018

CI:

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

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2018

Actually since this is only in the ros1_bridge it only needs a Linux packaging job (we don't build the bridge in our normal CI):

Build Status

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2018

@ArkadiuszNiemiec looks like the linters are still unhappy about some trailing whitespace or something. Can you check ament_uncrustify and ament_cpplint one more time?

@ArkadiuszNiemiec
Copy link
Contributor Author

@wjwwood Fixed, empty line was not empty 😄

@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2018

Thanks, new CI:

  • Linux Packaging: Build Status

@mikaelarguedas mikaelarguedas added the in review Waiting for review (Kanban column) label Apr 12, 2018
@wjwwood wjwwood merged commit 32a4c60 into ros2:master Apr 12, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Apr 12, 2018
@wjwwood
Copy link
Member

wjwwood commented Apr 12, 2018

Thanks @ArkadiuszNiemiec!

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.

3 participants