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

Added Header to obstacles message #55

Merged
merged 2 commits into from
Mar 16, 2024
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 25, 2022

Signed-off-by: Alejandro Hernández Cordero [email protected]

In this PR, I used tf2_ros::MessageFilter as one of the TODOs that @Yadunund left in the code. This field is required in order to use this feature

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@Yadunund
Copy link
Member

@ahcorde each Obstacle has its own header. Can this information be used for your application?

Originally this message was meant to be published over /rmf_obstacles by an rmf_obstacle_server that collates obstacle information it receives from multiple detection sources. Hence the message did not have a header but each Obstacle does.
But till such a server is developed, most of the detectors we have directly publish to /rmf_obstacles using this message.

Let me know your thoughts. Happy to iterate.

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 25, 2022

The tf2_ros::MessageFilter needs a Header in the message in order to check the frame_id. This is part of your PR open-rmf/rmf_ros2#210 where you added a TODO to include the MessageFilter.

@koonpeng
Copy link

koonpeng commented Dec 1, 2022

rmf_obstacle_ros2 cannot compile without this PR

--- stderr: rmf_obstacle_ros2                                
In file included from /home/koonpeng/ws-osrf/chart-sim/src/rmf/rmf_ros2/rmf_obstacle_ros2/src/lane_blocker/LaneBlocker.hpp:35,
                 from /home/koonpeng/ws-osrf/chart-sim/src/rmf/rmf_ros2/rmf_obstacle_ros2/src/lane_blocker/LaneBlocker.cpp:19:
/opt/ros/humble/include/tf2_ros/tf2_ros/message_filter.h: In instantiation of ‘void tf2_ros::MessageFilter<M, BufferT>::add(const MEvent&) [with M = rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> >; BufferT = tf2_ros::Buffer; tf2_ros::MessageFilter<M, BufferT>::MEvent = message_filters::MessageEvent<const rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> > >]’:
/opt/ros/humble/include/tf2_ros/tf2_ros/message_filter.h:603:8:   required from ‘void tf2_ros::MessageFilter<M, BufferT>::incomingMessage(const message_filters::MessageEvent<const M>&) [with M = rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> >; BufferT = tf2_ros::Buffer]’
/opt/ros/humble/include/tf2_ros/tf2_ros/message_filter.h:265:46:   required from ‘void tf2_ros::MessageFilter<M, BufferT>::connectInput(F&) [with F = message_filters::Subscriber<rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> > >; M = rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> >; BufferT = tf2_ros::Buffer]’
/home/koonpeng/ws-osrf/chart-sim/src/rmf/rmf_ros2/rmf_obstacle_ros2/src/lane_blocker/LaneBlocker.cpp:224:38:   required from here
/opt/ros/humble/include/tf2_ros/tf2_ros/message_filter.h:365:60: error: ‘value’ is not a member of ‘message_filters::message_traits::FrameId<rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> >, void>’
  365 |     std::string frame_id = stripSlash(mt::FrameId<M>::value(*message));
      |                                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
In file included from /opt/ros/humble/include/rclcpp/rclcpp/logging.hpp:24,
                 from /opt/ros/humble/include/rclcpp/rclcpp/client.hpp:40,
                 from /opt/ros/humble/include/rclcpp/rclcpp/callback_group.hpp:23,
                 from /opt/ros/humble/include/rclcpp/rclcpp/any_executable.hpp:20,
                 from /opt/ros/humble/include/rclcpp/rclcpp/memory_strategy.hpp:25,
                 from /opt/ros/humble/include/rclcpp/rclcpp/memory_strategies.hpp:18,
                 from /opt/ros/humble/include/rclcpp/rclcpp/executor_options.hpp:20,
                 from /opt/ros/humble/include/rclcpp/rclcpp/executor.hpp:37,
                 from /opt/ros/humble/include/rclcpp/rclcpp/executors/multi_threaded_executor.hpp:25,
                 from /opt/ros/humble/include/rclcpp/rclcpp/executors.hpp:21,
                 from /opt/ros/humble/include/rclcpp/rclcpp/rclcpp.hpp:155,
                 from /home/koonpeng/ws-osrf/chart-sim/src/rmf/rmf_ros2/rmf_obstacle_ros2/src/lane_blocker/LaneBlocker.hpp:21,
                 from /home/koonpeng/ws-osrf/chart-sim/src/rmf/rmf_ros2/rmf_obstacle_ros2/src/lane_blocker/LaneBlocker.cpp:19:
