ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
425 stars 275 forks source link

Implement Foreign Mapping Check Override #367

Closed methylDragon closed 2 years ago

methylDragon commented 2 years ago

As requested by, and closes: https://github.com/ros2/ros1_bridge/issues/364

Description

This PR implements a new enable_foreign_mappings mapping rule that allows one to override the package name check that forces a user to define any mapping rules that target a bridge ROS 2 package from that package itself.

This will let you define mappings from OUTSIDE a target message's package.

Reminder

As in the Known Issues section, you'll probably need to run the bridge with --bridge-all-topics to get the bridge working with custom messages.

Example Usage

See the docs for more details

Set the enable_foreign_mappings rule to true/True/1 in your custom mapping rules file! It'll override the check for EVERY RULE defined in that file!

enable_foreign_mappings: true
ros1_package_name: 'ros1_pkg_name'
ros1_service_name: 'ros1_srv_name'
ros2_package_name: 'ros2_**FOREIGN**_pkg_name'
ros2_service_name: 'ros2_srv_name'

And also add an ament resource (ros1_bridge_foreign_mapping) in the mapping package, which will allow the mapping package to be found! In the CMakeLists.txt of your mapping package (which is NOT the msgs package!!), before the mapping rules install call:

ament_index_register_resource("ros1_bridge_foreign_mapping")
install(
    FILES YOUR_MAPPING_RULE_FILE.yaml
    DESTINATION share/${PROJECT_NAME})

The implementation is tested locally, and seems to work (with messages). image

methylDragon commented 2 years ago

:notes: :notes: :notes:


EDIT: Nevermind, it's explicitly ignored on those jobs...

:notes: :zap: :notes: :zap: :notes: :zap: :notes:

Using ci_packaging_linux instead...............

methylDragon commented 2 years ago

Looks like this package needs a fix (unrelated to this PR): https://github.com/ros2/ci/pull/669

The test build is running assuming that the CI PR is merged in, it shouldn't block this PR.

methylDragon commented 2 years ago

Edit: This isn't meant to work on jammy for now, so the focal build passing/being unstable for unrelated reasons is ok.

Build Status

It's unstable for unrelated reasons.

I think we're good to go once you can test it on your setup! @jaelrod

methylDragon commented 2 years ago

Looks good but I couldn't run it successfully, using this setup let me know if I'm missing something!

Humm, let me see

methylDragon commented 2 years ago

I'll need to add a custom ament resource... Sec

methylDragon commented 2 years ago

Build Status

Let's try to fix some linting errors while we're at it: Build Status Edit: Nevermind.. It's between having failing actions CI and unstable builds on ci_packaging_linux... I'll take unstable

EDIT: I couldn't help it, I'm trying one more time, merging as long as we don't fail explicitly: Build Status

methylDragon commented 2 years ago

OMG it's green!!! Merging

jaelrod commented 2 years ago

Nice! Thanks, again! Was following along the PR discussion and planning to test it today but ended up getting sidetracked on something else. Will try backporting to eloquent locally and test it next week.

methylDragon commented 2 years ago

Nice! Thanks, again! Was following along the PR discussion and planning to test it today but ended up getting sidetracked on something else. Will try backporting to eloquent locally and test it next week.

Be sure to check out the docs for how to set this up!

jaelrod commented 2 years ago

Backported locally to eloquent and can confirm that it works as expected.

methylDragon commented 2 years ago

Excellent!