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

load sensors_3d.yaml #1387

Merged
merged 6 commits into from
Jun 28, 2022
Merged

Conversation

mikeferguson
Copy link
Contributor

@mikeferguson mikeferguson commented Jun 23, 2022

Description

this implements #1297 - also requires some changes to the MSA, which will be a separate PR to that feature branch. The changes are namely:

  • "sensors" is expected to be a vector of strings (sensor names)
  • the sensor config then needs to be namespaced under the sensor name provided
  • all of the distance/padding parameters need to be floating point - not integer or string.

Example of changes to get a working configuration: mikeferguson/ubr_reloaded@45b9e52

Screenshot from 2022-06-22 23-07-45

Please explain the changes you made, including a reference to the related issue if applicable

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #1387 (841dba5) into main (3a51feb) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1387      +/-   ##
==========================================
+ Coverage   61.55%   61.57%   +0.03%     
==========================================
  Files         274      274              
  Lines       24977    24977              
==========================================
+ Hits        15371    15378       +7     
+ Misses       9606     9599       -7     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.73% <0.00%> (+0.44%) ⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 76.43% <0.00%> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a51feb...841dba5. Read the comment docs.

@vatanaksoytezer
Copy link
Contributor

@mikeferguson thank you so much for the PR! I'm unsure why the CI is failing, the failure seems unrelated to this PR, I'll look into it. Pasting the error message just for reference:

2022-06-23T03:31:36.4720982Z 3: Test command: /usr/bin/python3.10 "-u" "/opt/ros/humble/share/ament_cmake_test/cmake/run_test.py" "/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test_results/moveit_kinematics/test_launch_fanuc-kdl-singular.test.py.xunit.xml" "--package-name" "moveit_kinematics" "--output-file" "/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/ros_test/test_launch_fanuc-kdl-singular.test.py.txt" "--command" "ros2" "test" "/home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_kinematics/test/launch/fanuc-kdl-singular.test.py" "test_binary_dir:=/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test" "--junit-xml=/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test_results/moveit_kinematics/test_launch_fanuc-kdl-singular.test.py.xunit.xml" "--package-name=moveit_kinematics"
2022-06-23T03:31:36.4722302Z 3: Test timeout computed to be: 60
2022-06-23T03:31:36.4722835Z 3: -- run_test.py: invoking following command in '/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test':
2022-06-23T03:31:36.4724266Z 3:  - ros2 test /home/runner/work/moveit2/moveit2/.work/target_ws/src/moveit2/moveit_kinematics/test/launch/fanuc-kdl-singular.test.py test_binary_dir:=/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test --junit-xml=/home/runner/work/moveit2/moveit2/.work/target_ws/build/moveit_kinematics/test_results/moveit_kinematics/test_launch_fanuc-kdl-singular.test.py.xunit.xml --package-name=moveit_kinematics
2022-06-23T03:31:36.4725027Z 3: Running with ROS_DOMAIN_ID 1
2022-06-23T03:31:36.4725266Z 3: ROS_DOMAIN_ID 1
2022-06-23T03:31:36.4725498Z 3: Failed to handle type {}

@mikeferguson
Copy link
Contributor Author

@vatanaksoytezer don't worry about the build failure quite yet - at least part of it is this PR - the various robot configs in moveit_resources are all missing the sensors_3d.yaml (so the first time around it failed due to missing file - I added a file.exists - but now I see that causes a parsing issue later since it ends up putting an empty dictionary into some of the tests - I'll work on resolving that today)

@vatanaksoytezer
Copy link
Contributor

@mikeferguson sounds good, thank you for looking at this!

)
if sensors_path.exists():
sensors_data = load_yaml(sensors_path)
if len(sensors_data["sensors"]) > 0 and sensors_data["sensors"][0]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the second part of this (that the first sensor is non-empty) can be removed once moveit/moveit_resources#141 is released

@mikeferguson
Copy link
Contributor Author

@vatanaksoytezer @DLu this is now ready for a review (tests are passing, I'd suggest we merge with the extra logic and I'll come back and remove that extra checking in a month or so once the moveit_resources stuff percolates through the build farm)

@vatanaksoytezer
Copy link
Contributor

I just released moveit_resources for rolling and humble with David's fixes: ros/rosdistro#33703 and ros/rosdistro#33702. So we are just waiting them to get merged and synced. @mikeferguson the PR looks good to me, I'm happy to approve and merge if you don't mind adding a TODO and open up an issue to remove it so we don't forget. Thank you very much!

this implements moveit#1297 - also requires some changes
to the MSA, which will be a separate PR to that feature
branch
this leads to an error of Failed to handle type {}
@mikeferguson
Copy link
Contributor Author

@vatanaksoytezer TODO has been added (it should only take a few weeks for a sync to happen and then we can remove the code). This has been rebased after the massive feature/msa merge - if you have time to do a review would like to get this in.

@vatanaksoytezer vatanaksoytezer merged commit d5af44b into moveit:main Jun 28, 2022
@mikeferguson mikeferguson deleted the load_sensors3d branch June 28, 2022 11:28
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
130s added a commit to 130s/moveit_resources that referenced this pull request Jun 22, 2023
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.

3 participants