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

Fix parameter name & Fix exception about parameter type #461

Merged
merged 2 commits into from
May 13, 2021

Conversation

JafarAbdi
Copy link
Contributor

Description

Previously the following parameters were not being loaded at all projection_evaluator longest_valid_segment_fraction enforce_joint_model_state_space enforce_constrained_state_space

This PR
1- Fix the parameter prefix
2- Fix the exception being throw by calling node_->get_parameter(param_name, ...) due to types mismatch

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

Copy link
Contributor

@vatanaksoytezer vatanaksoytezer left a comment

Choose a reason for hiding this comment

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

I've read the code and left a small question, otherwise looks good to me!

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #461 (8e831c2) into main (3a41a20) will decrease coverage by 0.58%.
The diff coverage is 85.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #461      +/-   ##
==========================================
- Coverage   52.01%   51.43%   -0.57%     
==========================================
  Files         223      223              
  Lines       23340    23339       -1     
==========================================
- Hits        12137    12002     -135     
- Misses      11203    11337     +134     
Impacted Files Coverage Δ
...lanners/ompl/ompl_interface/src/ompl_interface.cpp 76.93% <85.72%> (+26.45%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 0.00% <0.00%> (-83.01%) ⬇️
...rameterization/work_space/pose_model_state_space.h 0.00% <0.00%> (-73.68%) ⬇️
.../ompl_interface/src/detail/constrained_sampler.cpp 0.00% <0.00%> (-59.45%) ⬇️
...tion/work_space/pose_model_state_space_factory.cpp 41.67% <0.00%> (-33.33%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 57.74% <0.00%> (-14.33%) ⬇️
...mpl_interface/src/model_based_planning_context.cpp 60.65% <0.00%> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp 56.24% <0.00%> (+0.13%) ⬆️
...e/collision_detection_fcl/src/collision_common.cpp 75.19% <0.00%> (+0.50%) ⬆️
moveit_core/robot_model/src/joint_model_group.cpp 54.62% <0.00%> (+0.53%) ⬆️
... and 9 more

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 b5ad6b0...8e831c2. Read the comment docs.

@AndyZe
Copy link
Member

AndyZe commented May 11, 2021

Is a good way to test this to start one of the demos and check that longest_valid_segment_fraction (for example) is loaded?

@AndyZe
Copy link
Member

AndyZe commented May 11, 2021

With this PR I ran the moveit_cpp demo then listed parameters like this:

ros2 launch run_moveit_cpp run_moveit_cpp.launch.py

ros2 param list | longest_valid

So it seems like the parameters weren't loaded? Maybe they are not supposed to be loaded for this particular demo?

@JafarAbdi
Copy link
Contributor Author

With this PR I ran the moveit_cpp demo then listed parameters like this:

ros2 launch run_moveit_cpp run_moveit_cpp.launch.py

ros2 param list | longest_valid

So it seems like the parameters weren't loaded? Maybe they are not supposed to be loaded for this particular demo?

We use this ompl_planning.yaml file for loading the OMPL configs, could you try updating it to have one of these parameters

@AndyZe
Copy link
Member

AndyZe commented May 12, 2021

@JafarAbdi I went ahead and added those parameters. It looks good EXCEPT enforce_constrained_state_space is not a bool but it was read anyway.

  RRTConnectkConfigDefault:
    type: geometric::RRTConnect
    range: 0.0  # Max motion added to tree. ==> maxDistance_ default: 0.0, if 0.0, set on setup()
    longest_valid_segment_fraction: 0.1
    projection_evaluator: test
    enforce_joint_model_state_space: true
    enforce_constrained_state_space: 1.3  ==> Should be a boolean

ros2 param get /run_moveit_cpp ompl.planner_configs.RRTConnectkConfigDefault.enforce_constrained_state_space gives
Double value is: 1.3

So I guess something is not quite working.

@JafarAbdi
Copy link
Contributor Author

@JafarAbdi I went ahead and added those parameters. It looks good EXCEPT enforce_constrained_state_space is not a bool but it was read anyway.

  RRTConnectkConfigDefault:
    type: geometric::RRTConnect
    range: 0.0  # Max motion added to tree. ==> maxDistance_ default: 0.0, if 0.0, set on setup()
    longest_valid_segment_fraction: 0.1
    projection_evaluator: test
    enforce_joint_model_state_space: true
    enforce_constrained_state_space: 1.3  ==> Should be a boolean

ros2 param get /run_moveit_cpp ompl.planner_configs.RRTConnectkConfigDefault.enforce_constrained_state_space gives
Double value is: 1.3

So I guess something is not quite working.

Currently, if there's a type mismatch it will only print an error, did you get any error about that .?

Also, these parameters have to be loaded under the group namespace similar to planner_configs

@AndyZe
Copy link
Member

AndyZe commented May 12, 2021

OK, that looks good. (Consider putting test instructions in the PR description next time)

@AndyZe
Copy link
Member

AndyZe commented May 12, 2021

I'll leave it to you to squash and merge

@JafarAbdi JafarAbdi merged commit 289437b into moveit:main May 13, 2021
@JafarAbdi JafarAbdi deleted the pr-ompl_param_fix branch May 13, 2021 12:00
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* fixed list in tutorial

* added controller tutorial

* fixed formatting

* fixed title formatting

* fixed explicit reference

* removed placeholder text

* formatting changes and added drawio link

* Update doc/tutorials/controller_teleoperation/controller_teleoperation.rst

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update doc/tutorials/controller_teleoperation/controller_teleoperation.rst

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update doc/tutorials/controller_teleoperation/controller_teleoperation.rst

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update doc/tutorials/controller_teleoperation/controller_teleoperation.rst

Co-authored-by: AndyZe <andyz@utexas.edu>

* controller -> gamepad

* moved to how-to guides

* changed prerequisites section and changed one final occurrence of "controller"

* updated title for clarity

* modified TOC tree

Co-authored-by: AndyZe <andyz@utexas.edu>
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