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

rule can be a message mapping and a field_mapping #135

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Aug 10, 2018

This is now ready for review with steps to reproduce and test

Connects to #134

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Aug 10, 2018
@@ -360,7 +360,7 @@ def __init__(self, data, expected_package_name):
'Mapping for package %s contains unknown field(s)' % self.ros2_package_name)

def is_message_mapping(self):
return self.ros1_message_name is not None and self.fields_1_to_2 is None
return self.ros1_message_name is not None
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. I don't see why fields_1_to_2 was being constrained here. It is later used through is_field_mapping to determine how to map the fields.

@mikaelarguedas
Copy link
Member Author

How to reproduce the issue and test the fix:

Build environment:

Shell 1 (setup a container)

docker pull osrf/ros2:bouncy-ros1-bridge && docker run -it --rm osrf/ros2:bouncy-ros1-bridge
# apt update && apt install -y git python3-colcon-common-extensions build-essential
# cd && git clone https://github.com/mikaelarguedas/ros1_bridge_bug.git

Shell 2: (log in container and build ros1 workspace)

docker exec -it <CONTAINER_ID> bash
cd ~/ros1_bridge_bug/ros1_msgs_overlay/
source /opt/ros/melodic/setup.bash 
catkin_make_isolated --install

Shell 3: log in container and build ros2 ws

docker exec -it <CONTAINER_ID> bash
source /opt/ros/bouncy/setup.bash
cd ~/ros1_bridge_bug/ros2_msgs_overlay/
colcon build

Shell 4: log in container and build custom bridge

docker exec -it <CONTAINER_ID> bash
mkdir -p ~/ros1_bridge_bug/ros1_bridge_ws/src && cd ~/ros1_bridge_bug/ros1_bridge_ws/src
git clone https://github.com/ros2/ros1_bridge.git
cd ~/ros1_bridge_bug/ros1_bridge_ws
source /opt/ros/melodic/setup.bash 
source /opt/ros/bouncy/setup.bash 
source ~/ros1_bridge_bug/ros1_msgs_overlay/install_isolated/setup.bash 
source ~/ros1_bridge_bug/ros2_msgs_overlay/install/setup.bash
colcon build --event-handlers console_direct+

Confirm the absence of mapping:

cat build/ros1_bridge/generated/ros2_msgs_factories.hpp
File content without mapping
// generated from ros1_bridge/resource/pkg_factories.hpp.em

#include <ros1_bridge/factory.hpp>

// include ROS 1 messages

// include ROS 2 messages

namespace ros1_bridge
{

std::shared_ptr<FactoryInterface>
get_factory_ros2_msgs(const std::string & ros1_type_name, const std::string & ros2_type_name);

std::unique_ptr<ServiceFactoryInterface>
get_service_factory_ros2_msgs(const std::string & ros_id, const std::string & package_name, const std::string & service_name);

// conversion functions for available interfaces
}  // namespace ros1_bridge

Rebuild the bridge with this fix:

cd ~/ros1_bridge_bug/ros1_bridge_ws && rm -rf build install
cd ~/ros1_bridge_bug/ros1_bridge_ws/src/ros1_bridge && git checkout -t origin/fix_message_and_field_mapping && cd ~/ros1_bridge_bug/ros1_bridge_ws
colcon build --event-handlers console_direct+

Confirm the presence of mapping:

cat build/ros1_bridge/generated/ros2_msgs_factories.hpp
File content with mapping
// generated from ros1_bridge/resource/pkg_factories.hpp.em

#include <ros1_bridge/factory.hpp>

// include ROS 1 messages
#include <ros1_msgs/EgoData.h>

// include ROS 2 messages
#include <ros2_msgs/msg/size.hpp>

namespace ros1_bridge
{

std::shared_ptr<FactoryInterface>
get_factory_ros2_msgs(const std::string & ros1_type_name, const std::string & ros2_type_name);

std::unique_ptr<ServiceFactoryInterface>
get_service_factory_ros2_msgs(const std::string & ros_id, const std::string & package_name, const std::string & service_name);

// conversion functions for available interfaces

template<>
void
Factory<
  ros1_msgs::EgoData,
  ros2_msgs::msg::Size
>::convert_1_to_2(
  const ros1_msgs::EgoData & ros1_msg,
  ros2_msgs::msg::Size & ros2_msg);

template<>
void
Factory<
  ros1_msgs::EgoData,
  ros2_msgs::msg::Size
>::convert_2_to_1(
  const ros2_msgs::msg::Size & ros2_msg,
  ros1_msgs::EgoData & ros1_msg);

}  // namespace ros1_bridge

Test that bridging now works

Note: each shell is assumed to be a new shell in the container with nothing sourced in it

Shell a:

source /opt/ros/melodic/setup.bash
roscore

Shell b:

source /opt/ros/melodic/setup.bash 
source ~/ros1_bridge_bug/ros1_msgs_overlay/install_isolated/setup.bash 
source /opt/ros/bouncy/setup.bash 
source ~/ros1_bridge_bug/ros2_msgs_overlay/install/setup.bash 
source ~/ros1_bridge_bug/ros1_bridge_ws/install/setup.bash 
ros2 run ros1_bridge dynamic_bridge

Shell c:

apt install ros-melodic-rostopic
source /opt/ros/melodic/setup.bash 
source ~/ros1_bridge_bug/ros1_msgs_overlay/install_isolated/setup.bash 
rostopic pub /foo ros1_msgs/EgoData "vehicleLength: 0.0"

Shell d:

source ~/ros1_bridge_bug/ros2_msgs_overlay/install/setup.bash 
source /opt/ros/bouncy/setup.bash 
source ~/ros1_bridge_bug/ros2_msgs_overlay/install/setup.bash
ros2 topic echo /foo ros2_msgs/Size

You should see the bridged message printed:

length: 0.0

CI

Just regular packaging CI to make sure everything else works as before: Build Status

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 10, 2018
@mikaelarguedas mikaelarguedas changed the title [WIP] rule can be a message mapping and a field_mapping rule can be a message mapping and a field_mapping Aug 10, 2018
@mikaelarguedas mikaelarguedas merged commit 0f1dbba into master Aug 10, 2018
@mikaelarguedas mikaelarguedas deleted the fix_message_and_field_mapping branch August 10, 2018 21:47
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Aug 10, 2018
@sloretz sloretz mentioned this pull request Aug 15, 2018
12 tasks
dhananjaysathe pushed a commit to rapyuta-robotics/ros1_bridge that referenced this pull request Aug 22, 2019
…well (ros2#135)

Signed-off-by: Dhananjay Sathe <dhananjay.sathe@rapyuta-robotics.com>
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.

2 participants