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

PlanningComponent: Load plan_request_params and use it when calling p… #2033

Merged
merged 3 commits into from
May 26, 2020

Conversation

JafarAbdi
Copy link
Contributor

This PR do the following:

1- Load the plan_request_params parameters and use them as default when calling PlanningComponent::plan() if a parameter is not loaded it uses the previous default value
2- Constructs MoveItCpp's node_handle_ member variable with the input nh
3- It uses delegating constructor when no MoveItCpp object is passed to PlanningComponent

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

@mamoll mamoll self-requested a review April 25, 2020 16:55
Copy link
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

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

Looks good to me!

nh.param(ns + "planning_attempts", planning_time, 5.0);
nh.param(ns + "max_velocity_scaling_factor", max_velocity_scaling_factor, 1.0);
nh.param(ns + "max_acceleration_scaling_factor", max_acceleration_scaling_factor, 1.0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about error checking in this function? The param method will return false on failure but there is no checking for that here. The default value is then utilized but wouldn't it be better to inform user that the parameter values they set are not being used? For example, would it be possible to typo the max_velocity_scaling_factor, default to 1.0 and have the arm move really fast?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think error checking is required because of the default values. You could say that overriding these values is the optional behavior, so I don't think we need extensive user infos here. Sanity checks and warnings are issued by the planning pipeline in case something is wrong. In general, moveit_cpp is currently not meant to provide a fool-proof user interface, but I would agree that we could add more debugging output about configuration and checks.

@codecov-commenter
Copy link

Codecov Report

Merging #2033 into master will increase coverage by 3.28%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
+ Coverage   54.09%   57.38%   +3.28%     
==========================================
  Files         319      328       +9     
  Lines       24997    25666     +669     
==========================================
+ Hits        13523    14728    +1205     
+ Misses      11474    10938     -536     
Impacted Files Coverage Δ
...ng_interface/moveit_cpp/src/planning_component.cpp 48.76% <80.00%> (-1.24%) ⬇️
...cpp/include/moveit/moveit_cpp/planning_component.h 100.00% <100.00%> (ø)
...s/planning_interface/moveit_cpp/src/moveit_cpp.cpp 66.66% <100.00%> (ø)
...ls/include/moveit/py_bindings_tools/gil_releaser.h 0.00% <0.00%> (-100.00%) ⬇️
.../include/moveit/py_bindings_tools/py_conversions.h 0.00% <0.00%> (-86.67%) ⬇️
...s/include/moveit/py_bindings_tools/serialize_msg.h 0.00% <0.00%> (-80.00%) ⬇️
...ove_group_interface/src/wrap_python_move_group.cpp 31.96% <0.00%> (-11.08%) ⬇️
...rimental/moveit_jog_arm/src/jog_interface_base.cpp 73.01% <0.00%> (-5.62%) ⬇️
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp 72.97% <0.00%> (-2.03%) ⬇️
...t_setup_assistant/src/tools/moveit_config_data.cpp 20.27% <0.00%> (-0.64%) ⬇️
... and 63 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 9a9f779...b33af17. Read the comment docs.

@rhaschke rhaschke merged commit 3f3ef1f into moveit:master May 26, 2020
pradeepr-roboticist pushed a commit to pradeepr-roboticist/moveit that referenced this pull request Jun 3, 2020
Load plan_request_params and use them when calling plan(void).
v4hn pushed a commit to v4hn/moveit that referenced this pull request Jul 3, 2020
Load plan_request_params and use them when calling plan(void).

(cherry picked from commit 3f3ef1f)
v4hn pushed a commit to v4hn/moveit that referenced this pull request Jul 3, 2020
Load plan_request_params and use them when calling plan(void).

(cherry picked from commit 3f3ef1f)
@v4hn v4hn mentioned this pull request Jul 4, 2020
@tylerjw tylerjw mentioned this pull request Jul 18, 2020
20 tasks
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

6 participants