Skip to content

[moveit_cpp] Fix return value of execute#1144

Closed
galou wants to merge 1 commit intomoveit:mainfrom
galou:moveitcpp_fix_execute_return
Closed

[moveit_cpp] Fix return value of execute#1144
galou wants to merge 1 commit intomoveit:mainfrom
galou:moveitcpp_fix_execute_return

Conversation

@galou
Copy link
Contributor

@galou galou commented Mar 28, 2022

Previously, MoveItCpp::execute() would return true only if
waitForExecution() would return UNKNOWN.
This sort of break the API because users that did use the return value
of execute had to use an inverted logic.
These users will quickly notice the change.

Signed-off-by: Gaël Écorchard gael.ecorchard@cvut.cz

Description

Previously, MoveItCpp::execute() would return true only if
waitForExecution() would return UNKNOWN.
This sort of break the API because users that did use the return value
of execute had to use an inverted logic.
These users will quickly notice the change.

The returned value is not documented in the API documentation.

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

Previously, `MoveItCpp::execute()` would return true only if
`waitForExecution()` would return `UNKNOWN`.
This sort of break the API because users that did use the return value
of `execute` had to use an inverted logic.
These users will quickly notice the change.

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
@galou
Copy link
Contributor Author

galou commented Mar 28, 2022

I actually think that it would be better to return a moveit_controller_manager::ExecutionStatus::Value to better inform the caller, i.e. trajectory_execution_manager_->waitForExecution() when blocking and moveit_controller_manager::ExecutionStatus::SUCCESS otherwise. This would break API compatibility.

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #1144 (c6a4431) into main (a772d8e) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
- Coverage   61.47%   61.45%   -0.01%     
==========================================
  Files         275      275              
  Lines       23802    23802              
==========================================
- Hits        14629    14626       -3     
- Misses       9173     9176       +3     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 74.82% <0.00%> (-1.11%) ⬇️

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 a772d8e...c6a4431. Read the comment docs.

@v4hn
Copy link
Contributor

v4hn commented Mar 28, 2022

  1. I agree it makes sense to return an ExecutionStatus instead of the boolean. Why would you return Value directly? ExecutionStatus has a bool() operator, so that would not even break API.

But that brings me to my second question 2. ExecutionStatus::operator bool() performs the same comparison that you propose to add here? Why does that not already work the way you want it to?
I see an API that could be improved, but I do not yet see the bug you describe. 😕

@galou
Copy link
Contributor Author

galou commented Mar 28, 2022

Oh, I didn't notice that ExecutionStatus has a bool() operator, so in this case, of course, it's better to return it rather than its Value.
It's possible that I made a mistake when testing my code, sorry if it's the case (I'll double-check). Could it be that the bool() operator will only work in combination with static_cast: return static_cast<bool>(trajectory_execution_manager_->waitForExecution()); in MoveItCpp::execute?

@galou
Copy link
Contributor Author

galou commented Mar 28, 2022

I just double-checked. I don't know what happened when I tested it but the result is what is actually expected. Sorry about it. A static_cast would be more explicit but the result would not change.
I close the PR.

@galou galou closed this Mar 28, 2022
@v4hn
Copy link
Contributor

v4hn commented Mar 28, 2022 via email

@galou galou deleted the moveitcpp_fix_execute_return branch March 29, 2022 07:26
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