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

Do not overwrite the error code in planWithSinglePipeline #2723

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

galou
Copy link
Contributor

@galou galou commented Mar 4, 2024

Return the MotionPlanResponse as-is, also in case of error.

Description

The error code was forced-set to FAILURE in case of error, preventing the caller to get the correct information in case of error.

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

@galou
Copy link
Contributor Author

galou commented Mar 4, 2024

Should I write a note about this change in MIGRATION.md?

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

I think it might be possible that generatePlan returns false without properly setting the error_code. What do you think about only overwritting the error code if it is SUCCESS or UNDEFINED? That way nothing is overwritten + we're not ignoring an error

@galou
Copy link
Contributor Author

galou commented Mar 5, 2024

I did the changes to consider the case the generatePlan() return false but do not set the error code

This appears to me to be an API design issue since the failure-or-success piece of information must be set at two places.

Gaël Écorchard added 2 commits March 5, 2024 07:07
Return the `MotionPlanResponse` as-is.

Signed-off-by: Gaël Écorchard <gael@km-robotics.cz>
Do not rely on generatePlan() to set the error code in all cases and
ensure that the error code is set to FAILURE if `generatePlan()` returns
false.

Signed-off-by: Gaël Écorchard <gael@km-robotics.cz>
@galou galou force-pushed the do_not_overwrite_error_code branch from 34ef68d to 92ae93e Compare March 5, 2024 06:07
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 42.99%. Comparing base (d962501) to head (92ae93e).
Report is 15 commits behind head on main.

Files Patch % Lines
...ne_interfaces/src/planning_pipeline_interfaces.cpp 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2723      +/-   ##
==========================================
- Coverage   50.74%   42.99%   -7.75%     
==========================================
  Files         392      692     +300     
  Lines       32553    56325   +23772     
  Branches        0     7273    +7273     
==========================================
+ Hits        16517    24211    +7694     
- Misses      16036    31951   +15915     
- Partials        0      163     +163     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjahr
Copy link
Contributor

sjahr commented Mar 5, 2024

I did the changes to consider the case the generatePlan() return false but do not set the error code

This appears to me to be an API design issue since the failure-or-success piece of information must be set at two places.

Thank you! You're right, maybe this could be updated to return the response in the future to avoid duplicate error codes

@sjahr sjahr merged commit 680b783 into moveit:main Mar 5, 2024
11 of 12 checks passed
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.

2 participants