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

rmf_obstacle_msgs #42

Merged
merged 8 commits into from
May 24, 2022
Merged

rmf_obstacle_msgs #42

merged 8 commits into from
May 24, 2022

Conversation

Yadunund
Copy link
Member

This PR introduces a new rmf_obstacle_msgs package.

The intention is to use the proposed Obstacle.msg to describe obstacles in the environment with the information on the size, location and representation of the obstacle contained in the ObstacleData.msg. The ObstacleData.data field may contain data that can be used to reconstruct a 3D representation of the object. The proposal is to go with an octree and provide utility functions in rmf_obstacle_ros2 to aid with the serialization/deserialization of the octrees into this message field.

One area of ambiguity is the coordinate frame of reference with respect to which the measurements are described. The comments in the messages inform the user that these should be with respect to the "Global RMF coordinates" (Traffic editor). I'm not sure if it would be more consistent if these measurements were with respect to Obstacle.header.frame_id?

Open to other suggestions and look forward to iterating.

Yadunund and others added 5 commits May 9, 2022 18:34
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@gmail.com>
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #42 (61d4db4) into main (a9196f4) will decrease coverage by 0.60%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            main      #42      +/-   ##
=========================================
- Coverage   5.85%    5.25%   -0.61%     
=========================================
  Files       1397     1399       +2     
  Lines      94320   105997   +11677     
  Branches   10006    10207     +201     
=========================================
+ Hits        5525     5571      +46     
- Misses     85099    96664   +11565     
- Partials    3696     3762      +66     
Flag Coverage Δ
tests 5.25% <ø> (-0.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rc/rmf_fleet_adapter/jobs/detail/impl_Planning.hpp 35.29% <0.00%> (-7.57%) ⬇️
...mf_rxcpp/include/rmf_rxcpp/detail/RxJobsDetail.hpp 50.00% <0.00%> (-4.17%) ⬇️
..._adapter/src/rmf_fleet_adapter/BroadcastClient.cpp 35.64% <0.00%> (-3.90%) ⬇️
...eet_adapter/src/rmf_fleet_adapter/tasks/Patrol.cpp 8.64% <0.00%> (-2.65%) ⬇️
...ter/services/detail/impl_FindEmergencyPullover.hpp 50.84% <0.00%> (-1.70%) ⬇️
...er/src/rmf_fleet_adapter/events/ResponsiveWait.cpp 39.20% <0.00%> (-1.60%) ⬇️
...er/src/rmf_fleet_adapter/agv/RobotUpdateHandle.cpp 3.40% <0.00%> (-1.20%) ⬇️
...apter/src/rmf_fleet_adapter/phases/RequestLift.cpp 33.52% <0.00%> (-1.16%) ⬇️
...adapter/src/rmf_fleet_adapter/phases/MoveRobot.hpp 32.43% <0.00%> (-0.91%) ⬇️
...f_fleet_adapter/agv/internal_RobotUpdateHandle.hpp 22.22% <0.00%> (-0.86%) ⬇️
... and 153 more

Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
@codebot
Copy link
Contributor

codebot commented May 20, 2022

Nice! I like the big picture. Just a few minor comments:

Since there is only a single ObstacleData submessage in the Obstacle message, and since it's small, I'd suggest just putting the two ObstacleData fields inside Obstacle and deleting the ObstacleData submessage, just to keep it a bit simpler. Changing to a different obstacle representation would break all the code anyway 🤷

I'd suggest to keep the resolution field of the OctoMap defined as a user-settable field, rather than a constant 0.1m, since this code may eventually end up being used at different scales (for example, self-driving vehicles as well as small delivery robots).

I'm not sure of the tradeoffs in having OctoMaps with "large" coordinates, but in general I'd suggest to use the frame_id in the Obstacle.msg Header submessage to indicate the frame of the OctoMap. If we want to use global coordinates in the OctoMap, the publisher could set that frame_id to rmf or world or whatever convention we adopt. If the publisher wanted to define OctoMaps with "small numbers" in its data structure, the publisher could set the header.frame_id to security_camera_1 or whatever, and then we'd need to use TF to publish the frame of security_camera_1 with respect to the global rmf frame. But in general I think using the Header.frame_id convention is a good idea.

Signed-off-by: Yadunund <yadunund@openrobotics.org>
@Yadunund
Copy link
Member Author

Thanks for the great feedback @codebot . I've moved the contents of ObstacleData into Obstacle and added a note indicating that measurements should be wrt header.frame_id.

@Yadunund Yadunund merged commit ff7b68a into main May 24, 2022
@Yadunund Yadunund deleted the feature/rmf_obstacle branch May 24, 2022 07:30
@Jerrybaoyilei
Copy link

Hi @Yadunund , I wonder if this package, along with other packages in rmf_internal_msgs package, will have a Python version? For example, I was using sensor_msgs to write an extra publisher and subscriber pair for running rmf_obstacle_detector_laserscan package, and I could write that in Python because sensor_msgs has a Python version that came installed with ROS 2 Iron. Thank you!

@mxgrey
Copy link
Contributor

mxgrey commented May 7, 2024

All ROS message packages will provide Python bindings if you have the message package in your colcon workspace or if you download the message package from the build farm.

If you're having issues using these messages from Python, you can open a discussion to describe, in as much detail as possible, what you've tried doing and what errors you encountered. Comments on a Pull Request like this should be limited to concerns about the pull request itself.

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.

None yet

4 participants