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

Implement Foreign Mapping Check Override #367

Merged
merged 6 commits into from
Jun 30, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Jun 23, 2022

As requested by, and closes: #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
Copy link
Contributor Author

methylDragon commented Jun 23, 2022

🎶 🎶 🎶

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

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

🎶 ⚡ 🎶 ⚡ 🎶 ⚡ 🎶

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

  • Build Status

@methylDragon
Copy link
Contributor Author

methylDragon commented Jun 24, 2022

Looks like this package needs a fix (unrelated to this PR): ros2/ci#669

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

@methylDragon
Copy link
Contributor Author

methylDragon commented Jun 24, 2022

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

@quarkytale quarkytale self-requested a review June 28, 2022 01:27
Copy link
Contributor

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

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

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

@methylDragon
Copy link
Contributor Author

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 methylDragon marked this pull request as draft June 28, 2022 23:43
@methylDragon
Copy link
Contributor Author

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

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon marked this pull request as ready for review June 29, 2022 01:53
@methylDragon
Copy link
Contributor Author

methylDragon commented Jun 29, 2022

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 methylDragon added the enhancement New feature or request label Jun 29, 2022
@methylDragon methylDragon self-assigned this Jun 29, 2022
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the permissive_mapping branch 2 times, most recently from 1368a72 to ebb759a Compare June 29, 2022 17:21
@methylDragon
Copy link
Contributor Author

OMG it's green!!! Merging

@methylDragon methylDragon merged commit 4af421d into ros2:master Jun 30, 2022
@methylDragon methylDragon deleted the permissive_mapping branch June 30, 2022 00:59
@jaelrod
Copy link

jaelrod commented Jun 30, 2022

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

methylDragon commented Jun 30, 2022

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

jaelrod commented Jul 1, 2022

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

@methylDragon
Copy link
Contributor Author

Excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mapping Rules Limited to ROS2 Package
4 participants