Skip to content

Use a steady clock for timeout for IK#793

Closed
galou wants to merge 2 commits intomoveit:foxyfrom
galou:fix_timeout
Closed

Use a steady clock for timeout for IK#793
galou wants to merge 2 commits intomoveit:foxyfrom
galou:fix_timeout

Conversation

@galou
Copy link
Copy Markdown
Contributor

@galou galou commented Nov 11, 2021

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

Description

The searchPositionIK can hang forever when using a simulated time because the time is stuck while waiting for this function to return. For this simple reason the function never times out. This was reported by @jmarsik, I'll let him write his full report here. Most of the work, i.e. isolating the cause, was done by him.

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

I don't know how to write a test for this. It doesn't appear to be worth it.

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

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

@galou thanks for your PR! I agree, it makes perfect sense to use steady time here. I bet there are other cases in the codebase where we should fix this as well. Could you target main with your PR? We can backport it to foxy.

bool KDLKinematicsPlugin::timedOut(const rclcpp::Time& start_time, double duration) const
{
return ((node_->now() - start_time).seconds() >= duration);
static rclcpp::Clock clock{ RCL_STEADY_TIME };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you make clock a class member steady_clock_?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK for both but I cannot compile main so this will be a blind PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

main only builds for galactic or rolling, I presume that is why it does not build for you. If it is something else please let us know and we are happy to help. Again, thank you for this change.

@henningkayser henningkayser self-assigned this Nov 11, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 11, 2021

Codecov Report

Merging #793 (113bb37) into foxy (fd9a562) will decrease coverage by 0.01%.
The diff coverage is 83.34%.

❗ Current head 113bb37 differs from pull request most recent head 116195a. Consider uploading reports for the commit 116195a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             foxy     #793      +/-   ##
==========================================
- Coverage   46.81%   46.80%   -0.00%     
==========================================
  Files         185      185              
  Lines       19693    19695       +2     
==========================================
- Hits         9217     9216       -1     
- Misses      10476    10479       +3     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 74.91% <83.34%> (-0.94%) ⬇️

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 fd9a562...116195a. Read the comment docs.

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
@galou galou closed this Nov 11, 2021
@galou galou deleted the fix_timeout branch November 11, 2021 16:18
@galou galou mentioned this pull request Nov 11, 2021
6 tasks
@jmarsik
Copy link
Copy Markdown

jmarsik commented Nov 11, 2021

I have posted my report in the new PR ... see here #795 (comment)

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.

4 participants