/opt/ros/humble/include/tf2_ros/tf2_ros/message_filter.h:411:9: error: ‘value’ is not a member of ‘message_filters::message_traits::FrameId<rmf_obstacle_msgs::msg::Obstacles_<std::allocator<void> >, void>’
  411 |         TF2_ROS_MESSAGEFILTER_DEBUG(
      |

@codebot
Copy link
Contributor

codebot commented Feb 14, 2023

Anyone have any updates on this? Any downside to merging this?

@Yadunund
Copy link
Member

I think the original intention of the Obstacles message was to collate obstacle data from various sources and publish to /rmf_obstacles. This collation was to be done by an rmf_obstacle_server that is still a TODO.
As a work around, detection nodes have been directly publishing this Obstacles message.
With the original intention, having a header in the Obstacles message doesn't make much sense since each Obstacle has its own header with a frame_id of reference. But it looks like the implementation here and here have been updated to rely on the change in this PR. I think right now it's a bit confusing that users need to populate the header in Obstacles and also the header for each Obstacle.

I see two options moving forward:

  1. Undo changes to rmf_obstacle_ros2 rmf_ros2#210 such that it doesn't require this header and stick to the original intention of Obstacles.

  2. Accept the change here and maybe drop the header for each Obstacle. But we probably need to rethink the rmf_obstacle_server.

What do you think?

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Let's merge this in for now. GSoC 24 participants can help to improve things.

@Yadunund Yadunund merged commit a7a66e1 into main Mar 16, 2024
1 check passed
@Yadunund Yadunund deleted the ahcorde/obstacles_header branch March 16, 2024 15:39
Yadunund added a commit that referenced this pull request Mar 17, 2024
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Yadunund <[email protected]>
Yadunund added a commit that referenced this pull request Mar 17, 2024
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@Avisheet
Copy link

After ensuring that I pulled the latest rmf_internal_msgs and even after rechecking if I have done it correctly or not . I am getting the same build error for rmf_obstacle_detectors as given in open-rmf/rmf_obstacle#18.
Can anyone tell me why is this so ? Have I missed something ?

@Avisheet
Copy link

After ensuring that I pulled the latest rmf_internal_msgs and even after rechecking if I have done it correctly or not . I am getting the same build error for rmf_obstacle_detectors as given in open-rmf/rmf_obstacle#18. Can anyone tell me why is this so ? Have I missed something ?

@ahcorde @Yadunund can you assist me regarding this .

@Yadunund
Copy link
Member

Our ci which builds from source is passing with latest main and iron branches.

Please make sure you're not using binaries of Open-RMF to compile rmf_obstacle_ros2 as the changes from here have not yet been released.

@Avisheet
Copy link

Thanks @Yadunund for your reply but the build error I'm facing is in rmf_human_detector as described here . I have ensured to pull all the latest repositories.

@Avisheet
Copy link

Thanks @Yadunund for your reply but the build error I'm facing is in rmf_human_detector as described here . I have ensured to pull all the latest repositories.

@Yadunund I have ensured that all the files are upto date and i'm not using binaries for installation for rmf_obstacle_ros. I am getting the same build error .

@Yadunund
Copy link
Member

I am not able to reproduce the issue with a clean build. Infact, I ran your own PR through our CI and it builds successfully: See (iron, jammy) run.
It's clear that there is an issue with your local setup. I urge you not to comment on this closed PR. If you do encounter a different issue, please open a ticket in rmf_obstacles.

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.

5 participants