ros2 / ros1_bridge

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

Apply automatic mapping rules in case only package+message mapping exists #382

Closed LoyVanBeek closed 1 year ago

LoyVanBeek commented 1 year ago

In determine_field_mapping, there was an early return inside a loop over all mapping rules. If there were any mapping rules but they don't specify field mappings, the early return made the function return without creating field mappings automatically.

For a particular message type, ROS 1's uuid_msgs/UniqueID vs ROS 2's unique_identifier_msgs/UUID, the message definition is exactly the same but type name is not. The only mapping rule defined for unique_identifier_msgs/UUID is that it maps to uuid_msgs/UniqueID, but no field mappings are needed because the definitions are the same: https://github.com/ros2/unique_identifier_msgs/blob/rolling/mapping_rules.yaml

But, then we hit the early return (because the for-loop is ran without any rule applying to the message at hand and thus not continue-ing in a code branch handling a rule) and return without applying the normal automatic field mapping generation rules.

By removing the early return, the other rules are applied and the mapping rules for handling the exact same message definitions are applied.

This fixes the issue found in https://github.com/ros2/unique_identifier_msgs/pull/25 in ros1_bridge instead of unique_identifier_msgs

This PR applies to foxy because I combine it with Noetic and can test only that combo, but the early return causing this issue is still there on the main branch: https://github.com/ros2/ros1_bridge/blob/master/ros1_bridge/__init__.py#L788

gbiggs commented 1 year ago

Please correct the DCO for your commits.

gbiggs commented 1 year ago

Please re-target your PR to the rolling branch. We will backport it to older distributions.

LoyVanBeek commented 1 year ago

There is no rolling branch. Latest is ROS distro listed in galactic but there's also master

gbiggs commented 1 year ago

Sorry, you're correct. Please target it to master.

gbiggs commented 1 year ago

CI: Build Status

gbiggs commented 1 year ago

Looks like the instability is unrelated to this PR.

gbiggs commented 1 year ago

Thanks for the contribution!

LoyVanBeek commented 1 year ago

No thnx, nice to have this merged.